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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added cleanup loop unconditionally dereferences `req_qinfo` and `op_reply_qinfo`, causing a kernel panic if their initial allocations fail.

Pre-existing issues:
- [High] Failure during reset leaves `num_op_req_q` non-zero but `req_qinfo` NULL, leading to a deferred kernel panic during driver cleanup.
- [High] The memory leak is not fully fixed: if a queue fails to create but the controller remains in a ready state, the partially allocated segments are leaked.
--

commit 9ead875c2a5e252d14e44cff611d464fadb0d332
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix memory leak on operational queue creation failure

This commit modifies the error handling path in mpi3mr_create_op_queues() 
to iterate over the number of queues and release the DMA memory segments 
before freeing the main queue information arrays.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 32aeae20481ec..88444f04fb6a6 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -2529,6 +2529,10 @@ static int mpi3mr_create_op_queues(struct mpi3mr_ioc *mrioc)
>  
>  	return retval;

[Severity: High]
This isn't a bug introduced by this patch, but does this function still leak
memory if a queue fails to create and the controller remains ready?

If queue creation fails inside the allocation loop:

mpi3mr_create_op_queues() {
        ...
        for (i = 0; i < num_queues; i++) {
                if (mpi3mr_create_op_reply_q(mrioc, i)) {
                        break;
                }
        ...
}

The loop breaks, but if the controller state is still MRIOC_STATE_READY,
the function skips out_failed entirely, updates mrioc->num_op_req_q = i,
and returns 0. The segments allocated for the failed queue at index i
are abandoned and never cleaned up.

>  out_failed:
> +	for (i = 0; i < num_queues; i++) {
> +		mpi3mr_free_op_req_q_segments(mrioc, i);
> +		mpi3mr_free_op_reply_q_segments(mrioc, i);
> +	}

[Severity: High]
Will this cause a null pointer dereference if the initial queue information
allocation fails?

If kzalloc() fails to allocate mrioc->req_qinfo or mrioc->op_reply_qinfo
at the start of the function, execution jumps directly to out_failed.
This loop will then execute and call mpi3mr_free_op_req_q_segments(),
which blindly dereferences the unallocated array:

mpi3mr_free_op_req_q_segments() {
        ...
        segments = mrioc->req_qinfo[q_idx].q_segments;
        ...
}

>  	kfree(mrioc->req_qinfo);
>  	mrioc->req_qinfo = NULL;

[Severity: High]
This is a pre-existing issue, but does freeing the array here lead to a
deferred crash if a controller reset fails?

If mpi3mr_create_op_queues() fails during a reset, mrioc->req_qinfo is
freed and set to null here, but mrioc->num_op_req_q retains its non-zero
value from before the reset.

Later, when the driver cleans up, mpi3mr_free_mem() loops up to the stale
num_op_req_q count and calls mpi3mr_free_op_req_q_segments(), which will
dereference the null req_qinfo array.

>

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