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