Netdev List
 help / color / mirror / Atom feed
* [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
@ 2026-09-17 17:20 David Wei
  2026-09-18  1:59 ` Jason Xing
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: David Wei @ 2026-09-17 17:20 UTC (permalink / raw)
  To: netdev, bpf
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Simon Horman, Neal Cardwell, Kuniyuki Iwashima,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
	Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
	Ihor Solodrai, John Fastabend, Stanislav Fomichev,
	Willem de Bruijn, Hangbin Liu, Martin Karsten, Jason Xing,
	Björn Töpel, Jakub Sitnicki, Jiayuan Chen

tcp_tx_timestamp() can select skbs from the rtx queue when all the
copied data has been sent in tcp_sendmsg_locked(). These skbs may be
already cloned, sharing the same shinfo, and handed off into the lower
tx layers.

tcp_tx_timestamp() sets tx_flags before setting skb tskey, racing with
any reader of both. It is possible to observe a valid tx_flag, but an
uninitialized tskey, which produces a large underflow after subtracting
the socket tskey.

Reorder the writes in tcp_tx_timestamp() and
bpf_sock_ops_enable_tx_tstamp() to write the tskey first, followed by
publishing tx_flags via store-release.

Readers perform a symmetric load-acquire on the tx_flags, followed by a
relaxed read of tskey.

Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
Assisted-by: LLM
Signed-off-by: David Wei <dw@davidwei.uk>
---
v2:
 - switch from pre-setting tskey to reordering tskey/tx_flag

 include/linux/skbuff.h |  3 ++-
 include/net/tcp.h      |  6 ++++++
 net/core/dev.c         |  2 +-
 net/core/filter.c      |  8 ++++++--
 net/core/skbuff.c      | 27 ++++++++++++++++-----------
 net/ipv4/tcp.c         | 11 +++++++----
 net/ipv4/tcp_offload.c | 16 ++++++++++++----
 net/ipv4/tcp_output.c  |  6 ------
 net/socket.c           |  4 ++--
 9 files changed, 52 insertions(+), 31 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 421f6fc45451..489eeb4eb390 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4776,7 +4776,8 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
 static inline void skb_tx_timestamp(struct sk_buff *skb)
 {
 	skb_clone_tx_timestamp(skb);
-	if (skb_shinfo(skb)->tx_flags & (SKBTX_SW_TSTAMP | SKBTX_BPF))
+	if (READ_ONCE(skb_shinfo(skb)->tx_flags) &
+	    (SKBTX_SW_TSTAMP | SKBTX_BPF))
 		skb_tstamp_tx(skb, NULL);
 }
 
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5e5f5f9b89a3..cc7f78b5cca3 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1159,6 +1159,12 @@ struct tcp_skb_cb {
 
 #define TCP_SKB_CB(__skb)	((struct tcp_skb_cb *)&((__skb)->cb[0]))
 
+static inline bool tcp_has_tx_tstamp(const struct sk_buff *skb)
+{
+	return TCP_SKB_CB(skb)->txstamp_ack ||
+		(READ_ONCE(skb_shinfo(skb)->tx_flags) & SKBTX_ANY_TSTAMP);
+}
+
 extern const struct inet_connection_sock_af_ops ipv4_specific;
 
 #if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/core/dev.c b/net/core/dev.c
index ecfbd72d5d1a..172ebab5d38f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4828,7 +4828,7 @@ int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev)
 	skb_reset_mac_header(skb);
 	skb_assert_len(skb);
 
-	if (unlikely(skb_shinfo(skb)->tx_flags &
+	if (unlikely(READ_ONCE(skb_shinfo(skb)->tx_flags) &
 		     (SKBTX_SCHED_TSTAMP | SKBTX_BPF)))
 		__skb_tstamp_tx(skb, NULL, NULL, skb->sk, SCM_TSTAMP_SCHED);
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..ed6396704e11 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12602,7 +12602,9 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
 __bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops,
 					      u64 flags)
 {
+	struct skb_shared_info *shinfo;
 	struct sk_buff *skb;
+	u8 tx_flags;
 
 	if (skops->op != BPF_SOCK_OPS_TSTAMP_SENDMSG_CB)
 		return -EOPNOTSUPP;
@@ -12611,9 +12613,11 @@ __bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops,
 		return -EINVAL;
 
 	skb = skops->skb;
-	skb_shinfo(skb)->tx_flags |= SKBTX_BPF;
+	shinfo = skb_shinfo(skb);
+	WRITE_ONCE(shinfo->tskey, TCP_SKB_CB(skb)->seq + skb->len - 1);
 	TCP_SKB_CB(skb)->txstamp_ack |= TSTAMP_ACK_BPF;
-	skb_shinfo(skb)->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+	tx_flags = READ_ONCE(shinfo->tx_flags);
+	smp_store_release(&shinfo->tx_flags, tx_flags | SKBTX_BPF);
 
 	return 0;
 }
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9648782fe8cb..25ca82eb6cdb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5601,7 +5601,7 @@ static void __skb_complete_tx_timestamp(struct sk_buff *skb,
 	serr->opt_stats = opt_stats;
 	serr->header.h4.iif = skb->dev ? skb->dev->ifindex : 0;
 	if (READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_ID) {
-		serr->ee.ee_data = skb_shinfo(skb)->tskey;
+		serr->ee.ee_data = READ_ONCE(skb_shinfo(skb)->tskey);
 		if (sk_is_tcp(sk))
 			serr->ee.ee_data -= atomic_read(&sk->sk_tskey);
 	}
@@ -5652,6 +5652,8 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
 	 */
 	if (likely(refcount_inc_not_zero(&sk->sk_refcnt))) {
 		*skb_hwtstamps(skb) = *hwtstamps;
+		/* Order the tskey read after observing timestamp flags. */
+		(void)smp_load_acquire(&skb_shinfo(skb)->tx_flags);
 		__skb_complete_tx_timestamp(skb, sk, SCM_TSTAMP_SND, false);
 		sock_put(sk);
 		return;
@@ -5663,19 +5665,20 @@ void skb_complete_tx_timestamp(struct sk_buff *skb,
 EXPORT_SYMBOL_GPL(skb_complete_tx_timestamp);
 
 static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
+						 u8 tx_flags,
 						 struct skb_shared_hwtstamps *hwtstamps,
 						 int tstype)
 {
 	switch (tstype) {
 	case SCM_TSTAMP_SCHED:
-		return skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP;
+		return tx_flags & SKBTX_SCHED_TSTAMP;
 	case SCM_TSTAMP_SND:
-		return skb_shinfo(skb)->tx_flags & (hwtstamps ? SKBTX_HW_TSTAMP_NOBPF :
-						    SKBTX_SW_TSTAMP);
+		return tx_flags & (hwtstamps ? SKBTX_HW_TSTAMP_NOBPF :
+						      SKBTX_SW_TSTAMP);
 	case SCM_TSTAMP_ACK:
 		return TCP_SKB_CB(skb)->txstamp_ack & TSTAMP_ACK_SK;
 	case SCM_TSTAMP_COMPLETION:
-		return skb_shinfo(skb)->tx_flags & SKBTX_COMPLETION_TSTAMP;
+		return tx_flags & SKBTX_COMPLETION_TSTAMP;
 	}
 
 	return false;
@@ -5718,20 +5721,23 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	struct sk_buff *skb;
 	bool tsonly, opt_stats = false;
 	u32 tsflags;
+	u8 tx_flags;
 
 	if (!sk)
 		return;
 
-	if (skb_shinfo(orig_skb)->tx_flags & SKBTX_BPF)
+	tx_flags = smp_load_acquire(&skb_shinfo(orig_skb)->tx_flags);
+	if (tx_flags & SKBTX_BPF)
 		skb_tstamp_tx_report_bpf_timestamping(orig_skb, hwtstamps,
 						      sk, tstype);
 
-	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, hwtstamps, tstype))
+	if (!skb_tstamp_tx_report_so_timestamping(orig_skb, tx_flags,
+						  hwtstamps, tstype))
 		return;
 
 	tsflags = READ_ONCE(sk->sk_tsflags);
 	if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
-	    skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
+	    tx_flags & SKBTX_IN_PROGRESS)
 		return;
 
 	tsonly = tsflags & SOF_TIMESTAMPING_OPT_TSONLY;
@@ -5760,9 +5766,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		return;
 
 	if (tsonly) {
-		skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
-					     SKBTX_ANY_TSTAMP;
-		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
+		skb_shinfo(skb)->tx_flags |= tx_flags & SKBTX_ANY_TSTAMP;
+		skb_shinfo(skb)->tskey = READ_ONCE(skb_shinfo(orig_skb)->tskey);
 	}
 
 	if (hwtstamps)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 562752352afe..ec4ee5e5d2de 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -483,18 +483,21 @@ static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
 	struct sk_buff *skb = tcp_write_queue_tail(sk);
 	u32 tsflags = sockc->tsflags;
 
-	if (unlikely(!skb))
+	if (unlikely(!skb)) {
 		skb = skb_rb_last(&sk->tcp_rtx_queue);
+		if (skb && tcp_has_tx_tstamp(skb))
+			return;
+	}
 
 	if (tsflags && skb) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
 		struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
 
-		sock_tx_timestamp(sk, sockc, &shinfo->tx_flags);
+		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
+			WRITE_ONCE(shinfo->tskey, tcb->seq + skb->len - 1);
 		if (tsflags & SOF_TIMESTAMPING_TX_ACK)
 			tcb->txstamp_ack |= TSTAMP_ACK_SK;
-		if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
-			shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
+		sock_tx_timestamp(sk, sockc, &shinfo->tx_flags);
 	}
 
 	if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index e74d99ca9fac..73344cdf8e16 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -16,13 +16,20 @@
 static void tcp_gso_tstamp(struct sk_buff *skb, struct sk_buff *gso_skb,
 			   unsigned int seq, unsigned int mss)
 {
-	u32 flags = skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP;
-	u32 ts_seq = skb_shinfo(gso_skb)->tskey;
+	struct skb_shared_info *shinfo = skb_shinfo(gso_skb);
+	u32 ts_seq;
+	u8 flags;
 
+	/* Pair with timestamp request publication before copying tskey. */
+	flags = smp_load_acquire(&shinfo->tx_flags) & SKBTX_ANY_TSTAMP;
+	if (!flags)
+		return;
+
+	ts_seq = READ_ONCE(shinfo->tskey);
 	while (skb) {
 		if (before(ts_seq, seq + mss)) {
-			skb_shinfo(skb)->tx_flags |= flags;
 			skb_shinfo(skb)->tskey = ts_seq;
+			skb_shinfo(skb)->tx_flags |= flags;
 			return;
 		}
 
@@ -198,7 +205,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
 	th = tcp_hdr(skb);
 	seq = ntohl(th->seq);
 
-	if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP))
+	if (unlikely(READ_ONCE(skb_shinfo(gso_skb)->tx_flags) &
+		     SKBTX_ANY_TSTAMP))
 		tcp_gso_tstamp(segs, gso_skb, seq, mss);
 
 	newcheck = ~csum_fold(csum_add(csum_unfold(th->check), delta));
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 00417a429222..7bab67f5d327 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1795,12 +1795,6 @@ static void tcp_adjust_pcount(struct sock *sk, const struct sk_buff *skb, int de
 	tcp_verify_left_out(tp);
 }
 
-static bool tcp_has_tx_tstamp(const struct sk_buff *skb)
-{
-	return TCP_SKB_CB(skb)->txstamp_ack ||
-		(skb_shinfo(skb)->tx_flags & SKBTX_ANY_TSTAMP);
-}
-
 static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
diff --git a/net/socket.c b/net/socket.c
index c05d86e63abf..d16e1f491f15 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -766,7 +766,7 @@ EXPORT_SYMBOL(sock_release);
 
 void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
 {
-	u8 flags = *tx_flags;
+	u8 flags = READ_ONCE(*tx_flags);
 
 	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
 		flags |= SKBTX_HW_TSTAMP_NOBPF;
@@ -780,7 +780,7 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
 	if (tsflags & SOF_TIMESTAMPING_TX_COMPLETION)
 		flags |= SKBTX_COMPLETION_TSTAMP;
 
-	*tx_flags = flags;
+	smp_store_release(tx_flags, flags);
 }
 EXPORT_SYMBOL(__sock_tx_timestamp);
 
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-25 19:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-17 17:20 [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback David Wei
2026-09-18  1:59 ` Jason Xing
2026-09-18  4:35   ` David Wei
2026-09-18  5:05     ` Jason Xing
2026-09-21 17:53 ` netdev-bot+sashiko
2026-09-25 19:15 ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox