Re: [PATCH v2 08/12] dmaengine: change dmaengine_get_unmap_data() first arg to dma_chan

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: NULL pointer dereference in mv_xor self-tests due to uninitialized chan_dev
- [High] dmaengine: DMA device mismatch for mapping and unmapping in NTB and mv_xor drivers
--

commit ddb68198d418b187ac31a8ab8dc1644ae5ebab5d
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: change dmaengine_get_unmap_data() first arg to dma_chan

Replace the struct device *dev argument with struct dma_chan *chan so callers
do not need to reach into the dma_device internals to obtain the device
pointer. The implementation now derives the device via
dmaengine_get_dma_device(chan), which correctly handles the per-channel
IOMMU mapping case.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index c4e0dce3ca64e..dec01b6264d7a 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -794,7 +794,7 @@ static int mv_chan_memcpy_self_test(struct mv_xor_chan *mv_chan)
>  		goto out;
>  	}
>  
> -	unmap = dmaengine_get_unmap_data(dma_chan->device->dev, 2, GFP_KERNEL);
> +	unmap = dmaengine_get_unmap_data(dma_chan, 2, GFP_KERNEL);

[Severity: High]
Does this cause a NULL pointer dereference on initialization?

The driver mv_xor runs self-tests like mv_chan_memcpy_self_test() during
probe before calling dma_async_device_register().

Because this patch changes dmaengine_get_unmap_data() to call
dmaengine_get_dma_device(chan), it unconditionally dereferences
chan->chan_dev. Since chan->chan_dev is allocated inside
dma_async_device_register(), it is NULL when the self-tests run.

Could this also lead to a device mismatch for DMA mapping?

Later in mv_chan_memcpy_self_test(), dma_map_page() still explicitly uses
dma_chan->device->dev instead of tracking with the new unmap device logic:

drivers/dma/mv_xor.c:mv_chan_memcpy_self_test() {
    ...
	src_dma = dma_map_page(dma_chan->device->dev, virt_to_page(src),
			       offset_in_page(src), PAGE_SIZE,
			       DMA_TO_DEVICE);
    ...
}

Mapping memory using one device pointer and unmapping it via a different one
violates the DMA API contract, which can lead to IOMMU faults and memory
leaks on platforms where the channel uses a separate device.

> @@ -927,7 +927,7 @@ mv_chan_xor_self_test(struct mv_xor_chan *mv_chan)
>  		goto out;
>  	}
>  
> -	unmap = dmaengine_get_unmap_data(dma_chan->device->dev, src_count + 1,
> +	unmap = dmaengine_get_unmap_data(dma_chan, src_count + 1,
>  					 GFP_KERNEL);

[Severity: High]
Will this also trigger the same NULL pointer dereference here since it
runs during probe before registration?

It also has the same mapping mismatch issue where dma_map_page() explicitly
calls out dma_chan->device->dev instead of using the new unmap device logic.

[ ... ]

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f9caa1a653c5b..e1cb55890d72b 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1532,7 +1532,7 @@ static int ntb_async_rx_submit(struct ntb_queue_entry *entry, void *offset)
>  	if (!is_dma_copy_aligned(device, pay_off, buff_off, len))
>  		goto err;
>  
> -	unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOWAIT);
> +	unmap = dmaengine_get_unmap_data(chan, 2, GFP_NOWAIT);

[Severity: High]
Is there a device mismatch for DMA mapping here as well?

While unmap now uses dmaengine_get_dma_device(chan), ntb_async_rx_submit()
still explicitly maps memory using the parent device:

drivers/ntb/ntb_transport.c:ntb_async_rx_submit() {
    ...
	unmap->addr[0] = dma_map_phys(device->dev, virt_to_phys(offset),
				      len, DMA_TO_DEVICE, 0);
    ...
}

This mismatch between the mapping device and the unmapping device can lead
to IOMMU faults and memory leaks.

> @@ -1872,7 +1872,7 @@ static int ntb_async_tx_submit(struct ntb_transport_qp *qp,
>  	if (!is_dma_copy_aligned(device, buff_off, dest_off, len))
>  		goto err;
>  
> -	unmap = dmaengine_get_unmap_data(device->dev, 1, GFP_NOWAIT);
> +	unmap = dmaengine_get_unmap_data(chan, 1, GFP_NOWAIT);

[Severity: High]
Does this suffer from the same mapping mismatch?

The dma_map_phys() call just below this still maps memory using the
parent device (device->dev).

[ ... ]

> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> index dfd175f79e8f0..5fea36432ba92 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -809,7 +809,7 @@ static int perf_copy_chunk(struct perf_thread *pthr,
>  	dst_vaddr = dst;
>  	dst_dma_addr = peer->dma_dst_addr + (dst_vaddr - vbase);
>  
> -	unmap = dmaengine_get_unmap_data(dma_dev, 1, GFP_NOWAIT);
> +	unmap = dmaengine_get_unmap_data(pthr->dma_chan, 1, GFP_NOWAIT);

[Severity: High]
Might this also create a device mismatch for DMA mapping?

In perf_copy_chunk(), dma_dev is locally initialized to
pthr->dma_chan->device->dev, and used for dma_map_page():

drivers/ntb/test/ntb_perf.c:perf_copy_chunk() {
    ...
	unmap->addr[0] = dma_map_page(dma_dev, virt_to_page(src),
		offset_in_page(src), len, DMA_TO_DEVICE);
    ...
}

This maps memory using the parent device but unmaps it using the new
per-channel unmap device logic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-dmaengine_prep_dma_pq-v2-0-32ed65b8a9b4@nxp.com?part=8