Re: [PATCH REPOST REPOST v6] remoteproc: imx_rproc: Invoke the callback directly

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

Pre-existing issues:
- [High] Missing RCU read lock around `idr_for_each()` traversal of `notifyids` leading to potential Use-After-Free.
--

--- Patch [1]: [PATCH REPOST REPOST v6] remoteproc: imx_rproc: Invoke the callback directly ---
commit d2ef12e25a967a32f7b650f21d89c9b5c07eccf3
Author: Sebastian Andrzej Siewior <[email protected]>

remoteproc: imx_rproc: Invoke the callback directly

This commit removes the workqueue in the imx_rproc driver and invokes
the mailbox callback directly. It takes advantage of the imx-mailbox
driver moving callback invocation into its preemptible threaded IRQ handler.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 7662ebd9d2f49..e18ae33a5cf85 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c

[ ... ]

> @@ -892,21 +889,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 this callback is executed in a
preemptible context (now a threaded IRQ instead of a workqueue), does this
idr_for_each() call need rcu_read_lock() protection?

If a concurrent rproc_shutdown() calls rproc_free_vring() and removes nodes
from the IDR via idr_remove(), could the threaded IRQ be preempted during
this idr_for_each() traversal, allowing an RCU grace period to elapse?

If that sequence occurs, could the IDR traversal access freed memory when
the handler resumes?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1