Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [PATCH v2 0/3] scsi: mvumi: do not free or reuse buffers a timed-out command left with the device
@ 2026-11-15 12:21 Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 1/3] scsi: mvumi: stop the controller bus-mastering before freeing its buffers Yehyeong Lee
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Yehyeong Lee @ 2026-11-15 12:21 UTC (permalink / raw)
  To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel

1/3 stops the controller bus-mastering before mvumi_detach_one() frees
the buffers it was given.  2/3 keeps a timed-out internal command's
buffers until then, and stops the probe loop from handing them straight
back to the controller.  3/3 waits for the event work before the host it
points at is freed.

v1 -> v2:
 - 1/3 is unchanged.
 - 2/3 also stops mvumi_probe_devices() after a timeout instead of
   asking the next target id with the same frame and data buffer, and
   frees the command on the duplicate-wwid exit, which returned
   without doing so.
 - 3/3 is new.

The review of v1 raised three things, and each is answered above: the probe
loop's reuse and the duplicate-wwid leak in 2/3, and the event work that
mvumi_detach_one() never waited for in 3/3.

Two more, both older than this series, are deliberately left out.  The DMA
address rebuilt from the command frame in mvumi_delete_internal_cmd() is
already in flight as 7/9 of
https://lore.kernel.org/all/20261015045213.3975845-1-yhlee@isslab.korea.ac.kr/
which deletes that expression altogether.  The unlocked sync_cmd test in
mvumi_issue_blocked_cmd() is a separate defect in the submit/complete
handshake - it does not release a tag twice, both releases being under
host_lock, but it can leave the counter at -1 - and wants its own patch.

Based on mainline.  It still conflicts textually with that 7/9 in
mvumi_delete_internal_cmd(), only in 2/3, and I will rebase whichever
goes in second.  I have no mvumi hardware; this was checked with a
KUnit model.

v1:
  https://lore.kernel.org/all/20261114014430.489668-1-yhlee@isslab.korea.ac.kr/

Best regards,
Yehyeong Lee

Yehyeong Lee (3):
  scsi: mvumi: stop the controller bus-mastering before freeing its
    buffers
  scsi: mvumi: quarantine a timed-out internal command's buffers
  scsi: mvumi: flush the event work before the host is freed

 drivers/scsi/mvumi.c | 73 ++++++++++++++++++++++++++++++++++++++------
 drivers/scsi/mvumi.h |  3 ++
 2 files changed, 66 insertions(+), 10 deletions(-)


base-commit: 23c62304dde29abe92013d85910d03a77dada35d
-- 
2.43.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] scsi: mvumi: stop the controller bus-mastering before freeing its buffers
  2026-11-15 12:21 [PATCH v2 0/3] scsi: mvumi: do not free or reuse buffers a timed-out command left with the device Yehyeong Lee
