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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Hardware stall and wait loop timeout due to missing TOMBSTONE notification.
- [High] Incorrect wait condition when the ring buffer wraps and head == 0 leads to unconsumed messages being overwritten.
- [High] The tx_timeout parameter is ignored, causing premature failures.
- [High] Out-of-bounds write of TOMBSTONE due to incorrect maximum message size validation.
--

commit 267903cdb6f1ef0ba02cbd6bf4adfbe91f048bd9
Author: Deniz Aydogan <denizaydogan1902@gmail.com>

accel/amdxdna: fix race condition in mailbox send path

mailbox_send_msg() reads and updates x2i_tail without any
synchronization. The DRM scheduler and ioctl paths can call
xdna_mailbox_send_msg() concurrently on the same channel, which
allows two threads to read the same tail value and corrupt the
hardware ring buffer.

Add a mutex to serialize the entire send path. A mutex is used
rather than a spinlock because the existing code calls
read_poll_timeout() with a non-zero sleep, which can reschedule.

> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index cc8865f4e79c9..1d554fb943f2a 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
[ ... ]
> @@ -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]
This is a pre-existing issue, but xdna_mailbox_send_msg() receives a
tx_timeout parameter that is dropped here.

Does hardcoding a 100-microsecond timeout instead of using the caller's
requested tx_timeout cause the send path to fail with -ETIMEDOUT prematurely
under load?

> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&mb_chann->lock);
>  			return ret;
> +		}
>  
>  		if (tail >= head)
>  			goto check_again;

I have a few questions about the wait condition and wrapping logic just before
this wait loop:

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);

		/* tombstone is set. Write from the start of the ringbuf */
		tail = 0;
		tmp_tail = tail + mb_msg->pkg_size;
	}

	if (tail < head && tmp_tail >= head) {
		ret = read_poll_timeout(...);
...

[Severity: Critical]
This is a pre-existing issue, but when the ring buffer wraps and a TOMBSTONE
is written, does the hardware require a doorbell update before entering the
wait loop?

Because mailbox_set_tailptr() is not called after writel(TOMBSTONE,
write_addr), can the hardware stall and the wait loop timeout because it was
never notified to process the TOMBSTONE?

[Severity: High]
This is also a pre-existing issue, but if the ring buffer wraps and tail
resets to 0, does the next condition if (tail < head && tmp_tail >= head)
evaluate incorrectly if head is still 0?

If head is still 0 because the hardware hasn't processed early messages,
tail < head evaluates to false (0 < 0), completely bypassing the wait loop.
Can this bypass cause the driver to immediately overwrite unconsumed messages
at offset 0?

Finally, regarding the size validation:

drivers/accel/amdxdna/amdxdna_mailbox.c:xdna_mailbox_send_msg() {
...
	if (pkg_size > mailbox_get_ringbuf_size(mb_chann, CHAN_RES_X2I)) {
		MB_ERR(mb_chann, "Message size larger than ringbuf size");
		return -EINVAL;
	}
...

[Severity: High]
This is a pre-existing issue, but does the maximum message size validation in
xdna_mailbox_send_msg() fail to reserve 4 bytes for the TOMBSTONE marker?

If a message size exactly matches the physical ring buffer size, it fills the
buffer and tail advances to exactly ringbuf_size.

When the next send wraps and attempts to write the TOMBSTONE:

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);
...

Will this write the 4-byte TOMBSTONE entirely out-of-bounds past the end of
the allocated physical ring buffer, corrupting adjacent device memory?

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