[PATCH v4 net 0/4] net: hns3: fix multiple bugs in reset/error paths

[PATCH v4 net 0/4] net: hns3: fix multiple bugs in reset/error paths

[Jijie Shao]

This series contains four bug fixes for the HNS3 driver, all related
to resource lifecycle management on reset, driver unload, or error
paths.

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 a page_pool leak in the error path of
hns3_alloc_ring_memory, where an hns3_alloc_ring_buffers() failure
for an RX ring frees the descriptor ring but not the page_pool
allocated just before.

Patch 3 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 rings to break the deadlock.

Patch 4 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.

---
Changes in v4:
  - Patch 1: correct the reason for resetting priv->vector_num, as
    reported by Sashiko.  hns3_nic_dealloc_vector_data() is not
    reachable after hns3_nic_alloc_vector_data() fails, so the reset
    is not protecting that path; it keeps the count from describing
    vectors that were already handed back.
  - Patch 2: name hns3_fini_ring() instead of the non-existent
    hns3_free_ring_memory(), and drop "mirroring" because the destroy
    order differs.  The pool is destroyed before the descriptor ring
    here, which is safe because hns3_alloc_ring_buffers() has already
    detached every buffer.
  - Patch 3:
    * Allocate rx_ring_oom_bm in hns3_init_all_ring() only after every
      ring has been set up, and free it in hns3_uninit_all_ring().
      This fixes the leak and the NULL bitmap dereference reported for
      hns3_set_ringparam(), which calls hns3_init_all_ring() without a
      matching hns3_uninit_all_ring(); the allocation is reused when
      the pointer is already set.
    * Cancel the delayed work in hns3_nic_uninit_vector_data() instead
      of hns3_nic_net_down(), so every teardown path cancels it before
      the bitmap is freed.  This closes the use-after-free reported
      for the self-test and external loopback paths, which set DOWN
      without going through hns3_nic_net_down().
    * Drop the cancel and bitmap_zero from hns3_nic_net_down().  A
      pending work that runs while DOWN is set zeroes the bitmap and
      exits, and one that runs after the interface is brought back up
      only schedules a single extra poll round.
    * Always attempt the exit refill in hns3_clean_rx_ring(), so the
      descriptors cleaned by this poll stay armed and the head pointer
      is written as before.
    * Document why rx_oom_cnt is not the standard rx-alloc-fail
      counter, as requested in review.
  - Patch 4: describe both writer paths (client_uninit/reset_uninit vs
    set_ringparam) explicitly, note the -EBUSY user-visible change, and
    return -EBUSY from hns3_dbg_page_pool_info() instead of -EFAULT so
    that sibling readers agree.
v3: https://lore.kernel.org/all/20261030095633.377289-1-shaojijie@huawei.com/
Changes in v3:
  - Patch 1: simplify commit message; drop the accumulated-leak
    consequence paragraph and clarify that the vector_num reset avoids
    a NULL deref in hns3_nic_dealloc_vector_data().
  - Patch 2: new patch, fixes a page_pool leak in the error path of
    hns3_alloc_ring_memory().
  - Patch 3: rework the OOM bitmap from per-vector to per-ring,
    indexed by ring->queue_index.  Add cancel_delayed_work_sync() in
    hns3_nic_net_down() after hns3_disable_irqs_and_tqps().  Remove
    the OOM marking in hns3_handle_rx_copybreak() since the packet is
    still delivered via the original page.
  - Patch 4: add Fixes tag for the ring resize path.  Add per-ring
    page_pool NULL check at hns3_dump_page_pool_info() entry.
v2: https://lore.kernel.org/all/20261021121426.3096838-1-shaojijie@huawei.com/
Changes in v2:
Patch 1:
  - No change.
Patch 2:
  - Clear per-vector OOM bitmap when device is DOWN to avoid
    spurious napi_schedule() on next up event.
  - Move rx_oom_cnt accounting to hns3_ring_set_oom_state() with
    test_and_set_bit() for per-event counting.
Patch 3:
  - Drop hclge_debugfs.c changes; mutex only protects enet-layer.
  - Replace guard(mutex) with explicit mutex_lock/unlock.
  - Remove redundant NULL checks; unify -EPERM to -EBUSY.
v1: https://lore.kernel.org/all/20260915132434.1141742-1-shaojijie@huawei.com/
---

Jijie Shao (4):
  net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data
  net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path
  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    | 127 +++++++++++-------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  80 ++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   |  18 +++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   3 +
 5 files changed, 179 insertions(+), 50 deletions(-)


base-commit: 8d5ce4751fad9499ea408163af7f410f18a85782
-- 
2.43.0

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

[Jijie Shao]

When devm_kcalloc() fails for priv->tqp_vector after get_vector() has
already acquired MSI-X vectors, the error path returns without calling
put_vector() to release them, leaving the hclge/hclgevf bookkeeping
(num_msi_used, num_msi_left, vector_status[]) inconsistent.

Call put_vector() for every acquired vector before returning the error,
and reset priv->vector_num to 0 so the count never describes vectors
that were already handed back.

