tcp: annotate data-races in tcp_get_info_chrono_stats()

tcp_get_timestamping_opt_stats() does not own the socket lock,
this is intentional.

It calls tcp_get_info_chrono_stats() while other threads could
change chrono fields in tcp_chrono_set().

I do not think we need coherent TCP socket state snapshot
in tcp_get_timestamping_opt_stats(), I chose to only
add annotations to keep KCSAN happy.

Fixes: 1c885808e456 ("tcp: SOF_TIMESTAMPING_OPT_STATS option for SO_TIMESTAMPING")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Link: https://patch.msgid.link/20260416200319.3608680-2-edumazet@google.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/include/net/tcp.h b/include/net/tcp.h
index dfa52ceefd23b7a25ed725e3a7fde3bd7e442e4e..674af493882c802ebe03e0cac6e40b7c704aa0de 100644 (file)
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2208,10 +2208,14 @@ static inline void tcp_chrono_set(struct tcp_sock *tp, const enum tcp_chrono new
        const u32 now = tcp_jiffies32;
        enum tcp_chrono old = tp->chrono_type;
 
+       /* Following WRITE_ONCE()s pair with READ_ONCE()s in
+        * tcp_get_info_chrono_stats().
+        */
        if (old > TCP_CHRONO_UNSPEC)
-               tp->chrono_stat[old - 1] += now - tp->chrono_start;
-       tp->chrono_start = now;
-       tp->chrono_type = new;
+               WRITE_ONCE(tp->chrono_stat[old - 1],
+                          tp->chrono_stat[old - 1] + now - tp->chrono_start);
+       WRITE_ONCE(tp->chrono_start, now);
+       WRITE_ONCE(tp->chrono_type, new);
 }
 
 static inline void tcp_chrono_start(struct sock *sk, const enum tcp_chrono type)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1a494d18c5fd130c2a7bb4c425eda8e4ddcdf612..7b7812cb710f6e05a83615811eefd0cf8a845cab 100644 (file)
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4191,12 +4191,18 @@ static void tcp_get_info_chrono_stats(const struct tcp_sock *tp,
                                      struct tcp_info *info)
 {
        u64 stats[__TCP_CHRONO_MAX], total = 0;
-       enum tcp_chrono i;
+       enum tcp_chrono i, cur;
 
+       /* Following READ_ONCE()s pair with WRITE_ONCE()s in tcp_chrono_set().
+        * This is because socket lock might not be owned by us at this point.
+        * This is best effort, tcp_get_timestamping_opt_stats() can
+        * see wrong values. A real fix would be too costly for TCP fast path.
+        */
+       cur = READ_ONCE(tp->chrono_type);
        for (i = TCP_CHRONO_BUSY; i < __TCP_CHRONO_MAX; ++i) {
-               stats[i] = tp->chrono_stat[i - 1];
-               if (i == tp->chrono_type)
-                       stats[i] += tcp_jiffies32 - tp->chrono_start;
+               stats[i] = READ_ONCE(tp->chrono_stat[i - 1]);
+               if (i == cur)
+                       stats[i] += tcp_jiffies32 - READ_ONCE(tp->chrono_start);
                stats[i] *= USEC_PER_SEC / HZ;
                total += stats[i];
        }