[PATCH net v2] tcp: preserve timestamps across receive queue collapse

[PATCH net v2] tcp: preserve timestamps across receive queue collapse

[Jason Xing]

When tcp_collapse() rebuilds skbs under memory pressure, the copy
process doesn't include the right tstamp and hwtstamp from the
old skb. And memcpy(nskb->cb, skb->cb, ...) copies has_rxtstamp,
but nskb->tstamp and hwtstamps are left at zero, so
tcp_recv_timestamp() ends up emitting no cmsg at all.

In net timestamping case, if such an skb happens to be the last
one consumed in a recvmsg() call, the application receives no RX
timestamp for that call.

Fix this by copying both tstamp and hwtstamp to the new skb right
after the initialization.

Fixes: 98aaa913b4ed ("tcp: Extend SOF_TIMESTAMPING_RX_SOFTWARE to TCP recvmsg")
Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
V2
Link: https://lore.kernel.org/all/20260916124153.43457-1-kerneljasonxing@gmail.com/
1. add one sentence in commit log from Eric's reply
2. use the same semantic as tcp_add_backlog which is to sync the
   last skb (Eric)
---
 net/ipv4/tcp_input.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0f60a1dbf927..81d587654d3d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5854,6 +5854,12 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
 				if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
 					BUG();
 				TCP_SKB_CB(nskb)->end_seq += size;
+				if (TCP_SKB_CB(skb)->has_rxtstamp) {
+					TCP_SKB_CB(nskb)->has_rxtstamp = true;
+					nskb->tstamp = skb->tstamp;
+					skb_hwtstamps(nskb)->hwtstamp =
+						skb_hwtstamps(skb)->hwtstamp;
+				}
 				copy -= size;
 				start += size;
 			}
-- 
2.43.5

Re: [PATCH net v2] tcp: preserve timestamps across receive queue collapse

[Xuanqiang Luo]

在 2026/9/17 21:27, Jason Xing 写道:
> When tcp_collapse() rebuilds skbs under memory pressure, the copy
> process doesn't include the right tstamp and hwtstamp from the
> old skb. And memcpy(nskb->cb, skb->cb, ...) copies has_rxtstamp,
> but nskb->tstamp and hwtstamps are left at zero, so
> tcp_recv_timestamp() ends up emitting no cmsg at all.
> 
> In net timestamping case, if such an skb happens to be the last
> one consumed in a recvmsg() call, the application receives no RX
> timestamp for that call.
> 
> Fix this by copying both tstamp and hwtstamp to the new skb right
> after the initialization.
> 
> Fixes: 98aaa913b4ed ("tcp: Extend SOF_TIMESTAMPING_RX_SOFTWARE to TCP recvmsg")
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
Reviewed-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Thanks,
Xuanqiang

Re: [PATCH net v2] tcp: preserve timestamps across receive queue collapse

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 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).

- [Medium] Incomplete fix: a fully-covered (zero-contribution) source skb
  can still leave the collapsed skb with TCP_SKB_CB(nskb)->has_rxtstamp…
- [Low] Commit message does not describe the actual code placement or the
  resulting user-visible semantics.

--

tcp: preserve timestamps across receive queue collapse

When tcp_collapse() rebuilds receive queue skbs under memory pressure, the
cb memcpy carries has_rxtstamp over to the new skb but leaves nskb->tstamp
and the hwtstamps at zero.  This patch copies tstamp and hwtstamp from the
source skb while data is being copied into the new skb.

Link: https://lore.kernel.org/all/20260916124153.43457-1-kerneljasonxing@gmail.com/

A couple of questions below.

> tcp: preserve timestamps across receive queue collapse
[ ... ]
> Fix this by copying both tstamp and hwtstamp to the new skb right
> after the initialization.

[Severity: Low]
Does this sentence match where the code actually landed?  The copy is not
placed after the initialization block:

	memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
	skb_copy_decrypted(nskb, skb);
	TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;

