Re: [PATCH] accel/amdxdna: fix race condition in mailbox send path

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling a sleeping function (`read_poll_timeout`) while holding a spinlock in atomic context, leading to a kernel BUG.

Pre-existing issues:
- [High] An off-by-four bounds check error in message size validation allows a message to equal the total ring buffer capacity, leading to an out-of-bounds TOMBSTONE write on the subsequent message.
--

commit 53ca6b8d64c390a01eafa41c7e331865b9a73f9b
Author: Deniz Aydogan <denizaydogan1902@gmail.com>

accel/amdxdna: fix race condition in mailbox send path

This commit introduces a spinlock in mailbox_send_msg() to prevent
concurrent writers from overwriting messages and writing conflicting
tail pointers to the hardware ring buffer.

> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index cc8865f4e79c9..bb0cb34b41a7b 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
[ ... ]
> @@ -203,8 +204,10 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
>  	u32 head, tail;
>  	u32 start_addr;
>  	u32 tmp_tail;
> +	unsigned long flags;
>  	int ret;
>  
> +	spin_lock_irqsave(&mb_chann->lock, flags);
>  	head = mailbox_get_headptr(mb_chann, CHAN_RES_X2I);
>  	tail = mb_chann->x2i_tail;

[Severity: High]
This is a pre-existing issue, but does the bounds check in
xdna_mailbox_send_msg() allow a message to equal the total ring buffer
capacity, leaving no room for the wrap tombstone marker?

If a message is exactly equal to the maximum ring buffer capacity:

drivers/accel/amdxdna/amdxdna_mailbox.c:xdna_mailbox_send_msg() {
    ...
    if (pkg_size > mailbox_get_ringbuf_size(mb_chann, CHAN_RES_X2I)) {
    ...
}

During this first send, the tail pointer is advanced to equal the total
buffer size. On the next send, the wrap-around logic in mailbox_send_msg()
triggers:

drivers/accel/amdxdna/amdxdna_mailbox.c:mailbox_send_msg() {
    ...
    if (tail >= head && tmp_tail > ringbuf_size) {
        write_addr = mb_chann->mb->res.ringbuf_base + start_addr + tail;
        writel(TOMBSTONE, write_addr);
    ...
}

Since the tail is already at the end of the buffer, will this write the
4-byte TOMBSTONE marker out-of-bounds?

[ ... ]
> @@ -225,8 +228,10 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
>  		ret = read_poll_timeout(mailbox_get_headptr, head,
>  					tmp_tail < head || tail >= head,
>  					1, 100, false, mb_chann, CHAN_RES_X2I);

[Severity: High]
Does this code call a sleeping function while holding a spinlock in atomic
context?

mailbox_send_msg() acquires mb_chann->lock via spin_lock_irqsave(),
disabling interrupts. Inside this critical section, it calls
read_poll_timeout() with sleep_us = 1.

read_poll_timeout() internally calls poll_timeout_us(), which uses
usleep_range(), a sleeping function.

Could this lead to a scheduling-while-atomic bug when the driver waits for
ring buffer space?

> -		if (ret)
> +		if (ret) {
> +			spin_unlock_irqrestore(&mb_chann->lock, flags);
>  			return ret;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828221641.10034-1-denizaydogan1902@gmail.com?part=1