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. > 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() Using a spinlock to protect a local list makes no sense really, it spreads false sense of correctness. tipc_link_xmit() for example never acquires the spinlock, yet uses skb_peek() and __skb_dequeue() It is time to clean all this mess.
