Re: [PATCH v4 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: dsa: netc: skb_clone causes downstream buffer unsharing penalties and ownership violation
--

commit e1617e58a8399b2727842b7a8a9daba5929f414b
Author: Wei Fang <wei.fang@nxp.com>

net: dsa: netc: add PTP one-step timestamping support

This commit adds one-step Sync offload for NETC switch ports and advertises
HWTSTAMP_TX_ONESTEP_SYNC. It uses a deferred work mechanism and queue to
transmit one-step Sync frames, allowing hardware to compute the correction
field.

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> index 2e743443ca030..1bc0f09562062 100644
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c

[ ... ]

> @@ -68,12 +77,276 @@ static int netc_get_ts_req_id(struct netc_port *np)

[ ... ]

> +static void netc_port_program_onestep(struct netc_port *np,
> +				      struct sk_buff *skb)
> +{

[ ... ]

> +		old_sec_h = __get_unaligned_t(__be16, hdr + tstamp_offset);
> +		old_sec_l = __get_unaligned_t(__be32, hdr + tstamp_offset + 2);
> +		old_ns = __get_unaligned_t(__be32, hdr + tstamp_offset + 6);
> +		inet_proto_csum_replace2(&uh->check, skb, old_sec_h,
> +					 new_sec_h, false);

[Severity: Medium]
Does modifying the payload of a cloned SKB here violate the networking core SKB
buffer ownership guidelines?

The original SKB was cloned earlier in netc_port_onestep_work(), making its
data buffer shared (skb_cloned(skb) evaluates to true). Bypassing standard
buffer ownership rules and directly modifying the shared payload triggers an
expensive reallocation and copy (pskb_expand_head) in downstream conduit drivers
that check skb_cloned() before padding or modifying the frame, which could
severely degrade one-step PTP performance.

[ ... ]

> +static void netc_port_onestep_work(struct work_struct *work)
> +{
> +	struct netc_port *np = container_of(work, struct netc_port,
> +					    onestep_work);

[ ... ]

> +skb_dequeue:
> +	skb = __skb_dequeue(&np->onestep_queue);
> +	if (!skb)
> +		goto set_onestep_state_idle;
> +
> +	/* Clone is a ts_req_id token only; its payload is never read, so
> +	 * sharing the buffer with the mutated original is fine.
> +	 */
> +	clone = skb_clone(skb, GFP_ATOMIC);

[Severity: Medium]
Could this clone operation lead to the downstream buffer unsharing penalties
described above?

Since the clone is only used as a timeout token and its payload is never read,
would allocating a dummy lightweight SKB via alloc_skb(0, GFP_ATOMIC)
exclusively to hold the token avoid these performance penalties and
architectural ownership violations?

> +	if (unlikely(!clone)) {
> +		kfree_skb(skb);
> +		goto skb_dequeue;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918072852.501420-1-wei.fang@oss.nxp.com?part=8