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

[PATCH v3 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 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    | 125 ++++++++++++------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  74 ++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   |  18 +++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   3 +
 5 files changed, 174 insertions(+), 47 deletions(-)


base-commit: 5838f10563d58afca6711edb5e4eda968ca181d9
-- 
2.43.0

[PATCH v3 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 to avoid a NULL deref in the
subsequent hns3_nic_dealloc_vector_data(), which iterates
priv->tqp_vector (still NULL after the kcalloc failure) using
priv->vector_num.

Fixes: dd38c72604dc ("net: hns3: fix for coalesce configuration lost during reset")
Signed-off-by: Jijie Shao <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 v3 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.

Add page_pool_destroy() to the out_with_desc error path, guarded by
a NULL check, mirroring the cleanup done in hns3_free_ring_memory().

Fixes: 93188e9642c3 ("net: hns3: support skb's frag page recycling based on page pool")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 2f3aeade558c..024d435ab62b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5078,6 +5078,10 @@ static int hns3_alloc_ring_memory(struct hns3_enet_ring *ring)
 	return 0;
 
 out_with_desc:
+	if (!HNAE3_IS_TX_RING(ring) && ring->page_pool) {
+		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 v3 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. Add rx_oom_cnt ethtool counter, incremented once per OOM
event (on the 0 -> 1 bitmap transition) for diagnostics.

Fixes: 68752b24f51a ("net: hns3: schedule the polling again when allocation fails")
Signed-off-by: Jijie Shao <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   | 53 ++++++++++++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   | 18 +++++++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  1 +
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 024d435ab62b..2aa491095736 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
@@ -857,6 +859,7 @@ static void hns3_nic_net_down(struct net_device *netdev)
 	const struct hnae3_ae_ops *ops;
 
 	hns3_disable_irqs_and_tqps(netdev);
+	cancel_delayed_work_sync(&priv->oom_task);
 
 	/* stop ae_dev */
 	ops = priv->ae_handle->ae_algo->ops;
@@ -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,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 net_device *netdev = priv->netdev;
+	struct hnae3_handle *h = priv->ae_handle;
+	u16 i;
+
+	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
+		bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);
+		return;
+	}
+
+	netif_dbg(h, rx_err, netdev, "rx ring oom bitmap: 0x%*pb\n",
+		  h->kinfo.num_tqps, priv->rx_ring_oom_bm);
+	for (i = 0; i < h->kinfo.num_tqps; i++)
+		if (test_and_clear_bit(i, priv->rx_ring_oom_bm))
+			napi_schedule(&priv->ring[i].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 +4505,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 +4529,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 +4827,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:
@@ -5193,6 +5233,10 @@ int hns3_init_all_ring(struct hns3_nic_priv *priv)
 	int i, j;
 	int ret;
 
+	priv->rx_ring_oom_bm = bitmap_zalloc(h->kinfo.num_tqps, GFP_KERNEL);
+	if (!priv->rx_ring_oom_bm)
+		return -ENOMEM;
+
 	hns3_update_tx_spare_buf_config(priv);
 	for (i = 0; i < ring_num; i++) {
 		ret = hns3_alloc_ring_memory(&priv->ring[i]);
@@ -5212,6 +5256,8 @@ int hns3_init_all_ring(struct hns3_nic_priv *priv)
 	for (j = i - 1; j >= 0; j--)
 		hns3_fini_ring(&priv->ring[j]);
 
+	bitmap_free(priv->rx_ring_oom_bm);
+	priv->rx_ring_oom_bm = NULL;
 	return -ENOMEM;
 }
 
@@ -5224,6 +5270,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 v3 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). 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: 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 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    | 125 ++++++++++++------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |   7 +
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   2 +
 4 files changed, 91 insertions(+), 44 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..ec08672372f0 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,13 +828,9 @@ 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");
+		mutex_unlock(&priv->ae_handle->dbg_mutex);
 		return -EFAULT;
 	}
 
