* [PATCH net v2] ionic: fix use after netif_napi_del()
@ 2024-06-12 6:04 Taehee Yoo
2024-06-12 15:36 ` Brett Creeley
2024-06-13 14:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 4+ messages in thread
From: Taehee Yoo @ 2024-06-12 6:04 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, shannon.nelson, brett.creeley,
drivers, netdev
Cc: ap420073, jacob.e.keller
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);
irq_set_affinity_hint(qcq->intr.vector,
&qcq->intr.affinity_mask);
ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net v2] ionic: fix use after netif_napi_del()
2024-06-12 6:04 [PATCH net v2] ionic: fix use after netif_napi_del() Taehee Yoo
@ 2024-06-12 15:36 ` Brett Creeley
2024-06-12 16:58 ` Nelson, Shannon
2024-06-13 14:40 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 4+ messages in thread
From: Brett Creeley @ 2024-06-12 15:36 UTC (permalink / raw)
To: Taehee Yoo, davem, kuba, pabeni, edumazet, shannon.nelson,
brett.creeley, drivers, netdev
Cc: jacob.e.keller
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net v2] ionic: fix use after netif_napi_del()
2024-06-12 15:36 ` Brett Creeley
@ 2024-06-12 16:58 ` Nelson, Shannon
0 siblings, 0 replies; 4+ messages in thread
From: Nelson, Shannon @ 2024-06-12 16:58 UTC (permalink / raw)
To: Brett Creeley, Taehee Yoo, davem, kuba, pabeni, edumazet,
brett.creeley, drivers, netdev
Cc: jacob.e.keller
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net v2] ionic: fix use after netif_napi_del()
2024-06-12 6:04 [PATCH net v2] ionic: fix use after netif_napi_del() Taehee Yoo
2024-06-12 15:36 ` Brett Creeley
@ 2024-06-13 14:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-06-13 14:40 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, edumazet, shannon.nelson, brett.creeley,
drivers, netdev, jacob.e.keller
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-13 14:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-12 6:04 [PATCH net v2] ionic: fix use after netif_napi_del() Taehee Yoo
2024-06-12 15:36 ` Brett Creeley
2024-06-12 16:58 ` Nelson, Shannon
2024-06-13 14:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).