Fixes: dd38c72604dc ("net: hns3: fix for coalesce configuration lost during reset")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
Changes in v4:
  - Correct the reason for resetting priv->vector_num.  Sashiko pointed
    out that hns3_nic_dealloc_vector_data() is not reachable after
    hns3_nic_alloc_vector_data() fails, so the reset is not protecting
    that path; it keeps the count from describing vectors that were
    already handed back.
v3: https://lore.kernel.org/all/20261030095633.377289-2-shaojijie@huawei.com/
Changes in v3:
  - Simplify commit message: drop the accumulated-leak consequence
    paragraph and clarify that the vector_num reset avoids a NULL
    deref in hns3_nic_dealloc_vector_data().
v2: https://lore.kernel.org/all/20261021121426.3096838-2-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 v4 net 2/4] net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path

[Jijie Shao]

In hns3_alloc_ring_memory(), when hns3_alloc_ring_buffers() fails for
an RX ring, the error path jumps to out_with_desc, which frees the
descriptor ring but does not destroy the page_pool that was just
allocated by hns3_alloc_page_pool(). The page_pool and its pages are
leaked.

Destroy the page_pool in that error path as well. The pool is torn down
before the descriptor ring here, which is the opposite of the order in
hns3_fini_ring(); that is safe because hns3_alloc_ring_buffers() has
already detached every buffer it attached, so hns3_free_desc() walks
descriptors whose dma is zero and never hands a page back to the pool.

Fixes: 93188e9642c3 ("net: hns3: support skb's frag page recycling based on page pool")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
Changes in v4:
  - Name hns3_fini_ring() instead of the non-existent
    hns3_free_ring_memory(), and drop "mirroring" because the destroy
    order differs.  The pool is destroyed before the descriptor ring
    here, which is safe because hns3_alloc_ring_buffers() has already
    detached every buffer.
v3: https://lore.kernel.org/all/20261030095633.377289-3-shaojijie@huawei.com/
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 2f3aeade558c..dbf32c00bb7f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5078,6 +5078,8 @@ static int hns3_alloc_ring_memory(struct hns3_enet_ring *ring)
 	return 0;
 
 out_with_desc:
+	page_pool_destroy(ring->page_pool);
+	ring->page_pool = NULL;
 	hns3_free_desc(ring);
 out_with_desc_cb:
 	devm_kfree(ring_to_dev(ring), ring->desc_cb);
-- 
2.43.0

[PATCH v4 net 3/4] 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, 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, leaving the interface
deadlocked.

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

This supersedes the busy-poll recovery from commit 68752b24f51a
("net: hns3: schedule the polling again when allocation fails"),
which returned budget on allocation failure to keep NAPI polling
and caused CPU spinning under prolonged memory pressure. Now
hns3_clean_rx_ring() returns recv_pkts instead of budget on
allocation failure, letting NAPI exit and the delayed_work take
over. The exit refill is still attempted, so the descriptors cleaned
by this poll stay armed. Add rx_oom_cnt ethtool counter, incremented
once per OOM event (on the 0 -> 1 bitmap transition) for diagnostics.
It counts OOM episodes rather than individual allocation failures,
so it is not the standard rx-alloc-fail counter.

The bitmap is allocated in hns3_init_all_ring() and freed in
hns3_uninit_all_ring(). It is allocated after every ring is set up,
so a failure inside hns3_init_all_ring() leaves nothing behind, and
it is reused because hns3_set_ringparam() calls hns3_init_all_ring()
without hns3_uninit_all_ring(). The delayed_work is cancelled in
hns3_nic_uninit_vector_data(), which every teardown path reaches
before the bitmap is freed.

hns3_nic_net_down() does not cancel the work, so a pending work may
still run after the interface goes down. If it runs while DOWN is
set, it zeroes the bitmap and returns without scheduling NAPI. If
the interface is brought back up within the 250ms window, it sees
the stale bits with DOWN cleared and schedules NAPI for those rings
once, ending in a single extra poll round with no packet to clean.
Both outcomes are harmless.

Fixes: 68752b24f51a ("net: hns3: schedule the polling again when allocation fails")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
Changes in v4:
  - Allocate rx_ring_oom_bm in hns3_init_all_ring() only after every
    ring has been set up, and free it in hns3_uninit_all_ring().  This
    fixes the leak and the NULL bitmap dereference in the
    hns3_set_ringparam() path, which calls hns3_init_all_ring()
    without a matching hns3_uninit_all_ring(), and reuses the existing
    allocation on a ring resize.
  - Cancel the delayed work in hns3_nic_uninit_vector_data() instead
    of hns3_nic_net_down(), so every teardown path cancels it before
    the bitmap is freed.  The self-test and external loopback paths
    set DOWN without going through hns3_nic_net_down().
  - Drop the cancel and bitmap_zero from hns3_nic_net_down().  A
    pending work that runs while DOWN is set zeroes the bitmap and
    exits, and one that runs after the interface is brought back up
    only schedules a single extra poll round.
  - Always attempt the exit refill in hns3_clean_rx_ring(), so the
    descriptors cleaned by this poll stay armed and the head pointer
    is written as before.
  - Document why rx_oom_cnt is not the standard rx-alloc-fail counter.
