Re: [PATCH v4] remoteproc: imx_rproc: Invoke the callback directly

Re: [PATCH v4] 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] Missing RCU read lock around idr_for_each() and idr_find() in preemptible context.
- [High] Improper devm cleanup order leaves mailbox active during driver teardown, causing Use-After-Free.
--

commit 24cd624015487c1ba761a43443263ce588b90c10
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

remoteproc: imx_rproc: Invoke the callback directly

This commit removes the workqueue used for callback invocation in the
imx_rproc driver. The callback is now invoked directly from the threaded
IRQ handler since it runs in a preemptible context.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 745ce52cd8226..a1f2b76ec8187 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
[ ... ]
>  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
>  {
>  	struct rproc *rproc = dev_get_drvdata(cl->dev);
>  
> -	queue_work(priv->workqueue, &priv->rproc_work);
> +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless iteration
over the notifyids IDR lack RCU protection?

Because imx_rproc_rx_callback() now runs in a preemptible context (threaded
IRQ handler), it iterates over the rproc->notifyids IDR using idr_for_each()
without the rcu_read_lock(). 

The IDR API requires lockless readers to hold rcu_read_lock() to prevent radix
tree nodes from being freed during traversal. If a concurrent modification
like idr_remove() occurs during teardown, the RCU grace period could expire,
leading to a use-after-free on the IDR internal nodes.

>  }
>  
>  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
[ ... ]
> @@ -1273,17 +1253,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		priv->ops = dcfg->ops;
>  
>  	dev_set_drvdata(dev, rproc);
> -	priv->workqueue = create_workqueue(dev_name(dev));
> -	if (!priv->workqueue) {
> -		dev_err(dev, "cannot create workqueue\n");
> -		return -ENOMEM;
> -	}
> -
> -	ret = devm_add_action_or_reset(dev, imx_rproc_destroy_workqueue, priv->workqueue);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to add devm destroy workqueue action\n");
> -
> -	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
>  
>  	ret = imx_rproc_xtr_mbox_init(rproc, true);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but could the devm cleanup order in
imx_rproc_probe() lead to a use-after-free during driver teardown?

Earlier in imx_rproc_probe(), the mailbox cleanup action imx_rproc_free_mbox
is registered via devm_add_action_or_reset() before devm_rproc_add() is
called.

Because devm actions execute in LIFO order, devm_rproc_remove() executes
first, which tears down the remote processor and frees virtio subdevices and
their IDR entries.

This leaves a dangerous race window during driver unbind where the mailbox is
still fully active while the core remoteproc data structures are being torn
down. If a mailbox interrupt fires during this window, the RX handler will
access the notifyids IDR and rvring structures while they are concurrently
being modified or freed by rproc_del().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-imx_mbox_rproc-v4-1-67b10560a861@linutronix.de?part=1

Re: [PATCH v4] remoteproc: imx_rproc: Invoke the callback directly

[Sebastian Andrzej Siewior]

On 2026-07-03 08:45:21 [+0000], sashiko-bot@kernel.org wrote:
> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > index 745ce52cd8226..a1f2b76ec8187 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> [ ... ]
> >  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> >  {
> >  	struct rproc *rproc = dev_get_drvdata(cl->dev);
> >  
> > -	queue_work(priv->workqueue, &priv->rproc_work);
> > +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this lockless iteration
> over the notifyids IDR lack RCU protection?
> 
> Because imx_rproc_rx_callback() now runs in a preemptible context (threaded
> IRQ handler), it iterates over the rproc->notifyids IDR using idr_for_each()
> without the rcu_read_lock(). 
> 
> The IDR API requires lockless readers to hold rcu_read_lock() to prevent radix
> tree nodes from being freed during traversal. If a concurrent modification
> like idr_remove() occurs during teardown, the RCU grace period could expire,
> leading to a use-after-free on the IDR internal nodes.

If I see this right there is no RCU involved. First the interrupt gets
removed (via free_irq()) which ensures that it is no longer active and
then the channel is removed.

> >  }
> >  
> >  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
> [ ... ]
> > @@ -1273,17 +1253,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
> >  		priv->ops = dcfg->ops;
> >  
> >  	dev_set_drvdata(dev, rproc);
> > -	priv->workqueue = create_workqueue(dev_name(dev));
> > -	if (!priv->workqueue) {
> > -		dev_err(dev, "cannot create workqueue\n");
> > -		return -ENOMEM;
> > -	}
> > -
> > -	ret = devm_add_action_or_reset(dev, imx_rproc_destroy_workqueue, priv->workqueue);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret, "Failed to add devm destroy workqueue action\n");
> > -
> > -	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
> >  
> >  	ret = imx_rproc_xtr_mbox_init(rproc, true);
> >  	if (ret)
> 
> [Severity: High]
> This is a pre-existing issue, but could the devm cleanup order in
> imx_rproc_probe() lead to a use-after-free during driver teardown?
> 
> Earlier in imx_rproc_probe(), the mailbox cleanup action imx_rproc_free_mbox
> is registered via devm_add_action_or_reset() before devm_rproc_add() is
> called.
> 
> Because devm actions execute in LIFO order, devm_rproc_remove() executes
> first, which tears down the remote processor and frees virtio subdevices and
> their IDR entries.
> 
> This leaves a dangerous race window during driver unbind where the mailbox is
> still fully active while the core remoteproc data structures are being torn
> down. If a mailbox interrupt fires during this window, the RX handler will
> access the notifyids IDR and rvring structures while they are concurrently
> being modified or freed by rproc_del().

The notifyids IDR is removed by devm_rproc_free() not by
devm_rproc_remove(). This should remove the channel which removes IRQ.
This looks fine to me.

Sebastian