Re: [PATCH RFC net-next 9/9] tcp: handle the start time of each split skb in the tx path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: __tcp_transmit_skb() drops start_time when copying cloned SKBs
- [Medium] bpf: tcp_mtu_probe() misses start_time propagation for partially consumed SKBs
--

commit 29685480e121f376d52c9ec47c6328d8844a8e0f
Author: Jason Xing <kerneljasonxing@gmail.com>

tcp: handle the start time of each split skb in the tx path
    
Segmentation and collapsing create or reuse skbs with a fresh
shared_info, which would otherwise drop the start time recorded
on the original skb.
    
Handle every TCP path that manipulates the shared hwtstamps:
    
  - tcp_gso_segment(): handle the gso case. Propagate the same
    gso_skb's start time to every resulting segment.
    
  - tcp_fragment/tso_fragment(): handle the tso/recovery.. cases.
    Copy the start time onto the second half of the split so both
    fragments keep it.
    
  - tcp_skb_collapse_tstamp(): handle collapse case. Copy the eaten
    skb's start time onto the survivor.
    
  - __tcp_retransmit_skb(): handle retrans case - the retransmit
    path __tcp_retransmit_skb->__pskb_copy.

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 00417a4292225..cb43215e8b3c9 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c

[ ... ]

> @@ -3669,6 +3673,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
>  			nskb = __pskb_copy(skb, MAX_TCP_HEADER, GFP_ATOMIC);
>  			if (nskb) {
>  				nskb->dev = NULL;
> +				skb_copy_start_time(nskb, skb);
>  				err = tcp_transmit_skb(sk, nskb, 0, GFP_ATOMIC);
>  			} else {
>  				err = -ENOBUFS;

[Severity: Medium]
Does this code miss propagating the start_time when copying cloned SKBs in
__tcp_transmit_skb()?

In __tcp_transmit_skb(), if clone_it is true and the original SKB is
already cloned (e.g., still held by Qdisc), it calls pskb_copy():

net/ipv4/tcp_output.c:__tcp_transmit_skb() {
    ...
		tcp_skb_tsorted_save(oskb) {
			if (unlikely(skb_cloned(oskb)))
				skb = pskb_copy(oskb, gfp_mask);
			else
				skb = skb_clone(oskb, gfp_mask);
		} tcp_skb_tsorted_restore(oskb);
    ...
}

When pskb_copy() is executed, it fails to propagate the original SKB's
start_time to the new transmit SKB. This allocates a new skb_shared_info
with a zeroed start_time, silently dropping timestamps for these packets.

[Severity: Medium]
Could there be another missed start_time propagation for partially
consumed SKBs in tcp_mtu_probe()?

tcp_mtu_probe() allocates a new MTU probe SKB (nskb) with a zeroed
start_time, and then absorbs payload from queued SKBs. If the first
SKB is larger than the probe size, it takes the else branch to
partially consume it:

net/ipv4/tcp_output.c:tcp_mtu_probe() {
    ...
		} else {
			TCP_SKB_CB(nskb)->tcp_flags |= TCP_SKB_CB(skb)->tcp_flags &
						   ~(TCPHDR_FIN|TCPHDR_PSH);
			__pskb_trim_head(skb, copy);
			tcp_set_skb_tso_segs(skb, mss_now);
			TCP_SKB_CB(skb)->seq += copy;
		}
    ...
}

In this partial consumption branch, tcp_eat_one_skb() is not invoked, and
start_time is never explicitly copied, leaving the MTU probe packet with
start_time = 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919143732.11772-1-kerneljasonxing@gmail.com?part=9