v3: https://lore.kernel.org/all/20261030095633.377289-4-shaojijie@huawei.com/
Changes in v3:
  - Rework the OOM bitmap from per-vector to per-ring, indexed by
    ring->queue_index, so each ring's OOM event is counted exactly
    once per oom_task firing cycle regardless of vector sharing.
  - Add cancel_delayed_work_sync(&priv->oom_task) to
    hns3_nic_net_down(), placed after hns3_disable_irqs_and_tqps(),
    eliminating the stop/open window where a preempted worker could
    clear a bit newly set by NAPI on the reopened interface.
  - Remove the hns3_ring_set_oom_state() call from
    hns3_handle_rx_copybreak(): napi_alloc_frag() failure returns
    -ENOMEM but the packet is still delivered via the original page,
    so it should not drive the OOM recovery work.
v2: https://lore.kernel.org/all/20261021121426.3096838-3-shaojijie@huawei.com/
Changes in v2:
  - Clear per-vector OOM bitmap when device is DOWN to avoid
    spurious napi_schedule() on next up event.
  - Move rx_oom_cnt accounting to hns3_ring_set_oom_state() with
    test_and_set_bit() for per-event counting.
  - Expand changelog: reference commit 68752b24f51a, explain
    hns3_clean_rx_ring() return value change, document rx_oom_cnt
    per-event counting semantics.
v1: https://lore.kernel.org/all/20260915132434.1141742-3-shaojijie@huawei.com/
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 61 ++++++++++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 ++++++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  1 +
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index dbf32c00bb7f..90f0a8ac9295 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
@@ -4162,6 +4164,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 +4454,35 @@ 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 hnae3_handle *h = priv->ae_handle;
+	struct hns3_enet_ring *ring;
+	u16 i;
+
+	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
+		bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);
+		return;
+	}
+
+	for (i = 0; i < h->kinfo.num_tqps; i++)
+		if (test_and_clear_bit(i, priv->rx_ring_oom_bm)) {
+			ring = &priv->ring[i + h->kinfo.num_tqps];
+			napi_schedule(&ring->tqp_vector->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_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;
@@ -4490,10 +4525,12 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
 	 * FBD number with head pointer
 	 */
 	if (unused_count > 0)
-		failure = failure ||
-			  hns3_nic_alloc_rx_buffers(ring, unused_count);
+		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 +4825,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:
@@ -4889,6 +4927,8 @@ 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];
 
@@ -5204,6 +5244,16 @@ int hns3_init_all_ring(struct hns3_nic_priv *priv)
 		cond_resched();
 	}
 
+	/* hns3_set_ringparam() calls this without hns3_uninit_all_ring(). */
+	if (!priv->rx_ring_oom_bm) {
+		priv->rx_ring_oom_bm = bitmap_zalloc(h->kinfo.num_tqps,
+						     GFP_KERNEL);
+		if (!priv->rx_ring_oom_bm) {
+			ret = -ENOMEM;
+			goto out_when_alloc_ring_memory;
+		}
+	}
+
 	return 0;
 
 out_when_alloc_ring_memory:
@@ -5222,6 +5272,11 @@ static void hns3_uninit_all_ring(struct hns3_nic_priv *priv)
 		hns3_fini_ring(&priv->ring[i]);
 		hns3_fini_ring(&priv->ring[i + h->kinfo.num_tqps]);
 	}
+
+	if (priv->rx_ring_oom_bm) {
+		bitmap_free(priv->rx_ring_oom_bm);
+		priv->rx_ring_oom_bm = NULL;
+	}
 }
 
 /* Set mac addr if it is configured. or leave it to the AE driver */
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 933e3527ed82..5470df5d8efb 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 *rx_ring_oom_bm;
 
 	u64 tx_timeout_count;
 
@@ -711,6 +714,21 @@ 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));
+
+	if (!test_and_set_bit(ring->queue_index, priv->rx_ring_oom_bm))
+		hns3_ring_stats_update(ring, rx_oom_cnt);
+}
+
+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->queue_index, priv->rx_ring_oom_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 7b92a3c73ef6..14642f81efe1 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

[PATCH v4 net 4/4] 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, used by
enet-layer debugfs callbacks. Readers acquire the mutex, then check
device state per iteration -- this lets an in-flight reader exit
promptly when reset begins mid-dump, keeping reset latency bounded
by one iteration, and also rejects a reader that arrives after the
writer has finished tearing down resources (priv->ring is NULL by
then). The dbg_uninit() call stays outside the mutex to avoid
deadlock with debugfs_remove_recursive().

