Re: [PATCH v2 9/9] remoteproc: imx_rproc: Invoke the callback directly

[sashiko-bot@kernel.org]

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()` executing in a preemptible context, leading to potential use-after-free.
- [High] Error paths in mailbox initialization leave `ERR_PTR` or dangling pointers in the channel pointers, leading to a kernel panic during cleanup.
--

commit 4d44a5db67113dd38271bbfd5c93e988c06b7cf5
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

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
and there is no need to schedule the kworker for the
imx_rproc_notified_idr_cb() invocation.

> 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 does this idr_for_each() loop require RCU
read protection in drivers/remoteproc/imx_rproc.c?

Since imx_rproc_rx_callback() now runs in a preemptible threaded IRQ handler
(and previously ran in a preemptible kworker), it executes without holding an
RCU read lock.

If a concurrent unbind of the rproc-virtio driver removes an IDR entry via:

rproc_virtio_remove()
  rproc_free_vring()
    idr_remove()

The radix tree nodes could be scheduled for delayed RCU freeing. If the
threaded IRQ handler is preempted during the idr_for_each() iteration and the
RCU grace period expires, could the iteration traverse freed memory upon
resuming and cause a use-after-free crash?

>  }

[ ... ]

> @@ -1249,17 +1229,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 can the mailbox initialization error paths in
imx_rproc_xtr_mbox_init() lead to a use-after-free or panic during cleanup?

If mbox_request_channel_byname() fails for the "tx" channel, it returns an
error but assigns an ERR_PTR to priv->tx_ch:

drivers/remoteproc/imx_rproc.c:imx_rproc_xtr_mbox_init() {
    ...
	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
	if (IS_ERR(priv->tx_ch))
		return dev_err_probe(cl->dev, PTR_ERR(priv->tx_ch),
				     "failed to request tx mailbox channel\n");
    ...
}

If the "rx" channel fails, it leaves the ERR_PTR in priv->rx_ch and calls
mbox_free_channel(priv->tx_ch) but does not reset priv->tx_ch to NULL,
leaving it dangling:

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(cl->dev, PTR_ERR(priv->rx_ch),
				     "failed to request rx mailbox channel\n");
	}
    ...
}

When the device is later unbound, the devm cleanup action imx_rproc_free_mbox()
runs. It checks if the pointers are true, which they will be for an ERR_PTR
or dangling pointer:

drivers/remoteproc/imx_rproc.c:imx_rproc_free_mbox() {
    ...
	if (priv->tx_ch) {
		mbox_free_channel(priv->tx_ch);
		priv->tx_ch = NULL;
	}

	if (priv->rx_ch) {
		mbox_free_channel(priv->rx_ch);
		priv->rx_ch = NULL;
	}
    ...
}

Since mbox_free_channel() does not check IS_ERR(), will it attempt to
dereference these invalid pointers and cause a kernel panic or a double-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-imx_mbox_rproc-v2-0-a0059dc3b69a@linutronix.de?part=9