Re: [PATCH stable v4.19] tipc: clean up skb list lock handling on send path

[Greg KH]

On Wed, Jul 22, 2020 at 10:41:57AM -0700, Aviraj CJ wrote:
> From: Jon Maloy <jon.maloy@xxxxxxxxxxxx>
> 
> upstream e654f9f53b45fde3fcc8051830b212c7a8f36148 commit
> 
> The policy for handling the skb list locks on the send and receive paths
> is simple.
> 
> - On the send path we never need to grab the lock on the 'xmitq' list
>   when the destination is an exernal node.
> 
> - On the receive path we always need to grab the lock on the 'inputq'
>   list, irrespective of source node.
> 
> However, when transmitting node local messages those will eventually
> end up on the receive path of a local socket, meaning that the argument
> 'xmitq' in tipc_node_xmit() will become the 'ínputq' argument in  the
> function tipc_sk_rcv(). This has been handled by always initializing
> the spinlock of the 'xmitq' list at message creation, just in case it
> may end up on the receive path later, and despite knowing that the lock
> in most cases never will be used.
> 
> This approach is inaccurate and confusing, and has also concealed the
> fact that the stated 'no lock grabbing' policy for the send path is
> violated in some cases.
> 
> We now clean up this by never initializing the lock at message creation,
> instead doing this at the moment we find that the message actually will
> enter the receive path. At the same time we fix the four locations
> where we incorrectly access the spinlock on the send/error path.
> 
> This patch also reverts commit d12cffe9329f ("tipc: ensure head->lock
> is initialised") which has now become redundant.
> 
> Additional comment:
> Below crash was seen while accessing uninitialized spinlock in skb_dequeue.
> 
> [ 282.961198] general protection fault: 0000 1 SMP PTI
> [ 282.961204] CPU: 0 PID: 9 Comm: ksoftirqd/0 Kdump: loaded : G O 4.19.106 #1
> [ 282.961218] Hardware name: Insyde Grangeville/Type2 - Board Product Name1, BIOS 05.05.15.0026 06/23/2016
> [ 282.961233] RIP: 0010:queued_spin_lock_slowpath+0x14c/0x190
> [ 282.961239] Code: ff ff 83 e8 01 75 eb e9 14 ff ff ff c1 e9 12 83 e0 03 83 e9 01 48 c1 e0 04 48 63 c9 48 05 00 ed 01 00 48 03 04 cd 80 c4 83 87 <48> 89 10 8b 42 08 85 c0 75 09 f3 90 8b 42 08 85 c0 74 f7 48 8b 02
> [ 282.961248] RSP: 0018:ffffa2510008b988 EFLAGS: 00010006
> [ 282.961254] RAX: 0065646f6d615d9f RBX: 0000000000000202 RCX: 0000000000003ffe
> [ 282.961259] RDX: ffff9a2b77c1ed00 RSI: 0000000000040000 RDI: ffffa2510008ba5c
> [ 282.961264] RBP: ffffa2510008ba48 R08: 00000000ffffffff R09: 0000000000000550
> [ 282.961271] R10: ffff9a2b54bb9050 R11: 0000000000000001 R12: ffffa2510008ba5c
> [ 282.961277] R13: ffff9a2b5fc1e800 R14: ffff9a2b5fc1e808 R15: 000000000000058c
> [ 282.961283] FS: 0000000000000000(0000) GS:ffff9a2b77c00000(0000) knlGS:0000000000000000
> [ 282.961288] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 282.961292] CR2: 00007f3f1321d2ac CR3: 00000001f0c0a002 CR4: 00000000003606f0
> [ 282.961297] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 282.961301] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 282.961307] Call Trace:
> [ 282.961321] _raw_spin_lock_irqsave+0x24/0x30
> [ 282.961331] skb_dequeue+0x18/0x60
> [ 282.961339] skb_queue_purge+0x1b/0x30
> [ 282.961359] tipc_node_xmit+0x155/0x170 [tipc]
> [ 282.961376] ? tipc_msg_init+0x2c/0xa0 [tipc]
> [ 282.961390] tipc_named_node_up+0x1d6/0x210 [tipc]
> [ 282.961406] tipc_node_write_unlock+0xf0/0x100 [tipc]
> [ 282.961421] tipc_rcv+0x54c/0xc90 [tipc]
> [ 282.961431] ? br_handle_frame_finish+0x3e0/0x3e0
> [ 282.961439] ? br_handle_frame_finish+0x3e0/0x3e0
> [ 282.961450] tipc_l2_rcv_msg+0x4a/0x70 [tipc]
> [ 282.961458] __netif_receive_skb_one_core+0x52/0x70
> [ 282.961465] netif_receive_skb_internal+0x39/0xb0
> [ 282.961472] br_pass_frame_up+0xe8/0xf0
> [ 282.961478] ? fdb_find_rcu+0xcd/0x130
> [ 282.961485] br_handle_frame_finish+0x27a/0x3e0
> [ 282.961493] br_handle_frame+0x191/0x2e0
> [ 282.961499] ? br_pass_frame_up+0xf0/0xf0
> [ 282.961506] __netif_receive_skb_core+0x1da/0xa70
> 
> CC: Eric Dumazet <edumazet@xxxxxxxxxx>
> Reported-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> Acked-by: Ying Xue <ying.xue@xxxxxxxxxxxxx>
> Signed-off-by: Jon Maloy <jon.maloy@xxxxxxxxxxxx>
> Reviewed-by: Xin Long <lucien.xin@xxxxxxxxx>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> [acj: backport v4.19 -stable
> - adjust context
> - skipped the hunk modifying non-existent function tipc_mcast_send_sync
> - additional comment ]

Now queued up, thanks.

greg k-h