netdev - Re: ipx: fix locking regression in ipx_sendmsg and ipx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141118221057.GA13473@midget.suse.cz>
Date:	Tue, 18 Nov 2014 23:10:57 +0100
From:	Jiri Bohac <jbohac@...e.cz>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Jiri Bohac <jbohac@...e.cz>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg

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@...e.cz>
SUSE Labs, SUSE CZ

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html