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

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