Re: [PATCH v2 19/23] dmaengine: sdxi: Provide context start and stop APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] DMA Use-After-Free due to missing hardware abort on polling timeout.
- [High] Silent teardown failure in `sdxi_stop_cxt()` leads to active context Use-After-Free.
- [Medium] Race condition in `sdxi_completion_poll()` causes false timeouts due to thread preemption.
--

commit 5b80fef0d376d25e0f86622539d35c12aa0571d5
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Provide context start and stop APIs

This commit introduces high-level context start and stop APIs for SDXI clients.
It implements sdxi_start_cxt() and sdxi_stop_cxt() which operate on struct
sdxi_cxt objects to encapsulate administrative descriptor submission and wait
for completion.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> index 56e21aa088577..28eb4ccd6c1d4 100644
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c

[ ... ]

> @@ -335,6 +337,81 @@ int sdxi_admin_cxt_init(struct sdxi_dev *sdxi)
>  	return devm_add_action_or_reset(sdxi->dev, free_admin_cxt, sdxi);
>  }
>  
> +int sdxi_start_cxt(struct sdxi_cxt *cxt)
> +{

[ ... ]

> +	struct sdxi_completion *sc __free(sdxi_completion) =
> +		sdxi_completion_alloc(cxt->sdxi);

[ ... ]

> +	return sdxi_completion_poll(sc);
> +}

If sdxi_completion_poll() times out, it returns -ETIMEDOUT and the
cleanup runs, freeing the DMA-mapped completion memory.

Because the hardware operation is not aborted on timeout, is it possible for
the device to eventually complete the delayed operation and write to the
freed DMA address?

Also, does sdxi_completion_poll() have a race condition that could cause
false timeouts?

Looking at the implementation of sdxi_completion_poll():

	while (le64_to_cpu(READ_ONCE(sc->cst_blk->signal)) != 0) {
		if (time_after(jiffies, deadline))
			return -ETIMEDOUT;
		cpu_relax();
	}

If the polling thread is preempted immediately after the READ_ONCE evaluation,
time_after() might evaluate to true upon resumption. Even if the hardware
successfully updates the signal during the preemption window, wouldn't this
return -ETIMEDOUT without a final check of the signal?

> +void sdxi_stop_cxt(struct sdxi_cxt *cxt)
> +{
> +	struct sdxi_cxt *adm = to_admin_cxt(cxt);
> +	struct sdxi_desc *stop, *sync;
> +	struct sdxi_ring_resv resv;
> +	int err;
> +
> +	might_sleep();
> +
> +	struct sdxi_completion *sc __free(sdxi_completion) =
> +		sdxi_completion_alloc(cxt->sdxi);
> +
> +	if (!sc)
> +		return;

Since sdxi_stop_cxt() returns void, memory allocation failures here are
silently ignored.

If this happens, the caller won't know the stop command failed and might
proceed to tear down the context rings and buffers while the hardware is
still active.

Can this regression lead to the hardware accessing freed memory when it
continues fetching from the torn-down rings?

> +
> +	/* This is not how to stop the admin context. */
> +	if (WARN_ON(adm == cxt))
> +		return;
> +
> +	err = sdxi_ring_reserve(adm->ring_state, 2, &resv);
> +	if (WARN_ON_ONCE(err))
> +		return;

Similarly, if sdxi_ring_reserve() fails, the function returns without
stopping the hardware context, which may also cause the caller to free
resources that the active hardware might still access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=19