[PATCH v5 00/12] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[PATCH v5 00/12] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[Logan Gunthorpe]

This is the latest series of fixes that has been rebased onto v7.3-rc1.

There is an unresolved disagreement between me and Frank who does not
think patches 3 and 4 are worth taking. I think they fix unlikely but
theoretically possible issues with hardware failing to tear down
correctly. I think these patches are correct, the best we can do and worth
doing. And I especially want to have them merged if only to not have
Sashiko bringing up the same issues every time we send a patch set.

Please note: I'm going to be on vacation starting Friday the 4th until
the 15th so if there is any feedback in that window I'll respond when
I get back.

Thanks,

Logan

Changes since v4:
 * Rebased onto v7.3-rc1.
 * Added paragraph to patches 3 and 4 to make clear that they are
   leaking memory in favour of preventing theoretically buggy hardware
   from trashing re-used memory. I think this is the best thing to do.

Changes since v3:
 * Add a patch (3) making switchtec_dma_chan_stop() clear the DMA base
   registers even when halt_channel() times out, and return the halt
   result. switchtec_dma_free_chan_resources() (patch 3) and the
   alloc_chan_resources() error path (patch 4) now skip freeing the
   descriptor rings when the halt wasn't confirmed. This will leak some
   memory on tear down but that avoids broken hardware from scribbling
   on memory that may have been freed and reallocated. (Per Sashiko)
 * Remove each channel's list entry in switchtec_dma_chans_free()
   (patch 5), immediately before the memory is freed, instead of in
   switchtec_dma_chans_disable() (patch 8), which now only frees the
   channel status IRQ. (Per Sashiko)
 * Collected Reviewed-by tags from Frank and applied one of his
   commit message suggestions.

Changes since v2:

 * Fixed a race when unlisting the channels in the error path.
   The interrupt needed to be disabled before hand. (Per Sashiko)
 * Picked up Acked-by from Dave Jiang on the two ioat patches.

Changes since v1:

 * Added a fix for switchtec_dma_alloc_chan_resources()'s error path
   calling disable_channel() instead of properly halting the channel
   before freeing the descriptor rings. (Per Sashiko)
 * Added a fix for switchtec-dma channel structs being freed without
   being removed from dma_dev->channels on a registration failure,
   while the channel status IRQ is still live. (Per Sashiko)
 * Added a fix for switchtec_dma_remove() using swdma_dev after it may
   already have been freed by dma_async_device_unregister(). (Per
   Sashiko)
 * Added a fix for chan_status_irq being freed with the wrong API, and
   a valid vector index of 0 being incorrectly treated as unset.
   (Per Sashiko)
 * Made switchtec_dma_chans_release() void, since nothing checked its
   return value. (Noticed while reviewing the code for these changes).

Logan Gunthorpe (12):
  dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()
  dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources
  dmaengine: switchtec-dma: always clear DMA base registers on
    chan_stop()
  dmaengine: switchtec-dma: halt channel on alloc_chan_resources error
  dmaengine: switchtec-dma: fix channel leak on registration failure
  dmaengine: switchtec-dma: make switchtec_dma_chans_release() void
  dmaengine: switchtec-dma: fix chan_status_irq cleanup on create()
    error
  dmaengine: switchtec-dma: disable channels before freeing on
    registration failure
  dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()
  dmaengine: ioat: disable relaxed ordering before registering the
    device
  dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()
  dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

 drivers/dma/ioat/init.c     |  18 +++---
 drivers/dma/ioat/sysfs.c    |  22 ++++----
 drivers/dma/plx_dma.c       |  10 ++--
 drivers/dma/switchtec_dma.c | 107 ++++++++++++++++++++++++++----------
 4 files changed, 104 insertions(+), 53 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.3

[PATCH v5 01/12] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

[Logan Gunthorpe]

switchtec_dma_free_desc() frees swdma_chan->hw_sq, hw_cq, and every
desc_ring[] entry without clearing the pointers afterward. If
switchtec_dma_alloc_chan_resources() fails partway through and calls
it during unwind, then a later retry of alloc_chan_resources() fails
in switchtec_dma_alloc_desc() before reallocating one of those
pointers, its own failure path calls switchtec_dma_free_desc() again
and frees the same, already-freed pointers a second time.

NULL out each pointer as it's freed so a subsequent call is a no-op
for anything already released.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index c133535d3765..a10818efba4e 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
 	if (swdma_chan->hw_sq)
 		dma_free_coherent(swdma_dev->dma_dev.dev, size,
 				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
+	swdma_chan->hw_sq = NULL;
 
 	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
 	if (swdma_chan->hw_cq)
 		dma_free_coherent(swdma_dev->dma_dev.dev, size,
 				  swdma_chan->hw_cq, swdma_chan->dma_addr_cq);
+	swdma_chan->hw_cq = NULL;
 
-	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++)
+	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
 		kfree(swdma_chan->desc_ring[i]);
+		swdma_chan->desc_ring[i] = NULL;
+	}
 }
 
 static int switchtec_dma_alloc_desc(struct switchtec_dma_chan *swdma_chan)
-- 
2.47.3

[PATCH v5 02/12] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[Logan Gunthorpe]

switchtec_dma_alloc_chan_resources() returns directly on any later
failure, without ever freeing the descriptor rings and coherent DMA
memory it just allocated. The dmaengine core does not call
device_free_chan_resources() when device_alloc_chan_resources() fails,
so the driver has to unwind its own partial state.

The device-removed check also runs after ring_active and
comp_ring_active have already been set true, so a failure there left
the channel marked active despite alloc_chan_resources() reporting
failure.

Add an error-unwind path that disables the channel and frees the
descriptor rings on every failure after allocation. ring_active and
comp_ring_active are cleared under the same locks
switchtec_dma_free_chan_resources() already uses, since the completion
tasklet checks comp_ring_active under complete_lock before touching
the completion ring, and a stale IRQ can still be in flight when this
unwind path runs.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org
Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index a10818efba4e..e55fe9ff7e2c 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -988,15 +988,15 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 
 	rc = enable_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_free_desc;
 
 	rc = reset_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_disable_channel;
 
 	rc = unhalt_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_disable_channel;
 
 	swdma_chan->ring_active = true;
 	swdma_chan->comp_ring_active = true;
@@ -1007,7 +1007,8 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 	rcu_read_lock();
 	if (!rcu_dereference(swdma_dev->pdev)) {
 		rcu_read_unlock();
-		return -ENODEV;
+		rc = -ENODEV;
+		goto err_ring_inactive;
 	}
 
 	perf_cfg = readl(&swdma_chan->mmio_chan_fw->perf_cfg);
@@ -1029,6 +1030,20 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 		FIELD_GET(PERF_MRRS_MASK, perf_cfg));
 
 	return SWITCHTEC_DMA_SQ_SIZE;
+
+err_ring_inactive:
+	spin_lock_bh(&swdma_chan->submit_lock);
+	swdma_chan->ring_active = false;
+	spin_unlock_bh(&swdma_chan->submit_lock);
+
+	spin_lock_bh(&swdma_chan->complete_lock);
+	swdma_chan->comp_ring_active = false;
+	spin_unlock_bh(&swdma_chan->complete_lock);
+err_disable_channel:
+	disable_channel(swdma_chan);
+err_free_desc:
+	switchtec_dma_free_desc(swdma_chan);
+	return rc;
 }
 
 static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
-- 
2.47.3

