Re: [PATCH v3 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 3 potential issue(s) to consider:
- [High] The driver permanently pins its module in memory by unconditionally acquiring a module reference during port initialization, completely breaking rmmod.
- [High] Calling `module_put(THIS_MODULE)` as the last step of an asynchronous work item creates a race condition with module unloading, potentially leading to a kernel panic.
- [High] Relying on the host's SKB destructor for switch MAC transmission completion is unsafe and can lead to register corruption.
--

commit 4351c8721de223aecf2a4160d7f76a471ab63a0e
Author: Wei Fang <wei.fang@nxp.com>
Subject: net: dsa: netc: add PTP one-step timestamping support

This patch adds one-step TX timestamping support for PTP Sync frames on the
NETC switch. It defers transmission to a process-context work item to safely
program the per-port PM_SINGLE_STEP register, ensuring only one frame is in
flight at a time.

diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
--- a/drivers/net/dsa/netc/netc_ptp.c
+++ b/drivers/net/dsa/netc/netc_ptp.c
[ ... ]
> +static void netc_onestep_destroy_work(struct work_struct *work)
> +{
> +	struct netc_onestep *onestep = container_of(work, struct netc_onestep,
> +						    destroy_work);
> +
> +	/* refcnt reaching zero does not by itself mean onestep->work has
> +	 * stopped: the last in-flight skb destructor calls schedule_work(&work)
> +	 * *before* the netc_onestep_put() that drops the final reference, so at
> +	 * the moment refcnt hits zero onestep->work may still be pending or
> +	 * running on another CPU. destroy_work and work are distinct work_structs
> +	 * and can run concurrently, so cancel_work_sync() is required to drain
> +	 * onestep->work before mutex_destroy()/kfree() below, otherwise a
> +	 * still-running work would touch freed memory. No new schedule_work(&work)
> +	 * can occur after this point because no references remain, so this
> +	 * cancel is final.
> +	 */
> +	cancel_work_sync(&onestep->work);
> +	netc_port_purge_onestep_queue(onestep, true);
> +	mutex_destroy(&onestep->work_lock);
> +	kfree(onestep);
> +	module_put(THIS_MODULE);
> +}

[Severity: High]
Can calling module_put(THIS_MODULE) at the end of an asynchronous work item
lead to a use-after-free panic?

If this drops the final module reference, a concurrent rmmod could proceed to
unload the module and free the module text memory. The worker thread, which
is still running and needs to return to the workqueue core, would then execute
unmapped memory.

[ ... ]
> +static void netc_onestep_skb_destructor(struct sk_buff *skb)
> +{
> +	struct netc_onestep *onestep = skb_shinfo(skb)->destructor_arg;
> +
> +	/* skb has been transmitted by hardware. Schedule work to send the next
> +	 * queued one-step Sync packet, then release this skb's reference on the
> +	 * context. If the port has already been torn down and this is the last
> +	 * reference, the context is freed via netc_onestep_release().
> +	 */
> +	schedule_work(&onestep->work);
> +	netc_onestep_put(onestep);
> +}

[Severity: High]
Is it safe to use the skb destructor as a proxy for the switch hardware's
transmission completion?

The skb destructor fires when the host NIC completes DMA to the switch, which
happens before the switch MAC transmits the frame onto the wire. If the switch
egress queue is not empty, the first frame is still buffered inside the switch
when the destructor runs.

This would wake up the workqueue, which might then program the PM_SINGLE_STEP
register for a second frame. By the time the first frame finally leaves the
switch MAC, it could use the wrong register values, potentially corrupting the
PTP timestamps or UDP checksums on the wire.

[ ... ]
> +static int netc_port_onestep_alloc(struct netc_port *np)
> +{
> +	struct netc_onestep *onestep;
> +	int err;
> +
> +	/* Hold a module reference until the last in-flight one-step Sync skb
> +	 * is freed by the conduit. Without this, the module could be unloaded
> +	 * before netc_onestep_skb_destructor() returns, causing a panic.
> +	 * Released in netc_onestep_destroy_work() after all cleanup is done.
> +	 */
> +	if (!try_module_get(THIS_MODULE)) {
> +		dev_err(np->switch_priv->dev,
> +			"Failed to get the driver module\n");
> +		return -ENODEV;
> +	}
> +

[Severity: High]
Does this permanently pin the module in memory?

Since this is called during port initialization, acquiring a module reference
here will make the reference count non-zero for the lifetime of the port. As
a result, the module can never be unloaded via rmmod, because rmmod requires
the module reference count to be zero before it will unbind the driver and
tear down the ports.

If the goal is only to prevent the module from unloading while packets are
in-flight, would it be better to acquire the reference dynamically when the
packet is queued, rather than globally at initialization?

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