Re: [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition / TOCTOU when accessing fwevt_worker_thread can cause a NULL pointer dereference.
- [High] Incomplete deadlock fix in SAS transport path allows ABBA deadlock with SCSI mid-layer.
- [Medium] Memory leak of firmware event when workqueue is not available.
--

--- Patch [7]: [PATCH v4 07/10] mpi3mr: Fix firmware event reference leak during cleanup ---
commit 02873552fdbb7ea8056e6479b78735dc33631a59
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix firmware event reference leak during cleanup

This patch fixes a reference leak during firmware event cleanup when an event
is currently executing or pending at the SCSI mid-layer. It also resolves
multiple concurrency and deadlock issues in the firmware event handling paths.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=7
Closes: https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=7

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 23a6a5e3df5f4..a34ea7e056900 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -282,32 +282,6 @@ void mpi3mr_hdb_trigger_data_event(struct mpi3mr_ioc *mrioc,
>  	mpi3mr_fwevt_add_to_list(mrioc, fwevt);
>  }

[Severity: High]
This is a pre-existing issue, but can the lockless check in
mpi3mr_fwevt_add_to_list() cause a race condition leading to a NULL pointer
dereference?

If mpi3mr_fwevt_add_to_list() checks mrioc->fwevt_worker_thread locklessly:

	if (!mrioc->fwevt_worker_thread)
		return;

And concurrently, during driver shutdown or unload, the workqueue pointer is
set to NULL while holding fwevt_lock:

	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
	wq = mrioc->fwevt_worker_thread;
	mrioc->fwevt_worker_thread = NULL;
	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);

Could mpi3mr_fwevt_add_to_list() pass the lockless check, then block on
fwevt_lock, and after acquiring the lock, unconditionally pass the now-NULL
mrioc->fwevt_worker_thread to queue_work()?

[Severity: Medium]
This is also a pre-existing issue, but does the early return in
mpi3mr_fwevt_add_to_list() leak the firmware event?

	if (!mrioc->fwevt_worker_thread)
		return;

Since mpi3mr_alloc_fwevt() initializes the reference count to 1, if the worker
thread is already gone and we take this early return, do we need to call
mpi3mr_fwevt_put() to prevent leaking the allocated firmware event?

> -/**
> - * mpi3mr_fwevt_del_from_list - Delete firmware event from list
> - * @mrioc: Adapter instance reference
> - * @fwevt: Firmware event reference

[ ... ]

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..b309cfdf66873 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c

[ ... ]

> @@ -1457,8 +1458,10 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	}
>  	rphy->identify = mr_sas_port->remote_identify;
>  
> +	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
>  	if (mrioc->current_event)
>  		mrioc->current_event->pending_at_sml = 1;
> +	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
>  
>  	if ((sas_rphy_add(rphy))) {
>  		ioc_err(mrioc, "failure at %s:%d/%s()!\n",

[Severity: High]
This is a pre-existing issue, but does this code allow an ABBA deadlock with
the SCSI mid-layer?

Unlike mpi3mr_report_tgtdev_to_host(), this function does not check
mrioc->reset_in_progress or mrioc->stop_drv_processing before setting
pending_at_sml = 1 and calling sas_rphy_add().

If a controller reset initiates, it waits for the event worker via
cancel_work_sync(). If the worker thread is here and calls sas_rphy_add(),
the SML commands might receive SCSI_MLQUEUE_HOST_BUSY from
mpi3mr_queuecommand() because the reset is active.

Since SML will retry infinitely while the reset is underway, does this cause
the event worker thread to deadlock against the thread waiting for the reset
to complete?

[ ... ]

> @@ -1578,8 +1585,10 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
>  
>  	spin_unlock_irqrestore(&mrioc->sas_node_lock, flags);
>  
> +	spin_lock_irqsave(&mrioc->fwevt_lock, flags);
>  	if (mrioc->current_event)
>  		mrioc->current_event->pending_at_sml = 1;
> +	spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
>  
>  	list_for_each_entry_safe(mr_sas_phy, next_phy,
>  	    &mr_sas_port->phy_list, port_siblings) {

[Severity: High]
This is a pre-existing issue, but this appears to be susceptible to the same
deadlock with the SCSI mid-layer.

If a reset is in progress, setting pending_at_sml = 1 without a bailout
check can cause the worker thread to hang if the SML commands retry
infinitely due to the active reset. Should this have the same bailout check
as mpi3mr_remove_tgtdev_from_host()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805110634.346670-1-ranjan.kumar@broadcom.com?part=7