Date: Fri, 12 Apr 2013 17:43:31 +0200 From: Peter Holm <peter@holm.cc> To: Juan Mojica <jmojica@gmail.com> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Rick Macklem <rmacklem@uoguelph.ca>, Andre Oppermann <andre@freebsd.org>, Matt Miller <matt@matthewjmiller.net> Subject: Re: panic in tcp_do_segment() Message-ID: <20130412154331.GA46423@x2.osted.lan> In-Reply-To: <CAPKuH-zz_tARNUNs%2BHBsE3XVLk_-zvKxG8F2dG%2BUBeoRinb4XQ@mail.gmail.com> References: <1043692819.529554.1365114790772.JavaMail.root@erie.cs.uoguelph.ca> <CAFc6gu9bmJQ-Uxv7PzspSLDyFz6zE3fV=u7HEumCh6f=YQLU2Q@mail.gmail.com> <5162B474.6060808@freebsd.org> <20130409081652.GA50498@x2.osted.lan> <5163D2D2.2090407@freebsd.org> <20130409184439.GA62925@x2.osted.lan> <CAPKuH-zz_tARNUNs%2BHBsE3XVLk_-zvKxG8F2dG%2BUBeoRinb4XQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Apr 12, 2013 at 10:14:40AM -0400, Juan Mojica wrote:
Glad I could help.
- Peter
> I'm a little late to get back to the email thread, but this is great to
> hear. Changes look good (assuming the goto drop is changed
> dropunlock). Thanks guys.
>
> On Tue, Apr 9, 2013 at 2:44 PM, Peter Holm <[1]peter@holm.cc> wrote:
>
> On Tue, Apr 09, 2013 at 10:35:30AM +0200, Andre Oppermann wrote:
> > On 09.04.2013 10:16, Peter Holm wrote:
> > > On Mon, Apr 08, 2013 at 02:13:40PM +0200, Andre Oppermann wrote:
> > >> On 05.04.2013 13:09, Matt Miller wrote:
> > >>> Hey Rick,
> > >>>
> > >>> I believe Juan and I have root caused this crash recently. The
> t_state =
> > >>> 0x1, TCPS_LISTEN, in the link provided at the time of the
> assertion.
> > >>>
> > >>> In tcp_input(), if we're in TCPS_LISTEN, SO_ACCEPTCONN should be
> set on the
> > >>> socket and we should never enter tcp_do_segment() for this state.
> I think
> > >>> if you look in your corefile, you'll see the socket *doesn't*
> have this
> > >>> flag set in your case.
> > >>>
> > >>> 1043 /*
> > >>> 1044 * When the socket is accepting connections (the
> INPCB is in
> > >>> LISTEN
> > >>> 1045 * state) we look into the SYN cache if this is a
> new
> > >>> connection
> > >>> 1046 * attempt or the completion of a previous one.
> Because listen
> > >>> 1047 * sockets are never in TCPS_ESTABLISHED, the
> V_tcbinfo lock
> > >>> will be
> > >>> 1048 * held in this case.
> > >>> 1049 */
> > >>> 1050 if (so->so_options & SO_ACCEPTCONN) {
> > >>> 1051 struct in_conninfo inc;
> > >>> 1052
> > >>> 1053 KASSERT(tp->t_state == TCPS_LISTEN, ("%s: so
> accepting
> > >>> but "
> > >>> 1054 "tp not listening", __func__));
> > >>> ...
> > >>> 1356 syncache_add(&inc, &to, th, inp, &so, m,
> NULL, NULL);
> > >>> 1357 /*
> > >>> 1358 * Entry added to syncache and mbuf
> consumed.
> > >>> 1359 * Everything already unlocked by
> syncache_add().
> > >>> 1360 */
> > >>> 1361 INP_INFO_UNLOCK_ASSERT(&V_tcbinfo);
> > >>> 1362 return;
> > >>> 1363 }
> > >>> ...
> > >>> 1384 /*
> > >>> 1385 * Segment belongs to a connection in SYN_SENT,
> ESTABLISHED or
> > >>> later
> > >>> 1386 * state. tcp_do_segment() always consumes the mbuf
> chain,
> > >>> unlocks
> > >>> 1387 * the inpcb, and unlocks pcbinfo.
> > >>> 1388 */
> > >>> 1389 tcp_do_segment(m, th, so, tp, drop_hdrlen, tlen,
> iptos,
> > >>> ti_locked);
> > >>>
> > >>> I think this has to do with this patch in soclose() where
> SO_ACCEPTCONN is
> > >>> being turned off in soclose(). I suspect if you look at the
> other threads
> > >>> in your corefile, you'll see one at this point in soclose()
> operating on
> > >>> the same socket as the one in the tcp_do_segment() thread.
> > >>>
> > >>> [2]http://svnweb.freebsd.org/base?view=revision&revision=243627
> > >>>
> > >>> 817 /*
> > >>> 818 * Prevent new additions to the accept
> queues due
> > >>> 819 * to ACCEPT_LOCK races while we are
> draining them.
> > >>> 820 */
> > >>> 821 so->so_options &= ~SO_ACCEPTCONN;
> > >>> 822 while ((sp = TAILQ_FIRST(&so->so_incomp))
> != NULL) {
> > >>> 823 TAILQ_REMOVE(&so->so_incomp, sp,
> so_list);
> > >>> 824 so->so_incqlen--;
> > >>> 825 sp->so_qstate &= ~SQ_INCOMP;
> > >>> 826 sp->so_head = NULL;
> > >>> 827 ACCEPT_UNLOCK();
> > >>> 828 soabort(sp);
> > >>> 829 ACCEPT_LOCK();
> > >>> 830 }
> > >>>
> > >>> Juan had evaluated this code path and it seemed safe to just drop
> the
> > >>> packet in this case:
> > >>>
> > >>> + /*
> > >>> + * In closing down the socket, the SO_ACCEPTCONN flag is
> removed to
> > >>> + * prevent new connections from being established. This
> means that
> > >>> + * any frames in that were in the midst of being processed
> could
> > >>> + * make it here. Need to just drop the frame.
> > >>> + */
> > >>> + if (TCPS_LISTEN == tp->t_state) {
> > >>> + TCPSTAT_INC(tcps_rcvwhileclosing);
> > >>> + goto drop;
> > >>> + }
> > >>> KASSERT(tp->t_state > TCPS_LISTEN, ("%s: TCPS_LISTEN",
> > >>> __func__));
> > >>>
> > >>> Or, if there's someone more familiar with the locking in these
> paths, they
> > >>> may be able to come up with a way to restructure the locks and
> logic to
> > >>> close this window.
> > >>
> > >> Matt, Juan,
> > >>
> > >> excellent analysis. I don't see a better approach to handle this
> > >> under the current ACCEPT_LOCK model.
> > >>
> > >> Compared to your patch I'd like to handle this race earlier before
> > >> we hit tcp_do_segment().
> > >>
> > >> Could you please review the attached patch which handles it right
> > >> after the SO_ACCEPTCONN / syncache check?
> > >>
> > >> --
> > >> Andre
> > >>
> > >> Index: netinet/tcp_input.c
> > >>
> ===================================================================
> > >> --- netinet/tcp_input.c (revision 249253)
> > >> +++ netinet/tcp_input.c (working copy)
> > >> @@ -1351,6 +1351,16 @@
> > >> */
> > >> INP_INFO_UNLOCK_ASSERT(&V_tcbinfo);
> > >> return;
> > >> + } else if (tp->t_state == TCPS_LISTEN) {
> > >> + /*
> > >> + * When a listen socket is torn down the SO_ACCEPTCONN
> > >> + * flag is removed first while connections are drained
> > >> + * from the accept queue in a unlock/lock cycle of the
> > >> + * ACCEPT_LOCK, opening a race condition allowing a SYN
> > >> + * attempt go through unhandled.
> > >> + */
> > >> + TCPSTAT_INC(tcps_rcvdwhileclosing);
> > >> + goto drop;
> > >> }
> > >>
> > >> #ifdef TCP_SIGNATURE
> > >
> > > I was able to reproduce the original "panic: tcp_do_segment:
> > > TCPS_LISTEN" with ease; see
> > > [3]http://people.freebsd.org/~pho/stress/log/tcp.txt.
> > >
> > > With your patch (minus the TCPSTAT_INC) I got this "panic: Lock
> (rw)
> > > tcp locked @ netinet/tcp_input.c:1432."
> > >
> > > [4]http://people.freebsd.org/~pho/stress/log/tcp2.txt
> >
> > Please replace the 'goto drop' with 'goto dropunlock' to fix the
> panic.
> >
>
> Yes, this seems to take care of the two problems reported. Tested
> for
> 5 hours without any repeats.
> - Peter
>
> --
> Juan Mojica
> Email: [5]jmojica@gmail.com
>
> References
>
> 1. mailto:peter@holm.cc
> 2. http://svnweb.freebsd.org/base?view=revision&revision=243627
> 3. http://people.freebsd.org/~pho/stress/log/tcp.txt
> 4. http://people.freebsd.org/~pho/stress/log/tcp2.txt
> 5. mailto:jmojica@gmail.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130412154331.GA46423>