[PATCH v5 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

[Logan Gunthorpe]

switchtec_dma_chan_stop() returned early if halt_channel() timed out,
skipping the writes that clear sq_base/cq_base on the channel, and
gave its caller no way to tell the halt hadn't been confirmed.
switchtec_dma_free_chan_resources() unconditionally frees the
descriptor rings right after calling this function, so if the
hardware failed to halt, it could keep writing into memory that had
already been freed.

Attempt the register clear regardless of whether the halt was successful
and have switchtec_dma_chan_stop() return the halt result so callers can
tell when it wasn't confirmed. switchtec_dma_free_chan_resources() now
skips freeing the descriptor rings (leaking them instead) in case the
hardware continues to write into that memory.

All this is hardening that is pretty unlikely to be hit in the real
world. However, it is correct and the best thing to do against buggy
hardware that doesn't tear down correctly and could still write to
memory. In this unlikely situation it is better to leak the memory
than have hardware randomly trash memory that has already been used
for something else.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260721162531.BA01A1F01560@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index e55fe9ff7e2c..e2bb65237d2c 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -543,26 +543,33 @@ switchtec_dma_abort_desc(struct switchtec_dma_chan *swdma_chan, int force)
 	spin_unlock_bh(&swdma_chan->complete_lock);
 }
 
-static void switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
+static int switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
 {
+	struct pci_dev *pdev;
 	int rc;
 
 	rc = halt_channel(swdma_chan);
-	if (rc)
-		return;
 
 	rcu_read_lock();
-	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
+	pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
+	if (!pdev) {
 		rcu_read_unlock();
-		return;
+		return rc;
 	}
 
+	if (rc)
+		pci_err(pdev,
+			"Channel %d halt timed out, clearing DMA base registers anyway\n",
+			swdma_chan->index);
+
 	writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
 	writel(0, &swdma_chan->mmio_chan_fw->sq_base_hi);
 	writel(0, &swdma_chan->mmio_chan_fw->cq_base_lo);
 	writel(0, &swdma_chan->mmio_chan_fw->cq_base_hi);
 
 	rcu_read_unlock();
+
+	return rc;
 }
 
 static int switchtec_dma_terminate_all(struct dma_chan *chan)
@@ -1050,6 +1057,7 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
 {
 	struct switchtec_dma_chan *swdma_chan =
 		container_of(chan, struct switchtec_dma_chan, dma_chan);
+	int rc;
 
 	spin_lock_bh(&swdma_chan->submit_lock);
 	swdma_chan->ring_active = false;
@@ -1059,9 +1067,10 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
 	swdma_chan->comp_ring_active = false;
 	spin_unlock_bh(&swdma_chan->complete_lock);
 
-	switchtec_dma_chan_stop(swdma_chan);
+	rc = switchtec_dma_chan_stop(swdma_chan);
 	switchtec_dma_abort_desc(swdma_chan, 0);
-	switchtec_dma_free_desc(swdma_chan);
+	if (!rc)
+		switchtec_dma_free_desc(swdma_chan);
 
 	disable_channel(swdma_chan);
 }
-- 
2.47.3

[PATCH v5 04/12] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

[Logan Gunthorpe]

The error-unwind path called disable_channel() before freeing the
descriptor rings, but that only clears the enable bit with an
unflushed write -- it doesn't halt the channel or clear its DMA base
address registers. If unhalt_channel() timed out, the channel's actual
state is unknown at that point, so nothing guarantees the hardware
isn't still touching the rings when they're freed.

Call switchtec_dma_chan_stop() first, matching what
switchtec_dma_free_chan_resources() already does before freeing
descriptors on the normal teardown path: it synchronously halts the
channel and zeroes the DMA base registers. If the halt itself can't be
confirmed, skip freeing the descriptor rings (leaking them instead).

All this is hardening that is pretty unlikely to be hit in the real
world. However, it is correct and the best thing to do against buggy
hardware that doesn't tear down correctly and could still write to
memory. In this unlikely situation it is better to leak the memory
than have hardware randomly trash memory that has already been used
for something else.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223647.F0A051F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260727185101.D221C1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index e2bb65237d2c..2d5ba194c99a 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1047,6 +1047,10 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 	swdma_chan->comp_ring_active = false;
 	spin_unlock_bh(&swdma_chan->complete_lock);
 err_disable_channel:
+	if (switchtec_dma_chan_stop(swdma_chan)) {
+		disable_channel(swdma_chan);
+		return rc;
+	}
 	disable_channel(swdma_chan);
 err_free_desc:
 	switchtec_dma_free_desc(swdma_chan);
-- 
2.47.3

[PATCH v5 05/12] dmaengine: switchtec-dma: fix channel leak on registration failure

[Logan Gunthorpe]

switchtec_dma_chans_release() is called in three places but the
underlying memory is not freed in all of those places. In order to
clean this up, introduce a switchtec_dma_chans_free() helper that
will free the memory.

Ensure each call to switchtec_dma_chans_release() has a corresponding
switchtec_dma_chans_free() call. (The release in switchtec_dma_remove()
pairs with the free in switchtec_dma_release()).

swdma_dev->chan_cnt is now set to the number of channels that succeeded
when one fails to initialise, so switchtec_dma_chans_free() can still
be used if not all channels succeed in being allocated.

switchtec_dma_chans_free() also removes each channel from
dma_dev->channels before freeing it, since the channel status ISR
walks that list and would otherwise dereference freed memory.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260727190604.397DB1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 2d5ba194c99a..f71c64c137e8 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1188,6 +1188,18 @@ static int switchtec_dma_chans_release(struct pci_dev *pdev,
 	return 0;
 }
 
+static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
+{
+	int i;
+
+	for (i = 0; i < swdma_dev->chan_cnt; i++) {
+		list_del(&swdma_dev->swdma_chans[i]->dma_chan.device_node);
+		kfree(swdma_dev->swdma_chans[i]);
+	}
+
+	kfree(swdma_dev->swdma_chans);
+}
+
 static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 					 struct pci_dev *pdev, int chan_cnt)
 {
@@ -1213,7 +1225,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 		if (rc) {
 			dev_err(&pdev->dev, "Channel %d: init channel failed\n",
 				i);
-			chan_cnt = i;
+			swdma_dev->chan_cnt = i;
 			goto err_exit;
 		}
 	}
@@ -1221,10 +1233,8 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 	return chan_cnt;
 
 err_exit:
-	for (i = 0; i < chan_cnt; i++)
-		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
-
-	kfree(swdma_dev->swdma_chans);
+	switchtec_dma_chans_release(pdev, swdma_dev);
+	switchtec_dma_chans_free(swdma_dev);
 
 	return rc;
 }
