Re: [PATCH v2 11/23] dmaengine: sdxi: Add client context alloc and release APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing hardware synchronization and premature context teardown causes DMA Use-After-Free.
- [High] Forcefully freeing client contexts in `sdxi_unregister()` breaks ownership boundaries, causing Double-Free/UAF.
- [High] Broken XArray ID reservation allows concurrent contexts to receive the same ID, causing L1 table corruption.
- [High] Lockless `xa_for_each()` iteration over `client_cxts` races with synchronous deletion, leading to UAF.
- [Medium] Premature publication of context in `sdxi_cxt_new()` exposes partially-initialized objects.
--

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

dmaengine: sdxi: Add client context alloc and release APIs

This commit exposes sdxi_cxt_new() and sdxi_cxt_exit() for creating and
releasing SDXI contexts. It tracks client contexts in a device-wide
allocating xarray and introduces a local sdxi_cxt_id class for early ID
allocation.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> index c0b55c945cc4e..c0b294836edeb 100644
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c
[ ... ]
> @@ -258,6 +282,17 @@ static int sdxi_publish_cxt(const struct sdxi_cxt *cxt)
[ ... ]
> +/* Invalidate a context. */
> +static void sdxi_rescind_cxt(struct sdxi_cxt *cxt)
> +{
> +	u8 l1_idx = ID_TO_L1_INDEX(cxt->id);
> +	struct sdxi_cxt_L1_ent *ent = &cxt->sdxi->L1_table->entry[l1_idx];
> +
> +	invalidate_L1_entry(ent);
> +	invalidate_cxtl_ctl(cxt->cxt_ctl);
> +	/* todo: need to send DSC_CXT_UPD to admin */

Since the hardware is not notified to halt processing here, and the context's
DMA memory is freed in sdxi_free_cxt() right after this returns, will the
active hardware continue writing to the freed physical memory pages?

> +}
[ ... ]
> @@ -288,3 +323,90 @@ int sdxi_admin_cxt_init(struct sdxi_dev *sdxi)
[ ... ]
> +static struct sdxi_cxt_id sdxi_cxt_id_ctor(struct sdxi_dev *sdxi)
> +{
> +	struct xa_limit limit = XA_LIMIT(1, sdxi->max_cxtid);
> +	u32 index;
> +
> +	return (struct sdxi_cxt_id) {
> +		.sdxi = sdxi,
> +		.index = xa_alloc(&sdxi->client_cxts, &index, NULL,
> +				  limit, GFP_KERNEL) ? 0 : (u16)index,

Does passing NULL as the entry to xa_alloc() fail to reserve the ID?

Under XArray semantics, storing a NULL entry marks the slot as free, meaning
concurrent calls to sdxi_cxt_new() might be allocated the exact same ID and
overwrite each other.

If this is fixed by storing a dummy entry, will the WARN_ON() in
sdxi_cxt_id_assign() below incorrectly trigger on every assignment, since
xa_store() returns the previous entry instead of NULL?

> +	};
> +}
[ ... ]
> +/*
> + * Transfer ownership of the id to the context object, recording the
> + * context pointer in the device's client_cxt xarray. sdxi_cxt_free()
> + * is responsible for releasing the id from now on.
> + */
> +static void sdxi_cxt_id_assign(struct sdxi_cxt *cxt, struct sdxi_cxt_id *cxt_id)
> +{
> +	/* We reserved the space in the constructor so this should not fail. */
> +	WARN_ON(xa_store(&cxt_id->sdxi->client_cxts,
> +			 cxt_id->index, cxt, GFP_KERNEL));
> +	cxt->id = cxt_id->index;
> +	cxt_id->index = 0;
> +}
> +
> +/*
> + * Allocate a context for in-kernel use. Starting the context is the
> + * caller's responsibility.
> + */
> +struct sdxi_cxt *sdxi_cxt_new(struct sdxi_dev *sdxi)
> +{
> +	/*
> +	 * Ensure an ID is available before allocating memory for the
> +	 * context and its control structures.
> +	 */
> +	CLASS(sdxi_cxt_id, id)(sdxi);
> +	if (!sdxi_cxt_id_valid(&id))
> +		return NULL;
> +
> +	struct sdxi_cxt *cxt __free(sdxi_cxt) = sdxi_alloc_cxt(sdxi);
> +	if (!cxt)
> +		return NULL;
> +
> +	sdxi_cxt_id_assign(cxt, &id);

Is it safe to publish the context in the xarray here before it is fully
initialized?

If a lockless reader traverses the xarray, could it fetch this partially
initialized context?

Also, if sdxi_publish_cxt() below fails, the cleanup handler will free the
context, potentially exposing a freed pointer in the xarray.

> +
> +	cxt->db = sdxi->dbs + cxt->id * sdxi->db_stride;
> +
> +	if (sdxi_publish_cxt(cxt))
> +		return NULL;
> +
> +	return_ptr(cxt);
> +}
[ ... ]
> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 8e621875b10bd..cc289b271ae22 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
> @@ -338,6 +340,12 @@ int sdxi_register(struct device *dev, const struct sdxi_bus_ops *ops)
>  void sdxi_unregister(struct device *dev)
>  {
>  	struct sdxi_dev *sdxi = dev_get_drvdata(dev);
> +	struct sdxi_cxt *cxt;
> +	unsigned long index;
> +
> +	xa_for_each(&sdxi->client_cxts, index, cxt)
> +		sdxi_cxt_exit(cxt);

Could this lockless xa_for_each() iteration race with a concurrent deletion?

Since xa_for_each() drops the RCU read lock internally for the loop body and
sdxi_free_cxt() destroys the context synchronously using kfree(), it seems a
concurrent call to sdxi_cxt_exit() could free the context while this loop is
processing it.

Additionally, since sdxi_cxt_new() explicitly transfers ownership to the
caller, does forcefully freeing the client contexts here violate that
ownership contract?

When the DMA provider eventually cleans up its channels after
sdxi_unregister() completes, it might call sdxi_cxt_exit() on the
already-freed context pointers.

> +	xa_destroy(&sdxi->client_cxts);
>  
>  	sdxi_dev_stop(sdxi);

By tearing down the contexts before sdxi_dev_stop() halts the hardware,
could this allow the active hardware to continue writing to the freed context
DMA memory?

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