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>