[PATCH 05/17] dmaengine: idxd: unbind the device driver before its work queues go away

[Daniel Byrne]

idxd_remove() unregisters the work queue conf_devs first, and each
queue is freed by its conf_dev release. Only then does it unregister
the device's own conf_dev, whose driver remove, idxd_device_drv_remove(),
walks idxd->wqs and reads wq->state to disable the queues still
active. On a PCI unbind of an enabled device that is a read of freed
memory for every queue, which KASAN reports as a slab-use-after-free.
The usual administrative flow disables the device first and never hits
it.

Release the device driver before the queues are unregistered, so its
remove sees them while they exist. The conf_dev is unregistered where it
was, now without a driver bound, and the reference taken there only
keeps the memory alive for the remaining cleanup.

That remove also releases the driver of every bound queue, not only of
the queues that read enabled. A halt's clear_state leaves a bound queue
disabled, and skipping it would unregister the dma_device while that
queue's channel is still registered, so the queue's own unbind later
freed the channel and the device a second time.

The devices on the dsa bus stay open to a sysfs bind until they are
unregistered, and the bus does not take the parent's lock, so the PCI
device lock that the remove holds does not exclude a bind. The early
unbind leaves the device reading enabled while its queues' drivers are
released, and disabled before it is unregistered, which a queue probe
and the device probe accept respectively. The remove therefore marks
the device as going away before it unbinds the driver, and the device
probe and the queue enable, which every queue driver's probe calls,
refuse a device so marked, as the driver core did once the devices
were unregistered.

Fixes: 745e92a6d816 ("dmaengine: idxd: idxd: move remove() bits for idxd 'struct device' to device.c")
Assisted-by: Claude:claude-fable-5-1
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Daniel Byrne <daniel1.byrne@xxxxxxxxx>
---
 drivers/dma/idxd/device.c | 26 +++++++++++++++++++++++---
 drivers/dma/idxd/idxd.h   |  1 +
 drivers/dma/idxd/init.c   | 28 +++++++++++++++++++++++-----
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 131138483b87..a83b5da88f8c 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1415,6 +1415,16 @@ int idxd_drv_enable_wq(struct idxd_wq *wq)
 
 	lockdep_assert_held(&wq->wq_lock);
 
+	/*
+	 * The device remove releases the queues' drivers before it disables
+	 * the device, so the device still reads enabled meanwhile.
+	 */
+	if (test_bit(IDXD_FLAG_REMOVING, &idxd->flags)) {
+		idxd->cmd_status = IDXD_SCMD_DEV_NOT_ENABLED;
+		rc = -ENODEV;
+		goto err;
+	}
+
 	if (idxd->state != IDXD_DEV_ENABLED) {
 		idxd->cmd_status = IDXD_SCMD_DEV_NOT_ENABLED;
 		goto err;
@@ -1563,6 +1573,9 @@ int idxd_device_drv_probe(struct idxd_dev *idxd_dev)
 	struct idxd_device *idxd = idxd_dev_to_idxd(idxd_dev);
 	int rc = 0;
 
+	if (test_bit(IDXD_FLAG_REMOVING, &idxd->flags))
+		return -ENODEV;
+
 	/*
 	 * Device should be in disabled state for the idxd_drv to load. If it's in
 	 * enabled state, then the device was altered outside of driver's control.
@@ -1623,9 +1636,16 @@ void idxd_device_drv_remove(struct idxd_dev *idxd_dev)
 		struct idxd_wq *wq = idxd->wqs[i];
 		struct device *wq_dev = wq_confdev(wq);
 
-		if (wq->state == IDXD_WQ_DISABLED)
-			continue;
-		dev_warn(dev, "Active wq %d on disable %s.\n", i, dev_name(wq_dev));
+		/*
+		 * A halt's clear_state leaves a bound queue DISABLED while
+		 * its driver still holds the queue's channel and resources.
+		 * Release every bound driver, not only the enabled queues',
+		 * before the dma_device below frees their channels. For a
+		 * queue without a driver the release is a no-op.
+		 */
+		if (wq->state != IDXD_WQ_DISABLED)
+			dev_warn(dev, "Active wq %d on disable %s.\n", i,
+				 dev_name(wq_dev));
 		device_release_driver(wq_dev);
 	}
 
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index ce78b9a7c641..2343f0367d62 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -269,6 +269,7 @@ enum idxd_device_flag {
 	IDXD_FLAG_CMD_RUNNING,
 	IDXD_FLAG_PASID_ENABLED,
 	IDXD_FLAG_USER_PASID_ENABLED,
+	IDXD_FLAG_REMOVING,	/* PCI remove under way, refuse binds */
 };
 
 struct idxd_dma_dev {
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 4b827a329756..4443d23fde5a 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1267,13 +1267,31 @@ static void idxd_remove(struct pci_dev *pdev)
 {
 	struct idxd_device *idxd = pci_get_drvdata(pdev);
 
+	/*
+	 * The devices on the dsa bus stay registered, and so open to a
+	 * sysfs bind, until the unregisters below. A bind during the
+	 * removal would re-probe the device or enable one of its queues
+	 * while they are torn down, and the PCI device lock held here does
+	 * not exclude it, so the probes test this first. The command path
+	 * changes IDXD_FLAG_CMD_RUNNING in the same word with non-atomic
+	 * bit operations under cmd_lock, which would lose a bit set
+	 * concurrently, so the flag is set under that lock too.
+	 */
+	spin_lock_irq(&idxd->cmd_lock);
+	set_bit(IDXD_FLAG_REMOVING, &idxd->flags);
+	spin_unlock_irq(&idxd->cmd_lock);
+	/*
+	 * The idxd sub-driver's remove walks idxd->wqs to disable the
+	 * active queues, and each wq is freed with its conf_dev. Unbind
+	 * that driver while the queues still exist.
+	 */
+	device_release_driver(idxd_confdev(idxd));
 	idxd_unregister_devices(idxd);
 	/*
-	 * When ->release() is called for the idxd->conf_dev, it frees all the memory related
-	 * to the idxd context. The driver still needs those bits in order to do the rest of
-	 * the cleanup. However, we do need to unbound the idxd sub-driver. So take a ref
-	 * on the device here to hold off the freeing while allowing the idxd sub-driver
-	 * to unbind.
+	 * When ->release() is called for the idxd->conf_dev, it frees all
+	 * the memory related to the idxd context. The driver still needs
+	 * those bits in order to do the rest of the cleanup, so take a ref
+	 * on the device here to hold off the freeing until that is done.
 	 */
 	get_device(idxd_confdev(idxd));
 	device_unregister(idxd_confdev(idxd));
-- 
2.43.0