@@ -1233,12 +1243,8 @@ static void switchtec_dma_release(struct dma_device *dma_dev)
 {
 	struct switchtec_dma_dev *swdma_dev =
 		container_of(dma_dev, struct switchtec_dma_dev, dma_dev);
-	int i;
 
-	for (i = 0; i < swdma_dev->chan_cnt; i++)
-		kfree(swdma_dev->swdma_chans[i]);
-
-	kfree(swdma_dev->swdma_chans);
+	switchtec_dma_chans_free(swdma_dev);
 
 	put_device(dma_dev->dev);
 	kfree(swdma_dev);
@@ -1329,6 +1335,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 
 err_chans_release_exit:
 	switchtec_dma_chans_release(pdev, swdma_dev);
+	switchtec_dma_chans_free(swdma_dev);
 
 err_exit:
 	if (swdma_dev->chan_status_irq)
-- 
2.47.3

[PATCH v5 06/12] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

[Logan Gunthorpe]

switchtec_dma_chans_release() always returned 0, and no caller
checked it.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index f71c64c137e8..c30530ba78ca 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1177,15 +1177,13 @@ static int switchtec_dma_chan_free(struct pci_dev *pdev,
 	return 0;
 }
 
-static int switchtec_dma_chans_release(struct pci_dev *pdev,
-				       struct switchtec_dma_dev *swdma_dev)
+static void switchtec_dma_chans_release(struct pci_dev *pdev,
+					struct switchtec_dma_dev *swdma_dev)
 {
 	int i;
 
 	for (i = 0; i < swdma_dev->chan_cnt; i++)
 		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
-
-	return 0;
 }
 
 static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
-- 
2.47.3

[PATCH v5 07/12] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[Logan Gunthorpe]

chan_status_irq stores an MSI-X vector index, but err_exit freed it
with plain free_irq() instead of pci_free_irq(), which would free the
wrong Linux IRQ. The guard also treated a valid vector index of 0 as
unset, skipping the free entirely in that case and leaving the handler
registered against soon-to-be-freed swdma_dev.

Initialize chan_status_irq to -1 and use the value being non-negative
to signal when to free it with pci_free_irq().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index c30530ba78ca..3068d9de4e6d 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1262,6 +1262,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	if (!swdma_dev)
 		return -ENOMEM;
 
+	swdma_dev->chan_status_irq = -1;
+
 	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
 				 pci_resource_len(pdev, 0));
 
@@ -1336,8 +1338,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	switchtec_dma_chans_free(swdma_dev);
 
 err_exit:
-	if (swdma_dev->chan_status_irq)
-		free_irq(swdma_dev->chan_status_irq, swdma_dev);
+	if (swdma_dev->chan_status_irq >= 0)
+		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
 
 	iounmap(swdma_dev->bar);
 	kfree(swdma_dev);
-- 
2.47.3

[PATCH v5 08/12] dmaengine: switchtec-dma: disable channels before freeing on registration failure

[Logan Gunthorpe]

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels (either from switchtec_dma_chans_enumerate()'s own
error path, or from dma_async_device_register() failing), the channel
status IRQ is still live while the channels are released and freed,
so it can fire mid-teardown against channels that are only partially
torn down.

Add switchtec_dma_chans_disable(), which frees the channel status IRQ
before releasing and freeing channels in both error paths.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260721162822.05CDD1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 3068d9de4e6d..c2803af6589d 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1198,6 +1198,15 @@ static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
 	kfree(swdma_dev->swdma_chans);
 }
 
+static void switchtec_dma_chans_disable(struct pci_dev *pdev,
+					struct switchtec_dma_dev *swdma_dev)
+{
+	if (swdma_dev->chan_status_irq >= 0) {
+		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
+		swdma_dev->chan_status_irq = -1;
+	}
+}
+
 static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 					 struct pci_dev *pdev, int chan_cnt)
 {
@@ -1231,6 +1240,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 	return chan_cnt;
 
 err_exit:
+	switchtec_dma_chans_disable(pdev, swdma_dev);
 	switchtec_dma_chans_release(pdev, swdma_dev);
 	switchtec_dma_chans_free(swdma_dev);
 
@@ -1334,6 +1344,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	return 0;
 
 err_chans_release_exit:
+	switchtec_dma_chans_disable(pdev, swdma_dev);
 	switchtec_dma_chans_release(pdev, swdma_dev);
 	switchtec_dma_chans_free(swdma_dev);
 
-- 
2.47.3

[PATCH v5 09/12] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

[Logan Gunthorpe]

dma_async_device_unregister() can drop the last reference on dma_dev
and free swdma_dev synchronously via switchtec_dma_release(), but
switchtec_dma_remove() then uses swdma_dev->bar for iounmap().

Cache bar in a local variable before the unregister call.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index c2803af6589d..3b4ec4ad2bbe 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1393,6 +1393,7 @@ static int switchtec_dma_probe(struct pci_dev *pdev,
 static void switchtec_dma_remove(struct pci_dev *pdev)
 {
 	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
+	void __iomem *bar = swdma_dev->bar;
 
 	switchtec_dma_chans_release(pdev, swdma_dev);
 
@@ -1405,7 +1406,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
 
 	dma_async_device_unregister(&swdma_dev->dma_dev);
 
-	iounmap(swdma_dev->bar);
+	iounmap(bar);
 	pci_release_mem_regions(pdev);
 	pci_disable_device(pdev);
 }
-- 
2.47.3

[PATCH v5 10/12] dmaengine: ioat: disable relaxed ordering before registering the device

[Logan Gunthorpe]

ioat3_dma_probe() disabled PCIe relaxed ordering after calling
dma_async_device_register(), so if an error occurs and the code jumps
to err_disable_interrupts, the function returns with the device still
registered in the core's dma_device_list while the caller frees the
ioatdma_device struct, leaving a dangling registration that anything
walking the device list can dereference after it's been freed.

Move the capability read/write ahead of dma_async_device_register()
instead. Nothing after registration depends on relaxed ordering
already being disabled, and nothing before it depends on the device
being registered, so this is a plain reordering. It also means every
remaining step after registration can't fail, so there's no need to
ever have to unregister the device once registered.

Fixes: 511deae0261c ("dmaengine: ioatdma: disable relaxed ordering for ioatdma")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org
Acked-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/ioat/init.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 737496391109..a57024c4b066 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1170,15 +1170,6 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 		       ioat_chan->reg_base + IOAT_DCACTRL_OFFSET);
 	}
 
-	err = dma_async_device_register(&ioat_dma->dma_dev);
-	if (err)
-		goto err_disable_interrupts;
-
-	ioat_kobject_add(ioat_dma, &ioat_ktype);
-
-	if (dca)
-		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
-
 	/* disable relaxed ordering */
 	err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16);
 	if (err) {
@@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 		goto err_disable_interrupts;
 	}
 
+	err = dma_async_device_register(&ioat_dma->dma_dev);
+	if (err)
+		goto err_disable_interrupts;
+
+	ioat_kobject_add(ioat_dma, &ioat_ktype);
+
+	if (dca)
+		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
+
 	if (ioat_dma->cap & IOAT_CAP_DPS)
 		writeb(ioat_pending_level + 1,
 		       ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);
-- 
2.47.3

[PATCH v5 11/12] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

[Logan Gunthorpe]

Convert the sprintf() calls in the per-channel sysfs attribute show()
functions to sysfs_emit().

Acked-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/ioat/sysfs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/ioat/sysfs.c b/drivers/dma/ioat/sysfs.c
index e796ddb5383f..976134df8108 100644
--- a/drivers/dma/ioat/sysfs.c
+++ b/drivers/dma/ioat/sysfs.c
@@ -24,12 +24,12 @@ static ssize_t cap_show(struct dma_chan *c, char *page)
 {
 	struct dma_device *dma = c->device;
 
-	return sprintf(page, "copy%s%s%s%s%s\n",
-		       dma_has_cap(DMA_PQ, dma->cap_mask) ? " pq" : "",
-		       dma_has_cap(DMA_PQ_VAL, dma->cap_mask) ? " pq_val" : "",
-		       dma_has_cap(DMA_XOR, dma->cap_mask) ? " xor" : "",
-		       dma_has_cap(DMA_XOR_VAL, dma->cap_mask) ? " xor_val" : "",
-		       dma_has_cap(DMA_INTERRUPT, dma->cap_mask) ? " intr" : "");
+	return sysfs_emit(page, "copy%s%s%s%s%s\n",
+		dma_has_cap(DMA_PQ, dma->cap_mask) ? " pq" : "",
+		dma_has_cap(DMA_PQ_VAL, dma->cap_mask) ? " pq_val" : "",
+		dma_has_cap(DMA_XOR, dma->cap_mask) ? " xor" : "",
+		dma_has_cap(DMA_XOR_VAL, dma->cap_mask) ? " xor_val" : "",
+		dma_has_cap(DMA_INTERRUPT, dma->cap_mask) ? " intr" : "");
 
 }
 static const struct ioat_sysfs_entry ioat_cap_attr = __ATTR_RO(cap);
