Re: [PATCH v2 1/2] dmaengine: Add helper dmaengine_prep_submit_slave_single()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The macro accesses the descriptor after calling `dmaengine_submit()`, violating the DMA Engine API contract and causing a Use-After-Free.
- [Medium] Calling `dmaengine_desc_free()` fails to prevent memory leaks for standard, non-reusable DMA descriptors, defeating the primary goal of the patch.
- [Low] The fallback implementation of `dmaengine_prep_submit_slave_single` contains severe syntax errors that break compilation when `CONFIG_DMA_ENGINE` is disabled.
--

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

dmaengine: Add helper dmaengine_prep_submit_slave_single()

This commit introduces a helper macro and function to combine the preparation
and submission of a DMA slave single transfer into a single step. It also
aims to automatically free the descriptor if the submission fails.

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index ca13cd39330ba..1e25be78a22a5 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -1619,6 +1619,34 @@ void dma_run_dependencies(struct dma_async_tx_descriptor *tx)
>  }
>  EXPORT_SYMBOL_GPL(dma_run_dependencies);
>  
> +#define dmaengine_prep_submit(chan, cb, cb_param, func, ...)	\
> +({	struct dma_async_tx_descriptor *tx =			\
> +		dmaengine_prep_##func(chan, __VA_ARGS__);	\
> +		dma_cookie_t cookie = -ENOMEM;			\
> +								\
> +	if (tx) {						\
> +		tx->callback = cb;				\
> +		tx->callback_param = cb_param;			\
> +		cookie = dmaengine_submit(tx);			\
> +								\
> +		if (dma_submit_error(cookie))			\
> +			dmaengine_desc_free(tx);		\

[Severity: Critical]
Does this access the descriptor after ownership has been transferred?

The DMA Engine API contract specifies that once dmaengine_submit is called,
the client must consider the descriptor pointer invalid. If submission fails,
some DMA providers (like the idxd driver in idxd_dma_tx_submit) internally
free the descriptor before returning an error.

Calling dmaengine_desc_free here would read tx->flags on an already-freed
pointer on the error path, resulting in a use-after-free.

[Severity: Medium]
Will this actually free standard, non-reusable descriptors?

The dmaengine_desc_free function is explicitly designed for reusable
descriptors. It internally calls dmaengine_desc_test_reuse, which checks
the DMA_CTRL_REUSE flag and returns -EPERM immediately if it is not set. 

Since standard descriptors created via dmaengine_prep_slave_single don't 
have this flag set by default, it seems this call acts as a no-op and might
not resolve the resource leak mentioned in the commit message.

> +	}							\
> +	cookie;							\
> +})
> +
[ ... ]
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 99efe2b9b4ea9..0f789fac7e91a 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
[ ... ]
> @@ -1575,6 +1582,16 @@ static inline int dma_get_slave_caps(struct dma_chan *chan,
>  {
>  	return -ENXIO;
>  }
> +
> +static inline dma_cookie_t
> +dmaengine_prep_submit_slave_single(struct dma_chan *chan,
> +				   dma_async_tx_callback cb, void *cb_param;
> +				   dma_addr_t buf, size_t len,
> +				   enum dma_transfer_direction dir,
> +				   unsigned long flags);
> +{

[Severity: Low]
Are there syntax errors in this fallback implementation?

The parameter list uses a semicolon instead of a comma after cb_param, and
there is a stray semicolon immediately preceding the function body. This will
cause a build failure when CONFIG_DMA_ENGINE is disabled.

> +	return -ENODEV;
> +}
>  #endif
>  
>  static inline int dmaengine_desc_set_reuse(struct dma_async_tx_descriptor *tx)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-dma_prep_submit-v2-0-7a87a5a29525@nxp.com?part=1