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

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

New issues:
- [High] The threaded IRQ polling loop will loop indefinitely if `pend_ios` becomes negative due to skipped corrupt descriptors.

Pre-existing issues:
- [High] The admin reply queue path reposts unvalidated DMA addresses to the hardware free queue when virtual address lookup fails, causing host memory corruption.
- [High] Missing release memory barrier when unlocking the `in_use` queue lock, allowing race conditions on weakly ordered architectures.
--

--- Patch [5]: [PATCH v4 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep ---
commit 19ca49ca3a484c6837fd5a670f8512d409c2678d
Author: Ranjan Kumar <[email protected]>

mpi3mr: Fix performance regression caused by extended IRQ poll sleep
    
This patch restores the IRQ poll sleep base to 2 us and introduces bounds
checking for request queue IDs to prevent infinite polling. It also adds
memory barriers in the admin and operational reply queue processing loops
and fixes a panic condition on malformed DMA reply addresses.

Link: https://sashiko.dev/#/patchset/[email protected]?part=5
Link: https://sashiko.dev/#/patchset/[email protected]?part=5
Link: https://sashiko.dev/#/patchset/[email protected]?part=5

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c

[ ... ]

> @@ -565,15 +578,33 @@ int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
>  	reply_desc = mpi3mr_get_reply_desc(op_reply_q, reply_ci);
>  	if ((le16_to_cpu(reply_desc->reply_flags) &
>  	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
> +		/* Recheck under in_use before releasing, to avoid a reclaim race */
> +		dma_rmb();
> +		if ((le16_to_cpu(reply_desc->reply_flags) &
> +		    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) == exp_phase)
> +			goto process_desc;
>  		atomic_dec(&op_reply_q->in_use);

[Severity: High]
This is a pre-existing issue, but is a release memory barrier missing when
unlocking the in_use queue lock?

Both here and at the end of mpi3mr_process_op_reply_q(), the lock state is
released via atomic_dec():

    op_reply_q->ci = reply_ci;
    op_reply_q->ephase = exp_phase;
    atomic_sub(threshold_comps, &op_reply_q->pend_ios);
    atomic_dec(&op_reply_q->in_use);

Since non-value-returning RMW atomic operations like atomic_dec() do not
imply memory barriers, could weakly ordered architectures reorder the writes
to op_reply_q->ci and op_reply_q->ephase after the in_use decrement?

Could this allow another CPU to acquire in_use but read stale values for ci
and ephase, causing it to double-process reply descriptors?

>  		return 0;
>  	}

[ ... ]

> @@ -744,11 +787,12 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
>  			num_op_reply +=
>  			    mpi3mr_process_op_reply_q(mrioc,
>  				intr_info->op_reply_q);
> +		if (!atomic_read(&intr_info->op_reply_q->pend_ios))
> +			break;

[Severity: High]
Can pend_ios become negative and cause this loop to run indefinitely?

If mpi3mr_process_op_reply_q() encounters a corrupted descriptor:

    if (unlikely(req_q_idx >= mrioc->num_op_req_q)) {
        ioc_err(mrioc, "Invalid request queue id %d, skipping reply\n",
            req_q_idx + 1);
        goto next_reply;
    }

It skips the reply but still increments threshold_comps, which is later
subtracted from pend_ios. If the descriptor did not correspond to a tracked
host submission, couldn't this decrement pend_ios below zero?

Since this exact equality check (!atomic_read) evaluates to false for
negative values, will the polling loop fail to break and wait indefinitely?

> -		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));
> +	} while (num_op_reply < mrioc->max_host_ios);
>  

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -3430,8 +3430,12 @@ void mpi3mr_process_op_reply_desc(struct mpi3mr_ioc *mrioc,
>  		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);
> +			ioc_err(mrioc, "scsi_reply is NULL, invalid reply_frame_address\n");
> +			/*
> +			 * Do not let the caller repost an address that
> +			 * failed virt-addr lookup back to the hardware.
> +			 */
> +			*reply_dma = 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the admin reply queue
path suffer from the same unvalidated DMA address issue fixed here for the
operational queue?

In mpi3mr_process_admin_reply_desc():

    def_reply = mpi3mr_get_reply_virt_addr(mrioc, *reply_dma);
    if (!def_reply)
        goto out;

If def_reply is NULL, the function jumps to the out label but fails to
clear *reply_dma. Then in mpi3mr_process_admin_reply_q():

    mpi3mr_process_admin_reply_desc(mrioc, reply_desc, &reply_dma);
    if (reply_dma)
        mpi3mr_repost_reply_buf(mrioc, reply_dma);

Will this cause the caller to unknowingly repost the unvalidated, non-zero
reply_dma back to the hardware free queue, leading to host memory
corruption?

>  			goto out;
>  		}

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