From owner-freebsd-current@FreeBSD.ORG Sun May 21 18:53:37 2006 Return-Path: X-Original-To: current@FreeBSD.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CD5BF16A42B for ; Sun, 21 May 2006 18:53:37 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [209.31.154.42]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2F1A843D48 for ; Sun, 21 May 2006 18:53:37 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [209.31.154.41]) by cyrus.watson.org (Postfix) with ESMTP id 707AF46C8D; Sun, 21 May 2006 14:53:36 -0400 (EDT) Date: Sun, 21 May 2006 19:53:36 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Maxim Konovalov In-Reply-To: <20060521221016.F6324@mp2.macomnet.net> Message-ID: <20060521195004.H8068@fledge.watson.org> References: <20060317141627.W2181@fledge.watson.org> <20060329100839.V19236@fledge.watson.org> <20060401102918.P79188@fledge.watson.org> <20060401170554.R82503@fledge.watson.org> <20060402233436.P76562@fledge.watson.org> <20060515025600.U70399@mp2.macomnet.net> <20060521185034.K8068@fledge.watson.org> <20060521215823.H6324@mp2.macomnet.net> <20060521190358.G8068@fledge.watson.org> <20060521221016.F6324@mp2.macomnet.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: current@FreeBSD.org Subject: Re: HEADS UP: socket and pcb reference changes entering tree today X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 21 May 2006 18:53:39 -0000 On Sun, 21 May 2006, Maxim Konovalov wrote: >>> I "copied" this code from udp_usrreq.c::udp_disconnect(). There is no >>> such lock. Is it a bug too? >> >> Yes. >> >> I have some intuitions about why the datagram protocols manually frob the >> disconnected flag rather than calling soisdisconnected(), but am generally >> unsure that this is the right thing. > > I thought about that but hadn't find any explanations. > > I'm testing a patch below: This patch looks good to me. The races here are quite minor in practice, involving simultaneous changes of connection state between different threads acting on the same socket, but also the sort of thing that would be almost impossible to debug if it occurred in practice :-). Robert N M Watson > Index: raw_ip.c > =================================================================== > RCS file: /home/ncvs/src/sys/netinet/raw_ip.c,v > retrieving revision 1.161 > diff -u -p -r1.161 raw_ip.c > --- raw_ip.c 15 May 2006 09:28:57 -0000 1.161 > +++ raw_ip.c 21 May 2006 18:13:14 -0000 > @@ -671,9 +671,11 @@ rip_disconnect(struct socket *so) > INP_INFO_WLOCK(&ripcbinfo); > INP_LOCK(inp); > inp->inp_faddr.s_addr = INADDR_ANY; > + SOCK_LOCK(so); > + so->so_state &= ~SS_ISCONNECTED; > + SOCK_UNLOCK(so); > INP_UNLOCK(inp); > INP_INFO_WUNLOCK(&ripcbinfo); > - so->so_state &= ~SS_ISCONNECTED; > return (0); > } > > Index: udp_usrreq.c > =================================================================== > RCS file: /home/ncvs/src/sys/netinet/udp_usrreq.c,v > retrieving revision 1.188 > diff -u -p -r1.188 udp_usrreq.c > --- udp_usrreq.c 6 May 2006 11:24:59 -0000 1.188 > +++ udp_usrreq.c 21 May 2006 18:12:43 -0000 > @@ -1057,9 +1057,11 @@ udp_disconnect(struct socket *so) > > in_pcbdisconnect(inp); > inp->inp_laddr.s_addr = INADDR_ANY; > + SOCK_LOCK(so) > + so->so_state &= ~SS_ISCONNECTED; /* XXX */ > + SOCK_UNLOCK(so) > INP_UNLOCK(inp); > INP_INFO_WUNLOCK(&udbinfo); > - so->so_state &= ~SS_ISCONNECTED; /* XXX */ > return 0; > } > > %%% > > -- > Maxim Konovalov >