@@ -810,14 +838,17 @@ 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;
+		/* 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 2aa491095736..81f4c878f642 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5454,6 +5454,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);
 
@@ -5567,6 +5568,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;
 }
@@ -5590,6 +5592,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);
@@ -5601,9 +5604,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);
 }
 
@@ -5880,6 +5885,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);
@@ -5892,6 +5898,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 v3 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 code inspection). If it was triggered please include
   the symptoms, like the stack trace or error 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 v3 net 1/4] net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not give the real reason for one line of
  the fix.

--

net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

This patch fixes the error path in hns3_nic_alloc_vector_data() for when
devm_kcalloc() of priv->tqp_vector fails. The new path calls put_vector()
for every vector that get_vector() acquired, and resets priv->vector_num
to 0 before returning.

> Call put_vector() for every acquired vector before returning the
> error, and reset priv->vector_num to 0 to avoid a NULL deref in the
> subsequent hns3_nic_dealloc_vector_data(), which iterates
> priv->tqp_vector (still NULL after the kcalloc failure) using
> priv->vector_num.

[Severity: Low]
Is this the right reason for the priv->vector_num reset? As far as I can
tell, no path calls hns3_nic_dealloc_vector_data() after
hns3_nic_alloc_vector_data() fails.

In hns3_client_init(), the failure jumps to a label that comes after the
dealloc call:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_client_init() {
    ...
	ret = hns3_nic_alloc_vector_data(priv);
	if (ret) {
		ret = -ENOMEM;
		goto out_alloc_vector_data;
	}
    ...
out_init_vector_data:
	hns3_nic_dealloc_vector_data(priv);
out_alloc_vector_data:
    ...
}

hns3_reset_notify_init_enet() works the same way:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_reset_notify_init_enet() {
    ...
	ret = hns3_nic_alloc_vector_data(priv);
	if (ret)
		goto err_put_ring;
    ...
err_dealloc_vector:
	hns3_nic_dealloc_vector_data(priv);
err_put_ring:
    ...
}

hns3_client_uninit() and hns3_reset_notify_uninit_enet() both start with
test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state). They bail out
before calling hns3_nic_dealloc_vector_data() if the bit is clear.

HNS3_NIC_STATE_INITED is only set once init succeeds, so it is still
clear after this failure. That means the NULL deref described above
cannot happen, with or without this patch.

The reset may still be useful for other code that indexes
priv->tqp_vector by priv->vector_num while the netdev stays alive after
a failed reset re-init. One example is hns3_dump_coal_info() in
hns3_debugfs.c:

	for (i = 0; i < priv->vector_num; i++) {
		tqp_vector = &priv->tqp_vector[i];

Should the commit message name the code that actually depends on
priv->vector_num being 0, rather than hns3_nic_dealloc_vector_data()?

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

Re: [PATCH v3 net 2/4] net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the new cleanup in
  hns3_alloc_ring_memory()…

--

net: hns3: fix page_pool leak in hns3_alloc_ring_memory error path

When hns3_alloc_ring_buffers() fails for an RX ring, the out_with_desc
error path in hns3_alloc_ring_memory() frees the descriptor ring. It does
not destroy the page_pool that hns3_alloc_page_pool() just created. This
patch adds a guarded page_pool_destroy() to that error path.

> Add page_pool_destroy() to the out_with_desc error path, guarded by
> a NULL check, mirroring the cleanup done in hns3_free_ring_memory().

[Severity: Low]
This isn't a bug, but is hns3_free_ring_memory() the right name here? I
can't find a function with that name under
drivers/net/ethernet/hisilicon/hns3/.

The only other RX page_pool_destroy() is in hns3_fini_ring(). That function
calls hns3_free_desc() first and destroys the pool last:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_fini_ring() {
	hns3_free_desc(ring);
	devm_kfree(ring_to_dev(ring), ring->desc_cb);
    ...
	if (!HNAE3_IS_TX_RING(ring) && ring->page_pool) {
		page_pool_destroy(ring->page_pool);
		ring->page_pool = NULL;
	}
}

The new out_with_desc path does it in the opposite order. It destroys the
pool and sets ring->page_pool to NULL, then calls hns3_free_desc().

That order looks harmless today. By the time hns3_alloc_ring_buffers()
returns an error, it has already detached every buffer it attached. Each
desc_cb[i].dma is zero, so hns3_free_buffer_detach() returns early and
never hands a page back to the NULL pool.

Could the commit message name hns3_fini_ring() instead? It could also drop
the word "mirroring", since the order differs.

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

Re: [PATCH v3 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 5 potential
issue(s) to consider.

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

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).

- [High] hns3_set_ringparam() (ethtool -G) calls hns3_init_all_ring() on
  a live, already-initialised priv and never calls…
- [High] priv->oom_task is cancelled in only one place,
  hns3_nic_net_down().
- [Medium] Stale OOM bits survive an ifdown/ifup.
- [Medium] net: hns3: rx_oom_cnt in ethtool -S duplicates
  netdev_queue_stats_rx
- [Low] In hns3_clean_rx_ring(), the patch now sets `failure = true` when
  hns3_handle_rx_bd() returns -ENOMEM.

--

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

This commit adds a 250ms delayed_work (oom_task) and a per-RX-ring bitmap
so that NAPI is rescheduled for rings that hit an allocation failure,
instead of depending on hardware RX interrupts. hns3_clean_rx_ring() now
returns recv_pkts instead of budget on failure, and a new rx_oom_cnt
ethtool counter is added.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 024d435ab62b6..2aa491095736c 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c

[ ... ]

> @@ -857,6 +859,7 @@ static void hns3_nic_net_down(struct net_device *netdev)
>  	const struct hnae3_ae_ops *ops;
>  
>  	hns3_disable_irqs_and_tqps(netdev);
> +	cancel_delayed_work_sync(&priv->oom_task);

[Severity: Medium]
Do stale OOM bits carry over into the next ifup?

Suppose oom_task is still pending here because NAPI set a bit less than
250ms earlier. cancel_delayed_work_sync() removes the work without running
it, so the HNS3_NIC_STATE_DOWN branch of hns3_oom_task() never runs its
bitmap_zero().

hns3_nic_net_stop(), hns3_nic_net_open() and hns3_nic_net_up() never
touch rx_ring_oom_bm. Only hns3_init_all_ring() and hns3_uninit_all_ring()
do.

So after the next ifup, the first hns3_clean_rx_ring() on that ring sees
hns3_ring_is_oom_state() return true. It then queues recovery work and a
napi_schedule() even though no allocation failed.

The next real OOM on that ring is also not counted in rx_oom_cnt, because
test_and_set_bit() in hns3_ring_set_oom_state() finds the bit already set.

Would a bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps) right after
this cancel close the gap? A stale bit is also one way the self-test path
below can queue oom_task on a stopped device.

[ ... ]

> @@ -4451,6 +4455,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 net_device *netdev = priv->netdev;
> +	struct hnae3_handle *h = priv->ae_handle;
> +	u16 i;
> +
> +	if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
> +		bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);
> +		return;
> +	}
> +
> +	netif_dbg(h, rx_err, netdev, "rx ring oom bitmap: 0x%*pb\n",
> +		  h->kinfo.num_tqps, priv->rx_ring_oom_bm);
> +	for (i = 0; i < h->kinfo.num_tqps; i++)
> +		if (test_and_clear_bit(i, priv->rx_ring_oom_bm))
> +			napi_schedule(&priv->ring[i].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));
> +}

[Severity: High]
Can this work be queued after hns3_nic_net_down() has already run its
cancel, which is the only one?

The ethtool offline self-test (ethtool -t <dev> offline) polls RX rings
outside of NAPI. It does this after ndo_stop(), or on an interface that
was never opened, so HNS3_NIC_STATE_DOWN is set in both cases:

hns3_self_test()
  hns3_lp_run_test()
    hns3_lb_check_rx_ring()
      preempt_disable();
      hns3_clean_rx_ring(ring, budget, hns3_lb_check_skb_data);
      preempt_enable();

hns3_clean_rx_ring() now reaches hns3_oom_task_schedule() on any RX
allocation failure. It also gets there whenever a stale bit is set in
rx_ring_oom_bm.

Neither hns3_lp_down() nor hns3_selftest_restore() cancels the work, so
it can still be pending when the device is torn down.

For a reset, hns3_reset_notify_uninit_enet() skips hns3_nic_net_stop()
when HNS3_NIC_STATE_DOWN is set:

	if (!test_bit(HNS3_NIC_STATE_DOWN, &priv->state))
		hns3_nic_net_stop(netdev);
	...
	hns3_uninit_all_ring(priv);

That frees rx_ring_oom_bm and sets it to NULL. When the work then fires,
it takes the HNS3_NIC_STATE_DOWN branch of hns3_oom_task() above and calls
bitmap_zero() on a NULL bitmap.

If reset re-init runs first, hns3_nic_init_vector_data() calls
INIT_DELAYED_WORK() on a delayed_work whose timer is still pending.

For unload, hns3_client_uninit() on a down device goes through
unregister_netdev() without ndo_stop(), and then does:

	hns3_uninit_all_ring(priv);

	hns3_put_ring_config(priv);

out_netdev_free:
	hns3_dbg_uninit(handle);
	free_netdev(netdev);

Does anything cancel priv->oom_task before free_netdev() frees the priv
that embeds it?

If the interface was running before the test, hns3_selftest_restore()
calls ndo_open() while the work may still be pending or running.

A worker that has already seen HNS3_NIC_STATE_DOWN set can then clear a
bit that NAPI set after the reopen. The OOM recovery for that ring is then
lost.

The later patches in this series don't appear to add a cancel on any of
these paths. Would a cancel_delayed_work_sync(&priv->oom_task) be needed
in the teardown paths and after the self-test RX polling?

[ ... ]

> @@ -4472,6 +4505,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 +4529,10 @@ int hns3_clean_rx_ring(struct hns3_enet_ring *ring, int budget,
>  		failure = failure ||
>  			  hns3_nic_alloc_rx_buffers(ring, unused_count);

[Severity: Low]
Now that failure is set to true when hns3_handle_rx_bd() returns -ENOMEM,
does this short-circuit skip the final refill?

The only -ENOMEM source is napi_alloc_skb() failing in hns3_alloc_skb().
That also leaves ring->skb NULL, so the loop takes goto out.

At out, failure is already true. hns3_nic_alloc_rx_buffers() is therefore
never called, and its writel() to HNS3_RING_RX_RING_HEAD_REG is skipped.

Before this patch, failure was still false on this path, so the refill and
the head pointer sync that the comment above asks for always ran.

Now the cleaned BDs stay withheld from hardware until the next poll, which
can be up to 250ms later via oom_task. That includes reuse_flag BDs that
need no allocation at all.

Would writing it this way always attempt the refill?

	failure = hns3_nic_alloc_rx_buffers(ring, unused_count) || failure;

>  
> -	return failure ? budget : recv_pkts;
> +	if (unlikely(failure || hns3_ring_is_oom_state(ring)))
> +		hns3_oom_task_schedule(ring);
> +
> +	return recv_pkts;
>  }

[ ... ]

> @@ -5193,6 +5233,10 @@ int hns3_init_all_ring(struct hns3_nic_priv *priv)
>  	int i, j;
>  	int ret;
>  
> +	priv->rx_ring_oom_bm = bitmap_zalloc(h->kinfo.num_tqps, GFP_KERNEL);
> +	if (!priv->rx_ring_oom_bm)
> +		return -ENOMEM;

[Severity: High]
hns3_init_all_ring() is also called from hns3_set_ringparam() on a priv
that is already initialized, and there is no matching
hns3_uninit_all_ring():

hns3_set_ringparam() {
	...
	ret = hns3_init_all_ring(priv);
	if (ret) {
		...
		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
			memcpy(&priv->ring[i], &tmp_rings[i],
			       sizeof(struct hns3_enet_ring));
	} else {
		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
			hns3_fini_ring(&tmp_rings[i]);
	}
	...
	if (if_running)
		ret = ndev->netdev_ops->ndo_open(ndev);
}

On success, does this leak the previous rx_ring_oom_bm? The pointer is
overwritten here and nothing frees the old allocation.

On failure, priv->rx_ring_oom_bm ends up NULL. Either bitmap_zalloc()
failed, or a hns3_alloc_ring_memory() failure took the error path, which
now does:

	bitmap_free(priv->rx_ring_oom_bm);
	priv->rx_ring_oom_bm = NULL;
	return -ENOMEM;

The revert path in hns3_set_ringparam() only copies the ring structs back
and then reopens the device. The next RX poll goes:

hns3_nic_common_poll()
  hns3_clean_rx_ring()
    hns3_ring_is_oom_state()
      test_bit(ring->queue_index, priv->rx_ring_oom_bm);

failure is normally false at that point, so wouldn't this dereference a
NULL bitmap? If the interface was down during ethtool -G, the same thing
happens on the next ifup.

The comment in hns3_init_tx_spare_buffer() says hns3_init_all_ring() is
called from hns3_set_ringparam() with live state that is restored on
failure. Could the bitmap be allocated outside hns3_init_all_ring(),
somewhere hns3_set_ringparam() does not re-enter?

The later patch in this series, "net: hns3: fix use-after-free in debugfs
read during reset/unload", only adds dbg_mutex around this code and does
not address it.

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> --- 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)

[Severity: Medium]
Does this counter need to be a private ethtool -S string?

Per-queue RX allocation failures already have a standard uAPI. struct
netdev_queue_stats_rx has an alloc_fail member. It is reported as
rx-alloc-fail through the netdev netlink family when the driver
provides netdev_stat_ops->get_queue_stats_rx().

Documentation/networking/statistics.rst asks drivers not to add
counters to ethtool -S when they fit an existing standard category.
Once this string is in hns3_rxq_stats[], userspace starts to depend on
it, and it is hard to remove later.

hns3_ring_set_oom_state() only increments rx_oom_cnt when the ring's
bit in rx_ring_oom_bm goes from 0 to 1. So it counts OOM episodes
rather than individual failures. That is not quite what alloc_fail
means, but it is still a per-queue RX allocation failure count.

Could the driver implement get_queue_stats_rx() and report alloc_fail
per ring instead? The value could come from this counter or from the
existing failure sites in hns3_alloc_skb(), hns3_add_frag() and
hns3_nic_alloc_rx_buffers().

If the per-episode count is really needed to debug the oom_task
recovery, could the changelog explain why the standard counter is not
enough?

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

Re: [PATCH v3 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 6 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 (1) · Low: 3

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).

- [High] Within this series, the earlier commit "net: hns3: fix interface
  stuck after OOM with full hardware rx queue" makes…
- [High] net: hns3: uncanceled oom_task in hns3_client_uninit() and reset
  uninit
- [Medium] The init-side writers that rebuild and republish the debugfs-
  visible resources never take dbg_mutex, and they publish readiness…
- [Low] hns3_set_ringparam() is the third dbg_mutex writer, and it does
  not follow the writer protocol that the commit message and comments…
- [Low] The patch changes what userspace sees from several hns3 debugfs
  files, and the permanent commit body does not mention it.
- [Low] Sibling debugfs readers now report the same device state with
  different errno values.

Pre-existing issues:
- [Medium] In hns3_set_ringparam(), when hns3_init_all_ring() fails, ret
  holds the error and the old ring configuration is restored.

--

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 reset uninit, driver unload
and ethtool ring resize paths while they free or rebuild rings and
vectors. Readers also re-check the INITED/RESETTING state bits on every
loop iteration through a new helper, hns3_dbg_is_device_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..ec08672372f03 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c

[ ... ]

> @@ -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);
>  	}

[Severity: Medium]
Is the state check enough to order these readers against the reset init
side?

The uninit writers take dbg_mutex, but the init path that republishes
these resources does not. hns3_reset_notify_init_enet() assigns new
priv->ring, priv->tqp_vector, priv->vector_num and ring->desc without
dbg_mutex. It then publishes readiness with a plain bitop:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c: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() runs
clear_bit(HNS3_NIC_STATE_RESETTING, &priv->state), again without
dbg_mutex.

Neither set_bit() nor clear_bit() is ordered, and the test_bit() calls in
hns3_dbg_is_device_busy() have no acquire semantics. The reader's
mutex_lock() pairs only with an earlier unlock by an uninit writer or
another reader. It never pairs with the init writer.

On arm64, could a reader see INITED set and RESETTING clear but still load
the old priv->tqp_vector here? hns3_nic_dealloc_vector_data() frees that
array with devm_kfree() and does not clear the pointer, so the stale value
would point to freed memory. For priv->ring, the stale value would be NULL.

The window is probably very small in practice. Still, the publish side
relies only on plain state bits, which is the pattern the commit message
calls insufficient.

Would it work to set INITED and clear RESETTING while holding dbg_mutex?
Another option is to pair test_bit_acquire() on the reader side with
release ordering on the writer side.

[ ... ]

> @@ -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;
> +		}

[Severity: Low]
This changes what userspace sees from several debugfs files. Only the
v2/v3 notes below the --- line mention it, and those notes are dropped
when the patch is applied.

When the per-iteration check fails, hns3_dbg_rx_queue_info(),
hns3_dbg_tx_queue_info() and hns3_dbg_page_pool_info() now return -EBUSY
instead of -EPERM.

The "priv->ring is NULL" dev_err() messages are gone from the rx/tx queue
and page_pool readers.

hns3_dbg_page_pool_info() also used to fail with -EFAULT and log "page
pool is not initialized" when the first rx ring had no page_pool. It now
succeeds and hns3_dump_page_pool_info() prints a "page pool not
initialized" line for each ring.

Could the commit message mention these changes?

[ ... ]

> @@ -796,13 +828,9 @@ 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");
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
>  		return -EFAULT;
>  	}

[Severity: Low]
Should this return -EBUSY to match the sibling readers?

INITED is clear in every window where priv->ring is NULL. With their
pre-loop !priv->ring checks removed, hns3_dbg_rx_queue_info() and
hns3_dbg_tx_queue_info() now return -EBUSY in that state.

This reader keeps its pre-loop check and still returns -EFAULT for the
same state. One example is after hns3_reset_notify_uninit_enet() has
called hns3_put_ring_config() and before init has rebuilt the rings.

No user buffer is involved here, so EFAULT ("Bad address") looks like the
wrong errno.

[ ... ]

> 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);

[Severity: Low]
Does this writer follow the protocol described in the commit message? The
commit message says:

  Writers clear INITED first to signal readers, then acquire
  the mutex before freeing resources.

The reader loops also say "Per-iteration check bounds reset latency to one
iteration."

hns3_set_ringparam() only calls ndo_stop(), which sets
HNS3_NIC_STATE_DOWN. Before it takes dbg_mutex and rebuilds or frees the
rings, it never clears HNS3_NIC_STATE_INITED or sets
HNS3_NIC_STATE_RESETTING. As a result, hns3_dbg_is_device_busy() never
fires during a ring resize.

The mutex alone still keeps readers away from the rings, so there is no
use-after-free here. However, ethtool -G holds rtnl while it waits for a
whole show() pass, not one iteration. That pass can be a full tx_bd_info
or rx_bd_info dump of desc_num entries.

The dbg_mutex field comment in hnae3.h ("protect debugfs against
reset/unload") does not mention ring resize either.

Should the commit message and comments be updated, or should this path
also signal readers through the state bits?

> @@ -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]);
>  	}

[Severity: High]
Can this path leak priv->rx_ring_oom_bm, or leave it NULL?

The bitmap comes from the earlier patch in this series, "net: hns3: fix
interface stuck after OOM with full hardware rx queue". That patch makes
hns3_init_all_ring() always allocate it at the start:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_init_all_ring() {
	...
	priv->rx_ring_oom_bm = bitmap_zalloc(h->kinfo.num_tqps, GFP_KERNEL);
	if (!priv->rx_ring_oom_bm)
		return -ENOMEM;
	...
out_when_alloc_ring_memory:
	for (j = i - 1; j >= 0; j--)
		hns3_fini_ring(&priv->ring[j]);

	bitmap_free(priv->rx_ring_oom_bm);
	priv->rx_ring_oom_bm = NULL;
	return -ENOMEM;
}

hns3_set_ringparam() calls hns3_init_all_ring() on a live priv, where
rx_ring_oom_bm already points to the bitmap in use.

On success, the old rings are released only through hns3_fini_ring(),
which does not touch rx_ring_oom_bm. Only hns3_uninit_all_ring() frees
the bitmap. Does every successful ethtool -G leak the previous bitmap?

On failure, the old pointer has already been overwritten, and the new
bitmap is freed and set to NULL. The old rings are then copied back, and
the interface is reopened (or brought up later) with
rx_ring_oom_bm == NULL. Every NAPI rx poll then runs:

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

hns3_ring_is_oom_state()
    return test_bit(ring->queue_index, priv->rx_ring_oom_bm);

Wouldn't the first packet received after a failed resize cause a NULL
pointer dereference in softirq context? hns3_ring_set_oom_state() and
hns3_oom_task() dereference the same pointer.

> +	mutex_unlock(&h->dbg_mutex);
>  
>  	kfree(tmp_rings);
>  

[Severity: Medium]
This is a pre-existing issue that was not introduced by this patch, but
it is in the same function.

When hns3_init_all_ring() fails above, ret holds the error and the old
ring configuration is restored. The code after this hunk then does:

	if (if_running)
		ret = ndev->netdev_ops->ndo_open(ndev);

	return ret;

If hns3_nic_net_open() brings the restored rings back up and returns 0,
the earlier error is overwritten. Does ethtool -G then report success even
though the requested ring parameters were not applied?

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -5601,9 +5604,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);
>  }
>  

[Severity: High]
Can priv->oom_task still be pending when this teardown runs?

The oom work and priv->rx_ring_oom_bm look like they come from the
earlier patch in this series, "net: hns3: fix interface stuck after
OOM with full hardware rx queue". This patch reworks both teardown
paths, though, so it seems worth raising here.

hns3_nic_init_vector_data() sets up the work with
INIT_DELAYED_WORK(&priv->oom_task, hns3_oom_task). The only
cancel_delayed_work_sync() is in hns3_nic_net_down(), and
hns3_nic_uninit_vector_data() has no matching cancel.

hns3_client_uninit() and hns3_reset_notify_uninit_enet() only reach
hns3_nic_net_down() when HNS3_NIC_STATE_DOWN is clear. If DOWN is
already set, nothing cancels the work before hns3_uninit_all_ring()
frees priv->rx_ring_oom_bm and sets it to NULL. Nothing cancels it
before hns3_client_uninit() calls free_netdev() either, which frees
priv.

The work can be armed while DOWN is set. One case is the ethtool
offline selftest on a down interface. hns3_lp_run_test() goes through
hns3_lb_check_rx_ring() into hns3_clean_rx_ring(), which calls
hns3_oom_task_schedule() when an rx buffer or skb allocation fails.
hns3_selftest_restore() then leaves the interface down with the work
queued.

Another case is hns3_external_lb_prepare(). It sets DOWN with
test_and_set_bit() without going through hns3_nic_net_down(). If
hns3_external_lb_restore() bails out because of a concurrent reset or
a hns3_nic_reset_all_ring() failure, DOWN stays set and the work is
still pending.

If a reset or driver unload lands inside the delay window, the work
then runs:

hns3_oom_task()
    if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
        bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);
        return;
    }

After reset uninit, that is a NULL pointer dereference in
bitmap_zero(). After hns3_client_uninit(), the read of priv->state is
already a use-after-free of the freed netdev private area.

Should the work be cancelled unconditionally, either in
hns3_nic_uninit_vector_data() or in both uninit paths before
hns3_uninit_all_ring()? If so, does hns3_oom_task() take any lock
these paths already hold, such as rtnl or dbg_mutex? That would make a
synchronous cancel at that point deadlock.

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