svn commit: r243627 - head/sys/kern
Robert Watson
rwatson at FreeBSD.org
Tue Nov 27 22:48:50 UTC 2012
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 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);
>>> }
>>
>>
>>
>
>
More information about the svn-src-head
mailing list