[PATCH v2 00/11] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[PATCH v2 00/11] dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma

[Logan Gunthorpe]

When reviewing the recent switchtec patchset[1], the Sashiko bot noticed
a handful of pre-existing problems in the ioat and switchtec drivers.
I attempted to fix those plus an unrelated issue reported in plxdma but
when I submitted those patches, Sashiko found even more issues[2]. (It is
relentless!).

I've fixed the issues reported with v1 of this series and the ones for the
switchtec driver. But the pre-existing issues in ioat, plxdma and the
dmaengine itself I've punted until I can find some time to dig into them.

Hopefully, this is good enough for Sashiko this time and we can get
it reviewed by real humans.

The series is based off of v7.2-rc4.

Thanks,

Logan

[1] https://lore.kernel.org/all/20260707162045.23910-1-logang@deltatee.com
[2] https://sashiko.dev/#/patchset/20260717221001.361421-1-logang@deltatee.com

Changes since v1:

 * Added a fix for switchtec_dma_alloc_chan_resources()'s error path
   calling disable_channel() instead of properly halting the channel
   before freeing the descriptor rings. (Per Sashiko)
 * Added a fix for switchtec-dma channel structs being freed without
   being removed from dma_dev->channels on a registration failure,
   while the channel status IRQ is still live. (Per Sashiko)
 * Added a fix for switchtec_dma_remove() using swdma_dev after it may
   already have been freed by dma_async_device_unregister(). (Per
   Sashiko)
 * Added a fix for chan_status_irq being freed with the wrong API, and
   a valid vector index of 0 being incorrectly treated as unset.
   (Per Sashiko)
 * Made switchtec_dma_chans_release() void, since nothing checked its
   return value. (Noticed while reviewing the code for these changes).

Logan Gunthorpe (11):
  dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()
  dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources
  dmaengine: switchtec-dma: halt channel on alloc_chan_resources error
  dmaengine: switchtec-dma: fix channel leak on registration failure
  dmaengine: switchtec-dma: make switchtec_dma_chans_release() void
  dmaengine: switchtec-dma: unlink channels before freeing on
    registration failure
  dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()
  dmaengine: switchtec-dma: fix chan_status_irq cleanup on create()
    error
  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 | 78 +++++++++++++++++++++++++++----------
 4 files changed, 82 insertions(+), 46 deletions(-)


base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
--
2.47.3

[PATCH v2 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

[Logan Gunthorpe]

switchtec_dma_free_desc() frees swdma_chan->hw_sq, hw_cq, and every
desc_ring[] entry without clearing the pointers afterward. If
switchtec_dma_alloc_chan_resources() fails partway through and calls
it during unwind, then a later retry of alloc_chan_resources() fails
in switchtec_dma_alloc_desc() before reallocating one of those
pointers, its own failure path calls switchtec_dma_free_desc() again
and frees the same, already-freed pointers a second time.

NULL out each pointer as it's freed so a subsequent call is a no-op
for anything already released.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 3ef928640615..a4a7d66d042d 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
 	if (swdma_chan->hw_sq)
 		dma_free_coherent(swdma_dev->dma_dev.dev, size,
 				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
+	swdma_chan->hw_sq = NULL;
 
 	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
 	if (swdma_chan->hw_cq)
 		dma_free_coherent(swdma_dev->dma_dev.dev, size,
 				  swdma_chan->hw_cq, swdma_chan->dma_addr_cq);
+	swdma_chan->hw_cq = NULL;
 
-	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++)
+	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
 		kfree(swdma_chan->desc_ring[i]);
+		swdma_chan->desc_ring[i] = NULL;
+	}
 }
 
 static int switchtec_dma_alloc_desc(struct switchtec_dma_chan *swdma_chan)
-- 
2.47.3

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory leak of DMA channel resources on error path in `switchtec_dma_alloc_chan_resources()`.
- [High] Memory leak of `switchtec_dma_chan` structures and channel arrays on module probe error paths.
- [High] Deadlock due to client callback invocation while holding `complete_lock`.
- [High] `switchtec_dma_prep_desc()` returns with `submit_lock` held, preventing clients from preparing multiple descriptors.
- [High] Unsafe atomic delays of up to 100ms with a spinlock held and a 60ms mdelay in atomic context.
- [Medium] Large contiguous 1MB DMA memory allocations use GFP_NOWAIT in a sleepable context, risking allocation failure.
--

--- Patch [1]: [PATCH v2 01/11] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() ---
commit 4a5b609a9359892c57ed414c821b1570151f6b0b
Author: Logan Gunthorpe <logang@deltatee.com>

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

This commit adds NULL assignments to the hw_sq, hw_cq, and desc_ring pointers
after they are freed in switchtec_dma_free_desc(). This fixes a double-free
bug that occurs if switchtec_dma_alloc_chan_resources() fails during unwind
and a subsequent retry also fails, calling the free function a second time.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..a4a7d66d042de 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -886,14 +886,18 @@ static void switchtec_dma_free_desc(struct switchtec_dma_chan *swdma_chan)
>  	if (swdma_chan->hw_sq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_sq, swdma_chan->dma_addr_sq);
> +	swdma_chan->hw_sq = NULL;
>  
>  	size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
>  	if (swdma_chan->hw_cq)
>  		dma_free_coherent(swdma_dev->dma_dev.dev, size,
>  				  swdma_chan->hw_cq, swdma_chan->dma_addr_cq);
> +	swdma_chan->hw_cq = NULL;
>  
> -	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++)
> +	for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
>  		kfree(swdma_chan->desc_ring[i]);
> +		swdma_chan->desc_ring[i] = NULL;
> +	}
>  }
>  
>  static int switchtec_dma_alloc_desc(struct switchtec_dma_chan *swdma_chan)

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

