[PATCH net-next v5] net: Wangxun: protect ring accesses with RCU

[PATCH net-next v5] net: Wangxun: protect ring accesses with RCU

[Mengyuan Lou]

During queue teardown, interface stop, or channel reconfiguration (e.g. via
ethtool -L), ring pointers in wx->rx_ring[] and wx->tx_ring[] can be
cleared to NULL and freed asynchronously via kfree_rcu().

Concurrency between interface reconfiguration and background tasks (such
as service tasks, dev_get_stats, or PTP watchdogs) can result in NULL
pointer dereferences or Use-After-Free (UAF) issues when accessing
per-queue structures.

To fix these concurrency issues and ensure memory safety:
1. Annotate tx_ring[] and rx_ring[] arrays in 'struct wx' with __rcu.
2. Enclose lockless reader paths (such as wx_update_stats(), xoff handling,
   error recovery, and PTP watchdog status checks) inside RCU read-side
   critical sections (rcu_read_lock/unlock) and access ring pointers via
   rcu_dereference().
3. Add proper NULL pointer checks when iterating over rings in RCU read
   sections to guard against queues that are being torn down.
4. Use rcu_assign_pointer() when publishing or clearing ring pointers
   during queue vector allocation and freeing to guarantee proper release
   semantics.
5. Use rcu_dereference_protected() in control paths (such as HW setup,
   ring reconfiguration, and cleanup functions) where ring accesses are
   strictly guarded by outer locks (e.g. RTNL or dev state locks) rather
   than RCU.

Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
Changelogs:
v5:
- Switched to rcu_dereference()/rcu_assign_pointer().
- Since the code involved is extensive, rewrite the patch description to
  suit `net-next` rather than a bug fix, and remove the `Fixes` tag.
- Solve warnings from Sparse with rcu_dereference_protected().
v4: https://lore.kernel.org/netdev/4353E83B1147D652+20260830070624.7410-1-mengyuanlou@net-swift.com/
- Switched from rcu_dereference()/rcu_assign_pointer() back to
  READ_ONCE()/WRITE_ONCE().
- Standardized on READ_ONCE() and WRITE_ONCE() for all ring pointer
  accesses and assignments across the driver.
- Added RCU read-side critical sections (rcu_read_lock/unlock) and NULL
  pointer checks to wx_get_ethtool_stats() in wx_ethtool.c and
  wx_vlan_strip_control() in wx_hw.c.
- Updated commit message to explicitly document the expanded protection
  in ethtool statistics gathering and VLAN strip configuration tasks.
v3: https://lore.kernel.org/netdev/20260818100841.37483-1-mengyuanlou@net-swift.com/
- Replaced READ_ONCE() with rcu_dereference() when reading __rcu annotated
  rx_ring and tx_ring pointers to fix Sparse static checker warnings and
  enable Lockdep runtime validation.
- Wrapped wx_update_xoff_rx_lfc() with its own rcu_read_lock() and
  rcu_read_unlock() section to ensure self-contained protection regardless
  of caller context.
- Extended RCU read-side critical sections and NULL pointer checks to other
  background tasks accessing ring arrays, including wx_ring_tx_pending(),
  wx_detect_tx_hang() in wx_err.c, and wx_ptp_rx_hang() in wx_ptp.c.
- Updated commit message to accurately reflect the use of rcu_dereference()
  and the expanded scope of protected functions.
v2: https://lore.kernel.org/netdev/20260818100841.37483-1-mengyuanlou@net-swift.com/
- Moved rcu_read_unlock() after wx_update_xoff_rx_lfc() in wx_update_stats()
  to ensure flow control processing remains fully protected within the RCU
  read-side critical section.
- Replaced WRITE_ONCE() with rcu_assign_pointer() when assigning and clearing
  ring pointers in wx_alloc_q_vector() and wx_free_q_vector(), providing
  proper release memory barrier semantics for lockless RCU readers.
- Added __rcu annotations to tx_ring and rx_ring in struct wx (wx_type.h) to
  align with Linux kernel RCU coding standards and fix Sparse warnings.
v1: https://lore.kernel.org/netdev/20260813095141.88227-1-mengyuanlou@net-swift.com/
---
 drivers/net/ethernet/wangxun/libwx/wx_err.c   | 20 ++++--
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   | 10 +--
 drivers/net/ethernet/wangxun/libwx/wx_hw.c    | 67 ++++++++++++++-----
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   | 63 +++++++++--------
 drivers/net/ethernet/wangxun/libwx/wx_ptp.c   |  6 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  4 +-
 .../net/ethernet/wangxun/libwx/wx_vf_common.c |  2 +-
 .../net/ethernet/wangxun/libwx/wx_vf_lib.c    |  7 +-
 .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c  |  4 +-
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  4 +-
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  6 +-
 .../net/ethernet/wangxun/txgbe/txgbe_fdir.c   |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  4 +-
 13 files changed, 132 insertions(+), 67 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
index b56fbdc959de..8eadb4d3abcf 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
@@ -201,12 +201,18 @@ static bool wx_ring_tx_pending(struct wx *wx)
 {
 	int i;
 
+	rcu_read_lock();
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		struct wx_ring *tx_ring = wx->tx_ring[i];
+		struct wx_ring *tx_ring = rcu_dereference(wx->tx_ring[i]);
 
-		if (tx_ring->next_to_use != tx_ring->next_to_clean)
+		if (!tx_ring)
+			continue;
+		if (tx_ring->next_to_use != tx_ring->next_to_clean) {
+			rcu_read_unlock();
 			return true;
+		}
 	}
+	rcu_read_unlock();
 
 	return false;
 }
@@ -264,8 +270,14 @@ static void wx_detect_tx_hang(struct wx *wx)
 
 	/* Force detection of hung controller */
 	if (netif_carrier_ok(wx->netdev)) {
-		for (i = 0; i < wx->num_tx_queues; i++)
-			set_bit(WX_TX_DETECT_HANG, wx->tx_ring[i]->state);
+		rcu_read_lock();
+		for (i = 0; i < wx->num_tx_queues; i++) {
+			struct wx_ring *tx_ring = rcu_dereference(wx->tx_ring[i]);
+
+			if (tx_ring)
+				set_bit(WX_TX_DETECT_HANG, tx_ring->state);
+		}
+		rcu_read_unlock();
 	}
 }
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 940d2e59876c..eae5952c0f5f 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -154,8 +154,9 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 		}
 	}
 
