* [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence
@ 2025-03-11 6:12 Siddharth Vadapalli
2025-03-11 7:09 ` Sverdlin, Alexander
0 siblings, 1 reply; 5+ messages in thread
From: Siddharth Vadapalli @ 2025-03-11 6:12 UTC (permalink / raw)
To: andrew+netdev, davem, edumazet, kuba, pabeni, rogerq, horms,
alexander.sverdlin, dan.carpenter, jpanis
Cc: vigneshr, netdev, linux-kernel, linux-arm-kernel, srk, s-vadapalli
From: Vignesh Raghavendra <vigneshr@ti.com>
Registering the interrupts for TX or RX DMA Channels prior to registering
their respective NAPI callbacks can result in a NULL pointer dereference.
This is seen in practice as a random occurrence since it depends on the
randomness associated with the generation of traffic by Linux and the
reception of traffic from the wire.
Fixes: 681eb2beb3ef ("net: ethernet: ti: am65-cpsw: ensure proper channel cleanup in error path")
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Co-developed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
Hello,
This patch is based on commit
4d872d51bc9d Merge tag 'x86-urgent-2025-03-10' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
of Mainline Linux.
Regards,
Siddharth.
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 2806238629f8..d5291281c781 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2314,6 +2314,9 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
hrtimer_init(&tx_chn->tx_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
tx_chn->tx_hrtimer.function = &am65_cpsw_nuss_tx_timer_callback;
+ netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
+ am65_cpsw_nuss_tx_poll);
+
ret = devm_request_irq(dev, tx_chn->irq,
am65_cpsw_nuss_tx_irq,
IRQF_TRIGGER_HIGH,
@@ -2323,9 +2326,6 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
tx_chn->id, tx_chn->irq, ret);
goto err;
}
-
- netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
- am65_cpsw_nuss_tx_poll);
}
return 0;
@@ -2569,6 +2569,9 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
HRTIMER_MODE_REL_PINNED);
flow->rx_hrtimer.function = &am65_cpsw_nuss_rx_timer_callback;
+ netif_napi_add(common->dma_ndev, &flow->napi_rx,
+ am65_cpsw_nuss_rx_poll);
+
ret = devm_request_irq(dev, flow->irq,
am65_cpsw_nuss_rx_irq,
IRQF_TRIGGER_HIGH,
@@ -2579,9 +2582,6 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
flow->irq = -EINVAL;
goto err_flow;
}
-
- netif_napi_add(common->dma_ndev, &flow->napi_rx,
- am65_cpsw_nuss_rx_poll);
}
/* setup classifier to route priorities to flows */
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence
2025-03-11 6:12 [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence Siddharth Vadapalli
@ 2025-03-11 7:09 ` Sverdlin, Alexander
2025-03-11 8:51 ` s-vadapalli
0 siblings, 1 reply; 5+ messages in thread
From: Sverdlin, Alexander @ 2025-03-11 7:09 UTC (permalink / raw)
To: andrew+netdev, s-vadapalli, davem, dan.carpenter, jpanis, pabeni,
horms, edumazet, rogerq, kuba
Cc: linux-arm-kernel, vigneshr, netdev, linux-kernel, srk
Hi Siddharth!
On Tue, 2025-03-11 at 11:42 +0530, Siddharth Vadapalli wrote:
> From: Vignesh Raghavendra <vigneshr@ti.com>
>
> Registering the interrupts for TX or RX DMA Channels prior to registering
> their respective NAPI callbacks can result in a NULL pointer dereference.
> This is seen in practice as a random occurrence since it depends on the
> randomness associated with the generation of traffic by Linux and the
> reception of traffic from the wire.
>
> Fixes: 681eb2beb3ef ("net: ethernet: ti: am65-cpsw: ensure proper channel cleanup in error path")
The patch Vignesh mentions here...
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Co-developed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>
> Hello,
>
> This patch is based on commit
> 4d872d51bc9d Merge tag 'x86-urgent-2025-03-10' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> of Mainline Linux.
>
> Regards,
> Siddharth.
>
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 2806238629f8..d5291281c781 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2314,6 +2314,9 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> hrtimer_init(&tx_chn->tx_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> tx_chn->tx_hrtimer.function = &am65_cpsw_nuss_tx_timer_callback;
>
> + netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> + am65_cpsw_nuss_tx_poll);
> +
> ret = devm_request_irq(dev, tx_chn->irq,
> am65_cpsw_nuss_tx_irq,
> IRQF_TRIGGER_HIGH,
> @@ -2323,9 +2326,6 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> tx_chn->id, tx_chn->irq, ret);
> goto err;
> }
> -
> - netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> - am65_cpsw_nuss_tx_poll);
... has accounted for the fact ..._napi_add_... happens after [possibly unsuccessful] request_irq,
please grep for "for (--i ;". Is it necessary to adjust both loops, in the below case too?
> }
>
> return 0;
> @@ -2569,6 +2569,9 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
> HRTIMER_MODE_REL_PINNED);
> flow->rx_hrtimer.function = &am65_cpsw_nuss_rx_timer_callback;
>
> + netif_napi_add(common->dma_ndev, &flow->napi_rx,
> + am65_cpsw_nuss_rx_poll);
> +
> ret = devm_request_irq(dev, flow->irq,
> am65_cpsw_nuss_rx_irq,
> IRQF_TRIGGER_HIGH,
> @@ -2579,9 +2582,6 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
> flow->irq = -EINVAL;
> goto err_flow;
> }
> -
> - netif_napi_add(common->dma_ndev, &flow->napi_rx,
> - am65_cpsw_nuss_rx_poll);
> }
>
> /* setup classifier to route priorities to flows */
--
Alexander Sverdlin
Siemens AG
www.siemens.com
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence
2025-03-11 7:09 ` Sverdlin, Alexander
@ 2025-03-11 8:51 ` s-vadapalli
2025-03-11 8:56 ` Sverdlin, Alexander
0 siblings, 1 reply; 5+ messages in thread
From: s-vadapalli @ 2025-03-11 8:51 UTC (permalink / raw)
To: Sverdlin, Alexander
Cc: andrew+netdev, s-vadapalli, davem, dan.carpenter, jpanis, pabeni,
horms, edumazet, rogerq, kuba, linux-arm-kernel, vigneshr,
netdev, linux-kernel, srk
On Tue, Mar 11, 2025 at 07:09:56AM +0000, Sverdlin, Alexander wrote:
> Hi Siddharth!
Hello Alexander,
>
> On Tue, 2025-03-11 at 11:42 +0530, Siddharth Vadapalli wrote:
> > From: Vignesh Raghavendra <vigneshr@ti.com>
> >
> > Registering the interrupts for TX or RX DMA Channels prior to registering
> > their respective NAPI callbacks can result in a NULL pointer dereference.
> > This is seen in practice as a random occurrence since it depends on the
> > randomness associated with the generation of traffic by Linux and the
> > reception of traffic from the wire.
> >
> > Fixes: 681eb2beb3ef ("net: ethernet: ti: am65-cpsw: ensure proper channel cleanup in error path")
>
> The patch Vignesh mentions here...
>
> > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> > Co-developed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >
> > Hello,
> >
> > This patch is based on commit
> > 4d872d51bc9d Merge tag 'x86-urgent-2025-03-10' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> > of Mainline Linux.
> >
> > Regards,
> > Siddharth.
> >
> > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > index 2806238629f8..d5291281c781 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > @@ -2314,6 +2314,9 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > hrtimer_init(&tx_chn->tx_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> > tx_chn->tx_hrtimer.function = &am65_cpsw_nuss_tx_timer_callback;
> >
> > + netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > + am65_cpsw_nuss_tx_poll);
> > +
> > ret = devm_request_irq(dev, tx_chn->irq,
> > am65_cpsw_nuss_tx_irq,
> > IRQF_TRIGGER_HIGH,
> > @@ -2323,9 +2326,6 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > tx_chn->id, tx_chn->irq, ret);
> > goto err;
> > }
> > -
> > - netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > - am65_cpsw_nuss_tx_poll);
>
> ... has accounted for the fact ..._napi_add_... happens after [possibly unsuccessful] request_irq,
> please grep for "for (--i ;". Is it necessary to adjust both loops, in the below case too?
Yes! The order within the cleanup path has to be reversed too i.e.
release IRQ first followed by deleting the NAPI callback. I assume that
you are referring to the same. Please let me know otherwise. The diff
corresponding to it is:
---------------------------------------------------------------------------------------------------
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index d5291281c781..32c844816501 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2334,8 +2334,8 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
for (--i ; i >= 0 ; i--) {
struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
- netif_napi_del(&tx_chn->napi_tx);
devm_free_irq(dev, tx_chn->irq, tx_chn);
+ netif_napi_del(&tx_chn->napi_tx);
}
return ret;
@@ -2592,8 +2592,8 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
err_flow:
for (--i; i >= 0 ; i--) {
flow = &rx_chn->flows[i];
- netif_napi_del(&flow->napi_rx);
devm_free_irq(dev, flow->irq, flow);
+ netif_napi_del(&flow->napi_rx);
}
err:
---------------------------------------------------------------------------------------------------
Based on your confirmation, I will implement the above and post the v2
patch. Thank you for reviewing this patch and providing feedback.
Regards,
Siddharth.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence
2025-03-11 8:51 ` s-vadapalli
@ 2025-03-11 8:56 ` Sverdlin, Alexander
2025-03-11 9:00 ` s-vadapalli
0 siblings, 1 reply; 5+ messages in thread
From: Sverdlin, Alexander @ 2025-03-11 8:56 UTC (permalink / raw)
To: s-vadapalli
Cc: andrew+netdev, dan.carpenter, davem, jpanis, srk, linux-kernel,
linux-arm-kernel, pabeni, horms, edumazet, rogerq, vigneshr,
netdev, kuba
Hi Siddharth!
On Tue, 2025-03-11 at 14:21 +0530, s-vadapalli@ti.com wrote:
> > > Registering the interrupts for TX or RX DMA Channels prior to registering
> > > their respective NAPI callbacks can result in a NULL pointer dereference.
> > > This is seen in practice as a random occurrence since it depends on the
> > > randomness associated with the generation of traffic by Linux and the
> > > reception of traffic from the wire.
> > >
> > > Fixes: 681eb2beb3ef ("net: ethernet: ti: am65-cpsw: ensure proper channel cleanup in error path")
> >
> > The patch Vignesh mentions here...
> >
> > > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> > > Co-developed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
...
> > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > @@ -2314,6 +2314,9 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > > hrtimer_init(&tx_chn->tx_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> > > tx_chn->tx_hrtimer.function = &am65_cpsw_nuss_tx_timer_callback;
> > >
> > > + netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > > + am65_cpsw_nuss_tx_poll);
> > > +
> > > ret = devm_request_irq(dev, tx_chn->irq,
> > > am65_cpsw_nuss_tx_irq,
> > > IRQF_TRIGGER_HIGH,
> > > @@ -2323,9 +2326,6 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > > tx_chn->id, tx_chn->irq, ret);
> > > goto err;
> > > }
> > > -
> > > - netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > > - am65_cpsw_nuss_tx_poll);
> >
> > ... has accounted for the fact ..._napi_add_... happens after [possibly unsuccessful] request_irq,
> > please grep for "for (--i ;". Is it necessary to adjust both loops, in the below case too?
>
> Yes! The order within the cleanup path has to be reversed too i.e.
Not only reverting the order...
What I'm referring is: when requesting i-th IRQ fails there has been
i-th NAPI already added, but the cleanup loops start from [i-1]-th instance.
It looks like a potential leak to me...
> release IRQ first followed by deleting the NAPI callback. I assume that
> you are referring to the same. Please let me know otherwise. The diff
> corresponding to it is:
> ---------------------------------------------------------------------------------------------------
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index d5291281c781..32c844816501 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -2334,8 +2334,8 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> for (--i ; i >= 0 ; i--) {
> struct am65_cpsw_tx_chn *tx_chn = &common->tx_chns[i];
>
> - netif_napi_del(&tx_chn->napi_tx);
> devm_free_irq(dev, tx_chn->irq, tx_chn);
> + netif_napi_del(&tx_chn->napi_tx);
> }
>
> return ret;
> @@ -2592,8 +2592,8 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
> err_flow:
> for (--i; i >= 0 ; i--) {
> flow = &rx_chn->flows[i];
> - netif_napi_del(&flow->napi_rx);
> devm_free_irq(dev, flow->irq, flow);
> + netif_napi_del(&flow->napi_rx);
> }
>
> err:
> ---------------------------------------------------------------------------------------------------
> Based on your confirmation, I will implement the above and post the v2
> patch. Thank you for reviewing this patch and providing feedback.
--
Alexander Sverdlin
Siemens AG
www.siemens.com
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence
2025-03-11 8:56 ` Sverdlin, Alexander
@ 2025-03-11 9:00 ` s-vadapalli
0 siblings, 0 replies; 5+ messages in thread
From: s-vadapalli @ 2025-03-11 9:00 UTC (permalink / raw)
To: Sverdlin, Alexander
Cc: s-vadapalli, andrew+netdev, dan.carpenter, davem, jpanis, srk,
linux-kernel, linux-arm-kernel, pabeni, horms, edumazet, rogerq,
vigneshr, netdev, kuba
On Tue, Mar 11, 2025 at 08:56:49AM +0000, Sverdlin, Alexander wrote:
> Hi Siddharth!
>
> On Tue, 2025-03-11 at 14:21 +0530, s-vadapalli@ti.com wrote:
> > > > Registering the interrupts for TX or RX DMA Channels prior to registering
> > > > their respective NAPI callbacks can result in a NULL pointer dereference.
> > > > This is seen in practice as a random occurrence since it depends on the
> > > > randomness associated with the generation of traffic by Linux and the
> > > > reception of traffic from the wire.
> > > >
> > > > Fixes: 681eb2beb3ef ("net: ethernet: ti: am65-cpsw: ensure proper channel cleanup in error path")
> > >
> > > The patch Vignesh mentions here...
> > >
> > > > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> > > > Co-developed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>
> ...
>
> > > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > @@ -2314,6 +2314,9 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > > > hrtimer_init(&tx_chn->tx_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> > > > tx_chn->tx_hrtimer.function = &am65_cpsw_nuss_tx_timer_callback;
> > > >
> > > > + netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > > > + am65_cpsw_nuss_tx_poll);
> > > > +
> > > > ret = devm_request_irq(dev, tx_chn->irq,
> > > > am65_cpsw_nuss_tx_irq,
> > > > IRQF_TRIGGER_HIGH,
> > > > @@ -2323,9 +2326,6 @@ static int am65_cpsw_nuss_ndev_add_tx_napi(struct am65_cpsw_common *common)
> > > > tx_chn->id, tx_chn->irq, ret);
> > > > goto err;
> > > > }
> > > > -
> > > > - netif_napi_add_tx(common->dma_ndev, &tx_chn->napi_tx,
> > > > - am65_cpsw_nuss_tx_poll);
> > >
> > > ... has accounted for the fact ..._napi_add_... happens after [possibly unsuccessful] request_irq,
> > > please grep for "for (--i ;". Is it necessary to adjust both loops, in the below case too?
> >
> > Yes! The order within the cleanup path has to be reversed too i.e.
>
> Not only reverting the order...
> What I'm referring is: when requesting i-th IRQ fails there has been
> i-th NAPI already added, but the cleanup loops start from [i-1]-th instance.
> It looks like a potential leak to me...
Thank you for clarifying. I will address this and the previous feedback in
the v2 patch.
Regards,
Siddharth.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-03-11 9:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-03-11 6:12 [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence Siddharth Vadapalli
2025-03-11 7:09 ` Sverdlin, Alexander
2025-03-11 8:51 ` s-vadapalli
2025-03-11 8:56 ` Sverdlin, Alexander
2025-03-11 9:00 ` s-vadapalli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®