Re: [PATCH v1 3/3] misc: fastrpc: drain in-flight invokes before tearing down context banks

[sashiko-bot@kernel.org]

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 <jianping.li@oss.qualcomm.com>

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/20261007084447.922-1-jianping.li@oss.qualcomm.com?part=3