Re: [PATCH] 9p/trans_fd: avoid sending req to a cancelled conn

[asmadeus@codewreck.org]

Sishuai Gong wrote on Tue, Aug 08, 2023 at 12:44:31PM -0400:
> When a connection is cancelled by p9_conn_cancel(), all requests on it
> should be cancelled---mark req->status as REQ_STATUS_ERROR. However,
> because a race over m->err between p9_conn_cancel() and p9_fd_request(),
> p9_fd_request might see the old value of m->err, think that the connection
> is NOT cancelled, and then add new requests to this cancelled connection.
> 
> Fixing this issue by lock-protecting the check on m->err.

Sorry for the (long) delay in reviewing this, and thanks for the patch!

I've got a bit of a mixed feelings on this, as there are other obviously
unlocked m->err accesses in the work functions and we don't really want
to take the req lock there everytime.. These should be ok in the normal
p9_conn_destroy that disarms poll and cancels work before calling
p9_conn_cancel, but the other calls to p9_conn_cancel are simiarly racy
and for example a cancel that'd be initiated by the write end could
process a new request in p9_read_work after the reqs have been drained
out...

On the the other hand, this is better than the current situation so I
think we should take it for now until someone has time to do better;
it's just leak in the unlikely case this race is hit but we might as
well close part of the problem.

I don't see how that'd break anything anyway, so I'll push this to my
next branch tomorrow after testing the rest of my patches.

> 
> Signed-off-by: Sishuai Gong <sishuai.system@gmail.com>
> ---
>  net/9p/trans_fd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 00b684616e8d..e43a850f5190 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -671,10 +671,14 @@ static int p9_fd_request(struct p9_client *client, struct p9_req_t *req)
> 
>  	p9_debug(P9_DEBUG_TRANS, "mux %p task %p tcall %p id %d\n",
>  		 m, current, &req->tc, req->tc.id);
> -	if (m->err < 0)
> -		return m->err;
> 
>  	spin_lock(&m->req_lock);
> +
> +	if (m->err < 0) {
> +		spin_unlock(&m->req_lock);
> +		return m->err;
> +	}
> +
>  	WRITE_ONCE(req->status, REQ_STATUS_UNSENT);
>  	list_add_tail(&req->req_list, &m->unsent_req_list);
>  	spin_unlock(&m->req_lock);
> —
> 
> 

-- 
Dominique Martinet | Asmadeus