@ 2026-11-15 12:21 ` Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed Yehyeong Lee
  2 siblings, 0 replies; 6+ messages in thread
From: Yehyeong Lee @ 2026-11-15 12:21 UTC (permalink / raw)
  To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel

mvumi_detach_one() frees the controller's buffers before stopping its DMA:

	mvumi_release_fw(mhba);		/* frees them */
	scsi_host_put(host);
	pci_disable_device(pdev);	/* stops the DMA */

mvumi_release_fw() frees everything on mhba->res_list and the handshake
page.  The controller is given those addresses during the handshake and
may read or write them for as long as it is bus-mastering, and only
pci_disable_device() stops that.

Disable the device first.  Nothing in mvumi_release_fw() needs it
enabled: it contains no ioread32() or iowrite32() at all.

I have no mvumi hardware.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 91c2db697c6913..8d22d370d8608f 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -2548,9 +2548,9 @@ static void mvumi_detach_one(struct pci_dev *pdev)
 
 	mhba->instancet->disable_intr(mhba);
 	free_irq(mhba->pdev->irq, mhba);
+	pci_disable_device(pdev);
 	mvumi_release_fw(mhba);
 	scsi_host_put(host);
-	pci_disable_device(pdev);
 	dev_dbg(&pdev->dev, "driver is removed!\n");
 }
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers
  2026-11-15 12:21 [PATCH v2 0/3] scsi: mvumi: do not free or reuse buffers a timed-out command left with the device Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 1/3] scsi: mvumi: stop the controller bus-mastering before freeing its buffers Yehyeong Lee
@ 2026-11-15 12:21 ` Yehyeong Lee
  2026-11-15 12:39   ` sashiko-bot
  2026-11-15 12:21 ` [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed Yehyeong Lee
  2 siblings, 1 reply; 6+ messages in thread
From: Yehyeong Lee @ 2026-11-15 12:21 UTC (permalink / raw)
  To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel

mvumi_issue_blocked_cmd() waits 45 seconds for an internal command and,
if it has not completed, tidies up its own bookkeeping and returns 0, the
same value as success.  It writes nothing to the controller, so there is
no abort; the SCSI error handler is not involved either, because an
internal command has scmd == NULL.

The callers - mvumi_flush_cache(), mvumi_inquiry() and mvumi_get_event()
- cannot tell, and each goes on to mvumi_delete_internal_cmd(), which
frees the command frame and the data buffer.  The controller still has
both addresses: the frame as the source of a dynamic list entry on an
adapter with HS_CAPABILITY_SUPPORT_DYN_SRC, and the data buffer as the
target of the data-in transfer the frame's SGL describes.  One that
answers late writes into memory handed back to the page allocator.

Put both buffers on mhba->res_list instead.  The address and length are
taken where mvumi_delete_internal_cmd() takes them today, so nothing new
is trusted; mvumi_release_mem_resource() frees the list at unload, which
the preceding patch makes run after pci_disable_device().  The mark
cannot come from cmd_status, which is assigned from ob_frame->req_status
and can be left at REQ_STATUS_PENDING (0x80) by the controller.  The cost
is the frame and data buffer of each timed-out command, held until unload.

Not handing the buffers back means not reusing them either.
mvumi_probe_devices() creates one internal command and lends it to
mvumi_inquiry() for every target id, and mvumi_inquiry() only frees a
command it allocated itself, so after a timeout the next id is asked with
that same frame and data buffer, zeroed and filled in again while the
controller may still be writing to them.  Stop the loop there.  Its only
caller, mvumi_rescan_bus(), already treats -1 as "not this round" and
comes back on the next event, and once the controller has gone quiet for
45 seconds the remaining ids would cost 45 seconds each.  That also
leaves no caller that submits a second command on a struct mvumi_cmd
which has already timed out: mvumi_flush_cache() and mvumi_get_event()
create and delete one per use.

The found == -1 exit, where a wwid turns up on a second id, returned
without the mvumi_delete_internal_cmd() the normal exit does, so it also
leaked the command it was holding.  Give the function one exit.

Checked with a KUnit model, a fake controller that answers inline or
stays quiet, and no mvumi hardware.  Writing to the buffers after a
timeout, standing in for the controller, gets a KASAN use-after-free in
5 of 5 runs without this patch and none in 5 with it.  With one target id
left unanswered, the probe loop submitted 2 further commands on that
command's buffers in 5 of 5 runs and none in 5.  On the duplicate-wwid
exit the command was still allocated in 5 of 5 runs and in none of 5.

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 58 ++++++++++++++++++++++++++++++++++++++------
 drivers/scsi/mvumi.h |  1 +
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 8d22d370d8608f..48652640afc188 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -177,6 +177,29 @@ static void mvumi_release_mem_resource(struct mvumi_hba *mhba)
 	mhba->fw_flag &= ~MVUMI_FW_ALLOC;
 }
 
