Re: [PATCH v2 14/23] dmaengine: sdxi: Attach descriptor ring state to contexts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing Teardown Symmetry / Waitqueue Use-After-Free in `sdxi_free_cxt()`
- [High] Premature Publication of Context in `sdxi_cxt_new()`
- [High] RCU Writer-Side Lifetime Violation on Teardown
--

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

dmaengine: sdxi: Attach descriptor ring state to contexts

This commit attaches an instance of struct sdxi_ring_state to each
context upon allocation. Each ring state shares the same lifetime as
its context and is freed upon context release.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c
[ ... ]
> @@ -63,6 +64,7 @@ static void sdxi_free_cxt(struct sdxi_cxt *cxt)
>  		dma_free_coherent(sdxi->dev, sq->ring_size,
>  				  sq->desc_ring, sq->ring_dma);
>  	kfree(cxt->sq);
> +	kfree(cxt->ring_state);
>  	kfree(cxt);
>  }

Earlier in sdxi_free_cxt(), the context is removed from the client_cxts
xarray via xa_erase(). Since the xarray is RCU-protected, concurrent readers
could still be accessing the context under the RCU read lock.

Does this kfree(cxt->ring_state) need to be deferred via kfree_rcu() to
prevent a regression where readers might access freed memory if they obtained
the pointer just before erasure?

Additionally, if tasks are blocking on the wait queue in the ring state
(for example, during ring reservation), will they be woken up before this
memory is freed?

If a context is torn down while tasks are waiting, it seems they could
hang indefinitely in uninterruptible sleep, or encounter memory corruption
when the wait queue memory is repurposed.

Should there be a teardown sequence that marks the ring as shutting down
and calls wake_up_all(&cxt->ring_state->wqh) to unblock tasks before
freeing the ring state?

[ ... ]
> @@ -396,6 +406,10 @@ struct sdxi_cxt *sdxi_cxt_new(struct sdxi_dev *sdxi)
>  
>  	cxt->db = sdxi->dbs + cxt->id * sdxi->db_stride;
>  
> +	sq = cxt->sq;
> +	sdxi_ring_state_init(cxt->ring_state, &sq->cxt_sts->read_index,
> +			     sq->write_index, sq->ring_entries, sq->desc_ring);
> +
>  	if (sdxi_publish_cxt(cxt))
>  		return NULL;

It appears sdxi_cxt_id_assign() is called earlier in this function, which
stores the context into the client_cxts xarray.

Could this lead to a race condition? Concurrent lockless lookups might
discover the context through the xarray and attempt to interact with
cxt->ring_state before sdxi_ring_state_init() has initialized its
spinlock and wait queue.

Should sdxi_ring_state_init() be called prior to sdxi_cxt_id_assign() to
ensure the object is fully constructed before it becomes externally visible?

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