@@ -39,8 +39,8 @@ static ssize_t version_show(struct dma_chan *c, char *page)
 	struct dma_device *dma = c->device;
 	struct ioatdma_device *ioat_dma = to_ioatdma_device(dma);
 
-	return sprintf(page, "%d.%d\n",
-		       ioat_dma->version >> 4, ioat_dma->version & 0xf);
+	return sysfs_emit(page, "%d.%d\n",
+			   ioat_dma->version >> 4, ioat_dma->version & 0xf);
 }
 static const struct ioat_sysfs_entry ioat_version_attr = __ATTR_RO(version);
 
@@ -118,7 +118,7 @@ static ssize_t ring_size_show(struct dma_chan *c, char *page)
 {
 	struct ioatdma_chan *ioat_chan = to_ioat_chan(c);
 
-	return sprintf(page, "%d\n", (1 << ioat_chan->alloc_order) & ~1);
+	return sysfs_emit(page, "%d\n", (1 << ioat_chan->alloc_order) & ~1);
 }
 static const struct ioat_sysfs_entry ring_size_attr = __ATTR_RO(ring_size);
 
@@ -127,7 +127,7 @@ static ssize_t ring_active_show(struct dma_chan *c, char *page)
 	struct ioatdma_chan *ioat_chan = to_ioat_chan(c);
 
 	/* ...taken outside the lock, no need to be precise */
-	return sprintf(page, "%d\n", ioat_ring_active(ioat_chan));
+	return sysfs_emit(page, "%d\n", ioat_ring_active(ioat_chan));
 }
 static const struct ioat_sysfs_entry ring_active_attr = __ATTR_RO(ring_active);
 
@@ -135,7 +135,7 @@ static ssize_t intr_coalesce_show(struct dma_chan *c, char *page)
 {
 	struct ioatdma_chan *ioat_chan = to_ioat_chan(c);
 
-	return sprintf(page, "%d\n", ioat_chan->intr_coalesce);
+	return sysfs_emit(page, "%d\n", ioat_chan->intr_coalesce);
 }
 
 static ssize_t intr_coalesce_store(struct dma_chan *c, const char *page,
-- 
2.47.3

[PATCH v5 12/12] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

[Logan Gunthorpe]

plx_dma_create() registered the interrupt handler with request_irq()
before initializing plxdev->bar. If the device raised an interrupt in
that window, plx_dma_isr() would dereference the still-NULL bar.

Move the bar assignment ahead of request_irq() so everything the
handler can touch is initialized before it can run.

Reported-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
Reported-by: Kyungwook Boo <bookyungwook@gmail.com>
Link: https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr
Fixes: c2dbcaa8c672 ("dmaengine: plx-dma: Implement hardware initialization and cleanup")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 84941a918b01..409898e92c32 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -504,17 +504,17 @@ static int plx_dma_create(struct pci_dev *pdev)
 	if (!plxdev)
 		return -ENOMEM;
 
-	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
-			 KBUILD_MODNAME, plxdev);
-	if (rc)
-		goto free_plx;
-
 	spin_lock_init(&plxdev->ring_lock);
 	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);
 
 	RCU_INIT_POINTER(plxdev->pdev, pdev);
 	plxdev->bar = pcim_iomap_table(pdev)[0];
 
+	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
+			 KBUILD_MODNAME, plxdev);
+	if (rc)
+		goto free_plx;
+
 	dma = &plxdev->dma_dev;
 	INIT_LIST_HEAD(&dma->channels);
 	dma_cap_set(DMA_MEMCPY, dma->cap_mask);
-- 
2.47.3

Re: [PATCH v5 01/12] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

[sashiko-bot]

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

Pre-existing issues:
- [High] switchtec_dma_prep_desc() acquires and returns with a spinlock held, violating the dmaengine API and causing inevitable deadlocks.
--

commit 35f30174e5a3e6b3e6f62a9f94873f8da95cfc36
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

This commit prevents a double-free in switchtec_dma_free_desc() by setting
hw_sq, hw_cq, and desc_ring entries to NULL after freeing them. This fixes
an issue during channel resource allocation failures where unwind paths
could attempt to free already-released pointers.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index c133535d37656..a10818efba4e5 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
>  	if (swdma_chan->hw_sq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
> +	swdma_chan->hw_sq = NULL;
>  
>  	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
[ ... ]

While reviewing this file, I noticed a separate issue in
switchtec_dma_prep_desc():

drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
	spin_lock_bh(&swdma_chan->submit_lock);

	if (!swdma_chan->ring_active)
		goto err_unlock;
	...
	/* return with the lock held, it will be released in tx_submit */

	return &desc->txd;
}

[Severity: High]
This isn't a bug introduced by this patch, but does returning with the
submit_lock held violate the dmaengine API contract and cause deadlocks?

The dmaengine API allows clients to batch multiple descriptor preparations
before submitting them. If a client calls the prep function consecutively,
would the second call attempt to re-acquire the same non-recursive spinlock
and deadlock the CPU?

Additionally, the dmaengine API allows aborting and dropping descriptors
on error before submission. If a prepared descriptor is discarded on a
client error path, would the lock remain permanently held?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=1

Re: [PATCH v5 02/12] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[sashiko-bot]

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

Pre-existing issues:
- [High] switchtec_dma_remove() tears down hardware resources before unregistering the DMA device, exposing a race window where new clients can request channels on a dying device.
- [High] The driver holds a spinlock between dmaengine prep_desc and tx_submit, violating dmaengine semantics and risking deadlocks.
- [High] The driver performs massive allocations using GFP_NOWAIT in process context, which is highly likely to fail under memory pressure.
--

commit dac58125b9c3b4ce36e518a88347266e281e5a57
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

This commit fixes a resource leak in switchtec_dma_alloc_chan_resources()
where descriptor rings and coherent DMA memory were not freed on failure.
It adds an error-unwind path to properly disable the channel and free the
resources since the dmaengine core does not handle this automatically.

Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index a10818efba4e5..e55fe9ff7e2c5 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

