[PATCH v1] ptp: idt82p33: Stop PTP work producers before teardown

[PATCH v1] ptp: idt82p33: Stop PTP work producers before teardown

[Yibo Tan]

idt82p33_ptp_clock_unregister_all() cancels each channel's adjtime work
before unregistering its PTP clock. An adjustment callback already in
progress can schedule that work after cancellation returns. Device
removal then frees the channel while its delayed work remains queued,
causing a use-after-free.

Mark the device as stopping under its lock before draining work. Reject
new adjustment and EXTS requests, clear the EXTS polling mask, and
synchronously drain the EXTS and channel adjustment work before the
device storage is released.

With an i2c-stub device and kprobe-controlled callback timing, three
runs of the unmodified driver reported KASAN use-after-free after
unbind. Three runs with this change completed without a kernel
diagnostic. No physical IDT82P33 device was tested.

Signed-off-by: Yibo Tan <lhfff@tju.edu.cn>
---
 drivers/ptp/ptp_idt82p33.c | 25 +++++++++++++++++++++----
 drivers/ptp/ptp_idt82p33.h |  1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
index f01c50dfa44e8..ffc2ec1fc4bed 100644
--- a/drivers/ptp/ptp_idt82p33.c
+++ b/drivers/ptp/ptp_idt82p33.c
@@ -918,6 +918,13 @@ static void idt82p33_ptp_clock_unregister_all(struct idt82p33 *idt82p33)
 	struct idt82p33_channel *channel;
 	u8 i;
 