but instead inside the inner "while (copy > 0)" data copy loop, right after
TCP_SKB_CB(nskb)->end_seq += size.

That difference is user visible: since the assignment runs once per source
skb merged into nskb, the collapsed skb reports the timestamp of the last
contributing skb rather than the first.  That matches tcp_add_backlog() in
net/ipv4/tcp_ipv4.c and tcp_try_coalesce(), and the V2 note below the
scissors says as much ("use the same semantic as tcp_add_backlog which is
to sync the last skb"), but everything under "---" is dropped when the
patch is applied.

Could the changelog say that the timestamp of the last merged segment is
the one kept, and drop the "right after the initialization" wording?

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 92bc60716f33d..bcb3861a38b9c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5854,6 +5854,12 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
>  				if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
>  					BUG();
>  				TCP_SKB_CB(nskb)->end_seq += size;
> +				if (TCP_SKB_CB(skb)->has_rxtstamp) {
> +					TCP_SKB_CB(nskb)->has_rxtstamp = true;
> +					nskb->tstamp = skb->tstamp;
> +					skb_hwtstamps(nskb)->hwtstamp =
> +						skb_hwtstamps(skb)->hwtstamp;
> +				}

[Severity: Medium]
Can nskb still end up with has_rxtstamp set while both timestamps stay
zero, which is the state the changelog says is being fixed?

The new block only runs under "if (size > 0)", but the flag also arrives
unconditionally through the earlier cb copy at the top of the outer loop:

	memcpy(nskb->cb, skb->cb, sizeof(skb->cb));

If the previous nskb filled up exactly at a source skb boundary, the inner
loop already advanced skb via tcp_collapse_one() before exiting, so the
next outer iteration can seed nskb->cb from an skb whose

	int size = TCP_SKB_CB(skb)->end_seq - start;

is <= 0, i.e. a fully covered skb that contributes no bytes.  Its
has_rxtstamp bit is inherited, the new copy is skipped, and if the skbs
that actually supply the data have has_rxtstamp == false, nskb keeps
flag = true with tstamp and hwtstamp at zero.

Fully covered skbs do stay in the ofo tree, which tcp_collapse() itself
notes with "No new bits? It is possible on ofo queue."  One way to reach it
is tcp_data_queue_ofo() coalescing into a non-last predecessor:

	} else if (tcp_ooo_try_coalesce(sk, skb1,
					skb, &fragstolen)) {
		goto coalesce_done;
	}

which extends skb1->end_seq and jumps past the merge_right loop that would
otherwise drop the now covered successor.

Downstream, tcp_recvmsg_locked() then acts on the inherited flag:

	if (TCP_SKB_CB(skb)->has_rxtstamp) {
		tcp_update_recv_tstamps(skb, tss);
		*cmsg_flags |= TCP_CMSG_TS;
	}

and tcp_update_recv_tstamps() overwrites unconditionally:

	tss->ts[0] = skb->tstamp;
	tss->ts[2] = skb_hwtstamps(skb)->hwtstamp;

so zeros replace timestamps already collected from earlier skbs in the same
recvmsg(), and tcp_recv_timestamp() emits nothing because it is gated on
"if (tss->ts[0] | tss->ts[2])".

Would clearing TCP_SKB_CB(nskb)->has_rxtstamp right after the cb memcpy,
so the new in-loop block is the only place that sets it, close this corner
too?

>  				copy -= size;
>  				start += size;
>  			}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917132742.87117-1-kerneljasonxing%40gmail.com

Re: [PATCH net v2] tcp: preserve timestamps across receive queue collapse

[Eric Dumazet]

