Re: [PATCH RFC net-next 6/9] bpf-timestamping: restore skb hwtstamp if it is used by start time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: start_time clobbered by hwtstamp when start_time is zero
--

commit 0169de67fe5c3f53f4f477e65c35152b1fba307d
Author: Jason Xing <kerneljasonxing@gmail.com>

bpf-timestamping: restore skb hwtstamp if it is used by start time

When BPF Timestamping V1 (SKBTX_BPF) and V2 are both active, writing the
hardware timestamp into orig_skb clobbers V2's start_time on the
retransmit-queue skb. This commit saves start_time before the write and
restores it after the V1 callback returns, so both timestamping generations
can coexist.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5686,6 +5686,7 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
>  						  struct sock *sk,
>  						  int tstype)
>  {
> +	ktime_t start = 0;
>  	int op;
>  
>  	switch (tstype) {
> @@ -5695,6 +5696,8 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
>  	case SCM_TSTAMP_SND:
>  		if (hwtstamps) {
>  			op = BPF_SOCK_OPS_TSTAMP_SND_HW_CB;
> +			if (static_branch_unlikely(&bpfts_v2_needed_key))
> +				start = skb_hwtstamps(skb)->start_time;
>  			*skb_hwtstamps(skb) = *hwtstamps;
>  		} else {
>  			op = BPF_SOCK_OPS_TSTAMP_SND_SW_CB;
[ ... ]
>  	bpf_skops_tx_timestamping(sk, skb, op);
> +	if (static_branch_unlikely(&bpfts_v2_needed_key) && start)
> +		skb_hwtstamps(skb)->start_time = start;

[Severity: High]
Does this code permanently overwrite start_time with hwtstamps when
start_time is initially 0?

The && start condition skips the restoration when start_time was 0.
This leaves the skb_shared_hwtstamps union corrupted with hwtstamps instead
of 0 for start_time on the retransmit queue, causing downstream logic to read
a massive hardware timestamp instead of 0.

Also, could re-evaluating the global static key bpfts_v2_needed_key here
introduce a race condition that skips the restore if the key is disabled
concurrently? A local boolean flag might be needed to track whether
start_time was saved.

>  }

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