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: take fl->lock when moving mmaps on interrupted invoke Date: Thu, 04 Jun 2026 13:14:39 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **What it does:** Adds `spin_lock(&fl->lock)` / `spin_unlock(&fl->lock)` ar= ound the `list_for_each_entry_safe` loop in the `-ERESTARTSYS` path of `fas= trpc_internal_invoke()`. **Positive:** - The bug is real. Every other accessor of `fl->mmaps` holds `fl->lock` =E2= =80=94 `fastrpc_req_mmap()` at line 1988, `fastrpc_req_munmap()` at line 18= 88, `fastrpc_req_munmap_impl()` at line 1868. The unprotected `list_del()` = in the interrupt path could race with any of these. - The commit message is clear and correctly identifies the Fixes: tag for t= he commit that introduced the bug. - The `Cc: stable` tag is appropriate. **Concern =E2=80=94 destination list `cctx->invoke_interrupted_mmaps` is al= so unprotected:** The patch protects the *source* list (`fl->mmaps`) with `fl->lock`, but the= *destination* list (`fl->cctx->invoke_interrupted_mmaps`) belongs to the s= hared `fastrpc_channel_ctx`, not to this `fl`. Multiple `fastrpc_user` inst= ances can share the same `cctx`. If two users are interrupted concurrently,= each holds their own `fl->lock`, and both do: ```c list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps); ``` This is a concurrent modification of the same list without a common lock = =E2=80=94 `fl->lock` is per-user, so it provides no mutual exclusion betwee= n different users on the same channel. The `cctx` has its own `spinlock_t l= ock` (line 273) that could be used here, but this patch does not acquire it. The only other accessor of `invoke_interrupted_mmaps` is in `fastrpc_rpmsg_= remove()` (line 2495), which runs during channel teardown and iterates the = list without any lock: ```c list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node) list_del(&buf->node); ``` That teardown path also has a potential race with ongoing invokes, though i= t's likely mitigated by the device going away. **Recommendation:** This patch should either also acquire `cctx->lock` arou= nd the `list_add_tail` to protect the destination list, or the commit messa= ge should note that `invoke_interrupted_mmaps` is a known pre-existing issu= e being addressed separately. As-is, the patch fixes one race but leaves a = second race on the same code path. A nested locking approach (hold `fl->loc= k` for the `list_del`, then acquire `cctx->lock` for the `list_add_tail`) w= ould work, or both operations could be done under `cctx->lock` if the lock = ordering allows it. **Minor nit (not blocking):** The `fastrpc_rpmsg_remove()` cleanup at line = 2495-2496 does `list_del(&buf->node)` but never frees `buf`, which looks li= ke a memory leak. This is pre-existing and not introduced by this patch, bu= t worth noting. --- Generated by Claude Code Patch Reviewer