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

> 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.
> - Splitting the delta addition into two statements so the first u32
>   delta is widened to u64 on assignment and the second is added in
>   u64 arithmetic, preventing overflow when both deltas are large.
> 
> Fixes: 20bf7d07c956 ("net: airoha: Add sched ETS offload support")
> Reviewed-by: Simon Horman <[email protected]>
> Reviewed-by: Alexander Lobakin <[email protected]>
> Reviewed-by: Jacob Keller <[email protected]>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
>  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 d9a44a11d8db..8165084daaf5 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2521,16 +2521,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);
> +	tx_packets += (u32)(fwd_tx_packets -
> +			    dev->qos_stats[channel].fwd_tx_packets);
>  
>  	_bstats_update(opt->stats.bstats, 0, tx_packets);
> -	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;

commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260725-airoha-ethtool-priv_flags-v12-0-5136a30b2157%40kernel.org

- This isn't a bug introduced by this patch, but I noticed _bstats_update() is
  called here without mutual exclusion or BH-disabled context.
  Can a local user running 'tc -s qdisc show' invoke this dump path while the
  software qdisc datapath is running concurrently in softirq?
  Since _bstats_update() requires strict mutual exclusion among writers, could
  the lack of qdisc_lock() in this process context allow the softirq datapath
  to preempt or run concurrently?
  Would this concurrent access on 32-bit systems corrupt the seqcount_t inside
  struct gnet_stats_basic_sync and cause readers to spin infinitely?
  - As pointed out by sashiko, this issue has not been introduced by this
    patch. Moreover, airoha_eth driver runs just on arm64 architecture, so I
    guess this is not a real issue.

Regards,
Lorenzo

>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 24dd16fc509a..447a5a9552bb 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -580,9 +580,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];
>  
>  	unsigned long flags;
>  	int nbq;
> 
> -- 
> 2.55.0
>

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCamb6VAAKCRA6cBh0uS2t
rA4BAP0akbTHHixeiNxRhj7i0oQUEQ/3swX/GZcss2GkmrDwhQEAiqIETFwQfurk
fcBPV7tvUV4oLhccOCxd2s90kInyLgs=
=0sIR
-----END PGP SIGNATURE-----