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

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

[Jijie Shao]

This series contains three bug fixes for the HNS3 driver.

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

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

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

---
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 (3):
  net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data
  net: hns3: fix interface stuck after OOM with full hardware rx queue
  net: hns3: fix use-after-free in debugfs read during reset/unload

 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |   1 +
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 124 +++++++++++--------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  70 +++++++++-
 .../net/ethernet/hisilicon/hns3/hns3_enet.h   |  18 +++
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   3 +
 5 files changed, 167 insertions(+), 49 deletions(-)

base-commit: 305deb54c6c287e586bed92da2375ae1220e8dde
--
2.43.0

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

[Jijie Shao]

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

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

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

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

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

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

[Jijie Shao]

NAPI polling on hns3 is driven by hardware RX interrupts. Under
memory pressure, RX skb and frag allocations fail, 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-vector bitmap tracks which vectors have rings in OOM state;
the work function schedules NAPI for each affected vector. 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: 81ae0e0491f3 ("net: hns3: Add skb chain when num of RX buf exceeds MAX_SKB_FRAGS")
Signed-off-by: Jijie Shao <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 2f3aeade558c..09f688893aab 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -78,6 +78,8 @@ module_param(page_pool_enabled, bool, 0400);
 #define HNS3_MIN_TX_LEN		33U
 #define HNS3_MIN_TUN_PKT_LEN	65U
 
