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

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

[Sishuai Gong]

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.

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);
—

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

[asmadeus]

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

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

[Christian Schoenebeck]

On Monday, October 23, 2023 2:54:53 PM CEST asmadeus@codewreck.org wrote:
> 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.

Same here: I don't see it making it worse, so at least a little improvement:

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

> > 
> > 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);
> > —
> > 
> >