Re: [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The threaded IRQ handler `mpi3mr_isr_poll` may exit early without re-enabling the IRQ, leaving the interrupt line permanently disabled.
- [High] Polling paths pass a potentially NULL `op_reply_q` pointer to `mpi3mr_process_op_reply_q`, leading to a kernel panic.
--

--- Patch [6]: [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queue creation failure ---
commit 3a3eabe2f05c3500da7a32007ea4df206d106e35
Author: Ranjan Kumar <[email protected]>

mpi3mr: Fix memory leak on operational queue creation failure

When operational queue creation fails, the error path frees queue information
arrays but does not release DMA memory segments. This patch ensures partially
allocated segments are freed immediately. It also attempts to resolve races
in the queue segment free/alloc paths by clearing the queue pointer before
freeing.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=6
Closes: https://sashiko.dev/#/patchset/[email protected]?part=6
Closes: https://sashiko.dev/#/patchset/[email protected]?part=6

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index e469507045dd6..880c07bdc9738 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c

[ ... ]

> @@ -767,10 +770,15 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
>  {
>  	struct mpi3mr_intr_info *intr_info = privdata;
>  	struct mpi3mr_ioc *mrioc;
> +	struct op_reply_qinfo *op_reply_q;
>  	u16 midx;
>  	u32 num_op_reply = 0;
>  
> -	if (!intr_info || !intr_info->op_reply_q)
> +	if (!intr_info)
> +		return IRQ_NONE;
> +
> +	op_reply_q = READ_ONCE(intr_info->op_reply_q);
> +	if (!op_reply_q)
>  		return IRQ_NONE;

[Severity: High]
Does returning early here leave the interrupt permanently masked?

If a concurrent queue deletion (e.g., from a reset) sets op_reply_q to NULL
right after mpi3mr_isr() reads it as non-NULL, the hardirq handler will
disable the IRQ and schedule this threaded handler:

mpi3mr_isr()
   disable_irq_nosync(intr_info->os_irq);
   return IRQ_WAKE_THREAD;

When mpi3mr_isr_poll() runs, it will read op_reply_q as NULL and return
IRQ_NONE without calling enable_irq(intr_info->os_irq). Future IOs on the
operational queue associated with this vector would hang.

>  
>  	mrioc = intr_info->mrioc;

[ ... ]

> @@ -2021,10 +2032,17 @@ static void mpi3mr_free_op_reply_q_segments(struct mpi3mr_ioc *mrioc, u16 q_idx)
>  	u16 j;
>  	int size;
>  	struct segments *segments;
> +	u16 midx = REPLY_QUEUE_IDX_TO_MSIX_IDX(q_idx, mrioc->op_reply_q_offset);
>  
> -	segments = mrioc->op_reply_qinfo[q_idx].q_segments;
> -	if (!segments)
> -		return;
> +	/*
> +	 * Stop the ISR/poll thread from picking up this queue before its
> +	 * segments are freed below, and wait for any in-flight handler
> +	 * that already has the old pointer to finish using it.
> +	 */
> +	if (midx < mrioc->intr_info_count) {
> +		WRITE_ONCE(mrioc->intr_info[midx].op_reply_q, NULL);
> +		synchronize_irq(pci_irq_vector(mrioc->pdev, midx));
> +	}
>  
>  	if (mrioc->enable_segqueue) {

[Severity: High]
Could clearing op_reply_q to NULL here cause a NULL pointer dereference
in polling paths?

Polling paths read the op_reply_q pointer from the intr_info array and pass
it to mpi3mr_process_op_reply_q() without a NULL check:

mpi3mr_op_request_post()
    mpi3mr_process_op_reply_q(mrioc, mrioc->intr_info[midx].op_reply_q);

mpi3mr_poll_pend_io_completions()
    mpi3mr_process_op_reply_q(mrioc, mrioc->intr_info[i].op_reply_q);

If a concurrent controller reset frees the queues and sets this pointer
to NULL, mpi3mr_process_op_reply_q() will immediately dereference it.

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