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

On 18/08/2026 15:58, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=2
> ---
> --- Patch [2]: [PATCH v10 net-next 2/3] netdevsim: Implement basic ptp support ---
> commit ef2b7171af287d8c3a8f5064008830a7dd8f4db3
> Author: Maciek Machnikowski <[email protected]>
> 
> netdevsim: Implement basic ptp support
> 
> Adds virtual hardware timestamping to netdevsim by reading the mock PHC
> attached to the transmitting netdevsim and to its peer, reporting a TX
> timestamp to the socket and stamping the forwarded skb for the RX side.
> Also wires up ndo_hwtstamp_get / ndo_hwtstamp_set and extends
> nsim_get_ts_info().
> 
> A few questions on the timestamping path below.
> 
>> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
>> index 025ea79879f3a..24e7d5592e799 100644
>> --- a/drivers/net/netdevsim/ethtool.c
>> +++ b/drivers/net/netdevsim/ethtool.c
>> @@ -200,7 +200,22 @@ static int nsim_get_ts_info(struct net_device *dev,
>>  {
>>  	struct netdevsim *ns = netdev_priv(dev);
>>  
>> +	ethtool_op_get_ts_info(dev, info);
>> +	if (!ns->phc) {
>> +		info->phc_index = -1;
>> +		return 0;
>> +	}
>> +
> 
> [Severity: High]
> [Finding: main-1-1]
> [Sources: opus-5]
> This isn't a bug introduced by this patch, but the new !ns->phc guard looks
> like it silently fixes a NULL pointer dereference that exists today, without
> a mention in the changelog or a Fixes: tag.
> 
> Before this patch nsim_get_ts_info() did:
> 
> 	info->phc_index = mock_phc_index(ns->phc);
> 
> and nsim_ethtool_init() installs the shared nsim_ethtool_ops (including
> .get_ts_info = nsim_get_ts_info) on every netdevsim netdev, while only
> nsim_init_netdevsim() calls mock_phc_create().  nsim_init_netdevsim_vf()
> just assigns nsim_vf_netdev_ops and registers the netdev, so ns->phc stays
> NULL for VF netdevs.
> 
> With CONFIG_PTP_1588_CLOCK_MOCK=y, mock_phc_index() is the real function:
> 
> drivers/ptp/ptp_mock.c:mock_phc_index() {
> 	return ptp_clock_index(phc->clock);
> }
> 
> so ethtool -T on a netdevsim VF netdev dereferences NULL.  Would it make
> sense to split this guard into its own patch with a Fixes: tag so that
> stable tooling can pick it up?
> 
> Also, adding ethtool_op_get_ts_info() here changes ethtool -T output for all
> netdevsim devices by advertising SOF_TIMESTAMPING_TX_SOFTWARE,
> SOF_TIMESTAMPING_RX_SOFTWARE and SOF_TIMESTAMPING_SOFTWARE.  Could the
> changelog mention that behaviour change?
This patchset introduces this connection - not sure what would it fix?


> 
>>  	info->phc_index = mock_phc_index(ns->phc);
>> +	if (info->phc_index < 0)
>> +		return 0;
>> +
>> +	info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
>> +				 SOF_TIMESTAMPING_RX_HARDWARE |
>> +				 SOF_TIMESTAMPING_RAW_HARDWARE;
>> +
>> +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
>> +	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | BIT(HWTSTAMP_FILTER_ALL);
>>  
>>  	return 0;
>>  }
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index 4e9d7e10b527e..e586fd8f3bc37 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
> 
> [ ... ]
> 
>> @@ -164,6 +171,44 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>  		skb_linearize(skb);
>>  
>>  	skb_tx_timestamp(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 (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_orig, &shhwtstamps);
>> +		}
> 
> [Severity: Medium]
> [Finding: gpt-5-6-sol-3-10]
> [Sources: gpt-5-6-sol, opus-5]
> Can a socket receive two SCM_TSTAMP_SND reports for the same packet here?
> 
> The hardware report is emitted after skb_tx_timestamp(), which already
> delivered the software one:
> 
> include/linux/skbuff.h:skb_tx_timestamp() {
> 	skb_clone_tx_timestamp(skb);
> 	if (skb_shinfo(skb)->tx_flags & (SKBTX_SW_TSTAMP | SKBTX_BPF))
> 		skb_tstamp_tx(skb, NULL);
> }
> 
> The only suppression of the software report lives in __skb_tstamp_tx():
> 
> net/core/skbuff.c:__skb_tstamp_tx() {
> 	if (!hwtstamps && !(tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW) &&
> 	    skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS)
> 		return;
> 
> netdevsim never sets SKBTX_IN_PROGRESS anywhere - the new code only tests
> SKBTX_HW_TSTAMP - so for a socket that enabled both
> SOF_TIMESTAMPING_TX_SOFTWARE and SOF_TIMESTAMPING_TX_HARDWARE without
> SOF_TIMESTAMPING_OPT_TX_SWHW both reports are delivered.
> 
> Would following the usual driver sequence work here, i.e. test
> SKBTX_HW_TSTAMP and set SKBTX_IN_PROGRESS before calling
> skb_tx_timestamp()?
> 
That protection is needed for the code that delays tstamp completion
into some "worker" that handles the timestamp returned by HW once the
packet was transmitted out. For a netdevsim we return the tx tstamp
immediately.
>> +
>> +		/* Unshare the skb, stamping it must not corrupt cloned copies
>> +		 * and prevents returning single tstamp for Tx and Rx
>> +		 */
>> +		skb = skb_unshare(skb_orig, GFP_ATOMIC);
>> +		if (unlikely(!skb)) {
>> +			if (psp_ext)
>> +				__skb_ext_put(psp_ext);
>> +			goto out_drop_cnt;
>> +		}
> 
> [Severity: Low]
> [Finding: main-3-8]
> [Sources: opus-5]
> Should this skb_unshare() be gated on ptp_info_rx?
> 
> The enclosing condition is ptp_info_tx || ptp_info_rx, but the only write
> into the skb below is guarded by if (ptp_info_rx).  On the TX-only path
> (peer rx_filter == HWTSTAMP_FILTER_NONE) every timestamped frame gets copied
> by skb_copy() for nothing, which also linearizes the buffer that the
> hds_config logic just above deliberately left non-linear, and does not carry
> queue_mapping.
> 
> If the GFP_ATOMIC copy then fails, the frame is dropped via out_drop_cnt
> even though nothing needed to be written into it, and after the TX
> timestamp was already handed to the socket by skb_tstamp_tx(skb_orig, ...).
> 
No - both paths requires skb_unshare. TX one needs it to unshare from
the stack for example - for TCP retransmittion and PSP

> [Severity: Low]
> [Finding: main-5-12]
> [Sources: opus-5]
> Does skb_unshare() cover the case this comment describes?
> 
> include/linux/skbuff.h:skb_unshare() {
> 	might_sleep_if(gfpflags_allow_blocking(pri));
> 	if (skb_cloned(skb)) {
> 	...
> }
> 
> The copy is made only for clones, so an skb with skb->users > 1 and
> cloned == 0 is returned unchanged.  pktgen does
> refcount_add(burst, &pkt_dev->skb->users) and resubmits the same skb, so the
> skb_hwtstamps(skb) write below touches skb_shared_info that the other
> reference holder still owns, and that the peer's NAPI path may read
> concurrently after nsim_forward_skb() -> nsim_napi_rx() queues it on
> rq->skb_queue.  Should the driver check skb_shared() as well, or stamp a
> private copy?
> 
??

>> +
>> +		/* If RX hardware timestamping is enabled report RX timestamp */
>> +		if (ptp_info_rx)
>> +			skb_hwtstamps(skb)->hwtstamp =
>> +				timespec64_to_ktime(rx_ts);
>> +	}
>> +
> 
> [Severity: Critical]
> [Finding: main-6-13]
> [Sources: opus-5]
> Can this write corrupt kernel memory for AF_XDP TX skbs?
> 
> skb_hwtstamps() returns a pointer into a union in skb_shared_info:
> 
> include/linux/skbuff.h:struct skb_shared_info {
> 	union {
> 		struct skb_shared_hwtstamps hwtstamps;
> 		struct xsk_tx_metadata_compl xsk_meta;
> 	};
> 
> struct xsk_tx_metadata_compl has a single member, u64 *tx_timestamp, and the
> AF_XDP destructor dereferences it with only a NULL check:
> 
> net/xdp/xsk.c:xsk_destruct_skb() {
> 	struct xsk_tx_metadata_compl *compl = &skb_shinfo(skb)->xsk_meta;
> 
> 	if (compl->tx_timestamp) {
> 		/* sw completion timestamp, not a real one */
> 		*compl->tx_timestamp = ktime_get_tai_fast_ns();
> 	}
> 
> Every generic-xmit AF_XDP skb carries that destructor
> (xsk_skb_init_misc() sets skb->destructor = xsk_destruct_skb), and it is
> freshly allocated and not cloned, so skb_unshare() above returns the same
> skb with the destructor and the overwritten shinfo intact.
> __dev_forward_skb() -> skb_scrub_packet() does not orphan the skb or reset
> skb_shinfo(), so the value survives to the destructor.
> 
> The RX branch is gated only on the peer's setting:
> 
> 	if (unlikely(READ_ONCE(peer_ns->tstamp_config.rx_filter) !=
> 		     HWTSTAMP_FILTER_NONE))
> 		ptp_info_rx = mock_phc_get_ptp_info(peer_ns->phc);
> 
> so any AF_XDP frame forwarded while the peer has RX timestamping enabled
> gets a non-NULL, PHC-derived value in xsk_meta.tx_timestamp, and the
> destructor performs an 8-byte write to that address.  The mock PHC value is
> settable from userspace through clock_settime() on /dev/ptpN via
> mock_phc_settime64().
> 
> Real NICs avoid this because they only stamp skbs they allocated for RX.
> Since netdevsim reuses the TX skb as the RX skb, should it orphan the skb
> (clearing skb->destructor) or stamp a private copy before touching
> skb_shinfo()->hwtstamps?
> 
Sounds like this should be addressed in the generic skb_scrub_packet in
the __dev_forward_skb path?

>>  	if (unlikely(nsim_forward_skb(dev, peer_dev,
>>  				      skb, rq, psp_ext) == NET_RX_DROP))
>>  		goto out_drop_cnt;
>> @@ -185,6 +230,63 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	return NETDEV_TX_OK;
>>  }
>>  
>> +static int nsim_set_ts_config(struct net_device *netdev,
>> +			      struct kernel_hwtstamp_config *config,
>> +			      struct netlink_ext_ack *extack)
>> +{
>> +	struct netdevsim *ns = netdev_priv(netdev);
>> +	int rx_filter;
>> +
>> +	if (!ns->phc)
>> +		return -EOPNOTSUPP;
>> +
> 
> [ ... ]
> 
>> +static int nsim_get_ts_config(struct net_device *netdev,
>> +			      struct kernel_hwtstamp_config *config)
>> +{
>> +	struct netdevsim *ns = netdev_priv(netdev);
>> +
>> +	*config = ns->tstamp_config;
>> +	return 0;
>> +}
> 
> [Severity: Low]
> [Finding: gpt-5-6-sol-2-6]
> [Sources: gpt-5-6-sol, opus-5]
> Should nsim_get_ts_config() carry the same !ns->phc check as
> nsim_set_ts_config()?
> 
> drivers/net/Kconfig allows netdevsim without the mock clock:
> 
> 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
> 
> and with CONFIG_PTP_1588_CLOCK_MOCK=n the stub is used:
> 
> include/linux/ptp_mock.h {
> static inline struct mock_phc *mock_phc_create(struct device *dev)
> {
> 	return NULL;
> }
> 
> nsim_init_netdevsim() only rejects IS_ERR(), so ns->phc stays NULL.  In that
> configuration SIOCGHWTSTAMP succeeds and reports tx_type =
> HWTSTAMP_TX_OFF / rx_filter = HWTSTAMP_FILTER_NONE, i.e. "hardware
> timestamping present but disabled", while nsim_get_ts_info() reports
> phc_index = -1 with no hardware so_timestamping and every set attempt is
> rejected with -EOPNOTSUPP.  Other converted drivers keep the get/set pair
> symmetric here.
> 
> [ ... ]
We don't support setting, getting is always available - not sure what's
the hypothetical bug there