Re: [PATCH v2 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver extracts `request_queue_id` from the hardware-populated descriptor and uses it as an array index without bounds checking, leading to out-of-bounds memory corruption.
- [High] A TOCTOU race in `mpi3mr_process_op_reply_q` can cause operational reply descriptors to be missed, leading to IO stalls and timeouts.
- [High] The threaded IRQ polling routine `mpi3mr_isr_poll` enters an unbounded busy-sleep loop waiting for slow hardware IOs to complete, causing severe CPU consumption.
- [High] An invalid `reply_frame_address` provided in a DMA reply descriptor triggers a direct kernel panic.
--

--- Patch [5]: [PATCH v2 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep ---
commit 6c88e5d170a116451ea90d4d4dc8870ab7372f57
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix performance regression caused by extended IRQ poll sleep

This commit reverts a previous change to the threaded IRQ poll sleep
interval, restoring it to 2-20 us to fix a 7% throughput regression
on NVMe direct-attached RAID10 configurations. It also adds missing
dma_rmb() barriers in the admin and operational reply queue loops
to ensure phase bit checks complete before payloads are read.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=5

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 434b66f7b502d..2f787fa36ffdb 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
[ ... ]
> @@ -568,6 +581,12 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
>  		return 0;
>  	}
>  
> +	/*
> +	 * Ensure that the descriptor payload is read only after
> +	 * the phase bit check is complete.
> +	 */
> +	dma_rmb();
> +
>  	do {
>  		if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
>  			break;

[Severity: Critical]
This is a pre-existing issue, but does this code bounds check the queue ID
before using it as an array index?

In mpi3mr_process_op_reply_q(), the driver computes req_q_idx from
device-supplied DMA memory:

		req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
		op_req_q = &mrioc->req_qinfo[req_q_idx];

		WRITE_ONCE(op_req_q->ci, le16_to_cpu(reply_desc->request_queue_ci));

If the hardware provides an invalid queue ID (such as 0, underflowing to
65535, or a value larger than mrioc->num_op_req_q), could this lead to an
out-of-bounds memory write to the host kernel heap?


[Severity: High]
This isn't a bug introduced by this patch, but does
mpi3mr_process_op_reply_desc() safely handle malformed DMA reply addresses?

If a malicious or faulty PCIe device sends an invalid reply_frame_address,
mpi3mr_get_reply_virt_addr() will return NULL, triggering a panic:

		scsi_reply = mpi3mr_get_reply_virt_addr(mrioc,
		    *reply_dma);
		if (!scsi_reply) {
			panic("%s: scsi_reply is NULL, this shouldn't happen\n",
			    mrioc->name);
			goto out;
		}

Could a compromised device or hypervisor repeatedly send crafted DMA frames
containing invalid addresses to intentionally crash the host system?

[ ... ]
> @@ -594,6 +613,12 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
>  		if ((le16_to_cpu(reply_desc->reply_flags) &
>  		    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase)
>  			break;
> +
> +		/*
> +		 * Ensure that the descriptor payload is read only after
> +		 * the phase bit check is complete.
> +		 */
> +		dma_rmb();

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition when
finishing descriptor processing in mpi3mr_process_op_reply_q()?

If the active thread breaks out of the loop after checking the phase bit,
but before it executes:

	atomic_dec(&op_reply_q->in_use);

And the hardware writes a new descriptor and triggers an MSI-X interrupt in
that tiny window, the interrupt handler will fail the atomic_add_unless()
check:

	if (!atomic_add_unless(&op_reply_q->in_use, 1, 1))
		return 0;

Will this permanently drop the interrupt without re-checking for newly
arrived descriptors, potentially stalling the queue until another unrelated
command completes?

[ ... ]
> @@ -744,7 +769,7 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
>  			    mpi3mr_process_op_reply_q(mrioc,
>  				intr_info->op_reply_q);
>  
> -		usleep_range(MPI3MR_IRQ_POLL_SLEEP, MPI3MR_IRQ_POLL_SLEEP + 1);
> +		usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
>  
>  	} while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
>  	    (num_op_reply < mrioc->max_host_ios));

[Severity: High]
This isn't a bug introduced by this patch, but does this threaded IRQ polling
loop busy-wait unboundedly for slow hardware IOs?

When pend_ios exceeds 8, mpi3mr_isr_poll is awoken. The thread loops and
continuously executes usleep_range():

	do {
		...
		usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);

	} while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
	    (num_op_reply < mrioc->max_host_ios));

If multiple slow IOs are pending, will this keep the CPU occupied with
thousands of context switches, rather than going to sleep and waiting for
the actual hardware interrupt when the async IO finishes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=5