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

[Frank Li <Frank.li@nxp.com>]

On Fri, May 22, 2026 at 08:45:38PM +0000, sashiko-bot@kernel.org wrote:
> 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.

Not sure if it is defined beheavor or just idxd implement as it. If it is
defined beheavor, suggest update Document.

From API document:

 "Once a descriptor has been obtained, the callback information can be
  added and the descriptor must then be submitted. Some DMA engine
  drivers may hold a spinlock between a successful preparation and
  submission so it is important that these two operations are closely
  paired."

prep_submit() API should be provide to align document. but not sure
if dmaengine_desc_free() call for reusable descriptors, Or providor should
consider this like idxd_dma_tx_submit()?

Frank
>
> 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