Messages in this thread | | | Subject | Re: [PATCH] tipc: ensure skb->lock is initialised | From | Eric Dumazet <> | Date | Tue, 9 Jul 2019 15:45:32 +0200 |
| |
On 7/9/19 3:25 PM, Jon Maloy wrote: > > >> -----Original Message----- >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Sent: 9-Jul-19 03:31 >> To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>; Eric Dumazet >> <eric.dumazet@gmail.com>; Jon Maloy <jon.maloy@ericsson.com>; >> ying.xue@windriver.com; davem@davemloft.net >> Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux- >> kernel@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.packham@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.
Where is the lock acquired, why was it only acquired by queue purge and not normal dequeues ???
> >> > [...] >> >> 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()
tipc_node_xmit() calls tipc_link_xmit() eventually, right ?
Please show me where the head->lock is acquired, and why it needed.
If this is mandatory, then more fixes are needed than just initializing the lock for lockdep purposes.
| |