[PATCH v6 00/10] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[PATCH v6 00/10] 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.

After getting more Sashiko feedback on the two problematic patches I
have to eat crow and appologize. Frank was correct about them and I
was a bit too stubborn. Sorry about that. I have dropped those two
patches in this series so hopefully it can go in quickly.

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 v5:
 * Dropped the two patches that skipped freeing the descriptor rings
   when the channel could not be confirmed halted ("always clear DMA
   base registers on chan_stop()" and "halt channel on
   alloc_chan_resources error"), per Frank's recommendation. All
   remaining patches are unchanged.

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 (10):
  dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()
  dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources
  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 | 80 +++++++++++++++++++++++++++----------
 4 files changed, 84 insertions(+), 46 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
--
2.47.3

[PATCH v6 01/10] 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

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

[sashiko-bot]

> 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>

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

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

[PATCH v6 02/10] 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition in switchtec_dma_remove allows concurrent channel allocations on a broken channel.
--

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

dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

This commit adds an error-unwind path to switchtec_dma_alloc_chan_resources()
to prevent resource leaks when channel initialization fails. It ensures
descriptor rings are freed and the channel is disabled and marked inactive
on error.

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

[ ... ]

> @@ -1007,7 +1007,8 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	rcu_read_lock();
>  	if (!rcu_dereference(swdma_dev->pdev)) {

[Severity: High]
This isn't a bug introduced by this patch, but is there a pre-existing race
condition here between channel allocation and device removal?

During device unbind or hot-unplug, switchtec_dma_remove() calls
switchtec_dma_chans_release(), which frees the channel IRQs and kills
tasklets, then clears swdma_dev->pdev:

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);
}

Because dma_async_device_unregister() happens much later, a concurrent
client calling dma_request_chan() can still invoke
switchtec_dma_alloc_chan_resources(). If this executes before
swdma_dev->pdev is set to NULL, it successfully allocates the channel and
sets ring_active = true, unaware that the IRQ and tasklet were already
destroyed.

Could this cause the client to hang waiting for a freed IRQ, or trigger a
system crash due to unmapped memory access when switchtec_dma_remove()
subsequently unmaps the hardware registers?

>  		rcu_read_unlock();
> -		return -ENODEV;
> +		rc = -ENODEV;
> +		goto err_ring_inactive;
>  	}

[ ... ]

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

[PATCH v6 03/10] 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 e55fe9ff7e2c..02d7c38363d6 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1175,6 +1175,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)
 {
@@ -1200,7 +1212,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;
 		}
 	}
@@ -1208,10 +1220,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;
 }
@@ -1220,12 +1230,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);
@@ -1316,6 +1322,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

Re: [PATCH v6 03/10] 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] The channel status ISR iterates over `dma_dev->channels` before the list head is initialized, causing a NULL pointer dereference if the interrupt fires early.
--

commit b0c8d064063987d7e3518dbe0fb57de31b776bde
Author: Logan Gunthorpe <logang@deltatee.com>
Date:   Wed Sep 2 06:21:45 2026 +0000

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

This patch addresses a memory leak during initialization failure by
introducing a switchtec_dma_chans_free() helper. It ensures channel
memory is cleanly freed and removes channels from the DMA device list to
prevent the ISR from dereferencing freed memory.

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 e55fe9ff7e2c5..02d7c38363d68 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1316,6 +1322,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 isn't a bug introduced by this patch, but since we are modifying the
cleanup and setup paths here, could this code be vulnerable to a race 
condition with the interrupt handler?

In switchtec_dma_create(), we request and unmask the interrupt before the
channels are enumerated:

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

However, the dma->channels list head isn't initialized until later when 
switchtec_dma_chans_enumerate() is called:

	INIT_LIST_HEAD(&dma->channels);

If a pending interrupt triggers immediately upon registration, the ISR will
execute and iterate over the uninitialized list:

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

Because swdma_dev was allocated with kzalloc_obj(), dma->channels.next is
NULL at this point. Will this result in a NULL pointer dereference in the
ISR during device probe? Should INIT_LIST_HEAD(&dma->channels) be called
before the interrupt is requested?

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

