Re: [PATCH 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery

On Wed, Aug 19, 2026 at 12:02 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [High] Calling `mpi3mr_cleanup_fwevt_list` during PCI error recovery causes a deadlock due to a race with the fwevt worker thread calling SML functions.
> - [High] Setting `stop_drv_processing = 1` during PCI channel freeze breaks transparent I/O recovery by permanently failing commands with `DID_NO_CONNECT`.
>
> Pre-existing issues:
> - [High] Unprotected and unreferenced access to `mrioc->current_event` and `fwevt` in `mpi3mr_cleanup_fwevt_list` leads to a Use-After-Free, exacerbated by the patch executing it asynchronously during PCI error recovery.
> - [High] Skipping `cancel_work_sync` for events blocked in the SCSI mid-layer (SML) leaves the worker thread running, allowing access to unmapped registers on suspend.
> - [High] Missing memory barriers between `stop_drv_processing` and `mrioc->current_event` allows the worker thread to bypass the abort check and access unmapped memory.
> --
>
> --- Patch [17]: [PATCH 17/17] mpi3mr: Fix NULL pointer dereference on PCI error recovery ---
> commit 21853a66ae88d53ee80d24101e9ce79158e66a4e
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix NULL pointer dereference on PCI error recovery
>
> On a frozen PCI channel the driver unmaps the register window and frees
> the interrupts, but leaves the firmware event workqueue running. An
> event handler that is already in flight can still reach the register
> window after it has been unmapped. Stop event processing and drain the
> queue first, and start it again when the channel resumes.
>
> > --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > @@ -5972,6 +5972,8 @@ mpi3mr_pcierr_error_detected(struct pci_dev *pdev, pci_channel_state_t state)
> >                       return PCI_ERS_RESULT_DISCONNECT;
> >               }
> >
> > +             mrioc->stop_drv_processing = 1;
>
> [Severity: High]
> Does setting stop_drv_processing to 1 here break transparent I/O recovery?
>
> If mrioc->stop_drv_processing evaluates to true in mpi3mr_qcmd(), it
> preempts the block_on_pci_err check. Instead of returning
> SCSI_MLQUEUE_HOST_BUSY to allow the midlayer to safely pause and retry
> I/Os once the PCI channel recovers, it causes all incoming queued
> read/write commands to immediately fail with DID_NO_CONNECT:
>
> mpi3mr_qcmd() {
>     ...
>     if (mrioc->stop_drv_processing && !(mpi3mr_allow_scmd_to_fw(scmd))) {
>         scmd->result = DID_NO_CONNECT << 16;
>         scsi_done(scmd);
>         goto out;
>     }
>     ...
> }
>
> Could this lead to immediate I/O errors and potential filesystem corruption
> instead of transparently pausing I/O?
>
> [Severity: High]
> This is a pre-existing issue, but does the lack of explicit memory barriers
> here allow a race condition?
>
> The synchronization between writing stop_drv_processing here and reading
> mrioc->current_event in mpi3mr_cleanup_fwevt_list() appears to be lockless.
> Concurrently, mpi3mr_fwevt_bh() writes mrioc->current_event and reads
> stop_drv_processing.
>
> Without smp_mb(), could the CPU reorder these operations such that
> mpi3mr_pcierr_error_detected() reads a NULL current_event while
> mpi3mr_fwevt_bh() sees stop_drv_processing as 0? This would let the worker
> continue and access PCI registers after they have been unmapped.
>
> > +             mpi3mr_cleanup_fwevt_list(mrioc);
>
> [Severity: High]
> Can calling mpi3mr_cleanup_fwevt_list() during PCI error recovery cause a
> deadlock?
>
> During PCI error recovery, mrioc->block_on_pci_err is true. This cleanup
> function reads fwevt->pending_at_sml locklessly. If the worker hasn't yet
> set pending_at_sml to 1, the cleanup function sees 0 and calls
> cancel_work_sync(), which blocks the PCI error handler.
>
> The worker thread could then set pending_at_sml to 1 and call SCSI mid-layer
> functions like scsi_remove_target(), issuing I/O commands like
> SYNCHRONIZE_CACHE. Because mpi3mr_allow_scmd_to_fw() permits these bypass
> commands, mpi3mr_qcmd() will hit the block_on_pci_err check and return
> SCSI_MLQUEUE_HOST_BUSY.
>
> Since the SCSI mid-layer retries indefinitely, the worker thread remains
> permanently blocked, and cancel_work_sync() deadlocks the PCI error handler.
>
> [Severity: High]
> This is a pre-existing issue, but does the asynchronous call to
> mpi3mr_cleanup_fwevt_list() introduced here exacerbate a use-after-free
> risk?
>
> In mpi3mr_cleanup_fwevt_list(), mrioc->current_event is read locklessly
> into a local fwevt pointer. Concurrently, the firmware event worker thread
> might finish its execution, drop all references via mpi3mr_fwevt_put(), and
> free the mpi3mr_fwevt structure.
>
> If mpi3mr_cleanup_fwevt_list() accesses fwevt->pending_at_sml or fwevt->work
> after the last reference is dropped but before mrioc->current_event is
> set to NULL, does this trigger a use-after-free?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does skipping
> cancel_work_sync() in mpi3mr_cleanup_fwevt_list() when
> fwevt->pending_at_sml is set leave the worker thread running?
>
> If the event is pending at the SCSI mid-layer, the code sets
> fwevt->discard = 1 and returns early without stopping the worker thread:
>
> mpi3mr_cleanup_fwevt_list() {
>     ...
>     if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
>         fwevt->discard = 1;
>         return;
>     }
>     ...
> }
>
> During operations like system suspend where pci_err_recovery is false, the
> caller unmaps mrioc->sysif_regs. Once unblocked, the worker thread continues
> to mpi3mr_process_event_ack() and mpi3mr_admin_request_post().
>
> Since pci_err_recovery is false, it blindly dereferences the unmapped
> mrioc->sysif_regs to post the request, causing a kernel panic.
>
> >               scsi_block_requests(mrioc->shost);
> >               mpi3mr_stop_watchdog(mrioc);
> >               mpi3mr_cleanup_resources(mrioc);
>
> [ ... ]
>
> > @@ -6069,6 +6071,7 @@ static void mpi3mr_pcierr_resume(struct pci_dev *pdev)
> >
> >       if (mrioc->block_on_pci_err) {
> >               mrioc->block_on_pci_err = false;
> > +             mrioc->stop_drv_processing = 0;
> >               scsi_unblock_requests(shost);
> >               mpi3mr_start_watchdog(mrioc);
> >       }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17

Thanks for the review.

New issues:
- The observations regarding stop_drv_processing and
mpi3mr_cleanup_fwevt_list() are valid and will be fixed in v2:
  1. Setting stop_drv_processing during pci_channel_io_frozen caused
mpi3mr_qcmd() to fail I/Os with DID_NO_CONNECT instead of pausing them
with SCSI_MLQUEUE_HOST_BUSY
     via block_on_pci_err. In v2, stop_drv_processing and
mpi3mr_cleanup_fwevt_list() calls have been removed from
pci_channel_io_frozen.
  2. To prevent in-flight firmware event handlers from accessing
unmapped MMIO register space during PCI error recovery,
mpi3mr_fwevt_bh() now checks mrioc->pci_err_recovery
     and safely skips event processing when PCI recovery is in
progress. Incoming I/O remains safely paused via block_on_pci_err for
transparent recovery.

Pre-existing issues:
- The lockless current_event UAF in mpi3mr_cleanup_fwevt_list(),
missing cancel_work_sync() for pending SML events, and memory barrier
observations are pre-existing issues
  in the driver and will be addressed in a separate follow-up patch series.