[PATCH mptcp-next 0/3] add tcp_recv_should_stop helper

[PATCH mptcp-next 0/3] add tcp_recv_should_stop helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

While developing MPTCP splice_read, Paolo suggested adding a new helper,
tcp_recv_should_stop, to reduce code duplication. This patchset implements
it for use in both MPTCP and TLS.

Based on an earlier version and following Mat's feedback, the sock_error()
check has been moved out of tcp_recv_should_stop(). Also use this helper in
TLS and TCP-BPF.

Geliang Tang (3):
  tcp: add recv_should_stop helper
  mptcp: use recv_should_stop helper
  tls: use recv_should_stop helper

 include/net/tcp.h    | 23 ++++++++++++++++++
 net/ipv4/tcp.c       | 58 +++++++++-----------------------------------
 net/ipv4/tcp_bpf.c   | 22 ++++-------------
 net/mptcp/protocol.c | 52 +++++++++------------------------------
 net/tls/tls_sw.c     | 18 +++++---------
 5 files changed, 56 insertions(+), 117 deletions(-)

-- 
2.51.0

[PATCH mptcp-next 1/3] tcp: add recv_should_stop helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Factor out a new helper tcp_recv_should_stop() from tcp_recvmsg_locked()
and tcp_splice_read() to check whether to stop receiving.

Use this new helper in tcp_bpf_recvmsg_parser() to check whether to stop
receiving.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/tcp.h  | 23 ++++++++++++++++++
 net/ipv4/tcp.c     | 58 +++++++++-------------------------------------
 net/ipv4/tcp_bpf.c | 22 ++++--------------
 3 files changed, 39 insertions(+), 64 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index f5f6119f7716..e22f9fcb145d 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2953,4 +2953,27 @@ enum skb_drop_reason tcp_inbound_hash(struct sock *sk,
 		const void *saddr, const void *daddr,
 		int family, int dif, int sdif);
 
+static inline int tcp_recv_should_stop(struct sock *sk, long timeo)
+{
+	if (sock_flag(sk, SOCK_DONE))
+		return -ENETDOWN;
+
+	if (sk->sk_err)
+		return sk->sk_err;
+
+	if (sk->sk_shutdown & RCV_SHUTDOWN)
+		return -ESHUTDOWN;
+
+	if (sk->sk_state == TCP_CLOSE)
+		return -ENOTCONN;
+
+	if (!timeo)
+		return -EAGAIN;
+
+	if (signal_pending(current))
+		return sock_intr_errno(timeo);
+
+	return 0;
+}
+
 #endif	/* _TCP_H */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 40ad9656dc9e..f67d5cab5c15 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -815,6 +815,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 	long timeo;
 	ssize_t spliced;
 	int ret;
+	int err;
 
 	sock_rps_record_flow(sk);
 	/*
@@ -835,24 +836,14 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 		else if (!ret) {
 			if (spliced)
 				break;
-			if (sock_flag(sk, SOCK_DONE))
-				break;
 			if (sk->sk_err) {
 				ret = sock_error(sk);
 				break;
 			}
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-			if (sk->sk_state == TCP_CLOSE) {
-				/*
-				 * This occurs when user tries to read
-				 * from never connected socket.
-				 */
-				ret = -ENOTCONN;
-				break;
-			}
-			if (!timeo) {
-				ret = -EAGAIN;
+			err = tcp_recv_should_stop(sk, timeo);
+			if (err < 0) {
+				if (err != -ENETDOWN && err != -ESHUTDOWN)
+					ret = err;
 				break;
 			}
 			/* if __tcp_splice_read() got nothing while we have
@@ -864,10 +855,6 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 			ret = sk_wait_data(sk, &timeo, NULL);
 			if (ret < 0)
 				break;
-			if (signal_pending(current)) {
-				ret = sock_intr_errno(timeo);
-				break;
-			}
 			continue;
 		}
 		tss.len -= ret;
@@ -878,9 +865,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
 		release_sock(sk);
 		lock_sock(sk);
 
-		if (sk->sk_err || sk->sk_state == TCP_CLOSE ||
-		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-		    signal_pending(current))
+		if (tcp_recv_should_stop(sk, timeo))
 			break;
 	}
 
@@ -2724,39 +2709,18 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
 			break;
 
 		if (copied) {
-			if (!timeo ||
-			    sk->sk_err ||
-			    sk->sk_state == TCP_CLOSE ||
-			    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-			    signal_pending(current))
+			if (tcp_recv_should_stop(sk, timeo))
 				break;
 		} else {
-			if (sock_flag(sk, SOCK_DONE))
-				break;
-
 			if (sk->sk_err) {
 				copied = sock_error(sk);
 				break;
 			}
 
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-
-			if (sk->sk_state == TCP_CLOSE) {
-				/* This occurs when user tries to read
-				 * from never connected socket.
-				 */
-				copied = -ENOTCONN;
-				break;
-			}
-
-			if (!timeo) {
-				copied = -EAGAIN;
-				break;
-			}
-
-			if (signal_pending(current)) {
-				copied = sock_intr_errno(timeo);
+			err = tcp_recv_should_stop(sk, timeo);
+			if (err < 0) {
+				if (err != -ENETDOWN && err != -ESHUTDOWN)
+					copied = err;
 				break;
 			}
 		}
diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index a268e1595b22..314be75964fa 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -281,31 +281,19 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 	if (!copied) {
 		long timeo;
 		int data;
-
-		if (sock_flag(sk, SOCK_DONE))
-			goto out;
+		int err;
 
 		if (sk->sk_err) {
 			copied = sock_error(sk);
 			goto out;
 		}
 
-		if (sk->sk_shutdown & RCV_SHUTDOWN)
-			goto out;
-
-		if (sk->sk_state == TCP_CLOSE) {
-			copied = -ENOTCONN;
-			goto out;
-		}
-
 		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-		if (!timeo) {
-			copied = -EAGAIN;
-			goto out;
-		}
 
-		if (signal_pending(current)) {
-			copied = sock_intr_errno(timeo);
+		err = tcp_recv_should_stop(sk, timeo);
+		if (err < 0) {
+			if (err != -ENETDOWN && err != -ESHUTDOWN)
+				copied = err;
 			goto out;
 		}
 
-- 
2.51.0

[PATCH mptcp-next 2/3] mptcp: use recv_should_stop helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added tcp_recv_should_stop() helper in mptcp_recvmsg() and
mptcp_splice_read() to check whether to stop receiving.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 52 ++++++++++----------------------------------
 1 file changed, 11 insertions(+), 41 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 334fdb10fdf3..dce7fcb05fc0 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2315,11 +2315,7 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 			break;
 
 		if (copied) {
-			if (sk->sk_err ||
-			    sk->sk_state == TCP_CLOSE ||
-			    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-			    !timeo ||
-			    signal_pending(current))
+			if (tcp_recv_should_stop(sk, timeo))
 				break;
 		} else {
 			if (sk->sk_err) {
@@ -2327,21 +2323,10 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 				break;
 			}
 
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-
-			if (sk->sk_state == TCP_CLOSE) {
-				copied = -ENOTCONN;
-				break;
-			}
-
-			if (!timeo) {
-				copied = -EAGAIN;
-				break;
-			}
-
-			if (signal_pending(current)) {
-				copied = sock_intr_errno(timeo);
+			err = tcp_recv_should_stop(sk, timeo);
+			if (err < 0) {
+				if (err != -ESHUTDOWN)
+					copied = err;
 				break;
 			}
 		}
@@ -4438,6 +4423,7 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 	ssize_t spliced = 0;
 	int ret = 0;
 	long timeo;
+	int err;
 
 	/*
 	 * We can't seek on a socket input
@@ -4457,24 +4443,14 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 		} else if (!ret) {
 			if (spliced)
 				break;
-			if (sock_flag(sk, SOCK_DONE))
-				break;
 			if (sk->sk_err) {
 				ret = sock_error(sk);
 				break;
 			}
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-			if (sk->sk_state == TCP_CLOSE) {
-				/*
-				 * This occurs when user tries to read
-				 * from never connected socket.
-				 */
-				ret = -ENOTCONN;
-				break;
-			}
-			if (!timeo) {
-				ret = -EAGAIN;
+			err = tcp_recv_should_stop(sk, timeo);
+			if (err < 0) {
+				if (err != -ENETDOWN && err != -ESHUTDOWN)
+					ret = err;
 				break;
 			}
 			/* if __mptcp_splice_read() got nothing while we have
@@ -4486,10 +4462,6 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 			ret = sk_wait_data(sk, &timeo, NULL);
 			if (ret < 0)
 				break;
-			if (signal_pending(current)) {
-				ret = sock_intr_errno(timeo);
-				break;
-			}
 			continue;
 		}
 		tss.len -= ret;
@@ -4500,9 +4472,7 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
 		release_sock(sk);
 		lock_sock(sk);
 
-		if (sk->sk_err || sk->sk_state == TCP_CLOSE ||
-		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-		    signal_pending(current))
+		if (tcp_recv_should_stop(sk, timeo))
 			break;
 	}
 
-- 
2.51.0

[PATCH mptcp-next 3/3] tls: use recv_should_stop helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Use the newly added tcp_recv_should_stop() helper in tls_rx_rec_wait() to
check whether to stop receiving.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/tls/tls_sw.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 9937d4c810f2..26517c317ee7 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1377,14 +1377,12 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 				break;
 		}
 
-		if (sk->sk_shutdown & RCV_SHUTDOWN)
-			return 0;
-
-		if (sock_flag(sk, SOCK_DONE))
-			return 0;
-
-		if (!timeo)
-			return -EAGAIN;
+		ret = tcp_recv_should_stop(sk, timeo);
+		if (ret < 0) {
+			if (ret == -ENETDOWN || ret == -ESHUTDOWN)
+				ret = 0;
+			return ret;
+		}
 
 		released = true;
 		add_wait_queue(sk_sleep(sk), &wait);
@@ -1395,10 +1393,6 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
 				    &wait);
 		sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
 		remove_wait_queue(sk_sleep(sk), &wait);
-
-		/* Handle signals */
-		if (signal_pending(current))
-			return sock_intr_errno(timeo);
 	}
 
 	if (unlikely(!tls_strp_msg_load(&ctx->strp, released)))
-- 
2.51.0

Re: [PATCH mptcp-next 0/3] add tcp_recv_should_stop helper

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/20188543480

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/1b24ef0e3abf
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1032825


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)