If enable_channel() fails after switchtec_dma_alloc_desc() has already allocated
resources, the code returns immediately:

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

Will this permanently leak the DMA descriptors and coherent memory since
switchtec_dma_free_desc() is not called?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_chan_free() skip freeing
the channel structure itself?

If probe fails and calls switchtec_dma_chans_release() -> switchtec_dma_chan_free(),
it stops the channel but never calls kfree(swdma_chan):

drivers/dma/switchtec_dma.c:switchtec_dma_chan_free() {
    ...
	switchtec_dma_chan_stop(swdma_chan);

	return 0;
}

Also, on module probe error paths, switchtec_dma_create() calls
switchtec_dma_chans_release() but doesn't free the swdma_dev->swdma_chans array:

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

Could this lead to memory leaks of the switchtec_dma_chan structures and the
swdma_chans array during early probe failures?

[Severity: High]
This isn't a bug introduced by this patch, but can we self-deadlock when invoking
the client callback?

In switchtec_dma_cleanup_completed(), the callback is invoked synchronously while
holding complete_lock:

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

If the client callback checks TX status via dma_async_is_tx_complete(), it routes
to switchtec_dma_tx_status(), which calls switchtec_dma_cleanup_completed() again.

Will this cause a deadlock when trying to reacquire complete_lock?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_prep_desc() return with a
spinlock held?

drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
    ...
	spin_lock_bh(&swdma_chan->submit_lock);
    ...
	/* return with the lock held, it will be released in tx_submit */

	return &desc->txd;
}

Does this violate the DMA engine API by preventing clients from preparing
multiple descriptors before submitting them?

If a client batches requests and calls prep twice, won't it self-deadlock on
submit_lock?

[Severity: High]
This isn't a bug introduced by this patch, but do we have unsafe delays in
atomic contexts?

In pause_reset_channel(), which is reachable from the device_terminate_all
callback (allowed in atomic context), there is a 60ms busy-wait:

drivers/dma/switchtec_dma.c:pause_reset_channel() {
    ...
	/* wait 60ms to ensure no pending CEs */
	mdelay(60);
    ...
}

Additionally, wait_for_chan_status() spins for up to 100ms while holding
hw_ctrl_lock:

drivers/dma/switchtec_dma.c:wait_for_chan_status() {
    ...
	return readl_poll_timeout_atomic(&chan_hw->status, status,
					 (set && (status & mask)) ||
					 (!set && !(status & mask)),
					 10, 100 * USEC_PER_MSEC);
}

Will these long atomic delays cause severe system latency spikes or soft lockups?

[Severity: Medium]
This is a pre-existing issue, but is it safe to use GFP_NOWAIT for large
allocations in switchtec_dma_alloc_desc()?

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
    ...
	size = SWITCHTEC_DMA_SQ_SIZE * sizeof(*swdma_chan->hw_sq);
	swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev->dma_dev.dev, size,
					       &swdma_chan->dma_addr_sq,
					       GFP_NOWAIT);
    ...
}

Since device_alloc_chan_resources() is called under a mutex and is allowed to sleep,
could using GFP_NOWAIT here unnecessarily cause allocation failures under memory
pressure since it prevents direct reclaim and compaction?

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

[PATCH v2 02/11] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[Logan Gunthorpe]

switchtec_dma_alloc_chan_resources() returns directly on any later
failure, without ever freeing the descriptor rings and coherent DMA
memory it just allocated. The dmaengine core does not call
device_free_chan_resources() when device_alloc_chan_resources() fails,
so the driver has to unwind its own partial state.

The device-removed check also runs after ring_active and
comp_ring_active have already been set true, so a failure there left
the channel marked active despite alloc_chan_resources() reporting
failure.

Add an error-unwind path that disables the channel and frees the
descriptor rings on every failure after allocation. ring_active and
comp_ring_active are cleared under the same locks
switchtec_dma_free_chan_resources() already uses, since the completion
tasklet checks comp_ring_active under complete_lock before touching
the completion ring, and a stale IRQ can still be in flight when this
unwind path runs.

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

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index a4a7d66d042d..f77da31aeb65 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -988,15 +988,15 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 
 	rc = enable_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_free_desc;
 
 	rc = reset_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_disable_channel;
 
 	rc = unhalt_channel(swdma_chan);
 	if (rc)
-		return rc;
+		goto err_disable_channel;
 
 	swdma_chan->ring_active = true;
 	swdma_chan->comp_ring_active = true;
@@ -1007,7 +1007,8 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 	rcu_read_lock();
 	if (!rcu_dereference(swdma_dev->pdev)) {
 		rcu_read_unlock();
-		return -ENODEV;
+		rc = -ENODEV;
+		goto err_ring_inactive;
 	}
 
 	perf_cfg = readl(&swdma_chan->mmio_chan_fw->perf_cfg);
@@ -1029,6 +1030,20 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 		FIELD_GET(PERF_MRRS_MASK, perf_cfg));
 
 	return SWITCHTEC_DMA_SQ_SIZE;
+
+err_ring_inactive:
+	spin_lock_bh(&swdma_chan->submit_lock);
+	swdma_chan->ring_active = false;
+	spin_unlock_bh(&swdma_chan->submit_lock);
+
+	spin_lock_bh(&swdma_chan->complete_lock);
+	swdma_chan->comp_ring_active = false;
+	spin_unlock_bh(&swdma_chan->complete_lock);
+err_disable_channel:
+	disable_channel(swdma_chan);
+err_free_desc:
+	switchtec_dma_free_desc(swdma_chan);
+	return rc;
 }
 
 static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