While reviewing this code, I noticed a few pre-existing issues in the driver
that might need attention:

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
...
> 	switchtec_dma_chans_release(pdev, swdma_dev);
> 
> 	rcu_assign_pointer(swdma_dev->pdev, NULL);
> 	synchronize_rcu();
> 
> 	pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
> 
> 	pci_free_irq_vectors(pdev);
> 
> 	dma_async_device_unregister(&swdma_dev->dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown order expose
a race window where new clients can request channels on a dying device?

If a concurrent client requests a channel via dma_request_chan() during
switchtec_dma_remove(), the DMA core will route it to this device because it
has not yet been unregistered.

The client will then trigger switchtec_dma_alloc_chan_resources(), which
re-activates a channel whose IRQs and tasklets were already permanently
destroyed by switchtec_dma_chans_release().

Should dma_async_device_unregister() be called first to safely detach the
device from the subsystem before destroying local hardware resources?


drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
...
> 	spin_lock_bh(&swdma_chan->submit_lock);
> 
> 	if (!swdma_chan->ring_active)
> 		goto err_unlock;

and in switchtec_dma_tx_submit():

> 	spin_unlock_bh(&swdma_chan->submit_lock);
> 
> 	return cookie;

[Severity: High]
This is a pre-existing issue, but could holding submit_lock across dmaengine
API boundaries risk deadlocks?

If a client attempts to prepare multiple descriptors sequentially before
submitting them (batching prep operations), wouldn't the second call to
dmaengine_prep_dma_memcpy() attempt to acquire the exact same lock and
deadlock?

Furthermore, this leaves the caller's CPU permanently locked with softirqs
disabled if the client prepares a descriptor but aborts before submitting it.
The dmaengine API explicitly allows batching of prep operations.


drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
...
> 	swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
> 					       &swdma_chan->dma_addr_sq,
> 					       GFP_NOWAIT);
...
> 	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
> 		desc = kzalloc_obj(*desc, GFP_NOWAIT);
> 		if (!desc) {
> 			rc = -ENOMEM;
> 			goto free_and_exit;
> 		}

[Severity: High]
This isn't a bug introduced by this patch, but could these allocations use
GFP_KERNEL instead?

Because switchtec_dma_alloc_chan_resources() is called in process context and
is allowed to sleep, using GFP_NOWAIT here performs massive allocations
without direct reclaim.

The driver attempts to allocate 1MB of coherent DMA memory and performs 32,768
consecutive atomic allocations, which seems highly likely to fail on
fragmented or loaded systems under memory pressure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=2

Re: [PATCH v5 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

[sashiko-bot]

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

New issues:
- [High] Clearing DMA base registers to 0 redirects runaway DMA to physical address 0, causing critical memory corruption.
- [High] Unconditional memory leak of DMA descriptor rings during device hot-unplug because -ENODEV is treated as a halt timeout.
- [High] Dangling pointers allow intentionally leaked DMA memory to be accidentally freed on subsequent channel allocation failure, risking hardware memory corruption.

Pre-existing issues:
- [Critical] Client data buffers can be trashed by runaway hardware because pending descriptors are aborted and unmapped despite a halt failure.
--

commit 76ff5cafd116d5c109fd77135de2f45efb9196b7
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

Attempt the register clear regardless of whether the halt was successful,
and have switchtec_dma_chan_stop() return the halt result. In case of failure,
descriptor rings are intentionally leaked to prevent buggy hardware from
trashing freed memory.

Link: https://lore.kernel.org/dmaengine/20260721162531.BA01A1F01560@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index e55fe9ff7e2c5..e2bb65237d2c9 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -543,26 +543,33 @@ switchtec_dma_abort_desc(struct switchtec_dma_chan *swdma_chan, int force)
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  }
>  
> -static void switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
> +static int switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
>  {
> +	struct pci_dev *pdev;
>  	int rc;
>  
>  	rc = halt_channel(swdma_chan);
> -	if (rc)
> -		return;
>  
>  	rcu_read_lock();
> -	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
> +	pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
> +	if (!pdev) {
>  		rcu_read_unlock();
> -		return;
> +		return rc;
>  	}
>  
> +	if (rc)
> +		pci_err(pdev,
> +			"Channel %d halt timed out, clearing DMA base registers anyway\n",
> +			swdma_chan->index);
> +
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_hi);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_hi);

[Severity: High]
If the channel fails to halt (times out), writing 0 to sq_base and cq_base
will point the hardware's queues to physical address 0.

If the active hardware fetches a descriptor or writes a completion, does this
redirect runaway DMA to corrupt physical memory at address 0 instead of writing
to the intentionally leaked descriptor rings?

>  
>  	rcu_read_unlock();
> +
> +	return rc;
>  }

[ ... ]

> @@ -1050,6 +1057,7 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct switchtec_dma_chan *swdma_chan =
>  		container_of(chan, struct switchtec_dma_chan, dma_chan);
> +	int rc;
>  
>  	spin_lock_bh(&swdma_chan->submit_lock);
>  	swdma_chan->ring_active = false;
> @@ -1059,9 +1067,10 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  
> -	switchtec_dma_chan_stop(swdma_chan);
> +	rc = switchtec_dma_chan_stop(swdma_chan);
>  	switchtec_dma_abort_desc(swdma_chan, 0);

[Severity: Critical]
This is a pre-existing issue, but does calling switchtec_dma_abort_desc()
here defeat the purpose of leaking the descriptor memory?

If switchtec_dma_chan_stop() times out due to a hardware hang, this code
still unconditionally calls dma_descriptor_unmap() and invokes client
callbacks with DMA_TRANS_ABORTED:

switchtec_dma_abort_desc()
    ...
    dma_cookie_complete(&desc->txd);
    dma_descriptor_unmap(&desc->txd);
    if (!force)
        dmaengine_desc_get_callback_invoke(&desc->txd, &res);

Could this cause clients to free or reuse their data buffers while the
runaway hardware is still actively writing to or reading from them?

> -	switchtec_dma_free_desc(swdma_chan);
> +	if (!rc)
> +		switchtec_dma_free_desc(swdma_chan);

[Severity: High]
If the PCI device is removed (e.g. hot-unplug), pdev is set to NULL, and
switchtec_dma_chan_stop() detects this and returns -ENODEV.

Does treating -ENODEV as a hardware halt failure here intentionally skip
switchtec_dma_free_desc(), causing a permanent memory leak of the DMA
descriptor rings during normal device unbind?

[Severity: High]
If switchtec_dma_chan_stop() times out, the descriptor memory is
intentionally leaked by skipping switchtec_dma_free_desc(), but the pointers
(hw_sq, hw_cq, desc_ring) are not cleared to NULL in the struct.

If a client subsequently allocates the channel again, and any part of the
new allocation fails:

switchtec_dma_alloc_desc()
    swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
                                           &swdma_chan->dma_addr_sq,
                                           GFP_NOWAIT);
    if (!swdma_chan->hw_sq) {
        rc = -ENOMEM;
        goto free_and_exit;
    }

Would the error path (free_and_exit) call switchtec_dma_free_desc() and
erroneously free the old, leaked memory that the runaway hardware might
still be writing to?

>  
>  	disable_channel(swdma_chan);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=3

Re: [PATCH v5 04/12] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Zeroing DMA base registers when hardware fails to halt redirects runaway DMA to physical address 0x0.
- [High] Intentional descriptor leak leaves stale pointers intact, causing Use-After-Free of active DMA memory during allocation retries.
- [High] Mishandling of `-ENODEV` causes a deterministic, permanent memory leak of descriptor rings upon device hot-unplug.
- [High] The mitigation for `unhalt_channel()` timeout is completely bypassed because `halt_channel()` instantly returns success.
- [High] The intentional memory leak on channel initialization timeout creates an unbounded memory exhaustion vector if allocation is retried.
- [Medium] The error path needlessly leaks purely software-backed structures (`desc_ring`) when attempting to leak hardware rings.
--

commit 28f07bd0419a79f1993daba83975c013aac2cba4
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

The error-unwind path called disable_channel() before freeing the
descriptor rings, but that only clears the enable bit with an
unflushed write -- it doesn't halt the channel or clear its DMA base
address registers. If unhalt_channel() timed out, the channel's actual
state is unknown at that point, so nothing guarantees the hardware
isn't still touching the rings when they're freed.

