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

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

[Deniz Aydogan]

xdna_mailbox_send_msg() reads and writes x2i_tail without holding any
lock. This is problematic because the DRM scheduler thread can submit
jobs via aie2_execbuf() while the ioctl thread concurrently configures
the same hardware context via aie2_config_cu().

Both paths eventually call xdna_mailbox_send_msg() which does:

    tail = mb_chann->x2i_tail;
    ...
    mailbox_set_tailptr(mb_chann, tail + mb_msg->pkg_size);

Without synchronization, concurrent writers can read the same tail value,
overwrite each other's messages in the ring buffer, and write conflicting
tail pointers to hardware.

Fix this by protecting the tail pointer manipulation with a spinlock
that is held for the entire duration of mailbox_send_msg(). The lock
is initialized in xdna_mailbox_alloc_channel() and acquired/released
around the critical section in mailbox_send_msg().

Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_mailbox.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index 271617347..337f89113 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -60,6 +60,7 @@ struct mailbox_channel {
 	struct xarray			chan_xa;
 	u32				next_msgid;
 	u32				x2i_tail;
+	spinlock_t			lock;
 
 	/* Received msg related fields */
 	struct workqueue_struct		*work_q;
@@ -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;
 	ringbuf_size = mailbox_get_ringbuf_size(mb_chann, CHAN_RES_X2I) - sizeof(u32);
@@ -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);
-		if (ret)
+		if (ret) {
+			spin_unlock_irqrestore(&mb_chann->lock, flags);
 			return ret;
+		}
 
 		if (tail >= head)
 			goto check_again;
@@ -240,6 +245,7 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
 			    mb_msg->pkg.header.opcode,
 			    mb_msg->pkg.header.id);
 
+	spin_unlock_irqrestore(&mb_chann->lock, flags);
 	return 0;
 }
 
@@ -487,6 +493,7 @@ struct mailbox_channel *xdna_mailbox_alloc_channel(struct mailbox *mb)
 		goto free_chann;
 	}
 	mb_chann->mb = mb;
+	spin_lock_init(&mb_chann->lock);
 
 	return mb_chann;
 
-- 
2.55.0

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

[sashiko-bot]

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

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

[Deniz Aydogan]

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.

Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_mailbox.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index 271617347..8338f03bd 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -8,6 +8,7 @@
 #include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/xarray.h>
 
@@ -60,6 +61,7 @@ struct mailbox_channel {
 	struct xarray			chan_xa;
 	u32				next_msgid;
 	u32				x2i_tail;
+	struct mutex			lock;
 
 	/* Received msg related fields */
 	struct workqueue_struct		*work_q;
@@ -205,6 +207,7 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
 	u32 tmp_tail;
 	int ret;
 
+	mutex_lock(&mb_chann->lock);
 	head = mailbox_get_headptr(mb_chann, CHAN_RES_X2I);
 	tail = mb_chann->x2i_tail;
 	ringbuf_size = mailbox_get_ringbuf_size(mb_chann, CHAN_RES_X2I) - sizeof(u32);
@@ -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);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&mb_chann->lock);
 			return ret;
+		}
 
 		if (tail >= head)
 			goto check_again;
@@ -240,6 +245,7 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
 			    mb_msg->pkg.header.opcode,
 			    mb_msg->pkg.header.id);
 
+	mutex_unlock(&mb_chann->lock);
 	return 0;
 }
 
@@ -487,6 +493,7 @@ struct mailbox_channel *xdna_mailbox_alloc_channel(struct mailbox *mb)
 		goto free_chann;
 	}
 	mb_chann->mb = mb;
+	mutex_init(&mb_chann->lock);
 
 	return mb_chann;
 
-- 
2.55.0

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

[sashiko-bot]

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

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

[Lizhi Hou]

On 8/29/26 00:53, Deniz Aydogan wrote:
> 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.

Is this trying to address the sashiko comment?

The config cu ioctl should not concurrently running with commands 
(submitting by DRM scheduler). If this happens, it indicates a bug in 
user space. And the commands submitted to the same hwctx (mailbox 
channel) could fail. The incorrect application may mess up itself and it 
would not impact other processes.

Thanks,

Lizhi

>
> 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.
>
> Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
> Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
> ---
>   drivers/accel/amdxdna/amdxdna_mailbox.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index 271617347..8338f03bd 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -8,6 +8,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/interrupt.h>
>   #include <linux/iopoll.h>
> +#include <linux/mutex.h>
>   #include <linux/slab.h>
>   #include <linux/xarray.h>
>   
> @@ -60,6 +61,7 @@ struct mailbox_channel {
>   	struct xarray			chan_xa;
>   	u32				next_msgid;
>   	u32				x2i_tail;
> +	struct mutex			lock;
>   
>   	/* Received msg related fields */
>   	struct workqueue_struct		*work_q;
> @@ -205,6 +207,7 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
>   	u32 tmp_tail;
>   	int ret;
>   
> +	mutex_lock(&mb_chann->lock);
>   	head = mailbox_get_headptr(mb_chann, CHAN_RES_X2I);
>   	tail = mb_chann->x2i_tail;
>   	ringbuf_size = mailbox_get_ringbuf_size(mb_chann, CHAN_RES_X2I) - sizeof(u32);
> @@ -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);
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&mb_chann->lock);
>   			return ret;
> +		}
>   
>   		if (tail >= head)
>   			goto check_again;
> @@ -240,6 +245,7 @@ mailbox_send_msg(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
>   			    mb_msg->pkg.header.opcode,
>   			    mb_msg->pkg.header.id);
>   
> +	mutex_unlock(&mb_chann->lock);
>   	return 0;
>   }
>   
> @@ -487,6 +493,7 @@ struct mailbox_channel *xdna_mailbox_alloc_channel(struct mailbox *mb)
>   		goto free_chann;
>   	}
>   	mb_chann->mb = mb;
> +	mutex_init(&mb_chann->lock);
>   
>   	return mb_chann;
>