Re: [PATCH v3 10/10] remoteproc: imx_rproc: Invoke the callback directly

[sashiko-bot]

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

Pre-existing issues:
- [High] Unsafe lockless iteration of `rproc->notifyids` in preemptible context risks Use-After-Free of IDR nodes and `rvring` structures during concurrent device teardown.
- [High] If requesting the 'rx' mailbox channel fails, `priv->tx_ch` is freed but its pointer is not cleared, causing a subsequent start to mistakenly treat initialization as successful.
--

commit 6f600102625eeb04d37188290cefc55632750bc2
Author: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>

remoteproc: imx_rproc: Invoke the callback directly

The imx-mailbox driver moved the callback invocation into the threaded
IRQ handler. This means the callback is invoked in preemptible context,
so there is no need to schedule a kworker. This patch removes the workqueue
handling and invokes the callback directly.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0dd80e688b0ea..c97bc1c401655 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c

[ ... ]

> @@ -835,21 +832,11 @@ static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data)
>  	return 0;
>  }
>  
> -static void imx_rproc_vq_work(struct work_struct *work)
> -{
> -	struct imx_rproc *priv = container_of(work, struct imx_rproc,
> -					      rproc_work);
> -	struct rproc *rproc = priv->rproc;
> -
> -	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> -}
> -
>  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
>  {
>  	struct rproc *rproc = dev_get_drvdata(cl->dev);
> -	struct imx_rproc *priv = rproc->priv;
>  
> -	queue_work(priv->workqueue, &priv->rproc_work);
> +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);

[Severity: High]
This is a pre-existing issue, but since the iteration now runs directly
in the preemptible threaded IRQ handler, does the lack of RCU protection
risk a use-after-free?

During rproc_shutdown(), imx_rproc_stop() frees the mailbox channel, but
without disabling the IRQ, the callback can run concurrently with
rproc_resource_cleanup().

If the threaded IRQ handler is preempted here, could the RCU grace period
elapse while rproc_resource_cleanup() unregisters the device and removes
entries from the notifyids IDR?

rproc_vq_interrupt() also performs a lockless idr_find() lookup on the same
notifyids IDR:

drivers/remoteproc/remoteproc_virtio.c:rproc_vq_interrupt() {
    ...
    rvring = idr_find(&rproc->notifyids, notifyid);
    ...
}

Would wrapping the idr_for_each() call with rcu_read_lock() and
rcu_read_unlock() be necessary to safely iterate the IDR nodes and prevent
use-after-free of the rvring structures?

>  }
>  
>  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the error
handling within imx_rproc_xtr_mbox_init().

If requesting the rx mailbox channel fails, the code frees the tx channel but
does not clear the pointer:

drivers/remoteproc/imx_rproc.c:imx_rproc_xtr_mbox_init() {
    ...
    priv->rx_ch = mbox_request_channel_byname(cl, "rx");
    if (IS_ERR(priv->rx_ch)) {
        mbox_free_channel(priv->tx_ch);
        return dev_err_probe(...);
    }
    ...
}

Because priv->tx_ch is not set to NULL, and priv->rx_ch contains an ERR_PTR,
the early exit check at the top of the function evaluates to true on any
subsequent call:

    if (priv->tx_ch && priv->rx_ch)
        return 0;

If a user retries starting the remoteproc via sysfs, this will incorrectly
return 0 (success) with a dangling priv->tx_ch pointer. When the remoteproc
later sends a kick, imx_rproc_kick() will use this dangling pointer, which
can cause a use-after-free crash.

Could we set priv->tx_ch to NULL after freeing it to prevent this sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-imx_mbox_rproc-v3-0-77948112defc@xxxxxxxxxxxxx?part=10