Link: https://lore.kernel.org/dmaengine/20260717223647.F0A051F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260727185101.D221C1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index e2bb65237d2c9..2d5ba194c99a1 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1047,6 +1047,10 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  err_disable_channel:
> +	if (switchtec_dma_chan_stop(swdma_chan)) {

[Severity: High]
Is the mitigation for an unhalt_channel() timeout bypassed here?

If unhalt_channel() times out, the SWITCHTEC_CHAN_STS_HALTED bit remains 1.

When switchtec_dma_chan_stop() subsequently calls halt_channel():

drivers/dma/switchtec_dma.c:halt_channel() {
    ...
	ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
    ...
}

Because the bit is already 1, wait_for_chan_status() instantly returns 0.

Does this cause the error path to mistakenly perceive a successful halt and
proceed to free the descriptor rings while the hardware state is unknown?

[Severity: Critical]
Does zeroing the DMA base registers when the hardware fails to halt redirect
runaway DMA to physical address 0x0?

Looking at switchtec_dma_chan_stop():

drivers/dma/switchtec_dma.c:switchtec_dma_chan_stop() {
    ...
	if (rc)
		pci_err(pdev,
			"Channel %d halt timed out, clearing DMA base registers anyway\n",
			swdma_chan->index);

	writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
    ...
}

If the hardware is unhalted and active, and it dynamically fetches the zeroed
base addresses, can it begin performing DMA at physical address 0x0, causing
severe memory corruption?

> +		disable_channel(swdma_chan);
> +		return rc;

[Severity: High]
Does this intentional memory leak on timeout create an unbounded memory
exhaustion vector?

If a client driver repeatedly retries alloc_chan_resources after it fails
with -ETIMEDOUT, could each attempt continuously leak kernel memory until the
system is exhausted?

[Severity: Medium]
Does this error path needlessly leak purely software-backed structures?

While returning early prevents buggy hardware from writing to the physical
rings, this broadly skips freeing desc_ring.

Since the hardware has no knowledge of this purely software-backed array, can
this leak kernel memory unnecessarily?

[Severity: High]
Can this intentional descriptor leak leave stale pointers intact, leading to
a use-after-free during allocation retries?

If a client retries alloc_chan_resources after a timeout, the pointers for
hw_cq and desc_ring are not cleared.

When switchtec_dma_alloc_desc() is called again:

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
    ...
	swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
					       &swdma_chan->dma_addr_sq,
					       GFP_NOWAIT);
	if (!swdma_chan->hw_sq) {
		rc = -ENOMEM;
		goto free_and_exit;
	}
    ...
}

If the coherent allocation for hw_sq fails (e.g., -ENOMEM), the error path
calls switchtec_dma_free_desc().

Since hw_cq still holds the stale pointer to the leaked ring, would it be
erroneously freed while the unhalted hardware might still be actively writing
to it?

> +	}
>  	disable_channel(swdma_chan);
>  err_free_desc:
>  	switchtec_dma_free_desc(swdma_chan);

[Severity: High]
Does mishandling -ENODEV cause a permanent memory leak of the descriptor
rings upon device hot-unplug?

Looking at switchtec_dma_free_chan_resources():

drivers/dma/switchtec_dma.c:switchtec_dma_free_chan_resources() {
    ...
	rc = switchtec_dma_chan_stop(swdma_chan);
	switchtec_dma_abort_desc(swdma_chan, 0);
	if (!rc)
		switchtec_dma_free_desc(swdma_chan);
    ...
}

If the device is removed and switchtec_dma_chan_stop() returns -ENODEV from
halt_channel(), does the !rc check evaluate to false and permanently skip
freeing the descriptors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=4

Re: [PATCH v5 05/12] dmaengine: switchtec-dma: fix channel leak on registration failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized list head iteration in ISR causing NULL pointer dereference.
--

commit bbb6888bb23afe2e2ce4a0388779ab6fa86e4ce3
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix channel leak on registration failure

This patch introduces a helper function to free DMA channel memory and ensures
it is called correctly during registration failures and cleanup. It also updates
the channel count on initialization failure to allow safe cleanup of partially
allocated channels.

Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260727190604.397DB1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 2d5ba194c99a1..f71c64c137e83 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
[ ... ]
> @@ -1329,6 +1335,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
> +	switchtec_dma_chans_free(swdma_dev);
>  
>  err_exit:
>  	if (swdma_dev->chan_status_irq)

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition
during device initialization earlier in switchtec_dma_create().

The IRQ is requested and enabled before the channels list head is
initialized:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
	...
	rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
			     swdma_dev, KBUILD_MODNAME);
	...
	chan_cnt = switchtec_dma_chans_enumerate(swdma_dev, pdev, chan_cnt);
	...
}

If a pending device interrupt or a spurious interrupt fires immediately
after pci_request_irq() is called, switchtec_dma_chan_status_isr() will
execute.

In the ISR, list_for_each_entry() iterates over dma_dev->channels:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
	...
	list_for_each_entry(chan, &dma_dev->channels, device_node) {
	...
}

Since switchtec_dma_chans_enumerate() hasn't called INIT_LIST_HEAD()
on dma_dev->channels yet, the list head remains zero-initialized by
the earlier kzalloc().

Could this cause a NULL pointer dereference when the ISR attempts to
iterate over the uninitialized list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=5

Re: [PATCH v5 06/12] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

[sashiko-bot]

> switchtec_dma_chans_release() always returned 0, and no caller
> checked it.
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=6

Re: [PATCH v5 07/12] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked return value of ioremap() can lead to a NULL pointer dereference.
- [High] Interrupt handler is registered before the DMA channels list is initialized, risking a NULL pointer dereference.
- [High] DMA device is unregistered after channels and IRQs are torn down.
- [High] Holding a spinlock while busy-waiting for up to 100 milliseconds disables preemption for an unacceptable duration, potentially causing RCU stalls or soft lockups.
- [Medium] The return value of dma_set_mask_and_coherent() is ignored.
- [Medium] Missing put_device() causes reference count leak on the device structure in the error path of switchtec_dma_create().
--

commit 789f2f9ebf1e9dd9f52e2557e403d93f922154be
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

This commit ensures that chan_status_irq is properly cleaned up on error by
using pci_free_irq() instead of free_irq(). It also initializes the IRQ vector
index to -1 so that a valid index of 0 is no longer skipped during cleanup.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index c30530ba78caf..3068d9de4e6d3 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1262,6 +1262,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	if (!swdma_dev)
>  		return -ENOMEM;
>  
> +	swdma_dev->chan_status_irq = -1;
> +
>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));

[Severity: High]
This is a pre-existing issue, but shouldn't we check the return value of 
ioremap() here?

If ioremap() fails, swdma_dev->bar will be NULL. The code immediately proceeds
to dereference it via readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC). Can 
this lead to a NULL pointer dereference and kernel panic during device probe?

[ ... ]

>  err_exit:
> -	if (swdma_dev->chan_status_irq)
> -		free_irq(swdma_dev->chan_status_irq, swdma_dev);
> +	if (swdma_dev->chan_status_irq >= 0)
> +		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
>  
>  	iounmap(swdma_dev->bar);
>  	kfree(swdma_dev);
>  	return rc;

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the device 
reference?

Earlier in switchtec_dma_create(), a reference is acquired:

    dma->dev = get_device(&pdev->dev);

If dma_async_device_register() fails, the code jumps to 
err_chans_release_exit and eventually falls through to err_exit, but 
put_device(dma->dev) is never called. Since the device wasn't successfully 
registered, switchtec_dma_release() won't be called to drop this reference.

