[Devel] [PATCH VZ10] fs/fuse kio: track pending kRPC connect via state machine only

[Devel] [PATCH VZ10] fs/fuse kio: track pending kRPC connect via state machine only

[Liu Kui]

Rework the previous fix ("fs/fuse kio: fix kRPC connect issues") to not
require the new struct pcs_krpc member "connect_req", so the fix can be
shipped as a livepatch.

Both things connect_req was tracking are already derivable from the
existing state machine once PCS_KRPC_STATE_CONNECT is made to mean
exactly "a connect req is in flight":

 - krpc_connect_done() settles a failed connect back to UNCONN instead
   of leaving the state in CONNECT forever;

 - pcs_krpc_abort() no longer resets CONNECT to UNCONN: the req is
   still in flight, and only its completion settles the state;

 - pcs_krpc_connect() proceeds only from UNCONN or ABORTED, refusing
   new connects (-EPERM) while a req is in flight - at most one connect
   req exists at a time, same as with the connect_req check;

 - pcs_krpc_poll() reports EPOLLERR on UNCONN: poll bails out earlier
   unless ctx->gen == krpc->gen, and the current session can only be in
   UNCONN if its connect failed or was aborted, which is what the
   (CONNECT && !connect_req) test used to detect.

gen only advances in pcs_krpc_connect(), which is blocked during
CONNECT, so within that state the in-flight req always carries the
current gen and krpc_connect_done()'s existing staleness check is
sufficient.

Related to:
https://virtuozzo.atlassian.net/browse/VSTOR-135626

Signed-off-by: Liu Kui <kui.liu@virtuozzo.com>
---
 fs/fuse/kio/pcs/pcs_krpc.c | 39 ++++++++++++++++++++++++++++----------
 fs/fuse/kio/pcs/pcs_krpc.h |  2 --
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/fs/fuse/kio/pcs/pcs_krpc.c b/fs/fuse/kio/pcs/pcs_krpc.c
index b55c093c13d0..bcfbfc6d9304 100644
--- a/fs/fuse/kio/pcs/pcs_krpc.c
+++ b/fs/fuse/kio/pcs/pcs_krpc.c
@@ -738,8 +738,12 @@ static int pcs_krpc_abort(struct pcs_krpc *krpc)
 	spin_lock(&krpc->lock);
 
 	if (krpc->state != PCS_KRPC_STATE_CONNECTED) {
-		if (krpc->state == PCS_KRPC_STATE_CONNECT)
-			krpc->state = PCS_KRPC_STATE_UNCONN;
+		/*
+		 * A pending connect stays in CONNECT state: its connect req
+		 * is still in flight and krpc_connect_done() will settle the
+		 * state to UNCONN when it completes.  Until then new connects
+		 * are refused, so at most one connect req exists at a time.
+		 */
 		spin_unlock(&krpc->lock);
 		return 0;
 	}
@@ -907,8 +911,13 @@ static __poll_t pcs_krpc_poll(struct file *file, poll_table *wait)
 
 	spin_lock(&krpc->lock);
 
