Re: [PATCH 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue

On Tue, Aug 18, 2026 at 11:53 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Use-After-Free of firmware event (`fwevt`) in `mpi3mr_cleanup_fwevt_list` during driver teardown.
> --
>
> --- Patch [16]: [PATCH 16/17] mpi3mr: Fix use-after-free of the firmware event workqueue ---
> commit d1068d82a1a0b06d1a9647481bac5d43ff5f1ebc
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix use-after-free of the firmware event workqueue
>
> This commit fixes a use-after-free issue regarding the firmware event
> workqueue in the mpi3mr driver. It ensures the worker thread pointer is
> checked while holding the event lock to prevent a race condition with the
> workqueue being destroyed.
>
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > index 510c990d42d1b..4a72b3d89c294 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -233,10 +233,11 @@ static void mpi3mr_fwevt_add_to_list(struct mpi3mr_ioc *mrioc,
> >  {
> >       unsigned long flags;
> >
> > -     if (!mrioc->fwevt_worker_thread)
> > -             return;
> > -
> >       spin_lock_irqsave(&mrioc->fwevt_lock, flags);
> > +     if (!mrioc->fwevt_worker_thread) {
> > +             spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
> > +             return;
> > +     }
> >       /* get fwevt reference count while adding it to fwevt_list */
> >       mpi3mr_fwevt_get(fwevt);
> >       INIT_LIST_HEAD(&fwevt->list);
>
> [Severity: High]
> This is a pre-existing issue, but while reviewing the firmware event
> lifecycle, I noticed another potential use-after-free during driver teardown.
>
> In mpi3mr_dequeue_fwevt(), the list reference is dropped before returning
> the pointer:
>
> drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_dequeue_fwevt() {
>     ...
>                 fwevt = list_first_entry(&mrioc->fwevt_list,
>                     struct mpi3mr_fwevt, list);
>                 list_del_init(&fwevt->list);
>                 /*
>                  * Put fwevt reference count after
>                  * removing it from fwevt_list
>                  */
>                 mpi3mr_fwevt_put(fwevt);
>         }
>         spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
>
>         return fwevt;
> }
>
> Then in mpi3mr_cleanup_fwevt_list(), we pass this unreferenced pointer to
> cancel_work_sync():
>
> drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
>     ...
>         while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
>                 mpi3mr_cancel_work(fwevt);
>     ...
> }
>
> Could the background worker thread (mpi3mr_fwevt_worker) concurrently finish
> executing and free the fwevt structure, causing cancel_work_sync() inside
> mpi3mr_cancel_work() to access freed memory?
>
> Additionally, lockless access to mrioc->current_event in
> mpi3mr_cleanup_fwevt_list() could similarly read a concurrently freed event if
> the worker thread finishes in the middle of cleanup:
>
> drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_cleanup_fwevt_list() {
>     ...
>         if (mrioc->current_event) {
>                 fwevt = mrioc->current_event;
>         ...
>                 mpi3mr_cancel_work(fwevt);
>         }
> }
>
> Does this code need to hold a reference to fwevt while calling
> mpi3mr_cancel_work() to prevent the worker thread from freeing it?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16

Thanks for the review.

New issues:
- None. This patch specifically ensures that
mrioc->fwevt_worker_thread is checked while holding fwevt_lock to
prevent a race with workqueue destruction.

Pre-existing issues:
- The teardown reference counting observation in
mpi3mr_cleanup_fwevt_list() and mpi3mr_dequeue_fwevt() is a
pre-existing issue in the driver and will be
  addressed in a separate follow-up patch series.