Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] fastrpc: deadlock in fastrpc_rpmsg_remove() due to TOCTOU race with context allocation --
commit cc1c8d918750aea3d9a92e77f3c2258f941ddc99 Author: Jianping Li <[email protected]> misc: fastrpc: drain in-flight invokes before tearing down context banks This commit introduces a synchronization mechanism to drain in-flight invokes before tearing down context banks. It adds a teardown flag and an invoke counter to wait until all active invokes complete during rpmsg removal. It also signals pending invokes with -EPIPE to wake them up. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index c54c450cb5714..3036925632d04 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1365,14 +1385,25 @@ static int fastrpc_internal_invoke(struct > fastrpc_user *fl, u32 kernel, > if (!fl->cctx->rpdev) > return -EPIPE; > > + spin_lock_irqsave(&fl->cctx->lock, flags); > + if (atomic_read(&fl->cctx->teardown)) { > + spin_unlock_irqrestore(&fl->cctx->lock, flags); > + return -EPIPE; > + } > + fastrpc_channel_update_invoke_cnt(fl->cctx, true); > + spin_unlock_irqrestore(&fl->cctx->lock, flags); [Severity: High] Does this create a critical race window? If the thread is preempted right after unlocking here, it has incremented invoke_cnt but hasn't yet called fastrpc_context_alloc() to add its context to the user->pending list. > > if (handle == FASTRPC_INIT_HANDLE && !kernel) { > dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a > kernel RPC message (%d)\n", handle); > - return -EPERM; > + err = -EPERM; > + goto out; > } > > ctx = fastrpc_context_alloc(fl, kernel, sc, args); [Severity: High] Could fastrpc_rpmsg_remove() run concurrently while the first thread is preempted? If so, when fastrpc_context_alloc() is finally called, it seems it is too late to receive the teardown notification since fastrpc_rpmsg_remove() has already swept the pending list. [ ... ] > @@ -2678,11 +2716,25 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device > *rpdev) > > /* No invocations past this point */ > spin_lock_irqsave(&cctx->lock, flags); > - cctx->rpdev = NULL; > + atomic_set(&cctx->teardown, 1); > list_for_each_entry(user, &cctx->users, user) > fastrpc_notify_users(user); > spin_unlock_irqrestore(&cctx->lock, flags); [Severity: High] Does this notify loop miss the context that is still suspended between the counter increment and allocation? fastrpc_notify_users() only iterates over the user->pending list, which the preempted thread has not yet reached. > > + /* > + * Wait for every invoke that was already past the gate to finish. > + * They have all just been woken with -EPIPE, and no new one can be > + * counted, so this is guaranteed to make progress. > + */ > + spin_lock_irqsave(&cctx->lock, flags); > + while (cctx->invoke_cnt > 0) { > + spin_unlock_irqrestore(&cctx->lock, flags); > + wait_event(cctx->ssr_wait_queue, cctx->invoke_cnt == 0); [Severity: High] Will this wait forever? The preempted context missed the explicit abort sweep, so it will block indefinitely in fastrpc_wait_for_completion(). Because the DSP is tearing down, it will never reply, meaning invoke_cnt never drops to 0, which would permanently deadlock the removal thread. > + spin_lock_irqsave(&cctx->lock, flags); > + } > + cctx->rpdev = NULL; > + spin_unlock_irqrestore(&cctx->lock, flags); > + > if (cctx->fdevice) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
