Re: [PATCH v6 7/9] nvmet: pci-epf: Use dmaengine_prep_config_single_safe() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removing the mutex introduces a cascading teardown failure and use-after-free where a single DMA timeout forcefully terminates all concurrent transfers on the shared channel.
- [High] Massive CPU wastage and starvation due to un-serialized concurrent busy-waiting on the DMA channel.
- [Low] Unused mutex fields and their initialization/teardown code were left behind in the structure and lifecycle functions.
--

commit 9bda24c8bcaef3ff0459f6035b72408052dec87b
Author: Frank Li <Frank.Li@nxp.com>

nvmet: pci-epf: Use dmaengine_prep_config_single_safe() API

This commit replaces dmaengine_slave_config() and dmaengine_prep_slave_single()
with dmaengine_prep_config_single_safe(), combining the configuration and
preparation into a single call. It also removes the mutex locks under the
assumption that atomic configuration makes them unnecessary.

> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index 2afe8f4d0e461..f917d6ec278b7 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -368,18 +368,15 @@ static int nvmet_pci_epf_dma_transfer(struct nvmet_pci_epf *nvme_epf,
>  	struct dma_chan *chan;
>  	dma_cookie_t cookie;
>  	dma_addr_t dma_addr;
> -	struct mutex *lock;
>  	int ret;
>  
>  	switch (dir) {
>  	case DMA_FROM_DEVICE:
> -		lock = &nvme_epf->dma_rx_lock;
>  		chan = nvme_epf->dma_rx_chan;
>  		sconf.direction = DMA_DEV_TO_MEM;
>  		sconf.src_addr = seg->pci_addr;
>  		break;
>  	case DMA_TO_DEVICE:
> -		lock = &nvme_epf->dma_tx_lock;

[Severity: Low]
Are the dma_tx_lock and dma_rx_lock fields still needed in struct nvmet_pci_epf?

Since the locks are no longer used here, it seems the field declarations
and their initialization/teardown code might have been left behind.

>  		chan = nvme_epf->dma_tx_chan;
>  		sconf.direction = DMA_MEM_TO_DEV;
>  		sconf.dst_addr = seg->pci_addr;

[ ... ]

> @@ -388,22 +385,15 @@ static int nvmet_pci_epf_dma_transfer(struct nvmet_pci_epf *nvme_epf,
>  		return -EINVAL;
>  	}
>  
> -	mutex_lock(lock);
> -

[Severity: High]
Does removing the mutex introduce concurrent busy-waiting on the shared DMA
channel?

dma_sync_wait() internally uses a cpu_relax() loop to busy-wait for hardware
completion. Before this patch, the mutex ensured only one thread actively
polled the hardware at a time, while other concurrent workers slept efficiently
on mutex_lock().

By removing the mutex, it appears multiple worker threads could concurrently
enter dma_sync_wait() on the same channel, unnecessarily consuming 100% of
multiple CPU cores and potentially causing CPU exhaustion.

[Severity: High]
Can removing the mutex also lead to a cascading teardown failure and
use-after-free?

Because the mutex was removed, multiple worker threads can concurrently submit
and wait on the shared dma_tx_chan or dma_rx_chan. If one transfer times out
in dma_sync_wait(), the error path calls dmaengine_terminate_sync(chan).

It looks like this would forcefully abort the shared channel, destroying all
other active and pending descriptors belonging to concurrent NVMe commands.

Also, if Thread A calls dmaengine_terminate_sync(chan) while Thread B is
between dmaengine_prep_config_single_safe() and dmaengine_submit():

Thread B:
	desc = dmaengine_prep_config_single_safe(...)
	// Thread A calls dmaengine_terminate_sync() here
	cookie = dmaengine_submit(desc);

Could Thread B's descriptor be freed by the termination, resulting in a
use-after-free when Thread B attempts to submit it?

>  	dma_dev = dmaengine_get_dma_device(chan);
>  	dma_addr = dma_map_single(dma_dev, seg->buf, seg->length, dir);
>  	ret = dma_mapping_error(dma_dev, dma_addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-dma_prep_config-v6-0-06e49b7acb38@nxp.com?part=7