[PATCH] sunrpc/xs_read_stream: fix dead loop when xs_read_discard

[PATCH] sunrpc/xs_read_stream: fix dead loop when xs_read_discard

[jack.zhou]

From: "jack.zhou" <jacketzc@outlook.com>

Hi maintainers,

This bug was discovered during following scenario:

1. Using NFS client with TCP mode
2. RPC_REPLY process


Let's first review the normal process:

1. The `xs_read_stream` function is located within the for loop of the `xs_stream_data_receive` function. The condition for exiting is that the return value ret of the `xs_read_stream` method is < 0.
2. When entering the function `xs_read_stream`, since `transport->recv.len` == 0, `xs_read_stream_header` will be called first. Based on `transport->recv.calldir`, it decides whether to execute `RPC_CALL` or `RPC_REPLY`;(and also sets the value of `transport->recv.len`)
3. Since it is executing `RPC_REPLY`, the return value of the function `xs_read_stream_reply` is provided by tcp.c's `tcp_recvmsg`, indicating the size of the copied skb, and this value is > 0.
4. Since it is a normal process, it will reach the end of the function and return, which is: 
```
transport->recv.offset = 0;
transport->recv.len = 0;
return read;
```
5. The returned 'read' is the return value of the function 'xs_read_stream_reply' in the RPC_REPLY process. Since this value is > 0, the `xs_stream_data_receive` function's for loop cannot be exited.
6. After re-entering the xs_read_stream function, since transport->recv.len == 0, xs_read_stream_header will still be called.
7. Since the skb is empty, tcp.c's `tcp_recvmsg` will return a value < 0, usually -EAGAIN (-11).
8. Since the return value of xs_read_stream_header is < 0, the goto out_err statement will be executed.
9. The for loop of the xs_stream_data_receive function will be exited.
10. The next time xs_stream_data_receive is entered, the above steps will be repeated.


Now we are encountering an abnormal process:


1. For some reason, skb is contaminated, and `transport->recv.calldir` parses out an incorrect value, so `msg.msg_flags |= MSG_TRUNC`;(and also sets the value of transport->recv.len)
2. The `if (transport->recv.offset < transport->recv.len) {}` condition will be executed
3. The return value of function `xs_read_discard` is > 0, so when returning to the upstream `xs_stream_data_receive` function, the for loop cannot be exited
4. When entering xs_read_stream again, since transport->recv.len != 0, `xs_read_stream_header` will no longer be executed, which means `transport->recv.calldir` cannot be correctly set anymore
5. This is fatal for all subsequent RPC Replies because `transport->recv.calldir` cannot be correctly set anymore, so they will all go to `xs_read_discard`
6. At the same time, since transport->recv.len has not been reset to 0, this loop will become an infinite loop

Therefore, my approach is:

After reaching the point of `xs_read_discard` due to an abnormal situation, the value of `transport->recv.len` will be reset in the same way as a normal return, in order to prevent a deadlock from occurring.

The modification has been tested and passed in version 5.10.160.



---
 net/sunrpc/xprtsock.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 2e1fe6013361..91d1b992eb7f 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -695,6 +695,13 @@ xs_read_stream_reply(struct sock_xprt *transport, struct msghdr *msg, int flags)
 	return ret;
 }
 
+static void
+xs_stream_reset_recv(struct sock_xprt *transport)
+{
+	transport->recv.offset = 0;
+	transport->recv.len = 0;
+}
+
 static ssize_t
 xs_read_stream(struct sock_xprt *transport, int flags)
 {
@@ -740,8 +747,10 @@ xs_read_stream(struct sock_xprt *transport, int flags)
 		msg.msg_flags = 0;
 		ret = xs_read_discard(transport->sock, &msg, flags,
 				transport->recv.len - transport->recv.offset);
-		if (ret <= 0)
+		if (ret <= 0) {
+			xs_stream_reset_recv(transport);
 			goto out_err;
+		}
 		transport->recv.offset += ret;
 		read += ret;
 		if (transport->recv.offset != transport->recv.len)
@@ -751,8 +760,7 @@ xs_read_stream(struct sock_xprt *transport, int flags)
 		trace_xs_stream_read_request(transport);
 		transport->recv.copied = 0;
 	}
-	transport->recv.offset = 0;
-	transport->recv.len = 0;
+	xs_stream_reset_recv(transport);
 	return read;
 out_err:
 	return ret != 0 ? ret : -ESHUTDOWN;
-- 
2.51.0

Re: [PATCH] sunrpc/xs_read_stream: fix dead loop when xs_read_discard

[Trond Myklebust]

