Claude review: misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

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

Patch Review

**Commit message**: Well-written. Clearly establishes the race, identifies all three UAF dereference sites, explains the fix approach, and includes the crash trace. Good use of Fixes/Cc: stable tags.

**kref_init ordering (bug)**

In `fastrpc_device_open()`, the kref is initialized **after** the user is added to the channel's user list:

```c
	spin_lock_irqsave(&cctx->lock, flags);
	list_add_tail(&fl->user, &cctx->users);
	spin_unlock_irqrestore(&cctx->lock, flags);
+	kref_init(&fl->refcount);
```

The refcount must be initialized **before** the structure becomes visible to other code paths. `fastrpc_rpmsg_remove()` traverses `cctx->users` under the same lock, and could race with open. Move `kref_init(&fl->refcount)` before the `list_add_tail`, ideally next to `spin_lock_init`/`mutex_init` where other initialization happens.

**Use-after-free on fl->pending in fastrpc_user_free() (bug)**

This is the most significant issue. The patch moves the pending context cleanup from `fastrpc_device_release()` into the kref release callback `fastrpc_user_free()`:

```c
+static void fastrpc_user_free(struct kref *ref)
+{
+	...
+	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
+		list_del(&ctx->node);
+		fastrpc_context_put(ctx);
+	}
```

But `fastrpc_context_free()` (the context kref release) does `kfree(ctx)` **without** removing `ctx->node` from `fl->pending`, then calls `fastrpc_user_put()`:

```c
 static void fastrpc_context_free(struct kref *ref)
 {
 	...
 	kfree(ctx);
 
 	fastrpc_channel_ctx_put(cctx);
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 }
```

Consider the "abandoned invoke" scenario (invoker interrupted with `-ERESTARTSYS` or timed out with `-ETIMEDOUT`):

1. Invoker sets `is_work_done = true` (or equivalent), does **not** call `list_del(&ctx->node)` or `fastrpc_context_put(ctx)`. Context remains on `fl->pending` with refcount 1; user refcount is 2 (initial + context).
2. `fastrpc_device_release()` calls `fastrpc_user_put()` → user refcount drops to 1.
3. DSP eventually responds; callback schedules `ctx->put_work`.
4. Workqueue runs `fastrpc_context_put_wq()` → `fastrpc_context_free()`:
   - `kfree(ctx)` — context memory freed, but `ctx->node` is **still linked** into `fl->pending`.
   - `fastrpc_user_put(fl)` → user refcount drops to 0 → `fastrpc_user_free()` runs.
5. `fastrpc_user_free()` does `list_for_each_entry_safe` over `fl->pending` — dereferences the just-freed `ctx`. **Use-after-free.**

With the patch's refcounting model, every context on `fl->pending` holds a user reference. For `fastrpc_user_free()` to execute, **all** those contexts must have already been freed (their `fastrpc_context_free()` called `fastrpc_user_put()`). So the pending list iteration in `fastrpc_user_free()` can never find a live context — it either finds an empty list or freed entries.

To fix this, `fastrpc_context_free()` must remove `ctx->node` from `fl->pending` before `kfree(ctx)`. Since the normal invoke path (`fastrpc_internal_invoke` bail label) already does `list_del(&ctx->node)`, you'd need to avoid double-delete. A clean approach:

- Change `fastrpc_internal_invoke`'s `list_del(&ctx->node)` to `list_del_init(&ctx->node)`.
- In `fastrpc_context_free()`, before `kfree(ctx)`, add:
  ```c
  spin_lock(&fl->lock);
  if (!list_empty(&ctx->node))
      list_del(&ctx->node);
  spin_unlock(&fl->lock);
  ```
- Then the `fl->pending` iteration in `fastrpc_user_free()` becomes a safety net for any contexts that somehow weren't cleaned up, rather than a guaranteed UAF.

**fastrpc_session_free ordering (minor observation)**

`fastrpc_session_free(cctx, fl->sctx)` remains in `fastrpc_device_release()` rather than moving to `fastrpc_user_free()`. This means if the user structure outlives `device_release` (because a context still holds a reference), the session is already freed when the deferred cleanup runs. This appears safe since `fastrpc_buf_free()` and `fastrpc_free_map()` dereference `fl->cctx`, not `fl->sctx`, but it deserves an explicit comment noting the intentional ordering.

**Trivial comment nit**

```c
+	/* Reference count */
+	struct kref refcount;
```

This comment adds no information beyond what `struct kref refcount` already conveys. Either drop it or say something about the lifetime contract (e.g., `/* one ref per open + one per in-flight ctx */`).

**Forward declaration**

```c
+static void fastrpc_context_put(struct fastrpc_invoke_ctx *ctx);
```

Needed because `fastrpc_user_free()` is defined before `fastrpc_context_put()`. This is acceptable but worth noting: if the pending list iteration is removed from `fastrpc_user_free()` (because it's unreachable with valid data), this forward declaration can also be removed.

---
Generated by Claude Code Patch Reviewer