Re: [PATCH v7 2/9] dmaengine: Add safe API to combine configuration and preparation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calling `device_config` or `device_prep_slave_sg` while holding a spinlock with interrupts disabled (`spin_lock_irqsave`) will cause lockups and `WARN_ON_ONCE` in drivers that use bottom-half locks.
- [Medium] `spinlock_flags` may be used uninitialized and the lock may become unbalanced due to re-evaluation of `chan->device->device_prep_config_sg` across an opaque function call.

Pre-existing issues:
- [Critical] The function `dmaengine_prep_config_single_safe` passes a stack-allocated `scatterlist` to the DMA engine driver, which causes a stack use-after-free if the driver defers processing the scatterlist.
--

commit d2363736f5c7b002b0f30c81cef1791d2f12abd2
Author: Frank Li <Frank.Li@nxp.com>
Subject: dmaengine: Add safe API to combine configuration and preparation

Introduce new safe APIs to atomically combine slave configuration and transfer
preparation. The new functions fall back to calling slave_config and
prep_slave_sg under a channel spinlock if the device doesn't support the
atomic callback.

> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index defa377d2ef54..6fe46c0c94527 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h

[ ... ]

> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_sg_safe(struct dma_chan *chan, struct scatterlist *sgl,
> +			      unsigned int sg_len,
> +			      enum dma_transfer_direction dir,
> +			      unsigned long flags,
> +			      struct dma_slave_config *config)
> +{
> +	struct dma_async_tx_descriptor *tx;
> +	unsigned long spinlock_flags;
> +
> +	if (!chan || !chan->device)
> +		return NULL;
> +
> +	if (!chan->device->device_prep_config_sg)
> +		spin_lock_irqsave(&chan->lock, spinlock_flags);
> +
> +	tx = dmaengine_prep_config_sg(chan, sgl, sg_len, dir, flags, config);

[Severity: High]
Will this cause lockups or warnings in drivers that use bottom-half locks?

Since spin_lock_irqsave() disables hard interrupts, calling into
dmaengine_prep_config_sg() invokes the underlying DMA driver's
device_prep_slave_sg() or device_config() callbacks. Some drivers use
bottom-half locks in these implementations:

dmaengine_prep_config_sg_safe()
  spin_lock_irqsave()
    dmaengine_prep_config_sg()
      chan->device->device_prep_slave_sg()
        td_prep_slave_sg()
          td_desc_get()
            spin_lock_bh()

When the driver eventually calls spin_unlock_bh(), it invokes
local_bh_enable(), which triggers a WARN_ON_ONCE(irqs_disabled()) because
hard interrupts were disabled by the outer spin_lock_irqsave().

> +
> +	if (!chan->device->device_prep_config_sg)
> +		spin_unlock_irqrestore(&chan->lock, spinlock_flags);

[Severity: Medium]
Could this lead to an uninitialized variable usage or an unbalanced lock?

The condition checking chan->device->device_prep_config_sg is evaluated
twice, before and after calling dmaengine_prep_config_sg(). Because that
function invokes opaque function pointers passing chan, the compiler must
assume memory reachable through chan might be modified.

This forces the compiler to consider a control flow path where the condition
is initially false (skipping spin_lock_irqsave and leaving spinlock_flags
uninitialized) but evaluates to true after the opaque call, leading to
spin_unlock_irqrestore() with uninitialized flags and an unbalanced lock.

Even if this doesn't happen dynamically, it can emit a -Wmaybe-uninitialized
warning that breaks builds with -Werror. Caching the condition in a local
boolean variable would prevent this.

> +
> +	return tx;
> +}
> +
> +/**
> + * dmaengine_prep_config_single_safe - prepare a single-buffer DMA transfer
> + *                                     with atomic slave configuration update

[ ... ]

> +static inline struct dma_async_tx_descriptor *
> +dmaengine_prep_config_single_safe(struct dma_chan *chan, dma_addr_t buf,
> +				  size_t len, enum dma_transfer_direction dir,
> +				  unsigned long flags,
> +				  struct dma_slave_config *config)
> +{
> +	struct scatterlist sg;
> +
> +	sg_init_table(&sg, 1);
> +	sg_dma_address(&sg) = buf;
> +	sg_dma_len(&sg) = len;
> +
> +	return dmaengine_prep_config_sg_safe(chan, &sg, 1, dir, flags, config);

[Severity: Critical]
This is a pre-existing issue, but does passing a stack-allocated scatterlist
to the driver cause a stack use-after-free if the driver defers processing?

The scatterlist sg is allocated on the stack and passed down to the
underlying DMA engine's device_prep_slave_sg() callback. While some drivers
consume the scatterlist synchronously, others save the pointer to process
it asynchronously later:

uniphier_mdmac_prep_slave_sg()
  md->sgl = sgl;

uniphier_mdmac_handle()
  dereferences md->sgl

Because dmaengine_prep_config_single_safe() returns immediately, the stack
frame is destroyed. When the DMA driver later accesses the saved pointer
from an IRQ context, it accesses freed stack memory.

This pattern already exists in dmaengine_prep_slave_single(), but
replicating it here propagates the regression to the new API.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-dma_prep_config-v7-0-1f73f4899883@nxp.com?part=2