On Mon, 2026-01-26 at 14:25 +0800, jack.zhou wrote:
> From: "jack.zhou" <jacketzc@outlook.com>
> 
> Hi maintainers,
> 
> This bug was discovered during following scenario:
> 
> 1. Using NFS client with TCP mode
> 2. RPC_REPLY process
> 
> 
> Let's first review the normal process:
> 
> 1. The `xs_read_stream` function is located within the for loop of
> the `xs_stream_data_receive` function. The condition for exiting is
> that the return value ret of the `xs_read_stream` method is < 0.
> 2. When entering the function `xs_read_stream`, since `transport-
> >recv.len` == 0, `xs_read_stream_header` will be called first. Based
> on `transport->recv.calldir`, it decides whether to execute
> `RPC_CALL` or `RPC_REPLY`;(and also sets the value of `transport-
> >recv.len`)
> 3. Since it is executing `RPC_REPLY`, the return value of the
> function `xs_read_stream_reply` is provided by tcp.c's `tcp_recvmsg`,
> indicating the size of the copied skb, and this value is > 0.
> 4. Since it is a normal process, it will reach the end of the
> function and return, which is: 
> ```
> transport->recv.offset = 0;
> transport->recv.len = 0;
> return read;
> ```
> 5. The returned 'read' is the return value of the function
> 'xs_read_stream_reply' in the RPC_REPLY process. Since this value is
> > 0, the `xs_stream_data_receive` function's for loop cannot be
> exited.
> 6. After re-entering the xs_read_stream function, since transport-
> >recv.len == 0, xs_read_stream_header will still be called.
> 7. Since the skb is empty, tcp.c's `tcp_recvmsg` will return a value
> < 0, usually -EAGAIN (-11).
> 8. Since the return value of xs_read_stream_header is < 0, the goto
> out_err statement will be executed.
> 9. The for loop of the xs_stream_data_receive function will be
> exited.
> 10. The next time xs_stream_data_receive is entered, the above steps
> will be repeated.
> 
> 
> Now we are encountering an abnormal process:
> 
> 
> 1. For some reason, skb is contaminated, and `transport-
> >recv.calldir` parses out an incorrect value, so `msg.msg_flags |=
> MSG_TRUNC`;(and also sets the value of transport->recv.len)
> 2. The `if (transport->recv.offset < transport->recv.len) {}`
> condition will be executed
> 3. The return value of function `xs_read_discard` is > 0, so when
> returning to the upstream `xs_stream_data_receive` function, the for
> loop cannot be exited
> 4. When entering xs_read_stream again, since transport->recv.len !=
> 0, `xs_read_stream_header` will no longer be executed, which means
> `transport->recv.calldir` cannot be correctly set anymore
> 5. This is fatal for all subsequent RPC Replies because `transport-
> >recv.calldir` cannot be correctly set anymore, so they will all go
> to `xs_read_discard`
> 6. At the same time, since transport->recv.len has not been reset to
> 0, this loop will become an infinite loop
> 
> Therefore, my approach is:
> 
> After reaching the point of `xs_read_discard` due to an abnormal
> situation, the value of `transport->recv.len` will be reset in the
> same way as a normal return, in order to prevent a deadlock from
> occurring.
> 
> The modification has been tested and passed in version 5.10.160.
> 
> 
> 
> ---
>  net/sunrpc/xprtsock.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 2e1fe6013361..91d1b992eb7f 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -695,6 +695,13 @@ xs_read_stream_reply(struct sock_xprt
> *transport, struct msghdr *msg, int flags)
>  	return ret;
>  }
>  
> +static void
> +xs_stream_reset_recv(struct sock_xprt *transport)
> +{
> +	transport->recv.offset = 0;
> +	transport->recv.len = 0;
> +}
> +
>  static ssize_t
>  xs_read_stream(struct sock_xprt *transport, int flags)
>  {
> @@ -740,8 +747,10 @@ xs_read_stream(struct sock_xprt *transport, int
> flags)
>  		msg.msg_flags = 0;
>  		ret = xs_read_discard(transport->sock, &msg, flags,
>  				transport->recv.len - transport-
> >recv.offset);
> -		if (ret <= 0)
> +		if (ret <= 0) {
> +			xs_stream_reset_recv(transport);
>  			goto out_err;
> +		}
>  		transport->recv.offset += ret;
>  		read += ret;
>  		if (transport->recv.offset != transport->recv.len)
> @@ -751,8 +760,7 @@ xs_read_stream(struct sock_xprt *transport, int
> flags)
>  		trace_xs_stream_read_request(transport);
>  		transport->recv.copied = 0;
>  	}
> -	transport->recv.offset = 0;
> -	transport->recv.len = 0;
> +	xs_stream_reset_recv(transport);
>  	return read;
>  out_err:
>  	return ret != 0 ? ret : -ESHUTDOWN;

NACK.

If we're in the xs_read_discard() case, then that's because we're out
of receive buffer space, and so we've given up reading the remainder of
the message into that receive buffer. However we still have to read to
the end of that message before we get to the next message in the TCP
stream. The only thing that can prevent that is if someone breaks the
connection, and in that case the client already takes care of resetting
the message state as part of reconnecting.

With this change, then as soon as there is no more buffered data to
read from the socket and it returns EAGAIN, you will reset the message
parameters. The result is that the next call to xs_read_stream() will
just leave us reading random data from the middle of the message as if
it were a new message header.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com