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: <8776a109-22c3-4c1e-a6a1-7bb0a4c70b06@kernel.org>
Date: Wed, 15 Jan 2025 12:04:17 +0200
From: Roger Quadros <rogerq@...nel.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: 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()

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.

Please try the below patch to simulate the error condition.

Then do the following
- bring down all network interfaces
- change num TX queues to 2. IRQ for 2nd channel fails.
- change num TX queues to 3. Now we try to free an invalid IRQ and we see warning.

Also I think it is good practice to code for set value than to code
for existing code paths as they can change in the future.

--test patch starts--

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 36c29d3db329..c22cadaaf3d3 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -155,7 +155,7 @@
 			 NETIF_MSG_IFUP	| NETIF_MSG_PROBE | NETIF_MSG_IFDOWN | \
 			 NETIF_MSG_RX_ERR | NETIF_MSG_TX_ERR)
 
-#define AM65_CPSW_DEFAULT_TX_CHNS	8
+#define AM65_CPSW_DEFAULT_TX_CHNS	1
 #define AM65_CPSW_DEFAULT_RX_CHN_FLOWS	1
 
 /* CPPI streaming packet interface */
@@ -2346,7 +2348,10 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common)
 		tx_chn->dsize_log2 = __fls(hdesc_size_out);
 		WARN_ON(hdesc_size_out != (1 << tx_chn->dsize_log2));
 
-		tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
+		if (i == 1)
+			tx_chn->irq = -ENODEV;
+		else
+			tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn);
 		if (tx_chn->irq < 0) {
 			dev_err(dev, "Failed to get tx dma irq %d\n",
 				tx_chn->irq);

--
cheers,
-roger