[PATCH net 0/3] net: hns3: three fixes for hns3 driver

[PATCH net 0/3] net: hns3: three fixes for hns3 driver

[Jijie Shao]

This series contains three bug fixes for the HNS3 driver.

Patch 1 fixes a vector resource leak in the error path of
hns3_nic_alloc_vector_data, where a devm_kcalloc failure after
successful get_vector() leaves allocated vectors unreleased.

Patch 2 fixes an interface stall that occurs when the hardware RX
queue is full and memory allocation fails.  Without an RX interrupt,
NAPI is never scheduled and the queue cannot drain.  A delayed work
re-arms NAPI polling on affected vectors to break the deadlock.

Patch 3 fixes a use-after-free in debugfs read callbacks that can
access priv->ring and ring->desc while reset, driver unload, or
ethtool ring resize is freeing those resources.  A mutex is added
to synchronize debugfs readers against resource teardown.

Jijie Shao (3):
  net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data
  net: hns3: fix interface stuck after OOM with full hardware rx queue
  net: hns3: fix use-after-free in debugfs read during reset/unload

 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |  1 +
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 95 +++++++++++--------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 68 ++++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 17 ++++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  2 +
 .../hisilicon/hns3/hns3pf/hclge_debugfs.c     | 14 +++
 6 files changed, 154 insertions(+), 43 deletions(-)


base-commit: 78445023439506ebd83b86d40b1e428a3b309d4a
-- 
2.43.0

[PATCH net 1/3] net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

[Jijie Shao]

After h->ae_algo->ops->get_vector() has acquired MSI-X vectors, a
subsequent devm_kcalloc failure for priv->tqp_vector jumps to 'out'
and returns without calling put_vector() to release them. The
underlying hclge/hclgevf bookkeeping (num_msi_used, num_msi_left,
vector_status[]) is left inconsistent: the vectors are marked as in
use but hns3_enet never tracks or releases them.

On the reset path this leak accumulates across failed reset attempts,
steadily reducing num_msi_left until get_vector() can no longer
satisfy the requested count, at which point the interface fails to
recover.

Call put_vector() for every vector acquired by get_vector() before
returning an error, and reset priv->vector_num so the dealloc path
cannot dereference a NULL priv->tqp_vector.

Fixes: dd38c72604dc ("net: hns3: fix for coalesce configuration lost during reset")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 47788be64be6..2f3aeade558c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -4854,7 +4854,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 			     GFP_KERNEL);
 	if (!priv->tqp_vector) {
 		ret = -ENOMEM;
-		goto out;
+		goto err_put_vector;
 	}
 
 	for (i = 0; i < priv->vector_num; i++) {
@@ -4865,7 +4865,13 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 		hns3_vector_coalesce_init(tqp_vector, priv);
 	}
 
-out:
+	devm_kfree(&pdev->dev, vector);
+	return 0;
+
+err_put_vector:
+	for (i = 0; i < vector_num; i++)
+		h->ae_algo->ops->put_vector(h, vector[i].vector);
+	priv->vector_num = 0;
 	devm_kfree(&pdev->dev, vector);
 	return ret;
 }
-- 
2.43.0

[PATCH net 2/3] net: hns3: fix interface stuck after OOM with full hardware rx queue

[Jijie Shao]

NAPI polling on hns3 is driven by hardware RX interrupts. Under
memory pressure, RX skb and frag allocations fail, so packets are
no longer reaped from the hardware rx ring. Once the ring fills,
the hardware stops generating RX interrupts and NAPI is never
scheduled again -- even after memory becomes available. The
interface falls into a deadlock: draining the ring needs NAPI,
scheduling NAPI needs an interrupt, and the interrupt needs the
ring to be drained.

Break the deadlock by adding a software recovery path that re-arms
NAPI via a 250ms delayed_work, independent of hardware interrupts.
A per-vector bitmap tracks which vectors have rings in OOM state,
and the work function schedules NAPI for each affected vector.
OOM state is marked at every RX allocation failure site.

Also fix hns3_clean_rx_ring() to return recv_pkts instead of budget
on allocation failure, preventing NAPI busy-poll under low memory.
Add rx_oom_cnt ethtool counter for diagnostics.

Fixes: 81ae0e0491f3 ("net: hns3: Add skb chain when num of RX buf exceeds MAX_SKB_FRAGS")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 52 ++++++++++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 17 ++++++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  1 +
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 2f3aeade558c..0c088feae03c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -78,6 +78,8 @@ module_param(page_pool_enabled, bool, 0400);
 #define HNS3_MIN_TX_LEN		33U
 #define HNS3_MIN_TUN_PKT_LEN	65U
 
+#define HNS3_OOM_POLL_INTERVAL_MS	250
+
 /* hns3_pci_tbl - PCI Device ID Table
  *
  * Last entry must be all 0s
@@ -3793,6 +3795,7 @@ static int hns3_handle_rx_copybreak(struct sk_buff *skb, int i,
 
 		hns3_rl_err(ring_to_netdev(ring),
 			    "failed to allocate rx frag\n");
+		hns3_ring_set_oom_state(ring);
 		return -ENOMEM;
 	}
 
@@ -4162,6 +4165,7 @@ static int hns3_add_frag(struct hns3_enet_ring *ring)
 			if (unlikely(!new_skb)) {
 				hns3_rl_err(ring_to_netdev(ring),
 					    "alloc rx fraglist skb fail\n");
+				hns3_ring_set_oom_state(ring);
 				return -ENXIO;
 			}
 
@@ -4451,6 +4455,34 @@ static int hns3_handle_rx_bd(struct hns3_enet_ring *ring)
 	return 0;
 }
 
+static void hns3_oom_task(struct work_struct *work)
+{
+	struct hns3_nic_priv *priv = container_of(work, struct hns3_nic_priv,
+						  oom_task.work);
+	struct net_device *netdev = priv->netdev;
+	struct hnae3_handle *h = priv->ae_handle;
+	u16 i;
+
+	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state))
+		return;
+
+	netif_dbg(h, rx_err, netdev, "oom napi_schedule 0x%*pb\n",
+		  priv->vector_num, priv->oom_vector_bm);
+	for (i = 0; i < priv->vector_num; i++)
+		if (test_and_clear_bit(i, priv->oom_vector_bm))
+			napi_schedule(&priv->tqp_vector[i].napi);
+}
+
+static void hns3_oom_task_schedule(struct hns3_enet_ring *ring)
+{
+	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
+
+	hns3_ring_stats_update(ring, rx_oom_cnt);
+	hns3_ring_set_oom_state(ring);
+	schedule_delayed_work(&priv->oom_task,
+			      msecs_to_jiffies(HNS3_OOM_POLL_INTERVAL_MS));
+}
+
 int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
 		       void (*rx_fn)(struct hns3_enet_ring *, struct sk_buff *))
 {
@@ -4472,6 +4504,9 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
 
 		/* Poll one pkt */
 		err = hns3_handle_rx_bd(ring);
