Re: [net,v4] net: libwx: protect ring accesses with RCU and NULL check

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Commit message premise is unsubstantiated and the single
  `Fixes: 46b92e10d631 ("net: libwx: support hardware statistics")` tag…
- [Medium] Ring pointers are published to lockless readers without
  release ordering.

Pre-existing issues:
- [Low] The loop bounds of every newly RCU-protected traversal
  (wx->num_tx_queues / wx->num_rx_queues) remain plain, unannotated…

--

net: libwx: protect ring accesses with RCU and NULL check

This converts the wx->tx_ring[]/wx->rx_ring[] traversals in the libwx
statistics, error, VLAN and PTP paths to READ_ONCE() plus a NULL check
inside rcu_read_lock()/rcu_read_unlock(), and switches the ring slot
assignment and clearing in wx_alloc_q_vector()/wx_free_q_vector() to
WRITE_ONCE().

A few questions below, first about the changelog and the Fixes tag, then
about the publish side in wx_lib.c.

> In wx_update_stats(), wx_get_ethtool_stats(), and other service tasks,
> the ring pointers wx->rx_ring[i] and wx->tx_ring[i] can become NULL or be
> freed asynchronously during queue teardown or channel reconfiguration.

[Severity: Medium]
Does this premise hold for the service task paths?

txgbe_disable_device() and ngbe_disable_device() stop the service task
synchronously:

	timer_delete_sync(&wx->service_timer);
	cancel_work_sync(&wx->service_task);

and the ring slots are only cleared afterwards, in
wx_clear_interrupt_scheme() -> wx_free_q_vectors() -> wx_free_q_vector().
So can wx_ring_tx_pending() or wx_detect_tx_hang() actually observe a slot
being cleared or a ring being freed underneath them?

The PTP aux work is similarly quiesced by wx_ptp_suspend() ->
ptp_cancel_worker_sync() before the rings go away, and
wx_get_ethtool_stats() and wx_vlan_strip_control() are reached from
RTNL-serialized callbacks (the latter only from wx_set_rx_mode()).

The one reader that does look unserialized against
wx_clear_interrupt_scheme() is wx_update_stats() reached from
wx_get_stats64() (ndo_get_stats64, e.g. a read of /proc/net/dev), which
before this patch dereferenced wx->rx_ring[i] with no NULL check. That is
the hunk the changelog does not call out. Could the message describe that
path specifically and mark the rest as hardening?

> Fixes: 46b92e10d631 ("net: libwx: support hardware statistics")

Is this tag right for most of the hunks? git blame attributes
wx_ring_tx_pending()/wx_detect_tx_hang() in wx_err.c and the tx_ring loop
in wx_update_xoff_rx_lfc() to 22d95e93c05b0, and wx_vlan_strip_control()
to f3b03c655f678. A stable backport selected on this single tag lands in
trees that have neither wx_err.c nor wx_ptp.c, so the patch cannot apply
there. Should the tag be narrowed to the wx_update_stats()/
wx_get_ethtool_stats() change that fixes the user-visible dereference?

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 122c4952d203..9e2cf794f81c 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c

[ ... ]

> @@ -2893,10 +2906,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 = READ_ONCE(wx->rx_ring[i]);
>  
> +		if (!rx_ring)
> +			continue;
>  		non_eop_descs += rx_ring->rx_stats.non_eop_descs;

[Severity: Low]
This is a pre-existing issue and not introduced by the patch, but the loop
bound itself stays an unannotated plain load while the reconfiguration path
writes it concurrently:

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

The count and the ring array are two independently updated variables, and
the same pattern is in wx_ring_tx_pending(), wx_detect_tx_hang() and
wx_vlan_strip_control(). Since the counts are zeroed before the slots are
cleared and the rings live in a q_vector freed with kfree_rcu(), a stale
count inside the new RCU section resolves to either a still-valid ring or a
NULL slot, so the visible effect looks limited to a partial statistics
sample or a skipped register write, but it remains a KCSAN-reportable race.
Would annotating the count loads be worth doing while the traversals are
being touched?

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index ed5aad7857bd..c227e9ab8a4a 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -2191,7 +2191,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;
> +		WRITE_ONCE(wx->tx_ring[txr_idx], ring);
>  
>  		/* update count and index */
>  		txr_count--;
> @@ -2217,7 +2217,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;
> +		WRITE_ONCE(wx->rx_ring[rxr_idx], ring);

[Severity: Medium]
The v3 posting used rcu_assign_pointer() here and v4 changed it to
WRITE_ONCE(). Does that leave the newly added read side without ordering?

WRITE_ONCE() only stops the compiler from tearing or reordering this single
store, it does not emit smp_store_release(). The ring is initialized with
plain stores immediately before the publish:

		ring->dev = &wx->pdev->dev;
		ring->netdev = wx->netdev;
		ring->q_vector = q_vector;
		...
		ring->queue_index = rxr_idx;

		/* assign ring to wx */
		WRITE_ONCE(wx->rx_ring[rxr_idx], ring);

On a weakly ordered CPU, can a reader doing READ_ONCE() under
rcu_read_lock() see the non-NULL pointer before those field stores become
visible? The NULL check added by this patch does not cover that case, since
the pointer is non-NULL and only the contents are stale.

reg_idx looks like a separate hole, because wx_alloc_q_vector() never
writes it. wx_init_interrupt_scheme() sets it after the pointers are
already published:

wx_cache_ring_rss()
	for (i = 0; i < wx->num_rx_queues; i++)
		wx->rx_ring[i]->reg_idx = i;

and wx_vlan_strip_control() feeds that value straight into a register
offset:

		j = ring->reg_idx;
		wr32m(wx, WX_PX_RR_CFG(j), WX_PX_RR_CFG_VLAN,
		      enable ? WX_PX_RR_CFG_VLAN : 0);

If wx_vlan_strip_control() can run against a freshly published ring set,
can this reprogram the VLAN strip bit of a different receive queue? Note
that switching the publish back to rcu_assign_pointer() would not close
this one, since reg_idx is written after publication.

Publication is reachable on a registered netdev through
txgbe_setup_tc()/ngbe_setup_tc() and the resume paths, concurrently with
wx_get_stats64() -> wx_update_stats(), so the statistics readers can pick
up uninitialized per-queue counters and export them to userspace.

> @@ -2245,10 +2245,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;
> +		WRITE_ONCE(wx->tx_ring[ring->queue_index], NULL);
>  
>  	wx_for_each_ring(ring, q_vector->rx)
> -		wx->rx_ring[ring->queue_index] = NULL;
> +		WRITE_ONCE(wx->rx_ring[ring->queue_index], NULL);

[ ... ]

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