-- 
2.47.3

Re: [PATCH v2 02/11] 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
> 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/20260721155739.62120-1-logang@deltatee.com?part=2

[PATCH v2 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

[Logan Gunthorpe]

The error-unwind path called disable_channel() before freeing the
descriptor rings, but that only clears the enable bit with an
unflushed write -- it doesn't halt the channel or clear its DMA base
address registers. If unhalt_channel() timed out, the channel's actual
state is unknown at that point, so nothing guarantees the hardware
isn't still touching the rings when they're freed.

Call switchtec_dma_chan_stop() first, matching what
switchtec_dma_free_chan_resources() already does before freeing
descriptors on the normal teardown path: it synchronously halts the
channel and zeroes the DMA base registers.

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

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index f77da31aeb65..107769cca772 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1040,6 +1040,7 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
 	swdma_chan->comp_ring_active = false;
 	spin_unlock_bh(&swdma_chan->complete_lock);
 err_disable_channel:
+	switchtec_dma_chan_stop(swdma_chan);
 	disable_channel(swdma_chan);
 err_free_desc:
 	switchtec_dma_free_desc(swdma_chan);
-- 
2.47.3

Re: [PATCH v2 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Early return in `switchtec_dma_chan_stop()` on hardware timeout fails to clear DMA base registers, leading to a Use-After-Free of DMA memory by the active hardware when descriptors are unconditionally freed.
- [High] The completion handler `switchtec_dma_cleanup_completed()` synchronously invokes client callbacks while holding the `complete_lock` spinlock, creating a severe deadlock hazard.
- [High] The driver intentionally leaves the `submit_lock` spinlock locked when returning from `switchtec_dma_prep_desc()`, requiring the client to submit the descriptor to release it. This breaks DMA API semantics and risks permanent deadlock.
- [High] Holding a spinlock while busy-waiting for hardware status for up to 100ms severely degrades system scheduling latency.
--

--- Patch [3]: [PATCH v2 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error ---
commit 53cba0d04c9fe827ca20ae65df9b3ba987573e8e
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

This commit modifies the error unwind path in
switchtec_dma_alloc_chan_resources() to call switchtec_dma_chan_stop()
before freeing descriptors, aiming to halt the channel and clear DMA base
registers.

Link: https://lore.kernel.org/dmaengine/20260717223647.F0A051F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..107769cca772f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1040,6 +1040,7 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  err_disable_channel:
> +	switchtec_dma_chan_stop(swdma_chan);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this error path risk a
DMA use-after-free if the hardware becomes unresponsive?

Looking at switchtec_dma_chan_stop():

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

        // ...
        writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
        // ...
}

If halt_channel() times out and returns an error, switchtec_dma_chan_stop()
returns early and bypasses clearing the DMA base registers. The error path
then proceeds to blindly free the descriptor rings via
switchtec_dma_free_desc().

If the hardware later resumes, couldn't it overwrite the freed system memory
using the uncleared physical addresses?

>  	disable_channel(swdma_chan);
>  err_free_desc:
>  	switchtec_dma_free_desc(swdma_chan);

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it safe to invoke client callbacks while
holding the completion spinlock?

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

If a client callback calls standard DMA engine APIs like
dmaengine_terminate_all() to handle errors, those driver functions will
attempt to acquire the exact same non-recursive complete_lock. Does this
create a deadlock hazard?


[Severity: High]
This isn't a bug introduced by this patch, but does switchtec_dma_prep_desc()
leak a spinlock to the caller?

drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
        spin_lock_bh(&swdma_chan->submit_lock);
        // ...
        /* return with the lock held, it will be released in tx_submit */
        return &desc->txd;
}

The DMA Engine API does not guarantee that a prepared descriptor will be
submitted. If a client aborts and discards the descriptor before submission,
does this leave the CPU permanently deadlocked with bottom-halves disabled?

Also, if a client attempts to prepare multiple segments in sequence before
submitting them, wouldn't the second call to switchtec_dma_prep_desc()
attempt to acquire the already-held submit_lock?


[Severity: High]
This is a pre-existing issue, but does the channel halting logic introduce
massive scheduling latency spikes?

drivers/dma/switchtec_dma.c:halt_channel() {
        spin_lock(&swdma_chan->hw_ctrl_lock);
        writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
        ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
        spin_unlock(&swdma_chan->hw_ctrl_lock);
}

The wait_for_chan_status() function uses readl_poll_timeout_atomic() with a
100ms timeout.

If the hardware is slow or unresponsive, will the CPU be trapped in a
busy-wait loop with preemption disabled for up to 100 milliseconds?

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

[PATCH v2 04/11] dmaengine: switchtec-dma: fix channel leak on registration failure

[Logan Gunthorpe]

switchtec_dma_chans_release() is called in three places but the
underlying memory is not freed in all of those places. In order to
clean this up, introduce a switchtec_dma_chans_free() helper that
will free the memory.

Ensure each call to switchtec_dma_chans_release() has a corresponding
switchtec_dma_chans_free() call. (The release in switchtec_dma_remove()
pairs with the free in switchtec_dma_release()).

swdma_dev->chan_cnt is now set to the number of channels that succeeded
when one fails to initialise, so switchtec_dma_chans_free() can still
be used if not all channels succeed in being allocated.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 107769cca772..13efd4189bbb 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1176,6 +1176,16 @@ static int switchtec_dma_chans_release(struct pci_dev *pdev,
 	return 0;
 }
 
+static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
+{
+	int i;
+
+	for (i = 0; i < swdma_dev->chan_cnt; i++)
+		kfree(swdma_dev->swdma_chans[i]);
+
+	kfree(swdma_dev->swdma_chans);
+}
+
 static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 					 struct pci_dev *pdev, int chan_cnt)
 {
@@ -1201,7 +1211,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 		if (rc) {
 			dev_err(&pdev->dev, "Channel %d: init channel failed\n",
 				i);
-			chan_cnt = i;
+			swdma_dev->chan_cnt = i;
 			goto err_exit;
 		}
 	}
@@ -1209,10 +1219,8 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 	return chan_cnt;
 
 err_exit:
-	for (i = 0; i < chan_cnt; i++)
-		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
-
-	kfree(swdma_dev->swdma_chans);
+	switchtec_dma_chans_release(pdev, swdma_dev);
+	switchtec_dma_chans_free(swdma_dev);
 
 	return rc;
 }
@@ -1221,12 +1229,8 @@ static void switchtec_dma_release(struct dma_device *dma_dev)
 {
 	struct switchtec_dma_dev *swdma_dev =
 		container_of(dma_dev, struct switchtec_dma_dev, dma_dev);
-	int i;
 
-	for (i = 0; i < swdma_dev->chan_cnt; i++)
-		kfree(swdma_dev->swdma_chans[i]);
-
-	kfree(swdma_dev->swdma_chans);
+	switchtec_dma_chans_free(swdma_dev);
 
 	put_device(dma_dev->dev);
 	kfree(swdma_dev);
@@ -1317,6 +1321,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 
 err_chans_release_exit:
 	switchtec_dma_chans_release(pdev, swdma_dev);
+	switchtec_dma_chans_free(swdma_dev);
 
 err_exit:
 	if (swdma_dev->chan_status_irq)
-- 
2.47.3

Re: [PATCH v2 04/11] dmaengine: switchtec-dma: fix channel leak on registration failure

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Use-After-Free of `swdma_dev` when unmapping BAR in driver removal.
--

--- Patch [4]: [PATCH v2 04/11] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 180269dedc262dabc95c3a154c59d53224e70c16
Author: Logan Gunthorpe <logang@deltatee.com>

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

    switchtec_dma_chans_release() is called in three places but the
    underlying memory is not freed in all of those places. In order to
    clean this up, introduce a switchtec_dma_chans_free() helper that
    will free the memory. Ensure each call to switchtec_dma_chans_release()
    has a corresponding switchtec_dma_chans_free() call.

    Link: https://lore.kernel.org/dmaengine/20260717223024.9BB8A1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 107769cca772f..13efd4189bbb4 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
[ ... ]
> @@ -1221,12 +1229,8 @@ static void switchtec_dma_release(struct dma_device *dma_dev)
>  {
>  	struct switchtec_dma_dev *swdma_dev =
>  		container_of(dma_dev, struct switchtec_dma_dev, dma_dev);
> -	int i;
>  
> -	for (i = 0; i < swdma_dev->chan_cnt; i++)
> -		kfree(swdma_dev->swdma_chans[i]);
> -
> -	kfree(swdma_dev->swdma_chans);
> +	switchtec_dma_chans_free(swdma_dev);
>  
>  	put_device(dma_dev->dev);
>  	kfree(swdma_dev);

[Severity: Critical]
This is a pre-existing issue, but does this kfree() result in a
use-after-free during driver removal?

In switchtec_dma_remove(), when dma_async_device_unregister() drops the
final reference, it synchronously invokes this switchtec_dma_release()
callback which frees swdma_dev.

However, switchtec_dma_remove() then immediately accesses the freed
swdma_dev pointer:

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

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

Should the iounmap() be moved before dma_async_device_unregister(), or
should the bar pointer be saved to a local variable to prevent memory
corruption?

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

[PATCH v2 05/11] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

[Logan Gunthorpe]

It always returned 0, and no caller checked it.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 13efd4189bbb..c752a1b05871 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1165,15 +1165,13 @@ static int switchtec_dma_chan_free(struct pci_dev *pdev,
 	return 0;
 }
 
-static int switchtec_dma_chans_release(struct pci_dev *pdev,
-				       struct switchtec_dma_dev *swdma_dev)
+static void switchtec_dma_chans_release(struct pci_dev *pdev,
+					struct switchtec_dma_dev *swdma_dev)
 {
 	int i;
 
 	for (i = 0; i < swdma_dev->chan_cnt; i++)
 		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
-
-	return 0;
 }
 
 static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
-- 
2.47.3

Re: [PATCH v2 05/11] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `ioremap()` is not checked for NULL in `switchtec_dma_create`.
--

--- Patch [5]: [PATCH v2 05/11] dmaengine: switchtec-dma: make switchtec_dma_chans_release() void ---
commit f0ba8482174958251130470e113a8f5fc0e207f1
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: make switchtec_dma_chans_release() void

It always returned 0, and no caller checked it.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 13efd4189bbb4..c752a1b05871d 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1165,15 +1165,13 @@ static int switchtec_dma_chan_free(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> -static int switchtec_dma_chans_release(struct pci_dev *pdev,
> -				       struct switchtec_dma_dev *swdma_dev)
> +static void switchtec_dma_chans_release(struct pci_dev *pdev,
> +					struct switchtec_dma_dev *swdma_dev)
>  {
>  	int i;
>  
>  	for (i = 0; i < swdma_dev->chan_cnt; i++)
>  		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);
> -
> -	return 0;
>  }
>  
>  static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed a
pre-existing issue in switchtec_dma_create():

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

        RCU_INIT_POINTER(swdma_dev->pdev, pdev);

        nr_vecs = pci_msix_vec_count(pdev);
        rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
        if (rc < 0)
                goto err_exit;

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

Does this code check if ioremap() returns NULL?

If ioremap() fails due to memory pressure or physical address space
exhaustion, swdma_dev->bar will be NULL.

The readw() call will then dereference this NULL pointer, which causes
a kernel oops on the device probe path.

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

[PATCH v2 06/11] dmaengine: switchtec-dma: unlink channels before freeing on registration failure

[Logan Gunthorpe]

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels (either from switchtec_dma_chans_enumerate()'s own
error path, or from dma_async_device_register() failing), the channels
are released and freed but never removed from dma_dev->channels.

The channel status IRQ is already live at this point, and its handler
walks dma_dev->channels, so it can dereference a freed channel.

Add switchtec_dma_chans_unlist() and call it before releasing and
freeing channels in both error paths.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index c752a1b05871..2a1ae27bd1a6 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1184,6 +1184,14 @@ static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
 	kfree(swdma_dev->swdma_chans);
 }
 
+static void switchtec_dma_chans_unlist(struct switchtec_dma_dev *swdma_dev)
+{
+	int i;
+
+	for (i = 0; i < swdma_dev->chan_cnt; i++)
+		list_del(&swdma_dev->swdma_chans[i]->dma_chan.device_node);
+}
+
 static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 					 struct pci_dev *pdev, int chan_cnt)
 {
@@ -1217,6 +1225,7 @@ static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
 	return chan_cnt;
 
 err_exit:
+	switchtec_dma_chans_unlist(swdma_dev);
 	switchtec_dma_chans_release(pdev, swdma_dev);
 	switchtec_dma_chans_free(swdma_dev);
 
@@ -1318,6 +1327,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	return 0;
 
 err_chans_release_exit:
+	switchtec_dma_chans_unlist(swdma_dev);
 	switchtec_dma_chans_release(pdev, swdma_dev);
 	switchtec_dma_chans_free(swdma_dev);
 
-- 
2.47.3

Re: [PATCH v2 06/11] dmaengine: switchtec-dma: unlink channels before freeing on registration failure

[sashiko-bot]

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

New issues:
- [High] The patch introduces a data race by calling `list_del()` concurrently with the active interrupt handler, resulting in a poisoned list pointer dereference.

Pre-existing issues:
- [High] The channel status IRQ is requested prematurely before the `dma->channels` list and `chan->dev` are fully initialized, causing NULL pointer dereferences in the ISR.
- [High] The error path in `switchtec_dma_create()` incorrectly uses `free_irq()` with an MSI-X vector index instead of `pci_free_irq()`, and fails to free vector index 0.
- [High] The number of DMA channels is read directly from an untrusted hardware register and used without bounds validation, leading to out-of-bounds MMIO accesses.
--

--- Patch [6]: [PATCH v2 06/11] dmaengine: switchtec-dma: unlink channels before freeing on registration failure ---
commit 304c38996e78423c61828786ab626c7f864c96fd
Author: Logan Gunthorpe <logang@deltatee.com>

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

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels (either from switchtec_dma_chans_enumerate()'s own
error path, or from dma_async_device_register() failing), the channels
are released and freed but never removed from dma_dev->channels.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index c752a1b05871d..2a1ae27bd1a69 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1184,6 +1184,14 @@ static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
>  	kfree(swdma_dev->swdma_chans);
>  }
>  
> +static void switchtec_dma_chans_unlist(struct switchtec_dma_dev *swdma_dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		list_del(&swdma_dev->swdma_chans[i]->dma_chan.device_node);
> +}
> +

[Severity: High]
Could this regression introduce a data race?

In switchtec_dma_create(), the error paths call switchtec_dma_chans_unlist()
while the channel status irq is still active. If an interrupt fires
concurrently, switchtec_dma_chan_status_isr() will traverse the
dma_dev->channels list locklessly using list_for_each_entry().

Can the isr traverse into the poisoned pointer from list_del() and crash
the system if it runs during or after this call? Should the irq be freed
or disabled before cleaning up the channels?

>  static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
>  					 struct pci_dev *pdev, int chan_cnt)

[ ... ]

> @@ -1318,6 +1327,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	return 0;
>  
>  err_chans_release_exit:
> +	switchtec_dma_chans_unlist(swdma_dev);
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  	switchtec_dma_chans_free(swdma_dev);
>  

[Severity: High]
This is a pre-existing issue, but is the channel status irq requested
prematurely in switchtec_dma_create()?

The irq is requested via pci_request_irq() before
switchtec_dma_chans_enumerate() is called and before the dma->channels list
is fully initialized.

If an interrupt fires immediately, switchtec_dma_chan_status_isr() will iterate
over an uninitialized list. Furthermore, the isr accesses chan->dev, which
is not set until dma_async_device_register() runs much later. Could this
cause a null pointer dereference?

[Severity: High]
This is a pre-existing issue, but does the error path in switchtec_dma_create()
fail to cleanly release the irq?

The code checks if swdma_dev->chan_status_irq is valid and calls free_irq().
If the vector index is 0, the check skips freeing, leaking the irq.
Additionally, if it is greater than 0, it passes the msi-x vector index
to free_irq() instead of the global linux irq number, which fails to free it.

If switchtec_dma_create() fails and swdma_dev is freed via kfree(), could a
subsequent interrupt execute the active isr and access the freed swdma_dev,
causing a use-after-free?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_create() read the channel
count without upper bounds validation?

The chan_cnt is read directly from the hardware register.
During enumeration in switchtec_dma_chan_init(), pointer arithmetic is
performed based on this count:

swdma_chan->mmio_chan_fw = swdma_dev->bar + offset + i * size;

If chan_cnt is excessively large due to untrusted or malfunctioning hardware
input, could this pointer extend beyond the bounds of the mapped pci bar
and cause an out-of-bounds mmio access when dereferenced?

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

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

[Logan Gunthorpe]

dma_async_device_unregister() can drop the last reference on dma_dev
and free swdma_dev synchronously via switchtec_dma_release(), but
switchtec_dma_remove() then uses swdma_dev->bar for iounmap().

Cache bar in a local variable before the unregister call.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 2a1ae27bd1a6..a7e25cb7c0b9 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1376,6 +1376,7 @@ static int switchtec_dma_probe(struct pci_dev *pdev,
 static void switchtec_dma_remove(struct pci_dev *pdev)
 {
 	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
+	void __iomem *bar = swdma_dev->bar;
 
 	switchtec_dma_chans_release(pdev, swdma_dev);
 
@@ -1388,7 +1389,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
 
 	dma_async_device_unregister(&swdma_dev->dma_dev);
 
-	iounmap(swdma_dev->bar);
+	iounmap(bar);
 	pci_release_mem_regions(pdev);
 	pci_disable_device(pdev);
 }
-- 
2.47.3

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect IRQ cleanup in `switchtec_dma_create` error path using `free_irq` with a hardware vector index instead of `pci_free_irq`.
- [High] Pending DMA transactions are not aborted during device removal, potentially causing DMA clients to hang indefinitely.
- [High] Missing bounds check on the hardware-provided channel count allows out-of-bounds MMIO accesses.
- [Medium] Missing `put_device(dma->dev)` in the `switchtec_dma_create` error path causes a reference count leak on the PCI device.
--

--- Patch [7]: [PATCH v2 07/11] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove() ---
commit 0e37b79ac2f6503c0a4dc42c7b0cf19efff70cbd
Author: Logan Gunthorpe <logang@deltatee.com>

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

This patch fixes a use-after-free regression in switchtec_dma_remove() by
caching the PCI BAR in a local variable before dma_async_device_unregister()
is called, avoiding access to swdma_dev after it is freed.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 2a1ae27bd1a69..a7e25cb7c0b9f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
[ ... ]
> @@ -1388,7 +1389,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
>  
>  	dma_async_device_unregister(&swdma_dev->dma_dev);
>  
> -	iounmap(swdma_dev->bar);
> +	iounmap(bar);
>  	pci_release_mem_regions(pdev);
>  	pci_disable_device(pdev);
>  }

While reviewing this patch, I noticed a few pre-existing issues in the driver
that might be worth addressing.

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
    chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);

[Severity: High]
This is a pre-existing issue, but is it safe to use this hardware-provided
channel count without bounds checking?

If the endpoint returns an unexpectedly large channel count, this value is
later used in switchtec_dma_chan_init() to calculate MMIO offsets:

    swdma_chan->mmio_chan_fw =
        swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
        i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;

Since chan_cnt is never validated against the mapped length of the PCI BAR,
could this result in out-of-bounds MMIO accesses?

    if (!chan_cnt) {
        pci_err(pdev, "No channel configured.\n");
        rc = -ENXIO;
        goto err_exit;
    }
    ...
err_chans_release_exit:
    switchtec_dma_chans_unlist(swdma_dev);
    switchtec_dma_chans_release(pdev, swdma_dev);
    switchtec_dma_chans_free(swdma_dev);

[Severity: Medium]
This is another pre-existing issue, but does this error path leak the device
reference count?

If dma_async_device_register() fails earlier in this function, we jump here,
but there is no call to put_device() to balance the get_device() called
previously. This could leave the PCI device with an elevated reference count
permanently.

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

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free
if an interrupt fires?

The code requests the IRQ using pci_request_irq() with a hardware vector
index, but attempts to free it here with free_irq() using that same vector
index instead of the global Linux IRQ number. Since free_irq() expects a
global IRQ number, it may fail to free the interrupt. The handler would
remain registered while swdma_dev is freed right after this.

Also, if the hardware uses vector 0, the if statement skips cleanup entirely.
Should this use pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev)
instead?

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


drivers/dma/switchtec_dma.c:switchtec_dma_chan_free() {
    ...
    pci_free_irq(pdev, swdma_chan->irq, swdma_chan);
    tasklet_kill(&swdma_chan->desc_task);

    switchtec_dma_chan_stop(swdma_chan);

[Severity: High]
This is a pre-existing issue, but does removing the device without aborting
pending transactions cause DMA clients to hang indefinitely?

If a client is blocking on a completion object (e.g., via
wait_for_completion), and the device is removed, the IRQ is freed and the
hardware is stopped here. However, active descriptor rings aren't traversed
to report DMA_TRANS_ABORTED. Can this prevent the client from ever unbinding?

    return 0;
}

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

[PATCH v2 08/11] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[Logan Gunthorpe]

chan_status_irq stores an MSI-X vector index, but err_exit freed it
with plain free_irq() instead of pci_free_irq(), which would free the
wrong Linux IRQ. The guard also treated a valid vector index of 0 as
unset, skipping the free entirely in that case and leaving the handler
registered against soon-to-be-freed swdma_dev.

Initialize chan_status_irq to -1 and use the value being non-negative
to signal when to free it with pci_free_irq().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index a7e25cb7c0b9..4a80aad2d339 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1257,6 +1257,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	if (!swdma_dev)
 		return -ENOMEM;
 
+	swdma_dev->chan_status_irq = -1;
+
 	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
 				 pci_resource_len(pdev, 0));
 