+/* The controller may still DMA here; free it once the device is disabled. */
+static void mvumi_quarantine_buf(struct mvumi_hba *mhba, void *virt_addr,
+				 dma_addr_t bus_addr, unsigned int size)
+{
+	struct mvumi_res *res = kzalloc_obj(*res);
+	unsigned long flags;
+
+	if (!res) {
+		dev_err(&mhba->pdev->dev,
+			"leaking %u bytes the controller may still write to\n",
+			size);
+		return;
+	}
+	res->type = RESOURCE_UNCACHED_MEMORY;
+	res->virt_addr = virt_addr;
+	res->bus_addr = bus_addr;
+	res->size = size;
+	INIT_LIST_HEAD(&res->entry);
+	spin_lock_irqsave(&mhba->shost->host_lock, flags);
+	list_add_tail(&res->entry, &mhba->res_list);
+	spin_unlock_irqrestore(&mhba->shost->host_lock, flags);
+}
+
 /**
  * mvumi_make_sgl -	Prepares  SGL
  * @mhba:		Adapter soft state
@@ -298,11 +321,19 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
 			phy_addr = (dma_addr_t) m_sg->baseaddr_l |
 				(dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
 
-			dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
-								phy_addr);
+			if (cmd->timed_out)
+				mvumi_quarantine_buf(mhba, cmd->data_buf,
+						     phy_addr, size);
+			else
+				dma_free_coherent(&mhba->pdev->dev, size,
+						  cmd->data_buf, phy_addr);
 		}
-		dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
-				cmd->frame, cmd->frame_phys);
+		if (cmd->timed_out)
+			mvumi_quarantine_buf(mhba, cmd->frame, cmd->frame_phys,
+					     mhba->ib_max_size);
+		else
+			dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
+					  cmd->frame, cmd->frame_phys);
 		kfree(cmd);
 	}
 }
@@ -734,6 +765,7 @@ static int mvumi_issue_blocked_cmd(struct mvumi_hba *mhba,
 	if (atomic_read(&cmd->sync_cmd)) {
 		spin_lock_irqsave(&mhba->shost->host_lock, flags);
 		atomic_dec(&cmd->sync_cmd);
+		cmd->timed_out = 1;
 		if (mhba->tag_cmd[cmd->frame->tag]) {
 			mhba->tag_cmd[cmd->frame->tag] = NULL;
 			dev_warn(&mhba->pdev->dev, "TIMEOUT:release tag [%d]\n",
@@ -1552,6 +1584,7 @@ static int mvumi_probe_devices(struct mvumi_hba *mhba)
 	struct mvumi_device *mv_dev = NULL;
 	struct mvumi_cmd *cmd = NULL;
 	int found = 0;
+	int ret;
 
 	cmd = mvumi_create_internal_cmd(mhba, 64);
 	if (!cmd)
@@ -1564,6 +1597,11 @@ static int mvumi_probe_devices(struct mvumi_hba *mhba)
 
 	for (id = 0; id < maxid; id++) {
 		wwid = mvumi_inquiry(mhba, id, cmd);
+		/* the controller still owns this command's buffers */
+		if (cmd->timed_out) {
+			ret = -1;
+			goto out;
+		}
 		if (!wwid) {
 			/* device no response, remove it */
 			mvumi_remove_devices(mhba, id);
@@ -1588,17 +1626,21 @@ static int mvumi_probe_devices(struct mvumi_hba *mhba)
 				dev_dbg(&mhba->pdev->dev,
 					"probe a new device(0:%d:0)"
 					" wwid(%llx)\n", id, mv_dev->wwid);
-			} else if (found == -1)
-				return -1;
-			else
+			} else if (found == -1) {
+				ret = -1;
+				goto out;
+			} else {
 				continue;
+			}
 		}
 	}
+	ret = 0;
 
+out:
 	if (cmd)
 		mvumi_delete_internal_cmd(mhba, cmd);
 
