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

["Wei Fang (OSS)" <wei.fang@oss.nxp.com>]

Remove NXP label,sorry for the noise.

> > +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.
> 

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.

> [ ... ]
> > +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.

> [ ... ]
> > +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.