Re: ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg

[Jiri Bohac <jbohac@suse.cz>]

On Tue, Nov 18, 2014 at 02:37:26PM +0100, Arnd Bergmann wrote:
> Does ipxrtr_route_packet() actually sleep while waiting for the network,
> or is it possible that you only need to change the recvmsg path?

You're right. 
In fact, it can sleep in sock_alloc_send_skb(), but my patch does
not fix this - it releases the lock after that.
So let's ignore that for now, I'll send a V2 modifying only
ipx_recvmsg().

> >  	if (sock_flag(sk, SOCK_ZAPPED))
> > -		goto out;
> > +		goto out_release;
> >  
> > +	release_sock(sk);
> >  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> >  				flags & MSG_DONTWAIT, &rc);
> >  	if (!skb) {
> 
> Same thing here: I think your patch could be simplified if you just
> release the socket lock before calling skb_recv_datagram and get
> it back afterwards, 

It would simplify the code a little to just get the lock again.
But do we really want to optimize for source code size at the cost of
taking locks that are not necessarry?

> and it would be much simpler if you could show that the lock is
> not needed at all.

At least the ipxitf_insert_socket() inside __ipx_bind() looks
like it must be protected not to corrupt the intrfc->if_sklist.
I am not familiar with the code, so there may be other things.

Thanks for the review!

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