Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] accel/qda: premature publication of ctx in qda_fastrpc_context_alloc() - [High] accel/qda: premature ctx_xa destruction in qda_deinit_device() --
commit 0b392f877740b474d80d7f4392edfd3f880836d4 Author: Ekansh Gupta <[email protected]> accel/qda: Add FastRPC invocation contexts This commit adds FastRPC invocation context allocation and reference-counted cleanup, tracking in-flight invocations per device in an XArray with an 8-bit context ID and a 48-bit sequence number. > diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c > index 21113deceaadb..bdfb23c3c0b1d 100644 > --- a/drivers/accel/qda/qda_drv.c > +++ b/drivers/accel/qda/qda_drv.c > @@ -100,6 +100,8 @@ struct qda_dev *qda_alloc_device(struct device *dev) > > INIT_LIST_HEAD(&qdev->cb_devs); > mutex_init(&qdev->import_lock); > + xa_init_flags(&qdev->ctx_xa, XA_FLAGS_ALLOC1); > + atomic64_set(&qdev->ctx_seq, 0); > > return qdev; > } > @@ -122,6 +124,7 @@ int qda_init_device(struct qda_dev *qdev, int num_cbs) > void qda_deinit_device(struct qda_dev *qdev) > { > qda_memory_manager_exit(qdev->iommu_mgr); > + xa_destroy(&qdev->ctx_xa); > mutex_destroy(&qdev->import_lock); > } [Severity: High] Can calling xa_destroy(&qdev->ctx_xa) in qda_deinit_device() in drivers/accel/qda/qda_drv.c race with outstanding invocation contexts that have not yet dropped their final reference? In qda_fastrpc_context_alloc() in drivers/accel/qda/qda_fastrpc.c, each struct qda_fastrpc_invoke_ctx is inserted into qdev->ctx_xa with an initial caller reference (refcount = 1) and stores a raw pointer ctx->qdev = qdev without holding a reference on qdev->drm_dev. By design, the entry is only erased from qdev->ctx_xa in qda_fastrpc_context_free() when the last reference is released. When invocations are wired up in the subsequent commits ("accel/qda: Add RPMsg send and response dispatch" and "accel/qda: Add the invocation ioctl and teardown on unplug"), qda_rpmsg_send_msg() takes a second reference for the response path and the caller sleeps in qda_rpmsg_wait_for_rsp() outside any drm_dev_enter() critical section. If the rpmsg device is removed while callers are waiting (for example, on a DSP crash or remoteproc restart), qda_rpmsg_remove() runs drm_dev_unplug(), qda_fastrpc_flush_pending(qdev), and then immediately qda_deinit_device(qdev). Because qda_fastrpc_flush_pending() only wakes the waiters and drops the response reference, the callers still hold their reference (and if qda_rpmsg_cb() already ran and queued ctx->put_work, ctx is still in qdev->ctx_xa while put_work is pending, causing qda_fastrpc_flush_pending() to drop a reference that put_work will also drop). When qda_deinit_device() immediately calls xa_destroy(&qdev->ctx_xa), the internal XArray nodes are freed while live contexts remain in the table. When those woken callers or pending put_work items later run kref_put(&ctx->refcount, qda_fastrpc_context_free), won't qda_fastrpc_context_free() call xa_erase(&ctx->qdev->ctx_xa, ...) on an already-destroyed XArray and dereference ctx->qdev after device teardown? > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > new file mode 100644 > index 0000000000000..14d9da8c20b3f > --- /dev/null > +++ b/drivers/accel/qda/qda_fastrpc.c > @@ -0,0 +1,63 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > +#include <linux/bitfield.h> > +#include <linux/slab.h> > +#include "qda_drv.h" > +#include "qda_fastrpc.h" > + > +/** > + * qda_fastrpc_context_free() - Release an invocation > + * @ref: Reference counter embedded in the invocation > + * > + * Runs when the last reference goes. The invocation leaves the lookup table > + * here and not earlier, so that its ID cannot be handed to a new invocation > + * while this one can still be found and completed. > + */ > +void qda_fastrpc_context_free(struct kref *ref) > +{ > + struct qda_fastrpc_invoke_ctx *ctx = > + container_of(ref, struct qda_fastrpc_invoke_ctx, refcount); > + > + xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, > ctx->ctxid)); > + kfree(ctx); > +} > + > +/** > + * qda_fastrpc_context_alloc() - Allocate an invocation and give it an ID > + * @qdev: Device the invocation is issued on > + * > + * The caller holds the initial reference and drops it with > + * qda_fastrpc_context_free(). > + * > + * Return: The new invocation, or ERR_PTR on failure > + */ > +struct qda_fastrpc_invoke_ctx *qda_fastrpc_context_alloc(struct qda_dev > *qdev) > +{ > + struct qda_fastrpc_invoke_ctx *ctx; > + u32 id; > + int ret; > + > + ctx = kzalloc_obj(*ctx); > + if (!ctx) > + return ERR_PTR(-ENOMEM); > + > + init_completion(&ctx->work); > + kref_init(&ctx->refcount); > + ctx->qdev = qdev; > + /* Anything but a status the DSP could send, until it answers */ > + ctx->retval = -1; > + > + /* ID 0 is never used, so a zeroed context in a response matches > nothing */ > + ret = xa_alloc(&qdev->ctx_xa, &id, ctx, XA_LIMIT(1, > QDA_FASTRPC_CTXID_MAX), > + GFP_KERNEL); > + if (ret) { > + kfree(ctx); > + return ERR_PTR(ret); > + } > + > + ctx->ctxid = FIELD_PREP(QDA_FASTRPC_CTXID_MASK, id) | > + FIELD_PREP(QDA_FASTRPC_CTXID_SEQ_MASK, > + (u64)atomic64_inc_return(&qdev->ctx_seq)); [Severity: High] In qda_fastrpc_context_alloc() in drivers/accel/qda/qda_fastrpc.c, does calling xa_alloc(&qdev->ctx_xa, &id, ctx, ...) publish ctx into qdev->ctx_xa and drop xa_lock before ctx->ctxid is initialized? Because the 8-bit context IDs (1..255) are reused as soon as a prior invocation is freed, a concurrent lookup on qdev->ctx_xa for that recycled id (such as qda_rpmsg_cb() handling a late, duplicate, or malformed DSP response, or qda_fastrpc_flush_pending() during device unplug) can observe ctx while ctx->ctxid is still 0 or is being written without locking: 1. If a late or malformed response carries sequence number 0 in bits [63:16] for slot id, FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, ctx->ctxid) evaluates to 0 and matches the uninitialized ctx->ctxid, completing ctx->work early and scheduling ctx->put_work to drop a reference that qda_rpmsg_send_msg() has not yet taken. 2. If qda_fastrpc_context_free() runs while ctx->ctxid is still 0, xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, ctx->ctxid)) erases index 0 instead of index id and frees ctx, leaving a dangling pointer in qdev->ctx_xa[id]. Should qda_fastrpc_context_alloc() either hold xa_lock(&qdev->ctx_xa) across __xa_alloc() and the ctx->ctxid assignment, or reserve the slot with NULL first and store ctx only after ctx->ctxid is initialized? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
