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

[netdev-bot+sashiko@kernel.org]

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