Re: [PATCH RFC v2 09/11] remoteproc: imx_rproc: always use non-blocking mailboxes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] remoteproc: imx_rproc: memory leak of mmsg in imx_rproc_kick() on mbox abort
--

commit 059fe2437ad2cd1bd7152b14297e2a685bc5d803
Author: Francesco Valla <francesco@valla.it>

remoteproc: imx_rproc: always use non-blocking mailboxes

This commit switches the i.MX remoteproc mailbox client to non-blocking
mode, dynamically allocating and freeing kick messages via the tx_done
callback, and removes the sys_off handler that previously reopened the
mailbox in non-blocking mode during system shutdown and restart.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 581bb07f5893f..a792b9ca5edd1 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c

[ ... ]

> @@ -696,28 +696,45 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
>  {
>  	struct imx_rproc *priv = rproc->priv;
>  	int err;
> -	__u32 mmsg;
> +	__u32 *mmsg;
>  
>  	if (!priv->tx_ch) {
>  		dev_err(priv->dev, "No initialized mbox tx channel\n");
>  		return;
>  	}
>  
> +	mmsg = kzalloc_obj(*mmsg, GFP_ATOMIC);
> +	if (!mmsg) {
> +		dev_err_ratelimited(priv->dev, "cannot allocate kick message\n");
> +		return;
> +	}
> +
>  	/*
>  	 * Send the index of the triggered virtqueue as the mu payload.
>  	 * Let remote processor know which virtqueue is used.
>  	 */
> -	mmsg = vqid << 16;
> +	*mmsg = vqid << 16;
>  
> -	err = mbox_send_message(priv->tx_ch, (void *)&mmsg);
> -	if (err < 0)
> +	err = mbox_send_message(priv->tx_ch, (void *)mmsg);
> +	if (err < 0) {
>  		dev_err(priv->dev, "%s: failed (%d, err:%d)\n",
>  			__func__, vqid, err);
> +		kfree(mmsg);
> +	}
> +}
> +
> +static void imx_rproc_kick_tx_done(struct mbox_client *cl, void *mmsg, int r)
> +{
> +	if (r != 0)
> +		dev_err_ratelimited(cl->dev, "failed to kick vqid %u: %d\n",
> +				    *(__u32 *)mmsg >> 16, r);
> +
> +	kfree(mmsg);
>  }

[Severity: High]
Does this leak mmsg when the mailbox channel is freed or when transmission
does not complete with a tx_done callback?

In imx_rproc_kick(), once mbox_send_message() returns a non-negative ring
buffer index, freeing mmsg relies solely on imx_rproc_kick_tx_done() being
called from tx_tick().

However, there are several paths where imx_rproc_kick_tx_done() is never
invoked for a queued or active mmsg:

1. When imx_rproc_free_mbox() calls mbox_free_channel(priv->tx_ch) during
stop, detach, crash recovery, or driver teardown,
mbox_free_channel()->mbox_clean_and_put_channel() aborts the active request
and all queued requests without invoking cl->tx_done:

drivers/mailbox/mailbox.c:mbox_clean_and_put_channel() {
	/* The queued TX requests are simply aborted, no callbacks are made */
	scoped_guard(spinlock_irqsave, &chan->lock) {
		chan->cl = NULL;
		chan->active_req = MBOX_NO_MSG;
		if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
			chan->txdone_method = MBOX_TXDONE_BY_POLL;
	}
...
}

Any in-flight kick in chan->active_req (such as kicks sent by virtio
subdevices during rproc_stop_subdevices() right before imx_rproc_stop()) or
queued kicks in chan->msg_data[] (for example, when the remote processor
crashes or stops reading the MU register) will be leaked.

2. In mbox_send_message()->msg_submit(), if chan->mbox->ops->send_data()
returns an error, chan->active_req is not set and mmsg remains in
chan->msg_data[] without tx_done being called, while mbox_send_message()
still returns the non-negative slot index t to imx_rproc_kick().

3. For IMX_MU_TYPE_TXDB_V2 channels, imx_mu_xlate() configures
txdone_method as MBOX_TXDONE_BY_ACK, while imx_rproc_xtr_mbox_init() sets
cl->knows_txdone = false and never calls mbox_client_txdone(), so tx_tick()
and imx_rproc_kick_tx_done() are never called.

Since mbox_send_message() only stores the void * pointer in
chan->msg_data[] and passes it to send_data(), could vqid (or vqid << 16)
be encoded directly in the pointer via uintptr_t or stored in a preallocated
per-vq/per-device array instead of dynamically allocating mmsg on every
kick?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-remoteproc_virtio_map-v2-0-26b300e01c62@valla.it?part=9