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

[netdev-bot+sashiko@kernel.org]

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