+#define HNS3_OOM_POLL_INTERVAL_MS	250
+
 /* hns3_pci_tbl - PCI Device ID Table
  *
  * Last entry must be all 0s
@@ -3793,6 +3795,7 @@ static int hns3_handle_rx_copybreak(struct sk_buff *skb, int i,
 
 		hns3_rl_err(ring_to_netdev(ring),
 			    "failed to allocate rx frag\n");
+		hns3_ring_set_oom_state(ring);
 		return -ENOMEM;
 	}
 
@@ -4162,6 +4165,7 @@ static int hns3_add_frag(struct hns3_enet_ring *ring)
 			if (unlikely(!new_skb)) {
 				hns3_rl_err(ring_to_netdev(ring),
 					    "alloc rx fraglist skb fail\n");
+				hns3_ring_set_oom_state(ring);
 				return -ENXIO;
 			}
 
@@ -4451,6 +4455,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->oom_vector_bm, priv->vector_num);
+		return;
+	}
+
+	netif_dbg(h, rx_err, netdev, "oom napi_schedule 0x%*pb\n",
+		  priv->vector_num, priv->oom_vector_bm);
+	for (i = 0; i < priv->vector_num; i++)
+		if (test_and_clear_bit(i, priv->oom_vector_bm))
+			napi_schedule(&priv->tqp_vector[i].napi);
+}
+
+static void hns3_oom_task_schedule(struct hns3_enet_ring *ring)
+{
+	struct hns3_nic_priv *priv = netdev_priv(ring_to_netdev(ring));
+
+	hns3_ring_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:
@@ -4865,9 +4905,18 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 		hns3_vector_coalesce_init(tqp_vector, priv);
 	}
 
+	priv->oom_vector_bm = bitmap_zalloc(vector_num, GFP_KERNEL);
+	if (!priv->oom_vector_bm) {
+		ret = -ENOMEM;
+		goto err_free_tqp_vector;
+	}
+
 	devm_kfree(&pdev->dev, vector);
 	return 0;
 
+err_free_tqp_vector:
+	devm_kfree(&pdev->dev, priv->tqp_vector);
+	priv->tqp_vector = NULL;
 err_put_vector:
 	for (i = 0; i < vector_num; i++)
 		h->ae_algo->ops->put_vector(h, vector[i].vector);
@@ -4889,6 +4938,7 @@ static void hns3_nic_uninit_vector_data(struct hns3_nic_priv *priv)
 	struct hns3_enet_tqp_vector *tqp_vector;
 	int i;
 
+	cancel_delayed_work_sync(&priv->oom_task);
 	for (i = 0; i < priv->vector_num; i++) {
 		tqp_vector = &priv->tqp_vector[i];
 
@@ -4920,6 +4970,7 @@ static void hns3_nic_dealloc_vector_data(struct hns3_nic_priv *priv)
 	struct pci_dev *pdev = h->pdev;
 	int i, ret;
 
+	bitmap_free(priv->oom_vector_bm);
 	for (i = 0; i < priv->vector_num; i++) {
 		struct hns3_enet_tqp_vector *tqp_vector;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 933e3527ed82..f71162af2da7 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -449,6 +449,7 @@ struct ring_stats {
 			u64 non_reuse_pg;
 			u64 frag_alloc_err;
 			u64 frag_alloc;
+			u64 rx_oom_cnt;
 		};
 		__le16 csum;
 	};
@@ -585,6 +586,8 @@ struct hns3_nic_priv {
 	struct hns3_enet_tqp_vector *tqp_vector;
 	u16 vector_num;
 	u8 max_non_tso_bd_num;
+	struct delayed_work oom_task;
+	unsigned long *oom_vector_bm;
 
 	u64 tx_timeout_count;
 
@@ -711,6 +714,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->tqp_vector->idx, priv->oom_vector_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->tqp_vector->idx, priv->oom_vector_bm);
+}
+
 void hns3_ethtool_set_ops(struct net_device *netdev);
 int hns3_set_channels(struct net_device *netdev,
 		      struct ethtool_channels *ch);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index e7318f236315..392653635bda 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -62,6 +62,7 @@ static const struct hns3_stats hns3_rxq_stats[] = {
 	HNS3_TQP_STAT("non_reuse_pg", non_reuse_pg),
 	HNS3_TQP_STAT("frag_alloc_err", frag_alloc_err),
 	HNS3_TQP_STAT("frag_alloc", frag_alloc),
+	HNS3_TQP_STAT("rx_oom_cnt", rx_oom_cnt),
 };
 
 #define HNS3_PRIV_FLAGS_LEN ARRAY_SIZE(hns3_priv_flags)
-- 
2.43.0

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

[Jijie Shao]

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

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

Fix by adding a mutex (dbg_mutex) to struct hnae3_handle, 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: 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 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    | 124 +++++++++++-------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |   7 +
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |   2 +
 4 files changed, 88 insertions(+), 46 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..40d715924e58 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);
 	}
 
@@ -796,13 +823,9 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
-	if (!priv->ring) {
-		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
-		return -EFAULT;
-	}
-
-	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
-		dev_err(&h->pdev->dev, "page pool is not initialized\n");
+	mutex_lock(&priv->ae_handle->dbg_mutex);
+	if (!priv->ring || !priv->ring[h->kinfo.num_tqps].page_pool) {
+		mutex_unlock(&priv->ae_handle->dbg_mutex);
 		return -EFAULT;
 	}
 
@@ -810,14 +833,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 +852,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))
-		return -EBUSY;
+	mutex_lock(&priv->ae_handle->dbg_mutex);
+	if (!priv->ring) {
+		mutex_unlock(&priv->ae_handle->dbg_mutex);
+		return -EFAULT;
+	}
 
 	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 09f688893aab..aa5332e4dd29 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5450,6 +5450,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);
 
@@ -5563,6 +5564,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;
 }
@@ -5586,6 +5588,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);
@@ -5597,9 +5600,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);
 }
 
@@ -5876,6 +5881,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);
@@ -5888,6 +5894,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 392653635bda..cb6a23085be4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -1258,6 +1258,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
 	if (if_running)
 		ndev->netdev_ops->ndo_stop(ndev);
 
+	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);
@@ -1276,6 +1277,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 v2 net 0/3] net: hns3: three fixes for hns3 driver

[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 v2 net 1/3] 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 says it sets `priv->vector_num = 0` in the new
  `err_put_vector` path of `hns3_nic_alloc_vector_data()`…

Pre-existing issues:
- [Medium] `hns3_nic_alloc_vector_data()` accepts a 0 return from
  `get_vector()` as success.

--

net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

This patch fixes the devm_kcalloc() failure path in
hns3_nic_alloc_vector_data(). On that path, the MSI-X vectors obtained
from get_vector() are now handed back with put_vector() and
priv->vector_num is reset to 0. The success and error exits are also
split.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 47788be64be68..2f3aeade558cd 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;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
happens if get_vector() returns 0 here?

hns3_nic_alloc_vector_data() {
    ...
	vector_num = h->ae_algo->ops->get_vector(h, vector_num, vector);

	priv->vector_num = vector_num;
	priv->tqp_vector = (struct hns3_enet_tqp_vector *)
		devm_kcalloc(&pdev->dev, vector_num, sizeof(*priv->tqp_vector),
			     GFP_KERNEL);
    ...
}

With a zero count, devm_kcalloc() returns ZERO_SIZE_PTR rather than NULL.
The -ENOMEM check above would not fire, and the function would return
success.

hns3_nic_init_vector_data() would then run:

	for (i = 0; i < h->kinfo.num_tqps; i++) {
		u16 vector_i = i % priv->vector_num;
		...
		tqp_vector = &priv->tqp_vector[vector_i];

Wouldn't that divide by zero, with priv->tqp_vector pointing at
ZERO_SIZE_PTR?

This looks hard to hit in practice. In hclge_get_vector(), a zero return
needs num_msi_left (or num_nic_msi - 1) to be 0. The HNAE3_MIN_VECTOR_NUM
checks and the pci_alloc_irq_vectors() minimum normally prevent that. The
leak closed by this patch was the only known way to drain num_msi_left.

Would a zero check on the get_vector() result still be worth adding?

[ ... ]

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

[Severity: Low]
The commit message says:

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

Is the dealloc path reachable after hns3_nic_alloc_vector_data() fails?

On failure, hns3_client_init() jumps to out_alloc_vector_data, and
hns3_reset_notify_init_enet() jumps to err_put_ring:

	ret = hns3_nic_alloc_vector_data(priv);
	if (ret)
		goto err_put_ring;

Both labels are below the calls to hns3_nic_uninit_vector_data() and
hns3_nic_dealloc_vector_data(), so neither helper runs.

The later teardown paths, hns3_reset_notify_uninit_enet() and
hns3_client_uninit(), return early. HNS3_NIC_STATE_INITED is only set
after init fully succeeds:

	if (!test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state)) {
		netdev_warn(netdev, "already uninitialized\n");
		return 0;
	}

hns3_change_channels() also only calls UNINIT_CLIENT once INIT_CLIENT has
succeeded.

The reader that does benefit from the reset seems to be the debugfs
coalesce dump. hns3_dump_coal_info() loops over priv->vector_num and
indexes priv->tqp_vector[i] without an INITED check:

	for (i = 0; i < priv->vector_num; i++) {
		tqp_vector = &priv->tqp_vector[i];
		hns3_get_coal_info(tqp_vector, s, i, is_tx);
	}

Could the commit message name that path instead of the dealloc path?

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · 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).

- [Medium] The patch adds a new driver-private per-RX-queue `ethtool -S`
  string, "rx_oom_cnt", to hns3_rxq_stats[] in…
- [Medium] hns3_oom_task() (hns3_enet.c:4466-4468) does `if
  (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {…
- [Medium] net: hns3: broken per-ring rx_oom_cnt in
  hns3_ring_set_oom_state()
- [Medium] net: hns3: spurious OOM state set in
  hns3_handle_rx_copybreak()
- [Low] The patch adds `if (unlikely(err == -ENOMEM)) failure = true;` in
  hns3_clean_rx_ring() (hns3_enet.c:4508-4509), but the exit refill is…

--

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

This patch adds a 250ms delayed_work that reschedules NAPI for vectors
whose RX rings hit allocation failures. Those vectors are tracked in a
per-vector bitmap. The patch also stops hns3_clean_rx_ring() returning
budget on allocation failure and adds an rx_oom_cnt ethtool -S counter.

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

[ ... ]

> @@ -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->oom_vector_bm, priv->vector_num);
> +		return;
> +	}

[Severity: Medium]
Is this bitmap_zero() safe against the NAPI producers, and against the
interface coming back up?

On stop, hns3_nic_net_stop() sets HNS3_NIC_STATE_DOWN before NAPI is
disabled:

hns3_nic_net_stop()
    test_and_set_bit(HNS3_NIC_STATE_DOWN, &priv->state)
    ...
    hns3_nic_net_down()
        hns3_disable_irqs_and_tqps()
            hns3_irq_disable()    <- napi_disable() happens here

So a poll that is still running can call test_and_set_bit() in
hns3_ring_set_oom_state() while this worker does a plain memset of the
same word on another CPU.

Isn't that a data race? Can a bit also survive the clear on DOWN?

On reopen, neither hns3_nic_net_stop() nor hns3_nic_net_up() cancels or
flushes oom_task. cancel_delayed_work_sync() only runs in
hns3_nic_uninit_vector_data(). Consider this sequence:

kworker                         open + NAPI
hns3_oom_task()
  test_bit(DOWN) is true
  <preempted>
                                hns3_nic_net_up()
                                  clear_bit(HNS3_NIC_STATE_DOWN, ...)
                                  hns3_enable_irqs_and_tqps()
                                hns3_clean_rx_ring() hits OOM
                                  hns3_oom_task_schedule()
                                    test_and_set_bit(idx, ...)
                                    schedule_delayed_work()
                                      (succeeds, work is running
                                       but not pending)
  bitmap_zero(priv->oom_vector_bm, ...)

When the requeued work runs 250ms later, it finds no bits set and calls
no napi_schedule(). Nothing requeues it after that.

If the RX ring has filled and RX interrupts have stopped by then,
wouldn't the interface get stuck again, which is the problem this patch
is meant to fix?

Would it help to clear only individual bits with test_and_clear_bit()?
Alternatively, the stop path could call
cancel_delayed_work_sync(&priv->oom_task) after napi_disable().

> +
> +	netif_dbg(h, rx_err, netdev, "oom napi_schedule 0x%*pb\n",
> +		  priv->vector_num, priv->oom_vector_bm);
> +	for (i = 0; i < priv->vector_num; i++)
> +		if (test_and_clear_bit(i, priv->oom_vector_bm))
> +			napi_schedule(&priv->tqp_vector[i].napi);
> +}

[Severity: Medium]
rx_oom_cnt is stored in the per-ring struct ring_stats and reported
per RX queue through hns3_rxq_stats[]. The increment here, though, is
gated on a per-vector bit. Can the per-queue values be trusted?

hns3_nic_init_vector_data() maps rings to vectors with
vector_i = i % priv->vector_num. vector_num is at most
min(num_online_cpus(), num_tqps), and get_vector() can return fewer.
Whenever num_tqps is larger than vector_num, several RX rings share
one tqp_vector, and hns3_nic_common_poll() walks all of them in
rx_group.

The first ring on the vector to fail flips the bit from 0 to 1 and is
counted. Another ring on the same vector may also fail before
hns3_oom_task() clears the bit. That ring sees the bit already set, so
its rxqN_rx_oom_cnt never moves. The per-queue numbers then depend on
ring order within the group, not on which queue actually ran out of
memory.

The changelog says the counter increments once per OOM event, but that
does not hold either. hns3_oom_task() clears the bit with
test_and_clear_bit() before the NAPI poll it schedules has succeeded.
Under sustained memory pressure the next poll fails again and bumps
the counter. One long OOM episode is counted about four times a
second.

Is the intent to count per queue or per vector? If per queue, could
the count be taken at the failure sites without the vector bit
gating it? The bit would then only drive the recovery work.

[ ... ]

> @@ -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]
With failure already set by the new -ENOMEM check above, doesn't this
short-circuit skip hns3_nic_alloc_rx_buffers() on exit?

The -ENOMEM comes from hns3_alloc_skb() when napi_alloc_skb() fails for
the skb head. That leaves ring->skb NULL and takes the goto out.

Because failure is already true, the BDs cleaned in this poll are never
re-armed. That can be up to 15 BDs, since the refill at the top of the
loop runs at 16. HNS3_RING_RX_RING_HEAD_REG is not written for them
either. This includes BDs with reuse_flag set, which need no allocation.

Before this patch, failure stayed false in this case and the exit refill
ran.

Under sustained skb head allocation failure, every poll triggered by the
OOM work could fail on the first BD and skip the refill again. Should the
refill always be attempted, something like:

	if (unused_count > 0 &&
	    hns3_nic_alloc_rx_buffers(ring, unused_count))
		failure = true;

[ ... ]

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

[Severity: Medium]
Should this go through the standard per-queue stats rather than a new
ethtool -S string?

struct netdev_queue_stats_rx in include/net/netdev_queues.h already has
alloc_fail. Netlink exposes it as rx-alloc-fail: "Number of times skb or
buffer allocation failed on the Rx datapath". hns3 does not implement
netdev_stat_ops today.

The new counter also does not count each failure:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:hns3_ring_set_oom_state() {
	if (!test_and_set_bit(ring->tqp_vector->idx, priv->oom_vector_bm))
		hns3_ring_stats_update(ring, rx_oom_cnt);
}

It only increments when a per-vector bit goes from 0 to 1, and
hns3_oom_task() clears that bit every 250ms. Its value therefore depends
on when the recovery work runs.

Since this is a Fixes: tagged change, the new ethtool string also becomes
permanent ABI and makes the stable backport bigger.

Could the counter be dropped from this fix? Or could it be reported as
alloc_fail through netdev_stat_ops->get_queue_stats_rx(), counted at each
failure site?

[ ... ]

> @@ -3793,6 +3795,7 @@ static int hns3_handle_rx_copybreak(struct sk_buff *skb, int i,
>  
>  		hns3_rl_err(ring_to_netdev(ring),
>  			    "failed to allocate rx frag\n");
> +		hns3_ring_set_oom_state(ring);
>  		return -ENOMEM;
>  	}
>  

[Severity: Medium]
Does a failed napi_alloc_frag() here leave the ring in a state that
needs the recovery work?

hns3_nic_reuse_page() returns void, so the -ENOMEM from
hns3_handle_rx_copybreak() goes no further. hns3_nic_reuse_page()
falls through to out: and attaches desc_cb->priv to the skb with
skb_add_rx_frag(), as it does for other non-reusable buffers. The
caller then advances next_to_clean and the packet is delivered. The
new err == -ENOMEM check in hns3_clean_rx_ring() never sees this
failure.

hns3_nic_alloc_rx_buffers() refills the consumed descriptor like any
other BD. If that refill fails, failure is already set and
hns3_oom_task_schedule() runs anyway. If it succeeds, nothing is
stuck.

With this call, though, the vector bit is set for a packet that was
received normally. hns3_ring_is_oom_state() at the end of
hns3_clean_rx_ring() then schedules oom_task, and rx_oom_cnt is
bumped. frag_alloc_err already counts this failure.

Could this call be dropped? The OOM state would then only be set
where a BD cannot be reaped or re-armed.

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

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

[netdev-bot+sashiko]

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

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

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] dbg_mutex is taken only on the teardown paths
  (hns3_reset_notify_uninit_enet, hns3_client_uninit,…
- [Medium] The Fixes tag and changelog put the whole root cause on
  eced3d1c41db ("net: hns3: use seq_file for files in queue/ in…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  rewrites the guard and makes the outcome worse.
- [Medium] This problem wasn't introduced by this patch, but
  hns3_set_ringparam() reports success for a failed ring resize when the…
- [Medium] This problem wasn't introduced by this patch, but
  hns3_alloc_ring_memory() leaks a newly created RX page pool when RX…

--

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, client
uninit and ethtool ring resize paths while they free rings and vectors.
The debugfs readers also re-check INITED/RESETTING on every loop
iteration.

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

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

[ ... ]

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

[Severity: Medium]
Is this Fixes tag complete?

Before eced3d1c41db, hns3_dbg_read() did a single INITED/RESETTING test
before running the dump. It shared no lock with
hns3_reset_notify_uninit_enet() or hns3_client_uninit. That is the same
window the changelog describes as "Plain state-bit checks leave a TOCTOU
window between check and use".

The ring resize race is older still. Since a723fb8efe29d3 ("net: hns3:
refine for set ring parameters"), hns3_set_ringparam() has freed the old
descriptors with:

	for (i = 0; i < h->kinfo.num_tqps * 2; i++)
		hns3_fini_ring(&tmp_rings[i]);

It never clears HNS3_NIC_STATE_INITED or sets HNS3_NIC_STATE_RESETTING.
So no state-bit check, old or new, would stop a concurrent tx_bd_info or
rx_bd_info reader from walking ring->desc. Also, eced3d1c41db only
converted rx_queue_info and queue_map.

Stable trees that predate eced3d1c41db have the debugfs vs ring resize
use-after-free. If backports go by this tag alone, would those trees be
skipped? Should a723fb8efe29d3 get a Fixes tag as well, and should the
changelog's wording about ethtool ring resize be adjusted?

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> index 1347edac76993..40d715924e588 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c

[ ... ]

> @@ -796,13 +823,9 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
>  	struct hns3_enet_ring *ring;
>  	u32 i;
>  
> -	if (!priv->ring) {
> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
> -		return -EFAULT;
> -	}
> -
> -	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
> -		dev_err(&h->pdev->dev, "page pool is not initialized\n");
> +	mutex_lock(&priv->ae_handle->dbg_mutex);
> +	if (!priv->ring || !priv->ring[h->kinfo.num_tqps].page_pool) {
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
>  		return -EFAULT;
>  	}
>  
> @@ -810,14 +833,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);

[Severity: High]
This is a pre-existing issue, but the check above only looks at the
page_pool of the first RX ring. The loop then passes every RX ring to
hns3_dump_page_pool_info(), which dereferences ring->page_pool without
a NULL check:

	READ_ONCE(ring->page_pool->pages_state_hold_cnt),
	atomic_read(&ring->page_pool->pages_state_release_cnt),
	ring->page_pool->p.pool_size,

hns3_alloc_page_pool() creates the pool separately for each RX ring. On
failure it leaves the pool NULL and ring init carries on:

	ring->page_pool = page_pool_create(&pp_params);
	if (IS_ERR(ring->page_pool)) {
		dev_warn(...);
		ring->page_pool = NULL;
	}

If the first RX ring got a pool and a later one did not (for example
after a transient allocation failure), can reading page_pool_info oops on
the NULL page_pool?

With this patch the oops would happen while dbg_mutex is held. Would
later calls to hns3_reset_notify_uninit_enet(), hns3_client_uninit() or
hns3_set_ringparam() then block forever in
mutex_lock(&handle->dbg_mutex)?

>  	}
>  
> +	mutex_unlock(&priv->ae_handle->dbg_mutex);
>  	return 0;
>  }
>  
> @@ -826,17 +852,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))
> -		return -EBUSY;
> +	mutex_lock(&priv->ae_handle->dbg_mutex);
> +	if (!priv->ring) {
> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
> +		return -EFAULT;
> +	}

[Severity: High]
Can a debugfs reader still dereference a freed priv->ring during reset
re-init?

hns3_reset_notify_uninit_enet(), hns3_client_uninit() and
hns3_set_ringparam() all take dbg_mutex, but
hns3_reset_notify_init_enet() never does. That function publishes a new
priv->ring while INITED is still clear. Its error unwind then frees
everything again without the mutex:

hns3_reset_notify_init_enet() {
	ret = hns3_get_ring_config(priv);
	...
err_init_irq_fail:
	hns3_uninit_all_ring(priv);
err_uninit_vector:
	hns3_nic_uninit_vector_data(priv);
err_dealloc_vector:
	hns3_nic_dealloc_vector_data(priv);
err_put_ring:
	hns3_put_ring_config(priv);
	...
}

The INITED/RESETTING entry check has been replaced with a bare priv->ring
check. So a reader in this window takes dbg_mutex unopposed and passes
the check. It then enters hns3_dbg_tx_bd_info() or hns3_dbg_rx_bd_info(),
and both dereference priv->ring before their first busy check:

	ring = &priv->ring[data->qid + data->handle->kinfo.num_tqps];
	for (i = 0; i < ring->desc_num; i++) {
		if (hns3_dbg_is_device_busy(priv))
			return -EBUSY;

If hns3_put_ring_config() runs at the same moment, the ring->desc_num
load reads devm-freed memory. If priv->ring has already been set back to
NULL, it faults on NULL plus an offset.

hns3_dbg_page_pool_info() looks to have the same problem, because its
check indexes priv->ring[h->kinfo.num_tqps] before any busy check.

Before this patch, hns3_dbg_bd_info_show() returned -EBUSY in this
window because INITED was clear. The changelog says the mutex "rejects a
reader that arrives after the writer has finished tearing down resources
(priv->ring is NULL by then)". That doesn't seem to hold once re-init has
reallocated priv->ring.

Would it work to take dbg_mutex in hns3_reset_notify_init_enet() around
the allocation, the error unwind and set_bit(HNS3_NIC_STATE_INITED)?
Alternatively, the busy check could stay ahead of the first priv->ring
dereference.

[ ... ]

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 392653635bda2..cb6a23085be48 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -1258,6 +1258,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>  	if (if_running)
>  		ndev->netdev_ops->ndo_stop(ndev);
>  
> +	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: Medium]
This is a pre-existing issue, but can the hns3_init_all_ring() call that
follows here leak an RX page pool when buffer allocation fails?

hns3_alloc_ring_memory() creates the pool and then allocates the
buffers:

	if (!HNAE3_IS_TX_RING(ring)) {
		if (page_pool_enabled)
			hns3_alloc_page_pool(ring);

		ret = hns3_alloc_ring_buffers(ring);
		if (ret)
			goto out_with_desc;
	...
out_with_desc:
	hns3_free_desc(ring);
out_with_desc_cb:
	devm_kfree(ring_to_dev(ring), ring->desc_cb);

The error path never calls page_pool_destroy(ring->page_pool). And
hns3_init_all_ring() only unwinds the rings before the failing one:

	for (j = i - 1; j >= 0; j--)
		hns3_fini_ring(&priv->ring[j]);

After that the pointer is lost. hns3_set_ringparam() memcpy()s the saved
ring over priv->ring[i], and the probe/reset paths free priv->ring via
hns3_put_ring_config().

Would the page_pool, its ptr_ring and any pages that
hns3_alloc_ring_buffers() already returned to it be leaked for good?

> @@ -1276,6 +1277,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);
>  

[Severity: Medium]
This is a pre-existing issue, but suppose if_running is true and
hns3_init_all_ring() fails. The revert path restores the old ring
parameters, and then ret is overwritten just after this point:

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

	return ret;

If the reopen succeeds, does hns3_set_ringparam() return 0 for a resize
that was rolled back, so that ethnl_set_rings() reports success to
userspace?

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