[PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device

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

[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

[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

[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

[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

[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 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[Logan Gunthorpe]

On 2026-07-27 11:48, Logan Gunthorpe wrote:
> 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

Shoot, very sorry. I sent the wrong series. Ignore this thread and I'll
resend the correct one.

Logan

Re: [PATCH v1 5/6] 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().
> 
> 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/20260727174837.6645-1-logang@deltatee.com?part=5

Re: [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[sashiko-bot]

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

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

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

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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Softirq deadlock due to using `spin_lock()` instead of `spin_lock_bh()` in `plx_dma_process_desc()`.
- [High] Missing check for surprise removal (0xffff) in interrupt handler causes IRQ storms on shared legacy interrupts.
--

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

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. This patch moves the bar assignment
ahead of request_irq() so everything is initialized before the handler
can run.

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

[Severity: High]
This is a pre-existing issue, but could this lock and tasklet setup lead to a
softirq deadlock?

Looking at plx_dma_process_desc(), it acquires plxdev->ring_lock using
spin_lock() without disabling softirqs. If called from process context, and a
hardware interrupt occurs on the same CPU, the ISR will execute and schedule
this desc_task tasklet.

Upon returning from the ISR, the kernel will execute the softirq on the same
CPU, which attempts to acquire the same ring_lock in plx_dma_desc_task(),
deadlocking the CPU.

Should plx_dma_process_desc() be using spin_lock_bh() instead?

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

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the ISR hooked
up here, could a surprise removal of the PCI device cause an IRQ storm?

If the device is removed, MMIO reads from plxdev->bar + PLX_REG_INTR_STATUS
in plx_dma_isr() will return 0xffff. Because 0xffff is non-zero, it bypasses
the !status check, writes 0xffff back, and returns IRQ_HANDLED.

Since IRQ_HANDLED is returned, the interrupt subsystem assumes the interrupt
was serviced. If the IRQ line is shared, it will immediately re-trigger,
causing an infinite loop.

Should the ISR explicitly check for 0xffff and return IRQ_NONE?

> +	if (rc)
> +		goto free_plx;
> +
>  	dma = &plxdev->dma_dev;

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

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:
- [Medium] Missing cleanup in the error paths of switchtec_dma_alloc_chan_resources() leads to memory leaks and overwritten DMA allocations.
- [Medium] Leaked switchtec_dma_chan objects and swdma_chans array when device enumeration or registration fails.
--

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

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

This commit nulls out each pointer as it is freed in switchtec_dma_free_desc()
so a subsequent call is a no-op for anything already released. This prevents
a double-free if the function is called twice.

> 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

[Severity: Medium]
This is a pre-existing issue, but does switchtec_dma_alloc_chan_resources()
actually call switchtec_dma_free_desc() during unwind?

Looking at switchtec_dma_alloc_chan_resources():

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;

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

If enable_channel(), reset_channel(), unhalt_channel(), or rcu_dereference()
fail, the function returns an error directly without freeing the DMA coherent
memory and descriptor rings that were just allocated by
switchtec_dma_alloc_desc().

Because the dmaengine framework does not automatically call
device_free_chan_resources() when device_alloc_chan_resources() fails,
the driver must handle its own cleanup.

Furthermore, if a client or the driver core retries the channel allocation,
switchtec_dma_alloc_desc() will blindly overwrite the dangling pointers with
new allocations, permanently orphaning the previously allocated memory.

Does this need an explicit cleanup path on error?

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

[ ... ]

While reviewing this code, I noticed another pre-existing issue.

[Severity: Medium]
Does the driver leak the switchtec_dma_chan objects during probe errors?

Looking at switchtec_dma_chans_enumerate():

drivers/dma/switchtec_dma.c:switchtec_dma_chans_enumerate() {
	for (i = 0; i < chan_cnt; i++) {
		rc = switchtec_dma_chan_init(swdma_dev, pdev, i);
		if (rc) {
			dev_err(&pdev->dev, "Channel %d: init channel failed\n",
				i);
			chan_cnt = i;
			goto err_exit;
		}
	}

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

	return rc;
}

The err_exit loop calls switchtec_dma_chan_free() which stops the hardware
but fails to free the actual swdma_chan memory before freeing the array.
switchtec_dma_chan_free() does not call kfree(swdma_chan).

Additionally, 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:
	if (swdma_dev->chan_status_irq)
		free_irq(swdma_dev->chan_status_irq, swdma_dev);

	iounmap(swdma_dev->bar);
	kfree(swdma_dev);
	return rc;
}

It frees swdma_dev but leaks both the swdma_dev->swdma_chans array itself
and the individual channel structures within it, because
switchtec_dma_chans_release() also just calls switchtec_dma_chan_free().

Could these error paths result in a memory leak?

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

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 6 potential issue(s) to consider:

New issues:
- [High] The patch introduces a Use-After-Free in the error path by freeing swdma_chan structures while the global chan_status_irq is still registered and active.

Pre-existing issues:
- [High] Use-after-free on swdma_dev during device removal.
- [High] Preexisting Use-After-Free on dma_async_device_register() failure due to chan->dev being freed while the IRQ is active.
- [High] Failure of ioremap() is not checked, leading to an immediate null pointer dereference.
- [Medium] Leaked channel structures during partial enumeration failure in switchtec_dma_chans_enumerate().
- [Medium] Missing put_device() for the DMA parent device on dma_async_device_register() failure.
--

--- Patch [3]: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 7464b8c763dc5bb4e72b11ab5698233a70070171
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; it never frees the per-channel swdma_chan structures or the
swdma_chans array itself.

[ ... ]

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

> --- 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]
Could this newly introduced kfree() loop cause a use-after-free?

Since chan_status_irq is registered earlier and remains active until
free_irq() is called in the err_exit block below, an interrupt could fire
while this error path is executing.

If the status interrupt fires after these structures are freed,
switchtec_dma_chan_status_isr() will iterate over dma_dev->channels and
dereference the freed swdma_chan structures.

Should the IRQ be freed, or the channels removed from the list, before we
free this memory?

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


While reviewing this patch, I also noticed a few pre-existing issues in
this driver:

[Severity: Medium]
This is a pre-existing issue, but does switchtec_dma_chans_enumerate()
leak memory on its error path?

If switchtec_dma_chan_init() fails, the error handling cleans up the array
but does not appear to free the individual swdma_chan elements:

drivers/dma/switchtec_dma.c:switchtec_dma_chans_enumerate() {
    ...
	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() only disables hardware and frees IRQs,
the memory allocated for the channel structures via kzalloc_obj() seems
to be permanently leaked here.


[Severity: Medium]
This isn't a bug introduced by this patch, but is there a reference leak if
dma_async_device_register() fails in switchtec_dma_create()?

The driver acquires a reference to the parent PCI device:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
	dma->dev = get_device(&pdev->dev);
    ...
}

If registration fails and the code jumps to err_chans_release_exit,
swdma_dev is eventually freed without calling put_device(dma->dev).
Because the device failed registration, its release callback isn't
invoked, which appears to leave the parent PCI device reference leaked.


[Severity: High]
This is a pre-existing issue, but can switchtec_dma_remove() trigger a
use-after-free when unmapping the bar?

When unregistering the DMA device, if there are no outstanding channel
references, the core synchronously invokes the release callback
(switchtec_dma_release) which frees swdma_dev.

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
    ...
	dma_async_device_unregister(&swdma_dev->dma_dev);

	iounmap(swdma_dev->bar);
    ...
}

Since iounmap() dereferences swdma_dev->bar immediately after the
unregister call may have freed swdma_dev, could this lead to a crash
during device removal?


[Severity: High]
This isn't a bug introduced by this patch, but is there another
use-after-free risk during dma_async_device_register() failure?

If registration fails after partially registering channels, the DMA core
automatically frees the chan->dev structures via device_unregister().

Because chan_status_irq remains active at this point, a spurious or
concurrent interrupt can fire:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
    ...
	chan_dev = &swdma_chan->dma_chan.dev->device;
    ...
}

This unconditionally dereferences the core-freed chan->dev structure.
Could this cause a crash if the interrupt fires before free_irq() is
reached in the error path?


[Severity: High]
This is a pre-existing issue, but does switchtec_dma_create() safely
handle ioremap() failures?

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
				 pci_resource_len(pdev, 0));
    ...
	irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);
    ...
}