In drivers/dma/switchtec_dma.c:switchtec_dma_probe():

    dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
    rc = pci_request_mem_regions(pdev, KBUILD_MODNAME);

[Severity: Medium]
This is a pre-existing issue, but is the return value of 
dma_set_mask_and_coherent() intentionally ignored here?

If the platform or IOMMU rejects the 64-bit mask, this function can fail. 
Should the driver check the return value and gracefully fallback to 32-bit
DMA or fail the probe to prevent silent data corruption or IOMMU faults?

In drivers/dma/switchtec_dma.c:switchtec_dma_create():

    rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
                         swdma_dev, KBUILD_MODNAME);
    if (rc)
        goto err_exit;

[Severity: High]
This is a pre-existing issue, but does registering the interrupt handler here 
risk a NULL pointer dereference?

Later in switchtec_dma_create(), switchtec_dma_chans_enumerate() is called, 
which initializes the dma->channels list:

    INIT_LIST_HEAD(&dma->channels);

At the time of IRQ registration, swdma_dev was allocated with kzalloc_obj(), 
meaning dma_dev->channels.next is NULL. If an interrupt fires immediately 
after registration, switchtec_dma_chan_status_isr() will execute:

    list_for_each_entry(chan, &dma_dev->channels, device_node) {

Since the list head is zeroed, this loop could underflow and dereference an 
invalid pointer, leading to a kernel oops.

In drivers/dma/switchtec_dma.c:switchtec_dma_remove():

    switchtec_dma_chans_release(pdev, swdma_dev);
    
    rcu_assign_pointer(swdma_dev->pdev, NULL);
    synchronize_rcu();
    
    pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
    pci_free_irq_vectors(pdev);
    
    dma_async_device_unregister(&swdma_dev->dma_dev);

[Severity: High]
This is a pre-existing issue, but is the DMA device unregistered too late in
the teardown process?

The driver initiates hardware teardown by stopping channels and freeing IRQs
before calling dma_async_device_unregister(). Tearing down hardware and IRQs
while the device is still exposed to the DMA engine subsystem can lead to 
use-after-free, hanging clients, or hardware access races if a client 
actively uses the DMA device during removal. Should the subsystem be 
unregistered first?

In drivers/dma/switchtec_dma.c:wait_for_chan_status():

    return readl_poll_timeout_atomic(&chan_hw->status, status,
                                     (set && (status & mask)) ||
                                     (!set && !(status & mask)),
                                     10, 100 * USEC_PER_MSEC);

[Severity: High]
This is a pre-existing issue, but could this atomic poll loop cause soft
lockups or RCU stalls?

This function is called from halt_channel() while holding a spinlock:

    spin_lock(&swdma_chan->hw_ctrl_lock);
    writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
    ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
    spin_unlock(&swdma_chan->hw_ctrl_lock);

Since holding a spinlock disables preemption, blocking the CPU for up to 100
milliseconds without preemption could severely degrade system latency.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=7

Re: [PATCH v5 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

[Logan Gunthorpe]

On 2026-08-31 15:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Clearing DMA base registers to 0 redirects runaway DMA to physical address 0, causing critical memory corruption.
> - [High] Unconditional memory leak of DMA descriptor rings during device hot-unplug because -ENODEV is treated as a halt timeout.
> - [High] Dangling pointers allow intentionally leaked DMA memory to be accidentally freed on subsequent channel allocation failure, risking hardware memory corruption.

Ugg, ok yes. Seems patches 3 and 4 still have issues that I thought were
resolved. I'll drop them for now and resend a v6 without them tomorrow.

Sorry, for the noise. I was wrong about this one.

Logan

Re: [PATCH v5 08/12] dmaengine: switchtec-dma: disable channels before freeing on registration failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing check for ioremap failure in switchtec_dma_create() leads to NULL pointer dereference.
- [High] The channel status IRQ handler dereferences uninitialized structures if the IRQ fires before dma_async_device_register() completes.
- [High] Missing bounds check on untrusted `chan_cnt` read from the PCI device allows out-of-bounds MMIO pointer arithmetic and memory writes.
--

commit 9d90eebe0f109c6aab9cf75114365b10451e1028
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: disable channels before freeing on registration failure

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels, the channel status IRQ is still live while the channels
are released and freed. Add switchtec_dma_chans_disable() to free the
channel status IRQ before releasing and freeing channels in both error paths.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260721162822.05CDD1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));
>  
>  	RCU_INIT_POINTER(swdma_dev->pdev, pdev);
>  
>  	nr_vecs = pci_msix_vec_count(pdev);
>  	rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
>  	if (rc < 0)
>  		goto err_exit;
>  
>  	irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);

[Severity: High]
This is a pre-existing issue, but does this code cause a NULL pointer
dereference if ioremap() fails? The swdma_dev->bar pointer does not appear
to be checked before being unconditionally dereferenced by readw() during
device probe.

>  	pci_dbg(pdev, "Channel pause irq vector %d\n", irq);
>  
>  	rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
>  			     swdma_dev, KBUILD_MODNAME);
>  	if (rc)
>  		goto err_exit;

[Severity: High]
This is a pre-existing issue, but does this request the IRQ too early? If
the hardware asserts the IRQ immediately, the ISR will run before the
channels are fully initialized.

Looking at switchtec_dma_chan_status_isr():

	list_for_each_entry(chan, &dma_dev->channels, device_node) {
		swdma_chan = container_of(chan, struct switchtec_dma_chan,
					  dma_chan);
		chan_dev = &swdma_chan->dma_chan.dev->device;
		chan_hw = swdma_chan->mmio_chan_hw;

Could this dereference an invalid pointer if the channels list is empty or
if chan->dev is not yet initialized by dma_async_device_register()?

>  	swdma_dev->chan_status_irq = irq;
>  
>  	chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);
>  	if (!chan_cnt) {
>  		pci_err(pdev, "No channel configured.\n");
>  		rc = -ENXIO;
>  		goto err_exit;
>  	}
>  
>  	chan_cnt = switchtec_dma_chans_enumerate(swdma_dev, pdev, chan_cnt);

[Severity: High]
This is a pre-existing issue, but is there a missing upper bounds check on
chan_cnt? Since chan_cnt is read directly from hardware, an untrusted
device could provide a very large count.

In switchtec_dma_chan_init(), this unvalidated count is used for pointer
arithmetic:

	swdma_chan->mmio_chan_fw =
		swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
		i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;
	swdma_chan->mmio_chan_hw =
		swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CTRL_OFFSET +
		i * SWITCHTEC_DMA_CHAN_HW_REGS_SIZE;

Could this allow out-of-bounds MMIO pointer arithmetic and memory writes
outside the valid bar resource window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=8

Re: [PATCH v5 09/12] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

[sashiko-bot]

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

Pre-existing issues:
- [High] DMA descriptor memory and coherent allocations are permanently leaked when channels are released or allocation fails after the PCI device is hot-removed.
- [High] Missing NULL check for ioremap return value can lead to a NULL pointer dereference.
- [High] Tearing down hardware state before RCU synchronization allows concurrent DMA submissions to trigger hardware faults.
- [High] Missing validation of hardware-provided chan_cnt against the mapped BAR length.
--

commit 0f74dee750d8feb49b287883e72b10e222134798
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

