linux-kernel - Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250311085109.q3g32v3ycoskhsko@uda0492258>
Date: Tue, 11 Mar 2025 14:21:09 +0530
From: "s-vadapalli@...com" <s-vadapalli@...com>
To: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
        "s-vadapalli@...com"
	<s-vadapalli@...com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "dan.carpenter@...aro.org" <dan.carpenter@...aro.org>,
        "jpanis@...libre.com"
	<jpanis@...libre.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "horms@...nel.org" <horms@...nel.org>,
        "edumazet@...gle.com"
	<edumazet@...gle.com>,
        "rogerq@...nel.org" <rogerq@...nel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
        "vigneshr@...com" <vigneshr@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "srk@...com"
	<srk@...com>
Subject: Re: [PATCH net] net: ethernet: ti: am65-cpsw: Fix NAPI registration
 sequence

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@...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@...com>
> > Co-developed-by: Siddharth Vadapalli <s-vadapalli@...com>
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@...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.