From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: fix context leak and hang on signal-interrupted invoke Date: Tue, 26 May 2026 07:14:36 +1000 Message-ID: In-Reply-To: <20260525124222.3082420-1-anandu.e@oss.qualcomm.com> References: <20260525124222.3082420-1-anandu.e@oss.qualcomm.com> <20260525124222.3082420-1-anandu.e@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **1. PID matching uses thread ID, not process ID =E2=80=94 naming confusion= and potential multi-thread issue** ```c +static struct fastrpc_invoke_ctx *fastrpc_context_restore_interrupted( + struct fastrpc_user *fl, u32 sc) +{ + struct fastrpc_invoke_ctx *ctx =3D NULL, *ictx, *n; + + spin_lock(&fl->lock); + list_for_each_entry_safe(ictx, n, &fl->interrupted, node) { + if (ictx->pid !=3D current->pid) + continue; + if (ictx->sc !=3D sc || ictx->fl !=3D fl) { + spin_unlock(&fl->lock); + return ERR_PTR(-EINVAL); + } ``` `ctx->pid` is set from `current->pid` (which is the kernel thread ID / `get= tid()`, not the userspace `getpid()` concept). The `fl` (fastrpc_user) is p= er-fd, so if multiple threads share the same fd, multiple threads can have = entries on `fl->interrupted`. The matching by `current->pid` (really TID) i= s correct for identifying the right thread's context. However, the error path is problematic: if a thread finds an interrupted co= ntext with matching TID but **different** `sc`, it returns `-EINVAL` and th= e lock is released. But what about the case where a thread was interrupted = on one `sc` and now calls with a different `sc`? That's a legitimate scenar= io (the userspace decided to abandon the old call and make a new one). Retu= rning `-EINVAL` prevents the thread from making **any** new invoke call unt= il the file is closed. The old interrupted context also remains stranded on= the list. A better approach might be to free the stale interrupted context= and let the new call proceed. Also, the `ictx->fl !=3D fl` check is redundant =E2=80=94 all contexts on `= fl->interrupted` have `fl` as their user by construction. **2. Missing `handle` parameter validation in the restore path** ```c + if (!kernel) { + ctx =3D fastrpc_context_restore_interrupted(fl, sc); + if (IS_ERR(ctx)) + return PTR_ERR(ctx); + if (ctx) + goto wait; + } ``` The restore only matches on `sc` (scalars descriptor), but `fastrpc_interna= l_invoke` also takes a `handle` parameter. A restored context could have a = different handle than the retry. The `handle` is not stored in `fastrpc_inv= oke_ctx`, so it can't be validated currently, but this should be considered= =E2=80=94 if userspace retries with the same `sc` but a different `handle`= , the wrong context would be restored. **3. mmaps handling removed without replacement** The old code moved `fl->mmaps` to `cctx->invoke_interrupted_mmaps` on `-ERE= STARTSYS`: ```c - if (err =3D=3D -ERESTARTSYS) { - list_for_each_entry_safe(buf, b, &fl->mmaps, node) { - list_del(&buf->node); - list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps); - } - } ``` The patch removes this but doesn't add any equivalent handling for the mmap= s. The context is preserved, but what about the DMA buffers on `fl->mmaps` = that were associated with the invoke? If a retry happens and goes through `= fastrpc_get_args` again (which won't happen due to the `goto wait`, but if = the context is eventually freed without completing, the mmaps might be orph= aned). It would be good to clarify in the commit message why the mmap migra= tion is no longer needed =E2=80=94 presumably because the context itself no= w survives and holds references to the maps, but this should be explicitly = stated. **4. `fastrpc_notify_users` iterates `interrupted` list and calls `complete= ()` =E2=80=94 but nobody is waiting** ```c + list_for_each_entry(ctx, &user->interrupted, node) { + ctx->retval =3D -EPIPE; + complete(&ctx->work); + } ``` Contexts on the `interrupted` list are by definition **not** being waited o= n =E2=80=94 the thread returned `-ERESTARTSYS` and left the wait. Calling `= complete()` here sets the completion so that if the thread retries and ente= rs the wait again, it will immediately see `-EPIPE`. This is functional, bu= t subtle =E2=80=94 it relies on the thread re-entering `wait_for_completion= _interruptible()` after the context is restored. If the thread never retrie= s (e.g., process is exiting), the context is cleaned up in `fastrpc_device_= release`. This seems correct but deserves a comment explaining the intent. **5. Bail path logic change alters `-ETIMEDOUT` handling** Original: ```c if (err !=3D -ERESTARTSYS && err !=3D -ETIMEDOUT) { /* We are done with this compute context */ spin_lock(&fl->lock); list_del(&ctx->node); spin_unlock(&fl->lock); fastrpc_context_put(ctx); } ``` New: ```c + if (ctx && err =3D=3D -ERESTARTSYS) { + fastrpc_context_save_interrupted(ctx); + } else if (ctx && err !=3D -ETIMEDOUT) { ``` The `ctx &&` guard is added, which is necessary because the restore path co= uld hypothetically have `ctx =3D=3D NULL` by the time we reach bail... exce= pt it can't, since if `ctx` is NULL the function would have proceeded to `f= astrpc_context_alloc` which always assigns `ctx`. Actually, reviewing the f= low: if `fastrpc_context_restore_interrupted` returns NULL, we fall through= to `fastrpc_context_alloc`, so `ctx` is always non-NULL at `bail`. The gua= rd is harmless but unnecessary for non-kernel paths. For the kernel path, `= ctx` was always non-NULL in the original code too. So the `ctx &&` check is= defensive but not required. **6. `list_for_each_entry_safe` may not be needed in the restore function** ```c + list_for_each_entry_safe(ictx, n, &fl->interrupted, node) { ``` Since the function `break`s after finding the first match (and the early re= turn on mismatch also exits the loop), `list_for_each_entry_safe` isn't str= ictly needed =E2=80=94 `list_for_each_entry` would suffice since at most on= e `list_del` happens before exiting. This is a minor style point. **7. Style: unnecessary braces around single statement** ```c + if (ctx && err =3D=3D -ERESTARTSYS) { + fastrpc_context_save_interrupted(ctx); + } else if (ctx && err !=3D -ETIMEDOUT) { ``` The kernel coding style generally doesn't require braces for single-stateme= nt `if` bodies, though this is minor and some subsystems tolerate it in `if= /else if` chains for visual consistency. **Summary of requested changes:** 1. **(Important)** Reconsider the `-EINVAL` return when `sc` doesn't match = =E2=80=94 either free the stale context and allow the new call, or at minim= um document why this restriction exists. 2. **(Important)** Consider validating the `handle` in the restore path, or= document why `sc` alone is sufficient for correctness. 3. **(Minor)** Remove the redundant `ictx->fl !=3D fl` check. 4. **(Minor)** Add a brief comment in `fastrpc_notify_users` explaining why= `complete()` is called on contexts that aren't currently being waited on. 5. **(Minor)** Explain in the commit message why the mmap migration to `inv= oke_interrupted_mmaps` is no longer needed. --- Generated by Claude Code Patch Reviewer