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

[Maciek Machnikowski <maciek@machnikowski.net>]

On 19/08/2026 12:17, Simon Horman wrote:
> On Wed, Aug 19, 2026 at 12:03:33AM +0200, Maciek Machnikowski wrote:

>>> [Severity: Low]
>>> [Finding: main-5-12]
>>> [Sources: opus-5]
>>> Does skb_unshare() cover the case this comment describes?
>>>
> d
>>> 	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?
>>>
>> ??
> 
> My understanding here is that it is possible for skbs to be shared without
> being clones. E.g. pktgen.
> 
> And that as a software driver that simulates hardware timestamps
> (unique in the Kernel?) this needs to be taken into account by
> copying skbs that are either shared or cloned.
> 
I believe it should not be an issue - if the SKB gets recycled through
the stack it'll just get a new RX tstamp when it re-enters this part of
the code. If someone disables Rx tstamps - the packet with a tstamp
without a consumer would be recycled.

>>
>>>> +
>>>> +		/* 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?
> 
> My understanding is that would be outside the scope of skb_scrub_packet.
> And doing so would would break socket memory accounting and the AF_XDP
> completion signal.
> 
> I think that this problem occurs because netdev sim both uses the same skbs
> for both RX and TX, and now modifies shared metadata. As this is an unusual
> (unique?) scenario, it seems reasonable to me for the driver to address the
> issue.
> 

I do understand your point of view, but I think it's generally something
we should scrub when forwarding the SKB. It doesn't seem right to
deliver a AF_XDP packet to a destination RX queue with some "random"
destructor.
But I'm not AF_XDP expert, so would like to get an opinion from someone
more familiar with it - Kuba - can you help with this one?