[PATCH net-next v2 0/7] u64_stats: Introduce u64_stats_reads()

[PATCH net-next v2 0/7] u64_stats: Introduce u64_stats_reads()

[David Yang]

On 64bit arches, struct u64_stats_sync is empty and provides no help
against load/store tearing. memcpy() and struct copying should not be
considered tear-free. Use u64_stats_reads() instead.

v1: https://lore.kernel.org/r/20260120092137.2161162-1-mmyangfl@gmail.com
  - rename to u64_stats_reads() to clarify data flow direction
  - refine the implementation
  - add docs
  - fix struct copying too

David Yang (7):
  u64_stats: Introduce u64_stats_reads()
  u64_stats: Doc incorrect usage with plain variables
  net: bridge: mcast: fix memcpy with u64_stats
  net: openvswitch: fix load tearing with u64_stats
  macsec: fix memcpy with u64_stats
  mpls: Fix load tearing 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      | 56 ++++++++++++++++++++++++-----
 net/bridge/br_multicast.c           |  2 +-
 net/mpls/af_mpls.c                  |  2 +-
 net/openvswitch/datapath.c          |  3 +-
 6 files changed, 55 insertions(+), 16 deletions(-)

-- 
2.51.0

[PATCH net-next v2 1/7] u64_stats: Introduce u64_stats_reads()

[David Yang]

The following pattern was observed in the code tree:

        do {
                start = u64_stats_fetch_begin(&pstats->syncp);
                memcpy(&temp, &pstats->stats, sizeof(temp));
        } while (u64_stats_fetch_retry(&pstats->syncp, start));

On 64bit arches, struct u64_stats_sync is empty and provides no help
against load/store tearing. memcpy() or struct copying does not
guarantee tear free, although compilers never generate such instructions
in practice.

Theoretically the affected code should convert to u64_stats_t, or use
atomic operations properly.

However since there are needs to copy chunks of statistics, instead of
writing loops everywhere, we provide a safe memcpy() variant for that
purpose.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 include/linux/u64_stats_sync.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index 457879938fc1..15ea4db2a77b 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -79,6 +79,14 @@ static inline u64 u64_stats_read(const u64_stats_t *p)
 	return local64_read(&p->v);
 }
 
+static inline void *u64_stats_reads(void *dst, const void *src, size_t len)
+{
+	BUILD_BUG_ON(len % sizeof(u64_stats_t));
+	for (size_t i = 0; i < len; i += sizeof(u64_stats_t))
+		*(u64 *)(dst + i) = u64_stats_read((const u64_stats_t *)(src + i));
+	return dst;
+}
+
 static inline void u64_stats_set(u64_stats_t *p, u64 val)
 {
 	local64_set(&p->v, val);
@@ -110,6 +118,7 @@ static inline bool __u64_stats_fetch_retry(const struct u64_stats_sync *syncp,
 }
 
 #else /* 64 bit */
+#include <linux/string.h>
 
 typedef struct {
 	u64		v;
@@ -120,6 +129,12 @@ static inline u64 u64_stats_read(const u64_stats_t *p)
 	return p->v;
 }
 
+static inline void *u64_stats_reads(void *dst, const void *src, size_t len)
+{
+	BUILD_BUG_ON(len % sizeof(u64_stats_t));
+	return memcpy(dst, src, len);
+}
+
 static inline void u64_stats_set(u64_stats_t *p, u64 val)
 {
 	p->v = val;
-- 
2.51.0

[PATCH net-next v2 2/7] u64_stats: Doc incorrect usage with plain variables

[David Yang]

On 64-bit architectures, u64_stats does rely on the load/store atomicity
of 64-bit data.

However, users often mistakenly believe that the helpers could also
protect/"lock" plain (64-bit) variables, which can lead to load/store
tearing.

Remove the misleading "non atomic operation" comments and add explicit
examples of incorrect usage.

Users may also be tempted to use memcpy() or struct copying. Doc the
usage of u64_stats_reads() for this case.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 include/linux/u64_stats_sync.h | 41 ++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index 15ea4db2a77b..10f988170e51 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -39,21 +39,44 @@
  *   spin_lock_bh(...) or other synchronization to get exclusive access
  *   ...
  *   u64_stats_update_begin(&stats->syncp);
- *   u64_stats_add(&stats->bytes64, len); // non atomic operation
- *   u64_stats_inc(&stats->packets64);    // non atomic operation
+ *   u64_stats_add(&stats->bytes64, len);
+ *   u64_stats_inc(&stats->packets64);
  *   u64_stats_update_end(&stats->syncp);
  *
  * While a consumer (reader) should use following template to get consistent
  * snapshot for each variable (but no guarantee on several ones)
  *
- * u64 tbytes, tpackets;
- * unsigned int start;
+ *   u64 tbytes, tpackets;
+ *   unsigned int start;
  *
- * do {
- *         start = u64_stats_fetch_begin(&stats->syncp);
- *         tbytes = u64_stats_read(&stats->bytes64); // non atomic operation
- *         tpackets = u64_stats_read(&stats->packets64); // non atomic operation
- * } while (u64_stats_fetch_retry(&stats->syncp, start));
+ *   do {
+ *           start = u64_stats_fetch_begin(&stats->syncp);
+ *           tbytes = u64_stats_read(&stats->bytes64);
+ *           tpackets = u64_stats_read(&stats->packets64);
+ *   } while (u64_stats_fetch_retry(&stats->syncp, start));
+ *
+ * Remember point #2: update_begin()/update_end() and
+ * fetch_begin()/fetch_retry() are no-ops on 64-bit architectures. u64_stats
+ * _cannot_ be used to protect plain variables against tearing.
+ *
+ *   u64 stats64, cnt;
+ *   struct { u64_stats_t stats[10]; } st, buf;
+ *
+ *   u64_stats_update_begin(&stats->syncp);
+ *   stats64 = cnt;  // no
+ *   stats64 += cnt; // no
+ *   stats64++;      // no
+ *   st = buf;                      // no
+ *   memcpy(&st, &buf, sizeof(st)); // no
+ *   u64_stats_update_end(&stats->syncp);
+ *
+ *   do {
+ *           start = u64_stats_fetch_begin(&stats->syncp);
+ *           cnt = stats64; // no
+ *           buf = st;                               // no
+ *           memcpy(&buf, &st, sizeof(st));          // no
+ *           u64_stats_reads(&buf, &st, sizeof(st)); // use this instead
+ *   } while (u64_stats_fetch_retry(&stats->syncp, start));
  *
  *
  * Example of use in drivers/net/loopback.c, using per_cpu containers,
-- 
2.51.0

[PATCH net-next v2 3/7] net: bridge: mcast: fix memcpy with u64_stats

[David Yang]

On 64bit arches, struct u64_stats_sync is empty and provides no help
against load/store tearing. memcpy() should not be considered tear-free.
Use u64_stats_reads() instead.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 net/bridge/br_multicast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index d55a4ab87837..0f8e15b4fa60 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -5201,7 +5201,7 @@ void br_multicast_get_stats(const struct net_bridge *br,
 
 		do {
 			start = u64_stats_fetch_begin(&cpu_stats->syncp);
-			memcpy(&temp, &cpu_stats->mstats, sizeof(temp));
+			u64_stats_reads(&temp, &cpu_stats->mstats, sizeof(temp));
 		} while (u64_stats_fetch_retry(&cpu_stats->syncp, start));
 
 		mcast_stats_add_dir(tdst.igmp_v1queries, temp.igmp_v1queries);
-- 
2.51.0