From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: misc: fastrpc: Add reference counting for fastrpc_user structure
Date: Fri, 27 Feb 2026 11:57:24 +1000
Message-ID: <review-patch1-20260226151121.818852-1-anandu.e@oss.qualcomm.com>
In-Reply-To: <20260226151121.818852-1-anandu.e@oss.qualcomm.com>
References: <20260226151121.818852-1-anandu.e@oss.qualcomm.com> <20260226151121.818852-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

**kref_init placement is after the user becomes visible (BUG)**

In `fastrpc_device_open()`, the patch places `kref_init()` **after** `list_=
add_tail()`:

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

At this point `fl` is already visible on `cctx->users`. If `fastrpc_rpmsg_r=
emove()` runs on another CPU between `list_add_tail()` and `kref_init()`, i=
t will iterate `cctx->users` and could operate on this user with a zero ref=
count (from `kzalloc` zero-init). The `kref_init()` should be placed alongs=
ide the other initialization calls (near `spin_lock_init`, `mutex_init`, et=
c.) **before** the user is linked into any list:

```c
	fl->is_secure_dev =3D fdevice->secure;
+	kref_init(&fl->refcount);

	fl->sctx =3D fastrpc_session_alloc(fl);
	...
	list_add_tail(&fl->user, &cctx->users);
```

**fastrpc_user_free only does partial cleanup =E2=80=94 by design, but dese=
rves a comment**

```c
+static void fastrpc_user_free(struct kref *ref)
+{
+	struct fastrpc_user *fl =3D container_of(ref, struct fastrpc_user, refcou=
nt);
+
+	fastrpc_channel_ctx_put(fl->cctx);
+	mutex_destroy(&fl->mutex);
+	kfree(fl);
+}
```

This only frees the struct, the mutex, and the cctx reference. All heavywei=
ght cleanup (session free, DSP process release, pending context teardown, m=
ap/mmap cleanup) remains in `fastrpc_device_release()`. This is correct =E2=
=80=94 the refcount only extends the *lifetime of the struct* past device r=
elease, not the logical ownership. But a brief comment in `fastrpc_user_fre=
e()` noting this would help future readers understand that this is intentio=
nal and that full teardown is done in `fastrpc_device_release()`.

**Channel context reference accounting is correct**

I verified there is no double-put of `cctx`. The references are:

| Get | Put |
|-----|-----|
| `fastrpc_device_open()` =E2=86=92 `fastrpc_channel_ctx_get(cctx)` | `fast=
rpc_user_free()` =E2=86=92 `fastrpc_channel_ctx_put(fl->cctx)` |
| `fastrpc_context_alloc()` =E2=86=92 `fastrpc_channel_ctx_get(cctx)` | `fa=
strpc_context_free()` =E2=86=92 `fastrpc_channel_ctx_put(cctx)` (existing, =
line 524) |

In `fastrpc_context_free()`, the new `fastrpc_user_put(fl)` is called befor=
e the existing `fastrpc_channel_ctx_put(cctx)`:

```c
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 	fastrpc_channel_ctx_put(cctx);
```

Even if `fastrpc_user_put()` triggers `fastrpc_user_free()` (which puts the=
 *user's* cctx ref), the *context's* cctx ref still keeps `cctx` alive for =
the next line. This ordering is safe.

**Error path in fastrpc_context_alloc is correct**

```c
+	fastrpc_user_put(user);
 	fastrpc_channel_ctx_put(cctx);
```

The error path correctly balances the `fastrpc_user_get(user)` taken earlie=
r in the function.

**fastrpc_buf_free safety during workqueue-deferred context_free**

The key UAF scenario the patch addresses: `fastrpc_buf_free()` at `fastrpc.=
c:417` dereferences `buf->fl->cctx`. In the patched code, when `fastrpc_con=
text_free()` is called from the workqueue, `fastrpc_buf_free(ctx->buf)` run=
s *before* `fastrpc_user_put(fl)`, so the user struct is guaranteed alive. =
This is correct.

**Minor: redundant comment on struct member**

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

The `kref` type already implies reference counting. This comment adds no in=
formation. Consider dropping it to match the style of `struct kref refcount=
` used elsewhere in the same file (e.g., in `fastrpc_channel_ctx`).

**Summary of required changes:**

1. **Must fix:** Move `kref_init(&fl->refcount)` before `list_add_tail()` i=
n `fastrpc_device_open()` to close the race window.
2. **Nice to have:** Add a comment in `fastrpc_user_free()` clarifying that=
 full teardown is in `fastrpc_device_release()`, and this only extends stru=
ct lifetime.

---
Generated by Claude Code Patch Reviewer