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 <[email protected]> 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/[email protected]?part=1