[PATCH v6 04/10] 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 02d7c38363d6..17bbf85bcb16 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1164,15 +1164,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

Re: [PATCH v6 04/10] 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/20260902062153.31048-1-logang@deltatee.com?part=4

[PATCH v6 05/10] 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 17bbf85bcb16..430b09fa5102 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1249,6 +1249,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));
 
@@ -1323,8 +1325,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

Re: [PATCH v6 05/10] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() in the error paths of switchtec_dma_create() violates strict PCI subsystem guidelines.
- [High] Missing NULL check for the result of ioremap() leads to a potential NULL pointer dereference.
--

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

dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

This commit replaces free_irq() with pci_free_irq() to correctly free MSI-X
vectors. It also changes the default initialization of chan_status_irq to -1
so that index 0 is not 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 17bbf85bcb169..430b09fa5102e 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1249,6 +1249,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 isn't a bug introduced by this patch, but does ioremap() need to be
checked for failure?

If ioremap() fails and returns NULL, swdma_dev->bar is assigned NULL. A bit
later in switchtec_dma_create(), swdma_dev->bar is dereferenced unconditionally
when reading the IRQ vector:

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

Could this result in a NULL pointer dereference?

[ ... ]

> @@ -1323,8 +1325,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);

[Severity: High]
This is a pre-existing issue, but does this error path need to free the IRQ
vectors?

In switchtec_dma_create(), if an error occurs after pci_alloc_irq_vectors()
succeeds, control jumps to err_exit and the function returns without calling
pci_free_irq_vectors().

Does this leak IRQ resources on error paths?

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

[PATCH v6 06/10] 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 430b09fa5102..63f80e2a5b6f 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1185,6 +1185,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)
 {
@@ -1218,6 +1227,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);
 
@@ -1321,6 +1331,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

Re: [PATCH v6 06/10] dmaengine: switchtec-dma: disable channels before freeing on registration failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Premature registration of chan_status_irq allows the ISR to execute before channels are initialized, causing NULL pointer dereferences.
- [High] Missing ioremap() return value check in switchtec_dma_create() can lead to a NULL pointer dereference.
--

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

dmaengine: switchtec-dma: disable channels before freeing on registration failure

This commit addresses an issue in switchtec_dma_create() where an error during
or after channel enumeration could leave the channel status IRQ active. It
introduces switchtec_dma_chans_disable() to free the IRQ before channels are
released and freed, preventing the IRQ from firing mid-teardown.

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
> index 430b09fa5102e..63f80e2a5b6f4 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
[ ... ]
> @@ -1321,6 +1331,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	return 0;
>  
>  err_chans_release_exit:

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference earlier in switchtec_dma_create() in
drivers/dma/switchtec_dma.c.

Can switchtec_dma_create() crash if ioremap() fails? The code maps the PCI BAR
but does not check the return value:

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

If ioremap() returns NULL due to memory exhaustion or an invalid resource, the
subsequent readw() will cause a page fault:

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


[Severity: High]
This is a pre-existing issue, but I noticed another problem with the IRQ
registration in switchtec_dma_create() in drivers/dma/switchtec_dma.c.

Could the interrupt fire before the channel list is initialized? The IRQ is
requested and enabled before dma_dev->channels is initialized:

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

If the hardware asserts the interrupt immediately after it is requested,
switchtec_dma_chan_status_isr() will execute and locklessly iterate over the
zero-initialized dma_dev->channels list:

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

This leads to iterating over a negative offset pointer, and reading
swdma_chan->dma_chan.dev will fault. Furthermore, even if the list was
initialized, dma_chan.dev is only allocated later in
dma_async_device_register(), so an early interrupt would still dereference a
NULL dev pointer.

> +	switchtec_dma_chans_disable(pdev, swdma_dev);
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  	switchtec_dma_chans_free(swdma_dev);
>

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

[PATCH v6 07/10] 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 63f80e2a5b6f..d390949b843c 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1380,6 +1380,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);
 
@@ -1392,7 +1393,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

Re: [PATCH v6 07/10] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked ioremap() return value leading to potential NULL pointer dereference
- [High] Spinlock held across DMA engine API boundaries leading to deadlock on sequential prep
- [High] The driver invokes the client's completion callback with `complete_lock` held, leading to a self-deadlock if the client interacts with the channel.
--

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

dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

Fixes a use-after-free bug in switchtec_dma_remove() where swdma_dev->bar
was used after dma_async_device_unregister() could have freed swdma_dev.
The patch caches bar in a local variable before unregistering the DMA device.

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 63f80e2a5b6f4..d390949b843cd 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1380,6 +1380,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);
>  
> @@ -1392,7 +1393,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 patch, I noticed some pre-existing issues in the same
file.

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
...
	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 is the return value of ioremap() checked
here?

If memory mapping fails and returns NULL, calling readw() on
swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC will result in a NULL pointer
dereference and kernel panic during device probe.


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;
...
}

drivers/dma/switchtec_dma.c:switchtec_dma_tx_submit() {
...
	cookie = dma_cookie_assign(desc);

	spin_unlock_bh(&swdma_chan->submit_lock);

	return cookie;
}

[Severity: High]
This isn't a bug introduced by this patch, but could holding the spinlock
across the DMA engine API boundaries lead to a deadlock on sequential prep
calls?

If a generic client calls prep_memcpy() twice in a row before submitting,
wouldn't the second call self-deadlock the CPU attempting to acquire the
already-held swdma_chan->submit_lock?

Also, does this permanently leak the lock if a client prepares a descriptor
but frees it instead of submitting?


drivers/dma/switchtec_dma.c:switchtec_dma_cleanup_completed() {
...
	do {
		spin_lock_bh(&swdma_chan->complete_lock);
...
		do {
			dma_cookie_complete(&desc->txd);
			dma_descriptor_unmap(&desc->txd);
			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
...
}

[Severity: High]
While this is a pre-existing issue, does invoking the client's completion
callback with complete_lock held expose the system to a self-deadlock?

It is legal for DMA clients to interact with the channel from their
completion callback, such as checking the status via
dmaengine_tx_status() or calling dmaengine_terminate_all().

If a client does this, the call routes to switchtec_dma_tx_status() or
switchtec_dma_terminate_all(), both of which attempt to acquire the same
complete_lock, resulting in a permanent self-deadlock on the same CPU.

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

[PATCH v6 08/10] 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free of `ioat_chan` and `ioatdma_device` structures on `ioat3_dma_probe` error path.
--

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

dmaengine: ioat: disable relaxed ordering before registering the device

This patch reorders the initialization sequence in ioat3_dma_probe(). It
moves the PCIe relaxed ordering configuration before dma_async_device_register()
to prevent leaving a dangling DMA device registration on the error path.

Link: https://lore.kernel.org/dmaengine/20260707165906.249F41F000E9@smtp.kernel.org

> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 737496391109e..a57024c4b066e 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
[ ... ]
> @@ -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;

[Severity: High]
This isn't a bug introduced by this patch, but could this error path lead
to a use-after-free if a shared interrupt fires?

If ioat3_dma_probe() fails here, it returns the error back to
ioat_pci_probe(), which immediately frees the device and its channels
manually:

drivers/dma/ioat/init.c:ioat_pci_probe() {
    ...
    for (i = 0; i < IOAT_MAX_CHANS; i++)
        kfree(device->idx[i]);
    kfree(device);
    ...
}

However, the interrupts were previously registered using devm_request_irq()
in ioat_dma_setup_interrupts():

drivers/dma/ioat/init.c:ioat_dma_setup_interrupts() {
    ...
    err = devm_request_irq(dev, pdev->irq, ioat_dma_do_interrupt,
                           IRQF_SHARED, "ioat-intx", ioat_dma);
    ...
}

Because devm managed resources are not cleaned up until the callbacks run
after the probe function returns, the IRQ handler remains active during
this window.

If a shared interrupt or pending MSI/MSI-X fires before devres cleans up
the IRQ, will the handler dereference the already-freed ioatdma_device and
ioat_chan structures?

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

[PATCH v6 09/10] 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

Re: [PATCH v6 09/10] 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/20260902062153.31048-1-logang@deltatee.com?part=9

[PATCH v6 10/10] 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 v6 10/10] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential deadlock in `plx_dma_process_desc()` due to missing bottom-half disabling.
- [High] `plx_dma_prep_memcpy()` acquires a spinlock and returns with it held, breaking the DMA Engine API contract.
- [High] Unbounded busy-wait loop spanning up to 1000ms inside an RCU read-side critical section.
- [High] Missing `dma_rmb()` in `plx_dma_process_desc()` before processing completion data.
- [Medium] Memory leak of `desc_ring` and its inner descriptor objects in the `plx_dma_alloc_chan_resources()` error path.
--

commit dbf39b912829e6113fcfa314a5a4384936eccf12
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

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.
This commit moves the bar assignment before request_irq() to fix the issue.

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
> @@ -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);

