> -----Original Message----- > From: Eric Dumazet <eric.duma...@gmail.com> > Sent: 9-Jul-19 03:31 > To: Chris Packham <chris.pack...@alliedtelesis.co.nz>; Eric Dumazet > <eric.duma...@gmail.com>; Jon Maloy <jon.ma...@ericsson.com>; > ying....@windriver.com; da...@davemloft.net > Cc: net...@vger.kernel.org; tipc-discuss...@lists.sourceforge.net; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH] tipc: ensure skb->lock is initialised > > > > On 7/8/19 11:13 PM, Chris Packham wrote: > > On 9/07/19 8:43 AM, Chris Packham wrote: > >> On 8/07/19 8:18 PM, Eric Dumazet wrote: > >>> > >>> > >>> On 7/8/19 12:53 AM, Chris Packham wrote: > >>>> tipc_named_node_up() creates a skb list. It passes the list to > >>>> tipc_node_xmit() which has some code paths that can call > >>>> skb_queue_purge() which relies on the list->lock being initialised. > >>>> Ensure tipc_named_node_up() uses skb_queue_head_init() so that the > >>>> lock is explicitly initialised. > >>>> > >>>> Signed-off-by: Chris Packham <chris.pack...@alliedtelesis.co.nz> > >>> > >>> I would rather change the faulty skb_queue_purge() to > >>> __skb_queue_purge() > >>> > >> > >> Makes sense. I'll look at that for v2. > >> > > > > Actually maybe not. tipc_rcast_xmit(), tipc_node_xmit_skb(), > > tipc_send_group_msg(), __tipc_sendmsg(), __tipc_sendstream(), and > > tipc_sk_timeout() all use skb_queue_head_init(). So my original change > > brings tipc_named_node_up() into line with them. > > > > I think it should be safe for tipc_node_xmit() to use > > __skb_queue_purge() since all the callers seem to have exclusive > > access to the list of skbs. It still seems that the callers should all > > use > > skb_queue_head_init() for consistency.
I agree with that. > > > > No, tipc does not use the list lock (it relies on the socket lock) and > therefore > should consistently use __skb_queue_head_init() instead of > skb_queue_head_init() TIPC is using the list lock at message reception within the scope of tipc_sk_rcv()/tipc_skb_peek_port(), so it is fundamental that the lock always is correctly initialized. > [...] > > tipc_link_xmit() for example never acquires the spinlock, yet uses skb_peek() > and __skb_dequeue() You should look at tipc_node_xmit instead. Node local messages are sent directly to tipc_sk_rcv(), and never go through tipc_link_xmit() Regards ///jon