+	mutex_lock(idt82p33->lock);
+	idt82p33->stopping = true;
+	idt82p33->extts_mask = 0;
+	mutex_unlock(idt82p33->lock);
+
+	cancel_delayed_work_sync(&idt82p33->extts_work);
+
 	for (i = 0; i < MAX_PHC_PLL; i++) {
 		channel = &idt82p33->channel[i];
 		cancel_delayed_work_sync(&channel->adjtime_work);
@@ -937,6 +944,10 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
 	int err = -EOPNOTSUPP;
 
 	mutex_lock(idt82p33->lock);
+	if (idt82p33->stopping) {
+		err = -ENODEV;
+		goto out;
+	}
 
 	switch (rq->type) {
 	case PTP_CLK_REQ_PEROUT:
@@ -958,6 +969,7 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
 		break;
 	}
 
+out:
 	mutex_unlock(idt82p33->lock);
 
 	if (err)
@@ -1044,11 +1056,14 @@ static int idt82p33_adjtime(struct ptp_clock_info *ptp, s64 delta_ns)
 		return -EBUSY;
 
 	mutex_lock(idt82p33->lock);
+	if (idt82p33->stopping) {
+		err = -ENODEV;
+		goto out;
+	}
 
 	if (abs(delta_ns) < phase_snap_threshold) {
 		err = idt82p33_start_ddco(channel, delta_ns);
-		mutex_unlock(idt82p33->lock);
-		return err;
+		goto out;
 	}
 
 	/* Use more accurate internal 1pps triggered write first */
@@ -1056,6 +1071,7 @@ static int idt82p33_adjtime(struct ptp_clock_info *ptp, s64 delta_ns)
 	if (err && delta_ns > IMMEDIATE_SNAP_THRESHOLD_NS)
 		err = _idt82p33_adjtime_immediate(channel, delta_ns);
 
+out:
 	mutex_unlock(idt82p33->lock);
 
 	if (err)
@@ -1342,6 +1358,8 @@ static void idt82p33_extts_check(struct work_struct *work)
 		return;
 
 	mutex_lock(idt82p33->lock);
+	if (idt82p33->stopping)
+		goto out;
 
 	for (i = 0; i < MAX_PHC_PLL; i++) {
 		mask = 1 << i;
@@ -1367,6 +1385,7 @@ static void idt82p33_extts_check(struct work_struct *work)
 		schedule_delayed_work(&idt82p33->extts_work,
 				      msecs_to_jiffies(EXTTS_PERIOD_MS));
 
+out:
 	mutex_unlock(idt82p33->lock);
 }
 
@@ -1442,8 +1461,6 @@ static void idt82p33_remove(struct platform_device *pdev)
 {
 	struct idt82p33 *idt82p33 = platform_get_drvdata(pdev);
 
-	cancel_delayed_work_sync(&idt82p33->extts_work);
-
 	idt82p33_ptp_clock_unregister_all(idt82p33);
 }
 
diff --git a/drivers/ptp/ptp_idt82p33.h b/drivers/ptp/ptp_idt82p33.h
index 6a63c14b6966b..b88c921adaa91 100644
--- a/drivers/ptp/ptp_idt82p33.h
+++ b/drivers/ptp/ptp_idt82p33.h
@@ -91,6 +91,7 @@ struct idt82p33 {
 	u8			extts_mask;
 	bool			extts_single_shot;
 	struct delayed_work	extts_work;
+	bool			stopping;
 	/* Remember the ptp channel to report extts */
 	struct idt82p33_channel	*event_channel[MAX_PHC_PLL];
 	/* Mutex to protect operations from being interrupted */
-- 
2.39.5

Re: [PATCH v1] ptp: idt82p33: Stop PTP work producers before teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new `stopping` gate in `idt82p33_enable()`
  (drivers/ptp/ptp_idt82p33.c:947-949) is placed before `switch…
- [Low] Missing `Fixes:` tag and `Cc: stable@vger.kernel.org` on a
  genuine use-after-free fix.

Pre-existing issues:
- [High] User-triggerable AB-BA deadlock in the very work-cancellation
  path this patch reworks, left unfixed.
- [Medium] `idt82p33_ptp_clock_unregister_all()` unconditionally calls
  `cancel_delayed_work_sync(&channel->adjtime_work)` for all MAX_PHC_PLL…
- [Medium] The teardown path drains `extts_work` and each channel's
  `adjtime_work` but does nothing about an in-flight double-DCO (ddco)…

--

ptp: idt82p33: Stop PTP work producers before teardown

This adds a stopping flag to struct idt82p33, set under the device mutex at
the head of idt82p33_ptp_clock_unregister_all(), and uses it to reject new
adjtime and enable requests while the EXTTS and per-channel adjtime work is
drained.  The extts_work cancellation is moved out of idt82p33_remove() into
the unregister helper.

A few questions below.

[Severity: Low]
The changelog describes a real use-after-free:

    "Device removal then frees the channel while its delayed work remains
    queued, causing a use-after-free."

The producer being closed is in _idt82p33_adjtime_internal_triggered():

	/* Schedule to implement the workaround in one second */
	(void)div_s64_rem(delta_ns, NSEC_PER_SEC, &remainder);
	if (remainder != 0)
		schedule_delayed_work(&channel->adjtime_work, HZ);

and struct idt82p33 is devm_kzalloc()ed in idt82p33_probe(), so the embedded
delayed_work is freed once idt82p33_remove() returns.  git blame attributes
the schedule_delayed_work() above to ad3cc7760dc45f.  Should this carry

	Fixes: ad3cc7760dc45f ("ptp: idt82p33: Add adjphase support")
	Cc: stable@vger.kernel.org

so the fix is picked up for the affected stable trees?

> diff --git a/drivers/ptp/ptp_idt82p33.c b/drivers/ptp/ptp_idt82p33.c
> index f01c50dfa44e8..ffc2ec1fc4bed 100644
> --- a/drivers/ptp/ptp_idt82p33.c
> +++ b/drivers/ptp/ptp_idt82p33.c
> @@ -918,6 +918,13 @@ static void idt82p33_ptp_clock_unregister_all(struct idt82p33 *idt82p33)
>  	struct idt82p33_channel *channel;
>  	u8 i;
>  
> +	mutex_lock(idt82p33->lock);
> +	idt82p33->stopping = true;
> +	idt82p33->extts_mask = 0;
> +	mutex_unlock(idt82p33->lock);
> +
> +	cancel_delayed_work_sync(&idt82p33->extts_work);
> +
>  	for (i = 0; i < MAX_PHC_PLL; i++) {
>  		channel = &idt82p33->channel[i];
>  		cancel_delayed_work_sync(&channel->adjtime_work);

[Severity: Medium]
This isn't a bug introduced by this patch, but since this is the teardown
function being hardened: can this cancel_delayed_work_sync() run on a
channel whose adjtime_work was never initialized?

INIT_DELAYED_WORK(&channel->adjtime_work, ...) is the last statement of
idt82p33_channel_init(), and the per-PLL loop in idt82p33_probe() breaks on
the first failure before calling this helper:

			if (idt82p33->pll_mask & (1 << i))
				err = idt82p33_enable_channel(idt82p33, i);
			else
				err = idt82p33_channel_init(idt82p33, i);
			if (err) {
				...
				break;
			}

With DEFAULT_PLL_MASK == 0x01 and MAX_PHC_PLL == 2, a failure inside
idt82p33_enable_channel(idt82p33, 0) (an i2c/regmap error, or
ptp_clock_register() failing) leaves channel[1].adjtime_work as the zeroed
devm_kzalloc() memory.  cancel_delayed_work_sync() then reaches

kernel/workqueue.c:__flush_work() {
	if (WARN_ON(!work->func))
		return false;
}

and work_grab_pending() -> try_to_grab_pending() calls timer_delete() on a
timer that never went through debug object init, which reports assert_init
with CONFIG_DEBUG_OBJECTS_TIMERS.  Would guarding the cancel, or moving the
INIT_DELAYED_WORK() so it runs for every channel before any failure is
possible, be preferable here?

[Severity: Medium]
This is a pre-existing issue, but the drain added here does not cover an
in-flight double-DCO adjustment.  idt82p33_start_ddco() programs a
transient frequency offset and relies only on the PTP aux worker to undo it:

	err = _idt82p33_adjfine(channel,
				idt82p33_ddco_scaled_ppm(current_ppm, ppb));
	if (err)
		return err;

	/* schedule the worker to cancel ddco */
	ptp_schedule_worker(channel->ptp_clock,
			    msecs_to_jiffies(duration_ms) - 1);
	channel->ddco = true;

ptp_clock_unregister() cancels rather than runs that work:

drivers/ptp/ptp_clock.c:ptp_clock_unregister() {
	if (ptp->kworker) {
		kthread_cancel_delayed_work_sync(&ptp->aux_work);
		kthread_destroy_worker(ptp->kworker);
	}
}

So if a small-delta clock_adjtime() (|delta| below phase_snap_threshold and
at least DDCO_THRESHOLD_NS) happens within the ddco duration before unbind,
idt82p33_stop_ddco() never restores channel->current_freq and the DPLL keeps
the slew offset after the driver is gone.  Should the teardown path stop an
active ddco before unregistering the clock?

> @@ -937,6 +944,10 @@ static int idt82p33_enable(struct ptp_clock_info *ptp,
>  	int err = -EOPNOTSUPP;
>  
>  	mutex_lock(idt82p33->lock);
> +	if (idt82p33->stopping) {
> +		err = -ENODEV;
> +		goto out;
> +	}
>  
>  	switch (rq->type) {
>  	case PTP_CLK_REQ_PEROUT:

[Severity: Medium]
This gate sits before the switch, so it also refuses requests with on == 0.
Since stopping is set before the ptp_clock_unregister() loop, doesn't that
mean the PTP core's own teardown disable call is now rejected?

idt82p33_ptp_clock_unregister_all()
  ptp_clock_unregister()
    ptp_disable_all_events()
      ptp_disable_pinfunc()
        ops->enable(ops, &rq{PTP_CLK_REQ_EXTTS}, 0)   -> -ENODEV

drivers/ptp/ptp_chardev.c:ptp_disable_all_events() {
	for (i = 0; i < info->n_pins; i++)
		if (info->pin_config[i].func == PTP_PF_EXTTS)
			ptp_disable_pinfunc(info, info->pin_config[i].func,
					    info->pin_config[i].chan);
}

The return value is discarded there, but idt82p33_enable() still ends with
the unconditional

	if (err)
		dev_err(idt82p33->dev,
			"Failed in %s with err %d!\n", __func__, err);

so every unbind of a device whose userspace assigned a pin to PTP_PF_EXTTS
prints "Failed in idt82p33_enable with err -19!" for a routine core call.

The second effect is on PEROUT.  A userspace perout-off landing between
stopping = true and that channel's unregistration is also refused, so
idt82p33_perout_enable() -> idt82p33_output_enable() never runs:

	if (enable)
		val &= ~SQUELCH_ENABLE;
	else
		val |= SQUELCH_ENABLE;

	return idt82p33_write(idt82p33, OUT_MUX_CNFG(outn), &val, sizeof(val));

and the chip keeps driving the output.  Would testing the arming direction
only (on && idt82p33->stopping), or setting stopping after the
ptp_clock_unregister() loop, avoid both?

[ ... ]

> @@ -1342,6 +1358,8 @@ static void idt82p33_extts_check(struct work_struct *work)
>  		return;
>  
>  	mutex_lock(idt82p33->lock);
> +	if (idt82p33->stopping)
> +		goto out;
>  
>  	for (i = 0; i < MAX_PHC_PLL; i++) {
>  		mask = 1 << i;

[Severity: High]
This is a pre-existing issue, but the new stopping check is placed after
mutex_lock(), so it cannot break an existing AB-BA cycle around
extts_work that this patch's drain now also depends on.

idt82p33_gettime() takes the mutex and calls _idt82p33_gettime(), which does:

	u8 old_mask = idt82p33->extts_mask;
	...
	/* Disable extts */
	if (old_mask)
		new_mask = idt82p33_extts_enable_mask(channel, old_mask, false);

and with enable == false that helper drains the poller while the caller
still holds the lock:

drivers/ptp/ptp_idt82p33.c:idt82p33_extts_enable_mask() {
	if (extts_mask == 0)
		return 0;

	if (enable == false)
		cancel_delayed_work_sync(&idt82p33->extts_work);
}

The work being drained is idt82p33_extts_check(), whose only pre-lock guard
is the lockless extts_mask test, and extts_mask is non-zero on this path:

	if (idt82p33->extts_mask == 0)
		return;

	mutex_lock(idt82p33->lock);
	if (idt82p33->stopping)
		goto out;

If the 95 ms poller is already blocked on that mutex_lock(), does
cancel_delayed_work_sync() wait for a callback that is waiting for the lock
the canceller holds?  The same window exists through idt82p33_adjtime() ->
_idt82p33_adjtime_internal_triggered() -> _idt82p33_gettime().

Also, because idt82p33_ptp_clock_unregister_all() now takes
mutex_lock(idt82p33->lock) first, an unbind racing a wedged gettime blocks
there as well, so the drain this patch adds would not complete.  The mutex is
the shared rsmu MFD lock (idt82p33->lock = &ddata->lock), so the other MFD
users are held off too.  Should idt82p33_extts_check() check stopping before
taking the lock, or should the cancel in idt82p33_extts_enable_mask() be
moved outside the lock?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922113601.855416-1-lhfff%40tju.edu.cn

Re: [PATCH v1] ptp: idt82p33: Stop PTP work producers before teardown

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 22 Sep 2026 19:36:01 +0800 you wrote:
> idt82p33_ptp_clock_unregister_all() cancels each channel's adjtime work
> before unregistering its PTP clock. An adjustment callback already in
> progress can schedule that work after cancellation returns. Device
> removal then frees the channel while its delayed work remains queued,
> causing a use-after-free.
> 
> Mark the device as stopping under its lock before draining work. Reject
> new adjustment and EXTS requests, clear the EXTS polling mask, and
> synchronously drain the EXTS and channel adjustment work before the
> device storage is released.
> 
> [...]

Here is the summary with links:
  - [v1] ptp: idt82p33: Stop PTP work producers before teardown
    https://git.kernel.org/netdev/net-next/c/a529f9258da5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html