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

[Sebastian Andrzej Siewior]

On 2026-07-03 08:45:21 [+0000], sashiko-bot@xxxxxxxxxx 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