While reviewing this file, I noticed a few pre-existing issues in other
functions that might need attention.

> static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
> {
[ ... ]
> 	spin_lock(&plxdev->ring_lock);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does using spin_lock() here instead of
spin_lock_bh() create a deadlock vulnerability? plx_dma_tx_status() calls
this function from process context. If an interrupt fires and triggers the
plx_dma_desc_task tasklet on the same CPU, wouldn't it attempt to acquire
this same lock, deadlocking the system?

[ ... ]
> 	while (plxdev->tail != plxdev->head) {
> 		desc = plx_dma_get_desc(plxdev, plxdev->tail);
> 
> 		flags = le32_to_cpu(READ_ONCE(desc->hw->flags_and_size));
> 
> 		if (flags & PLX_DESC_FLAG_VALID)
> 			break;
            ^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but should there be a dma_rmb()
inserted directly after this break statement? Without a read memory barrier,
weakly-ordered CPUs might speculatively read the destination buffer before
the status read completes, which could cause data corruption.

[ ... ]

> 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)
> {
[ ... ]
> 	spin_lock_bh(&plxdev->ring_lock);
[ ... ]
> 	/* return with the lock held, it will be released in tx_submit */
> 
> 	return &plxdesc->txd;

[Severity: High]
This isn't a bug introduced by this patch, but does returning with a
spinlock held break the DMA Engine API contract? The API allows clients to
prepare multiple descriptors before submission. If a client prepares two
descriptors sequentially, won't the second call deadlock trying to acquire
ring_lock? Also, if a client prepares a descriptor but aborts without
calling submit, wouldn't the lock leak and permanently leave softirqs
disabled on that CPU?

[ ... ]

> static void plx_dma_stop(struct plx_dma_dev *plxdev)
> {
> 	rcu_read_lock();
[ ... ]
> 	__plx_dma_stop(plxdev);
[ ... ]
> }

and __plx_dma_stop() does:

> static void __plx_dma_stop(struct plx_dma_dev *plxdev)
> {
> 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
[ ... ]
> 	while (!time_after(jiffies, timeout)) {
> 		val = readl(plxdev->bar + PLX_REG_CTRL);
> 		if (val & PLX_REG_CTRL_GRACEFUL_PAUSE_DONE)
> 			break;
> 
> 		cpu_relax();
> 	}
        ^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but this performs an unbounded delay of up to
1000ms inside the RCU read-side critical section established by
plx_dma_stop(). Could spinning for such a long duration block RCU grace
periods and severely degrade system stability?

[ ... ]

> static int plx_dma_alloc_chan_resources(struct dma_chan *chan)
> {
[ ... ]
> 	rc = plx_dma_alloc_desc(plxdev);
> 	if (rc)
> 		goto out_free_hw_ring;
> 
> 	rcu_read_lock();
> 	if (!rcu_dereference(plxdev->pdev)) {
> 		rcu_read_unlock();
> 		rc = -ENODEV;
> 		goto out_free_hw_ring;
                ^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
This isn't a bug introduced by this patch, but does this error path leak
the desc_ring array and its 2048 dynamically allocated descriptor objects?
It jumps to out_free_hw_ring, which frees hw_ring but skips the cleanup
loop needed to destroy desc_ring.

[ ... ]
> out_free_hw_ring:
> 	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
> 			  plxdev->hw_ring_dma);
> 	return rc;
> }

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