[PATCH] accel/amdxdna: fix double-free on mailbox channel stop

[PATCH] accel/amdxdna: fix double-free on mailbox channel stop

[Deniz Aydogan]

mailbox_release_msg() frees the message with kfree() but does not
remove it from the xarray. The stop function uses two loops to walk
pending entries in cyclic order, but since released entries remain in
the xarray, overlapping ranges cause the same entry to be freed twice.

In particular, when next_msgid is 0 (the initial value after kzalloc),
xa_for_each_start() covers all entries from index 0 onward, and
xa_for_each_range() with max=(u32)(0 - 1) = U32_MAX also covers all
entries. Every pending message gets double-freed.

Use xa_for_each() to iterate all remaining entries exactly once.
At this point the IRQ is already freed and the workqueue is drained,
so traversal order does not matter.

Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_mailbox.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index cc8865f4e..271617347 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -556,9 +556,7 @@ void xdna_mailbox_stop_channel(struct mailbox_channel *mb_chann)
 	drain_workqueue(mb_chann->work_q);
 
 	/* We can clean up and release resources */
-	xa_for_each_start(&mb_chann->chan_xa, msg_id, mb_msg, mb_chann->next_msgid)
-		mailbox_release_msg(mb_chann, mb_msg);
-	xa_for_each_range(&mb_chann->chan_xa, msg_id, mb_msg, 0, mb_chann->next_msgid - 1)
+	xa_for_each(&mb_chann->chan_xa, msg_id, mb_msg)
 		mailbox_release_msg(mb_chann, mb_msg);
 	xa_destroy(&mb_chann->chan_xa);
 
-- 
2.55.0

Re: [PATCH] accel/amdxdna: fix double-free on mailbox channel stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent execution of `xdna_mailbox_send_msg` by the DRM scheduler thread and the ioctl thread leads to hardware ring buffer corruption.
--

commit f00c302142d5de5c06245223cfbc1a367c1aef38
Author: Deniz Aydogan <denizaydogan1902@gmail.com>

accel/amdxdna: fix double-free on mailbox channel stop

This commit fixes a double-free issue when stopping a mailbox channel. The
previous implementation used two xarray iterators that could overlap and
release the same message twice. It replaces them with a single iterator to
ensure each message is freed exactly once.

> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index cc8865f4e79c9..27161734753d0 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -556,9 +556,7 @@ void xdna_mailbox_stop_channel(struct mailbox_channel *mb_chann)
>  	drain_workqueue(mb_chann->work_q);
>  
>  	/* We can clean up and release resources */
> -	xa_for_each_start(&mb_chann->chan_xa, msg_id, mb_msg, mb_chann->next_msgid)
> -		mailbox_release_msg(mb_chann, mb_msg);
> -	xa_for_each_range(&mb_chann->chan_xa, msg_id, mb_msg, 0, mb_chann->next_msgid - 1)
> +	xa_for_each(&mb_chann->chan_xa, msg_id, mb_msg)
>  		mailbox_release_msg(mb_chann, mb_msg);
>  	xa_destroy(&mb_chann->chan_xa);

[Severity: High]
This is a pre-existing issue, but while reviewing the mailbox channel code in
drivers/accel/amdxdna/amdxdna_mailbox.c, I noticed a potential data
corruption issue with concurrent messages.

Can concurrent execution of xdna_mailbox_send_msg() lead to hardware ring
buffer corruption?

The DRM scheduler thread can submit jobs without holding the device lock,
calling:
drivers/accel/amdxdna/aie2_message.c:aie2_execbuf() -> xdna_mailbox_send_msg()

Concurrently, the ioctl thread can configure the same hardware context and
channel while holding the device lock, calling:
drivers/accel/amdxdna/aie2_message.c:aie2_config_cu() -> xdna_mailbox_send_msg()

Inside mailbox_send_msg() in drivers/accel/amdxdna/amdxdna_mailbox.c, the tail
pointer is read locklessly:

    tail = mb_chann->x2i_tail;

And it is updated locklessly after the message is written:

    mailbox_set_tailptr(mb_chann, tail + mb_msg->pkg_size);

Since there is no common lock held between the DRM scheduler thread and the
ioctl thread for the mailbox channel, can they race to read the same tail
pointer, overwrite each other's messages in the ring buffer, and write
conflicting tail pointers to the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828212405.13124-1-denizaydogan1902@gmail.com?part=1

Re: [PATCH] accel/amdxdna: fix double-free on mailbox channel stop

[Lizhi Hou]

On 8/28/26 14:24, Deniz Aydogan wrote:
> mailbox_release_msg() frees the message with kfree() but does not
> remove it from the xarray. The stop function uses two loops to walk
> pending entries in cyclic order, but since released entries remain in
> the xarray, overlapping ranges cause the same entry to be freed twice.
>
> In particular, when next_msgid is 0 (the initial value after kzalloc),
> xa_for_each_start() covers all entries from index 0 onward, and
> xa_for_each_range() with max=(u32)(0 - 1) = U32_MAX also covers all
> entries. Every pending message gets double-freed.
>
> Use xa_for_each() to iterate all remaining entries exactly once.
> At this point the IRQ is already freed and the workqueue is drained,
> so traversal order does not matter.
>
> Fixes: 3ba13f5e7180 ("Merge tag 'devicetree-fixes-for-7.3-1'")
> Signed-off-by: Deniz Aydogan <denizaydogan1902@gmail.com>
> ---
>   drivers/accel/amdxdna/amdxdna_mailbox.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index cc8865f4e..271617347 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -556,9 +556,7 @@ void xdna_mailbox_stop_channel(struct mailbox_channel *mb_chann)
>   	drain_workqueue(mb_chann->work_q);
>   
>   	/* We can clean up and release resources */
> -	xa_for_each_start(&mb_chann->chan_xa, msg_id, mb_msg, mb_chann->next_msgid)
> -		mailbox_release_msg(mb_chann, mb_msg);
> -	xa_for_each_range(&mb_chann->chan_xa, msg_id, mb_msg, 0, mb_chann->next_msgid - 1)
> +	xa_for_each(&mb_chann->chan_xa, msg_id, mb_msg)

The msg need to be released in sequence. And I would suggest to release 
msgid in mailbox_release_msg()

@@ -192,6 +192,7 @@ static void mailbox_release_msg(struct 
mailbox_channel *mb_chann,
                mb_msg->pkg.header.id, mb_msg->pkg.header.opcode);
         if (mb_msg->notify_cb)
                 mb_msg->notify_cb(mb_msg->handle, NULL, 0);
+       mailbox_release_msgid(mb_chann, mb_msg->pkg.header.id);
         kfree(mb_msg);

Thanks,

Lizhi

>   		mailbox_release_msg(mb_chann, mb_msg);
>   	xa_destroy(&mb_chann->chan_xa);
>