On Mon, Sep 21, 2026 at 5:34 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 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).
>
> - [Medium] Incomplete fix: a fully-covered (zero-contribution) source skb
>   can still leave the collapsed skb with TCP_SKB_CB(nskb)->has_rxtstamp…
> - [Low] Commit message does not describe the actual code placement or the
>   resulting user-visible semantics.
>
> --
>
> tcp: preserve timestamps across receive queue collapse
>
> When tcp_collapse() rebuilds receive queue skbs under memory pressure, the
> cb memcpy carries has_rxtstamp over to the new skb but leaves nskb->tstamp
> and the hwtstamps at zero.  This patch copies tstamp and hwtstamp from the
> source skb while data is being copied into the new skb.
>
> Link: https://lore.kernel.org/all/20260916124153.43457-1-kerneljasonxing@gmail.com/
>
> A couple of questions below.
>
> > tcp: preserve timestamps across receive queue collapse
> [ ... ]
> > Fix this by copying both tstamp and hwtstamp to the new skb right
> > after the initialization.
>
> [Severity: Low]
> Does this sentence match where the code actually landed?  The copy is not
> placed after the initialization block:
>
>         memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
>         skb_copy_decrypted(nskb, skb);
>         TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
>
> but instead inside the inner "while (copy > 0)" data copy loop, right after
> TCP_SKB_CB(nskb)->end_seq += size.
>
> That difference is user visible: since the assignment runs once per source
> skb merged into nskb, the collapsed skb reports the timestamp of the last
> contributing skb rather than the first.  That matches tcp_add_backlog() in
> net/ipv4/tcp_ipv4.c and tcp_try_coalesce(), and the V2 note below the
> scissors says as much ("use the same semantic as tcp_add_backlog which is
> to sync the last skb"), but everything under "---" is dropped when the
> patch is applied.
>
> Could the changelog say that the timestamp of the last merged segment is
> the one kept, and drop the "right after the initialization" wording?
>
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 92bc60716f33d..bcb3861a38b9c 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -5854,6 +5854,12 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
> >                               if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
> >                                       BUG();
> >                               TCP_SKB_CB(nskb)->end_seq += size;
> > +                             if (TCP_SKB_CB(skb)->has_rxtstamp) {
> > +                                     TCP_SKB_CB(nskb)->has_rxtstamp = true;
> > +                                     nskb->tstamp = skb->tstamp;
> > +                                     skb_hwtstamps(nskb)->hwtstamp =
> > +                                             skb_hwtstamps(skb)->hwtstamp;
> > +                             }
>
> [Severity: Medium]
> Can nskb still end up with has_rxtstamp set while both timestamps stay
> zero, which is the state the changelog says is being fixed?
>
> The new block only runs under "if (size > 0)", but the flag also arrives
> unconditionally through the earlier cb copy at the top of the outer loop:
>
>         memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
>
> If the previous nskb filled up exactly at a source skb boundary, the inner
> loop already advanced skb via tcp_collapse_one() before exiting, so the
> next outer iteration can seed nskb->cb from an skb whose
>
>         int size = TCP_SKB_CB(skb)->end_seq - start;
>
> is <= 0, i.e. a fully covered skb that contributes no bytes.  Its
> has_rxtstamp bit is inherited, the new copy is skipped, and if the skbs
> that actually supply the data have has_rxtstamp == false, nskb keeps
> flag = true with tstamp and hwtstamp at zero.
>
> Fully covered skbs do stay in the ofo tree, which tcp_collapse() itself
> notes with "No new bits? It is possible on ofo queue."  One way to reach it
> is tcp_data_queue_ofo() coalescing into a non-last predecessor:
>
>         } else if (tcp_ooo_try_coalesce(sk, skb1,
>                                         skb, &fragstolen)) {
>                 goto coalesce_done;
>         }
>
> which extends skb1->end_seq and jumps past the merge_right loop that would
> otherwise drop the now covered successor.
>
> Downstream, tcp_recvmsg_locked() then acts on the inherited flag:
>
>         if (TCP_SKB_CB(skb)->has_rxtstamp) {
>                 tcp_update_recv_tstamps(skb, tss);
>                 *cmsg_flags |= TCP_CMSG_TS;
>         }
>
> and tcp_update_recv_tstamps() overwrites unconditionally:
>
>         tss->ts[0] = skb->tstamp;
>         tss->ts[2] = skb_hwtstamps(skb)->hwtstamp;
>
> so zeros replace timestamps already collected from earlier skbs in the same
> recvmsg(), and tcp_recv_timestamp() emits nothing because it is gated on
> "if (tss->ts[0] | tss->ts[2])".
>
> Would clearing TCP_SKB_CB(nskb)->has_rxtstamp right after the cb memcpy,
> so the new in-loop block is the only place that sets it, close this corner
> too?
>
> >                               copy -= size;
> >                               start += size;
> >                       }
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917132742.87117-1-kerneljasonxing%40gmail.com

