From owner-svn-src-head@FreeBSD.ORG Tue Nov 27 22:48:50 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 83101E7D; Tue, 27 Nov 2012 22:48:50 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 361298FC14; Tue, 27 Nov 2012 22:48:50 +0000 (UTC) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id C37CB46B17; Tue, 27 Nov 2012 17:48:49 -0500 (EST) Date: Tue, 27 Nov 2012 22:48:49 +0000 (GMT) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Andre Oppermann Subject: Re: svn commit: r243627 - head/sys/kern In-Reply-To: <50B54180.5020608@freebsd.org> Message-ID: References: <201211272004.qARK4qS8047209@svn.freebsd.org> <50B54180.5020608@freebsd.org> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Peter Wemm X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Nov 2012 22:48:50 -0000 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 >> 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 >>> 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); >>> } >> >> >> > >