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 <denizaydogan1...@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;