* [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()
2026-07-17 22:09 Logan Gunthorpe
@ 2026-07-17 22:10 ` Logan Gunthorpe
2026-07-17 22:32 ` sashiko-bot
0 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-17 22:10 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, Logan Gunthorpe, Sangyun Kim,
Kyungwook Boo
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()
2026-07-17 22:10 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
@ 2026-07-17 22:32 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-17 22:32 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: dmaengine, linux-pci, vkoul, Frank.Li
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma
@ 2026-07-27 17:48 Logan Gunthorpe
2026-07-27 17:48 ` [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() Logan Gunthorpe
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:26 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources Logan Gunthorpe
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
2026-07-27 17:48 ` [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:21 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure Logan Gunthorpe
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, Logan Gunthorpe, Sashiko
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
2026-07-27 17:48 ` [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() Logan Gunthorpe
2026-07-27 17:48 ` [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:27 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device Logan Gunthorpe
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, Logan Gunthorpe, Sashiko
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
` (2 preceding siblings ...)
2026-07-27 17:48 ` [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:28 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show() Logan Gunthorpe
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, Logan Gunthorpe, Sashiko
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
` (3 preceding siblings ...)
2026-07-27 17:48 ` [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:20 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
2026-07-27 18:14 ` [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
` (4 preceding siblings ...)
2026-07-27 17:48 ` [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show() Logan Gunthorpe
@ 2026-07-27 17:48 ` Logan Gunthorpe
2026-07-27 18:24 ` sashiko-bot
2026-07-27 18:14 ` [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
6 siblings, 1 reply; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 17:48 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung, Logan Gunthorpe, Sangyun Kim,
Kyungwook Boo
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
` (5 preceding siblings ...)
2026-07-27 17:48 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
@ 2026-07-27 18:14 ` Logan Gunthorpe
6 siblings, 0 replies; 16+ messages in thread
From: Logan Gunthorpe @ 2026-07-27 18:14 UTC (permalink / raw)
To: linux-kernel, linux-pci, dmaengine, Vinod Koul
Cc: Frank Li, Kelvin Cao, Thomas Weißschuh, Dave Jiang,
George Ge, Jaeyoung Chung
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()
2026-07-27 17:48 ` [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show() Logan Gunthorpe
@ 2026-07-27 18:20 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:20 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: dmaengine, Frank.Li, vkoul, linux-pci
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources
2026-07-27 17:48 ` [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources Logan Gunthorpe
@ 2026-07-27 18:21 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:21 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: Frank.Li, dmaengine, linux-pci, vkoul
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()
2026-07-27 17:48 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
@ 2026-07-27 18:24 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:24 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: vkoul, Frank.Li, dmaengine, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()
2026-07-27 17:48 ` [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() Logan Gunthorpe
@ 2026-07-27 18:26 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:26 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: dmaengine, Frank.Li, vkoul, linux-pci
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure
2026-07-27 17:48 ` [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure Logan Gunthorpe
@ 2026-07-27 18:27 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:27 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: linux-pci, Frank.Li, vkoul, dmaengine
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device
2026-07-27 17:48 ` [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device Logan Gunthorpe
@ 2026-07-27 18:28 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-07-27 18:28 UTC (permalink / raw)
To: Logan Gunthorpe; +Cc: vkoul, linux-pci, dmaengine, Frank.Li
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-07-27 18:28 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-27 17:48 [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
2026-07-27 17:48 ` [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() Logan Gunthorpe
2026-07-27 18:26 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 2/6] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources Logan Gunthorpe
2026-07-27 18:21 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure Logan Gunthorpe
2026-07-27 18:27 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 4/6] dmaengine: ioat: disable relaxed ordering before registering the device Logan Gunthorpe
2026-07-27 18:28 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 5/6] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show() Logan Gunthorpe
2026-07-27 18:20 ` sashiko-bot
2026-07-27 17:48 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
2026-07-27 18:24 ` sashiko-bot
2026-07-27 18:14 ` [PATCH v1 0/6] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma Logan Gunthorpe
-- strict thread matches above, loose matches on Subject: below --
2026-07-17 22:09 Logan Gunthorpe
2026-07-17 22:10 ` [PATCH v1 6/6] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr() Logan Gunthorpe
2026-07-17 22:32 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox