| Message ID | 20240612060446.1754392-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 79f18a41dd056115d685f3b0a419c7cd40055e13 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] ionic: fix use after netif_napi_del() | expand |
On 6/11/2024 11:04 PM, Taehee Yoo wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > When queues are started, netif_napi_add() and napi_enable() are called. > If there are 4 queues and only 3 queues are used for the current > configuration, only 3 queues' napi should be registered and enabled. > The ionic_qcq_enable() checks whether the .poll pointer is not NULL for > enabling only the using queue' napi. Unused queues' napi will not be > registered by netif_napi_add(), so the .poll pointer indicates NULL. > But it couldn't distinguish whether the napi was unregistered or not > because netif_napi_del() doesn't reset the .poll pointer to NULL. > So, ionic_qcq_enable() calls napi_enable() for the queue, which was > unregistered by netif_napi_del(). > > Reproducer: > ethtool -L <interface name> rx 1 tx 1 combined 0 > ethtool -L <interface name> rx 0 tx 0 combined 1 > ethtool -L <interface name> rx 0 tx 0 combined 4 > > Splat looks like: > kernel BUG at net/core/dev.c:6666! > Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI > CPU: 3 PID: 1057 Comm: kworker/3:3 Not tainted 6.10.0-rc2+ #16 > Workqueue: events ionic_lif_deferred_work [ionic] > RIP: 0010:napi_enable+0x3b/0x40 > Code: 48 89 c2 48 83 e2 f6 80 b9 61 09 00 00 00 74 0d 48 83 bf 60 01 00 00 00 74 03 80 ce 01 f0 4f > RSP: 0018:ffffb6ed83227d48 EFLAGS: 00010246 > RAX: 0000000000000000 RBX: ffff97560cda0828 RCX: 0000000000000029 > RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff97560cda0a28 > RBP: ffffb6ed83227d50 R08: 0000000000000400 R09: 0000000000000001 > R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000 > R13: ffff97560ce3c1a0 R14: 0000000000000000 R15: ffff975613ba0a20 > FS: 0000000000000000(0000) GS:ffff975d5f780000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f8f734ee200 CR3: 0000000103e50000 CR4: 00000000007506f0 > PKRU: 55555554 > Call Trace: > <TASK> > ? die+0x33/0x90 > ? do_trap+0xd9/0x100 > ? napi_enable+0x3b/0x40 > ? do_error_trap+0x83/0xb0 > ? napi_enable+0x3b/0x40 > ? napi_enable+0x3b/0x40 > ? exc_invalid_op+0x4e/0x70 > ? napi_enable+0x3b/0x40 > ? asm_exc_invalid_op+0x16/0x20 > ? napi_enable+0x3b/0x40 > ionic_qcq_enable+0xb7/0x180 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] > ionic_start_queues+0xc4/0x290 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] > ionic_link_status_check+0x11c/0x170 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] > ionic_lif_deferred_work+0x129/0x280 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] > process_one_work+0x145/0x360 > worker_thread+0x2bb/0x3d0 > ? __pfx_worker_thread+0x10/0x10 > kthread+0xcc/0x100 > ? __pfx_kthread+0x10/0x10 > ret_from_fork+0x2d/0x50 > ? __pfx_kthread+0x10/0x10 > ret_from_fork_asm+0x1a/0x30 > > Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling") > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > v2: > - Use ionic flag instead of napi flag. > > drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > index 24870da3f484..1934e9d6d9e4 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c > @@ -304,10 +304,8 @@ static int ionic_qcq_enable(struct ionic_qcq *qcq) > if (ret) > return ret; > > - if (qcq->napi.poll) > - napi_enable(&qcq->napi); > - > if (qcq->flags & IONIC_QCQ_F_INTR) { > + napi_enable(&qcq->napi); LGTM. Thanks for finding/fixing this! Reviewed-by: Brett Creeley <brett.creeley@amd.com> > irq_set_affinity_hint(qcq->intr.vector, > &qcq->intr.affinity_mask); > ionic_intr_mask(idev->intr_ctrl, qcq->intr.index, > -- > 2.34.1 >
On 6/12/2024 8:36 AM, Brett Creeley wrote: > On 6/11/2024 11:04 PM, Taehee Yoo wrote: >> Caution: This message originated from an External Source. Use proper >> caution when opening attachments, clicking links, or responding. >> >> >> When queues are started, netif_napi_add() and napi_enable() are called. >> If there are 4 queues and only 3 queues are used for the current >> configuration, only 3 queues' napi should be registered and enabled. >> The ionic_qcq_enable() checks whether the .poll pointer is not NULL for >> enabling only the using queue' napi. Unused queues' napi will not be >> registered by netif_napi_add(), so the .poll pointer indicates NULL. >> But it couldn't distinguish whether the napi was unregistered or not >> because netif_napi_del() doesn't reset the .poll pointer to NULL. >> So, ionic_qcq_enable() calls napi_enable() for the queue, which was >> unregistered by netif_napi_del(). >> >> Reproducer: >> ethtool -L <interface name> rx 1 tx 1 combined 0 >> ethtool -L <interface name> rx 0 tx 0 combined 1 >> ethtool -L <interface name> rx 0 tx 0 combined 4 >> >> Splat looks like: >> kernel BUG at net/core/dev.c:6666! >> Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI >> CPU: 3 PID: 1057 Comm: kworker/3:3 Not tainted 6.10.0-rc2+ #16 >> Workqueue: events ionic_lif_deferred_work [ionic] >> RIP: 0010:napi_enable+0x3b/0x40 >> Code: 48 89 c2 48 83 e2 f6 80 b9 61 09 00 00 00 74 0d 48 83 bf 60 01 >> 00 00 00 74 03 80 ce 01 f0 4f >> RSP: 0018:ffffb6ed83227d48 EFLAGS: 00010246 >> RAX: 0000000000000000 RBX: ffff97560cda0828 RCX: 0000000000000029 >> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff97560cda0a28 >> RBP: ffffb6ed83227d50 R08: 0000000000000400 R09: 0000000000000001 >> R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000 >> R13: ffff97560ce3c1a0 R14: 0000000000000000 R15: ffff975613ba0a20 >> FS: 0000000000000000(0000) GS:ffff975d5f780000(0000) >> knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007f8f734ee200 CR3: 0000000103e50000 CR4: 00000000007506f0 >> PKRU: 55555554 >> Call Trace: >> <TASK> >> ? die+0x33/0x90 >> ? do_trap+0xd9/0x100 >> ? napi_enable+0x3b/0x40 >> ? do_error_trap+0x83/0xb0 >> ? napi_enable+0x3b/0x40 >> ? napi_enable+0x3b/0x40 >> ? exc_invalid_op+0x4e/0x70 >> ? napi_enable+0x3b/0x40 >> ? asm_exc_invalid_op+0x16/0x20 >> ? napi_enable+0x3b/0x40 >> ionic_qcq_enable+0xb7/0x180 [ionic >> 59bdfc8a035436e1c4224ff7d10789e3f14643f8] >> ionic_start_queues+0xc4/0x290 [ionic >> 59bdfc8a035436e1c4224ff7d10789e3f14643f8] >> ionic_link_status_check+0x11c/0x170 [ionic >> 59bdfc8a035436e1c4224ff7d10789e3f14643f8] >> ionic_lif_deferred_work+0x129/0x280 [ionic >> 59bdfc8a035436e1c4224ff7d10789e3f14643f8] >> process_one_work+0x145/0x360 >> worker_thread+0x2bb/0x3d0 >> ? __pfx_worker_thread+0x10/0x10 >> kthread+0xcc/0x100 >> ? __pfx_kthread+0x10/0x10 >> ret_from_fork+0x2d/0x50 >> ? __pfx_kthread+0x10/0x10 >> ret_from_fork_asm+0x1a/0x30 >> >> Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling") >> Signed-off-by: Taehee Yoo <ap420073@gmail.com> >> --- >> v2: >> - Use ionic flag instead of napi flag. >> >> drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> index 24870da3f484..1934e9d6d9e4 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c >> @@ -304,10 +304,8 @@ static int ionic_qcq_enable(struct ionic_qcq *qcq) >> if (ret) >> return ret; >> >> - if (qcq->napi.poll) >> - napi_enable(&qcq->napi); >> - >> if (qcq->flags & IONIC_QCQ_F_INTR) { >> + napi_enable(&qcq->napi); > > LGTM. Thanks for finding/fixing this! > > Reviewed-by: Brett Creeley <brett.creeley@amd.com> Yes, thanks. Reviewed-by: Shannon Nelson <shannon.nelson@amd.com> > >> irq_set_affinity_hint(qcq->intr.vector, >> &qcq->intr.affinity_mask); >> ionic_intr_mask(idev->intr_ctrl, qcq->intr.index, >> -- >> 2.34.1 >>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 12 Jun 2024 06:04:46 +0000 you wrote: > When queues are started, netif_napi_add() and napi_enable() are called. > If there are 4 queues and only 3 queues are used for the current > configuration, only 3 queues' napi should be registered and enabled. > The ionic_qcq_enable() checks whether the .poll pointer is not NULL for > enabling only the using queue' napi. Unused queues' napi will not be > registered by netif_napi_add(), so the .poll pointer indicates NULL. > But it couldn't distinguish whether the napi was unregistered or not > because netif_napi_del() doesn't reset the .poll pointer to NULL. > So, ionic_qcq_enable() calls napi_enable() for the queue, which was > unregistered by netif_napi_del(). > > [...] Here is the summary with links: - [net,v2] ionic: fix use after netif_napi_del() https://git.kernel.org/netdev/net/c/79f18a41dd05 You are awesome, thank you!
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index 24870da3f484..1934e9d6d9e4 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -304,10 +304,8 @@ static int ionic_qcq_enable(struct ionic_qcq *qcq) if (ret) return ret; - if (qcq->napi.poll) - napi_enable(&qcq->napi); - if (qcq->flags & IONIC_QCQ_F_INTR) { + napi_enable(&qcq->napi); irq_set_affinity_hint(qcq->intr.vector, &qcq->intr.affinity_mask); ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
When queues are started, netif_napi_add() and napi_enable() are called. If there are 4 queues and only 3 queues are used for the current configuration, only 3 queues' napi should be registered and enabled. The ionic_qcq_enable() checks whether the .poll pointer is not NULL for enabling only the using queue' napi. Unused queues' napi will not be registered by netif_napi_add(), so the .poll pointer indicates NULL. But it couldn't distinguish whether the napi was unregistered or not because netif_napi_del() doesn't reset the .poll pointer to NULL. So, ionic_qcq_enable() calls napi_enable() for the queue, which was unregistered by netif_napi_del(). Reproducer: ethtool -L <interface name> rx 1 tx 1 combined 0 ethtool -L <interface name> rx 0 tx 0 combined 1 ethtool -L <interface name> rx 0 tx 0 combined 4 Splat looks like: kernel BUG at net/core/dev.c:6666! Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI CPU: 3 PID: 1057 Comm: kworker/3:3 Not tainted 6.10.0-rc2+ #16 Workqueue: events ionic_lif_deferred_work [ionic] RIP: 0010:napi_enable+0x3b/0x40 Code: 48 89 c2 48 83 e2 f6 80 b9 61 09 00 00 00 74 0d 48 83 bf 60 01 00 00 00 74 03 80 ce 01 f0 4f RSP: 0018:ffffb6ed83227d48 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff97560cda0828 RCX: 0000000000000029 RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff97560cda0a28 RBP: ffffb6ed83227d50 R08: 0000000000000400 R09: 0000000000000001 R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000 R13: ffff97560ce3c1a0 R14: 0000000000000000 R15: ffff975613ba0a20 FS: 0000000000000000(0000) GS:ffff975d5f780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f8f734ee200 CR3: 0000000103e50000 CR4: 00000000007506f0 PKRU: 55555554 Call Trace: <TASK> ? die+0x33/0x90 ? do_trap+0xd9/0x100 ? napi_enable+0x3b/0x40 ? do_error_trap+0x83/0xb0 ? napi_enable+0x3b/0x40 ? napi_enable+0x3b/0x40 ? exc_invalid_op+0x4e/0x70 ? napi_enable+0x3b/0x40 ? asm_exc_invalid_op+0x16/0x20 ? napi_enable+0x3b/0x40 ionic_qcq_enable+0xb7/0x180 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] ionic_start_queues+0xc4/0x290 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] ionic_link_status_check+0x11c/0x170 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] ionic_lif_deferred_work+0x129/0x280 [ionic 59bdfc8a035436e1c4224ff7d10789e3f14643f8] process_one_work+0x145/0x360 worker_thread+0x2bb/0x3d0 ? __pfx_worker_thread+0x10/0x10 kthread+0xcc/0x100 ? __pfx_kthread+0x10/0x10 ret_from_fork+0x2d/0x50 ? __pfx_kthread+0x10/0x10 ret_from_fork_asm+0x1a/0x30 Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v2: - Use ionic flag instead of napi flag. drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)