Re: [PATCH v2 1/2] accel/amdxdna: clear the mailbox channel pointer when starting it fails

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] aie2_create_context() fails to reset hwctx->fw_ctx_id to an invalid state on error paths, causing telemetry corruption.

Pre-existing issues:
- [High] Premature DRM scheduler restart in aie2_hwctx_stop() leads to data races, uninitialized memory access, and Use-After-Free of mbox_chann.
- [High] Jobs are silently marked as successfully completed when the hardware context is dead, rather than returning an error.
- [High] Mailbox channel memory and firmware context are permanently leaked when set_dpm_level() fails during hardware context allocation.
- [Medium] aie2_fill_hwctx_map() breaks telemetry queries for all clients if any hardware context is stopped or unconfigured.
- [Low] The mapped IRQ number is overwritten by the return value of xdna_mailbox_start_channel() before it is logged.
--

commit a549f036c2506b6146292b236477aa4ed260a99e
Author: Eva Crystal <0xiviel@gmail.com>

accel/amdxdna: clear the mailbox channel pointer when starting it fails

aie2_create_context() allocates a mailbox channel, starts it, and on a
failed start frees it again, leaving a stale pointer in hwctx->priv->mbox_chann.
This patch sets hwctx->priv->mbox_chann to NULL after freeing it to avoid
use-after-free and double-free issues during context teardown.

>       - a second command timeout reaches aie2_hwctx_stop() ->
>         aie2_destroy_context(),

[Severity: Medium]
This is a pre-existing issue, but when a context is stopped via
aie2_destroy_context(), hwctx->fw_ctx_id is set to -1
(AMDXDNA_INVALID_CTX_HANDLE).

If aie2_get_telemetry() is subsequently called, aie2_fill_hwctx_map() checks:

drivers/accel/amdxdna/aie2_pci.c:aie2_fill_hwctx_map() {
    if (hwctx->fw_ctx_id >= xdna->dev_handle->priv->hwctx_limit) {
        XDNA_ERR(xdna, "Invalid fw ctx id %d/%d ", hwctx->fw_ctx_id,
        ...
        return -EINVAL;
    }
    ...
}

Since fw_ctx_id is a u32, -1 evaluates to 0xFFFFFFFF, which is strictly greater
than hwctx_limit. Does this error bubble up and completely abort the telemetry
query for all clients just because one context is in a valid stopped state?

>       - the next job submitted reaches xdna_mailbox_send_msg() on the freed
>         channel, via the !mbox_chann guard in aie2_sched_job_run(),

[Severity: High]
This is a pre-existing issue, but does aie2_sched_job_run() silently drop jobs?

drivers/accel/amdxdna/aie2_ctx.c:aie2_sched_job_run() {
    ...
    if (!hwctx->priv->mbox_chann)
        return NULL;
    ...
}

Under DRM scheduler semantics, returning a NULL fence indicates the job
completed synchronously and successfully. Does this cause the scheduler to
signal the finished fence without error, leading user-space applications to
process uninitialized output buffers as valid data instead of receiving an
error pointer like ERR_PTR(-ENODEV)?

> The create path itself is not affected: when aie2_xrs_load() fails,
> xrs_allocate_resource() removes the solver node without calling
> ->unload, and aie2_hwctx_init() frees hwctx->priv, so the stale pointer
> never outlives the structure holding it.

[Severity: High]
This is a pre-existing issue, but does xrs_allocate_resource() leak mailbox
channel memory and the firmware context if set_dpm_level() fails?

drivers/accel/amdxdna/aie2_solver.c:xrs_allocate_resource() {
    ...
    ret = xrs->cfg.actions->load(cb_arg, &load_act);
    if (ret)
        goto free_node;

    ret = set_dpm_level(xrs, req, &dpm_level);
    if (ret)
        goto free_node;
    ...
free_node:
    remove_solver_node(&xrs->rgp, snode);
    return ret;
}

Because the free_node error path skips calling xrs->cfg.actions->unload(),
are resources successfully allocated by load() permanently leaked when
set_dpm_level() fails?

> Starting a channel can fail today without this patch: the ring buffer
> sizes firmware reports are rejected unless both are powers of two, and
> request_irq() can fail.

[Severity: Low]
This is a pre-existing issue, but does aie2_create_context() overwrite the
mapped IRQ number before logging it?

drivers/accel/amdxdna/aie2_message.c:aie2_create_context() {
    ...
    ret = pci_irq_vector(to_pci_dev(xdna->ddev.dev), resp.msix_id);
    ...
    ret = xdna_mailbox_start_channel(hwctx->priv->mbox_chann, &x2i, &i2x,
                     intr_reg, ret);
    ...
    XDNA_DBG(xdna, "Mailbox channel irq: %d, msix_id: %d", ret, resp.msix_id);
    ...
}

Since xdna_mailbox_start_channel() returns 0 on success into ret, will this
incorrectly always log 0 for the IRQ number instead of the actual assigned IRQ?

diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
index b4c49259a1a23..f658760c3d480 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);

[Severity: High]
This is a pre-existing issue, but does aie2_hwctx_stop() unpark the DRM
scheduler thread before aie2_hwctx_restart() completely recreates the context?

drivers/accel/amdxdna/aie2_ctx.c:aie2_hwctx_stop() {
    drm_sched_stop(&hwctx->priv->sched, bad_job);
    aie2_destroy_context(xdna->dev_handle, hwctx);
    drm_sched_start(&hwctx->priv->sched, 0);
}

If the scheduler restarts prematurely, it can execute queued jobs and call
aie2_sched_job_run(), which locklessly accesses hwctx->priv->mbox_chann.

If this runs concurrently with the error path here in aie2_create_context(),
could aie2_sched_job_run() load the pointer just before it is freed and set
to NULL, resulting in a use-after-free?

+	hwctx->priv->mbox_chann = NULL;
 del_ctx_req:
 	aie2_destroy_context_req(ndev, hwctx->fw_ctx_id);
 	return ret;

[Severity: Medium]
Does zeroing hwctx->priv->mbox_chann here inadvertently skip resetting
hwctx->fw_ctx_id during subsequent teardown?

Because aie2_destroy_context() checks if the channel pointer is NULL and
early-returns 0, it will completely skip resetting hwctx->fw_ctx_id to an
invalid state (e.g., AMDXDNA_INVALID_CTX_HANDLE).

If the firmware later reuses this ID for a new context, will
aie2_fill_hwctx_map() map multiple host contexts to the same firmware context
ID, leading to incorrect telemetry attribution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789334558.git.0xiviel@gmail.com?part=1