| Message ID | 20250311061214.4111634-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence | expand |
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 */
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.
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.
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.
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 */