linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] net: hns3: some cleanups for hns3 driver
@ 2026-08-04 13:09 Jijie Shao
  2026-08-04 13:09 ` [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() Jijie Shao
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Jijie Shao @ 2026-08-04 13:09 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, andrew+netdev, horms
  Cc: shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel, shaojijie

This series contains three small cleanups for the hns3 driver.

Patch 1 sets msg->desc to NULL after kfree to avoid leaving a
dangling pointer in a struct that is reused across loop iterations.

Patch 2 adds the missing const qualifier to the reg parameter of
hclge_log_error(), which is never modified within the function.

Patch 3 uses the txqueue parameter passed by the ndo_tx_timeout
callback directly, instead of iterating all tx queues to find the
timed out one.

Jian Shen (2):
  net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
  net: hns3: use txqueue parameter directly in ndo_tx_timeout

Jijie Shao (1):
  net: hns3: add missing const qualifier to hclge_log_error() reg
    parameter

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 47 ++++++++-----------
 .../hisilicon/hns3/hns3pf/hclge_err.c         |  3 +-
 2 files changed, 22 insertions(+), 28 deletions(-)


base-commit: 69963a0678a347d57c4ac8b16939dba216eb95ce
-- 
2.43.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
  2026-08-04 13:09 [PATCH net-next 0/3] net: hns3: some cleanups for hns3 driver Jijie Shao
@ 2026-08-04 13:09 ` Jijie Shao
  2026-08-05 16:52   ` Simon Horman
  2026-08-04 13:09 ` [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter Jijie Shao
  2026-08-04 13:09 ` [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout Jijie Shao
  2 siblings, 1 reply; 12+ messages in thread
From: Jijie Shao @ 2026-08-04 13:09 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, andrew+netdev, horms
  Cc: shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel, shaojijie

From: Jian Shen <shenjian15@huawei.com>

In hclge_query_reg_info(), msg->desc is freed by kfree(), but the
caller continues to use msg across loop iterations. Set msg->desc
to NULL to avoid leaving a dangling pointer in the reused struct.

Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
index dac051e798da..7e124e2c718d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
@@ -1592,6 +1592,7 @@ hclge_query_reg_info(struct hclge_dev *hdev,
 	}
 
 	kfree(msg->desc);
+	msg->desc = NULL;
 }
 
 static void hclge_query_reg_info_of_ssu(struct hclge_dev *hdev)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter
  2026-08-04 13:09 [PATCH net-next 0/3] net: hns3: some cleanups for hns3 driver Jijie Shao
  2026-08-04 13:09 ` [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() Jijie Shao
@ 2026-08-04 13:09 ` Jijie Shao
  2026-08-05 16:52   ` Simon Horman
  2026-08-04 13:09 ` [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout Jijie Shao
  2 siblings, 1 reply; 12+ messages in thread
From: Jijie Shao @ 2026-08-04 13:09 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, andrew+netdev, horms
  Cc: shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel, shaojijie

The reg parameter of hclge_log_error() is never modified within the
function, but is declared as 'char *'. Callers pass const strings,
causing a compiler warning about discarding the 'const' qualifier.
Add the missing const to fix the warning.

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
index 7e124e2c718d..6093a60d257b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
@@ -1762,7 +1762,7 @@ static const struct hclge_hw_type_id hclge_hw_type_id_st[] = {
 	},
 };
 
-static void hclge_log_error(struct device *dev, char *reg,
+static void hclge_log_error(struct device *dev, const char *reg,
 			    const struct hclge_hw_error *err,
 			    u32 err_sts, unsigned long *reset_requests)
 {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout
  2026-08-04 13:09 [PATCH net-next 0/3] net: hns3: some cleanups for hns3 driver Jijie Shao
  2026-08-04 13:09 ` [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() Jijie Shao
  2026-08-04 13:09 ` [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter Jijie Shao
@ 2026-08-04 13:09 ` Jijie Shao
  2026-08-05 16:52   ` Simon Horman
  2 siblings, 1 reply; 12+ messages in thread
From: Jijie Shao @ 2026-08-04 13:09 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, andrew+netdev, horms
  Cc: shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel, shaojijie

From: Jian Shen <shenjian15@huawei.com>

The ndo_tx_timeout callback already provides the timed out txqueue
index. Use it directly instead of iterating all tx queues to find
the timed out one.

Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 47 ++++++++-----------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 6ecb32e28e79..d0656289d68e 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2825,32 +2825,25 @@ static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu)
 	return ret;
 }
 
-static int hns3_get_timeout_queue(struct net_device *ndev)
+static void hns3_dump_timeout_queue(struct net_device *ndev,
+				    unsigned int txqueue)
 {
-	unsigned int i;
-
-	/* Find the stopped queue the same way the stack does */
-	for (i = 0; i < ndev->num_tx_queues; i++) {
-		unsigned int timedout_ms;
-		struct netdev_queue *q;
+	unsigned int timedout_ms;
+	struct netdev_queue *q;
 
-		q = netdev_get_tx_queue(ndev, i);
-		timedout_ms = netif_xmit_timeout_ms(q);
-		if (timedout_ms) {
+	q = netdev_get_tx_queue(ndev, txqueue);
+	timedout_ms = netif_xmit_timeout_ms(q);
+	if (timedout_ms) {
 #ifdef CONFIG_BQL
-			struct dql *dql = &q->dql;
+		struct dql *dql = &q->dql;
 
-			netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
-				    dql->last_obj_cnt, dql->num_queued,
-				    dql->adj_limit, dql->num_completed);
+		netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
+			    dql->last_obj_cnt, dql->num_queued,
+			    dql->adj_limit, dql->num_completed);
 #endif
-			netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
-				    q->state, timedout_ms);
-			break;
-		}
+		netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
+			    q->state, timedout_ms);
 	}
-
-	return i;
 }
 
 static void hns3_dump_queue_stats(struct net_device *ndev,
@@ -2900,25 +2893,25 @@ static void hns3_dump_queue_reg(struct net_device *ndev,
 				      HNS3_RING_TX_RING_EBD_OFFSET_REG));
 }
 
-static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
+static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev,
+					 unsigned int txqueue)
 {
 	struct hns3_nic_priv *priv = netdev_priv(ndev);
 	struct hnae3_handle *h = hns3_get_handle(ndev);
 	struct hns3_enet_ring *tx_ring;
-	u32 timeout_queue;
 
-	timeout_queue = hns3_get_timeout_queue(ndev);
-	if (timeout_queue >= ndev->num_tx_queues) {
+	if (txqueue >= ndev->num_tx_queues) {
 		netdev_info(ndev,
 			    "no netdev TX timeout queue found, timeout count: %llu\n",
 			    priv->tx_timeout_count);
 		return false;
 	}
+	hns3_dump_timeout_queue(ndev, txqueue);
 
 	priv->tx_timeout_count++;
 
-	tx_ring = &priv->ring[timeout_queue];
-	hns3_dump_queue_stats(ndev, tx_ring, timeout_queue);
+	tx_ring = &priv->ring[txqueue];
+	hns3_dump_queue_stats(ndev, tx_ring, txqueue);
 
 	/* When mac received many pause frames continuous, it's unable to send
 	 * packets, which may cause tx timeout
@@ -2941,7 +2934,7 @@ static void hns3_nic_net_timeout(struct net_device *ndev, unsigned int txqueue)
 	struct hns3_nic_priv *priv = netdev_priv(ndev);
 	struct hnae3_handle *h = priv->ae_handle;
 
-	if (!hns3_get_tx_timeo_queue_info(ndev))
+	if (!hns3_get_tx_timeo_queue_info(ndev, txqueue))
 		return;
 
 	/* request the reset, and let the hclge to determine
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
  2026-08-04 13:09 ` [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() Jijie Shao
@ 2026-08-05 16:52   ` Simon Horman
  2026-08-06  7:32     ` Jijie Shao
  0 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2026-08-05 16:52 UTC (permalink / raw)
  To: shaojijie
  Cc: Simon Horman, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=1
---
--- Patch [1]: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() ---
commit ba3e282274aa0911fb278f48937385c06cc8f3a1
Author: Jian Shen <shenjian15@huawei.com>

net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()

This adds msg->desc = NULL after the kfree(msg->desc) at the end of
hclge_query_reg_info() in the hns3 PF error handling code, described as
avoiding a dangling pointer in a struct that the caller reuses.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> index dac051e798da2..7e124e2c718da 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> @@ -1592,6 +1592,7 @@ hclge_query_reg_info(struct hclge_dev *hdev,
>  	}
>  
>  	kfree(msg->desc);
> +	msg->desc = NULL;

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
Is the commit message description accurate here?  It says:

    In hclge_query_reg_info(), msg->desc is freed by kfree(), but the
    caller continues to use msg across loop iterations. Set msg->desc
    to NULL to avoid leaving a dangling pointer in the reused struct.

The only caller is hclge_query_reg_info_of_ssu(), and it re-initializes the
whole stack struct from the static template at the top of every iteration:

	for (i = 0; i < num; i++) {
		msg = hclge_ssu_reg_common_msg[i];
		...
		hclge_query_reg_info(hdev, &msg, loop_time, loop_para);
	}

None of the hclge_ssu_reg_common_msg[] entries initialize .desc, so msg.desc
is NULL again on each pass.

Even without that, hclge_query_reg_info() overwrites msg->desc before any
read of it:

	msg->desc = kzalloc(desc_len, GFP_KERNEL);
	if (!msg->desc) {
		...
		return;
	}

so the freed pointer is never dereferenced or freed a second time.

Would it be worth rewording the message to describe this as defensive
cleanup rather than a dangling pointer fix, so a stable backporter does not
read it as a use-after-free fix?  The patch correctly carries no Fixes: tag.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter
  2026-08-04 13:09 ` [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter Jijie Shao
@ 2026-08-05 16:52   ` Simon Horman
  2026-08-06  7:48     ` Jijie Shao
  0 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2026-08-05 16:52 UTC (permalink / raw)
  To: shaojijie
  Cc: Simon Horman, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=2
---
--- Patch [2]: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter ---
commit c77e6cbb6d302d584ade8cd89a76f4750fe00694
Author: Jijie Shao <shaojijie@huawei.com>

net: hns3: add missing const qualifier to hclge_log_error() reg parameter

This adds a const qualifier to the reg parameter of hclge_log_error() in
the hns3 PF error handling code. The parameter is only read inside the
function, so the change itself is a straightforward const-correctness
cleanup.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> index 7e124e2c718da..6093a60d257b1 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> @@ -1762,7 +1762,7 @@ static const struct hclge_hw_type_id hclge_hw_type_id_st[] = {
>  	},
>  };
>  
> -static void hclge_log_error(struct device *dev, char *reg,
> +static void hclge_log_error(struct device *dev, const char *reg,
>  			    const struct hclge_hw_error *err,
>  			    u32 err_sts, unsigned long *reset_requests)
>  {

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
This isn't a bug in the code, but is the justification in the changelog
accurate?  The commit message says:

    Callers pass const strings, causing a compiler warning about
    discarding the 'const' qualifier.

hclge_log_error() is static, and every call site is in hclge_err.c.  They
all pass a bare string literal, for example in hclge_handle_mpf_ras_error():

    hclge_log_error(dev, "IMP_TCM_ECC_INT_STS",
                    &hclge_imp_tcm_ecc_int[0], status,
                    &ae_dev->hw_err_reset_req);

and similarly for "SSU_ECC_MULTI_BIT_INT_0" and the remaining register
names down to "PPU_PF_ABNORMAL_INT_ST" in the MSI-X handlers.

In C a string literal has type char[N] rather than const char[N], so
passing one to a char *reg parameter does not discard a qualifier, and the
kernel build does not enable -Wwrite-strings.  None of the call sites pass
a value held in a const char * lvalue (such as
struct hclge_mod_reg_info::reg_name), which is the case that would produce
the described diagnostic.

Could the changelog be reworded to describe this as a const-correctness
cleanup instead of a build warning fix?  As written it suggests a broken
build and reads like a stable backport candidate.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout
  2026-08-04 13:09 ` [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout Jijie Shao
@ 2026-08-05 16:52   ` Simon Horman
  2026-08-06 11:39     ` Jijie Shao
  0 siblings, 1 reply; 12+ messages in thread
From: Simon Horman @ 2026-08-05 16:52 UTC (permalink / raw)
  To: shaojijie
  Cc: Simon Horman, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=3
---
--- Patch [3]: [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout ---
commit 0b583f65b44c7357ff4dab7e2ce6b5d624acc178
Author: Jian Shen <shenjian15@huawei.com>

net: hns3: use txqueue parameter directly in ndo_tx_timeout

This changes hns3_nic_net_timeout() to pass the txqueue index supplied by
the ndo_tx_timeout callback down to hns3_get_tx_timeo_queue_info() instead
of rescanning every TX queue with hns3_get_timeout_queue().  The old
scanning loop is turned into hns3_dump_timeout_queue(), which now only
prints DQL and queue state information for the given queue.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 6ecb32e28e79a..d0656289d68e5 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2825,32 +2825,25 @@ static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu)
>  	return ret;
>  }
>  
> -static int hns3_get_timeout_queue(struct net_device *ndev)
> +static void hns3_dump_timeout_queue(struct net_device *ndev,
> +				    unsigned int txqueue)
>  {
> -	unsigned int i;
> -
> -	/* Find the stopped queue the same way the stack does */
> -	for (i = 0; i < ndev->num_tx_queues; i++) {
> -		unsigned int timedout_ms;
> -		struct netdev_queue *q;
> +	unsigned int timedout_ms;
> +	struct netdev_queue *q;
>  
> -		q = netdev_get_tx_queue(ndev, i);
> -		timedout_ms = netif_xmit_timeout_ms(q);
> -		if (timedout_ms) {
> +	q = netdev_get_tx_queue(ndev, txqueue);
> +	timedout_ms = netif_xmit_timeout_ms(q);
> +	if (timedout_ms) {

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
Was the change in meaning of the netif_xmit_timeout_ms() test intentional?

Before this patch the return value of that test decided whether the driver
would act at all.  hns3_get_timeout_queue() only returned an index for a
queue whose netif_xmit_timeout_ms() was still non-zero, otherwise it
returned ndev->num_tx_queues and hns3_get_tx_timeo_queue_info() returned
false, so hns3_nic_net_timeout() skipped reset_event() and left
priv->tx_timeout_count alone.

After the patch that same test only gates the two netdev_info() prints in
hns3_dump_timeout_queue(), and the caller continues on regardless.

The stall really can clear between the watchdog detecting it and this
callback running.  netif_xmit_timeout_ms() -> netif_xmit_stopped() only
tests QUEUE_STATE_ANY_XOFF, while netif_freeze_queues() in dev_watchdog()
sets only __QUEUE_STATE_FROZEN, and hns3_clean_tx_ring() clears DRV_XOFF
from NAPI context without dev->tx_global_lock:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_clean_tx_ring() {
	...
	if (netif_tx_queue_stopped(dev_queue) &&
	    !test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
		netif_tx_wake_queue(dev_queue);
	...
}

In that window, does this now request an hclge reset_event() (function or
port reset, so a link flap) for a queue that has already recovered, where
previously it just logged and returned?

>  #ifdef CONFIG_BQL
> -			struct dql *dql = &q->dql;
> +		struct dql *dql = &q->dql;
>  
> -			netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
> -				    dql->last_obj_cnt, dql->num_queued,
> -				    dql->adj_limit, dql->num_completed);
> +		netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
> +			    dql->last_obj_cnt, dql->num_queued,
> +			    dql->adj_limit, dql->num_completed);
>  #endif
> -			netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
> -				    q->state, timedout_ms);
> -			break;
> -		}
> +		netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
> +			    q->state, timedout_ms);
>  	}
> -
> -	return i;
>  }
>  
>  static void hns3_dump_queue_stats(struct net_device *ndev,
> @@ -2900,25 +2893,25 @@ static void hns3_dump_queue_reg(struct net_device *ndev,
>  				      HNS3_RING_TX_RING_EBD_OFFSET_REG));
>  }
>  
> -static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
> +static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev,
> +					 unsigned int txqueue)
>  {
>  	struct hns3_nic_priv *priv = netdev_priv(ndev);
>  	struct hnae3_handle *h = hns3_get_handle(ndev);
>  	struct hns3_enet_ring *tx_ring;
> -	u32 timeout_queue;
>  
> -	timeout_queue = hns3_get_timeout_queue(ndev);
> -	if (timeout_queue >= ndev->num_tx_queues) {
> +	if (txqueue >= ndev->num_tx_queues) {
>  		netdev_info(ndev,
>  			    "no netdev TX timeout queue found, timeout count: %llu\n",
>  			    priv->tx_timeout_count);
>  		return false;
>  	}

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
With the index now coming from the caller, can this branch still be taken?
dev_watchdog() in net/sched/sch_generic.c is the only in-tree caller of
ndo_tx_timeout and it always passes an i it obtained from a loop bounded by
dev->num_tx_queues:

net/sched/sch_generic.c:dev_watchdog() {
	...
			netif_freeze_queues(dev);
			dev->netdev_ops->ndo_tx_timeout(dev, i);
	...
}

If so, the "no netdev TX timeout queue found" message is unreachable and its
wording no longer matches what the test checks.  Would it be worth either
dropping it or rewording it as an index sanity check?

Also, the commit message only mentions "Use it directly instead of iterating
all tx queues to find the timed out one" and does not mention that the
re-check no longer gates the reset request.  Could that behaviour change be
described there?

> +	hns3_dump_timeout_queue(ndev, txqueue);
>  
>  	priv->tx_timeout_count++;
>  
> -	tx_ring = &priv->ring[timeout_queue];
> -	hns3_dump_queue_stats(ndev, tx_ring, timeout_queue);
> +	tx_ring = &priv->ring[txqueue];
> +	hns3_dump_queue_stats(ndev, tx_ring, txqueue);

[Severity: High]
[Finding: main-2-1]
[Sources: opus-5]
This isn't a bug introduced by this patch, since the removed
hns3_get_timeout_queue() loop used the same bound and the same check before
indexing priv->ring[], but since the validation is being rewritten here it
may be a good spot to fix it.

Is ndev->num_tx_queues the right bound for indexing priv->ring[]?
ndev->num_tx_queues is the hardware maximum fixed at probe time in
hns3_client_init():

	netdev = alloc_etherdev_mq(sizeof(struct hns3_nic_priv), alloc_tqps);

while priv->ring is sized from h->kinfo.num_tqps in hns3_get_ring_config(),
and only the first num_tqps entries are initialized:

	priv->ring = devm_kzalloc(&pdev->dev,
				  array3_size(h->kinfo.num_tqps,
					      sizeof(*priv->ring), 2),
				  GFP_KERNEL);
	...
	for (i = 0; i < h->kinfo.num_tqps; i++)
		hns3_queue_to_ring(h->kinfo.tqp[i], priv);

kinfo.num_tqps equals real_num_tx_queues (see hns3_nic_set_real_num_queue())
and can be smaller than alloc_tqps, for example with fewer CPUs than TQPs or
after ethtool -L.

dev_watchdog() scans dev->num_tx_queues rather than real_num_tx_queues:

net/sched/sch_generic.c:dev_watchdog() {
	...
		for (i = 0; i < dev->num_tx_queues; i++) {
			struct netdev_queue *txq;

			txq = netdev_get_tx_queue(dev, i);
			if (!netif_xmit_stopped(txq))
				continue;
	...
}

and hns3_link_status_change() marks all num_tx_queues queues stopped without
dev->tx_global_lock:

	netif_carrier_off(netdev);
	netif_tx_stop_all_queues(netdev);

A link-down concurrent with a watchdog scan can therefore hand back an index
in [kinfo.num_tqps, num_tx_queues), since those queues have never
transmitted and have trans_start == 0.  Does that then read past the
devm_kzalloc'ed ring array and dereference a garbage tx_ring->tqp_vector, in
hns3_dump_queue_stats():

	struct napi_struct *napi = &tx_ring->tqp_vector->napi;

and in hns3_dump_queue_reg():

		    readl(tx_ring->tqp_vector->mask_addr));

Would ndev->real_num_tx_queues (or h->kinfo.num_tqps) be the correct bound
for the check above?

>  
>  	/* When mac received many pause frames continuous, it's unable to send
>  	 * packets, which may cause tx timeout
> @@ -2941,7 +2934,7 @@ static void hns3_nic_net_timeout(struct net_device *ndev, unsigned int txqueue)
>  	struct hns3_nic_priv *priv = netdev_priv(ndev);
>  	struct hnae3_handle *h = priv->ae_handle;
>  
> -	if (!hns3_get_tx_timeo_queue_info(ndev))
> +	if (!hns3_get_tx_timeo_queue_info(ndev, txqueue))
>  		return;
>  
>  	/* request the reset, and let the hclge to determine

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
  2026-08-05 16:52   ` Simon Horman
@ 2026-08-06  7:32     ` Jijie Shao
  2026-08-06 11:26       ` Simon Horman
  0 siblings, 1 reply; 12+ messages in thread
From: Jijie Shao @ 2026-08-06  7:32 UTC (permalink / raw)
  To: Simon Horman
  Cc: shaojijie, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel


on 2026/8/6 0:52, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=1
> ---
> --- Patch [1]: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() ---
> commit ba3e282274aa0911fb278f48937385c06cc8f3a1
> Author: Jian Shen <shenjian15@huawei.com>
>
> net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
>
> This adds msg->desc = NULL after the kfree(msg->desc) at the end of
> hclge_query_reg_info() in the hns3 PF error handling code, described as
> avoiding a dangling pointer in a struct that the caller reuses.
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> index dac051e798da2..7e124e2c718da 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> @@ -1592,6 +1592,7 @@ hclge_query_reg_info(struct hclge_dev *hdev,
>>   	}
>>   
>>   	kfree(msg->desc);
>> +	msg->desc = NULL;
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5]
> Is the commit message description accurate here?  It says:
>
>      In hclge_query_reg_info(), msg->desc is freed by kfree(), but the
>      caller continues to use msg across loop iterations. Set msg->desc
>      to NULL to avoid leaving a dangling pointer in the reused struct.
>
> The only caller is hclge_query_reg_info_of_ssu(), and it re-initializes the
> whole stack struct from the static template at the top of every iteration:
>
> 	for (i = 0; i < num; i++) {
> 		msg = hclge_ssu_reg_common_msg[i];
> 		...
> 		hclge_query_reg_info(hdev, &msg, loop_time, loop_para);
> 	}
>
> None of the hclge_ssu_reg_common_msg[] entries initialize .desc, so msg.desc
> is NULL again on each pass.
>
> Even without that, hclge_query_reg_info() overwrites msg->desc before any
> read of it:
>
> 	msg->desc = kzalloc(desc_len, GFP_KERNEL);
> 	if (!msg->desc) {
> 		...
> 		return;
> 	}
>
> so the freed pointer is never dereferenced or freed a second time.
>
> Would it be worth rewording the message to describe this as defensive
> cleanup rather than a dangling pointer fix, so a stable backporter does not
> read it as a use-after-free fix?  The patch correctly carries no Fixes: tag.

Keeping the message. kfree'd pointer is factually dangling; the patch
is defensive. No Fixes: tag already signals it's not a UAF fix.

Thanks,
Jijie Shao


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter
  2026-08-05 16:52   ` Simon Horman
@ 2026-08-06  7:48     ` Jijie Shao
  2026-08-06 11:21       ` Simon Horman
  0 siblings, 1 reply; 12+ messages in thread
From: Jijie Shao @ 2026-08-06  7:48 UTC (permalink / raw)
  To: Simon Horman
  Cc: shaojijie, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel


on 2026/8/6 0:52, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=2
> ---
> --- Patch [2]: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter ---
> commit c77e6cbb6d302d584ade8cd89a76f4750fe00694
> Author: Jijie Shao <shaojijie@huawei.com>
>
> net: hns3: add missing const qualifier to hclge_log_error() reg parameter
>
> This adds a const qualifier to the reg parameter of hclge_log_error() in
> the hns3 PF error handling code. The parameter is only read inside the
> function, so the change itself is a straightforward const-correctness
> cleanup.
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> index 7e124e2c718da..6093a60d257b1 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
>> @@ -1762,7 +1762,7 @@ static const struct hclge_hw_type_id hclge_hw_type_id_st[] = {
>>   	},
>>   };
>>   
>> -static void hclge_log_error(struct device *dev, char *reg,
>> +static void hclge_log_error(struct device *dev, const char *reg,
>>   			    const struct hclge_hw_error *err,
>>   			    u32 err_sts, unsigned long *reset_requests)
>>   {
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5]
> This isn't a bug in the code, but is the justification in the changelog
> accurate?  The commit message says:
>
>      Callers pass const strings, causing a compiler warning about
>      discarding the 'const' qualifier.
>
> hclge_log_error() is static, and every call site is in hclge_err.c.  They
> all pass a bare string literal, for example in hclge_handle_mpf_ras_error():
>
>      hclge_log_error(dev, "IMP_TCM_ECC_INT_STS",
>                      &hclge_imp_tcm_ecc_int[0], status,
>                      &ae_dev->hw_err_reset_req);
>
> and similarly for "SSU_ECC_MULTI_BIT_INT_0" and the remaining register
> names down to "PPU_PF_ABNORMAL_INT_ST" in the MSI-X handlers.
>
> In C a string literal has type char[N] rather than const char[N], so
> passing one to a char *reg parameter does not discard a qualifier, and the
> kernel build does not enable -Wwrite-strings.  None of the call sites pass
> a value held in a const char * lvalue (such as
> struct hclge_mod_reg_info::reg_name), which is the case that would produce
> the described diagnostic.
>
> Could the changelog be reworded to describe this as a const-correctness
> cleanup instead of a build warning fix?  As written it suggests a broken
> build and reads like a stable backport candidate.

The warning is real in our internal build, which enables
-Wwrite-strings. Would it be acceptable to keep the commit message
as-is, or should I reword it to const-correctness cleanup and note
the internal build context in the changelog?

This series is cleanup, not bugfix — no Fixes: tags, not intended for stable.

Thanks,
Jijie Shao



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter
  2026-08-06  7:48     ` Jijie Shao
@ 2026-08-06 11:21       ` Simon Horman
  0 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2026-08-06 11:21 UTC (permalink / raw)
  To: Jijie Shao
  Cc: davem, edumazet, kuba, pabeni, andrew+netdev, shenjian15,
	liuyonglong, chenhao418, yangshuaisong, ningwei15, netdev,
	linux-kernel

On Thu, Aug 06, 2026 at 03:48:20PM +0800, Jijie Shao wrote:
> 
> on 2026/8/6 0:52, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=2
> > ---
> > --- Patch [2]: [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter ---
> > commit c77e6cbb6d302d584ade8cd89a76f4750fe00694
> > Author: Jijie Shao <shaojijie@huawei.com>
> > 
> > net: hns3: add missing const qualifier to hclge_log_error() reg parameter
> > 
> > This adds a const qualifier to the reg parameter of hclge_log_error() in
> > the hns3 PF error handling code. The parameter is only read inside the
> > function, so the change itself is a straightforward const-correctness
> > cleanup.
> > 
> > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > index 7e124e2c718da..6093a60d257b1 100644
> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > @@ -1762,7 +1762,7 @@ static const struct hclge_hw_type_id hclge_hw_type_id_st[] = {
> > >   	},
> > >   };
> > > -static void hclge_log_error(struct device *dev, char *reg,
> > > +static void hclge_log_error(struct device *dev, const char *reg,
> > >   			    const struct hclge_hw_error *err,
> > >   			    u32 err_sts, unsigned long *reset_requests)
> > >   {
> > [Severity: Low]
> > [Finding: main-1-0]
> > [Sources: opus-5]
> > This isn't a bug in the code, but is the justification in the changelog
> > accurate?  The commit message says:
> > 
> >      Callers pass const strings, causing a compiler warning about
> >      discarding the 'const' qualifier.
> > 
> > hclge_log_error() is static, and every call site is in hclge_err.c.  They
> > all pass a bare string literal, for example in hclge_handle_mpf_ras_error():
> > 
> >      hclge_log_error(dev, "IMP_TCM_ECC_INT_STS",
> >                      &hclge_imp_tcm_ecc_int[0], status,
> >                      &ae_dev->hw_err_reset_req);
> > 
> > and similarly for "SSU_ECC_MULTI_BIT_INT_0" and the remaining register
> > names down to "PPU_PF_ABNORMAL_INT_ST" in the MSI-X handlers.
> > 
> > In C a string literal has type char[N] rather than const char[N], so
> > passing one to a char *reg parameter does not discard a qualifier, and the
> > kernel build does not enable -Wwrite-strings.  None of the call sites pass
> > a value held in a const char * lvalue (such as
> > struct hclge_mod_reg_info::reg_name), which is the case that would produce
> > the described diagnostic.
> > 
> > Could the changelog be reworded to describe this as a const-correctness
> > cleanup instead of a build warning fix?  As written it suggests a broken
> > build and reads like a stable backport candidate.
> 
> The warning is real in our internal build, which enables
> -Wwrite-strings. Would it be acceptable to keep the commit message
> as-is, or should I reword it to const-correctness cleanup and note
> the internal build context in the changelog?
> 
> This series is cleanup, not bugfix — no Fixes: tags, not intended for stable.

Thanks, I agree with your analysis.
Sorry for the noise on this one.

Reviewed-by: Simon Horman <horms@kernel.org>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
  2026-08-06  7:32     ` Jijie Shao
@ 2026-08-06 11:26       ` Simon Horman
  0 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2026-08-06 11:26 UTC (permalink / raw)
  To: Jijie Shao
  Cc: davem, edumazet, kuba, pabeni, andrew+netdev, shenjian15,
	liuyonglong, chenhao418, yangshuaisong, ningwei15, netdev,
	linux-kernel

On Thu, Aug 06, 2026 at 03:32:23PM +0800, Jijie Shao wrote:
> 
> on 2026/8/6 0:52, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=1
> > ---
> > --- Patch [1]: [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() ---
> > commit ba3e282274aa0911fb278f48937385c06cc8f3a1
> > Author: Jian Shen <shenjian15@huawei.com>
> > 
> > net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info()
> > 
> > This adds msg->desc = NULL after the kfree(msg->desc) at the end of
> > hclge_query_reg_info() in the hns3 PF error handling code, described as
> > avoiding a dangling pointer in a struct that the caller reuses.
> > 
> > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > index dac051e798da2..7e124e2c718da 100644
> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
> > > @@ -1592,6 +1592,7 @@ hclge_query_reg_info(struct hclge_dev *hdev,
> > >   	}
> > >   	kfree(msg->desc);
> > > +	msg->desc = NULL;
> > [Severity: Low]
> > [Finding: main-1-0]
> > [Sources: opus-5]
> > Is the commit message description accurate here?  It says:
> > 
> >      In hclge_query_reg_info(), msg->desc is freed by kfree(), but the
> >      caller continues to use msg across loop iterations. Set msg->desc
> >      to NULL to avoid leaving a dangling pointer in the reused struct.
> > 
> > The only caller is hclge_query_reg_info_of_ssu(), and it re-initializes the
> > whole stack struct from the static template at the top of every iteration:
> > 
> > 	for (i = 0; i < num; i++) {
> > 		msg = hclge_ssu_reg_common_msg[i];
> > 		...
> > 		hclge_query_reg_info(hdev, &msg, loop_time, loop_para);
> > 	}
> > 
> > None of the hclge_ssu_reg_common_msg[] entries initialize .desc, so msg.desc
> > is NULL again on each pass.
> > 
> > Even without that, hclge_query_reg_info() overwrites msg->desc before any
> > read of it:
> > 
> > 	msg->desc = kzalloc(desc_len, GFP_KERNEL);
> > 	if (!msg->desc) {
> > 		...
> > 		return;
> > 	}
> > 
> > so the freed pointer is never dereferenced or freed a second time.
> > 
> > Would it be worth rewording the message to describe this as defensive
> > cleanup rather than a dangling pointer fix, so a stable backporter does not
> > read it as a use-after-free fix?  The patch correctly carries no Fixes: tag.
> 
> Keeping the message. kfree'd pointer is factually dangling; the patch
> is defensive. No Fixes: tag already signals it's not a UAF fix.

Thanks, sorry for the noise on this one too.
I agree that in the context of hardening this change makes sense.

Reviewed-by: Simon Horman <horms@kernel.org>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout
  2026-08-05 16:52   ` Simon Horman
@ 2026-08-06 11:39     ` Jijie Shao
  0 siblings, 0 replies; 12+ messages in thread
From: Jijie Shao @ 2026-08-06 11:39 UTC (permalink / raw)
  To: Simon Horman
  Cc: shaojijie, davem, edumazet, kuba, pabeni, andrew+netdev,
	shenjian15, liuyonglong, chenhao418, yangshuaisong, ningwei15,
	netdev, linux-kernel


on 2026/8/6 0:52, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804130933.880552-2-shaojijie@huawei.com?part=3
> ---
> --- Patch [3]: [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout ---
> commit 0b583f65b44c7357ff4dab7e2ce6b5d624acc178
> Author: Jian Shen <shenjian15@huawei.com>
>
> net: hns3: use txqueue parameter directly in ndo_tx_timeout
>
> This changes hns3_nic_net_timeout() to pass the txqueue index supplied by
> the ndo_tx_timeout callback down to hns3_get_tx_timeo_queue_info() instead
> of rescanning every TX queue with hns3_get_timeout_queue().  The old
> scanning loop is turned into hns3_dump_timeout_queue(), which now only
> prints DQL and queue state information for the given queue.
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 6ecb32e28e79a..d0656289d68e5 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -2825,32 +2825,25 @@ static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu)
>>   	return ret;
>>   }
>>   
>> -static int hns3_get_timeout_queue(struct net_device *ndev)
>> +static void hns3_dump_timeout_queue(struct net_device *ndev,
>> +				    unsigned int txqueue)
>>   {
>> -	unsigned int i;
>> -
>> -	/* Find the stopped queue the same way the stack does */
>> -	for (i = 0; i < ndev->num_tx_queues; i++) {
>> -		unsigned int timedout_ms;
>> -		struct netdev_queue *q;
>> +	unsigned int timedout_ms;
>> +	struct netdev_queue *q;
>>   
>> -		q = netdev_get_tx_queue(ndev, i);
>> -		timedout_ms = netif_xmit_timeout_ms(q);
>> -		if (timedout_ms) {
>> +	q = netdev_get_tx_queue(ndev, txqueue);
>> +	timedout_ms = netif_xmit_timeout_ms(q);
>> +	if (timedout_ms) {
> [Severity: Medium]
> [Finding: main-1-0]
> [Sources: opus-5]
> Was the change in meaning of the netif_xmit_timeout_ms() test intentional?
>
> Before this patch the return value of that test decided whether the driver
> would act at all.  hns3_get_timeout_queue() only returned an index for a
> queue whose netif_xmit_timeout_ms() was still non-zero, otherwise it
> returned ndev->num_tx_queues and hns3_get_tx_timeo_queue_info() returned
> false, so hns3_nic_net_timeout() skipped reset_event() and left
> priv->tx_timeout_count alone.
>
> After the patch that same test only gates the two netdev_info() prints in
> hns3_dump_timeout_queue(), and the caller continues on regardless.
>
> The stall really can clear between the watchdog detecting it and this
> callback running.  netif_xmit_timeout_ms() -> netif_xmit_stopped() only
> tests QUEUE_STATE_ANY_XOFF, while netif_freeze_queues() in dev_watchdog()
> sets only __QUEUE_STATE_FROZEN, and hns3_clean_tx_ring() clears DRV_XOFF
> from NAPI context without dev->tx_global_lock:
>
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_clean_tx_ring() {
> 	...
> 	if (netif_tx_queue_stopped(dev_queue) &&
> 	    !test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
> 		netif_tx_wake_queue(dev_queue);
> 	...
> }
>
> In that window, does this now request an hclge reset_event() (function or
> port reset, so a link flap) for a queue that has already recovered, where
> previously it just logged and returned?
>
>>   #ifdef CONFIG_BQL
>> -			struct dql *dql = &q->dql;
>> +		struct dql *dql = &q->dql;
>>   
>> -			netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
>> -				    dql->last_obj_cnt, dql->num_queued,
>> -				    dql->adj_limit, dql->num_completed);
>> +		netdev_info(ndev, "DQL info last_cnt: %u, queued: %u, adj_limit: %u, completed: %u\n",
>> +			    dql->last_obj_cnt, dql->num_queued,
>> +			    dql->adj_limit, dql->num_completed);
>>   #endif
>> -			netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
>> -				    q->state, timedout_ms);
>> -			break;
>> -		}
>> +		netdev_info(ndev, "queue state: 0x%lx, delta msecs: %u\n",
>> +			    q->state, timedout_ms);
>>   	}
>> -
>> -	return i;
>>   }
>>   
>>   static void hns3_dump_queue_stats(struct net_device *ndev,
>> @@ -2900,25 +2893,25 @@ static void hns3_dump_queue_reg(struct net_device *ndev,
>>   				      HNS3_RING_TX_RING_EBD_OFFSET_REG));
>>   }
>>   
>> -static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev)
>> +static bool hns3_get_tx_timeo_queue_info(struct net_device *ndev,
>> +					 unsigned int txqueue)
>>   {
>>   	struct hns3_nic_priv *priv = netdev_priv(ndev);
>>   	struct hnae3_handle *h = hns3_get_handle(ndev);
>>   	struct hns3_enet_ring *tx_ring;
>> -	u32 timeout_queue;
>>   
>> -	timeout_queue = hns3_get_timeout_queue(ndev);
>> -	if (timeout_queue >= ndev->num_tx_queues) {
>> +	if (txqueue >= ndev->num_tx_queues) {
>>   		netdev_info(ndev,
>>   			    "no netdev TX timeout queue found, timeout count: %llu\n",
>>   			    priv->tx_timeout_count);
>>   		return false;
>>   	}
> [Severity: Medium]
> [Finding: main-1-0]
> [Sources: opus-5]
> With the index now coming from the caller, can this branch still be taken?
> dev_watchdog() in net/sched/sch_generic.c is the only in-tree caller of
> ndo_tx_timeout and it always passes an i it obtained from a loop bounded by
> dev->num_tx_queues:
>
> net/sched/sch_generic.c:dev_watchdog() {
> 	...
> 			netif_freeze_queues(dev);
> 			dev->netdev_ops->ndo_tx_timeout(dev, i);
> 	...
> }
>
> If so, the "no netdev TX timeout queue found" message is unreachable and its
> wording no longer matches what the test checks.  Would it be worth either
> dropping it or rewording it as an index sanity check?
>
> Also, the commit message only mentions "Use it directly instead of iterating
> all tx queues to find the timed out one" and does not mention that the
> re-check no longer gates the reset request.  Could that behaviour change be
> described there?
>
>> +	hns3_dump_timeout_queue(ndev, txqueue);
>>   
>>   	priv->tx_timeout_count++;
>>   
>> -	tx_ring = &priv->ring[timeout_queue];
>> -	hns3_dump_queue_stats(ndev, tx_ring, timeout_queue);
>> +	tx_ring = &priv->ring[txqueue];
>> +	hns3_dump_queue_stats(ndev, tx_ring, txqueue);
> [Severity: High]
> [Finding: main-2-1]
> [Sources: opus-5]
> This isn't a bug introduced by this patch, since the removed
> hns3_get_timeout_queue() loop used the same bound and the same check before
> indexing priv->ring[], but since the validation is being rewritten here it
> may be a good spot to fix it.
>
> Is ndev->num_tx_queues the right bound for indexing priv->ring[]?
> ndev->num_tx_queues is the hardware maximum fixed at probe time in
> hns3_client_init():
>
> 	netdev = alloc_etherdev_mq(sizeof(struct hns3_nic_priv), alloc_tqps);
>
> while priv->ring is sized from h->kinfo.num_tqps in hns3_get_ring_config(),
> and only the first num_tqps entries are initialized:
>
> 	priv->ring = devm_kzalloc(&pdev->dev,
> 				  array3_size(h->kinfo.num_tqps,
> 					      sizeof(*priv->ring), 2),
> 				  GFP_KERNEL);
> 	...
> 	for (i = 0; i < h->kinfo.num_tqps; i++)
> 		hns3_queue_to_ring(h->kinfo.tqp[i], priv);
>
> kinfo.num_tqps equals real_num_tx_queues (see hns3_nic_set_real_num_queue())
> and can be smaller than alloc_tqps, for example with fewer CPUs than TQPs or
> after ethtool -L.
>
> dev_watchdog() scans dev->num_tx_queues rather than real_num_tx_queues:
>
> net/sched/sch_generic.c:dev_watchdog() {
> 	...
> 		for (i = 0; i < dev->num_tx_queues; i++) {
> 			struct netdev_queue *txq;
>
> 			txq = netdev_get_tx_queue(dev, i);
> 			if (!netif_xmit_stopped(txq))
> 				continue;
> 	...
> }
>
> and hns3_link_status_change() marks all num_tx_queues queues stopped without
> dev->tx_global_lock:
>
> 	netif_carrier_off(netdev);
> 	netif_tx_stop_all_queues(netdev);
>
> A link-down concurrent with a watchdog scan can therefore hand back an index
> in [kinfo.num_tqps, num_tx_queues), since those queues have never
> transmitted and have trans_start == 0.  Does that then read past the
> devm_kzalloc'ed ring array and dereference a garbage tx_ring->tqp_vector, in
> hns3_dump_queue_stats():
>
> 	struct napi_struct *napi = &tx_ring->tqp_vector->napi;
>
> and in hns3_dump_queue_reg():
>
> 		    readl(tx_ring->tqp_vector->mask_addr));
>
> Would ndev->real_num_tx_queues (or h->kinfo.num_tqps) be the correct bound
> for the check above?

Regarding the bound check on priv->ring[]:
This is not introduced by this patch — the removed hns3_get_timeout_queue()
loop used the same bound (num_tx_queues)
and the same check before indexing priv->ring[].
                       
We have not encountered the OOB issue internally. Preliminary
analysis suggests it is not reachable in current code:

- dev_watchdog() is gated by netif_carrier_ok(), and
hns3_link_status_change() sets carrier_off before
netif_tx_stop_all_queues() on link down.
- netif_tx_stop_queue() pairs trans_start=jiffies with XOFF, so
time_after(jiffies, trans_start + watchdog_timeo) is false right
after stop.

I will analyze the TOCTOU path in more detail myself. If a real
issue exists, it will be addressed by a separate bugfix.



Regarding the unreachable "no netdev TX timeout queue found" branch
and the now-redundant re-check:

dev_watchdog() always passes a valid index bounded by num_tx_queues,
so the branch is unreachable. The re-check was incidental to the
scanning loop, not an intentional gate. v2 will drop both.

Thanks,
Jijie Shao



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-08-06 11:39 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-04 13:09 [PATCH net-next 0/3] net: hns3: some cleanups for hns3 driver Jijie Shao
2026-08-04 13:09 ` [PATCH net-next 1/3] net: hns3: set msg->desc to NULL after kfree in hclge_query_reg_info() Jijie Shao
2026-08-05 16:52   ` Simon Horman
2026-08-06  7:32     ` Jijie Shao
2026-08-06 11:26       ` Simon Horman
2026-08-04 13:09 ` [PATCH net-next 2/3] net: hns3: add missing const qualifier to hclge_log_error() reg parameter Jijie Shao
2026-08-05 16:52   ` Simon Horman
2026-08-06  7:48     ` Jijie Shao
2026-08-06 11:21       ` Simon Horman
2026-08-04 13:09 ` [PATCH net-next 3/3] net: hns3: use txqueue parameter directly in ndo_tx_timeout Jijie Shao
2026-08-05 16:52   ` Simon Horman
2026-08-06 11:39     ` Jijie Shao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).