Re: [PATCH net-next v7 2/3] net: airoha: fix ETS QoS stats counter underflow and cross-channel corruption

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

> airoha_qdma_get_tx_ets_stats() has two bugs:
> - The hardware counters read via airoha_qdma_rr() are 32-bit values
>   but are stored in u64 locals and subtracted from u64 baselines. When
>   a 32-bit hardware counter wraps around, the subtraction produces a
>   large underflow value passed to _bstats_update().
> - The baseline counters (cpu_tx_packets, fwd_tx_packets) are stored as
>   single per-device fields, but airoha_qdma_get_tx_ets_stats() is
>   called with different channel values (0-3). Each call reads a
>   different channel's hardware counter but overwrites the same
>   baseline, corrupting the delta computation for other channels.
> 
> Fix both by:
> - Narrowing the counter locals and baselines to u32 so that 32-bit
>   unsigned subtraction handles wrap-around naturally.
> - Grouping the baselines into a per-channel qos_stats array so each
>   channel tracks its own previous counter value independently.
> 
> Fixes: 20bf7d07c956 ("net: airoha: Add sched ETS offload support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260701-airoha-ethtool-priv_flags-v7-0-b4153bd44428%40kernel.org

> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 18 +++++++++++-------
>  drivers/net/ethernet/airoha/airoha_eth.h |  7 ++++---
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 8bba54ebcf07..2c9ceb9f16f8 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2491,16 +2491,20 @@ static int airoha_qdma_get_tx_ets_stats(struct net_device *netdev, int channel,
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
>  	struct airoha_qdma *qdma = dev->qdma;
> +	u32 cpu_tx_packets, fwd_tx_packets;
> +	u64 tx_packets;
>  
> -	u64 cpu_tx_packets = airoha_qdma_rr(qdma, REG_CNTR_VAL(channel << 1));
> -	u64 fwd_tx_packets = airoha_qdma_rr(qdma,
> -					    REG_CNTR_VAL((channel << 1) + 1));
> -	u64 tx_packets = (cpu_tx_packets - dev->cpu_tx_packets) +
> -			 (fwd_tx_packets - dev->fwd_tx_packets);
> +	cpu_tx_packets = airoha_qdma_rr(qdma, REG_CNTR_VAL(channel << 1));
> +	fwd_tx_packets = airoha_qdma_rr(qdma,
> +					REG_CNTR_VAL((channel << 1) + 1));
> +	tx_packets = (u32)(cpu_tx_packets -
> +			   dev->qos_stats[channel].cpu_tx_packets) +
> +		     (u32)(fwd_tx_packets -
> +			   dev->qos_stats[channel].fwd_tx_packets);

- Will this addition overflow in 32-bit space before the result is assigned to
  the 64-bit tx_packets?
  - I do not think this is a problem since we are just considering the delta
    betwen cpu_tx_packets/fwd_tx_packets and the previous value. Moreover, the
    u32 cast will take care of possible wrap-around.

>  
>  	_bstats_update(opt->stats.bstats, 0, tx_packets);

- This isn't a bug introduced by this patch, but does calling _bstats_update()
  here directly from process context race with the software datapath?
  - Sashiko is right here. This is a pre-existing (theoretical) issue not
    introduced by this patch. However, since the Airoha EN7581/EN7583 is
    ARM64-only, u64_stats_update_begin/end are NOPs on this platform and
    there is no actual race. IIUC the seqcount corruption scenario only
    applies to 32-bit architectures. I guess we can 

Regards,
Lorenzo

> -	dev->cpu_tx_packets = cpu_tx_packets;
> -	dev->fwd_tx_packets = fwd_tx_packets;
> +	dev->qos_stats[channel].cpu_tx_packets = cpu_tx_packets;
> +	dev->qos_stats[channel].fwd_tx_packets = fwd_tx_packets;
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 87ab3ea10664..ac5f571f3e53 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -545,9 +545,10 @@ struct airoha_gdm_dev {
>  	struct airoha_eth *eth;
>  
>  	DECLARE_BITMAP(qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS);
> -	/* qos stats counters */
> -	u64 cpu_tx_packets;
> -	u64 fwd_tx_packets;
> +	struct {
> +		u32 cpu_tx_packets;
> +		u32 fwd_tx_packets;
> +	} qos_stats[AIROHA_NUM_QOS_CHANNELS];
>  
>  	u32 flags;
>  	int nbq;
> 
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]