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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: concurrent writers to tx_stats->syncp without mutual exclusion
- [High] net: missing BH disable around u64_stats_update_begin() in mana_fold_rxq_stats()
--

commit 347c8bd66ee5005e7aea875ed691f7a765b23452
Author: Long Li <longli@microsoft.com>

net: mana: swap queue sets in mana_set_channels

This patch introduces a replacement queue set building before quiescing TX.
It drains the old TX queues and swaps pointers, handling statistics folding
upon successful or failed rollback.

> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> @@ -59,6 +59,11 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames,
>  	if (unlikely(!apc->port_is_up))
>  		return 0;
>  
> +	/* Pair with the smp_wmb() in mana_publish_qset() before reading queue
> +	 * state.
> +	 */
> +	smp_rmb();
> +
>  	q_idx = smp_processor_id() % ndev->real_num_tx_queues;
>  
>  	for (i = 0; i < n; i++) {

[Severity: High]
Does mana_xdp_xmit() require a lock when updating tx_stats->syncp further
down in this function?

This function acts as the ndo_xdp_xmit callback and maps CPUs to queues
using smp_processor_id() % ndev->real_num_tx_queues. If multiple CPUs map
to the same queue index, or if mana_start_xmit() runs concurrently on the
same queue, multiple threads could concurrently invoke
u64_stats_update_begin(&tx_stats->syncp).

Since there isn't a per-queue mutual exclusion lock acquired here (unlike
the core stack serialization in mana_start_xmit()), could this concurrency
corrupt the sequence counter and cause readers to spin infinitely on 32-bit
systems?

[ ... ]

> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -1123,6 +1161,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);
> +	dst->packets		+= src->packets;
> +	dst->bytes		+= src->bytes;

[Severity: High]
Could this u64_stats_update_begin() call deadlock a preempting softirq
reader?

This function runs in process context under rtnl_lock(), which is a mutex
and does not disable bottom halves. Readers of network statistics (like
ndo_get_stats64) can be executed from softirq context.

If a softirq interrupts mana_fold_rxq_stats() on the same CPU while the
writer holds an odd sequence counter, the reader will loop forever in
u64_stats_fetch_retry().

Should this use local_bh_disable() or the _bh wrapper for the stats update
to prevent this scenario?

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