-	return 0;
+	return ret;
 }
 
 static int mvumi_rescan_bus(void *data)
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index 1306a4abf19ad2..e248dfc1dd1bb7 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -252,6 +252,7 @@ struct mvumi_cmd {
 	void *data_buf;
 	unsigned short request_id;
 	unsigned char cmd_status;
+	unsigned char timed_out;
 };
 
 struct mvumi_cmd_priv {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed
  2026-11-15 12:21 [PATCH v2 0/3] scsi: mvumi: do not free or reuse buffers a timed-out command left with the device Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 1/3] scsi: mvumi: stop the controller bus-mastering before freeing its buffers Yehyeong Lee
  2026-11-15 12:21 ` [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers Yehyeong Lee
@ 2026-11-15 12:21 ` Yehyeong Lee
  2026-11-15 12:33   ` sashiko-bot
  2 siblings, 1 reply; 6+ messages in thread
From: Yehyeong Lee @ 2026-11-15 12:21 UTC (permalink / raw)
  To: mkp, James.Bottomley; +Cc: linux-scsi, linux-kernel

mvumi_launch_events() runs from the interrupt handler and hands a
DRBL_EVENT_NOTIFY off to mvumi_scan_events() on the system workqueue.
That work calls mvumi_get_event(), which dereferences the mvumi_hba it
was given, allocates an internal command from mhba->pdev and then waits
up to 45 seconds for the controller to answer it.

mvumi_detach_one() never waits for it.  It frees the interrupt, releases
the firmware resources and calls scsi_host_put(), which frees the
Scsi_Host the mvumi_hba lives in, while the work is still queued or
still parked in that 45 second wait.

Give the driver its own queue and destroy it in mvumi_detach_one() once
the interrupt is gone, so nothing can be queued after that point and
nothing is still running when the host is freed.  A work that was
already queued runs with the interrupt freed and its internal command
times out, which the preceding patch makes safe.  mvumi_probe_one()'s
error path needs the same order, because mvumi_io_attach() can fail with
the interrupt already enabled: flush the queue there once the interrupt
is freed, before mvumi_release_fw() takes away what the work uses.

In a KUnit model where the event command is left unanswered and the host
is put while the work sits in the wait, the work then reads the freed
host in 5 of 5 runs and in none of 5 with this patch:

[  111.616956] BUG: KASAN: slab-use-after-free in mvumi_scan_events+0xc83/0xd10
[  111.617010] Read of size 8 at addr ffff888009ff8908 by task kworker/1:1/30
[  111.617043]
[  111.617073] CPU: 1 UID: 0 PID: 30 Comm: kworker/1:1 Tainted: G    B            N  7.4.0-rc2-00716-g23c62304dde2-dirty #2 PREEMPT(lazy)
[  111.617126] Tainted: [B]=BAD_PAGE, [N]=TEST
[  111.617144] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[  111.617173] Workqueue: events mvumi_scan_events
[  111.617258] Call Trace:
[  111.617304]  <TASK>
[  111.617354]  dump_stack_lvl+0x53/0x70
[  111.617438]  print_report+0xd0/0x630
[  111.617549]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[  111.617622]  ? mvumi_issue_blocked_cmd.isra.0+0x62b/0x940
[  111.617707]  ? mvumi_scan_events+0xc83/0xd10
[  111.617785]  kasan_report+0xe5/0x120
[  111.617871]  ? mvumi_scan_events+0xc83/0xd10
[  111.617950]  mvumi_scan_events+0xc83/0xd10
[  111.618052]  process_one_work+0x697/0x1060
[  111.618146]  ? __pfx___schedule+0x10/0x10
[  111.618207]  ? __pfx_process_one_work+0x10/0x10
[  111.618288]  ? _raw_spin_lock_irq+0x81/0xe0
[  111.618370]  ? __pfx_mvumi_scan_events+0x10/0x10
[  111.618466]  ? assign_work+0x11d/0x370
[  111.618578]  worker_thread+0x45b/0xd10
[  111.618688]  ? __pfx_worker_thread+0x10/0x10
[  111.618752]  ? __pfx_worker_thread+0x10/0x10
[  111.618840]  kthread+0x2c8/0x3b0
[  111.618910]  ? recalc_sigpending+0x15c/0x1e0
[  111.618968]  ? __pfx_kthread+0x10/0x10
[  111.619040]  ret_from_fork+0x36e/0x5a0
[  111.619127]  ? __pfx_ret_from_fork+0x10/0x10
[  111.619213]  ? __switch_to+0xac3/0xdc0
[  111.619254]  ? __switch_to_asm+0x33/0x70
[  111.619312]  ? __pfx_kthread+0x10/0x10
[  111.619399]  ret_from_fork_asm+0x1a/0x30
[  111.619484]  </TASK>
[  111.619542]
[  111.619596] Allocated by task 84:
[  111.619643]  kasan_save_stack+0x33/0x60
[  111.619703]  kasan_save_track+0x14/0x30
[  111.619754]  __kasan_kmalloc+0x8f/0xa0
[  111.619808]  __kmalloc_noprof+0x193/0x450
[  111.619860]  scsi_host_alloc+0x29/0x1290
[  111.619934]  mvt_setup+0x14e/0xa50
[  111.619997]  mvt_event_work_after_put+0x93/0x3f0
[  111.620063]  kunit_try_run_case+0x1ad/0x490
[  111.620096]  kunit_generic_run_threadfn_adapter+0x7b/0xe0
[  111.620153]  kthread+0x2c8/0x3b0
[  111.620212]  ret_from_fork+0x36e/0x5a0
[  111.620291]  ret_from_fork_asm+0x1a/0x30
[  111.620369]
[  111.620415] Freed by task 84:
[  111.620462]  kasan_save_stack+0x33/0x60
[  111.620509]  kasan_save_track+0x14/0x30
[  111.620542]  kasan_save_free_info+0x3b/0x60
[  111.620598]  __kasan_slab_free+0x43/0x70
[  111.620659]  kfree+0x121/0x390
[  111.620719]  device_release+0xc8/0x240
[  111.620790]  kobject_put+0x14d/0x280
[  111.620856]  mvt_event_work_after_put+0x22f/0x3f0
[  111.620907]  kunit_try_run_case+0x1ad/0x490
[  111.620961]  kunit_generic_run_threadfn_adapter+0x7b/0xe0
[  111.621035]  kthread+0x2c8/0x3b0
[  111.621083]  ret_from_fork+0x36e/0x5a0
[  111.621154]  ret_from_fork_asm+0x1a/0x30
[  111.621230]
[  111.621281] The buggy address belongs to the object at ffff888009ff8000
[  111.621281]  which belongs to the cache kmalloc-4k of size 4096
[  111.621321] The buggy address is located 2312 bytes inside of

Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/mvumi.c | 13 ++++++++++++-
 drivers/scsi/mvumi.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 48652640afc188..f58b29e55a3778 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -1797,7 +1797,7 @@ static void mvumi_launch_events(struct mvumi_hba *mhba, u32 isr_status)
 			mu_ev->event = APICDB1_EVENT_GETEVENT;
 			isr_status &= ~(DRBL_EVENT_NOTIFY);
 			mu_ev->param = NULL;
-			schedule_work(&mu_ev->work_q);
+			queue_work(mhba->work_q, &mu_ev->work_q);
 		}
 	}
 }
@@ -2534,6 +2534,13 @@ static int mvumi_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhba->shost = host;
 	mhba->unique_id = pci_dev_id(pdev);
 
+	mhba->work_q = alloc_workqueue("mvumi_wq_%d", WQ_PERCPU, 0,
+				       host->host_no);
+	if (!mhba->work_q) {
+		ret = -ENOMEM;
+		goto fail_alloc_workqueue;
+	}
+
 	ret = mvumi_init_fw(mhba);
 	if (ret)
 		goto fail_init_fw;
@@ -2560,9 +2567,12 @@ static int mvumi_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 fail_io_attach:
 	mhba->instancet->disable_intr(mhba);
 	free_irq(mhba->pdev->irq, mhba);
+	flush_workqueue(mhba->work_q);
 fail_init_irq:
 	mvumi_release_fw(mhba);
 fail_init_fw:
+	destroy_workqueue(mhba->work_q);
+fail_alloc_workqueue:
 	scsi_host_put(host);
 
 fail_alloc_instance:
@@ -2590,6 +2600,7 @@ static void mvumi_detach_one(struct pci_dev *pdev)
 
 	mhba->instancet->disable_intr(mhba);
 	free_irq(mhba->pdev->irq, mhba);
+	destroy_workqueue(mhba->work_q);
 	pci_disable_device(pdev);
 	mvumi_release_fw(mhba);
 	scsi_host_put(host);
diff --git a/drivers/scsi/mvumi.h b/drivers/scsi/mvumi.h
index e248dfc1dd1bb7..04cd844851423e 100644
--- a/drivers/scsi/mvumi.h
+++ b/drivers/scsi/mvumi.h
@@ -493,6 +493,8 @@ struct mvumi_hba {
 	atomic_t fw_outstanding;
 	struct mvumi_instance_template *instancet;
 
+	struct workqueue_struct *work_q;
+
 	void *ib_list;
 	dma_addr_t ib_list_phys;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed
  2026-11-15 12:21 ` [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed Yehyeong Lee
@ 2026-11-15 12:33   ` sashiko-bot
  0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-11-15 12:33 UTC (permalink / raw)
  To: Yehyeong Lee; +Cc: linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] scsi: mvumi: mvumi_release_fw() called before
  pci_disable_device() on error
--

commit 4d0117b39b9433095d582919461757b3980ec9e3
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: mvumi: flush the event work before the host is freed

This commit gives mvumi a dedicated workqueue for event notifications and
flushes or destroys it during probe failure and device removal before the
host is freed.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 48652640afc18..f58b29e55a377 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c

[ ... ]

> @@ -2560,9 +2567,12 @@ static int mvumi_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  fail_io_attach:
>  	mhba->instancet->disable_intr(mhba);
>  	free_irq(mhba->pdev->irq, mhba);
> +	flush_workqueue(mhba->work_q);
>  fail_init_irq:
>  	mvumi_release_fw(mhba);
>  fail_init_fw:
> +	destroy_workqueue(mhba->work_q);
> +fail_alloc_workqueue:
>  	scsi_host_put(host);
>  
>  fail_alloc_instance:

[Severity: Medium]
Does fail_io_attach free quarantined DMA buffers in mvumi_release_fw()
while the controller still has bus-mastering enabled?

Because flush_workqueue(mhba->work_q) runs after free_irq(), any queued
mvumi_scan_events() work times out in mvumi_issue_blocked_cmd(), and
mvumi_delete_internal_cmd() quarantines cmd->data_buf and cmd->frame onto
mhba->res_list to be freed only after pci_disable_device().  However, unlike
mvumi_detach_one(), this error path calls mvumi_release_fw() ->
mvumi_release_mem_resource() before fail_set_dma_mask calls
pci_disable_device(pdev), so dma_free_coherent() returns those buffers and
mhba->handshake_page to the allocator while the controller can still DMA to
them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115122153.1446810-1-yhlee@isslab.korea.ac.kr?part=3

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers
  2026-11-15 12:21 ` [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers Yehyeong Lee
@ 2026-11-15 12:39   ` sashiko-bot
  0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-11-15 12:39 UTC (permalink / raw)
  To: Yehyeong Lee; +Cc: linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] scsi: mvumi: stranded mhba_dev_list entries when
  mvumi_probe_devices() aborts
--

commit b1478e67683e6d020afcb1ac3f2ae79623c98cd3
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: mvumi: quarantine a timed-out internal command's buffers

This commit moves the DMA buffers of timed-out internal commands onto
mhba->res_list instead of freeing them immediately, and stops the target
scan loop in mvumi_probe_devices() when an inquiry command times out.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8d22d370d8608..48652640afc18 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
[ ... ]
> @@ -1552,6 +1584,7 @@ static int mvumi_probe_devices(struct mvumi_hba *mhba)
>  	struct mvumi_device *mv_dev = NULL;
>  	struct mvumi_cmd *cmd = NULL;
>  	int found = 0;
> +	int ret;
>  
>  	cmd = mvumi_create_internal_cmd(mhba, 64);
>  	if (!cmd)
> @@ -1564,6 +1597,11 @@ static int mvumi_probe_devices(struct mvumi_hba *mhba)
>  
>  	for (id = 0; id < maxid; id++) {
>  		wwid = mvumi_inquiry(mhba, id, cmd);
> +		/* the controller still owns this command's buffers */
> +		if (cmd->timed_out) {
> +			ret = -1;
> +			goto out;
> +		}

[Severity: Medium]
Does aborting the loop here strand struct mvumi_device entries allocated
for earlier target IDs on mhba->mhba_dev_list?

When an earlier iteration finds a new target, mvumi_probe_devices()
removes any prior entry from mhba->shost_dev_list and appends a new
struct mvumi_device to mhba->mhba_dev_list.  If a later target ID times
out and returns -1, mvumi_rescan_bus() skips draining mhba->mhba_dev_list:

drivers/scsi/mvumi.c:mvumi_rescan_bus() {
	...
		ret = mvumi_probe_devices(mhba);
		if (!ret) {
			list_for_each_entry_safe(mv_dev, dev_next,
						 &mhba->mhba_dev_list, list) {
	...
}

Because mvumi_match_devices() and mvumi_remove_devices() only inspect
mhba->shost_dev_list, a later rescan will allocate duplicate entries on
mhba->mhba_dev_list for those earlier targets or bring stale entries
online if a target disappeared in the meantime.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115122153.1446810-1-yhlee@isslab.korea.ac.kr?part=2

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-11-15 12:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-15 12:21 [PATCH v2 0/3] scsi: mvumi: do not free or reuse buffers a timed-out command left with the device Yehyeong Lee
2026-11-15 12:21 ` [PATCH v2 1/3] scsi: mvumi: stop the controller bus-mastering before freeing its buffers Yehyeong Lee
2026-11-15 12:21 ` [PATCH v2 2/3] scsi: mvumi: quarantine a timed-out internal command's buffers Yehyeong Lee
2026-11-15 12:39   ` sashiko-bot
2026-11-15 12:21 ` [PATCH v2 3/3] scsi: mvumi: flush the event work before the host is freed Yehyeong Lee
2026-11-15 12:33   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox