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

On Tue, Aug 18, 2026 at 11:41 PM <[email protected]> wrote:
>
> 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

Thanks for the review.

New issues:
- None. This patch specifically fixes an I/O block counter leak when
posting a task management request fails by decrementing
stgt_priv->block_io on the failure path.

Pre-existing issues:
- The stgt_priv lifecycle/UAF risk during concurrent target removal is
a pre-existing issue in mpi3mr_bsg_process_mpt_cmds() (also present on
the completion path)
  and will be addressed in a separate follow-up series.