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

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

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>

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().

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()).

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().

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.