Two kinds of writer paths are protected:

  * hns3_client_uninit() and hns3_reset_notify_uninit_enet() clear
    HNS3_NIC_STATE_INITED first so that readers running outside the
    mutex see the device as busy and return -EBUSY early, then take
    dbg_mutex before freeing resources.

  * hns3_set_ringparam() calls ndo_stop() first (which sets
    HNS3_NIC_STATE_DOWN) and then takes dbg_mutex around the ring
    re-allocation. The busy check does not test the DOWN bit, so
    readers that arrive here are blocked by the mutex itself rather
    than by the INITED signal.

Userspace-visible change: debugfs reads under
/sys/kernel/debug/hns3/*/ that race with reset, driver unload, or
ethtool ring resize now return -EBUSY instead of potentially reading
freed or zero-filled memory.

Fixes: a723fb8efe29d3 ("net: hns3: refine for set ring parameters")
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>
---
Changes in v4:
  - Describe both writer paths (client_uninit/reset_uninit vs
    set_ringparam) explicitly and note the -EBUSY user-visible change.
  - Return -EBUSY from hns3_dbg_page_pool_info() instead of -EFAULT
    when priv->ring is NULL, matching the sibling readers.
v3: https://lore.kernel.org/all/20261030095633.377289-5-shaojijie@huawei.com/
Changes in v3:
  - Add Fixes tag for a723fb8efe29d3 ("net: hns3: refine for set ring
    parameters"): the ring resize path it introduced is the writer that
    can free and rebuild rings, which is the root cause of the UAF
    window exposed by the seq_file refactoring.
  - Add per-ring page_pool NULL check at the entry of
    hns3_dump_page_pool_info(), avoiding a NULL deref while holding
    dbg_mutex (which would deadlock reset on the mutex).
v2: https://lore.kernel.org/all/20261021121426.3096838-4-shaojijie@huawei.com/
Changes in v2:
  - Drop hclge_debugfs.c changes; mutex only protects enet-layer
    debugfs callbacks.
  - Replace guard(mutex) with explicit mutex_lock/unlock and
    "goto unlock" error-path labels.
  - Remove redundant if (!priv->ring) checks in
    hns3_dbg_rx_queue_info() and hns3_dbg_tx_queue_info().
  - Unify per-iteration return values from -EPERM to -EBUSY.
  - Reorder local variables to reverse-Christmas-tree order.
  - Reword changelog to drop the "PF-layer" description.
v1: https://lore.kernel.org/all/20260915132434.1141742-4-shaojijie@huawei.com/
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |   1 +
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 127 +++++++++++-------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |   7 +
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   2 +
 4 files changed, 92 insertions(+), 45 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..a58ca42dbcd9 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,34 @@ 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++) {
+		/* Per-iteration check bounds reset latency to one iteration. */
+		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);
+	struct hnae3_handle *h = hnae3_seq_file_to_handle(s);
+	struct hns3_nic_priv *priv = h->priv;
+	int ret;
+
+	mutex_lock(&priv->ae_handle->dbg_mutex);
+	ret = hns3_dump_coal_info(s, true);
+	if (ret)
+		goto unlock;
+
 	seq_puts(s, "\n");
-	hns3_dump_coal_info(s, false);
+	ret = hns3_dump_coal_info(s, false);
 
-	return 0;
+unlock:
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
+	return ret;
 }
 
 static void hns3_dump_rx_queue_info(struct hns3_enet_ring *ring,
@@ -504,27 +526,22 @@ 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;
-	}
-
+	mutex_lock(&priv->ae_handle->dbg_mutex);
 	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;
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv)) {
+			mutex_unlock(&priv->ae_handle->dbg_mutex);
+			return -EBUSY;
+		}
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
 		hns3_dump_rx_queue_info(ring, s, i);
 	}
 
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
 	return 0;
 }
 
@@ -569,27 +586,22 @@ 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;
-	}
-
+	mutex_lock(&priv->ae_handle->dbg_mutex);
 	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;
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv)) {
+			mutex_unlock(&priv->ae_handle->dbg_mutex);
+			return -EBUSY;
+		}
 
 		ring = &priv->ring[i];
 		hns3_dump_tx_queue_info(ring, s, i);
 	}
 
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
 	return 0;
 }
 
@@ -604,14 +616,23 @@ static int hns3_dbg_queue_map(struct seq_file *s, void *data)
 
 	seq_puts(s, "local_queue_id  global_queue_id  vector_id\n");
 
+	mutex_lock(&priv->ae_handle->dbg_mutex);
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv)) {
+			mutex_unlock(&priv->ae_handle->dbg_mutex);
+			return -EBUSY;
+		}
+
 		if (!priv->ring || !priv->ring[i].tqp_vector)
 			continue;
+
 		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);
 	}
 
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
 	return 0;
 }
 
@@ -661,8 +682,11 @@ 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];
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_rx_bd_info(priv, desc, s, i);
 	}
 
@@ -706,8 +730,11 @@ 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];
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_tx_bd_info(desc, s, i);
 	}
 
@@ -779,6 +806,11 @@ static int hns3_dbg_dev_info(struct seq_file *s, void *data)
 static void hns3_dump_page_pool_info(struct hns3_enet_ring *ring,
 				     struct seq_file *s, u32 index)
 {
+	if (!ring->page_pool) {
+		seq_printf(s, "%-10u %s\n", index, "page pool not initialized");
+		return;
+	}
+
 	seq_printf(s, "%-10u%-14u%-14d%-21u%-7u%-9d%uK\n",
 		   index,
 		   READ_ONCE(ring->page_pool->pages_state_hold_cnt),
@@ -796,28 +828,27 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
+	mutex_lock(&priv->ae_handle->dbg_mutex);
 	if (!priv->ring) {
-		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
-		return -EFAULT;
-	}
-
-	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
-		dev_err(&h->pdev->dev, "page pool is not initialized\n");
-		return -EFAULT;
+		mutex_unlock(&priv->ae_handle->dbg_mutex);
+		return -EBUSY;
 	}
 
 	seq_puts(s, "QUEUE_ID  ALLOCATE_CNT  FREE_CNT      ");
 	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;
+		/* Per-iteration check bounds reset latency to one iteration. */
+		if (hns3_dbg_is_device_busy(priv)) {
+			mutex_unlock(&priv->ae_handle->dbg_mutex);
+			return -EBUSY;
+		}
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
 		hns3_dump_page_pool_info(ring, s, i);
 	}
 
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
 	return 0;
 }
 
@@ -826,17 +857,23 @@ static int hns3_dbg_bd_info_show(struct seq_file *s, void *private)
 	struct hns3_dbg_data *data = s->private;
 	struct hnae3_handle *h = data->handle;
 	struct hns3_nic_priv *priv = h->priv;
+	int ret;
 
-	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+	mutex_lock(&priv->ae_handle->dbg_mutex);
+	if (hns3_dbg_is_device_busy(priv)) {
+		mutex_unlock(&priv->ae_handle->dbg_mutex);
 		return -EBUSY;
+	}
 
 	if (data->cmd == HNAE3_DBG_CMD_TX_BD)
-		return hns3_dbg_tx_bd_info(s, private);
+		ret = hns3_dbg_tx_bd_info(s, private);
 	else if (data->cmd == HNAE3_DBG_CMD_RX_BD)
-		return hns3_dbg_rx_bd_info(s, private);
+		ret = hns3_dbg_rx_bd_info(s, private);
+	else
+		ret = -EOPNOTSUPP;
 
-	return -EOPNOTSUPP;
+	mutex_unlock(&priv->ae_handle->dbg_mutex);
+	return ret;
 }
 DEFINE_SHOW_ATTRIBUTE(hns3_dbg_bd_info);
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 90f0a8ac9295..472194ba3fcb 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5456,6 +5456,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);
 
@@ -5569,6 +5570,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;
 }
@@ -5592,6 +5594,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);
@@ -5603,9 +5606,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);
 }
 
@@ -5882,6 +5887,7 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
 		return 0;
 	}
 
+	mutex_lock(&handle->dbg_mutex);
 	hns3_free_rx_cpu_rmap(netdev);
 	hns3_nic_uninit_irq(priv);
 	hns3_clear_all_ring(handle, true);
@@ -5894,6 +5900,7 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
 	hns3_uninit_all_ring(priv);
 
 	hns3_put_ring_config(priv);
+	mutex_unlock(&handle->dbg_mutex);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 14642f81efe1..7005d8b2d010 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -1252,6 +1252,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	if (if_running)
 		ndev->netdev_ops->ndo_stop(ndev);
 
+	mutex_lock(&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);
@@ -1270,6 +1271,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
 		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
 			hns3_fini_ring(&tmp_rings[i]);
 	}
+	mutex_unlock(&h->dbg_mutex);
 
 	kfree(tmp_rings);
 
-- 
2.43.0

Re: [PATCH v4 net 0/4] net: hns3: fix multiple bugs in reset/error paths

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH v4 net 0/4] net: hns3: fix multiple bugs in reset/error paths

[Jakub Kicinski]

On Wed, 11 Nov 2026 13:01:03 +0000 netdev-bot+sinfo@kernel.org wrote:
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.

Please make sure you include the answers to these questions in the
cover letter if you repost

Re: [PATCH v4 net 0/4] net: hns3: fix multiple bugs in reset/error paths

[Jijie Shao]

on 2026/11/13 3:06, Jakub Kicinski wrote:
> On Wed, 11 Nov 2026 13:01:03 +0000 netdev-bot+sinfo@kernel.org wrote:
>> Please do not repost the series just to address the above. Instead,
>> reply to this email with the missing information, so that reviewers
>> can take it into account. If the series needs another revision for
>> other reasons, please include the information in the commit messages
>> then.
> Please make sure you include the answers to these questions in the
> cover letter if you repost

ok,

Patch 1: found in internal code review. Not hit at runtime - it needs
devm_kcalloc() to fail after get_vector() has succeeded.

Patch 2: found by Sashiko on v2. Error path only, not hit at runtime.

