[PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Donggeun Yoo]

dma_alloc_attrs() passes *dma_handle to trace_dma_alloc() and
debug_dma_alloc_coherent() without checking whether the allocation
succeeded. No backend writes it on failure: dma_direct_alloc(),
iommu_dma_alloc() and the dma_map_ops instances assign it only on the
path that returns a buffer. Callers usually pass an uninitialized
automatic variable, so with the tracepoint enabled a failed allocation
records whatever the stack held, next to the virt_addr=(null) that marks
the record as an error.

The device coherent pool path is the same: a non-zero return from
dma_alloc_from_dev_coherent() means the request was handled, not that it
succeeded, so cpu_addr is NULL and dma_handle untouched once the pool
runs out.

Split both sites on cpu_addr, as dma_alloc_pages() and
dma_alloc_noncontiguous() do further down the file, and pass 0 for the
failure case like the two error paths already in this function.

Fixes: 038eb433dc14 ("dma-mapping: add tracing for dma-mapping API calls")
Fixes: 68b6dbf1f441 ("dma-mapping: trace more error paths")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
Compile-tested only, with CONFIG_DMA_API_DEBUG=y and CONFIG_TRACEPOINTS=y
so that both changed calls are built. The claim that no backend writes
*dma_handle on failure was checked against dma_direct_alloc() and its
helpers, iommu_dma_alloc(), iommu_dma_alloc_remap() and every in-tree
dma_map_ops .alloc implementation.

 kernel/dma/mapping.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index bf2651a70b7c..098cd57e1157 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -656,8 +656,12 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
 		attrs |= __DMA_ATTR_ALLOC_CC_SHARED;
 
 	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) {
-		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
-				DMA_BIDIRECTIONAL, flag, attrs);
+		if (cpu_addr)
+			trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
+					DMA_BIDIRECTIONAL, flag, attrs);
+		else
+			trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL,
+					flag, attrs);
 		return cpu_addr;
 	}
 
@@ -676,9 +680,15 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
 		return NULL;
 	}
 
-	trace_dma_alloc(dev, cpu_addr, *dma_handle, size, DMA_BIDIRECTIONAL,
-			flag, attrs);
-	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr, attrs);
+	if (cpu_addr) {
+		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
+				DMA_BIDIRECTIONAL, flag, attrs);
+		debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr,
+					 attrs);
+	} else {
+		trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL, flag,
+				attrs);
+	}
 	return cpu_addr;
 }
 EXPORT_SYMBOL(dma_alloc_attrs);
-- 
2.53.0

Re: [PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Sean Anderson]

On 9/5/26 03:19, Donggeun Yoo wrote:
> dma_alloc_attrs() passes *dma_handle to trace_dma_alloc() and
> debug_dma_alloc_coherent() without checking whether the allocation
> succeeded.

I don't understand what you mean. We are literally in an if-statement
that checks whether the allocation succeeded.

--Sean

