* [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
* Re: [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
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-21 17:53 ` netdev-bot+sashiko
2026-09-25 19:15 ` Jakub Kicinski
2 siblings, 1 reply; 6+ messages in thread
From: Jason Xing @ 2026-09-18 1:59 UTC (permalink / raw)
To: David Wei
Cc: netdev, bpf, 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,
Björn Töpel, Jakub Sitnicki, Jiayuan Chen
On Fri, Sep 18, 2026 at 1:20 AM David Wei <dw@davidwei.uk> wrote:
>
> 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>
Thanks for the work.
Yesterday I remarked on your V1 at
https://lore.kernel.org/all/CAL+tcoDMR87sxJftp7T0JX-+NUz8VkXU67M3oNZy8Tk88sDPOA@mail.gmail.com/
I'm still doubtful if we really need to introduce this much code churn
just to fix the problem of this best-effort behavior. Even if the
patch is fixed thoroughly, it's still a best-effort attempt since
there are a few report points where it misses generating timestamps.
It has nothing to do with the patch itself, but rather the design of
net timestamping.
Thanks,
Jason
> ---
> 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 [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
2026-09-18 1:59 ` Jason Xing
@ 2026-09-18 4:35 ` David Wei
2026-09-18 5:05 ` Jason Xing
0 siblings, 1 reply; 6+ messages in thread
From: David Wei @ 2026-09-18 4:35 UTC (permalink / raw)
To: Jason Xing
Cc: netdev, bpf, 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,
Björn Töpel, Jakub Sitnicki, Jiayuan Chen
On 2026-09-17 18:59, Jason Xing wrote:
> On Fri, Sep 18, 2026 at 1:20 AM David Wei <dw@davidwei.uk> wrote:
>>
>> 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>
>
> Thanks for the work.
>
> Yesterday I remarked on your V1 at
> https://lore.kernel.org/all/CAL+tcoDMR87sxJftp7T0JX-+NUz8VkXU67M3oNZy8Tk88sDPOA@mail.gmail.com/
>
> I'm still doubtful if we really need to introduce this much code churn
> just to fix the problem of this best-effort behavior. Even if the
> patch is fixed thoroughly, it's still a best-effort attempt since
> there are a few report points where it misses generating timestamps.
> It has nothing to do with the patch itself, but rather the design of
> net timestamping.
Hi Jason, I saw your response yesterday but it seemed like you were
discussing with Eric and not me. I had this code ready anyway before you
responded, so I sent it should Eric decide to keep 838eb9687691 and
review this approach.
If you decide to revert 838eb9687691, that is fine too and I can abandon
this patch. The main thing for me is to prevent userspace from seeing
underflows, but missing timestamps are okay.
>
> Thanks,
> Jason
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
2026-09-18 4:35 ` David Wei
@ 2026-09-18 5:05 ` Jason Xing
0 siblings, 0 replies; 6+ messages in thread
From: Jason Xing @ 2026-09-18 5:05 UTC (permalink / raw)
To: David Wei, Willem de Bruijn
Cc: netdev, bpf, 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, Hangbin Liu,
Martin Karsten, Björn Töpel, Jakub Sitnicki,
Jiayuan Chen
On Fri, Sep 18, 2026 at 12:35 PM David Wei <dw@davidwei.uk> wrote:
>
> On 2026-09-17 18:59, Jason Xing wrote:
> > On Fri, Sep 18, 2026 at 1:20 AM David Wei <dw@davidwei.uk> wrote:
> >>
> >> 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>
> >
> > Thanks for the work.
> >
> > Yesterday I remarked on your V1 at
> > https://lore.kernel.org/all/CAL+tcoDMR87sxJftp7T0JX-+NUz8VkXU67M3oNZy8Tk88sDPOA@mail.gmail.com/
> >
> > I'm still doubtful if we really need to introduce this much code churn
> > just to fix the problem of this best-effort behavior. Even if the
> > patch is fixed thoroughly, it's still a best-effort attempt since
> > there are a few report points where it misses generating timestamps.
> > It has nothing to do with the patch itself, but rather the design of
> > net timestamping.
>
> Hi Jason, I saw your response yesterday but it seemed like you were
> discussing with Eric and not me. I had this code ready anyway before you
+ Willem
Oh, well, my true intention is to discuss this with all of you :)
> responded, so I sent it should Eric decide to keep 838eb9687691 and
> review this approach.
>
> If you decide to revert 838eb9687691, that is fine too and I can abandon
> this patch. The main thing for me is to prevent userspace from seeing
> underflows, but missing timestamps are okay.
I see. I'm still struggling, to be honest...
Thanks,
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
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-21 17:53 ` netdev-bot+sashiko
2026-09-25 19:15 ` Jakub Kicinski
2 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-21 17:53 UTC (permalink / raw)
To: dw
Cc: netdev, bpf, davem, edumazet, kuba, pabeni, horms, ncardwell,
kuniyu, ast, daniel, andrii, eddyz87, memxor, martin.lau, song,
yonghong.song, jolsa, emil, ihor.solodrai, john.fastabend, sdf,
willemb, liuhangbin, mkarsten, kerneljasonxing, bjorn, jakub,
jiayuan.chen
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] tcp_tx_timestamp() (net/ipv4/tcp.c) gains an undocumented early
return in the retransmit-queue fallback: `if (skb &&…
- [Medium] The new release/acquire publication protocol is bypassed by
copy-on-write of skb_shared_info.
Pre-existing issues:
- [Low] Concern that __sock_tx_timestamp() (net/socket.c:767-783) and
bpf_sock_ops_enable_tx_tstamp() (net/core/filter.c:12613-12620) now…
--
tcp: prevent stale tx timestamp keys on rtx fallback
This reorders the tskey/tx_flags stores in tcp_tx_timestamp() and
bpf_sock_ops_enable_tx_tstamp() so that tskey is written first and
tx_flags is published with smp_store_release(), and converts the
readers of tx_flags to smp_load_acquire()/READ_ONCE(). It also moves
tcp_has_tx_tstamp() from net/ipv4/tcp_output.c into include/net/tcp.h.
A few questions below.
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 436495ff2271d..dccfa127d8c26 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1156,6 +1156,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/filter.c b/net/core/filter.c
> index 61940e7535523..ed6396704e114 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -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;
> }
[Severity: Low]
This isn't a bug introduced by this patch, but the split read-modify-write
here and in __sock_tx_timestamp() is still not atomic, and drivers modify
the same shinfo byte of an in-flight clone without any shared lock, for
example igb_xmit_frame_ring():
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
Before this patch __sock_tx_timestamp() already did
u8 flags = *tx_flags;
...
*tx_flags = flags;
and the SKBTX_BPF update was likewise a plain |=, so the window for a lost
bit is the same size as before; only the annotations changed. Is it worth
noting in the changelog that these remain non-atomic updates, since the
new release annotation may read as if the update itself became safe?
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 609f2c7f4a47a..56609fc28e8a5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -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);
[Severity: Medium]
Can the acquire here still see an inconsistent flags/tskey pair when the
shinfo was copied by copy-on-write in between?
The publication in tcp_tx_timestamp() targets the shinfo of an skb that
was already cloned and handed down, and a driver on the clone can reach
pskb_expand_head() via skb_cow_head()/skb_unclone(), which snapshots the
whole shinfo with no barrier and no flag test:
net/core/skbuff.c:pskb_expand_head() {
...
memcpy((struct skb_shared_info *)(data + size),
skb_shinfo(skb),
offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
...
}
tx_flags and tskey are separate fields in that copy, so on a weakly
ordered machine the copy can pick up the newly published tx_flags
together with the pre-publication tskey of 0. After that the private
copy holds the inconsistent pair permanently, and this smp_load_acquire()
reads the copy rather than the original store, so serr->ee.ee_data ends
up as 0 minus sk_tskey - the same underflow the changelog describes.
Does the release/acquire pairing need to cover the shinfo copying paths
too, or does the metadata need to be set before the skb is handed down?
I should note the decisive step is a reordering inside memcpy() on the
copying cpu, which I can only argue is unprevented rather than
demonstrate.
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 562752352afe4..ec4ee5e5d2dec 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;
> + }
[Severity: High]
Is this early return an intended behaviour change? The changelog and the
v2 notes only describe reordering the tskey/tx_flags stores and adding
symmetric reader annotations, but this return also drops the timestamp
request belonging to the sendmsg() currently in progress.
The return sits above both the recording block and the sendmsg callback:
net/ipv4/tcp.c:tcp_tx_timestamp() {
if (tsflags && skb) {
...
sock_tx_timestamp(sk, sockc, &shinfo->tx_flags);
}
if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TX_TIMESTAMPING) && skb)
bpf_skops_tx_timestamping(sk, skb, BPF_SOCK_OPS_TSTAMP_SENDMSG_CB);
}
so when the rtx tail already carries any timestamp state, shinfo->tskey
keeps the byte offset recorded by the earlier sendmsg(), a newly
requested class is never armed through sock_tx_timestamp() or
tcb->txstamp_ack, and the callback is skipped even when sockc->tsflags
is 0.
The path that reaches the fallback is the one commit 838eb9687691 added
it for:
net/ipv4/tcp.c:tcp_sendmsg_locked() {
...
out:
if (copied) {
tcp_tx_timestamp(sk, &sockc);
tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
}
...
do_error:
tcp_remove_empty_skb(sk);
if (copied)
goto out;
...
}
With a first send arming the tail skb and a second send failing partway
(non blocking -EAGAIN from sk_stream_wait_memory(), or -ENOMEM), the
empty tail is removed, copied is non zero, and tcp_tx_timestamp() runs
with an empty write queue. skb is then the armed rtx tail,
tcp_has_tx_tstamp() is true, and nothing is recorded for this send.
Since tcp_has_tx_tstamp() also tests txstamp_ack, does an earlier
SOF_TIMESTAMPING_TX_ACK-only request suppress a later
SOF_TIMESTAMPING_TX_SOFTWARE request on the same skb? And with
SK_BPF_CB_TX_TIMESTAMPING, bpf_sock_ops_enable_tx_tstamp() sets
SKBTX_BPF and TSTAMP_ACK_BPF on every sendmsg, so the next send taking
this fallback loses BPF_SOCK_OPS_TSTAMP_SENDMSG_CB, which
include/uapi/linux/bpf.h documents as:
BPF_SOCK_OPS_TSTAMP_SENDMSG_CB, /* Called when every sendmsg syscall
* is triggered. It's used to correlate
* sendmsg timestamp with corresponding
* tskey.
*/
Also, when the rtx tail has no timestamp bits set yet, tskey and
tx_flags are still written into a possibly already cloned shinfo, so
does this guard cover the case the changelog is about?
> 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);
> }
[ ... ]
> diff --git a/net/socket.c b/net/socket.c
> index c05d86e63abf7..d16e1f491f154 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);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917172050.2191062-1-dw%40davidwei.uk
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] tcp: prevent stale tx timestamp keys on rtx fallback
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-21 17:53 ` netdev-bot+sashiko
@ 2026-09-25 19:15 ` Jakub Kicinski
2 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2026-09-25 19:15 UTC (permalink / raw)
To: David Wei
Cc: netdev, bpf, David S. Miller, Eric Dumazet, 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
On Thu, 17 Sep 2026 10:20:50 -0700 David Wei wrote:
> 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.
Not sure were we are with the discussion TBH but this is now the oldest
patch in patchwork. I'm drop it, pls repost if you want to restart the
discussion (modulo the AI review).
^ permalink raw reply [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