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 `mpi=
3mr_process_op_reply_q`, leading to a kernel panic.
--

--- Patch [6]: [PATCH v4 06/10] mpi3mr: Fix memory leak on operational queu=
e 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 informati=
on
arrays but does not release DMA memory segments. This patch ensures partial=
ly
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/20260626114109.43685-1-ranjan.kumar@=
broadcom.com?part=3D6
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar=
@broadcom.com?part=3D6
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar=
@broadcom.com?part=3D6

> 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 *p=
rivdata)
>  {
>  	struct mpi3mr_intr_info *intr_info =3D privdata;
>  	struct mpi3mr_ioc *mrioc;
> +	struct op_reply_qinfo *op_reply_q;
>  	u16 midx;
>  	u32 num_op_reply =3D 0;
> =20
> -	if (!intr_info || !intr_info->op_reply_q)
> +	if (!intr_info)
> +		return IRQ_NONE;
> +
> +	op_reply_q =3D 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.

> =20
>  	mrioc =3D intr_info->mrioc;

[ ... ]

> @@ -2021,10 +2032,17 @@ static void mpi3mr_free_op_reply_q_segments(struc=
t mpi3mr_ioc *mrioc, u16 q_idx)
>  	u16 j;
>  	int size;
>  	struct segments *segments;
> +	u16 midx =3D REPLY_QUEUE_IDX_TO_MSIX_IDX(q_idx, mrioc->op_reply_q_offse=
t);
> =20
> -	segments =3D 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));
> +	}
> =20
>  	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.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260805110634.3466=
[email protected]?part=3D6