From: Ranjan Kumar <ranjan.kumar@broadcom.com>
To: linux-scsi@vger.kernel.org, martin.petersen@oracle.com
Cc: sathya.prakash@broadcom.com, chandrakanth.patil@broadcom.com,
vishakhavc@google.com, ipylypiv@google.com,
Ranjan Kumar <ranjan.kumar@broadcom.com>,
Sashiko <sashiko-bot@kernel.org>
Subject: [PATCH v5 07/10] mpi3mr: Fix firmware event reference leak during cleanup
Date: Wed, 16 Sep 2026 13:57:02 +0530 [thread overview]
Message-ID: <20260916082705.44712-8-ranjan.kumar@broadcom.com> (raw)
In-Reply-To: <20260916082705.44712-1-ranjan.kumar@broadcom.com>
During firmware event cleanup, when an event is currently executing or
pending at the SCSI mid-layer, the driver sets a discard flag and exits
the cleanup routine early. This early exit skips the normal cancel path,
resulting in the firmware event reference count not being decremented,
leading to a reference leak.
Additionally, resolve the following concurrency issues in the firmware
event handling paths:
1. mpi3mr_cleanup_fwevt_list() read current_event locklessly. It is
now acquired under fwevt_lock.
2. mpi3mr_dequeue_fwevt() dropped the reference before returning it,
risking a use-after-free. The drop is now moved into
mpi3mr_cancel_work().
3. mpi3mr_fwevt_bh() dropped fwevt_lock mid-move, racing with unload.
The move is now inlined under one continuous lock hold.
4. pending_at_sml was read/written without a lock, risking an ABBA
deadlock. It is now protected by fwevt_lock throughout.
5. mpi3mr_suspend() could unmap PCI resources before the event
worker finished. It now flushes the workqueue first.
6. mpi3mr_report_tgtdev_to_host() and mpi3mr_remove_tgtdev_from_host()
could still set pending_at_sml and block in the SCSI mid-layer
after a stop or reset had already begun, deadlocking against the
thread waiting on that flag. Both now bail out beforehand once
stop_drv_processing or reset_in_progress is set.
Reported-by: Sashiko <sashiko-bot@kernel.org>
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
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
drivers/scsi/mpi3mr/mpi3mr_os.c | 139 +++++++++++++++++--------
drivers/scsi/mpi3mr/mpi3mr_transport.c | 20 +++-
2 files changed, 109 insertions(+), 50 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 3412e1e0e8ce..dd11b13f9d0b 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -283,32 +283,6 @@ void mpi3mr_hdb_trigger_data_event(struct mpi3mr_ioc *mrioc,
mpi3mr_fwevt_add_to_list(mrioc, fwevt);
}
-/**
- * mpi3mr_fwevt_del_from_list - Delete firmware event from list
- * @mrioc: Adapter instance reference
- * @fwevt: Firmware event reference
- *
- * Delete the given firmware event from the firmware event list.
- *
- * Return: Nothing.
- */
-static void mpi3mr_fwevt_del_from_list(struct mpi3mr_ioc *mrioc,
- struct mpi3mr_fwevt *fwevt)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&mrioc->fwevt_lock, flags);
- if (!list_empty(&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);
-}
-
/**
* mpi3mr_dequeue_fwevt - Dequeue firmware event from the list
* @mrioc: Adapter instance reference
@@ -328,11 +302,7 @@ static struct mpi3mr_fwevt *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);
@@ -366,6 +336,11 @@ static void mpi3mr_cancel_work(struct mpi3mr_fwevt *fwevt)
*/
mpi3mr_fwevt_put(fwevt);
}
+
+ /*
+ * Drop the reference count that was acquired by the caller.
+ */
+ mpi3mr_fwevt_put(fwevt);
}
/**
@@ -380,17 +355,44 @@ static void mpi3mr_cancel_work(struct mpi3mr_fwevt *fwevt)
void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
{
struct mpi3mr_fwevt *fwevt = NULL;
+ unsigned long flags;
+ /*
+ * Safely read current_event under lock to prevent TOCTOU race
+ * with the firmware event worker thread.
+ */
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if ((list_empty(&mrioc->fwevt_list) && !mrioc->current_event) ||
- !mrioc->fwevt_worker_thread)
+ !mrioc->fwevt_worker_thread) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
return;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
while ((fwevt = mpi3mr_dequeue_fwevt(mrioc)))
mpi3mr_cancel_work(fwevt);
- if (mrioc->current_event) {
- fwevt = mrioc->current_event;
+ /*
+ * Safely read current_event under lock to prevent TOCTOU race
+ * with the firmware event worker thread.
+ */
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ fwevt = mrioc->current_event;
+ if (fwevt) {
+ /*
+ * Take a reference to ensure the event is not freed by the
+ * worker thread while we are evaluating or cancelling it.
+ */
+ mpi3mr_fwevt_get(fwevt);
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
+ if (fwevt) {
+ bool pending_at_sml;
+
/*
+ * Read pending_at_sml under lock to avoid a stale value.
+ *
* Don't call cancel_work_sync() API for the
* fwevt work if the controller reset is
* get called as part of processing the
@@ -398,8 +400,13 @@ void mpi3mr_cleanup_fwevt_list(struct mpi3mr_ioc *mrioc)
* waiting for device add/remove APIs to complete.
* Otherwise we will see deadlock.
*/
- if (current_work() == &fwevt->work || fwevt->pending_at_sml) {
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ pending_at_sml = fwevt->pending_at_sml;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
+ if (current_work() == &fwevt->work || pending_at_sml) {
fwevt->discard = 1;
+ mpi3mr_fwevt_put(fwevt);
return;
}
@@ -913,6 +920,8 @@ void mpi3mr_remove_tgtdev_from_host(struct mpi3mr_ioc *mrioc,
struct mpi3mr_tgt_dev *tgtdev)
{
struct mpi3mr_stgt_priv_data *tgt_priv;
+ unsigned long flags;
+ bool discard = false;
ioc_info(mrioc, "%s :Removing handle(0x%04x), wwid(0x%016llx)\n",
__func__, tgtdev->dev_handle, (unsigned long long)tgtdev->wwid);
@@ -925,17 +934,27 @@ void mpi3mr_remove_tgtdev_from_host(struct mpi3mr_ioc *mrioc,
if (!mrioc->sas_transport_enabled || (tgtdev->dev_type !=
MPI3_DEVICE_DEVFORM_SAS_SATA) || tgtdev->non_stl) {
if (tgtdev->starget) {
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->stop_drv_processing ||
+ mrioc->reset_in_progress) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ return;
+ }
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
scsi_remove_target(&tgtdev->starget->dev);
tgtdev->host_exposed = 0;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard) {
- mpi3mr_print_device_event_notice(mrioc,
- false);
- return;
- }
+ discard = mrioc->current_event->discard;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard) {
+ mpi3mr_print_device_event_notice(mrioc,
+ false);
+ return;
}
}
} else
@@ -963,6 +982,8 @@ static int mpi3mr_report_tgtdev_to_host(struct mpi3mr_ioc *mrioc,
{
int retval = 0;
struct mpi3mr_tgt_dev *tgtdev;
+ unsigned long flags;
+ bool discard = false;
if (mrioc->reset_in_progress || mrioc->pci_err_recovery)
return -1;
@@ -979,19 +1000,29 @@ static int mpi3mr_report_tgtdev_to_host(struct mpi3mr_ioc *mrioc,
if (!mrioc->sas_transport_enabled || (tgtdev->dev_type !=
MPI3_DEVICE_DEVFORM_SAS_SATA) || tgtdev->non_stl){
tgtdev->host_exposed = 1;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (mrioc->stop_drv_processing || mrioc->reset_in_progress) {
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ tgtdev->host_exposed = 0;
+ goto out;
+ }
if (mrioc->current_event)
mrioc->current_event->pending_at_sml = 1;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
scsi_scan_target(&mrioc->shost->shost_gendev,
mrioc->scsi_device_channel, tgtdev->perst_id,
SCAN_WILD_CARD, SCSI_SCAN_INITIAL);
if (!tgtdev->starget)
tgtdev->host_exposed = 0;
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard) {
- mpi3mr_print_device_event_notice(mrioc, true);
- goto out;
- }
+ discard = mrioc->current_event->discard;
+ }
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard) {
+ mpi3mr_print_device_event_notice(mrioc, true);
+ goto out;
}
dprint_event_bh(mrioc,
"exposed target device with handle(0x%04x), perst_id(%d)\n",
@@ -2134,9 +2165,19 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
u16 perst_id, handle, dev_info;
struct mpi3_device0_sas_sata_format *sasinf = NULL;
unsigned int timeout;
+ unsigned long flags;
- mpi3mr_fwevt_del_from_list(mrioc, fwevt);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ if (!list_empty(&fwevt->list)) {
+ list_del_init(&fwevt->list);
+ /*
+ * Put fwevt reference count after
+ * removing it from fwevt_list
+ */
+ mpi3mr_fwevt_put(fwevt);
+ }
mrioc->current_event = fwevt;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
if (mrioc->stop_drv_processing || mrioc->pci_err_recovery) {
dprint_event_bh(mrioc,
@@ -2271,9 +2312,12 @@ static void mpi3mr_fwevt_bh(struct mpi3mr_ioc *mrioc,
mpi3mr_process_event_ack(mrioc, fwevt->event_id,
fwevt->evt_ctx);
out:
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
+ mrioc->current_event = NULL;
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+
/* Put fwevt reference count to neutralize kref_init increment */
mpi3mr_fwevt_put(fwevt);
- mrioc->current_event = NULL;
}
/**
@@ -5874,6 +5918,9 @@ mpi3mr_suspend(struct device *dev)
ssleep(1);
mrioc->stop_drv_processing = 1;
mpi3mr_cleanup_fwevt_list(mrioc);
+ /* Flush any pending discarded event before unmapping PCI resources below. */
+ if (mrioc->fwevt_worker_thread)
+ flush_workqueue(mrioc->fwevt_worker_thread);
scsi_block_requests(shost);
mpi3mr_stop_watchdog(mrioc);
mpi3mr_cleanup_ioc(mrioc);
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index 2afff1a002ed..be794fe8eb7d 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1337,6 +1337,7 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
struct mpi3mr_sas_phy *mr_sas_phy, *next;
struct mpi3mr_sas_port *mr_sas_port;
unsigned long flags;
+ bool discard = false;
struct mpi3mr_sas_node *mr_sas_node;
struct sas_rphy *rphy;
struct mpi3mr_tgt_dev *tgtdev = NULL;
@@ -1464,8 +1465,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",
@@ -1487,11 +1490,14 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
list_add_tail(&mr_sas_port->port_list, &mr_sas_node->sas_port_list);
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 = 0;
- if (mrioc->current_event->discard)
- mpi3mr_print_device_event_notice(mrioc, true);
+ discard = mrioc->current_event->discard;
}
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard)
+ mpi3mr_print_device_event_notice(mrioc, true);
/* fill in report manufacture */
if (mr_sas_port->remote_identify.device_type ==
@@ -1529,6 +1535,7 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
{
int i;
unsigned long flags;
+ bool discard = false;
struct mpi3mr_sas_port *mr_sas_port, *next;
struct mpi3mr_sas_node *mr_sas_node;
u8 found = 0;
@@ -1585,8 +1592,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) {
@@ -1608,11 +1617,14 @@ static void mpi3mr_sas_port_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
ioc_info(mrioc, "%s: removed sas_address(0x%016llx)\n",
__func__, (unsigned long long)sas_address);
+ spin_lock_irqsave(&mrioc->fwevt_lock, flags);
if (mrioc->current_event) {
mrioc->current_event->pending_at_sml = 0;
- if (mrioc->current_event->discard)
- mpi3mr_print_device_event_notice(mrioc, false);
+ discard = mrioc->current_event->discard;
}
+ spin_unlock_irqrestore(&mrioc->fwevt_lock, flags);
+ if (discard)
+ mpi3mr_print_device_event_notice(mrioc, false);
kfree(mr_sas_port);
}
--
2.47.3
next prev parent reply other threads:[~2026-09-16 8:35 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-16 8:26 [PATCH v5 00/10] mpi3mr: Few Enhancements and minor fixes Ranjan Kumar
2026-09-16 8:26 ` [PATCH v5 01/10] mpi3mr: Skip device shutdown during unload per controller configuration Ranjan Kumar
2026-09-16 8:26 ` [PATCH v5 02/10] mpi3mr: Update MPI Headers to revision 41 Ranjan Kumar
2026-09-16 8:26 ` [PATCH v5 03/10] mpi3mr: Add early timestamp synchronization after driver load Ranjan Kumar
2026-09-16 8:26 ` [PATCH v5 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices Ranjan Kumar
2026-09-16 8:27 ` [PATCH v5 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep Ranjan Kumar
2026-09-16 8:27 ` [PATCH v5 06/10] mpi3mr: Fix memory leak on operational queue creation failure Ranjan Kumar
2026-09-16 8:27 ` Ranjan Kumar [this message]
2026-09-16 8:27 ` [PATCH v5 08/10] mpi3mr: Fix SAS port allocation and registration error handling Ranjan Kumar
2026-09-16 8:27 ` [PATCH v5 09/10] mpi3mr: Fix SAS PHY cleanup in host addition error paths Ranjan Kumar
2026-09-16 8:27 ` [PATCH v5 10/10] mpi3mr: Driver version update to 8.18.0.8.50 Ranjan Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260916082705.44712-8-ranjan.kumar@broadcom.com \
--to=ranjan.kumar@broadcom.com \
--cc=chandrakanth.patil@broadcom.com \
--cc=ipylypiv@google.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=sashiko-bot@kernel.org \
--cc=sathya.prakash@broadcom.com \
--cc=vishakhavc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox