diff mbox series

[mptcp-next] net/mptcp: don't overwrite sock_ops in mptcp_is_tcpsk()

Message ID a2581b975c0c5bec417b2dd5e0b014acd7770e14.1698764762.git.dcaratti@redhat.com (mailing list archive)
State Superseded, archived
Commit 65304cb7d742e7dec2ebc73a533b0162fd7285b5
Headers show
Series [mptcp-next] net/mptcp: don't overwrite sock_ops in mptcp_is_tcpsk() | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 65 lines checked
matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ success Success! ✅

Commit Message

Davide Caratti Oct. 31, 2023, 3:09 p.m. UTC 
Eric suggests:

 > The fact that mptcp_is_tcpsk() was able to write over sock->ops was a
 > bit strange to me.
 > mptcp_is_tcpsk() should answer a question, with a read-only argument.

re-factor code to avoid overwriting sock_ops inside that function. Also,
change the helper name to reflect the semantic, and to disambiguate from
its dual sk_is_mptcp().

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/432
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/mptcp/protocol.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

Comments

MPTCP CI Oct. 31, 2023, 3:31 p.m. UTC | #1 
Hi Davide,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/a2581b975c0c5bec417b2dd5e0b014acd7770e14.1698764762.git.dcaratti@redhat.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/6708756766

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ece97d6ac360

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
MPTCP CI Oct. 31, 2023, 5:26 p.m. UTC | #2 
Hi Davide,

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! ✅:
  - Task: https://cirrus-ci.com/task/5260796763045888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5260796763045888/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4979321786335232
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4979321786335232/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6386696669888512
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6386696669888512/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6105221693177856
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6105221693177856/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d6687043f997


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-debug

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 (Tessares)
Matthieu Baerts Oct. 31, 2023, 5:33 p.m. UTC | #3 
Hi Davide,

On 31/10/2023 16:31, MPTCP CI wrote:
> Thank you for your modifications, that's great!
> 
> But sadly, our CI spotted some issues with it when trying to build it.
> 
> You can find more details there:
> 
>   https://patchwork.kernel.org/project/mptcp/patch/a2581b975c0c5bec417b2dd5e0b014acd7770e14.1698764762.git.dcaratti@redhat.com/
>   https://github.com/multipath-tcp/mptcp_net-next/actions/runs/6708756766
> 
> Status: failure
> Initiator: MPTCPimporter
> Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ece97d6ac360

As discussed at the weekly meeting, this issue was due to one upstream.

A fix for that has been added to our tree, and the CI is no longer
complaining about your patch now!

Cheers,
Matt
Mat Martineau Nov. 2, 2023, 12:51 a.m. UTC | #4 
On Tue, 31 Oct 2023, Davide Caratti wrote:

> Eric suggests:
>
> > The fact that mptcp_is_tcpsk() was able to write over sock->ops was a
> > bit strange to me.
> > mptcp_is_tcpsk() should answer a question, with a read-only argument.
>
> re-factor code to avoid overwriting sock_ops inside that function. Also,
> change the helper name to reflect the semantic, and to disambiguate from
> its dual sk_is_mptcp().
>
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/432
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/protocol.c | 38 ++++++++++++++++++--------------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0ad507ac6bc7..9a9f8acd979e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -55,28 +55,15 @@ u64 mptcp_wnd_end(const struct mptcp_sock *msk)
> 	return READ_ONCE(msk->wnd_end);
> }
>
> -static bool mptcp_is_tcpsk(struct sock *sk)
> +static const struct proto_ops *mptcp_get_fallback_tcp_ops(const struct sock *sk)
> {
> -	struct socket *sock = sk->sk_socket;
> -
> -	if (unlikely(sk->sk_prot == &tcp_prot)) {
> -		/* we are being invoked after mptcp_accept() has
> -		 * accepted a non-mp-capable flow: sk is a tcp_sk,
> -		 * not an mptcp one.
> -		 *
> -		 * Hand the socket over to tcp so all further socket ops
> -		 * bypass mptcp.
> -		 */
> -		WRITE_ONCE(sock->ops, &inet_stream_ops);
> -		return true;
> +	if (unlikely(sk->sk_prot == &tcp_prot))
> +		return &inet_stream_ops;
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -	} else if (unlikely(sk->sk_prot == &tcpv6_prot)) {
> -		WRITE_ONCE(sock->ops, &inet6_stream_ops);
> -		return true;
> +	else if (unlikely(sk->sk_prot == &tcpv6_prot))
> +		return &inet6_stream_ops;
> #endif
> -	}
> -
> -	return false;
> +	return NULL;
> }
>
> static int __mptcp_socket_create(struct mptcp_sock *msk)
> @@ -3832,6 +3819,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 			       int flags, bool kern)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> +	const struct proto_ops *fallback_ops;
> 	struct sock *ssk, *newsk;
> 	int err;
>
> @@ -3851,7 +3839,8 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 	lock_sock(newsk);
>
> 	__inet_accept(sock, newsock, newsk);
> -	if (!mptcp_is_tcpsk(newsock->sk)) {
> +	fallback_ops = mptcp_get_fallback_tcp_ops(newsock->sk);
> +	if (!fallback_ops) {
> 		struct mptcp_sock *msk = mptcp_sk(newsk);
> 		struct mptcp_subflow_context *subflow;
>
> @@ -3877,6 +3866,15 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 			if (unlikely(list_is_singular(&msk->conn_list)))
> 				inet_sk_state_store(newsk, TCP_CLOSE);
> 		}
> +	} else {
> +		/* we are being invoked after mptcp_accept() has
> +		 * accepted a non-mp-capable flow: sk is a tcp_sk,
> +		 * not an mptcp one.
> +		 *
> +		 * Hand the socket over to tcp so all further socket ops
> +		 * bypass mptcp.
> +		 */
> +		WRITE_ONCE(newsock->sk->sk_socket->ops, fallback_ops);

Hi Davide -

From your comment in the meeting about using a different helper to 
determine if it's a MPTCP or TCP sk, I think the idea is to get rid of the 
helper function (rather than rename) and then include the ipv4/ipv6 logic 
in this block of code? That approach sounds good to me.

If that doesn't work out, another option is to rename the helper function 
to something that meets Eric's criteria (avoiding "mptcp_is_xxx()" or 
"mptcp_get_xxx()" names).

