[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

[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

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