Re: [PATCH 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure

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

Pre-existing issues:
- [Critical] Dereferencing `stgt_priv` on the error and timeout/completion paths leads to a Use-After-Free if the SCSI target is concurrently removed.
--

--- Patch [3]: [PATCH 03/17] mpi3mr: Fix I/O block counter leak on admin request post failure ---
commit 1533f8fc86e8eb7e7a5042ad5aab564671eb7b8c
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix I/O block counter leak on admin request post failure

This commit addresses an issue where the per-device I/O block counter is not
decremented if posting a task management request fails. It lowers the counter
on the failure path to prevent I/O from remaining blocked.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1cd1c38dcd565..a4fe10a05cb3a 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2782,6 +2782,8 @@ static long mpi3mr_bsg_process_mpt_cmds(struct bsg_job *job)
>  		dprint_bsg_err(mrioc,
>  		    "%s: posting bsg request is failed\n", __func__);
>  		rval = -EAGAIN;
> +		if (block_io && stgt_priv)
> +			atomic_dec(&stgt_priv->block_io);

[Severity: Critical]
This is a pre-existing issue, but does this add another path that can trigger
a use-after-free if the SCSI target is concurrently removed?

Looking at mpi3mr_bsg_process_mpt_cmds(), stgt_priv is fetched from
tgtdev->starget->hostdata, but the tgtdev reference is immediately dropped
before posting the bsg request:

	if (block_io) {
		tgtdev = mpi3mr_get_tgtdev_by_handle(mrioc, dev_handle);
		if (tgtdev) {
			if (tgtdev->starget && tgtdev->starget->hostdata) {
				stgt_priv = (struct mpi3mr_stgt_priv_data *)
				    tgtdev->starget->hostdata;
				atomic_inc(&stgt_priv->block_io);
			}
			mpi3mr_tgtdev_put(tgtdev);
		}
	}

Because there are no references or locks held, if the device is concurrently
removed (e.g., via hot-unplug), the SCSI midlayer can invoke
mpi3mr_target_destroy(), which unconditionally frees the hostdata memory:

mpi3mr_target_destroy() {
	...
	kfree(starget->hostdata);
	...
}

Since the original completion path also dereferences stgt_priv after sleeping
in wait_for_completion_timeout(), both the timeout/completion path and this
newly added error path seem to potentially call
atomic_dec(&stgt_priv->block_io) on freed memory.

>  		goto out_unlock;
>  	}
>  	wait_for_completion_timeout(&mrioc->bsg_cmds.done,

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