+	rcu_read_lock();
 	for (j = 0; j < netdev->num_tx_queues; j++) {
-		ring = wx->tx_ring[j];
+		ring = rcu_dereference(wx->tx_ring[j]);
 		if (!ring) {
 			data[i++] = 0;
 			data[i++] = 0;
@@ -170,7 +171,7 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 		i += 2;
 	}
 	for (j = 0; j < WX_NUM_RX_QUEUES; j++) {
-		ring = wx->rx_ring[j];
+		ring = rcu_dereference(wx->rx_ring[j]);
 		if (!ring) {
 			data[i++] = 0;
 			data[i++] = 0;
@@ -184,6 +185,7 @@ void wx_get_ethtool_stats(struct net_device *netdev,
 		} while (u64_stats_fetch_retry(&ring->syncp, start));
 		i += 2;
 	}
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL(wx_get_ethtool_stats);
 
@@ -805,9 +807,9 @@ static int wx_set_ringparam_vf(struct net_device *netdev,
 
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
-			wx->tx_ring[i]->count = new_tx_count;
+			rcu_dereference_protected(wx->tx_ring[i], 1)->count = new_tx_count;
 		for (i = 0; i < wx->num_rx_queues; i++)
-			wx->rx_ring[i]->count = new_rx_count;
+			rcu_dereference_protected(wx->rx_ring[i], 1)->count = new_rx_count;
 		wx->tx_ring_count = new_tx_count;
 		wx->rx_ring_count = new_rx_count;
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index 122c4952d203..c78136abe92f 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -1598,13 +1598,17 @@ static void wx_vlan_strip_control(struct wx *wx, bool enable)
 {
 	int i, j;
 
+	rcu_read_lock();
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		struct wx_ring *ring = wx->rx_ring[i];
+		struct wx_ring *ring = rcu_dereference(wx->rx_ring[i]);
 
+		if (!ring)
+			continue;
 		j = ring->reg_idx;
 		wr32m(wx, WX_PX_RR_CFG(j), WX_PX_RR_CFG_VLAN,
 		      enable ? WX_PX_RR_CFG_VLAN : 0);
 	}
+	rcu_read_unlock();
 }
 
 static void wx_vlan_promisc_enable(struct wx *wx)
@@ -1797,7 +1801,7 @@ static void wx_set_rx_buffer_len(struct wx *wx)
 	 * the Base and Length of the Rx Descriptor Ring
 	 */
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		rx_ring = wx->rx_ring[i];
+		rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
 		rx_ring->rx_buf_len = WX_RXBUFFER_2K;
 #if (PAGE_SIZE < 8192)
 		if (test_bit(WX_FLAG_RSC_ENABLED, wx->flags))
@@ -2021,8 +2025,11 @@ static void wx_configure_tx(struct wx *wx)
 	      WX_TDM_CTL_TE, WX_TDM_CTL_TE);
 
 	/* Setup the HW Tx Head and Tail descriptor pointers */
-	for (i = 0; i < wx->num_tx_queues; i++)
-		wx_configure_tx_ring(wx, wx->tx_ring[i]);
+	for (i = 0; i < wx->num_tx_queues; i++) {
+		struct wx_ring *tx_ring = rcu_dereference_protected(wx->tx_ring[i], 1);
+
+		wx_configure_tx_ring(wx, tx_ring);
+	}
 
 	wr32m(wx, WX_TSC_BUF_AE, WX_TSC_BUF_AE_THR, 0x10);
 
@@ -2247,8 +2254,11 @@ void wx_configure_rx(struct wx *wx)
 	/* Setup the HW Rx Head and Tail Descriptor Pointers and
 	 * the Base and Length of the Rx Descriptor Ring
 	 */
-	for (i = 0; i < wx->num_rx_queues; i++)
-		wx_configure_rx_ring(wx, wx->rx_ring[i]);
+	for (i = 0; i < wx->num_rx_queues; i++) {
+		struct wx_ring *rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
+
+		wx_configure_rx_ring(wx, rx_ring);
+	}
 
 	/* Enable all receives, disable security engine prior to block traffic */
 	ret = wx_disable_sec_rx_path(wx);
@@ -2838,11 +2848,17 @@ int wx_fc_enable(struct wx *wx, bool tx_pause, bool rx_pause)
 	 *  and performance reasons.
 	 */
 	if (wx->num_rx_queues > 1 && !tx_pause) {
-		for (i = 0; i < wx->num_rx_queues; i++)
-			wx_enable_rx_drop(wx, wx->rx_ring[i]);
+		for (i = 0; i < wx->num_rx_queues; i++) {
+			struct wx_ring *rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
+
+			wx_enable_rx_drop(wx, rx_ring);
+		}
 	} else {
-		for (i = 0; i < wx->num_rx_queues; i++)
-			wx_disable_rx_drop(wx, wx->rx_ring[i]);
+		for (i = 0; i < wx->num_rx_queues; i++) {
+			struct wx_ring *rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
+
+			wx_disable_rx_drop(wx, rx_ring);
+		}
 	}
 
 	return 0;
@@ -2870,8 +2886,15 @@ static void wx_update_xoff_rx_lfc(struct wx *wx)
 	if (!data)
 		return;
 
-	for (i = 0; i < wx->num_tx_queues; i++)
-		clear_bit(WX_HANG_CHECK_ARMED, wx->tx_ring[i]->state);
+	rcu_read_lock();
+	for (i = 0; i < wx->num_tx_queues; i++) {
+		struct wx_ring *tx_ring = rcu_dereference(wx->tx_ring[i]);
+
+		if (!tx_ring)
+			continue;
+		clear_bit(WX_HANG_CHECK_ARMED, tx_ring->state);
+	}
+	rcu_read_unlock();
 }
 
 /**
@@ -2893,10 +2916,13 @@ void wx_update_stats(struct wx *wx)
 
 	spin_lock(&wx->hw_stats_lock);
 
+	rcu_read_lock();
 	/* gather some stats to the wx struct that are per queue */
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		struct wx_ring *rx_ring = wx->rx_ring[i];
+		struct wx_ring *rx_ring = rcu_dereference(wx->rx_ring[i]);
 
+		if (!rx_ring)
+			continue;
 		non_eop_descs += rx_ring->rx_stats.non_eop_descs;
 		alloc_rx_buff_failed += rx_ring->rx_stats.alloc_rx_buff_failed;
 		hw_csum_rx_good += rx_ring->rx_stats.csum_good_cnt;
@@ -2912,19 +2938,28 @@ void wx_update_stats(struct wx *wx)
 		u64 rsc_flush = 0;
 
 		for (i = 0; i < wx->num_rx_queues; i++) {
-			rsc_count += wx->rx_ring[i]->rx_stats.rsc_count;
-			rsc_flush += wx->rx_ring[i]->rx_stats.rsc_flush;
+			struct wx_ring *rx_ring = rcu_dereference(wx->rx_ring[i]);
+
+			if (!rx_ring)
+				continue;
+
+			rsc_count += rx_ring->rx_stats.rsc_count;
+			rsc_flush += rx_ring->rx_stats.rsc_flush;
 		}
 		wx->rsc_count = rsc_count;
 		wx->rsc_flush = rsc_flush;
 	}
 
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		struct wx_ring *tx_ring = wx->tx_ring[i];
+		struct wx_ring *tx_ring = rcu_dereference(wx->tx_ring[i]);
+
+		if (!tx_ring)
+			continue;
 
 		restart_queue += tx_ring->tx_stats.restart_queue;
 		tx_busy += tx_ring->tx_stats.tx_busy;
 	}
+	rcu_read_unlock();
 	wx->restart_queue = restart_queue;
 	wx->tx_busy = tx_busy;
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index ed5aad7857bd..9f90aa92f840 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -1714,7 +1714,12 @@ netdev_tx_t wx_xmit_frame(struct sk_buff *skb,
 
 	if (r_idx >= wx->num_tx_queues)
 		r_idx = r_idx % wx->num_tx_queues;
-	tx_ring = wx->tx_ring[r_idx];
+	tx_ring = rcu_dereference(wx->tx_ring[r_idx]);
+
+	if (unlikely(!tx_ring)) {
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
 
 	return wx_xmit_frame_ring(skb, tx_ring);
 }
@@ -2058,26 +2063,26 @@ static bool wx_cache_ring_vmdq(struct wx *wx)
 			/* If we are greater than indices move to next pool */
 			if ((reg_idx & ~vmdq->mask) >= rss->indices)
 				reg_idx = __ALIGN_MASK(reg_idx, ~vmdq->mask);
-			wx->rx_ring[i]->reg_idx = reg_idx;
+			rcu_dereference_protected(wx->rx_ring[i], 1)->reg_idx = reg_idx;
 		}
 		reg_idx = vmdq->offset * __ALIGN_MASK(1, ~vmdq->mask);
 		for (i = 0; i < wx->num_tx_queues; i++, reg_idx++) {
 			/* If we are greater than indices move to next pool */
 			if ((reg_idx & rss->mask) >= rss->indices)
 				reg_idx = __ALIGN_MASK(reg_idx, ~vmdq->mask);
-			wx->tx_ring[i]->reg_idx = reg_idx;
+			rcu_dereference_protected(wx->tx_ring[i], 1)->reg_idx = reg_idx;
 		}
 	} else {
 		/* start at VMDq register offset for SR-IOV enabled setups */
 		reg_idx = vmdq->offset;
 		for (i = 0; i < wx->num_rx_queues; i++)
 			/* If we are greater than indices move to next pool */
-			wx->rx_ring[i]->reg_idx = reg_idx + i;
+			rcu_dereference_protected(wx->rx_ring[i], 1)->reg_idx = reg_idx + i;
 
 		reg_idx = vmdq->offset;
 		for (i = 0; i < wx->num_tx_queues; i++)
 			/* If we are greater than indices move to next pool */
-			wx->tx_ring[i]->reg_idx = reg_idx + i;
+			rcu_dereference_protected(wx->tx_ring[i], 1)->reg_idx = reg_idx + i;
 	}
 
 	return true;