+		if (unlikely(err == -ENOMEM))
+			failure = true;
+
 		/* Do not get FE for the packet or failed to alloc skb */
 		if (unlikely(!ring->skb || err == -ENXIO)) {
 			goto out;
@@ -4493,7 +4528,10 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
 		failure = failure ||
 			  hns3_nic_alloc_rx_buffers(ring, unused_count);
 
-	return failure ? budget : recv_pkts;
+	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
+		hns3_oom_task_schedule(ring);
+
+	return recv_pkts;
 }
 
 static void hns3_update_rx_int_coalesce(struct hns3_enet_tqp_vector *tqp_vector)
@@ -4788,6 +4826,7 @@ static int hns3_nic_init_vector_data(struct hns3_nic_priv *priv)
 			       hns3_nic_common_poll);
 	}
 
+	INIT_DELAYED_WORK(&priv->oom_task, hns3_oom_task);
 	return 0;
 
 map_ring_fail:
@@ -4865,9 +4904,18 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 		hns3_vector_coalesce_init(tqp_vector, priv);
 	}
 
+	priv->oom_vector_bm = bitmap_zalloc(vector_num, GFP_KERNEL);
+	if (!priv->oom_vector_bm) {
+		ret = -ENOMEM;
+		goto err_free_tqp_vector;
+	}
+
 	devm_kfree(&pdev->dev, vector);
 	return 0;
 
+err_free_tqp_vector:
+	devm_kfree(&pdev->dev, priv->tqp_vector);
+	priv->tqp_vector = NULL;
 err_put_vector:
 	for (i = 0; i < vector_num; i++)
 		h->ae_algo->ops->put_vector(h, vector[i].vector);
@@ -4889,6 +4937,7 @@ static void hns3_nic_uninit_vector_data(struct hns3_nic_priv *priv)
 	struct hns3_enet_tqp_vector *tqp_vector;
 	int i;
 
