netdev - Re: [PATCH net] net: ethernet: ti: am65-cpsw: fix freeing IRQ in am65_cpsw_nuss_remove_tx

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: <2bhpxcdducequwchyobyinj3xp2vsnpxkshtwqy24swto6zqvz@mbnnn7calbhv>
Date: Thu, 16 Jan 2025 10:45:30 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Roger Quadros <rogerq@...nel.org>
CC: Siddharth Vadapalli <s-vadapalli@...com>,
        Andrew Lunn
	<andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        "Eric
 Dumazet" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>,
        Grygorii Strashko <grygorii.strashko@...com>, <srk@...com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: ethernet: ti: am65-cpsw: fix freeing IRQ in
 am65_cpsw_nuss_remove_tx_chns()

On Wed, Jan 15, 2025 at 06:38:57PM +0200, Roger Quadros wrote:
> Siddharth,
> 
> On 15/01/2025 17:49, Roger Quadros wrote:
> > Hi Siddharth,
> > 
> > On 15/01/2025 12:38, Siddharth Vadapalli wrote:
> >> On Wed, Jan 15, 2025 at 12:04:17PM +0200, Roger Quadros wrote:
> >>> Hi Siddharth,
> >>>
> >>> On 15/01/2025 07:18, Siddharth Vadapalli wrote:
> >>>> On Tue, Jan 14, 2025 at 06:44:02PM +0200, Roger Quadros wrote:
> >>>>
> >>>> Hello Roger,
> >>>>
> >>>>> When getting the IRQ we use k3_udma_glue_rx_get_irq() which returns
> >>>>
> >>>> You probably meant "k3_udma_glue_tx_get_irq()" instead? It is used to
> >>>> assign tx_chn->irq within am65_cpsw_nuss_init_tx_chns() as follows:
> >>>
> >>> Yes I meant tx instead of rx.
> >>>
> >>>>
> >>>> 		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
> >>>>
> >>>> Additionally, following the above section we have:
> >>>>
> >>>> 		if (tx_chn->irq < 0) {
> >>>> 			dev_err(dev, "Failed to get tx dma irq %d\n",
> >>>> 				tx_chn->irq);
> >>>> 			ret = tx_chn->irq;
> >>>> 			goto err;
> >>>> 		}
> >>>>
> >>>> Could you please provide details on the code-path which will lead to a
> >>>> negative "tx_chn->irq" within "am65_cpsw_nuss_remove_tx_chns()"?
> >>>>
> >>>> There seem to be two callers of am65_cpsw_nuss_remove_tx_chns(), namely:
> >>>> 1. am65_cpsw_nuss_update_tx_rx_chns()
> >>>> 2. am65_cpsw_nuss_suspend()
> >>>> Since both of them seem to invoke am65_cpsw_nuss_remove_tx_chns() only
> >>>> in the case where am65_cpsw_nuss_init_tx_chns() *did not* error out, it
> >>>> appears to me that "tx_chn->irq" will never be negative within
> >>>> am65_cpsw_nuss_remove_tx_chns()
> >>>>
> >>>> Please let me know if I have overlooked something.
> >>>
> >>> The issue is with am65_cpsw_nuss_update_tx_rx_chns(). It can be called
> >>> repeatedly (by user changing number of TX queues) even if previous call
> >>> to am65_cpsw_nuss_init_tx_chns() failed.
> >>
> >> Thank you for clarifying. So the issue/bug was discovered since the
> >> implementation of am65_cpsw_nuss_update_tx_rx_chns(). The "Fixes" tag
> >> misled me. Maybe the "Fixes" tag should be updated? Though we should
> >> code to future-proof it as done in this patch, the "Fixes" tag pointing
> >> to the very first commit of the driver might not be accurate as the
> >> code-path associated with the bug cannot be exercised at that commit.
> > 
> > Fair enough. I'll change the Fixes commit.
> 
> Now that I check the code again, am65_cpsw_nuss_remove_tx_chns(),
> am65_cpsw_nuss_update_tx_chns() and am65_cpsw_nuss_init_tx_chns()
> were all introduced in the Fixes commit I stated.
> 
> Could you please share why you thought it is not accurate?

Though the functions were introduced in the Fixes commit that you have
mentioned in the commit message, the check for "tx_chn->irq" being
strictly positive as implemented in this patch, is not required until
the commit which added am65_cpsw_nuss_update_tx_rx_chns(). The reason
I say so is that a negative value for "tx_chn->irq" within
am65_cpsw_nuss_remove_tx_chns() requires am65_cpsw_nuss_init_tx_chns()
to partially fail *followed by* invocation of
am65_cpsw_nuss_remove_tx_chns(). That isn't possible in the blamed
commit which introduced them, since the driver probe fails when
am65_cpsw_nuss_init_tx_chns() fails. The code path:

	am65_cpsw_nuss_init_tx_chns() => Partially fails / Fails
	  am65_cpsw_nuss_remove_tx_chns() => Invoked later on

isn't possible in the blamed commit.

Regards,
Siddharth.