@@ -2098,10 +2103,10 @@ static void wx_cache_ring_rss(struct wx *wx)
 		return;
 
 	for (i = 0; i < wx->num_rx_queues; i++)
-		wx->rx_ring[i]->reg_idx = i;
+		rcu_dereference_protected(wx->rx_ring[i], 1)->reg_idx = i;
 
 	for (i = 0; i < wx->num_tx_queues; i++)
-		wx->tx_ring[i]->reg_idx = i;
+		rcu_dereference_protected(wx->tx_ring[i], 1)->reg_idx = i;
 }
 
 static void wx_add_ring(struct wx_ring *ring, struct wx_ring_container *head)
@@ -2191,7 +2196,7 @@ static int wx_alloc_q_vector(struct wx *wx,
 		ring->queue_index = txr_idx;
 
 		/* assign ring to wx */
-		wx->tx_ring[txr_idx] = ring;
+		rcu_assign_pointer(wx->tx_ring[txr_idx], ring);
 
 		/* update count and index */
 		txr_count--;
@@ -2217,7 +2222,7 @@ static int wx_alloc_q_vector(struct wx *wx,
 		ring->queue_index = rxr_idx;
 
 		/* assign ring to wx */
-		wx->rx_ring[rxr_idx] = ring;
+		rcu_assign_pointer(wx->rx_ring[rxr_idx], ring);
 
 		/* update count and index */
 		rxr_count--;
@@ -2245,10 +2250,10 @@ static void wx_free_q_vector(struct wx *wx, int v_idx)
 	struct wx_ring *ring;
 
 	wx_for_each_ring(ring, q_vector->tx)
-		wx->tx_ring[ring->queue_index] = NULL;
+		rcu_assign_pointer(wx->tx_ring[ring->queue_index], NULL);
 
 	wx_for_each_ring(ring, q_vector->rx)
-		wx->rx_ring[ring->queue_index] = NULL;
+		rcu_assign_pointer(wx->rx_ring[ring->queue_index], NULL);
 
 	wx->q_vector[v_idx] = NULL;
 	netif_napi_del(&q_vector->napi);
@@ -2659,7 +2664,7 @@ void wx_clean_all_rx_rings(struct wx *wx)
 	int i;
 
 	for (i = 0; i < wx->num_rx_queues; i++)
-		wx_clean_rx_ring(wx->rx_ring[i]);
+		wx_clean_rx_ring(rcu_dereference_protected(wx->rx_ring[i], 1));
 }
 EXPORT_SYMBOL(wx_clean_all_rx_rings);
 
@@ -2701,7 +2706,7 @@ static void wx_free_all_rx_resources(struct wx *wx)
 	int i;
 
 	for (i = 0; i < wx->num_rx_queues; i++)
-		wx_free_rx_resources(wx->rx_ring[i]);
+		wx_free_rx_resources(rcu_dereference_protected(wx->rx_ring[i], 1));
 }
 
 /**
@@ -2775,7 +2780,7 @@ void wx_clean_all_tx_rings(struct wx *wx)
 	int i;
 
 	for (i = 0; i < wx->num_tx_queues; i++)
-		wx_clean_tx_ring(wx->tx_ring[i]);
+		wx_clean_tx_ring(rcu_dereference_protected(wx->tx_ring[i], 1));
 }
 EXPORT_SYMBOL(wx_clean_all_tx_rings);
 
@@ -2823,7 +2828,7 @@ static void wx_free_all_tx_resources(struct wx *wx)
 	int i;
 
 	for (i = 0; i < wx->num_tx_queues; i++)
-		wx_free_tx_resources(wx->tx_ring[i]);
+		wx_free_tx_resources(rcu_dereference_protected(wx->tx_ring[i], 1));
 }
 
 void wx_free_resources(struct wx *wx)
@@ -2933,7 +2938,7 @@ static int wx_setup_all_rx_resources(struct wx *wx)
 	int i, err = 0;
 
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		err = wx_setup_rx_resources(wx->rx_ring[i]);
+		err = wx_setup_rx_resources(rcu_dereference_protected(wx->rx_ring[i], 1));
 		if (!err)
 			continue;
 
@@ -2945,7 +2950,7 @@ static int wx_setup_all_rx_resources(struct wx *wx)
 err_setup_rx:
 	/* rewind the index freeing the rings as we go */
 	while (i--)
-		wx_free_rx_resources(wx->rx_ring[i]);
+		wx_free_rx_resources(rcu_dereference_protected(wx->rx_ring[i], 1));
 	return err;
 }
 
@@ -3036,7 +3041,7 @@ static int wx_setup_all_tx_resources(struct wx *wx)
 	int i, err = 0;
 
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		err = wx_setup_tx_resources(wx->tx_ring[i]);
+		err = wx_setup_tx_resources(rcu_dereference_protected(wx->tx_ring[i], 1));
 		if (!err)
 			continue;
 
@@ -3048,7 +3053,7 @@ static int wx_setup_all_tx_resources(struct wx *wx)
 err_setup_tx:
 	/* rewind the index freeing the rings as we go */
 	while (i--)
-		wx_free_tx_resources(wx->tx_ring[i]);
+		wx_free_tx_resources(rcu_dereference_protected(wx->tx_ring[i], 1));
 	return err;
 }
 
