Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 27 Nov 2012 22:48:49 +0000 (GMT)
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Andre Oppermann <andre@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Wemm <peter@wemm.org>
Subject:   Re: svn commit: r243627 - head/sys/kern
Message-ID:  <alpine.BSF.2.00.1211272246560.37292@fledge.watson.org>
In-Reply-To: <50B54180.5020608@freebsd.org>
References:  <201211272004.qARK4qS8047209@svn.freebsd.org> <CAGE5yCpxOdsjefe6quR_gjs82pk9a2e_H_WUNUWhUGA3WZPJaw@mail.gmail.com> <50B54180.5020608@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help

On Tue, 27 Nov 2012, Andre Oppermann wrote:

> On 27.11.2012 23:35, Peter Wemm wrote:
>> Andre.. this breaks incoming connections.  TCP is immediately reset and 
>> never even gets to the listener process.  You need to back out of fix this 
>> urgently please.
>
> I just found out and fixed it.  Sorry for the breakage.

I'd like to see a much more thorough use of "Reviewed by:" in socket and 
TCP-related commits -- this is very sensitive code, and a second pair of eyes 
is always valuable.  Post-commit review is not a substitute.  Looking back 
over similar changes in the socket code over the last two years, I see that 
almost all have reviewers, so I think it would be reasonable to consider it 
mandatory for these subsystems at this point.  The good news is that we have 
lots of people with expertise in it.

Robert

>
> -- 
> Andre
>
>> On Tue, Nov 27, 2012 at 12:04 PM, Andre Oppermann <andre@freebsd.org> 
>> wrote:
>>> Author: andre
>>> Date: Tue Nov 27 20:04:52 2012
>>> New Revision: 243627
>>> URL: http://svnweb.freebsd.org/changeset/base/243627
>>> 
>>> Log:
>>>    Fix a race on listen socket teardown where while draining the
>>>    accept queues a new socket/connection may be added to the queue
>>>    due to a race on the ACCEPT_LOCK.
>>>
>>>    The submitted patch is slightly changed in comments, teardown
>>>    and locking order and extended with KASSERT's.
>>>
>>>    Submitted by: Vijay Singh <vijju.singh-at-gmail-dot-com>
>>>    Found by:     His team.
>>>    MFC after:    1 week
>>> 
>>> Modified:
>>>    head/sys/kern/uipc_socket.c
>>> 
>>> Modified: head/sys/kern/uipc_socket.c
>>> ==============================================================================
>>> --- head/sys/kern/uipc_socket.c Tue Nov 27 19:35:21 2012        (r243626)
>>> +++ head/sys/kern/uipc_socket.c Tue Nov 27 20:04:52 2012        (r243627)
>>> @@ -555,6 +555,16 @@ sonewconn(struct socket *head, int conns
>>>          so->so_snd.sb_flags |= head->so_snd.sb_flags & SB_AUTOSIZE;
>>>          so->so_state |= connstatus;
>>>          ACCEPT_LOCK();
>>> +       /*
>>> +        * The accept socket may be tearing down but we just
>>> +        * won a race on the ACCEPT_LOCK.
>>> +        */
>>> +       if (!(so->so_options & SO_ACCEPTCONN)) {
>>> +               SOCK_LOCK(so);
>>> +               so->so_head = NULL;
>>> +               sofree(so);             /* NB: returns ACCEPT_UNLOCK'ed. 
>>> */
>>> +               return (NULL);
>>> +       }
>>>          if (connstatus) {
>>>                  TAILQ_INSERT_TAIL(&head->so_comp, so, so_list);
>>>                  so->so_qstate |= SQ_COMP;
>>> @@ -780,9 +790,14 @@ soclose(struct socket *so)
>>>   drop:
>>>          if (so->so_proto->pr_usrreqs->pru_close != NULL)
>>>                  (*so->so_proto->pr_usrreqs->pru_close)(so);
>>> +       ACCEPT_LOCK();
>>>          if (so->so_options & SO_ACCEPTCONN) {
>>>                  struct socket *sp;
>>> -               ACCEPT_LOCK();
>>> +               /*
>>> +                * Prevent new additions to the accept queues due
>>> +                * to ACCEPT_LOCK races while we are draining them.
>>> +                */
>>> +               so->so_options &= ~SO_ACCEPTCONN;
>>>                  while ((sp = TAILQ_FIRST(&so->so_incomp)) != NULL) {
>>>                          TAILQ_REMOVE(&so->so_incomp, sp, so_list);
>>>                          so->so_incqlen--;
>>> @@ -801,13 +816,15 @@ drop:
>>>                          soabort(sp);
>>>                          ACCEPT_LOCK();
>>>                  }
>>> -               ACCEPT_UNLOCK();
>>> +               KASSERT((TAILQ_EMPTY(&so->so_comp)),
>>> +                   ("%s: so_comp populated", __func__));
>>> +               KASSERT((TAILQ_EMPTY(&so->so_incomp)),
>>> +                   ("%s: so_incomp populated", __func__));
>>>          }
>>> -       ACCEPT_LOCK();
>>>          SOCK_LOCK(so);
>>>          KASSERT((so->so_state & SS_NOFDREF) == 0, ("soclose: NOFDREF"));
>>>          so->so_state |= SS_NOFDREF;
>>> -       sorele(so);
>>> +       sorele(so);                     /* NB: Returns with 
>>> ACCEPT_UNLOCK(). */
>>>          CURVNET_RESTORE();
>>>          return (error);
>>>   }
>> 
>> 
>> 
>
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.1211272246560.37292>