+	cancel_delayed_work_sync(&priv->oom_task);
 	for (i = 0; i < priv->vector_num; i++) {
 		tqp_vector = &priv->tqp_vector[i];
 
@@ -4920,6 +4969,7 @@ static void hns3_nic_dealloc_vector_data(struct hns3_nic_priv *priv)
 	struct pci_dev *pdev = h->pdev;
 	int i, ret;
 
+	bitmap_free(priv->oom_vector_bm);
 	for (i = 0; i < priv->vector_num; i++) {
 		struct hns3_enet_tqp_vector *tqp_vector;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 933e3527ed82..27a09629cbca 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -449,6 +449,7 @@ struct ring_stats {
 			u64 non_reuse_pg;
 			u64 frag_alloc_err;
 			u64 frag_alloc;
+			u64 rx_oom_cnt;
 		};
 		__le16 csum;
 	};
@@ -585,6 +586,8 @@ struct hns3_nic_priv {
 	struct hns3_enet_tqp_vector *tqp_vector;
 	u16 vector_num;
 	u8 max_non_tso_bd_num;
+	struct delayed_work oom_task;
+	unsigned long *oom_vector_bm;
 
 	u64 tx_timeout_count;
 
@@ -711,6 +714,20 @@ static inline unsigned int hns3_page_order(struct hns3_enet_ring *ring)
 #define hns3_rl_usec_to_reg(int_rl) ((int_rl) >> 2)
 #define hns3_rl_round_down(int_rl) round_down(int_rl, 4)
 
+static inline void hns3_ring_set_oom_state(struct hns3_enet_ring *ring)
+{
+	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
+
+	set_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
+}
+
+static inline bool hns3_ring_is_oom_state(struct hns3_enet_ring *ring)
+{
+	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
+
+	return test_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
+}
+
 void hns3_ethtool_set_ops(struct net_device *netdev);
 int hns3_set_channels(struct net_device *netdev,
 		      struct ethtool_channels *ch);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index e7318f236315..392653635bda 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -62,6 +62,7 @@ static const struct hns3_stats hns3_rxq_stats[] = {
 	HNS3_TQP_STAT("non_reuse_pg", non_reuse_pg),
 	HNS3_TQP_STAT("frag_alloc_err", frag_alloc_err),
 	HNS3_TQP_STAT("frag_alloc", frag_alloc),
+	HNS3_TQP_STAT("rx_oom_cnt", rx_oom_cnt),
 };
 
 #define HNS3_PRIV_FLAGS_LEN ARRAY_SIZE(hns3_priv_flags)
-- 
2.43.0

Re: [PATCH net 2/3] net: hns3: fix interface stuck after OOM with full hardware rx queue

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 5 · Low: 1

- [Medium] The changelog misdescribes the pre-patch allocation-failure
  handling of hns3_clean_rx_ring() and hides that the patch removes the…
- [Medium] rx_oom_cnt is exported as a per-RX-queue counter (offset
  inside struct hns3_enet_ring, printed as rxqN_...), but the state that…
- [Medium] The new per-RX-queue allocation-failure counter is exported as
  a driver-private ethtool -S string (HNS3_TQP_STAT("rx_oom_cnt",…
- [Medium] In hns3_clean_rx_ring() the newly added `if (unlikely(err ==
  -ENOMEM)) failure = true;` (hns3_enet.c:4507-4508) poisons the pre-…
- [Medium] hns3_oom_task() returns early when HNS3_NIC_STATE_DOWN is set
  (hns3_enet.c:4466-4467) without ever reaching the…
- [Low] The per-vector OOM bit is read with hns3_ring_is_oom_state() at
  hns3_enet.c:4531 and then unconditionally re-set by…

--

net: hns3: fix interface stuck after OOM with full hardware rx queue

This patch adds a 250ms delayed_work that re-arms NAPI for vectors whose rx
rings hit an allocation failure, tracked in a new per-vector bitmap. It also
changes hns3_clean_rx_ring() to always return recv_pkts and adds a new
rx_oom_cnt ethtool counter.

A few questions below.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 2f3aeade558cd..0c088feae03ca 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c

[ ... ]

> @@ -4451,6 +4455,34 @@ static int hns3_handle_rx_bd(struct hns3_enet_ring *ring)
>  	return 0;
>  }
>  
> +static void hns3_oom_task(struct work_struct *work)
> +{
> +	struct hns3_nic_priv *priv = container_of(work, struct hns3_nic_priv,
> +						  oom_task.work);
> +	struct net_device *netdev = priv->netdev;
> +	struct hnae3_handle *h = priv->ae_handle;
> +	u16 i;
> +
> +	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state))
> +		return;

[Severity: Medium]

Can this early return leave bits set in priv->oom_vector_bm forever?

This is the only function that ever clears the bitmap, and the DOWN check
returns before the test_and_clear_bit() loop. The up path does not reset it
either, hns3_nic_net_up() only does:

	clear_bit(HNS3_NIC_STATE_DOWN, &priv->state);

and priv->oom_vector_bm is zeroed only when hns3_nic_alloc_vector_data()
reallocates it, which does not happen on a plain ifdown/ifup.

Bits can be set while DOWN is set: hns3_lb_check_rx_ring() in hns3_ethtool.c
calls hns3_clean_rx_ring() during the loopback selftest with the netdev
stopped, and a work item queued just before hns3_nic_net_stop() will run
after DOWN is set.

After the interface comes back up, the new tail test in hns3_clean_rx_ring()

	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
		hns3_oom_task_schedule(ring);

then evaluates true on every poll of that vector, bumping rx_oom_cnt,
re-setting the bit and re-queueing the work each time, until the work
finally executes. Would clearing the bitmap in hns3_nic_net_up(), or
clearing the bits before the DOWN early return, avoid that?

> +
> +	netif_dbg(h, rx_err, netdev, "oom napi_schedule 0x%*pb\n",
> +		  priv->vector_num, priv->oom_vector_bm);
> +	for (i = 0; i < priv->vector_num; i++)
> +		if (test_and_clear_bit(i, priv->oom_vector_bm))
> +			napi_schedule(&priv->tqp_vector[i].napi);
> +}
> +
> +static void hns3_oom_task_schedule(struct hns3_enet_ring *ring)
> +{
> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
> +
> +	hns3_ring_stats_update(ring, rx_oom_cnt);
> +	hns3_ring_set_oom_state(ring);
> +	schedule_delayed_work(&priv->oom_task,
> +			      msecs_to_jiffies(HNS3_OOM_POLL_INTERVAL_MS));
> +}

[Severity: Medium]

Is rx_oom_cnt charged to rings that never failed an allocation?

The counter lives in struct hns3_enet_ring and is exported per rx queue, but
the state that gates the increment is per vector, since
hns3_ring_set_oom_state() and hns3_ring_is_oom_state() index
priv->oom_vector_bm with ring->tqp_vector->idx.

One vector commonly serves several rx rings:

hns3_nic_alloc_vector_data()
	vector_num = min_t(u16, num_online_cpus(), tqp_num);

hns3_nic_init_vector_data()
		u16 vector_i = i % priv->vector_num;

and hns3_nic_common_poll() cleans all rx rings of the vector via
hns3_for_each_ring(ring, tqp_vector->rx_group). So after ring A fails,
hns3_clean_rx_ring() for sibling ring B sees hns3_ring_is_oom_state(ring)
true and calls hns3_oom_task_schedule(ring), which increments B's
rx_oom_cnt.

Since the bit stays set until the work runs about 250 ms later, does the
same single failure get re-counted on every poll in that window, making the
value track polling frequency rather than the number of failures?

Also, hns3_nic_alloc_rx_buffers() collapses all refill errors into its
boolean return, including the dma_mapping_error() path:

hns3_map_buffer()
	if (unlikely(dma_mapping_error(ring_to_dev(ring), cb->dma)))
		return -EIO;

so should a mapping failure be reported as an OOM event? Bumping the
counter at the actual allocation failure sites instead would avoid both
issues.

> +
>  int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>  		       void (*rx_fn)(struct hns3_enet_ring *, struct sk_buff *))
>  {
> @@ -4472,6 +4504,9 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>  
>  		/* Poll one pkt */
>  		err = hns3_handle_rx_bd(ring);
> +		if (unlikely(err == -ENOMEM))
> +			failure = true;
> +

[Severity: Medium]

Does setting failure here skip the buffer refill and the head doorbell
write at the exit block?

The only -ENOMEM reaching here comes from hns3_alloc_skb(), which leaves
ring->skb NULL:

	if (unlikely(!skb)) {
		hns3_rl_err(netdev, "alloc rx skb fail\n");
		hns3_ring_stats_update(ring, sw_err_cnt);

		return -ENOMEM;

so control goes straight to the out: label with failure already true, and
the short circuit there never evaluates the right operand:

out:
	/* sync head pointer before exiting, since hardware will calculate
	 * FBD number with head pointer
	 */
	if (unused_count > 0)
		failure = failure ||
			  hns3_nic_alloc_rx_buffers(ring, unused_count);

That means the up to 15 unused descriptors are not replenished and the
HNS3_RING_RX_RING_HEAD_REG write inside hns3_nic_alloc_rx_buffers() does not
happen for that poll, contrary to the comment above it. Buffer refill uses
the page_pool cache or dev_alloc_pages() rather than napi_alloc_skb(), so it
could still have succeeded.

Before this patch, failure being true at out: implied
hns3_nic_alloc_rx_buffers() had already run and written the head pointer.
Would setting failure after the out: block, or using a separate variable
for the head SKB failure, keep the refill attempt?

>  		/* Do not get FE for the packet or failed to alloc skb */
>  		if (unlikely(!ring->skb || err == -ENXIO)) {
>  			goto out;
> @@ -4493,7 +4528,10 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>  		failure = failure ||
>  			  hns3_nic_alloc_rx_buffers(ring, unused_count);
>  
> -	return failure ? budget : recv_pkts;
> +	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
> +		hns3_oom_task_schedule(ring);
> +
> +	return recv_pkts;
>  }

[Severity: Medium]

The changelog says:

  "Also fix hns3_clean_rx_ring() to return recv_pkts instead of budget
   on allocation failure, preventing NAPI busy-poll under low memory."

Was the removed return really a bug? git blame attributes
return failure ? budget : recv_pkts; to 68752b24f51a7 ("net: hns3: schedule
the polling again when allocation fails"), which added it so that a buffer
allocation failure reports an incomplete poll and keeps NAPI running:

hns3_nic_common_poll()
		int rx_cleaned = hns3_clean_rx_ring(ring, rx_budget,
						    hns3_rx_skb);
		if (rx_cleaned >= rx_budget)
			clean_complete = false;

With this change, the retry for hns3_nic_alloc_rx_buffers() failures moves
from the very next poll to up to 250 ms later, and only if the delayed work
actually runs. Could the changelog state that the return value change
supersedes the recovery added by 68752b24f51a7, and that the two changes are
now coupled, since the patch carries a Fixes: tag?

Is the described deadlock also broader than what the code shows? Before this
patch, only the napi_alloc_skb() failure in hns3_add_frag() returns -ENXIO
with failure left false, so that was the single path that could complete
NAPI with the ring undrained, which matches Fixes: 81ae0e0491f3. Refill
failures kept polling.

[Severity: Low]

Is the read of the bit here and the set inside hns3_oom_task_schedule() racy
against the work function?

hns3_ring_is_oom_state() tests the bit, then hns3_ring_set_oom_state()
unconditionally sets it again, as two separate non-atomic operations, while
hns3_oom_task() does test_and_clear_bit() on another CPU. A poll that read
the bit as true before the clear can set it again after the clear,
resurrecting a request with no allocation failure behind it, so the work
fires again 250 ms later, clears it and issues another napi_schedule().

Would setting the bit only at the real allocation failure sites and gating
this branch on failure alone, so that hns3_oom_task() is the only writer
that clears and nobody re-sets without a failure, remove the resurrection
cycle? Making the state per ring instead of per vector would have the same
effect.

>  
>  static void hns3_update_rx_int_coalesce(struct hns3_enet_tqp_vector *tqp_vector)

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> index 933e3527ed824..27a09629cbcab 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h

[ ... ]

> @@ -711,6 +714,20 @@ static inline unsigned int hns3_page_order(struct hns3_enet_ring *ring)
>  #define hns3_rl_usec_to_reg(int_rl) ((int_rl) >> 2)
>  #define hns3_rl_round_down(int_rl) round_down(int_rl, 4)
>  
> +static inline void hns3_ring_set_oom_state(struct hns3_enet_ring *ring)
> +{
> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
> +
> +	set_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
> +}
> +
> +static inline bool hns3_ring_is_oom_state(struct hns3_enet_ring *ring)
> +{
> +	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
> +
> +	return test_bit(ring->tqp_vector->idx, priv->oom_vector_bm);
> +}
> +
>  void hns3_ethtool_set_ops(struct net_device *netdev);

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index e7318f2363157..392653635bda2 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -62,6 +62,7 @@ static const struct hns3_stats hns3_rxq_stats[] = {
>  	HNS3_TQP_STAT("non_reuse_pg", non_reuse_pg),
>  	HNS3_TQP_STAT("frag_alloc_err", frag_alloc_err),
>  	HNS3_TQP_STAT("frag_alloc", frag_alloc),
> +	HNS3_TQP_STAT("rx_oom_cnt", rx_oom_cnt),
>  };

[Severity: Medium]

Should this new per-rx-queue allocation failure counter go through the
standard netlink queue statistics instead of a private ethtool -S string?

struct netdev_queue_stats_rx in include/net/netdev_queues.h already has:

	u64 alloc_fail;

exposed as rx-alloc-fail and documented in
Documentation/netlink/specs/netdev.yaml as "Number of times skb or buffer
allocation failed on the Rx datapath. Allocation failure may, or may not
result in a packet drop...", which matches what rx_oom_cnt counts.

hns3 has no netdev_stat_ops/get_queue_stats_rx implementation today, so this
adds a driver-private string for a counter category that
Documentation/networking/statistics.rst assigns to netdev generic netlink.
Would implementing get_queue_stats_rx() and filling stats->alloc_fail be
preferable to adding the ethtool string?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915132434.1141742-1-shaojijie%40huawei.com

[PATCH net 3/3] net: hns3: fix use-after-free in debugfs read during reset/unload

[Jijie Shao]

The seq_file refactoring of hns3 debugfs removed the per-file entry
state checks. Without them, debugfs show callbacks can access
priv->ring, priv->tqp_vector, and ring->desc while these resources
are being freed by reset, driver unload, or ethtool ring resize.

Plain state-bit checks leave a TOCTOU window between check and use.

Fix by adding a mutex (dbg_mutex) to struct hnae3_handle, shared by
enet-layer and PF-layer debugfs callbacks via the common handle.
Readers acquire the mutex and check device state at entry and per
iteration -- the latter lets an in-flight reader exit promptly when
reset begins mid-dump, keeping reset latency bounded. Writers clear
INITED first to signal readers, then acquire the mutex before freeing
resources. The dbg_uninit() call stays outside the mutex to avoid
deadlock with debugfs_remove_recursive.

Fixes: eced3d1c41db ("net: hns3: use seq_file for files in queue/ in debugfs")
Closes: https://lore.kernel.org/all/a0853cd9-cab5-441d-b181-8ba97f2f58b0@huawei.com/
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |  1 +
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 95 +++++++++++--------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  6 ++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  1 +
 .../hisilicon/hns3/hns3pf/hclge_debugfs.c     | 14 +++
 5 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 4286af9239b0..e94cc33da864 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -944,6 +944,7 @@ struct hnae3_handle {
 
 	u8 netdev_flags;
 	struct dentry *hnae3_dbgfs;
+	struct mutex dbg_mutex;	/* protect debugfs against reset/unload */
 
 	/* Network interface message level enabled bits */
 	u32 msg_enable;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 1347edac7699..7d913302f442 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -389,6 +389,12 @@ static const char * const dim_state_str[] = { "START", "IN_PROG", "APPLY" };
 static const char * const
 dim_tune_stat_str[] = { "ON_TOP", "TIRED", "RIGHT", "LEFT" };
 
+static bool hns3_dbg_is_device_busy(struct hns3_nic_priv *priv)
+{
+	return !test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
+	       test_bit(HNS3_NIC_STATE_RESETTING, &priv->state);
+}
+
 static void hns3_get_coal_info(struct hns3_enet_tqp_vector *tqp_vector,
 			       struct seq_file *s, int i, bool is_tx)
 {
@@ -434,7 +440,7 @@ static void hns3_get_coal_info(struct hns3_enet_tqp_vector *tqp_vector,
 	}
 }
 
-static void hns3_dump_coal_info(struct seq_file *s, bool is_tx)
+static int hns3_dump_coal_info(struct seq_file *s, bool is_tx)
 {
 	struct hnae3_handle *h = hnae3_seq_file_to_handle(s);
 	struct hns3_enet_tqp_vector *tqp_vector;
@@ -448,18 +454,32 @@ static void hns3_dump_coal_info(struct seq_file *s, bool is_tx)
 	seq_puts(s, "HW_GL  HW_QL\n");
 
 	for (i = 0; i < priv->vector_num; i++) {
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
+
 		tqp_vector = &priv->tqp_vector[i];
 		hns3_get_coal_info(tqp_vector, s, i, is_tx);
 	}
+
+	return 0;
 }
 
 static int hns3_dbg_coal_info(struct seq_file *s, void *data)
 {
-	hns3_dump_coal_info(s, true);
-	seq_puts(s, "\n");
-	hns3_dump_coal_info(s, false);
+	struct hnae3_handle *h = hnae3_seq_file_to_handle(s);
+	struct hns3_nic_priv *priv = h->priv;
+	int ret;
 
-	return 0;
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
+	ret = hns3_dump_coal_info(s, true);
+	if (ret)
+		return ret;
+
+	seq_puts(s, "\n");
+	return hns3_dump_coal_info(s, false);
 }
 
 static void hns3_dump_rx_queue_info(struct hns3_enet_ring *ring,
@@ -504,22 +524,16 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
-	if (!priv->ring) {
-		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
-		return -EFAULT;
-	}
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
 
 	seq_puts(s, "QUEUE_ID  BD_NUM  BD_LEN  TAIL  HEAD  FBDNUM  ");
 	seq_puts(s, "PKTNUM     COPYBREAK  RING_EN  RX_RING_EN  BASE_ADDR\n");
 
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
-		/* Each cycle needs to determine whether the instance is reset,
-		 * to prevent reference to invalid memory. And need to ensure
-		 * that the following code is executed within 100ms.
-		 */
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
-			return -EPERM;
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
 		hns3_dump_rx_queue_info(ring, s, i);
@@ -569,22 +583,16 @@ static int hns3_dbg_tx_queue_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
-	if (!priv->ring) {
-		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
-		return -EFAULT;
-	}
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
 
 	seq_puts(s, "QUEUE_ID  BD_NUM  TC  TAIL  HEAD  FBDNUM  OFFSET  ");
 	seq_puts(s, "PKTNUM     RING_EN  TX_RING_EN  BASE_ADDR\n");
 
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
-		/* Each cycle needs to determine whether the instance is reset,
-		 * to prevent reference to invalid memory. And need to ensure
-		 * that the following code is executed within 100ms.
-		 */
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
-			return -EPERM;
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
 		ring = &priv->ring[i];
 		hns3_dump_tx_queue_info(ring, s, i);
@@ -604,9 +612,14 @@ static int hns3_dbg_queue_map(struct seq_file *s, void *data)
 
 	seq_puts(s, "local_queue_id  global_queue_id  vector_id\n");
 
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
-		if (!priv->ring || !priv->ring[i].tqp_vector)
-			continue;
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
+
 		seq_printf(s, "%-16u%-17u%d\n", i,
 			   h->ae_algo->ops->get_global_queue_id(h, i),
 			   priv->ring[i].tqp_vector->vector_irq);
@@ -661,8 +674,10 @@ static int hns3_dbg_rx_bd_info(struct seq_file *s, void *private)
 
 	ring = &priv->ring[data->qid + data->handle->kinfo.num_tqps];
 	for (i = 0; i < ring->desc_num; i++) {
-		desc = &ring->desc[i];
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_rx_bd_info(priv, desc, s, i);
 	}
 
@@ -706,8 +721,10 @@ static int hns3_dbg_tx_bd_info(struct seq_file *s, void *private)
 
 	ring = &priv->ring[data->qid];
 	for (i = 0; i < ring->desc_num; i++) {
-		desc = &ring->desc[i];
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_tx_bd_info(desc, s, i);
 	}
 
@@ -796,10 +813,9 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
-	if (!priv->ring) {
-		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
-		return -EFAULT;
-	}
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
 
 	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
 		dev_err(&h->pdev->dev, "page pool is not initialized\n");
@@ -810,9 +826,8 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
 	seq_puts(s, "POOL_SIZE(PAGE_NUM)  ORDER  NUMA_ID  MAX_LEN\n");
 
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
-			return -EPERM;
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
 		hns3_dump_page_pool_info(ring, s, i);
@@ -827,8 +842,8 @@ static int hns3_dbg_bd_info_show(struct seq_file *s, void *private)
 	struct hnae3_handle *h = data->handle;
 	struct hns3_nic_priv *priv = h->priv;
 
-	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+	guard(mutex)(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv))
 		return -EBUSY;
 
 	if (data->cmd == HNAE3_DBG_CMD_TX_BD)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 0c088feae03c..bae8b32ffc5b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5449,6 +5449,7 @@ static int hns3_client_init(struct hnae3_handle *handle)
 	priv->min_tx_copybreak = 0;
 	priv->min_tx_spare_buf_size = 0;
 	set_bit(HNS3_NIC_STATE_DOWN, &priv->state);
+	mutex_init(&handle->dbg_mutex);
 
 	handle->msg_enable = netif_msg_init(debug, DEFAULT_MSG_LEVEL);
 
@@ -5562,6 +5563,7 @@ static int hns3_client_init(struct hnae3_handle *handle)
 	priv->ring = NULL;
 out_get_ring_cfg:
 	priv->ae_handle = NULL;
+	mutex_destroy(&handle->dbg_mutex);
 	free_netdev(netdev);
 	return ret;
 }
@@ -5585,6 +5587,7 @@ static void hns3_client_uninit(struct hnae3_handle *handle, bool reset)
 
 	hns3_free_rx_cpu_rmap(netdev);
 
+	mutex_lock(&handle->dbg_mutex);
 	hns3_nic_uninit_irq(priv);
 
 	hns3_clear_all_ring(handle, true);
@@ -5596,9 +5599,11 @@ static void hns3_client_uninit(struct hnae3_handle *handle, bool reset)
 	hns3_uninit_all_ring(priv);
 
 	hns3_put_ring_config(priv);
+	mutex_unlock(&handle->dbg_mutex);
 
 out_netdev_free:
 	hns3_dbg_uninit(handle);
+	mutex_destroy(&handle->dbg_mutex);
 	free_netdev(netdev);
 }
 
@@ -5875,6 +5880,7 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
 		return 0;
 	}
 
+	guard(mutex)(&handle->dbg_mutex);
 	hns3_free_rx_cpu_rmap(netdev);
 	hns3_nic_uninit_irq(priv);
 	hns3_clear_all_ring(handle, true);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 392653635bda..4e7a7e6b21ee 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -1258,6 +1258,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	if (if_running)
 		ndev->netdev_ops->ndo_stop(ndev);
 
+	guard(mutex)(&h->dbg_mutex);
 	hns3_change_all_ring_bd_num(priv, new_ringparam.tx_desc_num,
 				    new_ringparam.rx_desc_num);
 	hns3_change_rx_buf_len(ndev, new_ringparam.rx_buf_len);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
index 9a4e29bfa166..1e9d3e08b7de 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
@@ -15,6 +15,12 @@
 #define hclge_seq_file_to_hdev(s)	\
 		(((struct hnae3_ae_dev *)hnae3_seq_file_to_ae_dev(s))->priv)
 
+static bool hclge_dbg_is_device_busy(struct hclge_dev *hdev)
+{
+	return test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state) ||
+	       test_bit(HCLGE_STATE_RST_FAIL, &hdev->state);
+}
+
 static const char * const hclge_mac_state_str[] = {
 	"TO_ADD", "TO_DEL", "ACTIVE"
 };
