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

[Peng Fan <peng.fan@oss.nxp.com>]

Add back remoteproc list.

Peng

On Tue, Nov 10, 2026 at 08:40:44PM +0900, khj wrote:
>Thanks -- both are correct, and my patch is wrong.
>
>[High] The AB-BA deadlock is real. imx_dsp_rproc_free_mbox() runs from the
>pm_runtime suspend path (__rproc_shutdown() -> rproc_unprepare_device() ->
>imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
>imx_dsp_runtime_suspend()),
>and __rproc_shutdown() holds rproc->lock across that. imx_dsp_rproc_vq_work()
>also takes rproc->lock, so cancel_work_sync() there waits for the work while the
>work waits for the lock -- a deadlock. The cancel must not run under
>rproc->lock,
>so it cannot live in free_mbox().
>
>[Critical] You're right that the unbind path can skip free_mbox() entirely:
>devm_rproc_add() is registered before devm_pm_runtime_enable(), so on unbind
>devres disables runtime PM before devm_rproc_remove() runs rproc_del() /
>__rproc_shutdown(). pm_runtime_put_sync() then returns -EACCES without calling
>imx_dsp_runtime_suspend(), so free_mbox() never runs -- the mailbox channels and
>rproc_work stay live while the rproc / imx_dsp_rproc are freed (UAF + the clk
>leak you note). So my one-liner does not even fix the unbind UAF it targeted.
>
>So this needs a teardown rework rather than a cancel in free_mbox(): e.g.
>reorder devm_pm_runtime_enable() before devm_rproc_add() so the suspend path
>(and mailbox free) actually runs on unbind, and drain rproc_work from a context
>that does not hold rproc->lock. Since that touches the driver's probe/teardown
>ordering, I'd rather not guess at the shape -- Mathieu, Frank, how would you
>prefer this handled? I'm happy to send a v2 once the preferred
>direction is clear.
>
>Thanks,
>Hyunjung
>
>2026년 11월 10일 (화) 오후 8:08, <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
>
>