+	/*
+	 * ctx->gen == krpc->gen (checked above) means this is the current
+	 * session, so UNCONN here can only mean its connect attempt has
+	 * failed (see krpc_connect_done()) or the session was aborted.
+	 */
 	if (krpc->state == PCS_KRPC_STATE_ABORTED ||
-	    (krpc->state == PCS_KRPC_STATE_CONNECT && !krpc->connect_req)) {
+	    krpc->state == PCS_KRPC_STATE_UNCONN) {
 		pollflags |= EPOLLERR;
 	} else if (krpc->state == PCS_KRPC_STATE_CONNECTED) {
 		pollflags |= EPOLLOUT;
@@ -999,7 +1008,6 @@ int pcs_krpc_create(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id,
 	krpc->gen = 0;
 	krpc->state = PCS_KRPC_STATE_UNCONN;
 	krpc->cs = NULL;
-	krpc->connect_req = NULL;
 
 	krpc->rpc = pcs_rpc_clnt_create(&cc_from_krpcset(krpcs)->eng, id, addr, cs_flags);
 	if (!krpc->rpc) {
@@ -1051,8 +1059,6 @@ static void krpc_connect_done(struct pcs_msg *msg)
 	}
 
 	spin_lock(&krpc->lock);
-	if (krpc->connect_req == req)
-		krpc->connect_req = NULL;
 	/* from a stale session, do nothing  */
 	if (req->gen != krpc->gen || krpc->state != PCS_KRPC_STATE_CONNECT) {
 		spin_unlock(&krpc->lock);
@@ -1062,6 +1068,14 @@ static void krpc_connect_done(struct pcs_msg *msg)
 	if (!pcs_if_error(&msg->error)) {
 		krpc->state = PCS_KRPC_STATE_CONNECTED;
 		pollflags = EPOLLOUT;
+	} else {
+		/*
+		 * Connect failed: settle back to UNCONN so that a new connect
+		 * is allowed again, and report the failure to poll().  Since
+		 * gen is unchanged, the current session's poll sees UNCONN
+		 * and returns EPOLLERR.
+		 */
+		krpc->state = PCS_KRPC_STATE_UNCONN;
 	}
 	spin_unlock(&krpc->lock);
 
@@ -1119,9 +1133,15 @@ int pcs_krpc_connect(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id)
 	}
 
 	spin_lock(&krpc->lock);
-	if (krpc->state == PCS_KRPC_STATE_CONNECTED ||
-	    krpc->state == PCS_KRPC_STATE_DESTROYED ||
-	    krpc->connect_req) {
+	/*
+	 * A connect is allowed only when there is neither an established
+	 * session nor a connect req in flight (CONNECT state, see
+	 * krpc_connect_done()).  This limits connect reqs to one at a time:
+	 * if userspace gave up on a connect and retries, the new connect
+	 * fails immediately until the old req completes.
+	 */
+	if (krpc->state != PCS_KRPC_STATE_UNCONN &&
+	    krpc->state != PCS_KRPC_STATE_ABORTED) {
 		spin_unlock(&krpc->lock);
 		err = -EPERM;
 		/* fput() drops ctx and its krpc reference via pcs_krpc_release() */
@@ -1133,7 +1153,6 @@ int pcs_krpc_connect(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id)
 	connect_req->gen = krpc->gen;
 	connect_req->krpc = pcs_krpc_get(krpc);
 	krpc->state = PCS_KRPC_STATE_CONNECT;
-	krpc->connect_req = connect_req;
 	spin_unlock(&krpc->lock);
 
 	/* publish the fd only after the connect is committed */
diff --git a/fs/fuse/kio/pcs/pcs_krpc.h b/fs/fuse/kio/pcs/pcs_krpc.h
index 3db9e9712019..6a090ef66185 100644
--- a/fs/fuse/kio/pcs/pcs_krpc.h
+++ b/fs/fuse/kio/pcs/pcs_krpc.h
@@ -82,8 +82,6 @@ struct pcs_krpc {
 	/** Wait queue head for poll */
 	wait_queue_head_t		poll_wait;
 	struct pcs_cs			*cs;
-
-	struct krpc_connect_req *connect_req;
 };
 
 struct pcs_krpc_context {
-- 
2.50.1 (Apple Git-155)

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH VZ10] fs/fuse kio: track pending kRPC connect via state machine only

[Konstantin Khorenko]

Liu, please explain what do i do with that patch?
Should i revert ("fs/fuse kio: fix kRPC connect issues") and push this patch instead?

Should we release it as an RK? If yes - for which kernels and where are the corresponding bugs?
Currently i understand nothing, sorry.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 8/27/26 14:37, Liu Kui wrote:
> Rework the previous fix ("fs/fuse kio: fix kRPC connect issues") to not
> require the new struct pcs_krpc member "connect_req", so the fix can be
> shipped as a livepatch.
> 
> Both things connect_req was tracking are already derivable from the
> existing state machine once PCS_KRPC_STATE_CONNECT is made to mean
> exactly "a connect req is in flight":
> 
>  - krpc_connect_done() settles a failed connect back to UNCONN instead
>    of leaving the state in CONNECT forever;
> 
>  - pcs_krpc_abort() no longer resets CONNECT to UNCONN: the req is
>    still in flight, and only its completion settles the state;
> 
>  - pcs_krpc_connect() proceeds only from UNCONN or ABORTED, refusing
>    new connects (-EPERM) while a req is in flight - at most one connect
>    req exists at a time, same as with the connect_req check;
> 
>  - pcs_krpc_poll() reports EPOLLERR on UNCONN: poll bails out earlier
>    unless ctx->gen == krpc->gen, and the current session can only be in
>    UNCONN if its connect failed or was aborted, which is what the
>    (CONNECT && !connect_req) test used to detect.
> 
> gen only advances in pcs_krpc_connect(), which is blocked during
> CONNECT, so within that state the in-flight req always carries the
> current gen and krpc_connect_done()'s existing staleness check is
> sufficient.
> 
> Related to:
> https://virtuozzo.atlassian.net/browse/VSTOR-135626
> 
> Signed-off-by: Liu Kui <kui.liu@virtuozzo.com>
> ---
>  fs/fuse/kio/pcs/pcs_krpc.c | 39 ++++++++++++++++++++++++++++----------
>  fs/fuse/kio/pcs/pcs_krpc.h |  2 --
>  2 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_krpc.c b/fs/fuse/kio/pcs/pcs_krpc.c
> index b55c093c13d0..bcfbfc6d9304 100644
> --- a/fs/fuse/kio/pcs/pcs_krpc.c
> +++ b/fs/fuse/kio/pcs/pcs_krpc.c
> @@ -738,8 +738,12 @@ static int pcs_krpc_abort(struct pcs_krpc *krpc)
>  	spin_lock(&krpc->lock);
>  
>  	if (krpc->state != PCS_KRPC_STATE_CONNECTED) {
> -		if (krpc->state == PCS_KRPC_STATE_CONNECT)
> -			krpc->state = PCS_KRPC_STATE_UNCONN;
> +		/*
> +		 * A pending connect stays in CONNECT state: its connect req
> +		 * is still in flight and krpc_connect_done() will settle the
> +		 * state to UNCONN when it completes.  Until then new connects
> +		 * are refused, so at most one connect req exists at a time.
> +		 */
>  		spin_unlock(&krpc->lock);
>  		return 0;
>  	}
> @@ -907,8 +911,13 @@ static __poll_t pcs_krpc_poll(struct file *file, poll_table *wait)
>  
>  	spin_lock(&krpc->lock);
>  
> +	/*
> +	 * ctx->gen == krpc->gen (checked above) means this is the current
> +	 * session, so UNCONN here can only mean its connect attempt has
> +	 * failed (see krpc_connect_done()) or the session was aborted.
> +	 */
>  	if (krpc->state == PCS_KRPC_STATE_ABORTED ||
> -	    (krpc->state == PCS_KRPC_STATE_CONNECT && !krpc->connect_req)) {
> +	    krpc->state == PCS_KRPC_STATE_UNCONN) {
>  		pollflags |= EPOLLERR;
>  	} else if (krpc->state == PCS_KRPC_STATE_CONNECTED) {
>  		pollflags |= EPOLLOUT;
> @@ -999,7 +1008,6 @@ int pcs_krpc_create(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id,
>  	krpc->gen = 0;
>  	krpc->state = PCS_KRPC_STATE_UNCONN;
>  	krpc->cs = NULL;
> -	krpc->connect_req = NULL;
>  
>  	krpc->rpc = pcs_rpc_clnt_create(&cc_from_krpcset(krpcs)->eng, id, addr, cs_flags);
>  	if (!krpc->rpc) {
> @@ -1051,8 +1059,6 @@ static void krpc_connect_done(struct pcs_msg *msg)
>  	}
>  
>  	spin_lock(&krpc->lock);
> -	if (krpc->connect_req == req)
> -		krpc->connect_req = NULL;
>  	/* from a stale session, do nothing  */
>  	if (req->gen != krpc->gen || krpc->state != PCS_KRPC_STATE_CONNECT) {
>  		spin_unlock(&krpc->lock);
> @@ -1062,6 +1068,14 @@ static void krpc_connect_done(struct pcs_msg *msg)
>  	if (!pcs_if_error(&msg->error)) {
>  		krpc->state = PCS_KRPC_STATE_CONNECTED;
>  		pollflags = EPOLLOUT;
> +	} else {
> +		/*
> +		 * Connect failed: settle back to UNCONN so that a new connect
> +		 * is allowed again, and report the failure to poll().  Since
> +		 * gen is unchanged, the current session's poll sees UNCONN
> +		 * and returns EPOLLERR.
> +		 */
> +		krpc->state = PCS_KRPC_STATE_UNCONN;
>  	}
>  	spin_unlock(&krpc->lock);
>  
> @@ -1119,9 +1133,15 @@ int pcs_krpc_connect(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id)
>  	}
>  
>  	spin_lock(&krpc->lock);
> -	if (krpc->state == PCS_KRPC_STATE_CONNECTED ||
> -	    krpc->state == PCS_KRPC_STATE_DESTROYED ||
> -	    krpc->connect_req) {
> +	/*
> +	 * A connect is allowed only when there is neither an established
> +	 * session nor a connect req in flight (CONNECT state, see
> +	 * krpc_connect_done()).  This limits connect reqs to one at a time:
> +	 * if userspace gave up on a connect and retries, the new connect
> +	 * fails immediately until the old req completes.
> +	 */
> +	if (krpc->state != PCS_KRPC_STATE_UNCONN &&
> +	    krpc->state != PCS_KRPC_STATE_ABORTED) {
>  		spin_unlock(&krpc->lock);
>  		err = -EPERM;
>  		/* fput() drops ctx and its krpc reference via pcs_krpc_release() */
> @@ -1133,7 +1153,6 @@ int pcs_krpc_connect(struct pcs_krpc_set *krpcs, PCS_NODE_ID_T *id)
>  	connect_req->gen = krpc->gen;
>  	connect_req->krpc = pcs_krpc_get(krpc);
>  	krpc->state = PCS_KRPC_STATE_CONNECT;
> -	krpc->connect_req = connect_req;
>  	spin_unlock(&krpc->lock);
>  
>  	/* publish the fd only after the connect is committed */
> diff --git a/fs/fuse/kio/pcs/pcs_krpc.h b/fs/fuse/kio/pcs/pcs_krpc.h
> index 3db9e9712019..6a090ef66185 100644
> --- a/fs/fuse/kio/pcs/pcs_krpc.h
> +++ b/fs/fuse/kio/pcs/pcs_krpc.h
> @@ -82,8 +82,6 @@ struct pcs_krpc {
>  	/** Wait queue head for poll */
>  	wait_queue_head_t		poll_wait;
>  	struct pcs_cs			*cs;
> -
> -	struct krpc_connect_req *connect_req;
>  };
>  
>  struct pcs_krpc_context {

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH VZ10] fs/fuse kio: track pending kRPC connect via state machine only

[Konstantin Khorenko]

> Rework the previous fix ("fs/fuse kio: fix kRPC connect issues") to not
> require the new struct pcs_krpc member "connect_req", so the fix can be
> shipped as a livepatch.
> 
> Both things connect_req was tracking are already derivable from the
> existing state machine once PCS_KRPC_STATE_CONNECT is made to mean
> exactly "a connect req is in flight":
> 
>  - krpc_connect_done() settles a failed connect back to UNCONN instead
>    of leaving the state in CONNECT forever;
> 
>  - pcs_krpc_abort() no longer resets CONNECT to UNCONN: the req is
>    still in flight, and only its completion settles the state;
> 
>  - pcs_krpc_connect() proceeds only from UNCONN or ABORTED, refusing
>    new connects (-EPERM) while a req is in flight - at most one connect
>    req exists at a time, same as with the connect_req check;
> 
>  - pcs_krpc_poll() reports EPOLLERR on UNCONN: poll bails out earlier
>    unless ctx->gen == krpc->gen, and the current session can only be in
>    UNCONN if its connect failed or was aborted, which is what the
>    (CONNECT && !connect_req) test used to detect.
> 
> gen only advances in pcs_krpc_connect(), which is blocked during
> CONNECT, so within that state the in-flight req always carries the
> current gen and krpc_connect_done()'s existing staleness check is
> sufficient.
> 
> Related to:
> https://virtuozzo.atlassian.net/browse/VSTOR-135626
> Fixes: d0d6034c36010 ("fs/fuse kio: fix kRPC connect issues")
> Feature: fuse: kRPC - single RPC for kernel and userspace
> 
> Signed-off-by: Liu Kui <kui.liu@virtuozzo.com>
>
> diff --git a/fs/fuse/kio/pcs/pcs_krpc.c b/fs/fuse/kio/pcs/pcs_krpc.c
> index 0930fb4adf125..f9fb6b3699062 100644
> --- a/fs/fuse/kio/pcs/pcs_krpc.c
> +++ b/fs/fuse/kio/pcs/pcs_krpc.c
> @@ -787,8 +787,12 @@ static int pcs_krpc_abort(struct pcs_krpc *krpc)
>       spin_lock(&krpc->lock);
>
>       if (krpc->state != PCS_KRPC_STATE_CONNECTED) {
> -             if (krpc->state == PCS_KRPC_STATE_CONNECT)
> -                     krpc->state = PCS_KRPC_STATE_UNCONN;
> +             /*
> +              * A pending connect stays in CONNECT state: its connect req
> +              * is still in flight and krpc_connect_done() will settle the
> +              * state to UNCONN when it completes.  Until then new connects
> +              * are refused, so at most one connect req exists at a time.
> +              */
>               spin_unlock(&krpc->lock);
>               return 0;
>       }

[Severity: High]
Can this change leave a krpc stuck in the connected state with no
session fd attached to it?

Consider a connect req in flight (state is CONNECT) whose fd is closed
by userspace after its connect timeout expires - the scenario the
original fix was written for:

pcs_krpc_release()
    if (ctx->gen == krpc->gen)
        pcs_krpc_abort(krpc);    /* state is CONNECT: does nothing now */

gen cannot advance while the state stays CONNECT, because
pcs_krpc_connect() refuses new connects with -EPERM in that state.  So
when the in-flight req later completes successfully (the peer became
reachable again), krpc_connect_done() passes its staleness check and
commits the dead session:

    if (req->gen != krpc->gen || krpc->state != PCS_KRPC_STATE_CONNECT) {
        spin_unlock(&krpc->lock);
        goto out;
    }

    if (!pcs_if_error(&msg->error)) {
        krpc->state = PCS_KRPC_STATE_CONNECTED;

Now the krpc is CONNECTED while its gen still names a session whose fd
is gone.  Every following PCS_IOC_KRPC_CONNECT returns -EPERM, there is
no fd left on which userspace could issue PCS_KRPC_IOC_ABORT, and no
kernel path resets the state, so the node stays unconnectable until the
krpc is destroyed.

The connect_req based code handled this case: pcs_krpc_abort() moved
CONNECT to UNCONN, a successful krpc_connect_done() then took the stale
path without transitioning to CONNECTED, and the next connect was
allowed as soon as the old req completed.

Does the abort/release path need to invalidate the pending connect, so
that a late successful completion settles the state to UNCONN instead
of resurrecting the closed session?

> @@ -956,8 +960,13 @@ static __poll_t pcs_krpc_poll(struct file *file, poll_table *wait)
>
>       spin_lock(&krpc->lock);
>
> +     /*
> +      * ctx->gen == krpc->gen (checked above) means this is the current
> +      * session, so UNCONN here can only mean its connect attempt has
> +      * failed (see krpc_connect_done()) or the session was aborted.
> +      */
>       if (krpc->state == PCS_KRPC_STATE_ABORTED ||
> -         (krpc->state == PCS_KRPC_STATE_CONNECT && !krpc->connect_req)) {
> +         krpc->state == PCS_KRPC_STATE_UNCONN) {
>               pollflags |= EPOLLERR;
>       } else if (krpc->state == PCS_KRPC_STATE_CONNECTED) {
>               pollflags |= EPOLLOUT;

[Severity: Low]
This isn't a bug, but after this change pcs_krpc_abort() never sets
UNCONN, and an aborted session is left in ABORTED, which the first
check already handles.

For a session with ctx->gen == krpc->gen, can UNCONN still be reached
through an abort as the comment says, or only through a failed
connect?  The commit message carries the same wording ("the current
session can only be in UNCONN if its connect failed or was aborted").

[ ... ]

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel