Re: [PATCH net-next v6 04/13] net: mana: swap queue sets in mana_set_channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] networking: u64_stats_sync writer in mana_fold_rxq_stats does not disable preemption/BH
--

commit 93d72a1c224b35993067fa6f5051dfe42f90cf43
Author: Long Li <longli@microsoft.com>

net: mana: swap queue sets in mana_set_channels

Build a replacement queue set before quiescing TX. After closing the
TX/XDP gate and draining its readers, wait up to 120 seconds for ALL
old TX queues before changing pointers, queue counts or RSS.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 02cd5f7656ed8..10225c09f5b7f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -1094,6 +1132,58 @@ static void mana_free_queue_stats(struct mana_port_context *apc)
>  	apc->txq_stats = NULL;
>  }
>  
> +/* Fold under RTNL after drain_stats writers quiesce. Clear drain_stats to
> + * prevent double counting on rollback.
> + */
> +static void mana_fold_rxq_stats(struct mana_port_context *apc,
> +				struct mana_rxq *rxq)
> +{
> +	struct mana_stats_rx *src = &rxq->drain_stats;
> +	struct mana_stats_rx *dst;
> +	unsigned int i;
> +
> +	ASSERT_RTNL();
> +
> +	if (!apc->rxq_stats_ret || rxq->rxq_idx >= apc->max_queues)
> +		return;
> +
> +	dst = &apc->rxq_stats_ret[rxq->rxq_idx];
> +
> +	u64_stats_update_begin(&dst->syncp);

[Severity: High]
Does this code expose a sequence counter race window to lockless readers?

The mana_fold_rxq_stats() function runs in process context under RTNL, but
it calls u64_stats_update_begin() without first disabling preemption or
bottom-halves (softirqs).

The u64_stats_sync API strictly requires writers to run with preemption
disabled, and BH disabled if readers can be in softirq context. On 32-bit
architectures, this macro increments a sequence counter.

If a reader (like ndo_get_stats64 invoked via dev_get_stats() from a
softirq or another thread) preempts the writer on the same CPU while the
sequence counter is odd, the reader could spin infinitely in
u64_stats_fetch_retry() waiting for it to become even, causing a hard
deadlock.

Should the update be wrapped in local_bh_disable() and local_bh_enable(),
or use the _irqsave variant?

> +	dst->packets		+= src->packets;
> +	dst->bytes		+= src->bytes;
> +	dst->xdp_drop		+= src->xdp_drop;
> +	dst->xdp_tx		+= src->xdp_tx;
> +	dst->xdp_redirect	+= src->xdp_redirect;
> +	dst->pkt_len0_err	+= src->pkt_len0_err;
> +	for (i = 0; i < ARRAY_SIZE(dst->coalesced_cqe); i++)
> +		dst->coalesced_cqe[i] += src->coalesced_cqe[i];
> +	u64_stats_update_end(&dst->syncp);
> +
> +	src->packets		= 0;
> +	src->bytes		= 0;
> +	src->xdp_drop		= 0;
> +	src->xdp_tx		= 0;
> +	src->xdp_redirect	= 0;
> +	src->pkt_len0_err	= 0;
> +	for (i = 0; i < ARRAY_SIZE(src->coalesced_cqe); i++)
> +		src->coalesced_cqe[i] = 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790795005.git.weh@linux.microsoft.com?part=4