Re: Request for testers - socket locking diff

[Andrew Doran <ad%netbsd.org@localhost>]

On Sun, Apr 13, 2008 at 11:52:41AM -0700, Matt Thomas wrote:

> On Apr 13, 2008, at 10:17 AM, Andrew Doran wrote:
> >On Sat, Apr 12, 2008 at 10:41:18PM +0100, Andrew Doran wrote:
> >
> >>Here's an updated patch.
> >>
> >>    http://www.netbsd.org/~ad/socklock-2008041201.diff
> >>
> >>- Rudimentary tests on inet6 show it working.
> >>- The bluetooth patch is merged in.
> >>- More bugfixes, and assertions.
> >>- The sbcheck change is bogus (sbcheck appears to be bogus, anyway)
> >>- I think that I would prefer to move acquire of kernel_lock into the
> >>  individual protocols, instead of always having the wrappers which
> >>  will be unneeded baggage in the future.
> >
> >One problem I know of: I think there is a race when sending SCM_RIGHTS
> >messages through a Unix socket. The symptom I see is gnome-terminal  
> >hanging
> >due to a corrupted sockbuf. I think that the loop around  
> >unp_externalize in
> >soreceive needs to be atomic. I'm surprised we never got bitten by  
> >this
> >before since it could block, but whatever.
> >
> >Aside from the below I think the patch is nearly ready:
> >
> >- Make protosw::pr_lock mandatory.
> 
> You mean pr_getlock?
> 
> >- Make protocol methods take kernel_lock directly, and remove  
> >wrappers.
> >- Profiling and further testing.
> >- In netisrs, always lock in the socket -> kernel_lock direction,  
> >instead
> >  of letting the softint code take kernel_lock. This is just for  
> >efficiency.
> >
> >Any comments on it?
> 
> 
> In sys/unpcb.h, shouldn't unp_streamlock be kmutex_t * instead of  
> void *?

Yes, fixed.
 
> sonewconn should have an extra param which is the kmutex_t * for the new
> socket.  Actually, I want two new params, the kmutex_t * and a void *  
> for
> initializing so_pcb but that's in the future.
>
> pr_getlock() just seems wrong to me.  How hard is it going to be change
> the so_lock of a socket?  (which would happen in PRU_ATTACH)?
>
> I don't you handle the case where head_so != so in the soq* routines.

Have you read the comments in uipc_socket2 and uipc_usrreq? Embryonic
connections share a lock with the listening socket, until the connection is
fully established - at that point the association can be broken.

> I guess I think PRU_ATTACH should be called with so_lock NULL by default
> and unlocked.

I've changed it so that PRU_ATTACH sets the lock, unless the socket already
has a lock. In all cases it is assumed to be returned locked by PRU_ATTACH
(even on error).

Andrew