Re: [PATCH v2 12/23] dmaengine: sdxi: Add descriptor ring management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unkillable task hang and Use-After-Free in ring reservation during device removal or hardware wedge.
- [High] Potential torn reads of the shared 64-bit read index on 32-bit architectures permanently wedge the ring.
- [Low] Macro `sdxi_ring_resv_foreach` evaluates the `resv_` argument multiple times.
--

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

dmaengine: sdxi: Add descriptor ring management

This commit introduces a library for managing SDXI descriptor ring state,
encapsulating free space checks, write index updates, and reservation
iteration. It provides blocking and non-blocking reservation APIs and
mechanisms to iterate over descriptors within a reservation.

> diff --git a/drivers/dma/sdxi/ring.c b/drivers/dma/sdxi/ring.c
> new file mode 100644
> index 0000000000000..91b28c7afbbf9
> --- /dev/null
> +++ b/drivers/dma/sdxi/ring.c
[ ... ]
> +static u64 sdxi_ring_state_load_ridx(struct sdxi_ring_state *rs)
> +{
> +	lockdep_assert_held(&rs->lock);
> +	return le64_to_cpu(READ_ONCE(*rs->read_index_ptr));
> +}

Can this result in a torn read on 32-bit architectures? READ_ONCE() on a
64-bit variable expands to two 32-bit load instructions on 32-bit systems.

Since the SDXI hardware updates this memory location concurrently via DMA,
the CPU might observe a torn value if the read occurs while the index
crosses a 32-bit boundary. This could produce a massively inflated read
index, causing the subsequent check in sdxi_ring_try_reserve() to
permanently return -EIO.

[ ... ]
> +/* Blocking ring reservation. Retries until success or non-transient error. */
> +int sdxi_ring_reserve(struct sdxi_ring_state *rs, size_t nr,
> +		      struct sdxi_ring_resv *resv)
> +{
> +	int ret;
> +
> +	wait_event(rs->wqh,
> +		   (ret = sdxi_ring_try_reserve(rs, nr, resv)) != -EBUSY);
> +
> +	return ret;
> +}

Does this wait_event() need to handle timeouts or signals? If the hardware
wedges or the device is hot-unplugged, the read index will never advance,
leaving tasks permanently hung in TASK_UNINTERRUPTIBLE sleep.

Furthermore, if the driver is unbound and destroys the wait queue while
tasks are still waiting here, could this result in a use-after-free and
subsequent kernel panic when the scheduler interacts with the freed wait
queue?

[ ... ]
> diff --git a/drivers/dma/sdxi/ring.h b/drivers/dma/sdxi/ring.h
> new file mode 100644
> index 0000000000000..d5682687c05cd
> --- /dev/null
> +++ b/drivers/dma/sdxi/ring.h
[ ... ]
> +#define sdxi_ring_resv_foreach(resv_, desc_)			\
> +	for (sdxi_ring_resv_reset(resv_),			\
> +	     desc_ = sdxi_ring_resv_next(resv_);		\
> +	     desc_;						\
> +	     desc_ = sdxi_ring_resv_next(resv_))

This isn't a bug, but this macro evaluates the resv_ argument multiple times.
If a caller passes an expression with side effects, it could cause unintended
behavior.

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