Patch 3: real customer issue. Under memory pressure the RX ring filled
up, no more RX interrupts came in and the interface went dead. No stack
trace, the interface going dead was the only symptom. Reproduced and
fixed on EVB.

Patch 4: hit on a lab EVB while reading the debugfs queue files during
reset/unload. NULL pointer dereference, machine hung.

All four were built and tested on a Huawei Kunpeng evaluation board (EVB)
with the on-board hns3 NIC. Patch 1 and 2 were not exercised on the
failure path.

Thanks,
Jijie Shao

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds a new private `ethtool -S` per-queue string,
  "rx_oom_cnt", at hns3_rxq_stats[] in hns3_ethtool.c line 65.
- [Medium] net: hns3: rx_ring_oom_bm freed before cancelling oom_task on
  init error
- [Low] In hns3_clean_rx_ring() (hns3_enet.c around lines 4499-4533), the
  commit message says that because the exit refill is always attempted,…
- [Low] When HNS3_NIC_STATE_DOWN is set, hns3_oom_task() clears
  priv->rx_ring_oom_bm with the non-atomic bitmap_zero()…

--

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

This patch adds a 250ms delayed_work that schedules NAPI again for RX
rings that hit allocation failures. The interface can then recover after
OOM even when the full hardware ring stops raising RX interrupts. It
replaces the return-budget busy-poll recovery in hns3_clean_rx_ring() and
adds a per-ring rx_oom_cnt ethtool counter.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index dbf32c00bb7f3..90f0a8ac9295c 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c

[ ... ]

> @@ -4451,6 +4454,35 @@ 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 hnae3_handle *h = priv->ae_handle;
> +	struct hns3_enet_ring *ring;
> +	u16 i;
> +
> +	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
> +		bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);

[Severity: Medium]
The commit message says oom_task is cancelled in
hns3_nic_uninit_vector_data(), which every teardown path reaches
before the bitmap is freed. Does that hold on the init error unwind
paths?

hns3_client_init() calls hns3_nic_init_vector_data(), which does the
INIT_DELAYED_WORK(), before hns3_init_all_ring(), which allocates
rx_ring_oom_bm. The error unwind therefore runs in the opposite
order:

    out_init_phy:
        hns3_uninit_all_ring(priv);
    out_init_ring:
        hns3_nic_uninit_vector_data(priv);

hns3_reset_notify_init_enet() is laid out the same way:

    err_init_irq_fail:
        hns3_uninit_all_ring(priv);
    err_uninit_vector:
        hns3_nic_uninit_vector_data(priv);

On both paths hns3_uninit_all_ring() frees rx_ring_oom_bm and sets
it to NULL before cancel_delayed_work_sync(&priv->oom_task) runs. If
oom_task is pending or running at that point, hns3_oom_task() calls
bitmap_zero() or test_and_clear_bit() on a freed or NULL pointer.

Can the work be pending there? In hns3_client_init(),
register_netdev() can open the device from a NETDEV_REGISTER
notifier, for example net_failover_slave_register() calling
netif_open(). An RX allocation failure in that window reaches
hns3_oom_task_schedule() and queues oom_task. If
register_netdevice() then fails, the device is closed through
hns3_nic_net_stop(), which sets HNS3_NIC_STATE_DOWN but does not
cancel the work. hns3_client_init() then unwinds from
out_reg_netdev_fail through out_init_phy and frees the bitmap while
the work can still fire.

The reset path looks harder to hit, but it has the same ordering, and
both contradict the invariant the commit message relies on.

This is a cold error path, but the result is a NULL dereference or a
use-after-free write from the workqueue. Would it be better to cancel
oom_task in hns3_uninit_all_ring() just before bitmap_free()? Or
could the bitmap allocation and free move next to
INIT_DELAYED_WORK() and the cancel in hns3_nic_init_vector_data()
and hns3_nic_uninit_vector_data(), so the two are always torn down
in the right order?

[Severity: Low]
Is it safe to clear rx_ring_oom_bm with the non-atomic bitmap_zero() here?
Every other writer uses atomic bitops: test_and_set_bit() in
hns3_ring_set_oom_state() and test_and_clear_bit() below. bitmap_zero() is
a plain store or a memset.

A writer can run while DOWN is set in at least two ways.

First, hns3_nic_net_stop() sets DOWN before hns3_nic_net_down() reaches
napi_disable():

    if (test_and_set_bit(HNS3_NIC_STATE_DOWN, &priv->state))
        return 0;

hns3_nic_common_poll() checks DOWN only on entry. A poll that is already
running can still reach hns3_ring_set_oom_state() from hns3_add_frag() or
from hns3_oom_task_schedule().

Second, the ethtool loopback self-test calls hns3_clean_rx_ring() directly
while DOWN is set:

hns3_lb_check_rx_ring()
    hns3_clean_rx_ring(ring, budget, hns3_lb_check_skb_data);

Wouldn't KCSAN report this plain store racing with test_and_set_bit() on
the same word?

