Re: [PATCH v12 net-next 2/3] netdevsim: Implement basic ptp support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] nsim_start_xmit() (drivers/net/netdevsim/netdev.c) only
  overwrites or clears skb_hwtstamps(skb)->hwtstamp inside `if…

--

netdevsim: Implement basic ptp support

This adds virtual hardware timestamping to netdevsim. Each netdevsim
uses an attached ptp_mock clock. TX timestamps are reported through
skb_tstamp_tx(), and RX timestamps are written into the forwarded
skb's shinfo hwtstamps. The patch also adds ndo_hwtstamp_get/set and
updates ethtool ts_info.

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 0372a9bd63bbb..f098520bdb034 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c

[ ... ]

> @@ -167,7 +173,53 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	      cfg->hds_thresh > len)))
>  		skb_linearize(skb);
>  
> +	if (unlikely(READ_ONCE(peer_ns->tstamp_config.rx_filter) !=
> +		     HWTSTAMP_FILTER_NONE))
> +		ptp_info_rx = mock_phc_get_ptp_info(peer_ns->phc);
> +
> +	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> +	    READ_ONCE(ns->tstamp_config.tx_type) == HWTSTAMP_TX_ON) {
> +		ptp_info_tx = mock_phc_get_ptp_info(ns->phc);
> +		if (ptp_info_tx)
> +			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	}
> +
>  	skb_tx_timestamp(skb);
> +
> +	if (unlikely(ptp_info_tx || ptp_info_rx)) {
> +		/* Capture both timestamps closely */
> +		if (ptp_info_tx)
> +			ptp_info_tx->gettime64(ptp_info_tx, &tx_ts);
> +		if (ptp_info_rx)
> +			ptp_info_rx->gettime64(ptp_info_rx, &rx_ts);
> +
> +		/* If TX hardware timestamping is enabled report TX timestamp */
> +		if (ptp_info_tx) {
> +			shhwtstamps.hwtstamp = timespec64_to_ktime(tx_ts);
> +			skb_tstamp_tx(skb, &shhwtstamps);
> +		}
> +
> +		/* Clones (e.g. the TX timestamp errqueue skb) share shinfo */
> +		if (unlikely(skb_unclone(skb, GFP_ATOMIC))) {
> +			kfree_skb(skb);
> +			if (psp_ext)
> +				__skb_ext_put(psp_ext);
> +			goto out_drop_cnt;
> +		}
> +
> +		if (ptp_info_rx) {
> +			/* hwtstamp aliases xsk_meta; orphan first so any
> +			 * pending AF_XDP completion isn't corrupted.
> +			 */
> +			skb_orphan(skb);
> +			skb_hwtstamps(skb)->hwtstamp =
> +				timespec64_to_ktime(rx_ts);
> +		} else {
> +			/* Don't leak the TX timestamp as an RX timestamp */
> +			skb_hwtstamps(skb)->hwtstamp = 0;
> +		}
> +	}

[Severity: Medium]
Can the TX timestamp still leak out as an RX timestamp through the
retransmit queue skb?

In nsim_start_xmit(), skb_tstamp_tx() runs before skb_unclone(). If the
socket does not use SOF_TIMESTAMPING_OPT_TSONLY, __skb_tstamp_tx() clones
the skb and writes the hardware timestamp into the clone:

net/core/skbuff.c:__skb_tstamp_tx() {
    ...
	skb = skb_clone(orig_skb, GFP_ATOMIC);
    ...
	if (hwtstamps)
		*skb_hwtstamps(skb) = *hwtstamps;
    ...
}

That clone shares skb_shared_info with the skb that was passed to the
driver. For TCP, the driver's skb is itself a clone of the retransmit
queue skb. If it is still cloned at this point, tx_ts ends up in the rtx
queue skb's shinfo.

skb_unclone() and the hwtstamp = 0 branch only clean the private copy
that gets forwarded.

By default, the earlier skb_linearize() goes through pskb_expand_head()
and gives the skb a private shinfo. That call is skipped when the peer
has tcp-data-split enabled and hds_thresh <= len. The default hds_thresh
is 0. The path is also open when skb_linearize() fails, because its
return value is ignored.

A later retransmit can then skip the TX branch in one of two ways:

  - tcp_fragment() copies the stale hwtstamps into the head via
    skb_unclone_keeptruesize(), while tcp_fragment_tstamp() moves
    SKBTX_HW_TSTAMP to the tail fragment.
  - tx_type has been switched to HWTSTAMP_TX_OFF in the meantime.

If the peer also has rx_filter == HWTSTAMP_FILTER_NONE, this check is
false:

	if (unlikely(ptp_info_tx || ptp_info_rx)) {

So nothing touches hwtstamp before
nsim_forward_skb()->__dev_forward_skb()->skb_scrub_packet(), and none of
those clear the shinfo hwtstamps.

Would the peer's TCP socket then report the sender's old TX PHC time as
a hardware RX timestamp, even though RX timestamping is disabled on that
device?

A related problem existed before this patch and was not introduced by
it: an AF_XDP generic-xmit skb's xsk_meta.tx_timestamp pointer lives in
the same union. It is forwarded to the peer unchanged when neither
branch runs.

> +
>  	if (unlikely(nsim_forward_skb(dev, peer_dev,
>  				      skb, rq, psp_ext) == NET_RX_DROP))
>  		goto out_drop_cnt;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108201503.85000-1-maciek%40machnikowski.net