Re: [PATCH net] ptp: ocp: prevent watchdog rearming during detach

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Incorrect bug premise in the commit message and unwarranted
  Fixes:/Cc: stable tags.

--

ptp: ocp: prevent watchdog rearming during detach

The patch replaces timer_delete_sync() with timer_shutdown_sync() for the
bp->watchdog teardown in ptp_ocp_detach(), and is tagged as a fix for
773bda964921 with Cc: stable, on the grounds that the watchdog callback can
rearm itself past the sync delete and later touch freed ptp_ocp state.

> ptp_ocp_detach() uses timer_delete_sync() to stop the watchdog before
> freeing the ptp_ocp state. timer_delete_sync() waits for a callback that is
> already running but does not prevent the callback from rearming the timer.
>
> Since ptp_ocp_watchdog() unconditionally calls mod_timer(), detach can
> return with the watchdog pending. A later callback can access freed state.

[Severity: Medium]
Is this description of timer_delete_sync() accurate for a timer whose only
rearm source is its own callback?

In this driver the only two places that arm bp->watchdog are the self-rearm
at the end of ptp_ocp_watchdog():

	mod_timer(&bp->watchdog, jiffies + HZ);

and the one-time arm in ptp_ocp_init_clock() next to the timer_setup():

	if (bp->pps_to_clk) {
		timer_setup(&bp->watchdog, ptp_ocp_watchdog, 0);
		mod_timer(&bp->watchdog, jiffies + HZ);
	}

There is no other thread or interrupt path that arms the watchdog.

For that case, doesn't __timer_delete_sync() already absorb the self-rearm
by retrying until the handler is no longer running?

kernel/time/timer.c:__timer_delete_sync() {
	...
	do {
		ret = __try_to_del_timer_sync(timer, shutdown);

		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);
	...
}

and __try_to_del_timer_sync() only returns -1 while the handler is running,
otherwise it detaches whatever the handler re-queued:

kernel/time/timer.c:__try_to_del_timer_sync() {
	...
	if (base->running_timer != timer) {
		ret = detach_if_pending(timer, base, true);
		if (shutdown)
			timer->function = NULL;
	}
	...
}

__mod_timer() also refuses to migrate a timer to another base while
base->running_timer == timer, so the callback's rearm stays on the base the
deleter inspects.  The kernel-doc for timer_delete_sync() states that upon
exit the timer is not queued and the handler is not running on any CPU, and
its rearm caveat is about other code arming the timer concurrently, which
does not exist for bp->watchdog.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9ec..6b5f615a279d9 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -4870,7 +4870,7 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>  	ptp_ocp_debugfs_remove_device(bp);
>  	ptp_ocp_detach_sysfs(bp);
>  	ptp_ocp_attr_group_del(bp);
> -	timer_delete_sync(&bp->watchdog);
> +	timer_shutdown_sync(&bp->watchdog);

[Severity: Medium]
The change itself looks harmless and matches the documented preference for
timer_shutdown_sync() on teardown paths, since bp is freshly allocated per
probe and the timer is never armed again after detach.

Given that, is there an actual use-after-free being closed here, or is this
a robustness cleanup?  If it is the latter, would it make sense to reword
the changelog without the use-after-free claim and drop the

	Fixes: 773bda964921 ("ptp: ocp: Expose various resources on the timecard.")
	Cc: stable@vger.kernel.org

tags, so the change is not backported into every stable tree as a memory
safety fix?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904064308.4044677-1-runyu.xiao%40seu.edu.cn