Re: [PATCH net v2] net: ti: icss-iep: fix deadlock when disabling PPS

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch moves cancel_work_sync(&iep->work) in
  icss_iep_pps_enable() out from under ptp_clk_mutex.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a use-
  after-free / NULL dereference in the teardown path that the new guard…

--

net: ti: icss-iep: fix deadlock when disabling PPS

This patch changes icss_iep_pps_enable() so that cancel_work_sync(&iep->work)
runs after ptp_clk_mutex is dropped. This avoids a deadlock with
icss_iep_cap_cmp_work(), which takes the same mutex. It also makes
icss_iep_cap_cmp_work() return early when neither pps_enabled nor
perout_enabled is set.

> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index ec085897edf09..3b2a88c2dad88 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -534,6 +534,17 @@ static void icss_iep_cap_cmp_work(struct work_struct *work)
>  
>  	mutex_lock(&iep->ptp_clk_mutex);
>  
> +	/*
> +	 * PPS/perout may have been disabled (and the compare hardware
> +	 * cleared) while this work was queued or waiting for the mutex.
> +	 * Bail out rather than re-arm CMP1 and emit a stale PPS event once
> +	 * the event source is gone.
> +	 */
> +	if (!iep->pps_enabled && !iep->perout_enabled) {
> +		mutex_unlock(&iep->ptp_clk_mutex);
> +		return;
> +	}

[Severity: High]
This is a pre-existing issue, but does this check cover teardown when perout
is enabled instead of PPS?

icss_iep_exit() unregisters the ptp clock before it disables perout:

icss_iep_exit() {
	if (iep->ptp_clock) {
		ptp_clock_unregister(iep->ptp_clock);
		iep->ptp_clock = NULL;
	}
	icss_iep_disable(iep);

	if (iep->pps_enabled)
		icss_iep_pps_enable(iep, false);
	else if (iep->perout_enabled)
		icss_iep_perout_enable(iep, NULL, false);
	...
}

Inside ptp_clock_unregister(), ptp_disable_all_events() only sends
PTP_CLK_REQ_PPS. icss_iep_pps_enable(off) then returns -EBUSY because
perout_enabled is set. So CMP1 stays armed and the work is never drained.

ptp_clock_unregister() then unregisters ptp->pps_source and calls
put_device() on the ptp device, which can free the ptp clock. After that,
iep->ptp_clock is set to NULL without holding ptp_clk_mutex.

Later, icss_iep_perout_enable(iep, NULL, false) clears perout_enabled. It
never calls cancel_work_sync().

During this window icss_iep_cap_cmp_irq() can still queue the work.
icss_iep_cap_cmp_work() gets past the new check because perout_enabled is
still 1, and then reaches:

	ptp_clock_event(iep->ptp_clock, &pevent);

For PTP_CLOCK_PPSUSR, ptp_clock_event() calls pps_event(ptp->pps_source, ...)
without a NULL check. Can this touch a freed pps_source or ptp clock, or
dereference a NULL iep->ptp_clock?

On am654 with icssg_prueth (clockops perout_enable), this looks reachable.
Enable perout with PTP_PEROUT_REQUEST, then bring the interface down, which
goes through prueth_emac_common_stop()->icss_iep_exit().

[ ... ]

> @@ -607,7 +619,7 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
>  	} else {
>  		ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
>  		if (iep->cap_cmp_irq)
> -			cancel_work_sync(&iep->work);
> +			drain_work = true;
>  	}
>  
>  	if (!ret)
> @@ -616,6 +628,15 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on)
>  exit:
>  	mutex_unlock(&iep->ptp_clk_mutex);
>  
> +	/*
> +	 * Drain the capture-compare work only after dropping ptp_clk_mutex.
> +	 * icss_iep_cap_cmp_work() takes ptp_clk_mutex itself, so cancelling it
> +	 * under the mutex deadlocks against a handler instance already running
> +	 * and waiting for that same mutex.
> +	 */
> +	if (drain_work)
> +		cancel_work_sync(&iep->work);

[Severity: Medium]
Now that the drain runs outside ptp_clk_mutex, is the disable still ordered
before a following PPS or perout enable?

The chardev ioctls hold ptp->pincfg_mux around ops->enable(). In
drivers/ptp/ptp_sysfs.c, pps_enable_store() and period_store() call
ops->enable() without it. That means a second writer can take ptp_clk_mutex
between the mutex_unlock() and the cancel_work_sync() above:

CPU1                                  CPU2
icss_iep_pps_enable(iep, 0)
  icss_iep_perout_enable_hw(off)
  iep->pps_enabled = 0
  mutex_unlock()
                                      icss_iep_pps_enable(iep, 1)
                                        icss_iep_perout_enable_hw(on)
                                        iep->pps_enabled = 1
                                        mutex_unlock()
  cancel_work_sync(&iep->work)

Two outcomes look possible.

In the first, a stale work item from the old session gets the mutex after
CPU2. It may have been queued by a CMP1 IRQ just before the disable, or it
may already be waiting in mutex_lock(). The new check in
icss_iep_cap_cmp_work() only looks at the flags, so the item gets through.
It reads the new start value that icss_iep_update_to_next_boundary() wrote to
CMP1. It writes start + iep->period back, then reports a PPSUSR event for an
edge that has not happened yet.

Would the first pulse of the new session be skipped, with a bogus timestamp
sent to userspace?

In the second, CPU1 is delayed until the new session's first CMP1 match.
icss_iep_cap_cmp_irq() queues the new session's work, and CPU1's late
cancel_work_sync() removes it before it runs.

In the non-clockops path, this work is the only code that advances CMP1 by
iep->period. icssg_prueth_sr1 is one example, since it calls
icss_iep_init(iep, NULL, NULL, 0).

Could PPS stop for good here while pps_enabled is still true?

Before this patch, ptp_clk_mutex serialized the disable, the drain and the
next enable.

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108074733.248784-1-hj351016%40gmail.com