There is a separate problem: the test_bit(DOWN) check and bitmap_zero()
are not atomic with respect to an ifup. Suppose the worker is preempted
between them, the interface comes back up, and a NAPI poll sets a bit and
queues the work again. The resumed worker then zeroes the new bit:

hns3_oom_task()
    test_bit(HNS3_NIC_STATE_DOWN) -> true
    <preempted>
                                    ifup clears DOWN
                                    hns3_clean_rx_ring()
                                      hns3_oom_task_schedule()
                                        test_and_set_bit(queue_index)
                                        schedule_delayed_work()
    bitmap_zero()

When the work runs again it finds no bits set and does not schedule NAPI
for that ring.

Could this lose the only recovery kick for the ring? The commit message
calls both outcomes harmless, but it does not cover this case.

[ ... ]

> @@ -4490,10 +4525,12 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>  	 * FBD number with head pointer
>  	 */
>  	if (unused_count > 0)
> -		failure = failure ||
> -			  hns3_nic_alloc_rx_buffers(ring, unused_count);
> +		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: Low]
The commit message says:

    The exit refill is still attempted, so the descriptors cleaned
    by this poll stay armed.

Is that accurate? The refill inside the loop in hns3_clean_rx_ring() is
unchanged:

    if (unused_count >= RCB_NOF_ALLOC_RX_BUFF_ONCE) {
        failure = failure ||
            hns3_nic_alloc_rx_buffers(ring, unused_count);
        unused_count = 0;
    }

After the first failure inside the loop, the || short-circuits. Even so,
unused_count is still reset to 0 each time it reaches 16. The exit refill
above therefore covers only what accumulated since the last reset, not
every descriptor consumed in this poll.

Before this patch, returning budget forced an immediate repoll, which
recomputed hns3_desc_unused(). Now recv_pkts is returned, so
hns3_nic_common_poll() can complete NAPI while that deficit is still
outstanding.

After a transient allocation failure, couldn't up to roughly a budget's
worth of consumed descriptors stay unarmed until the next RX interrupt or
the 250ms oom_task kick? That would include reuse_flag descriptors that
need no allocation, and it would happen even if memory recovers right
away.

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 7b92a3c73ef6d..14642f81efe1a 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]
Does this new ethtool -S string duplicate the standard per-queue
rx-alloc-fail qstat (struct netdev_queue_stats_rx.alloc_fail)? The netdev
netlink spec documents that counter as "Number of times skb or buffer
allocation failed on the Rx datapath".

Every increment of rx_oom_cnt comes from an RX allocation failure. Either
napi_alloc_skb() fails in hns3_add_frag(), or hns3_oom_task_schedule()
runs after hns3_handle_rx_bd() returns -ENOMEM or
hns3_nic_alloc_rx_buffers() fails.

The commit message says:

    It counts OOM episodes rather than individual allocation failures,
    so it is not the standard rx-alloc-fail counter.

However, hns3_oom_task() clears each ring's bit every time it runs:

    if (test_and_clear_bit(i, priv->rx_ring_oom_bm)) {

The next poll that fails sets the bit again and increments the counter in
hns3_ring_set_oom_state():

    if (!test_and_set_bit(ring->queue_index, priv->rx_ring_oom_bm))
        hns3_ring_stats_update(ring, rx_oom_cnt);

Under a prolonged OOM, wouldn't rx_oom_cnt increase about once per 250ms
per ring? That makes it a rate-limited count of allocation failures, not
a count of episodes.

hns3 does not implement netdev_stat_ops/get_queue_stats_rx yet. Could this
be reported as alloc_fail through the standard qstats interface rather
than as a private ethtool string?

This patch also carries a Fixes: tag. Would it be better to leave the new
user-visible counter out of the fix?

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch serializes teardown against debugfs readers with
  dbg_mutex, but the reset re-init path still hands the new resources to…
- [Low] The patch changes debugfs behaviour in ways the commit message
  does not describe.
- [Low] hns3_set_ringparam() runs under RTNL (ethtool core) and now calls
  mutex_lock(&h->dbg_mutex) without first telling in-flight debugfs…

--

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

This patch adds a dbg_mutex to struct hnae3_handle. The enet-layer
debugfs show callbacks take it, and so do the client uninit path, the
reset uninit path and the ring reallocation in hns3_set_ringparam().
The readers also re-check the INITED and RESETTING state bits on each
iteration and return -EBUSY when the device is busy.

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

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> index 1347edac76993..a58ca42dbcd97 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);
> +}
> +

[Severity: Medium]
dbg_mutex now serializes readers against the uninit writers. Is the
re-init side of reset covered as well?

hns3_reset_notify_init_enet() reallocates priv->ring, priv->tqp_vector,
priv->vector_num and ring->desc without taking dbg_mutex. It then
publishes them with a plain bitop:

hns3_reset_notify_init_enet() {
    ret = hns3_get_ring_config(priv);
    ...
    ret = hns3_nic_alloc_vector_data(priv);
    ...
    ret = hns3_init_all_ring(priv);
    ...
    set_bit(HNS3_NIC_STATE_INITED, &priv->state);
}