@@ -2621,6 +2627,10 @@ static int hclge_dbg_dump_umv_info(struct seq_file *s, void *data)
 	struct hclge_vport *vport;
 	u8 i;
 
+	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
+	if (hclge_dbg_is_device_busy(hdev))
+		return -EBUSY;
+
 	seq_printf(s, "num_alloc_vport   : %u\n", hdev->num_alloc_vport);
 	seq_printf(s, "max_umv_size     : %u\n", hdev->max_umv_size);
 	seq_printf(s, "wanted_umv_size  : %u\n", hdev->wanted_umv_size);
@@ -2831,6 +2841,10 @@ static int hclge_dbg_dump_vlan_offload_config(struct hclge_dev *hdev,
 	int ret;
 	u8 i;
 
+	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
+	if (hclge_dbg_is_device_busy(hdev))
+		return -EBUSY;
+
 	seq_puts(s, "FUNC_ID  PVID  ACCEPT_TAG1  ACCEPT_TAG2 ACCEPT_UNTAG1  ");
 	seq_puts(s, "ACCEPT_UNTAG2  INSERT_TAG1  INSERT_TAG2  SHIFT_TAG  ");
 	seq_puts(s, "STRIP_TAG1  STRIP_TAG2  DROP_TAG1  DROP_TAG2  ");
-- 
2.43.0

Re: [PATCH net 3/3] net: hns3: fix use-after-free in debugfs read during reset/unload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 1

- [High] On the PF side the new locking is inoperative.
- [Medium] The commit message justifies the change with "The seq_file
  refactoring of hns3 debugfs removed the per-file entry state checks"…
- [Medium] The new device-busy gate is added to only one of the two
  helpers that produce the output of the single `vlan_config` debugfs…
- [Medium] The reader/writer handshake the patch relies on has no
  publication ordering on the reset-reinitialization side.
- [Low] The Fixes: tag cites only eced3d1c41db ("net: hns3: use seq_file
  for files in queue/ in debugfs"), which is the correct origin for the…

--

net: hns3: fix use-after-free in debugfs read during reset/unload

This adds a new mutex (dbg_mutex) to struct hnae3_handle, has the debugfs
show callbacks take it plus a device-busy state check at entry and per
loop iteration, and has the enet teardown/ring-resize paths take it before
freeing rings and vectors. Two hclge (PF layer) debugfs callbacks also
take the same mutex through hdev->vport[0].nic.

Closes: https://lore.kernel.org/all/a0853cd9-cab5-441d-b181-8ba97f2f58b0@huawei.com/

A few questions on the commit message and the code below.

>     The seq_file refactoring of hns3 debugfs removed the per-file entry
>     state checks. Without them, debugfs show callbacks can access
>     priv->ring, priv->tqp_vector, and ring->desc while these resources
>     are being freed by reset, driver unload, or ethtool ring resize.

[Severity: Medium]
Does this description cover the hclge_debugfs.c part of the patch?  At the
baseline, grepping hclge_debugfs.c for HCLGE_STATE_RST_HANDLING or any
test_bit(HCLGE_STATE...) returns no matches, so there were no per-file
state checks there to be removed by the seq_file conversion.  The two
guarded PF callbacks also do not touch priv->ring, priv->tqp_vector or
ring->desc; they read hdev->* and hdev->vport[] and issue firmware
commands, and hdev->vport is only freed in hclge_uninit_ae_dev().

The selection also looks arbitrary.  Of the roughly fifty
hclge_dbg_dump_* seq_file callbacks registered in hclge_dbg_cmd_func[],
only hclge_dbg_dump_umv_info() and hclge_dbg_dump_vlan_offload_config()
gained the new guard, while structurally identical ones that read
hdev-owned state during reset (hclge_dbg_dump_tm_*, hclge_dbg_dump_fd_*,
hclge_dbg_dump_mng_table, hclge_dbg_dump_mac_uc/mc, the register dumps)
were left alone.

If the PF layer needs this protection, should the rest of the callbacks
get it too?  If it does not, could the hclge_debugfs.c hunks be dropped?

>     Fixes: eced3d1c41db ("net: hns3: use seq_file for files in queue/ in debugfs")

[Severity: Low]
This tag matches the hns3_debugfs.c (enet) change, but the seq_file
conversion of hclge_dbg_dump_umv_info() blames to 9e1545b48818e0
("net: hns3: use seq_file for debugfs in umv_info and vlan_config"-era
sibling commit by Yonglong Liu), not to eced3d1c41db.  Should a second
Fixes: tag be added so the hclge_debugfs.c hunks can be mapped to the
commit that introduced them?

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> index 4286af9239b01..e94cc33da8643 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> @@ -944,6 +944,7 @@ struct hnae3_handle {
>  
>  	u8 netdev_flags;
>  	struct dentry *hnae3_dbgfs;
> +	struct mutex dbg_mutex;	/* protect debugfs against reset/unload */
>  
>  	/* Network interface message level enabled bits */
>  	u32 msg_enable;
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> index 1347edac76993..7d913302f4424 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> @@ -389,6 +389,12 @@ static const char * const dim_state_str[] = { "START", "IN_PROG", "APPLY" };
>  static const char * const
>  dim_tune_stat_str[] = { "ON_TOP", "TIRED", "RIGHT", "LEFT" };
>  
> +static bool hns3_dbg_is_device_busy(struct hns3_nic_priv *priv)
> +{
> +	return !test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
> +	       test_bit(HNS3_NIC_STATE_RESETTING, &priv->state);
> +}
> +

[ ... ]

> @@ -504,22 +524,16 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
>  	struct hns3_enet_ring *ring;
>  	u32 i;
>  
> -	if (!priv->ring) {
> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
> -		return -EFAULT;
> -	}
> +	guard(mutex)(&priv->ae_handle->dbg_mutex);
> +	if (hns3_dbg_is_device_busy(priv))
> +		return -EBUSY;

[ ... ]

> @@ -604,9 +612,14 @@ static int hns3_dbg_queue_map(struct seq_file *s, void *data)
>  
>  	seq_puts(s, "local_queue_id  global_queue_id  vector_id\n");
>  
> +	guard(mutex)(&priv->ae_handle->dbg_mutex);
> +	if (hns3_dbg_is_device_busy(priv))
> +		return -EBUSY;
> +
>  	for (i = 0; i < h->kinfo.num_tqps; i++) {
> -		if (!priv->ring || !priv->ring[i].tqp_vector)
> -			continue;
> +		if (hns3_dbg_is_device_busy(priv))
> +			return -EBUSY;
> +
>  		seq_printf(s, "%-16u%-17u%d\n", i,
>  			   h->ae_algo->ops->get_global_queue_id(h, i),
>  			   priv->ring[i].tqp_vector->vector_irq);

[Severity: Medium]
Is the reader/writer handshake ordered on the reinitialization side?

The teardown side does what the changelog describes:

hns3_reset_notify_uninit_enet()
	test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state);
	guard(mutex)(&handle->dbg_mutex);
	...
	hns3_put_ring_config(priv);	/* priv->ring = NULL */

But the reinit side never takes dbg_mutex and publishes with a plain
bitop:

hns3_reset_notify_init_enet()
	... rebuilds priv->ring, priv->tqp_vector, descriptors ...
	set_bit(HNS3_NIC_STATE_INITED, &priv->state);

hns3_reset_notify_up_enet()
	clear_bit(HNS3_NIC_STATE_RESETTING, &priv->state);

Non-returning bitops carry no memory barriers
(Documentation/atomic_bitops.txt), and since the initializer never holds
dbg_mutex, the reader's mutex acquisition pairs only with the teardown
release, which stored priv->ring = NULL.  On a weakly ordered machine can
a reader observe INITED set and RESETTING clear while still reading the
stale priv->ring, and then dereference priv->ring[i].tqp_vector here?

The "!priv->ring || !priv->ring[i].tqp_vector" test that used to absorb
that case is being deleted in this hunk, as are the "if (!priv->ring)"
guards in hns3_dbg_rx_queue_info(), hns3_dbg_tx_queue_info() and
hns3_dbg_page_pool_info().  Would either keeping those NULL checks or
having hns3_reset_notify_init_enet() publish under dbg_mutex (or with
release/acquire semantics) close this?

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 0c088feae03ca..bae8b32ffc5b8 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c

[ ... ]

> @@ -5875,6 +5880,7 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
>  		return 0;
>  	}
>  
> +	guard(mutex)(&handle->dbg_mutex);
>  	hns3_free_rx_cpu_rmap(netdev);
>  	hns3_nic_uninit_irq(priv);
>  	hns3_clear_all_ring(handle, true);
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
> index 9a4e29bfa1666..1e9d3e08b7ded 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
> @@ -15,6 +15,12 @@
>  #define hclge_seq_file_to_hdev(s)	\
>  		(((struct hnae3_ae_dev *)hnae3_seq_file_to_ae_dev(s))->priv)
>  
> +static bool hclge_dbg_is_device_busy(struct hclge_dev *hdev)
> +{
> +	return test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state) ||
> +	       test_bit(HCLGE_STATE_RST_FAIL, &hdev->state);
> +}
> +
>  static const char * const hclge_mac_state_str[] = {
>  	"TO_ADD", "TO_DEL", "ACTIVE"
>  };
> @@ -2621,6 +2627,10 @@ static int hclge_dbg_dump_umv_info(struct seq_file *s, void *data)
>  	struct hclge_vport *vport;
>  	u8 i;
>  
> +	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
> +	if (hclge_dbg_is_device_busy(hdev))
> +		return -EBUSY;
> +

[Severity: High]
Does this mutex exclude anything on the PF side?

Grepping the driver for dbg_mutex after this patch gives:

  hnae3.h:1
  hns3_debugfs.c:6
  hns3_enet.c:6
  hns3_ethtool.c:1
  hns3pf/hclge_debugfs.c:2

hclge_main.c has no matches, so no writer of the state these two
callbacks read ever takes dbg_mutex:

hclge_reset_service_task()
	down(&hdev->reset_sem);
	set_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);
	hclge_reset_subtask(hdev);
	clear_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);

