Re: [PATCH net v5] net: wwan: t7xx: fix race between TX path and system PM suspend

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

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

- [High] Incomplete fix: the commit message claims to "Quiesce all
  hardware-accessing contexts with the PM freezer" and that "none of…
- [High] Marking the CLDMA TX/RX workers, the DPMAIF TX-done worker and
  the BAT-release workqueue WQ_FREEZABLE creates blocking dependencies…
- [High] The TX push kthread is parked with plain try_to_freeze(), which
  calls __refrigerator(false) (include/linux/freezer.h:56-64).

Pre-existing issues:
- [High] t7xx_dpmaif_tx_hw_push_thread() can end its own lifetime
  ('return ret' when pm_runtime_resume_and_get() fails with anything…
- [Medium] In t7xx_dpmaif_tx_hw_push_thread() the -EACCES return of
  pm_runtime_resume_and_get() is treated as success, but…

--

net: wwan: t7xx: fix race between TX path and system PM suspend

This makes the DPMAIF TX push kthread freezable (set_freezable(),
wait_event_freezable(), try_to_freeze(), plus a freezing(current) bail-out
in the DRB-ring-full retry loop) and marks the CLDMA TX/RX workers, the
DPMAIF TX-done worker and the BAT-release workqueue WQ_FREEZABLE, so the PM
freezer parks them before the device suspend callbacks run.

A few questions about the freezer-based approach below.

[Severity: High]

The commit message says:

  "Quiesce all hardware-accessing contexts with the PM freezer, which runs
   before dpm_suspend() invokes the device suspend callbacks."

and

  "Tasks and work items are thawed only after the resume callbacks have
   re-armed the hardware, so none of these contexts can issue MMIO against
   a torn-down or not-yet-rearmed device."

Are all such contexts really covered?  t7xx_cldma_send_skb() has the same
pattern this patch is trying to quiesce:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_send_skb() {
	ret = pm_runtime_resume_and_get(md_ctrl->dev);
	if (ret < 0 && ret != -EACCES)
		return ret;

	t7xx_pci_disable_sleep(md_ctrl->t7xx_dev);
	...
}

and it is reached from contexts the freezer never touches:

  fsm_main_thread()      (kthread_run in t7xx_state_monitor.c, no
                          set_freezable(), plain wait_event_interruptible)
  port_ctl_rx_thread()   (kthread_run in t7xx_port_ctrl_msg.c, same)
  md_hk_wq              (WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, not
                          freezable)

After this patch the only set_freezable() in the driver is the one added to
t7xx_dpmaif_tx_hw_push_thread(), so those two kthreads inherit PF_NOFREEZE
from kthreadd and keep running through the whole freeze window.  A modem
MHCCIF interrupt during that window appends an FSM command
(t7xx_fsm_recv_md_intr() -> t7xx_fsm_append_cmd()) and wakes the FSM
kthread, which then drives CLDMA start/stop and control-message MMIO.

The DPMAIF receive side looks similarly uncovered:

  t7xx_dpmaif_irq_rx_done()   takes the runtime PM reference from the
                              non-freezable threaded IRQ handler and
                              napi_schedule()s
  t7xx_dpmaif_napi_rx_poll()  writes registers via
                              t7xx_dpmaif_clr_ip_busy_sts() and
                              t7xx_dpmaif_dlq_unmask_rx_done() from softirq

The freezer cannot park a softirq, and t7xx_dpmaif_suspend() only calls
t7xx_dpmaif_rx_stop() after it has already run
t7xx_dpmaif_hw_stop_all_txq()/t7xx_dpmaif_hw_stop_all_rxq() and
t7xx_dpmaif_disable_irq(), with no napi_disable()/napi_synchronize()
anywhere in the PM callbacks.

Would explicit quiescing in the PM callbacks (napi_disable() /
napi_synchronize(), plus gating or making the control kthreads freezable) be
needed for those paths, rather than more freezer participation?

[Severity: High]

Can marking these four workqueues WQ_FREEZABLE block contexts that are not
themselves frozen?

freeze_workqueues_begin() sets workqueue_freezing and wq_adjust_max_active()
forces max_active to 0 for every WQ_FREEZABLE workqueue, so items queued
during the freeze window sit on pwq->inactive_works until thaw_workqueues()
at the very end of resume.  A flush_work() on such an item links its barrier
behind the inactive item, so the flush blocks until thaw.  The WQ_MEM_RECLAIM
rescuer only walks pool->worklist, so it cannot drain inactive items either.

There are synchronous waiters on exactly these items that the freezer does
not freeze:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_stop() {
	if (md_ctrl->is_late_init) {
		for (i = 0; i < CLDMA_TXQ_NUM; i++)
			flush_work(&md_ctrl->txq[i].cldma_work);

		for (i = 0; i < CLDMA_RXQ_NUM; i++)
			flush_work(&md_ctrl->rxq[i].cldma_work);
	}
}

t7xx_cldma_stop() is reached from the non-freezable FSM kthread
(fsm_routine_start() -> t7xx_lk_stage_event_handling(), fsm_routine_stopping()),
from t7xx_md_exception() and from t7xx_cldma_exit() on the remove path.

There is also a budget wait with no timeout:

drivers/net/wwan/t7xx/t7xx_hif_cldma.c:t7xx_cldma_send_skb() {
	ret = wait_event_interruptible_exclusive(queue->req_wq, queue->budget > 0);
}

The only place that replenishes queue->budget is t7xx_cldma_gpd_tx_collect()
(the sole "queue->budget++"), which runs from t7xx_cldma_tx_done() on the
md_hif%d_tx%d_worker this hunk freezes.  That wait is held with a runtime PM
reference and t7xx_pci_disable_sleep() in effect.

The same shape exists for the two DPMAIF workqueues touched below:
t7xx_dpmaif_bat_wq_rel() flushes bat_release_work with no preceding cancel,
and t7xx_dpmaif_txq_flush_rel() does cancel_work_sync() followed by
flush_work() on an item that t7xx_dpmaif_irq_tx_done() can re-queue after
the cancel.

If the blocked waiter is t7xx_cldma_exit() -> t7xx_cldma_stop() on an
unbind/hot-unplug, it holds device_lock(), and dpm_prepare() (which runs
after the freezer) then blocks in wait_for_device_probe()/device_prepare(),
so the suspend can never reach thaw_processes().  Can this hang the suspend
outright?

Separately, t7xx_pci_pm_prepare() waits on init_done with
T7XX_INIT_TIMEOUT; if the handshake path is stuck on a frozen workqueue,
does that turn into an aborted suspend after the timeout?

[ ... ]

[Severity: High]

Should this be kthread_freezable_should_stop() instead of a bare
try_to_freeze()?

try_to_freeze() calls __refrigerator(false), and inside __refrigerator() the
loop condition is:

kernel/freezer.c:__refrigerator() {
	...
	for (;;) {
		bool freeze;
		...
		freeze = freezing(current) && !(check_kthr_stop && kthread_should_stop());
	...
}

With check_kthr_stop false a pending stop is ignored, and the task sits in
TASK_FROZEN, which wake_up_process()'s TASK_NORMAL mask does not match.  So
a concurrent

drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c:t7xx_dpmaif_tx_thread_rel() {
	if (dpmaif_ctrl->tx_thread)
		kthread_stop(dpmaif_ctrl->tx_thread);
}

blocks in wait_for_completion(&kthread->exited) until thaw_processes().

kthread_freezable_should_stop() documents this case:

kernel/kthread.c:
 * kthread_should_stop() for freezable kthreads, which will enter
 * refrigerator if necessary.  This function is safe from kthread_stop() /
 * freezer deadlock and freezable kthreads should use this function instead
 * of calling try_to_freeze() directly.

The only callers of t7xx_dpmaif_tx_thread_rel() are the probe error path and
remove (t7xx_ccmni_exit() -> t7xx_dpmaif_hif_exit()), both of which hold
device_lock()/the probe count.  If dpm_prepare() then blocks on
wait_for_device_probe()/device_lock() for the same device, does the suspend
ever reach thaw_processes()?

 		ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
 		if (ret < 0 && ret != -EACCES)
 			return ret;

[Severity: Medium]

This isn't a bug introduced by this patch, and the changelog already notes
it is deferred, but for the record: pm_runtime_resume_and_get() already
dropped the usage count before returning -EACCES:

include/linux/pm_runtime.h:pm_runtime_get_active() {
	ret = __pm_runtime_resume(dev, RPM_GET_PUT | rpmflags);
	if (ret < 0) {
		pm_runtime_put_noidle(dev);
		return ret;
	}
}

The loop body still ends with an unconditional
pm_runtime_put_autosuspend(dpmaif_ctrl->dev), so does each -EACCES
iteration drop a reference the thread does not hold and drive usage_count
negative?

[Severity: High]

This is also pre-existing rather than something this patch introduces, but
the "return ret" here ends the thread's own lifetime while
dpmaif_ctrl->tx_thread still points at that task.  The thread is created
with kthread_run() and no extra task reference is kept, so kthreadd can reap
the task_struct.  A later t7xx_dpmaif_tx_thread_rel() then does:

drivers/net/wwan/t7xx/t7xx_hif_dpmaif_tx.c:t7xx_dpmaif_tx_thread_rel() {
	if (dpmaif_ctrl->tx_thread)
		kthread_stop(dpmaif_ctrl->tx_thread);
}

and kthread_stop() immediately does get_task_struct(k) and to_kthread(k) on
that pointer.  kthread_stop()'s kerneldoc covers this:

kernel/kthread.c:
 * If threadfn() may call kthread_exit() itself, the caller must ensure
 * task_struct can't go away.

Can this be a use-after-free of the task_struct when a hard error (for
example -EINVAL once dev->power.runtime_error is set after a failed resume)
makes the thread self-exit and the device is later unbound?  Before that,
TX push is permanently dead with nothing reported.

Would clearing dpmaif_ctrl->tx_thread on exit, holding a task reference, or
not self-exiting at all fit better here, given that this thread's PM
handling is being reworked anyway?

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