[PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[Logan Gunthorpe]

When reviewing the recent switchtec patchset, the Sashiko bot noticed
a handful of pre-existing problems in the ioat and switchtec drivers.
Additionally, I've included:

 * A fix for a plx-dma issue that was reported on the list[1] a few
   weeks ago.
 * A potential issue I noticed while reviewing the code where pointers
   need to be set to NULL.

These fixes are all valid and independent of the work I was doing so
I've sent them as their own submission. I'm still working on the rest
of the feedback Sashiko gave me on that submission.

Thanks,

Logan

[1] https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr/T/#u

Logan Gunthorpe (6):
  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: 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 | 36 ++++++++++++++++++++++++++++++------
 4 files changed, 55 insertions(+), 31 deletions(-)


base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
-- 
2.47.3

[PATCH v1 1/6] 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")
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 3ef928640615..a4a7d66d042d 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 v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

[sashiko-bot]

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

Pre-existing issues:
- [High] Allocated descriptor and coherent DMA memory resources are permanently leaked if hardware channel enablement fails in `switchtec_dma_alloc_chan_resources()`.
- [High] Multiple memory leaks of `switchtec_dma_chan` structures in driver initialization error paths.
--

--- Patch [1]: [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() ---
commit 36a3a50ecff021c64b2470dc8539fa5396858275
Author: Logan Gunthorpe <logang@deltatee.com>

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

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.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..a4a7d66d042de 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;
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed
that if hardware enablement fails in switchtec_dma_alloc_chan_resources(),
the resources allocated by switchtec_dma_alloc_desc() appear to be permanently
leaked:

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_chan_resources() {
	rc = switchtec_dma_alloc_desc(swdma_chan);
	if (rc)
		return rc;

	rc = enable_channel(swdma_chan);
	if (rc)
		return rc;
    ...
}

Since the DMA framework does not automatically clean up if
device_alloc_chan_resources fails, does this need a call to
switchtec_dma_free_desc() in the error path?

[Severity: High]
This is also a pre-existing issue, but I spotted a memory leak of the channel
structures during driver initialization:

drivers/dma/switchtec_dma.c:switchtec_dma_chans_enumerate() {
    ...
err_exit:
	for (i = 0; i < chan_cnt; i++)
		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);

	kfree(swdma_dev->swdma_chans);
    ...
}

Since switchtec_dma_chan_free() stops the channel and frees IRQs but does not
kfree the switchtec_dma_chan struct itself, does this permanently leak the
successfully initialized channel structs?

Similarly, in switchtec_dma_create(), if dma_async_device_register() fails:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
err_chans_release_exit:
	switchtec_dma_chans_release(pdev, swdma_dev);

err_exit:
    ...
	kfree(swdma_dev);
    ...
}

The array and channel structs are missed here since they are only normally freed
in the device_release callback. Does this path also need to kfree the array
and channel elements?

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

[PATCH v1 2/6] 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/T/#u
Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
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 a4a7d66d042d..f77da31aeb65 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 v1 2/6] 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:
- [High] The error path in switchtec_dma_alloc_chan_resources() frees descriptor rings without halting the hardware channel or clearing DMA base addresses, leading to a potential DMA use-after-free.
--

