Re: [PATCH RFC net-next 8/9] net: generate the start time for every skb in the rx path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net: start_time and hwtstamp union aliasing causes UAPI breakage
- [High] net: modifying shared skb_shinfo in net_timestamp_check corrupts cloned skbs
--

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

net: generate the start time for every skb in the rx path

This commit extends net_timestamp_check() to record an in-kernel start time
in the skb's shared hwtstamps. It enables computing how long an skb has
spent in the stack during the RX path.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1da3250f25d2c..e358d28ae6c3f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2501,11 +2501,17 @@ static inline void net_timestamp_set(struct sk_buff *skb)
>  		skb->tstamp = ktime_get_real();
>  }
>  
> -#define net_timestamp_check(COND, SKB)				\
> -	if (static_branch_unlikely(&netstamp_needed_key)) {	\
> -		if ((COND) && !(SKB)->tstamp)			\
> -			(SKB)->tstamp = ktime_get_real();	\
> -	}							\
> +#define net_timestamp_check(COND, SKB)					\
> +	if (static_branch_unlikely(&netstamp_needed_key) ||		\
> +	    static_branch_unlikely(&bpfts_v2_needed_key)) {		\
> +		if (static_branch_unlikely(&netstamp_needed_key) &&	\
> +		    (COND) && !(SKB)->tstamp)				\
> +			(SKB)->tstamp = ktime_get_real();		\
> +		if (static_branch_unlikely(&bpfts_v2_needed_key) &&	\
> +		    !skb_hwtstamps(SKB)->hwtstamp)			\
> +			skb_hwtstamps(SKB)->start_time =		\

[Severity: High]
Does this code modify shared skb_shinfo without verifying SKB exclusivity?

The skb_hwtstamps(SKB) macro resolves to &skb_shinfo(skb)->hwtstamps, which
is explicitly shared across all clones of an SKB.

In the RX path, such as in netif_rx_internal() where net_timestamp_check()
is called, packets may be clones. For example, TCP transmits a clone while
retaining the original SKB in its retransmit queue, and if sent over a
loopback interface, this clone is directly passed to netif_rx().

Writing directly to shared SKB metadata without calling skb_unshare() or
verifying exclusivity via skb_cloned() can cause data races and silent
corruption across all concurrent users of the cloned SKB.

[Severity: Critical]
Does this code also inadvertently overwrite hardware timestamps delivered to
userspace?

In include/linux/skbuff.h, start_time and hwtstamp are aliases in the same
union:

struct skb_shared_hwtstamps {
	union {
		ktime_t hwtstamp;
		ktime_t start_time;
		void *netdev_data;
	};
};

When a packet without a hardware timestamp comes through, this assignment
writes a software timestamp into start_time, silently overwriting the empty
hwtstamp field with a non-zero value.

Later, when the packet propagates up the stack to a socket with
SOF_TIMESTAMPING_RAW_HARDWARE enabled, __sock_recv_timestamp() checks the
timestamp:

net/socket.c:__sock_recv_timestamp() {
    ...
    if (shhwtstamps->hwtstamp) {
        tss.ts[2] = shhwtstamps->hwtstamp;
    ...
}

Will this cause applications like PTP daemons to falsely receive a software
timestamp as a hardware timestamp, breaking the SO_TIMESTAMPING UAPI?

> +				(SKB)->tstamp ?: ktime_get_real();	\
> +	}								\
>  
>  bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
>  {

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