| Message ID | 1576506242-12761-1-git-send-email-rahul.lakkireddy@chelsio.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] cxgb4: fix refcount init for TC-MQPRIO offload | expand |
From: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> Date: Mon, 16 Dec 2019 19:54:02 +0530 > @@ -205,7 +205,11 @@ static int cxgb4_mqprio_alloc_hw_resources(struct net_device *dev) > cxgb4_enable_rx(adap, &eorxq->rspq); > } > > - refcount_inc(&adap->tc_mqprio->refcnt); > + if (!refcount_read(&adap->tc_mqprio->refcnt)) > + refcount_set(&adap->tc_mqprio->refcnt, 1); > + else > + refcount_inc(&adap->tc_mqprio->refcnt); > + This is not correct. You're bypassing the whole point of the refcount_t type which is to make sure that code that initializes a new object explicitly calls refcount_set() and that once the refcount goes to zero the object is freed or the refcount is initialized again using refcount_set() or similar. I'm not applying this, it's just papering around the real problem.
On Tuesday, December 12/17/19, 2019 at 14:09:30 -0800, David Miller wrote: > From: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> > Date: Mon, 16 Dec 2019 19:54:02 +0530 > > > @@ -205,7 +205,11 @@ static int cxgb4_mqprio_alloc_hw_resources(struct net_device *dev) > > cxgb4_enable_rx(adap, &eorxq->rspq); > > } > > > > - refcount_inc(&adap->tc_mqprio->refcnt); > > + if (!refcount_read(&adap->tc_mqprio->refcnt)) > > + refcount_set(&adap->tc_mqprio->refcnt, 1); > > + else > > + refcount_inc(&adap->tc_mqprio->refcnt); > > + > > This is not correct. > > You're bypassing the whole point of the refcount_t type which is to make sure > that code that initializes a new object explicitly calls refcount_set() and > that once the refcount goes to zero the object is freed or the refcount is > initialized again using refcount_set() or similar. > > I'm not applying this, it's just papering around the real problem. Ok, I'll move refcount_set() closer to where the hardware queue arrays, that it's tracking, have been allocated. Will send a v2. Thanks, Rahul
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c index 477973d2e341..ff6a932e9517 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c @@ -205,7 +205,11 @@ static int cxgb4_mqprio_alloc_hw_resources(struct net_device *dev) cxgb4_enable_rx(adap, &eorxq->rspq); } - refcount_inc(&adap->tc_mqprio->refcnt); + if (!refcount_read(&adap->tc_mqprio->refcnt)) + refcount_set(&adap->tc_mqprio->refcnt, 1); + else + refcount_inc(&adap->tc_mqprio->refcnt); + return 0; out_free_msix:
Properly initialize refcount to 1 before starting refcount increment. Otherwise, following warning is observed. [ 130.075342] ------------[ cut here ]------------ [ 130.075343] refcount_t: addition on 0; use-after-free. [ 130.075355] WARNING: CPU: 0 PID: 10870 at lib/refcount.c:25 refcount_warn_saturate+0xe1/0x100 [ 130.075356] Modules linked in: sch_mqprio iptable_nat ib_iser libiscsi scsi_transport_iscsi ib_ipoib rdma_ucm ib_umad iw_cxgb4 libcxgb ib_uverbs x86_pkg_temp_thermal cxgb4 igb [ 130.075361] CPU: 0 PID: 10870 Comm: tc Kdump: loaded Not tainted 5.5.0-rc1+ #11 [ 130.075362] Hardware name: Supermicro X9SRE/X9SRE-3F/X9SRi/X9SRi-3F/X9SRE/X9SRE-3F/X9SRi/X9SRi-3F, BIOS 3.2 01/16/2015 [ 130.075363] RIP: 0010:refcount_warn_saturate+0xe1/0x100 [ 130.075364] Code: e8 14 41 c1 ff 0f 0b c3 80 3d 44 f4 10 01 00 0f 85 63 ff ff ff 48 c7 c7 38 9f 83 8c 31 c0 c6 05 2e f4 10 01 01 e8 ef 40 c1 ff <0f> 0b c3 48 c7 c7 10 9f 83 8c 31 c0 c6 05 17 f4 10 01 01 e8 d7 40 [ 130.075365] RSP: 0018:ffffa48d00c0b768 EFLAGS: 00010286 [ 130.075366] RAX: 0000000000000000 RBX: 0000000000000008 RCX: 0000000000000001 [ 130.075366] RDX: 0000000000000001 RSI: 0000000000000096 RDI: ffff8a2e9fa187d0 [ 130.075367] RBP: ffff8a2e93890000 R08: 0000000000000398 R09: 000000000000003c [ 130.075367] R10: 00000000000142a0 R11: 0000000000000397 R12: ffffa48d00c0b848 [ 130.075368] R13: ffff8a2e94746498 R14: ffff8a2e966f7000 R15: 0000000000000031 [ 130.075368] FS: 00007f689015f840(0000) GS:ffff8a2e9fa00000(0000) knlGS:0000000000000000 [ 130.075369] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 130.075369] CR2: 00000000006762a0 CR3: 00000007cf164005 CR4: 00000000001606f0 [ 130.075370] Call Trace: [ 130.075377] cxgb4_setup_tc_mqprio+0xbee/0xc30 [cxgb4] [ 130.075382] ? cxgb4_ethofld_restart+0x50/0x50 [cxgb4] [ 130.075384] ? pfifo_fast_init+0x7e/0xf0 [ 130.075386] mqprio_init+0x5f4/0x630 [sch_mqprio] [ 130.075389] qdisc_create+0x1bf/0x4a0 [ 130.075390] tc_modify_qdisc+0x1ff/0x770 [ 130.075392] rtnetlink_rcv_msg+0x28b/0x350 [ 130.075394] ? rtnl_calcit.isra.32+0x110/0x110 [ 130.075395] netlink_rcv_skb+0xc6/0x100 [ 130.075396] netlink_unicast+0x1db/0x330 [ 130.075397] netlink_sendmsg+0x2f5/0x460 [ 130.075399] ? _copy_from_user+0x2e/0x60 [ 130.075400] sock_sendmsg+0x59/0x70 [ 130.075401] ____sys_sendmsg+0x1f0/0x230 [ 130.075402] ? copy_msghdr_from_user+0xd7/0x140 [ 130.075403] ___sys_sendmsg+0x77/0xb0 [ 130.075404] ? ___sys_recvmsg+0x84/0xb0 [ 130.075406] ? __handle_mm_fault+0x377/0xaf0 [ 130.075407] __sys_sendmsg+0x53/0xa0 [ 130.075409] do_syscall_64+0x44/0x130 [ 130.075412] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 130.075413] RIP: 0033:0x7f688f13af10 [ 130.075414] Code: c3 48 8b 05 82 6f 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 8d d0 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ae cc 00 00 48 89 04 24 [ 130.075414] RSP: 002b:00007ffe6c7d9988 EFLAGS: 00000246 ORIG_RAX: 000000000000002e [ 130.075415] RAX: ffffffffffffffda RBX: 00000000006703a0 RCX: 00007f688f13af10 [ 130.075415] RDX: 0000000000000000 RSI: 00007ffe6c7d99f0 RDI: 0000000000000003 [ 130.075416] RBP: 000000005df38312 R08: 0000000000000002 R09: 0000000000008000 [ 130.075416] R10: 00007ffe6c7d93e0 R11: 0000000000000246 R12: 0000000000000000 [ 130.075417] R13: 00007ffe6c7e9c50 R14: 0000000000000001 R15: 000000000067c600 [ 130.075418] ---[ end trace 8fbb3bf36a8671db ]--- Fixes: 2d0cb84dd973 ("cxgb4: add ETHOFLD hardware queue support") Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com> --- drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_mqprio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)