@@ -1332,8 +1334,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
 	switchtec_dma_chans_free(swdma_dev);
 
 err_exit:
-	if (swdma_dev->chan_status_irq)
-		free_irq(swdma_dev->chan_status_irq, swdma_dev);
+	if (swdma_dev->chan_status_irq >= 0)
+		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
 
 	iounmap(swdma_dev->bar);
 	kfree(swdma_dev);
-- 
2.47.3

Re: [PATCH v2 08/11] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of ioremap is not checked, leading to a guaranteed NULL pointer dereference if the mapping fails.
- [High] The IRQ handler is registered and enabled before the DMA channels list head is initialized, creating a race condition that can crash the kernel.
--

--- Patch [8]: [PATCH v2 08/11] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error ---
commit ceb60e37fe28d72b900ae9ddab80b66c36311831
Author: Logan Gunthorpe <logang@deltatee.com>

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

This commit fixes an issue where chan_status_irq was freed with plain
free_irq() instead of pci_free_irq(). It also initializes the irq to -1
to properly track its allocation state.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1257,6 +1257,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	if (!swdma_dev)
>  		return -ENOMEM;
>  
> +	swdma_dev->chan_status_irq = -1;
> +
>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));

[Severity: High]
This is a pre-existing issue, but the return value of ioremap() is not
checked here.

If this allocation fails (e.g., due to exhausted vmalloc space) and returns
NULL, won't this cause a guaranteed NULL pointer dereference when the driver
later calls readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC)?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a race
condition regarding the IRQ registration in this same function.

Further down in switchtec_dma_create(), the driver registers the IRQ
handler:

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

At this point, the dma_dev->channels list is uninitialized (zero-filled by
kzalloc_obj). The required INIT_LIST_HEAD() is not called until later inside
switchtec_dma_chans_enumerate().

If an interrupt fires immediately after registration, won't the ISR crash
when it iterates over the uninitialized list head?

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
    ...
	list_for_each_entry(chan, &dma_dev->channels, device_node) {
		swdma_chan = container_of(chan, struct switchtec_dma_chan,
					  dma_chan);
		chan_dev = &swdma_chan->dma_chan.dev->device;
    ...
}

Could the IRQ registration be moved after switchtec_dma_chans_enumerate(),
or the list initialized earlier?

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

[PATCH v2 09/11] 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
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/ioat/init.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 737496391109..a57024c4b066 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1170,15 +1170,6 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 		       ioat_chan->reg_base + IOAT_DCACTRL_OFFSET);
 	}
 
-	err = dma_async_device_register(&ioat_dma->dma_dev);
-	if (err)
-		goto err_disable_interrupts;
-
-	ioat_kobject_add(ioat_dma, &ioat_ktype);
-
-	if (dca)
-		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
-
 	/* disable relaxed ordering */
 	err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16);
 	if (err) {
@@ -1194,6 +1185,15 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 		goto err_disable_interrupts;
 	}
 
+	err = dma_async_device_register(&ioat_dma->dma_dev);
+	if (err)
+		goto err_disable_interrupts;
+
+	ioat_kobject_add(ioat_dma, &ioat_ktype);
+
+	if (dca)
+		ioat_dma->dca = ioat_dca_init(pdev, ioat_dma->reg_base);
+
 	if (ioat_dma->cap & IOAT_CAP_DPS)
 		writeb(ioat_pending_level + 1,
 		       ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);
-- 
2.47.3

Re: [PATCH v2 09/11] dmaengine: ioat: disable relaxed ordering before registering the device

[Dave Jiang]

On 7/21/26 8:57 AM, Logan Gunthorpe wrote:
> 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
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Dave Jiang <dave.jiang@intel.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);

Re: [PATCH v2 09/11] 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] Use-After-Free in interrupt handler due to devres and manual memory management mismatch on probe error path.
- [High] PCI MSI/MSI-X vectors are leaked on probe failure and device removal.
- [High] IDA allocation nodes are permanently leaked in the dmaengine core when `dma_async_device_register` partially fails.
--

--- Patch [9]: [PATCH v2 09/11] dmaengine: ioat: disable relaxed ordering before registering the device ---
commit 0ad76783c9aa39fb6b36953c767e2a29d68490df
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: ioat: disable relaxed ordering before registering the device

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. Move the capability read/write ahead of
dma_async_device_register() instead.

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

> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index 737496391109e..a57024c4b066e 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -1170,15 +1170,6 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
[ ... ]
>  	/* 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;
>  	}

[Severity: High]
This is a pre-existing issue, but can we hit a use-after-free if this error
path is taken?

If ioat3_dma_probe() fails here and jumps to err_disable_interrupts, the
driver returns an error to ioat_pci_probe(). ioat_pci_probe() then manually
frees the device and its channels:

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

However, the interrupt handler was registered with devm_request_irq()
earlier in ioat_dma_setup_interrupts():

    err = devm_request_irq(dev, pdev->irq, ioat_dma_do_interrupt,
			   IRQF_SHARED, "ioat-intx", ioat_dma);

Since devres callbacks only execute after ioat_pci_probe() completes, the
shared IRQ remains active while the memory is freed.

If another device triggers a shared interrupt during this window, wouldn't
ioat_dma_do_interrupt() dereference the freed device pointer?

drivers/dma/ioat/dma.c:ioat_dma_do_interrupt() {
    struct ioatdma_device *instance = data;
    ...
    intrctrl = readb(instance->reg_base + IOAT_INTRCTRL_OFFSET);
    ...
}

[Severity: High]
This is another pre-existing issue, but does this probe error path leak
PCI MSI/MSI-X vectors?

The driver allocates vectors using pci_enable_msix_exact() or
pci_enable_msi() in ioat_dma_setup_interrupts(), but it doesn't appear
that either err_disable_interrupts or the device unbind function
ioat_dma_remove() ever calls pci_disable_msi() or pci_disable_msix() to
release them:

drivers/dma/ioat/init.c:ioat_dma_remove() {
	struct dma_device *dma = &ioat_dma->dma_dev;

	ioat_disable_interrupts(ioat_dma);
	ioat_kobject_del(ioat_dma);
	dma_async_device_unregister(dma);
}

Could this lead to resource exhaustion if the device is repeatedly unbound
or fails probe?

> +	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 does the dmaengine core leak
IDA allocation nodes when dma_async_device_register() partially fails?

If channel registration succeeds for at least one channel but fails later,
the code jumps to the err_out label. Because device->chancnt is non-zero,
it skips freeing the global dma_ida:

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

It then loops over the channels to unregister them, but fails to call
ida_free() to release the individual channel IDs allocated from
device->chan_ida:

	list_for_each_entry(chan, &device->channels, device_node) {
		if (chan->local == NULL)
			continue;
		mutex_lock(&dma_list_mutex);
		chan->dev->chan = NULL;
		mutex_unlock(&dma_list_mutex);
		device_unregister(&chan->dev->device);
		free_percpu(chan->local);
	}

Would this permanently leak IDA structures and IDs for any DMA driver
failing partially during registration?

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

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

[PATCH v2 10/11] 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 v2 10/11] 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/20260721155739.62120-1-logang@deltatee.com?part=10

Re: [PATCH v2 10/11] dmaengine: ioat: use sysfs_emit() in per-channel sysfs show()

[Dave Jiang]

On 7/21/26 8:57 AM, Logan Gunthorpe wrote:
> Convert the sprintf() calls in the per-channel sysfs attribute show()
> functions to sysfs_emit().
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>

Acked-by: Dave Jiang <dave.jiang@intel.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,

[PATCH v2 11/11] dmaengine: plx_dma: fix NULL pointer deref in plx_dma_isr()

[Logan Gunthorpe]

plx_dma_create() registered the interrupt handler with request_irq()
before initializing plxdev->bar. If the device raised an interrupt in
that window, plx_dma_isr() would dereference the still-NULL bar.

Move the bar assignment ahead of request_irq() so everything the
handler can touch is initialized before it can run.

Reported-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
Reported-by: Kyungwook Boo <bookyungwook@gmail.com>
Link: https://lore.kernel.org/all/20260610112121.676561-1-jjy600901@snu.ac.kr
Fixes: c2dbcaa8c672 ("dmaengine: plx-dma: Implement hardware initialization and cleanup")
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 v2 11/11] 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] Deadlock in `plx_dma_process_desc()` because it uses `spin_lock()` instead of `spin_lock_bh()`, allowing preemption by softirqs/tasklets.
- [High] Recursive deadlock in `plx_dma_process_desc()` and `plx_dma_abort_desc()` due to invoking client callbacks while holding `ring_lock`.
--

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

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

This commit moves the plxdev->bar initialization before calling
request_irq(). This prevents a NULL pointer dereference that could occur if
an interrupt fires immediately and plx_dma_isr() attempts to access the
uninitialized bar.

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

> diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
> index 84941a918b01f..409898e92c320 100644
> --- a/drivers/dma/plx_dma.c
> +++ b/drivers/dma/plx_dma.c
> @@ -504,17 +504,17 @@ static int plx_dma_create(struct pci_dev *pdev)
>  	if (!plxdev)
>  		return -ENOMEM;
>  
> -	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> -			 KBUILD_MODNAME, plxdev);
> -	if (rc)
> -		goto free_plx;
> -
>  	spin_lock_init(&plxdev->ring_lock);
>  	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);
>  
>  	RCU_INIT_POINTER(plxdev->pdev, pdev);
>  	plxdev->bar = pcim_iomap_table(pdev)[0];
>  
> +	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
> +			 KBUILD_MODNAME, plxdev);
> +	if (rc)
> +		goto free_plx;
> +

[Severity: High]
This is a pre-existing issue, but can this code cause a deadlock?

When plx_dma_tx_status() is called from process context with softirqs enabled,
it invokes plx_dma_process_desc() which acquires ring_lock using spin_lock():

drivers/dma/plx_dma.c:plx_dma_tx_status() {
    ...
    plx_dma_process_desc(plxdev);
    ...
}

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

If a hardware interrupt occurs on the same CPU while the lock is held, the IRQ
handler schedules the desc_task tasklet. When the IRQ returns, the softirq
handler executes the tasklet on the same CPU, which also calls
plx_dma_process_desc() and attempts to acquire the already-held lock.

Should plx_dma_process_desc() use spin_lock_bh() instead to disable softirqs?

[Severity: High]
This is a pre-existing issue, but does invoking the client callback while
holding ring_lock cause a recursive deadlock?

Looking at plx_dma_process_desc(), the client callback is invoked via
dmaengine_desc_get_callback_invoke() while the lock is still held:

drivers/dma/plx_dma.c:plx_dma_process_desc() {
    ...
    spin_lock(&plxdev->ring_lock);
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

This also happens during abort:

drivers/dma/plx_dma.c:plx_dma_abort_desc() {
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

If the DMA client's completion callback prepares and submits chained DMA
transactions by calling dmaengine_prep_dma_memcpy(), it re-enters the driver:

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

Could this attempt to re-acquire the non-recursive ring_lock on the same CPU
cause a hard lockup? Should the lock be dropped before invoking the callback?

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