Later, hns3_reset_notify_up_enet() does a plain
clear_bit(HNS3_NIC_STATE_RESETTING, &priv->state).

On the reader side, hns3_dbg_is_device_busy() uses plain test_bit().
The callers then load priv->ring, priv->tqp_vector and ring->desc.

The reader's mutex_lock() only pairs with the unlock in the uninit
path. The init path never takes the mutex.

On a weakly ordered CPU such as arm64, can a reader see INITED set and
RESETTING clear, but still load the post-uninit values of those
pointers?

If that happens, priv->ring would be NULL, because
hns3_put_ring_config() clears it. This patch also removes the
if (!priv->ring) checks from hns3_dbg_rx_queue_info() and
hns3_dbg_tx_queue_info().

priv->tqp_vector would be stale, because hns3_nic_dealloc_vector_data()
frees it with devm_kfree() and never sets it to NULL. In that case
hns3_dump_coal_info() would read freed memory.

The writer issues many full barriers before clear_bit(RESETTING), so
this is hard to hit in practice.

Would either of these close the window?

  - set INITED under dbg_mutex in the init path
  - use release/acquire bitops: smp_mb__before_atomic() before
    set_bit(), and test_bit_acquire() in the reader

[ ... ]

> @@ -779,6 +806,11 @@ static int hns3_dbg_dev_info(struct seq_file *s, void *data)
>  static void hns3_dump_page_pool_info(struct hns3_enet_ring *ring,
>  				     struct seq_file *s, u32 index)
>  {
> +	if (!ring->page_pool) {
> +		seq_printf(s, "%-10u %s\n", index, "page pool not initialized");
> +		return;
> +	}
> +
>  	seq_printf(s, "%-10u%-14u%-14d%-21u%-7u%-9d%uK\n",
>  		   index,
>  		   READ_ONCE(ring->page_pool->pages_state_hold_cnt),
> @@ -796,28 +828,27 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
>  	struct hns3_enet_ring *ring;
>  	u32 i;
>  
> +	mutex_lock(&priv->ae_handle->dbg_mutex);
>  	if (!priv->ring) {
> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
> -		return -EFAULT;
> -	}
> -
> -	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
> -		dev_err(&h->pdev->dev, "page pool is not initialized\n");
> -		return -EFAULT;
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
> +		return -EBUSY;
>  	}

[Severity: Low]
This changes the result of reading page_pool_info when page pools are
not set up, which has nothing to do with reset. It applies when the
module is loaded with page_pool_enabled=0, or when page_pool_create()
fails in hns3_alloc_page_pool().

Before this patch, the read failed with -EFAULT and logged "page pool
is not initialized". Now it succeeds and prints one "page pool not
initialized" row per queue.

The patch also changes some error codes:

  - the per-iteration error goes from -EPERM to -EBUSY in
    hns3_dbg_rx_queue_info(), hns3_dbg_tx_queue_info() and
    hns3_dbg_page_pool_info()
  - the priv->ring == NULL error here goes from -EFAULT to -EBUSY

The commit message only says:

    Userspace-visible change: debugfs reads under
    /sys/kernel/debug/hns3/*/ that race with reset, driver unload, or
    ethtool ring resize now return -EBUSY instead of potentially reading
    freed or zero-filled memory.

These changes are only explained in the v3/v4 changelog notes, which
won't be part of the commit. Could the commit message mention them?

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 14642f81efe1a..7005d8b2d010b 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -1252,6 +1252,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>  	if (if_running)
>  		ndev->netdev_ops->ndo_stop(ndev);
>  
> +	mutex_lock(&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);

[Severity: Low]
hns3_set_ringparam() runs under RTNL. The only state change it makes
before taking dbg_mutex is ndo_stop() -> hns3_nic_net_stop(), which
sets HNS3_NIC_STATE_DOWN.

hns3_dbg_is_device_busy() only checks INITED and RESETTING. A reader
already inside a show callback therefore never gets -EBUSY on this
path.

For example, hns3_dbg_bd_info_show() -> hns3_dbg_tx_bd_info() walks all
ring->desc_num entries, up to HNS3_RING_MAX_PENDING (32760), with one
seq_printf() per descriptor. Meanwhile:

ethtool -G
  rtnl_lock()
  hns3_set_ringparam()
    ndo_stop()                   /* sets DOWN only */
    mutex_lock(&h->dbg_mutex)    /* waits for the whole dump */

Doesn't this hold RTNL for a full show() call, blocking every other
RTNL user in the meantime?

The per-iteration checks bound the reset path's wait, but not this
writer's. The commit message covers readers that arrive during
set_ringparam, which are blocked by the mutex. It doesn't cover readers
already in flight.

Would it help to set a state bit that hns3_dbg_is_device_busy() checks
before set_ringparam takes dbg_mutex?

> @@ -1270,6 +1271,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>  		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
>  			hns3_fini_ring(&tmp_rings[i]);
>  	}
> +	mutex_unlock(&h->dbg_mutex);
>  
>  	kfree(tmp_rings);
>

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