- Mat

> 	}
> 	release_sock(newsk);
>
> -- 
> 2.41.0
>
>
>
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0ad507ac6bc7..9a9f8acd979e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -55,28 +55,15 @@  u64 mptcp_wnd_end(const struct mptcp_sock *msk)
 	return READ_ONCE(msk->wnd_end);
 }
 
-static bool mptcp_is_tcpsk(struct sock *sk)
+static const struct proto_ops *mptcp_get_fallback_tcp_ops(const struct sock *sk)
 {
-	struct socket *sock = sk->sk_socket;
-
-	if (unlikely(sk->sk_prot == &tcp_prot)) {
-		/* we are being invoked after mptcp_accept() has
-		 * accepted a non-mp-capable flow: sk is a tcp_sk,
-		 * not an mptcp one.
-		 *
-		 * Hand the socket over to tcp so all further socket ops
-		 * bypass mptcp.
-		 */
-		WRITE_ONCE(sock->ops, &inet_stream_ops);
-		return true;
+	if (unlikely(sk->sk_prot == &tcp_prot))
+		return &inet_stream_ops;
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-	} else if (unlikely(sk->sk_prot == &tcpv6_prot)) {
-		WRITE_ONCE(sock->ops, &inet6_stream_ops);
-		return true;
+	else if (unlikely(sk->sk_prot == &tcpv6_prot))
+		return &inet6_stream_ops;
 #endif
-	}
-
-	return false;
+	return NULL;
 }
 
 static int __mptcp_socket_create(struct mptcp_sock *msk)
@@ -3832,6 +3819,7 @@  static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 			       int flags, bool kern)
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
+	const struct proto_ops *fallback_ops;
 	struct sock *ssk, *newsk;
 	int err;
 
@@ -3851,7 +3839,8 @@  static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 	lock_sock(newsk);
 
 	__inet_accept(sock, newsock, newsk);
-	if (!mptcp_is_tcpsk(newsock->sk)) {
+	fallback_ops = mptcp_get_fallback_tcp_ops(newsock->sk);
+	if (!fallback_ops) {
 		struct mptcp_sock *msk = mptcp_sk(newsk);
 		struct mptcp_subflow_context *subflow;
 
@@ -3877,6 +3866,15 @@  static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 			if (unlikely(list_is_singular(&msk->conn_list)))
 				inet_sk_state_store(newsk, TCP_CLOSE);
 		}
+	} else {
+		/* we are being invoked after mptcp_accept() has
+		 * accepted a non-mp-capable flow: sk is a tcp_sk,
+		 * not an mptcp one.
+		 *
+		 * Hand the socket over to tcp so all further socket ops
+		 * bypass mptcp.
+		 */
+		WRITE_ONCE(newsock->sk->sk_socket->ops, fallback_ops);
 	}
 	release_sock(newsk);