--- Patch [2]: [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources ---
commit eaf75ddb364eba320691aad49bebc8922067a94b
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 handle failures during initialization. It disables the channel and frees
the descriptor rings to prevent resource leaks when setup fails.

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

> 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

[ ... ]

> @@ -988,15 +988,15 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	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;

[ ... ]

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

[Severity: High]
If unhalt_channel() times out and returns an error, we jump here and free
the descriptor rings. Does this risk a DMA use-after-free?

Looking at disable_channel(), it merely clears an enable bit via a posted,
unflushed PCIe write. Because this unwind path skips switchtec_dma_chan_stop()
(which is used in the normal teardown path to synchronously halt the channel
and clear the base registers), couldn't the hardware complete the unhalt
command and access the descriptor rings right after they are freed?

Should this error path ensure the channel is fully stopped and DMA base
addresses are cleared before freeing the coherent DMA memory?

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

[PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure

[Logan Gunthorpe]

If dma_async_device_register() fails during probe,
switchtec_dma_chans_release() only stops hardware and frees IRQs and
tasklets; it never frees the per-channel swdma_chan structures or the
swdma_chans array itself.

switchtec_dma_chans_release() is also called from switchtec_dma_remove()
before dma_async_device_unregister() has torn down the channels, so it
can't free that memory itself. Free the channels and the array
directly in the registration-failure path instead, where nothing else
will ever free them.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org/T/#u
Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index f77da31aeb65..5768948ef466 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1234,7 +1234,7 @@ static void switchtec_dma_release(struct dma_device *dma_dev)
 static int switchtec_dma_create(struct pci_dev *pdev)
 {
 	struct switchtec_dma_dev *swdma_dev;
-	int chan_cnt, nr_vecs, irq, rc;
+	int chan_cnt, nr_vecs, irq, rc, i;
 	struct dma_device *dma;
 	struct dma_chan *chan;
 
@@ -1317,6 +1317,11 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 err_chans_release_exit:
 	switchtec_dma_chans_release(pdev, swdma_dev);
 
+	for (i = 0; i < swdma_dev->chan_cnt; i++)
+		kfree(swdma_dev->swdma_chans[i]);
+
+	kfree(swdma_dev->swdma_chans);
+
 err_exit:
 	if (swdma_dev->chan_status_irq)
 		free_irq(swdma_dev->chan_status_irq, swdma_dev);
-- 
2.47.3

Re: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure

[sashiko-bot]

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

New issues:
- [High] The patch introduces a race condition leading to a use-after-free because it frees channel structures without removing them from `dma_dev->channels` while a global interrupt is still active.

Pre-existing issues:
- [High] In `switchtec_dma_remove()`, `swdma_dev` is used to unmap resources after it has already been freed.
- [High] Broken cleanup of `chan_status_irq` resulting in a memory leak, wrong IRQ being freed, and a dangling interrupt handler leading to a use-after-free.
--

--- Patch [3]: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 26b72b89d5c52be6bb3a650d3492040c2465768c
Author: Logan Gunthorpe <logang@deltatee.com>

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

If dma_async_device_register() fails during probe,
switchtec_dma_chans_release() only stops hardware and frees IRQs and
tasklets, but never frees the per-channel swdma_chan structures. This patch
frees the channels and the array directly in the registration-failure path.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..5768948ef4662 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1317,6 +1317,11 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		kfree(swdma_dev->swdma_chans[i]);

[Severity: High]
Does this free the channel structures while they are still linked in
dma_dev->channels?

During initialization, the embedded dma_chan nodes are added to
dma->channels. When freeing them here, the nodes are not removed from the
list, and the global chan_status_irq is still registered and active until
the subsequent err_exit block.

If the hardware asserts the interrupt, or if the interrupt is already being
processed concurrently, could switchtec_dma_chan_status_isr() iterate over
dma_dev->channels through this freed memory?

> +
> +	kfree(swdma_dev->swdma_chans);
> +
>  err_exit:
>  	if (swdma_dev->chan_status_irq)
>  		free_irq(swdma_dev->chan_status_irq, swdma_dev);

[Severity: High]
This is a pre-existing issue, but does this cleanup block use a vector
index rather than the global Linux IRQ number?

During probe, swdma_dev->chan_status_irq stores the MSI-X vector index
(e.g., 0 or 1). If it is 0, the condition evaluates to false, skipping
cleanup. If it is > 0, it calls free_irq() with a small integer, which
attempts to free an unrelated system IRQ.

This leaves the actual MSI-X interrupt handler registered. If the Linux IRQ
number is later reused by another device, could the kernel invoke
switchtec_dma_chan_status_isr() with a dev_id pointing to the freed
swdma_dev structure?


[Severity: High]
This is also a pre-existing issue, but is swdma_dev used to unmap resources
after it has already been freed in switchtec_dma_remove()?

switchtec_dma_remove() calls dma_async_device_unregister(&swdma_dev->dma_dev).
This synchronously drops a reference to the DMA device, which can trigger the
driver's switchtec_dma_release() callback and execute kfree(swdma_dev).

Once dma_async_device_unregister() returns, the swdma_dev pointer can be
invalid, but switchtec_dma_remove() immediately proceeds to call
iounmap(swdma_dev->bar). Could this result in a use-after-free on device
teardown?

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

[PATCH v1 4/6] 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/T/#u
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 v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device

[sashiko-bot]

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

Pre-existing issues:
- [High] Partial failure in dma_async_device_register() leaks global dma_ida and per-device chan_ida.
- [High] Use-after-free in ioat_dma_do_interrupt() due to mixing devres IRQ registration with manual kfree on the probe error path.
--

--- Patch [4]: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device ---
(Note: The format of this review is altered from the usual inline commenting style due to recitation restrictions. No patch diffs are quoted below.)

commit 29a5621a4420e7d413fe2125067821903edd8980
Author: Logan Gunthorpe <logang@deltatee.com>
dmaengine: ioat: disable relaxed ordering before registering the device

This commit fixes a dangling device registration issue by moving the 
PCIe relaxed ordering disablement before the dma_async_device_register() 
call. This ensures that the device does not remain in the core's list 
if an error occurs later in the probe function.

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

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential 
resource leak in drivers/dma/dmaengine.c:dma_async_device_register().

When a failure occurs and device->chancnt is greater than 0, the code 
jumps to the err_out label. However, this path seems to bypass freeing 
the global dma_ida:

err_out:
    /* if we never registered a channel just release the idr */
    if (!device->chancnt) {
        ida_free(&dma_ida, device->dev_id);
        return rc;
    }

Additionally, the loop that unregisters the channel devices omits calling 
ida_free(&device->chan_ida, chan->chan_id) and device->chancnt--. 

If a device probe fails due to memory pressure or hotplug errors, will this 
leak the global dma_ida and per-device chan_ida? If the global dma_ida 
space is exhausted, it might prevent any further DMA devices from 
registering system-wide.

[Severity: High]
This is also a pre-existing issue, but is there a risk of a use-after-free 
on the probe error path?

In drivers/dma/ioat/init.c:ioat_dma_setup_interrupts(), a shared interrupt 
is registered via devm_request_irq(), tying its lifetime to the device core. 
However, if ioat_pci_probe() later fails, it manually calls kfree(device):

ioat_pci_probe() {
    ...
    if (err) {
        for (i = 0; i < IOAT_MAX_CHANS; i++)
            kfree(device->idx[i]);
        kfree(device);
    ...
}

Because the IRQ was allocated with devres, it remains active until 
ioat_pci_probe() returns and the driver core runs the devres cleanup. 
If another device sharing this interrupt line triggers an IRQ during this 
window, wouldn't drivers/dma/ioat/dma.c:ioat_dma_do_interrupt() 
dereference the freed instance pointer?

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

[PATCH v1 5/6] 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().

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 v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `.release` callback in `ioat_ktype` leads to Use-After-Free during device teardown.
--

--- Patch [5]: [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show() ---
commit 92c0d5a2427ebf1a54399ad0e602ee20f0d12130
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

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

> diff --git a/drivers/dma/ioat/sysfs.c b/drivers/dma/ioat/sysfs.c
> index e796ddb5383f8..976134df81084 100644
> --- a/drivers/dma/ioat/sysfs.c
> +++ b/drivers/dma/ioat/sysfs.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does ioat_ktype need a .release callback?

Looking at drivers/dma/ioat/sysfs.c:ioat_ktype:

const struct kobj_type ioat_ktype = {
	.sysfs_ops = &ioat_sysfs_ops,
	.default_groups = ioat_groups,
};

Without a .release callback, doesn't this break the kobject reference counting
lifecycle?  If a user space process keeps a sysfs file open (like ring_size)
while the device is hot-unplugged or the driver is unbound, release_ioatdma()
frees the ioatdma_chan structures directly.

When the user space process later closes the sysfs file and the kobject
reference count drops to zero, the kernel will attempt to access the kobject
embedded in the already-freed ioatdma_chan memory.  Can this cause a
use-after-free during device teardown?

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

[PATCH v1 6/6] 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/T/#u
Fixes: c2dbcaa8c672 ("dmaengine: plx-dma: Implement hardware initialization and cleanup")
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 v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] API Contract Violation: `plx_dma_prep_memcpy` returns with a spinlock held.
- [High] Deadlock during client callback execution due to held spinlock in `plx_dma_process_desc`.
- [High] Memory leak of the descriptor ring in `plx_dma_alloc_chan_resources` on error path.
- [High] Deadlock due to missing bottom-half disable when taking spinlock in `plx_dma_process_desc`.
- [High] Hardware state corruption due to a 16-bit write to a 32-bit MMIO register (`PLX_REG_CTRL`).
- [Medium] Missing `dma_rmb()` memory barrier after reading the DMA completion flag.
--

--- Patch [6]: [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() ---
commit bd1bd202047866d5b31f34772f932d4e070c9fd6
Author: Logan Gunthorpe <logang@deltatee.com>

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

This patch fixes a NULL pointer dereference in plx_dma_isr(). It moves the
bar assignment ahead of request_irq() to ensure the interrupt handler doesn't
run before plxdev->bar is initialized.

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

> 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 driver, I noticed a few pre-existing issues in other
functions that might need attention:

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to invoke the
client callback while holding the spinlock in plx_dma_process_desc()?

drivers/dma/plx_dma.c:plx_dma_process_desc() {
...
	spin_lock(&plxdev->ring_lock);

	while (plxdev->tail != plxdev->head) {
		desc = plx_dma_get_desc(plxdev, plxdev->tail);
...
		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
...
	spin_unlock(&plxdev->ring_lock);
}

If a DMA client prepares a new transfer from its completion callback using
dmaengine_prep_dma_memcpy(), it will attempt to re-acquire this same
ring_lock in plx_dma_prep_memcpy(), causing a deadlock. This also affects
plx_dma_abort_desc() which does the same.

[Severity: Critical]
This is a pre-existing issue, but does plx_dma_prep_memcpy() intentionally
return with the spinlock held?

drivers/dma/plx_dma.c:plx_dma_prep_memcpy() {
...
	spin_lock_bh(&plxdev->ring_lock);
...
	/* return with the lock held, it will be released in tx_submit */
	return &plxdesc->txd;
}

If a client prepares multiple descriptors in sequence before submitting them,
the second call to prep_memcpy() will deadlock trying to acquire the lock
that the first call left held. Furthermore, any client that sleeps, allocates
memory with GFP_KERNEL, or takes a mutex between prep and submit will trigger
a scheduling while atomic panic.

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
plx_dma_alloc_chan_resources() leak the descriptor ring?

drivers/dma/plx_dma.c:plx_dma_alloc_chan_resources() {
...
	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;
	}
...
out_free_hw_ring:
	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
			  plxdev->hw_ring_dma);
	return rc;
}

If rcu_dereference(plxdev->pdev) evaluates to false, the code jumps to
out_free_hw_ring. This frees hw_ring, but fails to free the newly allocated
desc_ring array and its descriptors, causing a memory leak.

[Severity: High]
This is a pre-existing issue, but should plx_dma_process_desc() use
spin_lock_bh() instead of spin_lock()?

drivers/dma/plx_dma.c:plx_dma_process_desc() {
...
	spin_lock(&plxdev->ring_lock);
...
}

This function can be called from process context via plx_dma_tx_status().
Because bottom halves are left enabled, a softirq can interrupt the lock
holder and schedule the driver's tasklet (plx_dma_desc_task) on the same CPU,
which will attempt to acquire the lock and deadlock.

[Severity: High]
This isn't a bug introduced by this patch, but should plx_dma_issue_pending()
use writel() instead of writew()?

drivers/dma/plx_dma.c:plx_dma_issue_pending() {
...
	writew(PLX_REG_CTRL_START_VAL, plxdev->bar + PLX_REG_CTRL);
...
}

PLX_REG_CTRL is treated as a 32-bit register elsewhere in the driver (e.g. in
__plx_dma_stop). Writing a 16-bit value to a 32-bit MMIO target might cause
partial register updates or undefined hardware behavior.

[Severity: Medium]
This is a pre-existing issue, but is a dma_rmb() memory barrier needed after
reading the descriptor status?

drivers/dma/plx_dma.c:plx_dma_process_desc() {
...
		flags = le32_to_cpu(READ_ONCE(desc->hw->flags_and_size));

		if (flags & PLX_DESC_FLAG_VALID)
			break;
...
}

On architectures with relaxed read ordering, reading the completion flag
without a subsequent dma_rmb() might allow the CPU to speculatively read
stale payload data from before the DMA transfer was actually completed by
the hardware.

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