and hclge_reset_err_handle() sets HCLGE_STATE_RST_FAIL the same way.  The
only reset-side acquisition is hns3_reset_notify_uninit_enet(), which
covers priv->ring and priv->tqp_vector, not the UMV counters or the
per-function VLAN offload registers read here.

So isn't hclge_dbg_is_device_busy() here exactly the check-then-use
window the changelog says the mutex removes?  A reader admitted before
RST_HANDLING is set keeps issuing cmdq queries across the reset boundary
and prints values sampled from both sides of it.

Two follow-on questions:

Does the comment on the new struct member, "protect debugfs against
reset/unload", hold for hclge state given the above?

Does taking dbg_mutex and then hdev->vport_lock here create a new lock
order that the writer side is not aware of?

> @@ -2831,6 +2841,10 @@ static int hclge_dbg_dump_vlan_offload_config(struct hclge_dev *hdev,
>  	int ret;
>  	u8 i;
>  
> +	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
> +	if (hclge_dbg_is_device_busy(hdev))
> +		return -EBUSY;
> +

[Severity: Medium]
Should the gate be in the show callback instead of in this helper?

Both halves of the single vlan_config file run from one callback:

hclge_dbg_dump_vlan_config()
	ret = hclge_dbg_dump_vlan_filter_config(hdev, s);
	if (ret)
		return ret;

	return hclge_dbg_dump_vlan_offload_config(hdev, s);

hclge_dbg_dump_vlan_filter_config() issues
hclge_get_vlan_filter_state() and
hclge_get_port_vlan_filter_bypass_state() for every function and emits
its section with no mutex and no state check, so reading vlan_config
during a reset still runs that half against the resetting device and only
then returns -EBUSY from the second half.  Since seq_read_iter() resets
m->count to the record offset on a negative show() return, the output the
first half produced is discarded and the read fails anyway.

Also, unlike the enet readers, this loop holds dbg_mutex across
2 * (pci_num_vf() + 1) synchronous firmware commands with no
per-iteration busy re-check.  Given that the reset path reaches
guard(mutex)(&handle->dbg_mutex) via

  hclge_reset_rebuild() -> rtnl_lock() -> hclge_reset_stack() ->
  hclge_notify_client(HNAE3_UNINIT_CLIENT) ->
  hns3_reset_notify_uninit_enet()

can a PF debugfs reader stall the reset, and rtnl_lock with it, for the
duration of the dump?  The changelog says the per-iteration check keeps
reset latency bounded, which does not appear to apply to these two PF
callbacks.

>  	seq_puts(s, "FUNC_ID  PVID  ACCEPT_TAG1  ACCEPT_TAG2 ACCEPT_UNTAG1  ");
>  	seq_puts(s, "ACCEPT_UNTAG2  INSERT_TAG1  INSERT_TAG2  SHIFT_TAG  ");
>  	seq_puts(s, "STRIP_TAG1  STRIP_TAG2  DROP_TAG1  DROP_TAG2  ");

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915132434.1141742-1-shaojijie%40huawei.com

Re: [PATCH net 3/3] net: hns3: fix use-after-free in debugfs read during reset/unload

[Jakub Kicinski]

On Tue, 15 Sep 2026 21:24:34 +0800 Jijie Shao wrote:
> @@ -504,22 +524,16 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
>  	struct hns3_enet_ring *ring;
>  	u32 i;
>  
> -	if (!priv->ring) {
> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
> -		return -EFAULT;
> -	}
> +	guard(mutex)(&priv->ae_handle->dbg_mutex);
> +	if (hns3_dbg_is_device_busy(priv))
> +		return -EBUSY;
>  
>  	seq_puts(s, "QUEUE_ID  BD_NUM  BD_LEN  TAIL  HEAD  FBDNUM  ");
>  	seq_puts(s, "PKTNUM     COPYBREAK  RING_EN  RX_RING_EN  BASE_ADDR\n");

Quoting documentation:

  Using device-managed and cleanup.h constructs
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  
  Netdev remains skeptical about promises of all "auto-cleanup" APIs,
  including even ``devm_`` helpers, historically. They are not the preferred
  style of implementation, merely an acceptable one.
  
  Use of ``guard()`` is discouraged within any function longer than 20 lines,
  ``scoped_guard()`` is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.
  
  Low level cleanup constructs (such as ``__free()``) can be used when building
  APIs and helpers, especially scoped iterators. However, direct use of
  ``__free()`` within networking core and drivers is discouraged.
  Similar guidance applies to declaring variables mid-function.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
-- 
pw-bot: cr