svn commit: r243627 - head/sys/kern

[Peter Wemm]

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.

On Tue, Nov 27, 2012 at 12:04 PM, Andre Oppermann <andre at 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);
>  }



-- 
Peter Wemm - peter at wemm.org; peter at FreeBSD.org; peter at yahoo-inc.com; KI6FJV
"All of this is for nothing if we don't go to the stars" - JMS/B5
"If Java had true garbage collection, most programs would delete
themselves upon execution." -- Robert Sewell