Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] accel/qda: missing memory ordering on ctx->retval and ctx->is_work_done - [High] accel/qda: UAF and teardown race in qda_fastrpc_flush_pending() --
commit ff1bb82ba9e7f31a0c10a5ce3a19f4029819ffbf Author: Ekansh Gupta <[email protected]> accel/qda: Add poll mode for invocation completion This commit adds per-session completion polling support for dynamically loaded module invocations in a user PD via DRM_IOCTL_QDA_SET_OPTION and FASTRPC_IOCTL_SET_OPTION. It reserves a poll word at the end of the invocation metadata buffer and busy-waits on it before falling back to the interrupt-based RPMsg response. > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index 6044cfad53e96..0617bb4842b52 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c [ ... ] > @@ -598,6 +604,54 @@ int qda_fastrpc_invoke_pack(struct > qda_fastrpc_invoke_ctx *ctx, struct qda_msg * > return 0; > } > > +static u32 qda_fastrpc_read_poll_addr(struct qda_fastrpc_invoke_ctx *ctx) > +{ > + dma_rmb(); > + > + return READ_ONCE(*ctx->poll_addr); > +} > + > +/* > + * Busy-wait for the DSP to write QDA_FASTRPC_POLL_RESPONSE into the poll > word, > + * which saves the interrupt and scheduling latency of waiting for the rpmsg > + * response. The response still arrives afterwards and drops the reference > + * qda_rpmsg_send_msg() took for it as usual. > + * > + * Return: 0 if the invocation completed, negative error code if it has not > + * answered within QDA_FASTRPC_POLL_MAX_TIMEOUT_US > + */ > +static int qda_fastrpc_poll_for_rsp(struct qda_fastrpc_invoke_ctx *ctx) > +{ > + u32 val; > + int err; > + > + err = read_poll_timeout_atomic(qda_fastrpc_read_poll_addr, val, > + val == QDA_FASTRPC_POLL_RESPONSE || > + READ_ONCE(ctx->is_work_done), > + 1, QDA_FASTRPC_POLL_MAX_TIMEOUT_US, > false, ctx); > + if (err) > + return err; > + > + /* The rpmsg path records its own status, so only the poll path's is > set here */ > + if (val == QDA_FASTRPC_POLL_RESPONSE) { > + WRITE_ONCE(ctx->is_work_done, true); > + ctx->retval = 0; > + } > + > + return 0; > +} > + > +static int qda_fastrpc_wait_for_completion(struct qda_fastrpc_invoke_ctx > *ctx) > +{ > + if (ctx->is_polled && !qda_fastrpc_poll_for_rsp(ctx)) > + return 0; > + > + /* Polling timed out, so fall back to waiting for the rpmsg response */ > + ctx->is_polled = false; > + > + return qda_rpmsg_wait_for_rsp(ctx); > +} [Severity: High] Can a polled invocation observe ctx->is_work_done == true before the store to ctx->retval is visible, or race on ctx->retval between the poll and RPMsg paths? In qda_fastrpc_context_alloc(), ctx->retval is initialized to -1. When qda_rpmsg_cb() or qda_fastrpc_flush_pending() completes an invocation, it writes ctx->retval with a plain store and then sets ctx->is_work_done with WRITE_ONCE(): ctx->retval = resp_msg->retval; WRITE_ONCE(ctx->is_work_done, true); complete(&ctx->work); If read_poll_timeout_atomic() in qda_fastrpc_poll_for_rsp() exits because READ_ONCE(ctx->is_work_done) became true while val != QDA_FASTRPC_POLL_RESPONSE, qda_fastrpc_poll_for_rsp() returns 0 and qda_fastrpc_wait_for_completion() returns 0 immediately without calling qda_rpmsg_wait_for_rsp(ctx), skipping wait_for_completion_interruptible(&ctx->work). Because wait_for_completion_interruptible(&ctx->work) is bypassed and there is no release-acquire ordering between writing ctx->retval and setting ctx->is_work_done (the dma_rmb() in qda_fastrpc_invoke_common() only orders reads on the polling CPU, not the two stores on the CPU running qda_rpmsg_cb() or qda_fastrpc_flush_pending()), couldn't the polling CPU read the stale initial ctx->retval == -1 (-EPERM) in qda_fastrpc_invoke_common()? Also, neither qda_rpmsg_cb() nor qda_fastrpc_flush_pending() checks ctx->is_work_done before writing ctx->retval, so when qda_fastrpc_poll_for_rsp() sees val == QDA_FASTRPC_POLL_RESPONSE and sets ctx->retval = 0, a concurrent qda_rpmsg_cb() or qda_fastrpc_flush_pending() will still write ctx->retval without synchronization against the polling thread's write or its subsequent read in qda_fastrpc_invoke_common(). Should the writers and reader use smp_store_release() and smp_load_acquire() (or atomic state transition) to order and arbitrate updates to ctx->retval? > @@ -617,6 +671,7 @@ void qda_fastrpc_flush_pending(struct qda_dev *qdev) > > xa_for_each(&qdev->ctx_xa, idx, ctx) { > ctx->retval = -EPIPE; > + WRITE_ONCE(ctx->is_work_done, true); > complete(&ctx->work); > kref_put(&ctx->refcount, qda_fastrpc_context_free); > } [Severity: High] Does qda_fastrpc_flush_pending() risk a use-after-free or premature device teardown when polled invocations are in flight or have recently completed? There appear to be two teardown races when qda_rpmsg_remove() runs: drm_dev_unplug(&qdev->drm_dev); qdev->rpdev = NULL; qda_fastrpc_flush_pending(qdev); qda_cb_unpopulate(qdev); qda_deinit_device(qdev); First, when a polled invocation finishes via qda_fastrpc_poll_for_rsp(), the calling thread returns from qda_fastrpc_invoke() and drops its reference via kref_put(&ctx->refcount, qda_fastrpc_context_free), leaving ctx in qdev->ctx_xa with refcount == 1 until the RPMsg reply arrives. When qda_rpmsg_cb() later receives that reply, it schedules ctx->put_work to drop the final reference from process context, because ctx is only erased from qdev->ctx_xa inside qda_fastrpc_context_free(). If qda_rpmsg_remove() runs after qda_rpmsg_cb() schedules ctx->put_work but before qda_fastrpc_context_put_wq() executes, qda_fastrpc_flush_pending() will find ctx still in qdev->ctx_xa and drop its refcount to 0, freeing ctx while ctx->put_work is still pending on system_wq. Second, qda_fastrpc_poll_for_rsp() runs outside drm_dev_enter(). If a thread is actively polling in qda_fastrpc_poll_for_rsp() when qda_rpmsg_remove() runs, qda_fastrpc_flush_pending() sets ctx->is_work_done = true and drops the RPMsg reference (decrementing refcount from 2 to 1), and qda_rpmsg_remove() immediately proceeds to qda_cb_unpopulate(qdev) and qda_deinit_device(qdev) without waiting for the polling thread to exit and drop its final reference. When the polling thread subsequently runs qda_fastrpc_context_free(), won't it call xa_erase() on the destroyed qdev->ctx_xa and free DMA buffers against the destroyed context-bank platform devices? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=25
