Re: [PATCH net] tcp: initialize skb tx timestamp key before cloning

On Thu, Sep 17, 2026 at 1:32 AM Eric Dumazet [off-list ref] wrote:

On Wed, Sep 16, 2026 at 10:23 AM David Wei [off-list ref] wrote:

quoted

On 2026-09-15 15:21, Eric Dumazet wrote:

quoted

On Tue, Sep 15, 2026 at 2:44 PM David Wei [off-list ref] wrote:

quoted

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.

Initialize skb tskey just before cloning while its shinfo is still
private. In tcp_tx_timestamp(), if it selects a cloned skb from the rtx
queue from the current tcp_sendmsg_locked() call (i.e. tx_flags and
txstamp_ack are 0) and the pre-init skb tskey matches, then tx_flags can
be set safely.

If the skb is not shared, coming from the write queue, then both tskey
and tx_flags are set, as before.

Fixes: 838eb9687691 ("tcp: tcp_tx_timestamp() must look at the rtx queue")
Assisted-by: LLM
Signed-off-by: David Wei <redacted>

The commit 838eb9687691 is a best-effort attempt because it's likely
to miss reporting qdisc/driver timestamps. Everything is caused by
failing to accurately spot the last portion of each send syscall.
Things like this (when memory is under pressure) happen.

quoted

quoted

pw-bot: cr

Sorry, this patch significantly hurts TCP performance and has many bugs.

I am sure your LLM can find a cheaper solution if you ask it to try harder.

This wasn't a yolo patch straight from an LLM, I tried quite hard to
reason with the bug and possible solutions. Maybe that's the problem -
the LLM is actually smarter than me :)

quoted

Some hints if needed:

First idea:

1) In tcp_tx_timestamp():

Keep shinfo / tcb accesses inside if (unlikely(tsflags)) so normal
traffic pays zero cost.
When falling back to skb = skb_rb_last(&sk->tcp_rtx_queue):
Verify TCP_SKB_CB(skb)->end_seq == tp->write_seq and
!tcp_has_tx_tstamp(skb). If either fails, do not touch skb.
Always write shinfo->tskey before sock_tx_timestamp(sk, sockc,
&shinfo->tx_flags) (and do the same in
bpf_sock_ops_enable_tx_tstamp()).
If pre-initializing shinfo->tskey in __tcp_transmit_skb() before
skb_clone(oskb, gfp_mask) (which already dirties shinfo->dataref in
the same cache line), guard it with !tcp_has_tx_tstamp(oskb) and use
TCP_SKB_CB(oskb)->end_seq - 1 (or seq + len - 1), without the bogus if
(shinfo->tskey != tskey) check in tcp_tx_timestamp().

It minimizes the scope of handling this case without hurting normal
traffic performance. Indeed.

quoted

quoted

2) In tcp_sendmsg_locked() (to preserve TX_SCHED / TX_SOFTWARE /
TX_HARDWARE on non-blocking partial sends):

Before calling tcp_push_one() / __tcp_push_pending_frames() at the end
of the loop (when skb->len >= size_goal), check if
(!sk_stream_memory_free(sk)) goto wait_for_space;.
At wait_for_space:, only call tcp_push() if timeo != 0 (since when
!timeo, sk_stream_wait_memory() immediately returns -EAGAIN and out:
will run tcp_tx_timestamp() before tcp_push()).

The minor flaw of net timestamping is just the case where
wait_for_space and do_error conditions are met. Moving
sk_stream_wait_memory ahead of tcp_push is probably a good idea to
detect if this time the skb happens to be the last one. But it only
covers the non-blocking scenario, right?

quoted

quoted

Second idea:

We can solve this cleanly and minimally by:

Reordering the writes in tcp_tx_timestamp() (and
bpf_sock_ops_enable_tx_tstamp()) so shinfo->tskey is written first via
WRITE_ONCE(), followed by publishing shinfo->tx_flags via
smp_store_release() in __sock_tx_timestamp().

It takes care of all the readers and writers tx_flags. I'm not sure if
adding the pairs here is worth it. Admittedly it's cleaner and easy to
understand.

No offense, Eric, I'm wondering if we need to revert the commit
838eb9687691 or just let the bad case happen silently, which both are
actually a best effort. If so, the users indeed lose one timestamp
record for that send syscall, but fortunately, the bad latency will
soon be reflected on the next send syscall. I think this is how we
discussed it at Netconf a few months ago? I'm not so sure if we need
to add too much code just to work around the minor flaw of net
timestamping feature.

Thanks,
Jason

quoted

I did think about this solution. I still don't fully grasp the memory
model and how READ/WRITE_ONCE and acquire-release works, so I wasn't
entirely sure if it was sufficient.

There could be situations where the skb is pushed early, and the lower
layers copy the shinfo e.g. GSO and COW. I don't have experience with
this code, so I find it hard to evaluate plausible paths that LLMs
suggest.

That's why I opted for the pre-set tskey before clone approach in this
RFC. I didn't think this would affect perf that badly, but yeah it is a
really hot function.

I believe I can make the GSO path work with acquire-release. For COW, do
you think it is actually relevant?

I am traveling today back to France, I will answer to your email in
~24 hours or so.

Thanks.