Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Stale fw_ctx_id is left intact on aie2_create_context error paths, leaking context state. - [High] Missing mmput() on retry and success paths in aie2_populate_range() causes mm_struct reference leak. - [High] Concurrent access to mailbox ringbuffer from ioctl and DRM scheduler thread. -- --- Patch [1]: [PATCH] accel/amdxdna: Clear mailbox channel after create failure --- commit c7aab37bf2d56874aaf94d232e6a55a7ab8e19c2 Author: Ruoyu Wang <[email protected]> accel/amdxdna: Clear mailbox channel after create failure This commit clears the mailbox channel pointer in the error path of aie2_create_context() after freeing it. This prevents later context cleanup routines from operating on stale memory if the creation fails. > diff --git a/drivers/accel/amdxdna/aie2_message.c > b/drivers/accel/amdxdna/aie2_message.c > index c4b364801cc04..a22d025794598 100644 > --- a/drivers/accel/amdxdna/aie2_message.c > +++ b/drivers/accel/amdxdna/aie2_message.c > @@ -277,6 +277,7 @@ int aie2_create_context(struct amdxdna_dev_hdl *ndev, > struct amdxdna_hwctx *hwct > > free_channel: > xdna_mailbox_free_channel(hwctx->priv->mbox_chann); > + hwctx->priv->mbox_chann = NULL; > del_ctx_req: > aie2_destroy_context_req(ndev, hwctx->fw_ctx_id); [Severity: High] This isn't a bug introduced by this patch, but should we also invalidate hwctx->fw_ctx_id here? If a job timeout triggers a context restart via aie2_hwctx_restart() and aie2_create_context() fails during mailbox start, the allocated context ID is freed in firmware but left in hwctx->fw_ctx_id. Could a user then query DRM_AMDXDNA_HW_CONTEXT_ALL and leak the health report (PC, exception info) of another process if the firmware reassigned this ID? > return ret; > } [Severity: High] This is a pre-existing issue outside the immediate diff, but I noticed a potential resource leak in drivers/accel/amdxdna/aie2_ctx.c. When populating a range, mmget_not_zero(mm) takes a reference. If the retry path is taken: drivers/accel/amdxdna/aie2_ctx.c:aie2_populate_range() { ... if (ret == -EBUSY) { amdxdna_umap_put(mapp); goto again; } ... } Or if the success path is taken: drivers/accel/amdxdna/aie2_ctx.c:aie2_populate_range() { ... mapp->invalid = false; up_write(&xdna->notifier_lock); amdxdna_umap_put(mapp); goto again; ... } Does jumping to again bypass mmput(mm) and leak the mm_struct reference? [Severity: High] This is another pre-existing issue, but I have a question about concurrent mailbox access. If a user submits a job starting the DRM scheduler thread (aie2_sched_job_run calling aie2_execbuf), and concurrently calls the DRM_AMDXDNA_CONFIG_HWCTX ioctl: drivers/accel/amdxdna/aie2_ctx.c:aie2_hwctx_cu_config() { ... ret = aie2_config_cu(hwctx, aie2_config_cu_resp_handler); ... } Could both threads invoke xdna_mailbox_send_msg() on the same mbox_chann simultaneously? drivers/accel/amdxdna/amdxdna_mailbox.c:mailbox_send_msg() { ... head = mailbox_get_headptr(mb_chann, CHAN_RES_X2I); tail = mb_chann->x2i_tail; ... } Since mailbox_send_msg() performs an unlocked read-modify-write on the ringbuffer tail pointer and memory, can this race corrupt the hardware mailbox ring buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
