Re: [PATCH v2 2/3] bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write

[sashiko-bot@kernel.org]

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

New issues:
- [High] Draining async transfers *after* sending a disconnect notification causes out-of-order successful callbacks to be delivered to a disconnected client.
- [High] Controller-wide `flush_async()` called inside a per-channel loop disrupts active channels and causes livelocks.

Pre-existing issues:
- [Critical] The `MHI_PKT_TYPE_RESET_CHAN_CMD` handler frees the transfer ring without flushing in-flight async DMA operations.
- [High] Lockless TOCTOU race on `mhi_chan->xfer_cb` in completion handlers leads to a Use-After-Free.
- [High] Null pointer dereference on `mhi_chan->mhi_dev` in `mhi_ep_process_ch_ring` during controller teardown.
--

--- Patch [2]: [PATCH v2 2/3] bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write ---
commit 2e23a5ef008c4098d55a3f07d062a8941cc90ccd
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Subject: bus: mhi: ep: Add mhi_cntrl->flush_async() callback to flush the async read/write

This patch introduces a flush_async() callback to the MHI EP controller and
invokes it during device removal. It aims to prevent use-after-free issues
by ensuring that any in-flight asynchronous DMA operations are completed
before the channel's transfer callback is unregistered.

> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c

[ ... ]

> @@ -1636,6 +1637,12 @@ static void mhi_ep_remove(struct device *dev)
>  		}
>  
>  		mhi_chan->state = MHI_CH_STATE_DISABLED;
> +		mutex_unlock(&mhi_chan->lock);
> +
> +		if (mhi_cntrl->flush_async)
> +			mhi_cntrl->flush_async(mhi_cntrl);
> +

[Severity: High]
Does this sequence result in successful callbacks being delivered after the
client has already been notified of disconnection?

When mhi_ep_remove() sends -ENOTCONN to mhi_chan->xfer_cb(), it signals the
client to free its context. If we then drop the lock and block on
flush_async() while xfer_cb is still registered, pending DMA completions
can invoke mhi_ep_read_completion():

mhi_ep_read_completion()
    if (mhi_chan->xfer_cb) {
        ...
        mhi_chan->xfer_cb(mhi_dev, &result);
    }

Can this lead to the client driver receiving a success callback after it has
already freed its resources in response to the -ENOTCONN notification?

[Severity: High]
Can calling a controller-wide flush operation inside a per-channel loop
disrupt other active channels or cause a livelock?

The code loops over the device's channels (UL then DL). When disabling the UL
channel, it calls mhi_cntrl->flush_async(). Since flush_async() operates on
the controller context and MHI EP architectures typically share RX/TX DMA
channels across the entire controller, the DL channel might still be active
and queuing new transfers.

Could waiting for all controller DMAs to flush while other channels are active
cause a livelock or drain traffic inappropriately?

> +		mutex_lock(&mhi_chan->lock);
>  		mhi_chan->xfer_cb = NULL;
>  		mutex_unlock(&mhi_chan->lock);
>  	}

[Severity: High]
This is a pre-existing issue, but does conditionally skipping the flush
leave the fallback path unprotected against concurrent teardown?

If mhi_cntrl->flush_async is NULL, the flush is skipped. However, DMA
completion handlers evaluate the callback pointer locklessly:

mhi_ep_read_completion()
    if (mhi_chan->xfer_cb) {
        ...
        mhi_chan->xfer_cb(mhi_dev, &result);
    }

If the thread is preempted here, mhi_ep_remove() can run, set
mhi_chan->xfer_cb = NULL, and free the device. When the handler resumes, it
could invoke the stale callback pointer. Does the lockless read of xfer_cb
lack the necessary synchronization?

While reviewing this file, I also noticed a couple of pre-existing issues
elsewhere in the code:

[Severity: Critical]
This is a pre-existing issue, but does resetting the channel ring without
flushing async DMAs lead to a NULL pointer dereference?

When the host sends a reset command, mhi_ep_process_ctrl_interrupt() calls
mhi_ep_ring_reset() which frees ring->ring_cache. Because this path does not
cancel in-flight async DMAs, a pending DMA completion handler might execute
shortly after:

mhi_ep_read_completion()
    struct mhi_ring_element *el = &ring->ring_cache[ring->rd_offset];

Can this result in a crash if the completion handler dereferences the cleared
ring_cache?

[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer
dereference during controller teardown?

In mhi_ep_abort_transfer(), client devices are destroyed using
device_for_each_child(), which sets mhi_chan->mhi_dev = NULL. Because the
channel rings are not stopped until later, a doorbell interrupt can fire and
queue mhi_ep_ch_ring_worker().

The worker calls mhi_ep_process_ch_ring() which checks for a missing client
driver:

mhi_ep_process_ch_ring()
    if (!mhi_chan->xfer_cb) {
        dev_err(&mhi_chan->mhi_dev->dev, "Client driver not available\n");

Since mhi_dev is now NULL, can this dev_err() call trigger a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-mhi-ep-flush-v2-0-b6a9db011e85@oss.qualcomm.com?part=2