On 64bit arches, struct u64_stats_sync is empty and provides no help
against load/store tearing. memcpy() should not be considered atomic
against u64 values. Use u64_stats_copy() instead.

David Yang (4):
  u64_stats: Introduce u64_stats_copy()
  net: bridge: mcast: fix memcpy with u64_stats
  macsec: fix memcpy with u64_stats
  vxlan: vnifilter: fix memcpy with u64_stats

 drivers/net/macsec.c                |  6 +++---
 drivers/net/vxlan/vxlan_vnifilter.c |  2 +-
 include/linux/u64_stats_sync.h      | 15 +++++++++++++++
 net/bridge/br_multicast.c           |  2 +-
 4 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.51.0

On Tue, Jan 20, 2026 at 05:21:28PM +0800, David Yang wrote:
> On 64bit arches, struct u64_stats_sync is empty and provides no help
> against load/store tearing. memcpy() should not be considered atomic
> against u64 values. Use u64_stats_copy() instead.

The existing memcpy() does seem problematic (even if in practice it's
not) and the proposed solution in patch #1 seems OK to me given that all
the callers only pass structures containing 64 bit counters. Couldn't
find any more instances of this pattern.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

2026-01-21, 13:16:35 +0200, Ido Schimmel wrote:
> On Tue, Jan 20, 2026 at 05:21:28PM +0800, David Yang wrote:
> > On 64bit arches, struct u64_stats_sync is empty and provides no help
> > against load/store tearing. memcpy() should not be considered atomic
> > against u64 values. Use u64_stats_copy() instead.
> 
> The existing memcpy() does seem problematic (even if in practice it's
> not) and the proposed solution in patch #1 seems OK to me given that all
> the callers only pass structures containing 64 bit counters. Couldn't
> find any more instances of this pattern.

No direct instances using memcpy, but do we need to also full structs
copied within a u64_stats_fetch_begin/u64_stats_fetch_retry loop?


// net/mpls/af_mpls.c
static void mpls_get_stats(struct mpls_dev *mdev,
			   struct mpls_link_stats *stats)
{
[...]
	for_each_possible_cpu(i) {
		struct mpls_link_stats local;
		unsigned int start;

		p = per_cpu_ptr(mdev->stats, i);
		do {
			start = u64_stats_fetch_begin(&p->syncp);
			local = p->stats;
		} while (u64_stats_fetch_retry(&p->syncp, start));
[...]

// net/openvswitch/datapath.c
static void get_dp_stats(const struct datapath *dp, struct ovs_dp_stats *stats,
			 struct ovs_dp_megaflow_stats *mega_stats)
{
[...]
	for_each_possible_cpu(i) {
		const struct dp_stats_percpu *percpu_stats;
		struct dp_stats_percpu local_stats;
		unsigned int start;

		percpu_stats = per_cpu_ptr(dp->stats_percpu, i);

		do {
			start = u64_stats_fetch_begin(&percpu_stats->syncp);
			local_stats = *percpu_stats;
		} while (u64_stats_fetch_retry(&percpu_stats->syncp, start));
[...]


And if not: can't we just use the same pattern for those other cases
that this series is touching?

-- 
Sabrina

On Wed, Jan 21, 2026 at 06:21:05PM +0100, Sabrina Dubroca wrote:
> No direct instances using memcpy, but do we need to also full structs
> copied within a u64_stats_fetch_begin/u64_stats_fetch_retry loop?

My understanding is that we cannot rely on the compiler to perform the
copy in any particular way. With the suggested helper it is at least
clear how the copy is done.

2026-01-22, 10:00:43 +0200, Ido Schimmel wrote:
> On Wed, Jan 21, 2026 at 06:21:05PM +0100, Sabrina Dubroca wrote:
> > No direct instances using memcpy, but do we need to also full structs
> > copied within a u64_stats_fetch_begin/u64_stats_fetch_retry loop?
> 
> My understanding is that we cannot rely on the compiler to perform the
> copy in any particular way. With the suggested helper it is at least
> clear how the copy is done.

Ok, thanks. So those (and a few more similar things in drivers/net)
should also be switched to this new helper.

-- 
Sabrina