dma_async_device_unregister() can drop the last reference on dma_dev and free
swdma_dev synchronously. This patch caches the bar locally before unregistering
to avoid a use-after-free during iounmap().

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index c2803af6589d4..3b4ec4ad2bbe9 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1393,6 +1393,7 @@ static int switchtec_dma_probe(struct pci_dev *pdev,
>  static void switchtec_dma_remove(struct pci_dev *pdev)
>  {
>  	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> +	void __iomem *bar = swdma_dev->bar;
>  
>  	switchtec_dma_chans_release(pdev, swdma_dev);

[Severity: High]
This is a pre-existing issue, but does tearing down the hardware channels here
before RCU synchronization leave a window for concurrent submissions to trigger
hardware faults?

If switchtec_dma_chans_release() zeroes the DMA base registers (via
switchtec_dma_chan_stop()), this happens before swdma_dev->pdev is cleared and
before dma_async_device_unregister() removes the channels from use.

Could a concurrent client in switchtec_dma_issue_pending() check that
pdev != NULL under rcu_read_lock(), and write to the sq_tail register?

switchtec_dma_issue_pending() {
    ...
    spin_lock_bh(&swdma_chan->submit_lock);
    writew(swdma_chan->head, &swdma_chan->mmio_chan_hw->sq_tail);
    spin_unlock_bh(&swdma_chan->submit_lock);
    ...
}

Would this instruct the hardware to fetch descriptors from the now-zeroed base
address, causing DMA or IOMMU faults?

[ ... ]

> @@ -1405,7 +1406,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
>  
>  	dma_async_device_unregister(&swdma_dev->dma_dev);
>  
> -	iounmap(swdma_dev->bar);
> +	iounmap(bar);
>  	pci_release_mem_regions(pdev);
>  	pci_disable_device(pdev);
>  }

While reviewing this driver, I noticed a few other pre-existing issues.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
switchtec_dma_free_chan_resources() and switchtec_dma_alloc_chan_resources()
permanently leak DMA descriptor memory and coherent allocations?

If the PCI device is hot-unplugged, switchtec_dma_chan_stop() can return
-ENODEV. In switchtec_dma_free_chan_resources(), the code conditionally skips
freeing the descriptors:

    if (!rc)
        switchtec_dma_free_desc(swdma_chan);

A similar early return exists in switchtec_dma_alloc_chan_resources():

    if (switchtec_dma_chan_stop(swdma_chan)) {
        disable_channel(swdma_chan);
        return rc;
    }

Do these paths leave the hardware queues (hw_sq and hw_cq) permanently leaked
when allocating or releasing channels after the device is removed?

[Severity: High]
This is a pre-existing issue, but is there a missing check for ioremap()
failures in switchtec_dma_create()?

    swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
                             pci_resource_len(pdev, 0));

Shortly after, the bar pointer is unconditionally dereferenced:

    irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);

Could this cause a kernel oops during device initialization if ioremap() fails?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver validate the
hardware-provided channel count against the mapped BAR size?

In switchtec_dma_create(), chan_cnt is read directly from hardware:

    chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);

Later, in switchtec_dma_chan_init(), this untrusted count is used for pointer
arithmetic to calculate the MMIO pointers:

    swdma_chan->mmio_chan_fw =
        swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
        i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;

Could a malicious or malfunctioning PCIe endpoint supply an arbitrarily large
chan_cnt, leading to out-of-bounds MMIO accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=9

Re: [PATCH v5 10/12] dmaengine: ioat: disable relaxed ordering before registering the device

[sashiko-bot]

> ioat3_dma_probe() disabled PCIe relaxed ordering after calling
> dma_async_device_register(), so if an error occurs and the code jumps
> to err_disable_interrupts, the function returns with the device still
> registered in the core's dma_device_list while the caller frees the
> ioatdma_device struct, leaving a dangling registration that anything
> walking the device list can dereference after it's been freed.
> 
> Move the capability read/write ahead of dma_async_device_register()
> instead. Nothing after registration depends on relaxed ordering
> already being disabled, and nothing before it depends on the device
> being registered, so this is a plain reordering. It also means every
> remaining step after registration can't fail, so there's no need to
> ever have to unregister the device once registered.
> 
> Fixes: 511deae0261c ("dmaengine: ioatdma: disable relaxed ordering for ioatdma")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org
> Acked-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=10

Re: [PATCH v5 11/12] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

[sashiko-bot]

> Convert the sprintf() calls in the per-channel sysfs attribute show()
> functions to sysfs_emit().
> 
> Acked-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=11

Re: [PATCH v5 12/12] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

[sashiko-bot]

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

Pre-existing issues:
- [High] Softirq preemption deadlock in `plx_dma_process_desc()` due to using `spin_lock()` instead of `spin_lock_bh()`.
- [High] Recursive deadlock in `plx_dma_process_desc()` when a client callback submits a new DMA transaction.
- [High] `plx_dma_prep_memcpy()` acquires a spinlock and returns without releasing it, violating the DMA API contract.
--

commit eba3f5594a9491de8ee08b096a4a7e95dd48da19
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

This commit prevents a NULL pointer dereference in the interrupt handler.
It achieves this by moving the assignment of plxdev->bar ahead of
request_irq() during hardware initialization.

Link: https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 84941a918b01f..409898e92c320 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c
[ ... ]
> static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
> {
> 	struct dmaengine_result res;
> 	struct plx_dma_desc *desc;
> 	u32 flags;
> 
> 	spin_lock(&plxdev->ring_lock);

[Severity: High]
This is a pre-existing issue, but does using spin_lock() here instead of
spin_lock_bh() leave us vulnerable to a softirq preemption deadlock?

If plx_dma_tx_status() is called from process context, it will call
plx_dma_process_desc() and acquire this lock while leaving softirqs enabled.
If a hardware interrupt occurs on the same CPU while the lock is held,
plx_dma_isr() will schedule the plx_dma_desc_task tasklet.

Upon return from the interrupt, the softirq executes the tasklet, which calls
plx_dma_process_desc() again. Can this result in a recursive deadlock when it
attempts to acquire the already-held ring_lock?

[ ... ]
> 		dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
This isn't a bug introduced by this patch, but does invoking the client
callback while holding the internal spinlock expose us to a recursive deadlock?

It is a common DMA engine pattern for client callbacks to chain descriptor
submissions. If the callback submits a new DMA transaction, it will call:
plx_dma_prep_memcpy() -> spin_lock_bh(&plxdev->ring_lock)

Since the lock is already actively held by plx_dma_process_desc() on the same
CPU, will this cause an immediate deadlock?

[ ... ]
> static struct dma_async_tx_descriptor *plx_dma_prep_memcpy(struct dma_chan *c,
> 		dma_addr_t dma_dst, dma_addr_t dma_src, size_t len,
> 		unsigned long flags)
> 	__acquires(plxdev->ring_lock)
> {
> 	struct plx_dma_dev *plxdev = chan_to_plx_dma_dev(c);
> 	struct plx_dma_desc *plxdesc;
> 
> 	spin_lock_bh(&plxdev->ring_lock);
[ ... ]
> 	/* return with the lock held, it will be released in tx_submit */
> 
> 	return &plxdesc->txd;

[Severity: High]
This is a pre-existing issue, but does returning to the caller with the
spinlock held and bottom halves disabled violate the DMA API contract?

If a client driver prepares multiple descriptors sequentially, such as calling
dmaengine_prep_dma_memcpy() twice before submitting, won't it deadlock on the
second call?

Additionally, if the client encounters an error between preparation and
submission and never calls tx_submit(), will the lock be permanently leaked,
leaving softirqs disabled on that CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=12