Claude review: misc: fastrpc: fix context leak and hang on signal-interrupted invoke

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. PID matching uses thread ID, not process ID — 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 = NULL, *ictx, *n;
+
+	spin_lock(&fl->lock);
+	list_for_each_entry_safe(ictx, n, &fl->interrupted, node) {
+		if (ictx->pid != current->pid)
+			continue;
+		if (ictx->sc != sc || ictx->fl != fl) {
+			spin_unlock(&fl->lock);
+			return ERR_PTR(-EINVAL);
+		}
```

`ctx->pid` is set from `current->pid` (which is the kernel thread ID / `gettid()`, not the userspace `getpid()` concept). The `fl` (fastrpc_user) is per-fd, so if multiple threads share the same fd, multiple threads can have entries on `fl->interrupted`. The matching by `current->pid` (really TID) is correct for identifying the right thread's context.

However, the error path is problematic: if a thread finds an interrupted context with matching TID but **different** `sc`, it returns `-EINVAL` and the 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 scenario (the userspace decided to abandon the old call and make a new one). Returning `-EINVAL` prevents the thread from making **any** new invoke call until 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 != fl` check is redundant — 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 = 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_internal_invoke` also takes a `handle` parameter. A restored context could have a different handle than the retry. The `handle` is not stored in `fastrpc_invoke_ctx`, so it can't be validated currently, but this should be considered — 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 `-ERESTARTSYS`:

```c
-	if (err == -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 mmaps. 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 orphaned). It would be good to clarify in the commit message why the mmap migration is no longer needed — presumably because the context itself now survives and holds references to the maps, but this should be explicitly stated.

**4. `fastrpc_notify_users` iterates `interrupted` list and calls `complete()` — but nobody is waiting**

```c
+	list_for_each_entry(ctx, &user->interrupted, node) {
+		ctx->retval = -EPIPE;
+		complete(&ctx->work);
+	}
```

Contexts on the `interrupted` list are by definition **not** being waited on — the thread returned `-ERESTARTSYS` and left the wait. Calling `complete()` here sets the completion so that if the thread retries and enters the wait again, it will immediately see `-EPIPE`. This is functional, but subtle — it relies on the thread re-entering `wait_for_completion_interruptible()` after the context is restored. If the thread never retries (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 != -ERESTARTSYS && err != -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 == -ERESTARTSYS) {
+		fastrpc_context_save_interrupted(ctx);
+	} else if (ctx && err != -ETIMEDOUT) {
```

The `ctx &&` guard is added, which is necessary because the restore path could hypothetically have `ctx == NULL` by the time we reach bail... except it can't, since if `ctx` is NULL the function would have proceeded to `fastrpc_context_alloc` which always assigns `ctx`. Actually, reviewing the flow: if `fastrpc_context_restore_interrupted` returns NULL, we fall through to `fastrpc_context_alloc`, so `ctx` is always non-NULL at `bail`. The guard 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 return on mismatch also exits the loop), `list_for_each_entry_safe` isn't strictly needed — `list_for_each_entry` would suffice since at most one `list_del` happens before exiting. This is a minor style point.

**7. Style: unnecessary braces around single statement**

```c
+	if (ctx && err == -ERESTARTSYS) {
+		fastrpc_context_save_interrupted(ctx);
+	} else if (ctx && err != -ETIMEDOUT) {
```

The kernel coding style generally doesn't require braces for single-statement `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 — either free the stale context and allow the new call, or at minimum 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 != 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 `invoke_interrupted_mmaps` is no longer needed.

---
Generated by Claude Code Patch Reviewer