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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unloading the netc driver module does not wait for in-flight one-step Sync SKBs or asynchronous context destruction to complete, causing a kernel panic when unmapped module code is executed.
- [High] Forcefully clearing the `in_flight` flag during port disablement allows multiple one-step frames to be in flight concurrently, corrupting the shared SINGLE_STEP hardware register.
- [Medium] Manual software modification of the UDP checksum can evaluate to 0, which is invalid for IPv6 and disables checksums for IPv4.
--

--- Patch [7]: [PATCH v2 net-next 7/7] net: dsa: netc: add PTP one-step timestamping support ---
commit 4c7369406992245c62dd03f01878b8201447752c
Author: Wei Fang <wei.fang@nxp.com>

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

This commit adds PTP one-step TX timestamping support for the NETC switch.
It handles one-step Sync packets by deferring transmission to a process-context
work queue to accommodate sleeping hardware register accesses. It bypasses the
normal DSA user xmit path to avoid double counting TX stats.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 4e139ffc2f76a..55664045ba19d 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -1005,6 +1028,19 @@ static void netc_free_ports_resources(struct netc_switch *priv)
>  			continue;
>  
>  		netc_port_purge_txtstamp_queue(np);
> +
> +		/* dsa_tree_teardown() calls dsa_tree_teardown_ports() before
> +		 * dsa_tree_teardown_switches(), so netc_port_disable() is
> +		 * executed before netc_teardown() and purges onestep->queue,
> +		 * so here we only need to drop the port's owner reference.
> +		 * In-flight one-step skbs still hold references via the
> +		 * destructor; the context (and its work) is freed only after
> +		 * the conduit frees the last in-flight skb. By then np may
> +		 * be gone, but the work no longer dereferences np because
> +		 * onestep->active has been cleared.
> +		 */
> +		netc_onestep_put(np->onestep);
> +		np->onestep = NULL;
>  	}
>  }

[Severity: High]
Does unloading the netc driver module wait for in-flight one-step Sync SKBs
to complete transmission by the conduit device?

Because the driver drops its reference here and allows the module unload to
complete, the kernel could attempt to execute netc_onestep_skb_destructor()
after the module memory has been unmapped when the conduit eventually frees
the SKB, leading to a panic.

[ ... ]
> @@ -1579,6 +1622,7 @@ static int netc_port_enable(struct dsa_switch *ds, int port,
>  static void netc_port_disable(struct dsa_switch *ds, int port)
>  {
>  	struct netc_port *np = NETC_PORT(ds, port);
> +	struct netc_onestep *onestep = np->onestep;
>  
>  	/* When .port_disable() is called, .port_enable() may not have been
>  	 * called. In this case, both the prepare_count and enable_count of
> @@ -1588,6 +1632,13 @@ static void netc_port_disable(struct dsa_switch *ds, int port)
>  	if (!np->enable)
>  		return;
>  
> +	if (onestep) {
> +		mutex_lock(&onestep->work_lock);
> +		onestep->active = false;
> +		netc_port_purge_onestep_queue(onestep, true);
> +		mutex_unlock(&onestep->work_lock);
> +	}
> +
>  	clk_disable_unprepare(np->ref_clk);
>  	np->enable = false;
>  }
> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> index 1384a6f31d1c1..d0423b3c8c33a 100644
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c
[ ... ]
> +static void netc_port_program_onestep(struct netc_port *np,
> +				      struct netc_onestep *onestep,
> +				      struct sk_buff *skb,
> +				      u64 tstamp)
> +{
[ ... ]
> +		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);
> +		inet_proto_csum_replace4(&uh->check, skb, old_sec_l,
> +					 new_sec_l, false);
> +		inet_proto_csum_replace4(&uh->check, skb, old_ns,
> +					 new_ns, false);
> +		csum_update = true;
> +	}

[Severity: Medium]
Could this manual software modification of the UDP checksum evaluate to 0?

A UDP checksum of 0 is invalid for IPv6 and disables checksums for IPv4.
Does this require a check to convert a 0 result into CSUM_MANGLED_0 (0xFFFF)
to prevent non-compliant packets?

[ ... ]
> +void netc_port_purge_onestep_queue(struct netc_onestep *onestep,
> +				   bool clear_flight)
> +{
> +	struct sk_buff_head free_list;
> +
> +	__skb_queue_head_init(&free_list);
> +
> +	spin_lock_bh(&onestep->queue_lock);
> +	skb_queue_splice_init(&onestep->queue, &free_list);
> +	if (clear_flight)
> +		onestep->in_flight = false;
> +	spin_unlock_bh(&onestep->queue_lock);
> +
> +	__skb_queue_purge(&free_list);
> +}

[Severity: High]
Does forcefully clearing the in_flight flag here during port disablement
allow a new frame to be dequeued if the port is quickly brought back up?

If so, a new frame could be programmed into the shared SINGLE_STEP hardware
register while the previous frame is still physically in flight in the conduit.
Could this break the 1-to-1 serialization required by the hardware and lead to
corrupted timestamps?

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