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 <[email protected]>
---
I'm updating our products to use the latest kernel. One change that we have that
doesn't appear to have been upstreamed is related to the following soft lockup.
NMI watchdog: BUG: soft lockup - CPU#3 stuck for 23s! [swapper/3:0]
Modules linked in: tipc jitterentropy_rng echainiv drbg platform_driver(O) ipifwd(PO)
CPU: 3 PID: 0 Comm: swapper/3 Tainted: P O 4.4.6-at1 #1
task: a3054e00 ti: ac6b4000 task.ti: a307a000
NIP: 806891c4 LR: 804f5060 CTR: 804f50d0
REGS: ac6b59b0 TRAP: 0901 Tainted: P O (4.4.6-at1)
MSR: 00029002 <CE,EE,ME> CR: 84002088 XER: 20000000
GPR00: 804f50fc ac6b5a60 a3054e00 00029002 00000101 01001011 00000000 00000001
GPR08: 00021002 c1502d1c ac6b5ae4 00000000 804f50d0
NIP [806891c4] _raw_spin_lock_irqsave+0x44/0x80
LR [804f5060] skb_dequeue+0x20/0x90
Call Trace:
[ac6b5a80] [804f50fc] skb_queue_purge+0x2c/0x50
[ac6b5a90] [c1511058] tipc_node_xmit+0x138/0x170 [tipc]
[ac6b5ad0] [c1509e58] tipc_named_node_up+0x88/0xa0 [tipc]
[ac6b5b00] [c150fc1c] tipc_netlink_compat_stop+0x9bc/0xf50 [tipc]
[ac6b5b20] [c1511638] tipc_rcv+0x418/0x9b0 [tipc]
[ac6b5bc0] [c150218c] tipc_bcast_stop+0xfc/0x7b0 [tipc]
[ac6b5bd0] [80504e38] __netif_receive_skb_core+0x468/0xa10
[ac6b5c70] [805082fc] netif_receive_skb_internal+0x3c/0xe0
[ac6b5ca0] [80642a48] br_handle_frame_finish+0x1d8/0x4d0
[ac6b5d10] [80642f30] br_handle_frame+0x1f0/0x330
[ac6b5d60] [80504ec8] __netif_receive_skb_core+0x4f8/0xa10
[ac6b5e00] [805082fc] netif_receive_skb_internal+0x3c/0xe0
[ac6b5e30] [8044c868] _dpa_rx+0x148/0x5c0
[ac6b5ea0] [8044b0c8] priv_rx_default_dqrr+0x98/0x170
[ac6b5ed0] [804d1338] qman_p_poll_dqrr+0x1b8/0x240
[ac6b5f00] [8044b1c0] dpaa_eth_poll+0x20/0x60
[ac6b5f20] [805087cc] net_rx_action+0x15c/0x320
[ac6b5f80] [8002594c] __do_softirq+0x13c/0x250
[ac6b5fe0] [80025c34] irq_exit+0xb4/0xf0
[ac6b5ff0] [8000d81c] call_do_irq+0x24/0x3c
[a307be60] [80004acc] do_IRQ+0x8c/0x120
[a307be80] [8000f450] ret_from_except+0x0/0x18
--- interrupt: 501 at arch_cpu_idle+0x24/0x70
Eyeballing the code I think it can still happen since tipc_named_node_up
allocates struct sk_buff_head head on the stack so it could have arbitary
content.
net/tipc/name_distr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index 61219f0b9677..44abc8e9c990 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -190,7 +190,7 @@ void tipc_named_node_up(struct net *net, u32 dnode)
struct name_table *nt = tipc_name_table(net);
struct sk_buff_head head;
- __skb_queue_head_init(&head);
+ skb_queue_head_init(&head);
read_lock_bh(&nt->cluster_scope_lock);
named_distribute(net, &head, dnode, &nt->cluster_scope);
--
2.22.0
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 <[email protected]>
I would rather change the faulty skb_queue_purge() to __skb_queue_purge()
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 <[email protected]>
>
> I would rather change the faulty skb_queue_purge() to __skb_queue_purge()
>
Makes sense. I'll look at that for v2.
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 <[email protected]>
>>
>> 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.
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 <[email protected]>
>>>
>>> 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.
> -----Original Message-----
> From: Eric Dumazet <[email protected]>
> Sent: 9-Jul-19 03:31
> To: Chris Packham <[email protected]>; Eric Dumazet
> <[email protected]>; Jon Maloy <[email protected]>;
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> 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 <[email protected]>
> >>>
> >>> 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
On 7/9/19 3:25 PM, Jon Maloy wrote:
>
>
>> -----Original Message-----
>> From: Eric Dumazet <[email protected]>
>> Sent: 9-Jul-19 03:31
>> To: Chris Packham <[email protected]>; Eric Dumazet
>> <[email protected]>; Jon Maloy <[email protected]>;
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]
>> 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 <[email protected]>
>>>>>
>>>>> 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.
> -----Original Message-----
> From: Eric Dumazet <[email protected]>
> Sent: 9-Jul-19 09:46
> To: Jon Maloy <[email protected]>; Eric Dumazet
> <[email protected]>; Chris Packham
> <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
>
>
>
> On 7/9/19 3:25 PM, Jon Maloy wrote:
[...]
> > 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 ???
It is acquired twice:
- First, in tipc_sk_rcv()->tipc_skb_peek_port(), to peek into one or more queue members and read their destination port number.
- Second, in tipc_sk_rcv()->tipc_sk_enqueue()->tipc_skb_dequeue() to unlink a list member from the queue.
> >>
> > [...]
> >>
> >> 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 ?
No.
tipc_link_xmit() is called only for messages with a non-local destination. Otherwise, tipc_node_xmit() sends node local messages directly to the destination socket via tipc_sk_rcv().
The argument 'xmitq' becomes 'inputq' in tipc_sk_rcv() and 'list' in tipc_skb_peek_port(), since those functions don't distinguish between local and node external incoming messages.
>
> Please show me where the head->lock is acquired, and why it needed.
The argument 'inputq' to tipc_sk_rcv() may come from two sources:
1) As an aggregated member of each tipc_node::tipc_link_entry. This queue is needed to guarantee sequential delivery of messages from the node/link layer to the socket layer. In this case, there may be buffers for multiple destination sockets in the same queue, and we may have multiple concurrent tipc_sk_rcv() jobs working that queue. So, the lock is needed both for adding (in link.c::tipc_data_input()), peeking and removing buffers.
2) The case you have been looking at, where it is created as 'xmitq' on the stack by a local socket. Here, the lock is not strictly needed, as you have observed. But to reduce code duplication we have chosen to let the code in tipc_sk_rcv() handle both types of queues uniformly, i.e., as if they all contain buffers with potentially multiple destination sockets, worked on by multiple concurrent calls. This requires that the lock is initialized even for this type of queue. We have seen no measurable performance difference between this 'generic' reception algorithm and a tailor-made ditto for local messages only, while the amount of saved code is significant.
>
> If this is mandatory, then more fixes are needed than just initializing the lock
> for lockdep purposes.
It is not only for lockdep purposes, -it is essential. But please provide details about where you see that more fixes are needed.
BR
///jon
On 7/9/19 10:15 PM, Jon Maloy wrote:
>
> It is not only for lockdep purposes, -it is essential. But please provide details about where you see that more fixes are needed.
>
Simple fact that you detect a problem only when skb_queue_purge() is called should talk by itself.
As I stated, there are many places where the list is manipulated _without_ its spinlock being held.
You want consistency, then
- grab the spinlock all the time.
- Or do not ever use it.
Do not initialize the spinlock just in case a path will use skb_queue_purge() (instead of using __skb_queue_purge())
> -----Original Message-----
> From: Eric Dumazet <[email protected]>
> Sent: 10-Jul-19 04:00
> To: Jon Maloy <[email protected]>; Eric Dumazet
> <[email protected]>; Chris Packham
> <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
>
>
>
> On 7/9/19 10:15 PM, Jon Maloy wrote:
> >
> > It is not only for lockdep purposes, -it is essential. But please provide details
> about where you see that more fixes are needed.
> >
>
> Simple fact that you detect a problem only when skb_queue_purge() is called
> should talk by itself.
>
> As I stated, there are many places where the list is manipulated _without_ its
> spinlock being held.
Yes, and that is the way it should be on the send path.
>
> You want consistency, then
>
> - grab the spinlock all the time.
> - Or do not ever use it.
That is exactly what we are doing.
- The send path doesn't need the spinlock, and never grabs it.
- The receive path does need it, and always grabs it.
However, since we don't know from the beginning which path a created message will follow, we initialize the queue spinlock "just in case" when it is created, even though it may never be used later.
You can see this as a violation of the principle you are stating above, but it is a prize that is worth paying, given savings in code volume, complexity and performance.
>
> Do not initialize the spinlock just in case a path will use skb_queue_purge()
> (instead of using __skb_queue_purge())
I am ok with that. I think we can agree that Chris goes for that solution, so we can get this bug fixed.
///jon
On 11/07/19 1:10 AM, Jon Maloy wrote:
>> -----Original Message-----
>> From: Eric Dumazet <[email protected]>
>> Sent: 10-Jul-19 04:00
>> To: Jon Maloy <[email protected]>; Eric Dumazet
>> <[email protected]>; Chris Packham
>> <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
>>
>>
>>
>> On 7/9/19 10:15 PM, Jon Maloy wrote:
>>>
>>> It is not only for lockdep purposes, -it is essential. But please provide details
>> about where you see that more fixes are needed.
>>>
>>
>> Simple fact that you detect a problem only when skb_queue_purge() is called
>> should talk by itself.
>>
>> As I stated, there are many places where the list is manipulated _without_ its
>> spinlock being held.
>
> Yes, and that is the way it should be on the send path.
>
>>
>> You want consistency, then
>>
>> - grab the spinlock all the time.
>> - Or do not ever use it.
>
> That is exactly what we are doing.
> - The send path doesn't need the spinlock, and never grabs it.
> - The receive path does need it, and always grabs it.
>
> However, since we don't know from the beginning which path a created
> message will follow, we initialize the queue spinlock "just in case"
> when it is created, even though it may never be used later.
> You can see this as a violation of the principle you are stating
> above, but it is a prize that is worth paying, given savings in code
> volume, complexity and performance.
>
>>
>> Do not initialize the spinlock just in case a path will use skb_queue_purge()
>> (instead of using __skb_queue_purge())
>
> I am ok with that. I think we can agree that Chris goes for that
> solution, so we can get this bug fixed.
So would you like a v2 with an improved commit message? I note that I
said skb->lock instead of head->lock in the subject line so at least
that should be corrected.
> -----Original Message-----
> From: Chris Packham <[email protected]>
> Sent: 10-Jul-19 16:58
> To: Jon Maloy <[email protected]>; Eric Dumazet
> <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
>
>
> On 11/07/19 1:10 AM, Jon Maloy wrote:
> >> -----Original Message-----
> >> From: Eric Dumazet <[email protected]>
> >> Sent: 10-Jul-19 04:00
> >> To: Jon Maloy <[email protected]>; Eric Dumazet
> >> <[email protected]>; Chris Packham
> >> <[email protected]>; [email protected];
> >> [email protected]
> >> Cc: [email protected]; [email protected];
> >> linux- [email protected]
> >> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
> >>
> >>
> >>
> >> On 7/9/19 10:15 PM, Jon Maloy wrote:
> >>>
> >>> It is not only for lockdep purposes, -it is essential. But please
> >>> provide details
> >> about where you see that more fixes are needed.
> >>>
> >>
> >> Simple fact that you detect a problem only when skb_queue_purge() is
> >> called should talk by itself.
> >>
> >> As I stated, there are many places where the list is manipulated
> >> _without_ its spinlock being held.
> >
> > Yes, and that is the way it should be on the send path.
> >
> >>
> >> You want consistency, then
> >>
> >> - grab the spinlock all the time.
> >> - Or do not ever use it.
> >
> > That is exactly what we are doing.
> > - The send path doesn't need the spinlock, and never grabs it.
> > - The receive path does need it, and always grabs it.
> >
> > However, since we don't know from the beginning which path a created
> > message will follow, we initialize the queue spinlock "just in case"
> > when it is created, even though it may never be used later.
> > You can see this as a violation of the principle you are stating
> > above, but it is a prize that is worth paying, given savings in code
> > volume, complexity and performance.
> >
> >>
> >> Do not initialize the spinlock just in case a path will use
> >> skb_queue_purge() (instead of using __skb_queue_purge())
> >
> > I am ok with that. I think we can agree that Chris goes for that
> > solution, so we can get this bug fixed.
>
> So would you like a v2 with an improved commit message? I note that I said
> skb->lock instead of head->lock in the subject line so at least that should be
> corrected.
Yes, unless Eric has any more objections.
I addition, I have realized that we can change from skb_queue_head_init() to __skb_queue_head_init() at all the disputed locations in the socket code.
Then, we do a separate call to spin_lock_init(&list->lock) at the moment we find that the message will follow the receive path, i.e., in tipc_node_xmit().
That should make everybody happy.
I will post a patch when net-next re-opens.
BR
///jon