Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] amdxdna: ABA vulnerability in msg_id accepts stale firmware responses - [High] amdxdna: management channel bottleneck causes concurrent -ENOBUFS failures - [Medium] amdxdna: xdna_mailbox_wait_ack() misses completion due to TOCTOU on tail --
commit ea210dde67519c35a295be58a57ebd45f0948e98 Author: Max Zhen <[email protected]> accel/amdxdna: eliminate GFP_KERNEL allocation from mailbox send path This patch replaces the per-message dynamic allocations with a pre-allocated pool of N slots. The message ID is updated to encode the slot index directly for O(1) lookup, and the management channel size is bound statically based on the maximum number of asynchronous events plus one. > --- a/drivers/accel/amdxdna/aie2_pci.c > +++ b/drivers/accel/amdxdna/aie2_pci.c > @@ -385,11 +386,16 @@ static int aie2_hw_start(struct amdxdna_dev *xdna) > } > > xdna_mailbox_intr_reg = ndev->aie.mgmt_i2x.mb_head_ptr_reg + 4; > + /* > + * At any given time, at most AMDXDNA_MAX_ASYNC_EVENT_BUFS async event > + * messages plus 1 other management command can be unresponded. > + */ > ret = xdna_mailbox_start_channel(ndev->aie.mgmt_chann, > &ndev->aie.mgmt_x2i, > &ndev->aie.mgmt_i2x, > xdna_mailbox_intr_reg, > - mgmt_mb_irq); > + mgmt_mb_irq, > + AMDXDNA_MAX_ASYNC_EVENT_BUFS + 1); [Severity: High] Could this hardcoded capacity lead to spurious -ENOBUFS failures for userspace? Since 4 of the 5 slots are permanently consumed by asynchronous event buffers waiting for firmware errors, only 1 slot is left for other management commands. If two threads execute concurrent IOCTLs that issue management commands, won't the second thread immediately fail with -ENOBUFS? > --- a/drivers/accel/amdxdna/amdxdna_mailbox.c > +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c [ ... ] > @@ -248,8 +297,9 @@ mailbox_get_resp(struct mailbox_channel *mb_chann, struct > xdna_msg_header *heade > void __iomem *data) > { [ ... ] > - msg_id &= ~MAGIC_VAL_MASK; > - mb_msg = xa_erase_irq(&mb_chann->chan_xa, msg_id); > - if (!mb_msg) { > - MB_ERR(mb_chann, "Cannot find msg 0x%x", msg_id); > + slot = msg_id & ~MAGIC_VAL_MASK; > + if (unlikely(slot >= mb_chann->msg_buf_num)) { > + MB_ERR(mb_chann, "Invalid msg_id 0x%x", msg_id); > return -EINVAL; > } > + mb_msg = mailbox_msg_ptr(mb_chann, slot); > > MB_DBG(mb_chann, "opcode 0x%x size %d id 0x%x", > header->opcode, header->total_size, header->id); > - if (mb_msg->notify_cb) { > - ret = mb_msg->notify_cb(mb_msg->handle, data, > header->total_size); > - if (unlikely(ret)) > - MB_ERR(mb_chann, "Message callback ret %d", ret); > + > + if (unlikely(!mb_msg->busy)) { > + MB_WARN_ONCE(mb_chann, "Unexpected response for idle slot > 0x%x", msg_id); > + return 0; > } [Severity: High] Does replacing xa_alloc_cyclic_irq with a static ID using slot | MAGIC_VAL introduce an ABA vulnerability here? If a stale or duplicate firmware response arrives after a slot has been reused for a completely new message, mb_msg->busy will be true again. Since the msg_id no longer contains a sequence number to differentiate between the old and new message, won't this allow a stale firmware response to bypass the check and invoke the wrong callback with invalid data? [ ... ] > @@ -398,8 +451,33 @@ static void mailbox_rx_worker(struct work_struct > *rx_work) > goto again; > } > > +static int xdna_mailbox_wait_ack(struct mailbox_channel *mb_chann, u64 > tx_timeout_ms) > +{ > + u32 tail = mb_chann->x2i_tail; > + u32 head; > + int ret; > + > + /* > + * Poll until firmware advances the head pointer past our message, > + * confirming it has consumed (acknowledged) the send. > + */ > + ret = read_poll_timeout(mailbox_get_headptr, head, > + head == tail, 1000, tx_timeout_ms * 1000, > + false, mb_chann, CHAN_RES_X2I); [Severity: Medium] Can this polling loop stall for the full timeout duration due to a race condition on the tail pointer? The tail pointer is read outside the lock and might reflect a subsequent message's tail if another thread concurrently sends a message. When the firmware processes both messages, the head pointer will advance past the expected tail. Since the completion condition uses strict equality (head == tail), won't it permanently fail and cause the caller to stall until timeout if the head overtakes the captured tail? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