The AI is right that the flag can still be inherited through the cb
memcpy from an skb
that contributes no bytes
(fully covered skb left in the ofo tree by the tcp_ooo_try_coalesce()
-> coalesce_done path).

Please add

TCP_SKB_CB(nskb)->has_rxtstamp = 0;
right after the memcpy(nskb->cb, ...) so that the new in-loop block is
the only place setting it,
and reword the changelog to say the timestamp of the last merged
segment is kept,
matching tcp_try_coalesce()/tcp_add_backlog().

pw-bot: cr

Thanks.

Re: [PATCH net v2] tcp: preserve timestamps across receive queue collapse

[Jason Xing]

On Mon, Sep 21, 2026 at 11:48 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Sep 21, 2026 at 5:34 PM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 2 potential
> > issue(s) to consider.
> >
> > Critical: 0 · High: 0 · Medium: 1 · Low: 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).
> >
> > - [Medium] Incomplete fix: a fully-covered (zero-contribution) source skb
> >   can still leave the collapsed skb with TCP_SKB_CB(nskb)->has_rxtstamp…
> > - [Low] Commit message does not describe the actual code placement or the
> >   resulting user-visible semantics.
> >
> > --
> >
> > tcp: preserve timestamps across receive queue collapse
> >
> > When tcp_collapse() rebuilds receive queue skbs under memory pressure, the
> > cb memcpy carries has_rxtstamp over to the new skb but leaves nskb->tstamp
> > and the hwtstamps at zero.  This patch copies tstamp and hwtstamp from the
> > source skb while data is being copied into the new skb.
> >
> > Link: https://lore.kernel.org/all/20260916124153.43457-1-kerneljasonxing@gmail.com/
> >
> > A couple of questions below.
> >
> > > tcp: preserve timestamps across receive queue collapse
> > [ ... ]
> > > Fix this by copying both tstamp and hwtstamp to the new skb right
> > > after the initialization.
> >
> > [Severity: Low]
> > Does this sentence match where the code actually landed?  The copy is not
> > placed after the initialization block:
> >
> >         memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
> >         skb_copy_decrypted(nskb, skb);
> >         TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
> >
> > but instead inside the inner "while (copy > 0)" data copy loop, right after
> > TCP_SKB_CB(nskb)->end_seq += size.
> >
> > That difference is user visible: since the assignment runs once per source
> > skb merged into nskb, the collapsed skb reports the timestamp of the last
> > contributing skb rather than the first.  That matches tcp_add_backlog() in
> > net/ipv4/tcp_ipv4.c and tcp_try_coalesce(), and the V2 note below the
> > scissors says as much ("use the same semantic as tcp_add_backlog which is
> > to sync the last skb"), but everything under "---" is dropped when the
> > patch is applied.
> >
> > Could the changelog say that the timestamp of the last merged segment is
> > the one kept, and drop the "right after the initialization" wording?
> >
> > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > index 92bc60716f33d..bcb3861a38b9c 100644
> > > --- a/net/ipv4/tcp_input.c
> > > +++ b/net/ipv4/tcp_input.c
> > > @@ -5854,6 +5854,12 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
> > >                               if (skb_copy_bits(skb, offset, skb_put(nskb, size), size))
> > >                                       BUG();
> > >                               TCP_SKB_CB(nskb)->end_seq += size;
> > > +                             if (TCP_SKB_CB(skb)->has_rxtstamp) {
> > > +                                     TCP_SKB_CB(nskb)->has_rxtstamp = true;
> > > +                                     nskb->tstamp = skb->tstamp;
> > > +                                     skb_hwtstamps(nskb)->hwtstamp =
> > > +                                             skb_hwtstamps(skb)->hwtstamp;
> > > +                             }
> >
> > [Severity: Medium]
> > Can nskb still end up with has_rxtstamp set while both timestamps stay
> > zero, which is the state the changelog says is being fixed?
> >
> > The new block only runs under "if (size > 0)", but the flag also arrives
> > unconditionally through the earlier cb copy at the top of the outer loop:
> >
> >         memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
> >
> > If the previous nskb filled up exactly at a source skb boundary, the inner
> > loop already advanced skb via tcp_collapse_one() before exiting, so the
> > next outer iteration can seed nskb->cb from an skb whose
> >
> >         int size = TCP_SKB_CB(skb)->end_seq - start;
> >
> > is <= 0, i.e. a fully covered skb that contributes no bytes.  Its
> > has_rxtstamp bit is inherited, the new copy is skipped, and if the skbs
> > that actually supply the data have has_rxtstamp == false, nskb keeps
> > flag = true with tstamp and hwtstamp at zero.
> >
> > Fully covered skbs do stay in the ofo tree, which tcp_collapse() itself
> > notes with "No new bits? It is possible on ofo queue."  One way to reach it
> > is tcp_data_queue_ofo() coalescing into a non-last predecessor:
> >
> >         } else if (tcp_ooo_try_coalesce(sk, skb1,
> >                                         skb, &fragstolen)) {
> >                 goto coalesce_done;
> >         }
> >
> > which extends skb1->end_seq and jumps past the merge_right loop that would
> > otherwise drop the now covered successor.
> >
> > Downstream, tcp_recvmsg_locked() then acts on the inherited flag:
> >
> >         if (TCP_SKB_CB(skb)->has_rxtstamp) {
> >                 tcp_update_recv_tstamps(skb, tss);
> >                 *cmsg_flags |= TCP_CMSG_TS;
> >         }
> >
> > and tcp_update_recv_tstamps() overwrites unconditionally:
> >
> >         tss->ts[0] = skb->tstamp;
> >         tss->ts[2] = skb_hwtstamps(skb)->hwtstamp;
> >
> > so zeros replace timestamps already collected from earlier skbs in the same
> > recvmsg(), and tcp_recv_timestamp() emits nothing because it is gated on
> > "if (tss->ts[0] | tss->ts[2])".
> >
> > Would clearing TCP_SKB_CB(nskb)->has_rxtstamp right after the cb memcpy,
> > so the new in-loop block is the only place that sets it, close this corner
> > too?
> >
> > >                               copy -= size;
> > >                               start += size;
> > >                       }
> >
> > --
> > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917132742.87117-1-kerneljasonxing%40gmail.com
>
> The AI is right that the flag can still be inherited through the cb
> memcpy from an skb
> that contributes no bytes
> (fully covered skb left in the ofo tree by the tcp_ooo_try_coalesce()
> -> coalesce_done path).

Thanks for the hit!

>
> Please add
>
> TCP_SKB_CB(nskb)->has_rxtstamp = 0;
> right after the memcpy(nskb->cb, ...) so that the new in-loop block is
> the only place setting it,
> and reword the changelog to say the timestamp of the last merged
> segment is kept,
> matching tcp_try_coalesce()/tcp_add_backlog().

Will adjust it. V3 is coming.

Thanks,
Jason