netdev - Re: [PATCH net-next] net: airoha: Use u64_stats_t with u64_stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260125223455.31fd7a93@pumpkin>
Date: Sun, 25 Jan 2026 22:34:55 +0000
From: David Laight <david.laight.linux@...il.com>
To: David Yang <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Lorenzo Bianconi <lorenzo@...nel.org>, Andrew
 Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: airoha: Use u64_stats_t with
 u64_stats_sync properly

On Fri, 23 Jan 2026 02:52:51 +0800
David Yang <mmyangfl@...il.com> wrote:

> On 64bit arches, struct u64_stats_sync is empty and provides no help
> against load/store tearing. Convert to u64_stats_t to ensure atomic
> operations.
> 
> Signed-off-by: David Yang <mmyangfl@...il.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 136 +++++++++++------------
>  drivers/net/ethernet/airoha/airoha_eth.h |  34 +++---
>  2 files changed, 85 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 62bcbbbe2a95..6ed220e5a094 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1472,131 +1472,131 @@ static void airoha_update_hw_stats(struct airoha_gdm_port *port)
>  
>  	/* TX */
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
> -	port->stats.tx_ok_pkts += ((u64)val << 32);
> +	u64_stats_add(&port->stats.tx_ok_pkts, (u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
> -	port->stats.tx_ok_pkts += val;
> +	u64_stats_add(&port->stats.tx_ok_pkts, val);

Wouldn't that be better written as:
	u64 val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
	val = val << 32 + airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
	port->stats.tx_ok_pkts += val;
(Assuming there something has stopped the hardware increment the register
between the two accesses, and there is an associated atomic zero.)

Otherwise you are generating 'tearing' on 64bit systems by adding the high
and low halves separately - regardless of how the stats are read.
I think that works for 32bit as well.

Making the code completely unreadable with 'special' types and 'special'
copy routines really doesn't seem worth while.
The compiler won't generate code that does 'data tearing' for aligned word
accesses, and even if it did nothing would really break.
The most you want is a memcpy_in_words() function that guarantees to use
'word' size tranfsers for aligned buffers - and is probably an alias for
normal memcpy() on all current systems.

OTOH data tearing can be seen for 64 bit adds on 32bit systems.

It is worth nothing that on 32bits the 'packet count' and 'byte count'
increments get seen as a pair, this doesn't happen on 64bit - one happens
before the other.

	David