Re: HEADS UP: socket and pcb reference changes entering tree today

On Sun, 21 May 2006, 19:04+0100, Robert Watson wrote:

>
> On Sun, 21 May 2006, Maxim Konovalov wrote:
>
> > > This looks good in terms of pcb structure, but you should
> > > acquire SOCK_LOCK around the so_state manipulation.  To prevent
> > > races, I suggest doing it while also holding the INP lock in the
> > > center of the locking sets from the inpcb. There are some other
> > > remaining bugs in the raw socket code elsewhere also, I think.
> >
> > 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:

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
_at__at_ -671,9 +671,11 _at__at_ 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
_at__at_ -1057,9 +1057,11 _at__at_ 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