RE: [PATCH v3 net-next 8/8] net: dsa: netc: add PTP one-step timestamping support
From: Wei Fang (OSS) <hidden>
Date: 2026-09-02 03:00:41
Also in:
imx, sashiko-reviews
quoted hunk
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[ ... ]quoted
+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 distinctwork_structsquoted
+ * 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 newschedule_work(&work)quoted
+ * 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.
Acknowledged as a theoretical race, but not worth addressing. Triggering it requires an extremely precise interleaving of rmmod with the async destroy_work's final module_put, and module unload is not a routine operation in production. The practical risk is negligible, so adding synchronization to close this window would add complexity for no meaningful benefit.
[ ... ]quoted
+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.
This is an inherent hardware limitation (PM_SINGLE_STEP is single-frame and the conduit TX completion is decoupled from the switch egress MAC), not something software can fully eliminate. In practice the window is closed by three effects: (1) the host NIC reclaims TX BDs via NAPI, so the destructor fires well after the frame has been DMA'd into the switch, not immediately; (2) the switch driver serializes via the workqueue and only schedules the next frame after the current one's TX completion, adding further delay before PM_SINGLE_STEP is reprogrammed; (3) PTP Sync is low-rate, so overlap is highly unlikely. The interval from "frame handed to the switch" to "PM_SINGLE_STEP reprogrammed" is more than enough for the switch to transmit the small frame. Even in the extremely rare case it does occur, only that one Sync frame is corrupted; the receiver drops it on checksum failure and PTP sync recovers on the next cycle, so clock synchronization is not affected.
[ ... ]quoted
+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?
The analysis doesn't hold. This reference only blocks a direct rmmod; it does not block driver unbind or port teardown. The module can still be unloaded normally with unbind first, then rmmod: unbind releases the reference during teardown, so by the time rmmod runs the refcount is back to zero and it succeeds. This is intended behavior, not a bug — no change needed. NXP Confidential