> No backend writes it on failure: dma_direct_alloc(),
> iommu_dma_alloc() and the dma_map_ops instances assign it only on the
> path that returns a buffer. Callers usually pass an uninitialized
> automatic variable, so with the tracepoint enabled a failed allocation
> records whatever the stack held, next to the virt_addr=(null) that marks
> the record as an error.
> 
> The device coherent pool path is the same: a non-zero return from
> dma_alloc_from_dev_coherent() means the request was handled, not that it
> succeeded, so cpu_addr is NULL and dma_handle untouched once the pool
> runs out.
> 
> Split both sites on cpu_addr, as dma_alloc_pages() and
> dma_alloc_noncontiguous() do further down the file, and pass 0 for the
> failure case like the two error paths already in this function.
> 
> Fixes: 038eb433dc14 ("dma-mapping: add tracing for dma-mapping API calls")
> Fixes: 68b6dbf1f441 ("dma-mapping: trace more error paths")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> Compile-tested only, with CONFIG_DMA_API_DEBUG=y and CONFIG_TRACEPOINTS=y
> so that both changed calls are built. The claim that no backend writes
> *dma_handle on failure was checked against dma_direct_alloc() and its
> helpers, iommu_dma_alloc(), iommu_dma_alloc_remap() and every in-tree
> dma_map_ops .alloc implementation.
> 
>   kernel/dma/mapping.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index bf2651a70b7c..098cd57e1157 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -656,8 +656,12 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   		attrs |= __DMA_ATTR_ALLOC_CC_SHARED;
>   
>   	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) {
> -		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> -				DMA_BIDIRECTIONAL, flag, attrs);
> +		if (cpu_addr)
> +			trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> +					DMA_BIDIRECTIONAL, flag, attrs);
> +		else
> +			trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL,
> +					flag, attrs);
>   		return cpu_addr;
>   	}
>   
> @@ -676,9 +680,15 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   		return NULL;
>   	}
>   
> -	trace_dma_alloc(dev, cpu_addr, *dma_handle, size, DMA_BIDIRECTIONAL,
> -			flag, attrs);
> -	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr, attrs);
> +	if (cpu_addr) {
> +		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> +				DMA_BIDIRECTIONAL, flag, attrs);
> +		debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr,
> +					 attrs);
> +	} else {
> +		trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL, flag,
> +				attrs);
> +	}
>   	return cpu_addr;
>   }
>   EXPORT_SYMBOL(dma_alloc_attrs);

Re: [PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Donggeun Yoo]

On 9/5/26 08:57, Sean Anderson wrote:
> I don't understand what you mean. We are literally in an if-statement
> that checks whether the allocation succeeded.

That if checks whether the device has a coherent pool, not whether the
allocation out of it worked. dma_alloc_from_dev_coherent():

 * Returns 0 if dma_alloc_coherent should continue with allocating from
 * generic memory areas, or !0 if dma_alloc_coherent should return @ret.

It returns 1 as soon as it finds a pool; __dma_alloc_from_coherent()
stores NULL in @ret and leaves *dma_handle alone when the request is
larger than the pool or the pool is full. The third paragraph of the
commit message says this.

The second hunk is under no condition at all: dma_direct_alloc(),
iommu_dma_alloc() and ops->alloc() return NULL on failure, and the trace
below the if/else chain runs either way.

Re: [PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Sean Anderson]

On 9/5/26 13:26, Donggeun Yoo wrote:
> On 9/5/26 08:57, Sean Anderson wrote:
>> I don't understand what you mean. We are literally in an if-statement
>> that checks whether the allocation succeeded.
> 
> That if checks whether the device has a coherent pool, not whether the
> allocation out of it worked. dma_alloc_from_dev_coherent():
> 
>   * Returns 0 if dma_alloc_coherent should continue with allocating from
>   * generic memory areas, or !0 if dma_alloc_coherent should return @ret.
> 
> It returns 1 as soon as it finds a pool; __dma_alloc_from_coherent()
> stores NULL in @ret and leaves *dma_handle alone when the request is
> larger than the pool or the pool is full. The third paragraph of the
> commit message says this.
> 
> The second hunk is under no condition at all: dma_direct_alloc(),
> iommu_dma_alloc() and ops->alloc() return NULL on failure, and the trace
> below the if/else chain runs either way.

Ah, I see what you mean. TBH I would just do something like

cpu_handle ? *dma_handle : NULL

to make it clear what we're doing

--Sean

Re: [PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Donggeun Yoo]

On 9/5/26 14:37, Sean Anderson wrote:
> Ah, I see what you mean. TBH I would just do something like
> 
> cpu_handle ? *dma_handle : NULL
> 
> to make it clear what we're doing

It would have to be 0, dma_handle being a dma_addr_t, and
debug_dma_alloc_coherent() reads *dma_handle too -- it drops the value
when virt is NULL, but the read is still there -- so the conditional ends
up written three times.

68b6dbf1f441 gave dma_alloc_pages() and dma_alloc_noncontiguous() the
if/else split rather than a guarded argument, and I kept dma_alloc_attrs()
in that shape so that the three read the same way.

I don't feel strongly about it. Say the word and I'll respin with the
conditional inline.