Re: [PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] remoteproc: imx_dsp_rproc: AB-BA deadlock in
  imx_dsp_rproc_free_mbox()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] remoteproc: imx_dsp_rproc: use-after-free and resource
  leaks on unbind [1]
--

commit 17e3fec9a8c6e436afd532f653405b5a78e316af
Author: Hyunjung Ko <hj351016@gmail.com>

remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

This commit calls cancel_work_sync(&priv->rproc_work) in
imx_dsp_rproc_free_mbox() after freeing the mailbox channels to drain any
pending virtqueue work item.

> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 96d619daf3f5b..89c72d916b1e4 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -642,6 +642,13 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
>  	mbox_free_channel(priv->tx_ch);
>  	mbox_free_channel(priv->rx_ch);
>  	mbox_free_channel(priv->rxdb_ch);
> +
> +	/*
> +	 * The mailbox rx callback schedules rproc_work; the channels are now
> +	 * freed, so no new work can be queued. Drain any pending work before
> +	 * the rproc (which the work dereferences) can be freed.
> +	 */
> +	cancel_work_sync(&priv->rproc_work);
>  }

[Severity: High]
Can calling cancel_work_sync(&priv->rproc_work) in imx_dsp_rproc_free_mbox()
cause an ABBA deadlock with rproc->lock?

CPU0                              CPU1
----                              ----
mutex_lock(&rproc->lock)
                                  imx_dsp_rproc_vq_work()
cancel_work_sync(&priv->rproc_work)
                                    mutex_lock(&rproc->lock)

 *** DEADLOCK ***

CPU0: __rproc_shutdown() -> rproc_unprepare_device() ->
      imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
      imx_dsp_runtime_suspend() -> imx_dsp_rproc_free_mbox()
CPU1: process_one_work() -> imx_dsp_rproc_vq_work()

Because priv->rx_ch remains active until mbox_free_channel(priv->rx_ch) runs
inside imx_dsp_rproc_free_mbox(), an inbound mailbox interrupt before or
during shutdown can queue priv->rproc_work while __rproc_shutdown() holds
rproc->lock.  imx_dsp_rproc_vq_work() then blocks on rproc->lock while
imx_dsp_rproc_free_mbox() waits in cancel_work_sync(&priv->rproc_work).

[Severity: Critical]
This problem wasn't introduced by this patch, but can unbinding an active
DSP remoteproc device skip imx_dsp_rproc_free_mbox() entirely [1] and
leave both the mailbox channels and priv->rproc_work active when the rproc is
freed?

drivers/remoteproc/imx_dsp_rproc.c:imx_dsp_rproc_probe() {
	...
	ret = devm_rproc_add(dev, rproc);
	if (ret)
		return dev_err_probe(dev, ret, "rproc_add failed\n");

	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);

	return devm_pm_runtime_enable(dev);
}

Because devm_rproc_add() is registered before devm_pm_runtime_enable(),
devres teardown disables runtime PM before devm_rproc_remove() calls
rproc_del() and __rproc_shutdown().  When imx_dsp_rproc_unprepare() then
calls pm_runtime_put_sync(), rpm_suspend() fails with -EACCES without
calling imx_dsp_runtime_suspend() or imx_dsp_rproc_free_mbox(), leaving
priv->clks enabled and the mailbox channels and priv->rproc_work able to
dereference the freed rproc and imx_dsp_rproc structures.

[1] https://sashiko.dev/#/bug/linux-a6e902ee-9958-4d44-96cc-d854c6256124

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110105436.2487822-1-hj351016@gmail.com?part=1