@@ -3097,7 +3102,7 @@ void wx_get_stats64(struct net_device *netdev,
 
 	rcu_read_lock();
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		struct wx_ring *ring = READ_ONCE(wx->rx_ring[i]);
+		struct wx_ring *ring = rcu_dereference(wx->rx_ring[i]);
 		u64 bytes, packets;
 		unsigned int start;
 
@@ -3113,7 +3118,7 @@ void wx_get_stats64(struct net_device *netdev,
 	}
 
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		struct wx_ring *ring = READ_ONCE(wx->tx_ring[i]);
+		struct wx_ring *ring = rcu_dereference(wx->tx_ring[i]);
 		u64 bytes, packets;
 		unsigned int start;
 
@@ -3324,7 +3329,7 @@ int wx_set_ring(struct wx *wx, u32 new_tx_count,
 	 */
 	if (new_tx_count != wx->tx_ring_count) {
 		for (i = 0; i < wx->num_tx_queues; i++) {
-			memcpy(&temp_ring[i], wx->tx_ring[i],
+			memcpy(&temp_ring[i], rcu_dereference_protected(wx->tx_ring[i], 1),
 			       sizeof(struct wx_ring));
 
 			temp_ring[i].count = new_tx_count;
@@ -3340,9 +3345,11 @@ int wx_set_ring(struct wx *wx, u32 new_tx_count,
 		}
 
 		for (i = 0; i < wx->num_tx_queues; i++) {
-			wx_free_tx_resources(wx->tx_ring[i]);
+			struct wx_ring *tx_ring = rcu_dereference_protected(wx->tx_ring[i], 1);
+
+			wx_free_tx_resources(tx_ring);
 
-			memcpy(wx->tx_ring[i], &temp_ring[i],
+			memcpy(tx_ring, &temp_ring[i],
 			       sizeof(struct wx_ring));
 		}
 
@@ -3352,7 +3359,7 @@ int wx_set_ring(struct wx *wx, u32 new_tx_count,
 	/* Repeat the process for the Rx rings if needed */
 	if (new_rx_count != wx->rx_ring_count) {
 		for (i = 0; i < wx->num_rx_queues; i++) {
-			memcpy(&temp_ring[i], wx->rx_ring[i],
+			memcpy(&temp_ring[i], rcu_dereference_protected(wx->rx_ring[i], 1),
 			       sizeof(struct wx_ring));
 
 			temp_ring[i].count = new_rx_count;
@@ -3368,8 +3375,10 @@ int wx_set_ring(struct wx *wx, u32 new_tx_count,
 		}
 
 		for (i = 0; i < wx->num_rx_queues; i++) {
-			wx_free_rx_resources(wx->rx_ring[i]);
-			memcpy(wx->rx_ring[i], &temp_ring[i],
+			struct wx_ring *rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
+
+			wx_free_rx_resources(rx_ring);
+			memcpy(rx_ring, &temp_ring[i],
 			       sizeof(struct wx_ring));
 		}
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
index 4708e7f3958f..68dc62977aab 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
@@ -274,11 +274,15 @@ static void wx_ptp_rx_hang(struct wx *wx)
 
 	/* determine the most recent watchdog or rx_timestamp event */
 	rx_event = wx->last_rx_ptp_check;
+	rcu_read_lock();
 	for (n = 0; n < wx->num_rx_queues; n++) {
-		rx_ring = wx->rx_ring[n];
+		rx_ring = rcu_dereference(wx->rx_ring[n]);
+		if (!rx_ring)
+			continue;
 		if (time_after(rx_ring->last_rx_timestamp, rx_event))
 			rx_event = rx_ring->last_rx_timestamp;
 	}
+	rcu_read_unlock();
 
 	/* only need to read the high RXSTMP register to clear the lock */
 	if (time_is_before_jiffies(rx_event + 5 * HZ)) {
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 9454e90258d8..9952d0234240 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1359,8 +1359,8 @@ struct wx {
 	u32 tx_ring_count;
 	u32 rx_ring_count;
 
-	struct wx_ring *tx_ring[64] ____cacheline_aligned_in_smp;
-	struct wx_ring *rx_ring[64];
+	struct wx_ring __rcu *tx_ring[64] ____cacheline_aligned_in_smp;
+	struct wx_ring __rcu *rx_ring[64];
 	struct wx_q_vector *q_vector[64];
 	int num_rx_pools;
 	int num_rx_queues_per_pool;
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 26de78e9a69e..c830824c4869 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -205,7 +205,7 @@ static void wx_configure_rx_vf(struct wx *wx)
 	 * the Base and Length of the Rx Descriptor Ring
 	 */
 	for (i = 0; i < wx->num_rx_queues; i++) {
-		struct wx_ring *rx_ring = wx->rx_ring[i];
+		struct wx_ring *rx_ring = rcu_dereference_protected(wx->rx_ring[i], 1);
 #ifdef HAVE_SWIOTLB_SKIP_CPU_SYNC
 		wx_set_rx_buffer_len_vf(wx, rx_ring);
 #endif
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_lib.c
index 7325b475ee10..0e09e66c6de6 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_lib.c
@@ -175,8 +175,11 @@ void wx_configure_tx_vf(struct wx *wx)
 	u32 i;
 
 	/* Setup the HW Tx Head and Tail descriptor pointers */
-	for (i = 0; i < wx->num_tx_queues; i++)
-		wx_configure_tx_ring_vf(wx, wx->tx_ring[i]);
+	for (i = 0; i < wx->num_tx_queues; i++) {
+		struct wx_ring *tx_ring = rcu_dereference_protected(wx->tx_ring[i], 1);
+
+		wx_configure_tx_ring_vf(wx, tx_ring);
+	}
 }
 
 static void wx_configure_srrctl_vf(struct wx *wx, struct wx_ring *ring,
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
index 1960f7154151..9e90702a5146 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
@@ -37,9 +37,9 @@ static int ngbe_set_ringparam(struct net_device *netdev,
 
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
-			wx->tx_ring[i]->count = new_tx_count;
+			rcu_dereference_protected(wx->tx_ring[i], 1)->count = new_tx_count;
 		for (i = 0; i < wx->num_rx_queues; i++)
-			wx->rx_ring[i]->count = new_rx_count;
+			rcu_dereference_protected(wx->rx_ring[i], 1)->count = new_rx_count;
 		wx->tx_ring_count = new_tx_count;
 		wx->rx_ring_count = new_rx_count;
 
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 855dc963c610..e45d7dfdc9f9 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -406,7 +406,7 @@ static void ngbe_disable_device(struct wx *wx)
 	/* disable all enabled rx queues */
 	for (i = 0; i < wx->num_rx_queues; i++)
 		/* this call also flushes the previous write */
-		wx_disable_rx_queue(wx, wx->rx_ring[i]);
+		wx_disable_rx_queue(wx, rcu_dereference_protected(wx->rx_ring[i], 1));
 	/* disable receives */
 	wx_disable_rx(wx);
 	wx_napi_disable_all(wx);
@@ -422,7 +422,7 @@ static void ngbe_disable_device(struct wx *wx)
 	wx_irq_disable(wx);
 	/* disable transmits in the hardware now that interrupts are off */
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		u8 reg_idx = wx->tx_ring[i]->reg_idx;
+		u8 reg_idx = rcu_dereference_protected(wx->tx_ring[i], 1)->reg_idx;
 
 		wr32(wx, WX_PX_TR_CFG(reg_idx), WX_PX_TR_CFG_SWFLSH);
 	}
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index 3e32aca72806..fcfaf6f2f544 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -61,9 +61,9 @@ static int txgbe_set_ringparam(struct net_device *netdev,
 
 	if (!netif_running(wx->netdev)) {
 		for (i = 0; i < wx->num_tx_queues; i++)
-			wx->tx_ring[i]->count = new_tx_count;
+			rcu_dereference_protected(wx->tx_ring[i], 1)->count = new_tx_count;
 		for (i = 0; i < wx->num_rx_queues; i++)
-			wx->rx_ring[i]->count = new_rx_count;
+			rcu_dereference_protected(wx->rx_ring[i], 1)->count = new_rx_count;
 		wx->tx_ring_count = new_tx_count;
 		wx->rx_ring_count = new_rx_count;
 
@@ -366,7 +366,7 @@ static int txgbe_add_ethtool_fdir_entry(struct txgbe *txgbe,
 
 		/* Map the ring onto the absolute queue index */
 		if (!vf)
-			queue = wx->rx_ring[ring]->reg_idx;
+			queue = rcu_dereference_protected(wx->rx_ring[ring], 1)->reg_idx;
 		else
 			queue = ((vf - 1) * wx->num_rx_queues_per_pool) + ring;
 	}
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_fdir.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_fdir.c
index a84010828551..60c8ea477827 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_fdir.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_fdir.c
@@ -599,7 +599,7 @@ static void txgbe_fdir_filter_restore(struct wx *wx)
 			}
 
 			/* Map the ring onto the absolute queue index */
-			queue = wx->rx_ring[ring]->reg_idx;
+			queue = rcu_dereference_protected(wx->rx_ring[ring], 1)->reg_idx;
 		}
 
 		ret = txgbe_fdir_write_perfect_filter(wx,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index eb91c4f28ecd..5361360f7c6e 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -239,7 +239,7 @@ static void txgbe_disable_device(struct wx *wx)
 	/* disable all enabled rx queues */
 	for (i = 0; i < wx->num_rx_queues; i++)
 		/* this call also flushes the previous write */
-		wx_disable_rx_queue(wx, wx->rx_ring[i]);
+		wx_disable_rx_queue(wx, rcu_dereference_protected(wx->rx_ring[i], 1));
 
 	netif_tx_stop_all_queues(netdev);
 	netif_tx_disable(netdev);
@@ -275,7 +275,7 @@ static void txgbe_disable_device(struct wx *wx)
 
 	/* disable transmits in the hardware now that interrupts are off */
 	for (i = 0; i < wx->num_tx_queues; i++) {
-		u8 reg_idx = wx->tx_ring[i]->reg_idx;
+		u8 reg_idx = rcu_dereference_protected(wx->tx_ring[i], 1)->reg_idx;
 
 		wr32(wx, WX_PX_TR_CFG(reg_idx), WX_PX_TR_CFG_SWFLSH);
 	}
-- 
2.30.1

Re: [PATCH net-next v5] net: Wangxun: protect ring accesses with RCU

[netdev-bot+sashiko]

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

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

- [High] wx_xmit_frame() now guards against wx->tx_ring[r_idx] being NULL
  during teardown, but the line immediately above still does `if (r_idx…
- [Medium] The commit body states that the pre-patch code can produce
  "NULL pointer dereferences or Use-After-Free (UAF) issues" when…
- [Medium] wx_xmit_frame() now reads the newly __rcu-annotated ring
  pointer with the classic rcu_dereference(), but it is the…
- [Medium] Bypassing lockdep validation by passing `1` as the lock
  condition to `rcu_dereference_protected()`.
- [Low] The commit message claims that during "interface stop" ring
  pointers "can be cleared to NULL and freed asynchronously via…

Pre-existing issues:
- [High] wxvf_suspend() (wx_vf_common.c:14-24), also used by
  wxvf_shutdown(), only does netif_device_detach() and then…
- [Medium] ngbe_set_ringparam() (ngbe_ethtool.c) and
  txgbe_set_ringparam() (txgbe_ethtool.c) initialise err to 0, call…
- [Medium] ngbe_setup_tc() (ngbe_main.c:620-645) and txgbe_setup_tc()
  (txgbe_main.c:611-635) close the running device, destroy the interrupt…

--

net: Wangxun: protect ring accesses with RCU

This annotates wx->tx_ring[] and wx->rx_ring[] with __rcu, converts the
lockless readers to rcu_dereference() inside rcu_read_lock() sections with
NULL checks, publishes and clears the slots with rcu_assign_pointer(), and
uses rcu_dereference_protected() in the control paths.

A couple of questions about the changelog and the converted call sites
below.

Two notes on the commit message first.

> Concurrency between interface reconfiguration and background tasks (such
> as service tasks, dev_get_stats, or PTP watchdogs) can result in NULL
> pointer dereferences or Use-After-Free (UAF) issues when accessing
> per-queue structures.

[Severity: Medium]
The v5 changelog says the description was reworded "to suit net-next rather
than a bug fix" and that the Fixes: tag was dropped, but the text above
still describes a real crash in shipped kernels.

Before this patch wx_update_stats() dereferenced wx->rx_ring[i] and
wx->tx_ring[i] unconditionally after only a non-atomic gate:

	if (test_bit(WX_STATE_DOWN, wx->state) ||
	    test_bit(WX_STATE_RESETTING, wx->state))
		return;

and it is reached from wx_get_stats64(), the ndo_get_stats64 callback,
which dev_get_stats() calls without RTNL (for example from /proc/net/dev
readers).

Meanwhile wx_free_q_vector() NULLs the slots and hands the containing
q_vector to kfree_rcu() during ethtool -L / setup_tc, and the pre-existing
READ_ONCE() plus NULL check already in wx_get_stats64() suggests the array
was known to be observable mid-teardown.

Since the state test is a check-then-use rather than mutual exclusion, is
the affected part of this change actually a fix that stable trees need,
and would a Fixes: tag be appropriate so backport tooling can find it?

> During queue teardown, interface stop, or channel reconfiguration (e.g. via
> ethtool -L), ring pointers in wx->rx_ring[] and wx->tx_ring[] can be
> cleared to NULL and freed asynchronously via kfree_rcu().

[Severity: Low]
Is "interface stop" accurate here? The ndo_stop implementations
ngbe_close(), txgbe_close() and wxvf_close() bring the device down and
release descriptor resources through wx_free_resources()/wx_free_irq(),
but none of them calls wx_clear_interrupt_scheme(), so the ring array
stays published and no q_vector is handed to kfree_rcu().

Only interrupt-scheme teardown clears the slots:

wx_clear_interrupt_scheme()
	wx_free_q_vectors()
		wx_free_q_vector()

which is reached from ngbe_setup_tc()/txgbe_setup_tc(), PCI remove,
shutdown, suspend and the probe error paths, rather than from a plain
interface stop.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index ed5aad7857bd..9f90aa92f840 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -1714,7 +1714,12 @@ netdev_tx_t wx_xmit_frame(struct sk_buff *skb,
>  
>  	if (r_idx >= wx->num_tx_queues)
>  		r_idx = r_idx % wx->num_tx_queues;

[Severity: High]
Can this modulo still divide by zero in the same teardown window the new
NULL check is meant to cover?

wx_free_q_vectors() zeroes the counts before the ring slots are cleared:

	wx->num_tx_queues = 0;
	wx->num_rx_queues = 0;
	wx->num_q_vectors = 0;

	while (v_idx--)
		wx_free_q_vector(wx, v_idx);

So a concurrent transmit observes num_tx_queues == 0 strictly before it can
observe a NULL ring. With count 0 the unsigned comparison is always true
and r_idx % 0 raises a divide error before the guard below is reached.

Is the window open on the VF suspend/shutdown path? wxvf_suspend() only
does:

	netif_device_detach(wx->netdev);
	wx_clear_interrupt_scheme(wx);

netif_device_detach() stops the queues without taking the per-txq locks, so
an xmit already inside wx_xmit_frame() on another CPU keeps running. The PF
paths differ because ngbe_close()/txgbe_close() run netif_tx_disable(),
which takes every txq lock and drains in-flight transmits.

Would snapshotting the count once with READ_ONCE() and returning early when
it is 0, or keying the whole decision off the ring pointer, be a better fit
than adding the NULL check after the division?

> -	tx_ring = wx->tx_ring[r_idx];
> +	tx_ring = rcu_dereference(wx->tx_ring[r_idx]);

[Severity: Medium]
Should this be rcu_dereference_bh()? wx_xmit_frame() is the
ndo_start_xmit callback and runs inside rcu_read_lock_bh() taken by
__dev_queue_xmit():

net/core/dev.c:__dev_queue_xmit() {
	/* Disable soft irqs for various locks below. Also
	 * stops preemption for RCU.
	 */
	rcu_read_lock_bh();
	...
}

rcu_dereference(p) expands to rcu_dereference_check(p, 0), whose condition
is rcu_read_lock_held(), and that returns lock_is_held(&rcu_lock_map) while
rcu_read_lock_bh() acquires rcu_bh_lock_map instead.

With CONFIG_PROVE_RCU=y, does the first packet on a wangxun NIC produce a
"suspicious rcu_dereference_check() usage" splat from the transmit path?
This also looks like the only reader converted here that has no
rcu_read_lock() of its own, now that struct wx carries:

	struct wx_ring __rcu *tx_ring[64] ____cacheline_aligned_in_smp;
	struct wx_ring __rcu *rx_ring[64];

> +
> +	if (unlikely(!tx_ring)) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
>  
>  	return wx_xmit_frame_ring(skb, tx_ring);
>  }

[ ... ]

> @@ -2245,10 +2250,10 @@ static void wx_free_q_vector(struct wx *wx, int v_idx)
>  	struct wx_ring *ring;
>  
>  	wx_for_each_ring(ring, q_vector->tx)
> -		wx->tx_ring[ring->queue_index] = NULL;
> +		rcu_assign_pointer(wx->tx_ring[ring->queue_index], NULL);
>  
>  	wx_for_each_ring(ring, q_vector->rx)
> -		wx->rx_ring[ring->queue_index] = NULL;
> +		rcu_assign_pointer(wx->rx_ring[ring->queue_index], NULL);
>  
>  	wx->q_vector[v_idx] = NULL;
>  	netif_napi_del(&q_vector->napi);

[Severity: High]
This isn't a bug introduced by this patch, but the VF suspend path reaches
this function without any prior quiesce, and the RCU conversion makes the
resource side unrecoverable.

wxvf_suspend() in wx_vf_common.c, also used by wxvf_shutdown(), does only:

	netif_device_detach(wx->netdev);
	wx_clear_interrupt_scheme(wx);
	pci_disable_device(pdev);

There is no wxvf_down(), no wx_free_irq(), no timer_delete_sync() /
cancel_work_sync(), and no wx_free_resources().

Does this free the q_vector while its users are still live? The NAPI
instance is still enabled, the DIM work items are only disabled by
wx_napi_disable_all(), the service timer is still armed from
wxvf_up_complete(), and the MSI-X handlers are still requested:

wx_msix_clean_rings() {
	struct wx_q_vector *q_vector = data;

	if (q_vector->rx.ring || q_vector->tx.ring) {
		napi_schedule_irqoff(&q_vector->napi);
	...
}

That runs in hard-IRQ context, which the kfree_rcu() grace period does not
synchronise against, and wx_reset_interrupt_capability() then calls
pci_free_irq_vectors() with the handlers still installed.

Does the same path also leak the ring memory? wx_free_q_vectors() zeroes
num_tx_queues/num_rx_queues/num_q_vectors before this function NULLs the
slots, so wx_free_all_tx_resources()/wx_free_all_rx_resources() would
iterate zero queues afterwards and the dma_alloc_coherent() descriptor
rings, the tx_buffer_info/rx_buffer_info arrays and the page pools become
unreachable, with page_pool_destroy() never called.

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> index 1960f7154151..9e90702a5146 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
> @@ -37,9 +37,9 @@ static int ngbe_set_ringparam(struct net_device *netdev,
>  
>  	if (!netif_running(wx->netdev)) {
>  		for (i = 0; i < wx->num_tx_queues; i++)
> -			wx->tx_ring[i]->count = new_tx_count;
> +			rcu_dereference_protected(wx->tx_ring[i], 1)->count = new_tx_count;
>  		for (i = 0; i < wx->num_rx_queues; i++)
> -			wx->rx_ring[i]->count = new_rx_count;
> +			rcu_dereference_protected(wx->rx_ring[i], 1)->count = new_rx_count;
>  		wx->tx_ring_count = new_tx_count;
>  		wx->rx_ring_count = new_rx_count;

[Severity: Medium]
This is a pre-existing issue, not something this patch introduces, but
while touching these functions: ngbe_set_ringparam() and
txgbe_set_ringparam() initialise err to 0 and then drop the return value
of wx_set_ring():

	ngbe_down(wx);

	wx_set_ring(wx, new_tx_count, new_rx_count, temp_ring);
	kvfree(temp_ring);

	ngbe_up(wx);

wx_set_ring() returns the failures from wx_setup_tx_resources() and
wx_setup_rx_resources(), and an Rx allocation failure can happen after the
Tx rings and wx->tx_ring_count were already committed. Does ethtool -G
then report success for a partially applied configuration?

wx_set_ringparam_vf() captures and returns this value, so should the PF
versions do the same?

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 855dc963c610..e45d7dfdc9f9 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -406,7 +406,7 @@ static void ngbe_disable_device(struct wx *wx)
>  	/* disable all enabled rx queues */
>  	for (i = 0; i < wx->num_rx_queues; i++)
>  		/* this call also flushes the previous write */
> -		wx_disable_rx_queue(wx, wx->rx_ring[i]);
> +		wx_disable_rx_queue(wx, rcu_dereference_protected(wx->rx_ring[i], 1));

[Severity: Medium]
This is a pre-existing issue rather than something added here, but it is in
the reconfiguration path the commit message names. ngbe_setup_tc() in this
same file, and txgbe_setup_tc(), close the running device, destroy the
interrupt scheme, and then ignore both return values:

	wx_init_interrupt_scheme(wx);

	if (netif_running(dev))
		ngbe_open(dev);

	return 0;

If wx_alloc_q_vectors() fails it zeroes the queue counts:

wx_alloc_q_vectors() {
err_out:
	wx->num_tx_queues = 0;
	wx->num_rx_queues = 0;
	wx->num_q_vectors = 0;
	...
}

so setup_tc re-opens a device with zero queues, ngbe_open()'s error is
dropped, and __LINK_STATE_START stays set from the direct close/open calls.
wx_set_channels() propagates only setup_tc's return value, so does
ethtool -L report success while leaving the interface unusable, and with
netif_running() true and num_tx_queues == 0 does it also feed the modulo
in wx_xmit_frame() discussed above?

[ ... ]

> @@ -2775,7 +2780,7 @@ void wx_clean_all_tx_rings(struct wx *wx)
>  	int i;
>  
>  	for (i = 0; i < wx->num_tx_queues; i++)
> -		wx_clean_tx_ring(wx->tx_ring[i]);
> +		wx_clean_tx_ring(rcu_dereference_protected(wx->tx_ring[i], 1));
>  }
>  EXPORT_SYMBOL(wx_clean_all_tx_rings);
>  

[Severity: Medium]
Every control-path conversion in this series passes a literal 1 as
the condition argument, here and in wx_configure_tx(),
wx_configure_rx(), wx_set_rx_buffer_len(), wx_fc_enable(),
wx_cache_ring_rss(), wx_cache_ring_vmdq(), wx_set_ring(),
wx_setup_all_{tx,rx}_resources(), wx_free_all_{tx,rx}_resources(),
wx_configure_tx_vf(), wx_configure_rx_vf(), the three
*_set_ringparam() variants, ngbe_disable_device(),
txgbe_disable_device(), txgbe_add_ethtool_fdir_entry() and
txgbe_fdir_filter_restore().

rcu_dereference_protected(p, 1) makes the lockdep check
unconditionally true, so it only casts away the __rcu annotation for
sparse and asserts nothing at runtime. The commit message says these
sites are "strictly guarded by outer locks (e.g. RTNL or dev state
locks)", which is exactly the claim the condition argument exists to
verify under CONFIG_PROVE_RCU. As written, a future caller that
reaches wx_configure_tx() or wx_clean_all_tx_rings() without the
outer lock held will not be caught by anything.

Which lock is actually being relied on at these sites? The
*_set_ringparam() paths hold wx->reset_lock and come in through
ethtool, while wx_configure_tx()/wx_configure_rx() are also reached
from the reset and open paths, so the answer may differ per call
site. If a single predicate covers all of them, could the condition
spell it out, for example lockdep_is_held(&wx->reset_lock) or a
combination with lockdep_rtnl_is_held()? Where no such predicate
exists, a short comment naming the lock that makes the access safe
would at least tell the next reader why the bare 1 is there.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909090005.79368-1-mengyuanlou%40net-swift.com

Re: [PATCH net-next v5] net: Wangxun: protect ring accesses with RCU

[mengyuanlou]

> 2026年9月10日 18:01，netdev-bot+sashiko@kernel.org 写道：
> 
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 (1) · Medium: 3 (2) · Low: 1
> 
> - [High] wx_xmit_frame() now guards against wx->tx_ring[r_idx] being NULL
>  during teardown, but the line immediately above still does `if (r_idx…
> - [Medium] The commit body states that the pre-patch code can produce
>  "NULL pointer dereferences or Use-After-Free (UAF) issues" when…
> - [Medium] wx_xmit_frame() now reads the newly __rcu-annotated ring
>  pointer with the classic rcu_dereference(), but it is the…
> - [Medium] Bypassing lockdep validation by passing `1` as the lock
>  condition to `rcu_dereference_protected()`.
> - [Low] The commit message claims that during "interface stop" ring
>  pointers "can be cleared to NULL and freed asynchronously via…
> 
> Pre-existing issues:
> - [High] wxvf_suspend() (wx_vf_common.c:14-24), also used by
>  wxvf_shutdown(), only does netif_device_detach() and then…
> - [Medium] ngbe_set_ringparam() (ngbe_ethtool.c) and
>  txgbe_set_ringparam() (txgbe_ethtool.c) initialise err to 0, call…
> - [Medium] ngbe_setup_tc() (ngbe_main.c:620-645) and txgbe_setup_tc()
>  (txgbe_main.c:611-635) close the running device, destroy the interrupt…
> 
> --
> 
> net: Wangxun: protect ring accesses with RCU
> 
> This annotates wx->tx_ring[] and wx->rx_ring[] with __rcu, converts the
> lockless readers to rcu_dereference() inside rcu_read_lock() sections with
> NULL checks, publishes and clears the slots with rcu_assign_pointer(), and
> uses rcu_dereference_protected() in the control paths.
> 
> A couple of questions about the changelog and the converted call sites
> below.
> 
> Two notes on the commit message first.
> 
>> Concurrency between interface reconfiguration and background tasks (such
>> as service tasks, dev_get_stats, or PTP watchdogs) can result in NULL
>> pointer dereferences or Use-After-Free (UAF) issues when accessing
>> per-queue structures.
> 
> [Severity: Medium]
> The v5 changelog says the description was reworded "to suit net-next rather
> than a bug fix" and that the Fixes: tag was dropped, but the text above
> still describes a real crash in shipped kernels.
> 
> Before this patch wx_update_stats() dereferenced wx->rx_ring[i] and
> wx->tx_ring[i] unconditionally after only a non-atomic gate:
> 
> if (test_bit(WX_STATE_DOWN, wx->state) ||
>    test_bit(WX_STATE_RESETTING, wx->state))
> return;
> 
> and it is reached from wx_get_stats64(), the ndo_get_stats64 callback,
> which dev_get_stats() calls without RTNL (for example from /proc/net/dev
> readers).
> 


> Meanwhile wx_free_q_vector() NULLs the slots and hands the containing
> q_vector to kfree_rcu() during ethtool -L / setup_tc, and the pre-existing
> READ_ONCE() plus NULL check already in wx_get_stats64() suggests the array
> was known to be observable mid-teardown.
> 
> Since the state test is a check-then-use rather than mutual exclusion, is
> the affected part of this change actually a fix that stable trees need,
> and would a Fixes: tag be appropriate so backport tooling can find it?

This patch involves a large amount of code and commits, and it is not necessarily
an issue that will definitely be triggered.
I prefer to rework it for net-next instead.

> 
>> During queue teardown, interface stop, or channel reconfiguration (e.g. via
>> ethtool -L), ring pointers in wx->rx_ring[] and wx->tx_ring[] can be
>> cleared to NULL and freed asynchronously via kfree_rcu().
> 
> [Severity: Low]
> Is "interface stop" accurate here? The ndo_stop implementations
> ngbe_close(), txgbe_close() and wxvf_close() bring the device down and
> release descriptor resources through wx_free_resources()/wx_free_irq(),
> but none of them calls wx_clear_interrupt_scheme(), so the ring array
> stays published and no q_vector is handed to kfree_rcu().
> 
> Only interrupt-scheme teardown clears the slots:
> 
> wx_clear_interrupt_scheme()
> wx_free_q_vectors()
> wx_free_q_vector()
> 
> which is reached from ngbe_setup_tc()/txgbe_setup_tc(), PCI remove,
> shutdown, suspend and the probe error paths, rather than from a plain
> interface stop.


> 
>> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
>> index ed5aad7857bd..9f90aa92f840 100644
>> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
>> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
>> @@ -1714,7 +1714,12 @@ netdev_tx_t wx_xmit_frame(struct sk_buff *skb,
>> 
>> if (r_idx >= wx->num_tx_queues)
>> r_idx = r_idx % wx->num_tx_queues;
> 
> [Severity: High]
> Can this modulo still divide by zero in the same teardown window the new
> NULL check is meant to cover?
> 
> wx_free_q_vectors() zeroes the counts before the ring slots are cleared:
> 
> wx->num_tx_queues = 0;
> wx->num_rx_queues = 0;
> wx->num_q_vectors = 0;
> 
> while (v_idx--)
> wx_free_q_vector(wx, v_idx);
> 
> So a concurrent transmit observes num_tx_queues == 0 strictly before it can
> observe a NULL ring. With count 0 the unsigned comparison is always true
> and r_idx % 0 raises a divide error before the guard below is reached.
> 
> Is the window open on the VF suspend/shutdown path? wxvf_suspend() only
> does:
> 
> netif_device_detach(wx->netdev);
> wx_clear_interrupt_scheme(wx);
> 
> netif_device_detach() stops the queues without taking the per-txq locks, so
> an xmit already inside wx_xmit_frame() on another CPU keeps running. The PF
> paths differ because ngbe_close()/txgbe_close() run netif_tx_disable(),
> which takes every txq lock and drains in-flight transmits.
> 
> Would snapshotting the count once with READ_ONCE() and returning early when
> it is 0, or keying the whole decision off the ring pointer, be a better fit
> than adding the NULL check after the division?

I can check the release order in the "we_free_q_vectors".
However, this is not the issue I need to address in this patch for wx_ring.


>> - tx_ring = wx->tx_ring[r_idx];
>> + tx_ring = rcu_dereference(wx->tx_ring[r_idx]);
> 
> [Severity: Medium]
> Should this be rcu_dereference_bh()? wx_xmit_frame() is the
> ndo_start_xmit callback and runs inside rcu_read_lock_bh() taken by
> __dev_queue_xmit():
> 
> net/core/dev.c:__dev_queue_xmit() {
> /* Disable soft irqs for various locks below. Also
> * stops preemption for RCU.
> */
> rcu_read_lock_bh();
> ...
> }
> 
> rcu_dereference(p) expands to rcu_dereference_check(p, 0), whose condition
> is rcu_read_lock_held(), and that returns lock_is_held(&rcu_lock_map) while
> rcu_read_lock_bh() acquires rcu_bh_lock_map instead.
> 
> With CONFIG_PROVE_RCU=y, does the first packet on a wangxun NIC produce a
> "suspicious rcu_dereference_check() usage" splat from the transmit path?
> This also looks like the only reader converted here that has no
> rcu_read_lock() of its own, now that struct wx carries:
> 
> struct wx_ring __rcu *tx_ring[64] ____cacheline_aligned_in_smp;
> struct wx_ring __rcu *rx_ring[64];
> 

> wx_xmit_frame() is the
> ndo_start_xmit callback and runs inside rcu_read_lock_bh() taken by
> __dev_queue_xmit():

Should be rcu_dereference_bh().


>> +
>> + if (unlikely(!tx_ring)) {
>> + dev_kfree_skb_any(skb);
>> + return NETDEV_TX_OK;
>> + }
>> 
>> return wx_xmit_frame_ring(skb, tx_ring);
>> }
> 
> [ ... ]
> 
>> @@ -2245,10 +2250,10 @@ static void wx_free_q_vector(struct wx *wx, int v_idx)
>> struct wx_ring *ring;
>> 
>> wx_for_each_ring(ring, q_vector->tx)
>> - wx->tx_ring[ring->queue_index] = NULL;
>> + rcu_assign_pointer(wx->tx_ring[ring->queue_index], NULL);
>> 
>> wx_for_each_ring(ring, q_vector->rx)
>> - wx->rx_ring[ring->queue_index] = NULL;
>> + rcu_assign_pointer(wx->rx_ring[ring->queue_index], NULL);
>> 
>> wx->q_vector[v_idx] = NULL;
>> netif_napi_del(&q_vector->napi);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the VF suspend path reaches
> this function without any prior quiesce, and the RCU conversion makes the
> resource side unrecoverable.
> 
> wxvf_suspend() in wx_vf_common.c, also used by wxvf_shutdown(), does only:
> 
> netif_device_detach(wx->netdev);
> wx_clear_interrupt_scheme(wx);
> pci_disable_device(pdev);
> 
> There is no wxvf_down(), no wx_free_irq(), no timer_delete_sync() /
> cancel_work_sync(), and no wx_free_resources().
> 
> Does this free the q_vector while its users are still live? The NAPI
> instance is still enabled, the DIM work items are only disabled by
> wx_napi_disable_all(), the service timer is still armed from
> wxvf_up_complete(), and the MSI-X handlers are still requested:
> 
> wx_msix_clean_rings() {
> struct wx_q_vector *q_vector = data;
> 
> if (q_vector->rx.ring || q_vector->tx.ring) {
> napi_schedule_irqoff(&q_vector->napi);
> ...
> }
> 
> That runs in hard-IRQ context, which the kfree_rcu() grace period does not
> synchronise against, and wx_reset_interrupt_capability() then calls
> pci_free_irq_vectors() with the handlers still installed.
> 
> Does the same path also leak the ring memory? wx_free_q_vectors() zeroes
> num_tx_queues/num_rx_queues/num_q_vectors before this function NULLs the
> slots, so wx_free_all_tx_resources()/wx_free_all_rx_resources() would
> iterate zero queues afterwards and the dma_alloc_coherent() descriptor
> rings, the tx_buffer_info/rx_buffer_info arrays and the page pools become
> unreachable, with page_pool_destroy() never called.
> 

Not for this patch.

> [ ... ]
> 
>> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
>> index 1960f7154151..9e90702a5146 100644
>> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
>> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c
>> @@ -37,9 +37,9 @@ static int ngbe_set_ringparam(struct net_device *netdev,
>> 
>> if (!netif_running(wx->netdev)) {
>> for (i = 0; i < wx->num_tx_queues; i++)
>> - wx->tx_ring[i]->count = new_tx_count;
>> + rcu_dereference_protected(wx->tx_ring[i], 1)->count = new_tx_count;
>> for (i = 0; i < wx->num_rx_queues; i++)
>> - wx->rx_ring[i]->count = new_rx_count;
>> + rcu_dereference_protected(wx->rx_ring[i], 1)->count = new_rx_count;
>> wx->tx_ring_count = new_tx_count;
>> wx->rx_ring_count = new_rx_count;
> 
> [Severity: Medium]
> This is a pre-existing issue, not something this patch introduces, but
> while touching these functions: ngbe_set_ringparam() and
> txgbe_set_ringparam() initialise err to 0 and then drop the return value
> of wx_set_ring():
> 
> ngbe_down(wx);
> 
> wx_set_ring(wx, new_tx_count, new_rx_count, temp_ring);
> kvfree(temp_ring);
> 
> ngbe_up(wx);
> 
> wx_set_ring() returns the failures from wx_setup_tx_resources() and
> wx_setup_rx_resources(), and an Rx allocation failure can happen after the
> Tx rings and wx->tx_ring_count were already committed. Does ethtool -G
> then report success for a partially applied configuration?
> 
> wx_set_ringparam_vf() captures and returns this value, so should the PF
> versions do the same?

Not for this patch.

> 
>> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
>> index 855dc963c610..e45d7dfdc9f9 100644
>> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
>> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
>> @@ -406,7 +406,7 @@ static void ngbe_disable_device(struct wx *wx)
>> /* disable all enabled rx queues */
>> for (i = 0; i < wx->num_rx_queues; i++)
>> /* this call also flushes the previous write */
>> - wx_disable_rx_queue(wx, wx->rx_ring[i]);
>> + wx_disable_rx_queue(wx, rcu_dereference_protected(wx->rx_ring[i], 1));
> 
> [Severity: Medium]
> This is a pre-existing issue rather than something added here, but it is in
> the reconfiguration path the commit message names. ngbe_setup_tc() in this
> same file, and txgbe_setup_tc(), close the running device, destroy the
> interrupt scheme, and then ignore both return values:
> 
> wx_init_interrupt_scheme(wx);
> 
> if (netif_running(dev))
> ngbe_open(dev);
> 
> return 0;
> 
> If wx_alloc_q_vectors() fails it zeroes the queue counts:
> 
> wx_alloc_q_vectors() {
> err_out:
> wx->num_tx_queues = 0;
> wx->num_rx_queues = 0;
> wx->num_q_vectors = 0;
> ...
> }
> 
> so setup_tc re-opens a device with zero queues, ngbe_open()'s error is
> dropped, and __LINK_STATE_START stays set from the direct close/open calls.
> wx_set_channels() propagates only setup_tc's return value, so does
> ethtool -L report success while leaving the interface unusable, and with
> netif_running() true and num_tx_queues == 0 does it also feed the modulo
> in wx_xmit_frame() discussed above?
> 
> [ ... ]

Not for this patch.
> 
>> @@ -2775,7 +2780,7 @@ void wx_clean_all_tx_rings(struct wx *wx)
>> int i;
>> 
>> for (i = 0; i < wx->num_tx_queues; i++)
>> - wx_clean_tx_ring(wx->tx_ring[i]);
>> + wx_clean_tx_ring(rcu_dereference_protected(wx->tx_ring[i], 1));
>> }
>> EXPORT_SYMBOL(wx_clean_all_tx_rings);
>> 
> 
> [Severity: Medium]
> Every control-path conversion in this series passes a literal 1 as
> the condition argument, here and in wx_configure_tx(),
> wx_configure_rx(), wx_set_rx_buffer_len(), wx_fc_enable(),
> wx_cache_ring_rss(), wx_cache_ring_vmdq(), wx_set_ring(),
> wx_setup_all_{tx,rx}_resources(), wx_free_all_{tx,rx}_resources(),
> wx_configure_tx_vf(), wx_configure_rx_vf(), the three
> *_set_ringparam() variants, ngbe_disable_device(),
> txgbe_disable_device(), txgbe_add_ethtool_fdir_entry() and
> txgbe_fdir_filter_restore().
> 
> rcu_dereference_protected(p, 1) makes the lockdep check
> unconditionally true, so it only casts away the __rcu annotation for
> sparse and asserts nothing at runtime. The commit message says these
> sites are "strictly guarded by outer locks (e.g. RTNL or dev state
> locks)", which is exactly the claim the condition argument exists to
> verify under CONFIG_PROVE_RCU. As written, a future caller that
> reaches wx_configure_tx() or wx_clean_all_tx_rings() without the
> outer lock held will not be caught by anything.
> 
> Which lock is actually being relied on at these sites? The
> *_set_ringparam() paths hold wx->reset_lock and come in through
> ethtool, while wx_configure_tx()/wx_configure_rx() are also reached
> from the reset and open paths, so the answer may differ per call
> site. If a single predicate covers all of them, could the condition
> spell it out, for example lockdep_is_held(&wx->reset_lock) or a
> combination with lockdep_rtnl_is_held()? Where no such predicate
> exists, a short comment naming the lock that makes the access safe
> would at least tell the next reader why the bare 1 is there.
> 
The possibility of ring access problems occurring at these locations is very low.
Does it make sense to add lockdep_is_held at every call site?


> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909090005.79368-1-mengyuanlou%40net-swift.com
>