Date: Fri, 5 Apr 2013 11:20:09 -0400 From: Juan Mojica <jmojica@gmail.com> To: Matt Miller <matt@matthewjmiller.net> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Rick Macklem <rmacklem@uoguelph.ca> Subject: Re: panic in tcp_do_segment() Message-ID: <CAPKuH-xJ6XB74UBi_f8g%2B8n1CO1fNP2yG_VbarumdGn0--DSpQ@mail.gmail.com> In-Reply-To: <CAFc6gu9bmJQ-Uxv7PzspSLDyFz6zE3fV=u7HEumCh6f=YQLU2Q@mail.gmail.com> References: <1043692819.529554.1365114790772.JavaMail.root@erie.cs.uoguelph.ca> <CAFc6gu9bmJQ-Uxv7PzspSLDyFz6zE3fV=u7HEumCh6f=YQLU2Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Agree with Matt.
Whenever there is an UNLOCK/LOCK like is present in soclose(), there is a
window to allow something through. Unsetting SO_ACCEPTCONN was put in
place because the LOCK/UNLOCK in soclose let a new socket to be added to
the so_incomp list causing a different ASSERT to be hit - and memory to be
leaked.
As far as I can tell, because of the upcall performed in soabort(), we
can't just hold the ACCEPT lock all the way through the close.
The simplest thing to do in this case seemed to be to just drop the TCP
segment - the connection is being closed anyway. Or like Matt said, having
someone look at the lock logic and see if there is something there that can
be exploited to prevent this would also help.
-Juan
On Fri, Apr 5, 2013 at 7:09 AM, Matt Miller <matt@matthewjmiller.net> 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.
>
> 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
>
>
>
>
> On Thu, Apr 4, 2013 at 6:33 PM, Rick Macklem <rmacklem@uoguelph.ca> wrote:
>
>> Hi,
>>
>> When pho@ was doing some NFS testing, he got the
>> following crash, which I can't figure out. (As far
>> as I can see, INP_WLOCK() is always held when
>> tp->t_state = TCPS_CLOSED and it is held from before
>> the test for TCPS_CLOSED in tcp_input() up until
>> the tcp_do_segment() call. As such, I don't see how
>> tp->t_state can be TCPS_CLOSED, but that seems to
>> be what causes the panic?)
>>
>> The "umount -f" will result in:
>> soshutdown(so, SHUT_WR);
>> soclose(so);
>> being done by the krpc on the socket.
>>
>> Anyone have any ideas on this?
>> pho@ wrote:
>> > I continued running the NFS tests and got this "panic: tcp_do_segment:
>> > TCPS_LISTEN". It's the second time I get this panic with the same test
>> > scenario, so it seems to be reproducible. The scenario is "umount -f"
>> > of a mount point that is very active.
>> >
>> > http://people.freebsd.org/~pho/stress/log/kostik555.txt
>>
>> Thanks in advance for any help, rick
>> _______________________________________________
>> freebsd-net@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-net
>> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
>>
>
>
--
Juan Mojica
Email: jmojica@gmail.com
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPKuH-xJ6XB74UBi_f8g%2B8n1CO1fNP2yG_VbarumdGn0--DSpQ>