[PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

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