Re: [PATCH net-next v5 09/13] net: mana: keep per-queue statistics in the port context

[sashiko-bot@kernel.org]

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

New issues:
- [High] u64_stats_update_begin called with preemption enabled causes potential deadlock.

Pre-existing issues:
- [High] Missing Mutual Exclusion on tx_stats updates between mana_xdp_xmit and mana_start_xmit.
--

commit 45f5d2a315921074d282869dd20e8233e5bb463b
Author: Long Li <longli@microsoft.com>
Subject: net: mana: keep per-queue statistics in the port context

Move RX/TX counters into port-lifetime arrays so queue rebuilds do not
reset them. Sum all max_queues slots for ndo_get_stats64(), keep the
current queue count for ethtool, and fold retiring RX queues count into
private drain_stats upon destruction.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> index debf0e7765862..2060e08d82277 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c
[ ... ]
> @@ -73,7 +73,7 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct xdp_frame **frames,
>  		count++;
>  	}
>  
> -	tx_stats = &apc->tx_qp[q_idx]->txq.stats;
> +	tx_stats = apc->tx_qp[q_idx]->txq.stats;
>  
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->xdp_xmit += count;
>  	u64_stats_update_end(&tx_stats->syncp);

[Severity: High]
This is a pre-existing issue, but is there a missing mutual exclusion
between mana_xdp_xmit() and mana_start_xmit() when updating tx_stats->syncp?

mana_xdp_xmit() bypasses the qdisc HARD_TX_LOCK and maps q_idx using
smp_processor_id(). As a result, it can run concurrently with
mana_start_xmit() which uses the skb's queue mapping. Because the skb
queue mapping could map to the same queue from another CPU, both
functions might run simultaneously on different CPUs and call
u64_stats_update_begin() on the same syncp. This would corrupt the
sequence counter.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 5752e3745ccc6..98007bb3ef7ca 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -575,7 +575,7 @@ netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	/* Populated the packet and bytes counters based on post GSO packet
>  	 * calculations
>  	 */
> -	tx_stats = &txq->stats;
> +	tx_stats = txq->stats;
>  	u64_stats_update_begin(&tx_stats->syncp);
>  	tx_stats->packets += num_gso_seg;
>  	tx_stats->bytes += len + ((num_gso_seg - 1) * gso_hs);

[Severity: High]
This is a pre-existing issue, but this is the primary packet transmission
path that can run concurrently with the XDP transmission path in
mana_xdp_xmit() mentioned above, contributing to the lost updates on the
shared tx_stats struct.

[ ... ]
> +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]
Does mana_fold_rxq_stats() need to disable preemption or bottom halves
before acquiring the sequence lock here?

This function executes in process context holding the RTNL lock, but it
calls u64_stats_update_begin() with preemption enabled. On 32-bit systems,
if the thread is preempted or interrupted by a softirq on the same CPU that
subsequently reads the stats via mana_get_stats64(), the reader could spin
infinitely waiting for the sequence lock to become even.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909222416.884246-1-longli@microsoft.com?part=9