Since ioremap() can return null, skipping a validation check here
seems to guarantee a null pointer dereference on the very next readw()
if the mapping fails.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727174837.6645-1-logang@deltatee.com?part=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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] IRQ handler Use-After-Free in probe error path due to mixing devres IRQ registration with manual kfree.
- [High] Use-After-Free on `ioatdma_chan` due to missing kobject `.release` function and unconditional kfree.
- [High] Legacy MSI/MSI-X vectors leak on error paths and interrupt fallback, causing device dysfunction and resource exhaustion.
--

--- Patch [4]: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device ---
commit eb15a8e1f4e218e8537f96abc3be35c46bf67999
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: ioat: disable relaxed ordering before registering the device

This commit fixes a dangling device registration in the error path of
ioat3_dma_probe(). By moving the relaxed ordering disablement before
dma_async_device_register(), it ensures the device is safely registered
only when subsequent initialization steps cannot fail.

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

> 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 doesn't this error path lead to
a use-after-free?

If dma_async_device_register() fails here and we jump to
err_disable_interrupts, the probe fails and ioat_pci_probe() will manually
call kfree() on the device and channel structures before returning:

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

However, because devm_request_irq() was used earlier to register the interrupt
handler, devres will only unregister the IRQ after the probe function fully
returns. If a shared interrupt fires in this window, ioat_dma_do_interrupt()
will dereference the freed instance pointer causing a use-after-free.

[Severity: High]
This is a pre-existing issue, but does the error handling properly clean up
MSI/MSI-X vectors?

If we jump to err_disable_interrupts and fail probe, or during normal unbind,
the driver masks hardware interrupts but fails to call pci_disable_msi() or
pci_disable_msix(). For example in ioat_dma_remove():

drivers/dma/ioat/init.c:ioat_dma_remove() {
    ...
    ioat_disable_interrupts(ioat_dma);
    ...
}

This permanently leaks the allocated IRQ vectors. Also, if devm_request_irq()
fails during MSI-X setup, the driver jumps to the msi fallback label without
calling pci_disable_msix(). This breaks INTx generation since the device will
not assert INTx while MSI-X is active.

> +	ioat_kobject_add(ioat_dma, &ioat_ktype);
> +

[Severity: High]
This is a pre-existing issue, but is it safe to embed kobjects here without a
release function?

When ioat_kobject_add() adds the kobject with ioat_ktype:

drivers/dma/ioat/sysfs.c:ioat_ktype {
    const struct kobj_type ioat_ktype = {
        .sysfs_ops = &ioat_sysfs_ops,
        .default_groups = ioat_groups,
    };
}

It is missing the required .release function. When the driver is unbound,
release_ioatdma() unconditionally frees the channel structures:

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

If unprivileged userspace holds a sysfs file open while the device is unbound,
the kobject refcount is still elevated. When userspace later closes the file,
kobject_put() will attempt to drop the final reference and access the already
freed memory.

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

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