[net,v1] net: protect tcf_block_unbind with block lock

Commit Message

Leon Romanovsky Oct. 26, 2020, 12:33 p.m. UTC

From: Leon Romanovsky <[e-mail]>

The tcf_block_unbind() expects that the caller will take block->cb_lock
before calling it, however the code took RTNL lock and dropped cb_lock
instead. This causes to the following kernel panic.

 WARNING: CPU: 1 PID: 13524 at net/sched/cls_api.c:1488 tcf_block_unbind+0x2db/0x420
 Modules linked in: mlx5_ib mlx5_core mlxfw ptp pps_core act_mirred act_tunnel_key cls_flower vxlan ip6_udp_tunnel udp_tunnel dummy sch_ingress openvswitch nsh xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 br_netfilter rpcrdma rdma_ucm ib_iser libiscsi scsi_transport_iscsi ib_umad ib_ipoib rdma_cm iw_cm ib_cm ib_uverbs ib_core overlay [last unloaded: mlxfw]
 CPU: 1 PID: 13524 Comm: test-ecmp-add-v Tainted: G        W         5.9.0+ #1
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
 RIP: 0010:tcf_block_unbind+0x2db/0x420
 Code: ff 48 83 c4 40 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8d bc 24 30 01 00 00 be ff ff ff ff e8 7d 7f 70 00 85 c0 0f 85 7b fd ff ff <0f> 0b e9 74 fd ff ff 48 c7 c7 dc 6a 24 84 e8 02 ec fe fe e9 55 fd
 RSP: 0018:ffff888117d17968 EFLAGS: 00010246
 RAX: 0000000000000000 RBX: ffff88812f713c00 RCX: 1ffffffff0848d5b
 RDX: 0000000000000001 RSI: ffff88814fbc8130 RDI: ffff888107f2b878
 RBP: 1ffff11022fa2f3f R08: 0000000000000000 R09: ffffffff84115a87
 R10: fffffbfff0822b50 R11: ffff888107f2b898 R12: ffff88814fbc8000
 R13: ffff88812f713c10 R14: ffff888117d17a38 R15: ffff88814fbc80c0
 FS:  00007f6593d36740(0000) GS:ffff8882a4f00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00005607a00758f8 CR3: 0000000131aea006 CR4: 0000000000170ea0
 Call Trace:
  tc_block_indr_cleanup+0x3e0/0x5a0
  ? tcf_block_unbind+0x420/0x420
  ? __mutex_unlock_slowpath+0xe7/0x610
  flow_indr_dev_unregister+0x5e2/0x930
  ? mlx5e_restore_tunnel+0xdf0/0xdf0 [mlx5_core]
  ? mlx5e_restore_tunnel+0xdf0/0xdf0 [mlx5_core]
  ? flow_indr_block_cb_alloc+0x3c0/0x3c0
  ? mlx5_db_free+0x37c/0x4b0 [mlx5_core]
  mlx5e_cleanup_rep_tx+0x8b/0xc0 [mlx5_core]
  mlx5e_detach_netdev+0xe5/0x120 [mlx5_core]
  mlx5e_vport_rep_unload+0x155/0x260 [mlx5_core]
  esw_offloads_disable+0x227/0x2b0 [mlx5_core]
  mlx5_eswitch_disable_locked.cold+0x38e/0x699 [mlx5_core]
  mlx5_eswitch_disable+0x94/0xf0 [mlx5_core]
  mlx5_device_disable_sriov+0x183/0x1f0 [mlx5_core]
  mlx5_core_sriov_configure+0xfd/0x230 [mlx5_core]
  sriov_numvfs_store+0x261/0x2f0
  ? sriov_drivers_autoprobe_store+0x110/0x110
  ? sysfs_file_ops+0x170/0x170
  ? sysfs_file_ops+0x117/0x170
  ? sysfs_file_ops+0x170/0x170
  kernfs_fop_write+0x1ff/0x3f0
  ? rcu_read_lock_any_held+0x6e/0x90
  vfs_write+0x1f3/0x620
  ksys_write+0xf9/0x1d0
  ? __x64_sys_read+0xb0/0xb0
  ? lockdep_hardirqs_on_prepare+0x273/0x3f0
  ? syscall_enter_from_user_mode+0x1d/0x50
  do_syscall_64+0x2d/0x40
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

<...>

 ---[ end trace bfdd028ada702879 ]---

Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
Signed-off-by: Leon Romanovsky <[e-mail]>
---
v1:
 * Returned rtnl_lock()
v0:
https://lore.kernel.org/netdev/[e-mail]
---
 net/sched/cls_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.26.2

Comments

Jakub Kicinski Oct. 28, 2020, 1:03 a.m. UTC | #1

On Mon, 26 Oct 2020 14:33:27 +0200 Leon Romanovsky wrote:
> From: Leon Romanovsky <[e-mail]>
> 
> The tcf_block_unbind() expects that the caller will take block->cb_lock
> before calling it, however the code took RTNL lock and dropped cb_lock
> instead. This causes to the following kernel panic.
 
> Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> Signed-off-by: Leon Romanovsky <[e-mail]>
> ---
> v1:
>  * Returned rtnl_lock()
> v0:
> https://lore.kernel.org/netdev/[e-mail]

Applied, thanks. 

I'm surprised you put the lore link in the notes. Maybe the usefulness
of the change log could be argued, but the number of times I tried to
find a specific revision and couldn't...

Leon Romanovsky Oct. 28, 2020, 5:28 a.m. UTC | #2

On Tue, Oct 27, 2020 at 06:03:31PM -0700, Jakub Kicinski wrote:
> On Mon, 26 Oct 2020 14:33:27 +0200 Leon Romanovsky wrote:
> > From: Leon Romanovsky <[e-mail]>
> >
> > The tcf_block_unbind() expects that the caller will take block->cb_lock
> > before calling it, however the code took RTNL lock and dropped cb_lock
> > instead. This causes to the following kernel panic.
>
> > Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> > Signed-off-by: Leon Romanovsky <[e-mail]>
> > ---
> > v1:
> >  * Returned rtnl_lock()
> > v0:
> > https://lore.kernel.org/netdev/[e-mail]
>
> Applied, thanks.
>
> I'm surprised you put the lore link in the notes. Maybe the usefulness
> of the change log could be argued, but the number of times I tried to
> find a specific revision and couldn't...

This is my preferred way of submitting patches, it allows to enjoy from
all worlds: clean commit message and ability to see previous versions
at the same time.

Lorifier [1] gives such links to me in automatic way, all that I need
is to copy them from previous versions.

[1] https://github.com/danrue/lorifier

Thanks

Patch

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index faeabff283a2..838b3fd94d77 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -652,12 +652,12 @@  static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
 			       block_cb->indr.binder_type,
 			       &block->flow_block, tcf_block_shared(block),
 			       &extack);
+	rtnl_lock();
 	down_write(&block->cb_lock);
 	list_del(&block_cb->driver_list);
 	list_move(&block_cb->list, &bo.cb_list);
-	up_write(&block->cb_lock);
-	rtnl_lock();
 	tcf_block_unbind(block, &bo);
+	up_write(&block->cb_lock);
 	rtnl_unlock();
 }