[PATCH mptcp-next 0/4] BPF 'force to MPTCP'

[PATCH mptcp-next 0/4] BPF 'force to MPTCP'

[Geliang Tang]

This series depends on the two netns patches, should be inserted before
the BPF scheduler series:
  selftests/bpf: use random netns name for mptcp
  selftests/bpf: add two mptcp netns helpers
  bpf: Add bpf_mptcpify helper
  selftests/bpf: Test bpf_mptcpify helper
  selftests/bpf: Add mptcpify selftest
  mptcp: refactor push_pending logic

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79

Geliang Tang (4):
  Squash to "selftests/bpf: add two mptcp netns helpers"
  bpf: Add bpf_mptcpify helper
  selftests/bpf: Test bpf_mptcpify helper
  selftests/bpf: Add mptcpify selftest

 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |   7 +
 kernel/bpf/verifier.c                         |   1 +
 kernel/trace/bpf_trace.c                      |   2 +
 net/core/filter.c                             |  21 +++
 scripts/bpf_doc.py                            |   1 +
 tools/include/uapi/linux/bpf.h                |   7 +
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |   1 +
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 123 +++++++++++++++---
 tools/testing/selftests/bpf/progs/mptcpify.c  |  32 +++++
 10 files changed, 176 insertions(+), 20 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcpify.c

-- 
2.35.3

[PATCH mptcp-next 1/4] Squash to "selftests/bpf: add two mptcp netns helpers"

[Geliang Tang]

Move the helpers to the beginning.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 40 +++++++++----------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index a968641cc94a..e430bebebcf0 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -26,6 +26,26 @@ struct mptcp_storage {
 	char ca_name[TCP_CA_NAME_MAX];
 };
 
+static struct nstoken *create_netns(void)
+{
+	srand(time(NULL));
+	snprintf(NS_TEST, sizeof(NS_TEST), "mptcp_ns_%d", rand());
+	SYS(fail, "ip netns add %s", NS_TEST);
+	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
+
+	return open_netns(NS_TEST);
+fail:
+	return NULL;
+}
+
+static void cleanup_netns(struct nstoken *nstoken)
+{
+	if (nstoken)
+		close_netns(nstoken);
+
+	SYS_NOFAIL("ip netns del %s &> /dev/null", NS_TEST);
+}
+
 static int verify_tsk(int map_fd, int client_fd)
 {
 	int err, cfd = client_fd;
@@ -142,26 +162,6 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 	return err;
 }
 
-static struct nstoken *create_netns(void)
-{
-	srand(time(NULL));
-	snprintf(NS_TEST, sizeof(NS_TEST), "mptcp_ns_%d", rand());
-	SYS(fail, "ip netns add %s", NS_TEST);
-	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
-
-	return open_netns(NS_TEST);
-fail:
-	return NULL;
-}
-
-static void cleanup_netns(struct nstoken *nstoken)
-{
-	if (nstoken)
-		close_netns(nstoken);
-
-	SYS_NOFAIL("ip netns del %s &> /dev/null", NS_TEST);
-}
-
 static void test_base(void)
 {
 	struct nstoken *nstoken = NULL;
-- 
2.35.3

[PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Geliang Tang]

This patch implements a new struct bpf_func_proto bpf_mptcpify_proto. And
define a new helper bpf_mptcpify() to mptcpify a TCP socket dynamically as
an MPTCP one.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       |  7 +++++++
 kernel/bpf/verifier.c          |  1 +
 kernel/trace/bpf_trace.c       |  2 ++
 net/core/filter.c              | 21 +++++++++++++++++++++
 scripts/bpf_doc.py             |  1 +
 tools/include/uapi/linux/bpf.h |  7 +++++++
 7 files changed, 40 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f58895830ada..424056fd5335 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2883,6 +2883,7 @@ extern const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto;
 extern const struct bpf_func_proto bpf_skc_to_udp6_sock_proto;
 extern const struct bpf_func_proto bpf_skc_to_unix_sock_proto;
 extern const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto;
+extern const struct bpf_func_proto bpf_mptcpify_proto;
 extern const struct bpf_func_proto bpf_copy_from_user_proto;
 extern const struct bpf_func_proto bpf_snprintf_btf_proto;
 extern const struct bpf_func_proto bpf_snprintf_proto;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6961a7b70028..ef175ea8ee4a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5569,6 +5569,12 @@ union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * struct sock *bpf_mptcpify(void *sk)
+ *	Description
+ *		Dynamically mptcpify a TCP socket *sk* pointer as an MPTCP one.
+ *	Return
+ *		*sk* if it's valid, or **NULL** otherwise.
  */
 #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
 	FN(unspec, 0, ##ctx)				\
@@ -5783,6 +5789,7 @@ union bpf_attr {
 	FN(user_ringbuf_drain, 209, ##ctx)		\
 	FN(cgrp_storage_get, 210, ##ctx)		\
 	FN(cgrp_storage_delete, 211, ##ctx)		\
+	FN(mptcpify, 212, ##ctx)			\
 	/* */
 
 /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b54193de762b..fa8d656d25cb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -523,6 +523,7 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id)
 		func_id == BPF_FUNC_skc_to_tcp6_sock ||
 		func_id == BPF_FUNC_skc_to_udp6_sock ||
 		func_id == BPF_FUNC_skc_to_mptcp_sock ||
+		func_id == BPF_FUNC_mptcpify ||
 		func_id == BPF_FUNC_skc_to_tcp_timewait_sock ||
 		func_id == BPF_FUNC_skc_to_tcp_request_sock;
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 03b7f6b8e4f0..26c170a456c5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1926,6 +1926,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_skc_to_unix_sock_proto;
 	case BPF_FUNC_skc_to_mptcp_sock:
 		return &bpf_skc_to_mptcp_sock_proto;
+	case BPF_FUNC_mptcpify:
+		return &bpf_mptcpify_proto;
 	case BPF_FUNC_sk_storage_get:
 		return &bpf_sk_storage_get_tracing_proto;
 	case BPF_FUNC_sk_storage_delete:
diff --git a/net/core/filter.c b/net/core/filter.c
index 968139f4a1ac..e439f8b5f203 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
 	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
 };
 
+BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
+{
+	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
+		sk->sk_protocol = IPPROTO_MPTCP;
+		return (unsigned long)sk;
+	}
+
+	return (unsigned long)NULL;
+}
+
+const struct bpf_func_proto bpf_mptcpify_proto = {
+	.func		= bpf_mptcpify,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_BTF_ID_OR_NULL,
+	.arg1_type	= ARG_PTR_TO_BTF_ID_SOCK_COMMON,
+	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_SOCK],
+};
+
 BPF_CALL_1(bpf_sock_from_file, struct file *, file)
 {
 	return (unsigned long)sock_from_file(file);
@@ -11632,6 +11650,9 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id)
 	case BPF_FUNC_skc_to_mptcp_sock:
 		func = &bpf_skc_to_mptcp_sock_proto;
 		break;
+	case BPF_FUNC_mptcpify:
+		func = &bpf_mptcpify_proto;
+		break;
 	case BPF_FUNC_ktime_get_coarse_ns:
 		return &bpf_ktime_get_coarse_ns_proto;
 	default:
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index eaae2ce78381..7a20ab9c6513 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -751,6 +751,7 @@ class PrinterHelpers(Printer):
             'struct file',
             'struct bpf_timer',
             'struct mptcp_sock',
+            'struct sock',
             'struct bpf_dynptr',
             'const struct bpf_dynptr',
             'struct iphdr',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6961a7b70028..ef175ea8ee4a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5569,6 +5569,12 @@ union bpf_attr {
  *		0 on success.
  *
  *		**-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * struct sock *bpf_mptcpify(void *sk)
+ *	Description
+ *		Dynamically mptcpify a TCP socket *sk* pointer as an MPTCP one.
+ *	Return
+ *		*sk* if it's valid, or **NULL** otherwise.
  */
 #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
 	FN(unspec, 0, ##ctx)				\
@@ -5783,6 +5789,7 @@ union bpf_attr {
 	FN(user_ringbuf_drain, 209, ##ctx)		\
 	FN(cgrp_storage_get, 210, ##ctx)		\
 	FN(cgrp_storage_delete, 211, ##ctx)		\
+	FN(mptcpify, 212, ##ctx)			\
 	/* */
 
 /* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
-- 
2.35.3

[PATCH mptcp-next 3/4] selftests/bpf: Test bpf_mptcpify helper

[Geliang Tang]

This patch tests the new helper bpf_mptcpify(). Store the new protocol
value after invoking the helper to a local BSS variable.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  1 +
 tools/testing/selftests/bpf/progs/mptcpify.c  | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcpify.c

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index 72c618037386..f846d5d62529 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -38,6 +38,7 @@ struct sock {
 #define sk_state		__sk_common.skc_state
 	unsigned long		sk_pacing_rate;
 	__u32			sk_pacing_status; /* see enum sk_pacing */
+	__u16			sk_protocol;
 } __attribute__((preserve_access_index));
 
 struct inet_sock {
diff --git a/tools/testing/selftests/bpf/progs/mptcpify.c b/tools/testing/selftests/bpf/progs/mptcpify.c
new file mode 100644
index 000000000000..0eb167dcfa22
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcpify.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023, SUSE. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_tcp_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+__u16 protocol = 0;
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+	int op = (int)ctx->op;
+	struct bpf_sock *bsk;
+	struct sock *sk;
+
+	if (op != BPF_SOCK_OPS_TCP_CONNECT_CB &&
+	    op != BPF_SOCK_OPS_TCP_LISTEN_CB)
+		return 1;
+
+	bsk = ctx->sk;
+	if (!bsk)
+		return 1;
+
+	sk = bpf_mptcpify(bsk);
+	if (!sk)
+		return 1;
+
+	protocol = sk->sk_protocol;
+	return 0;
+}
-- 
2.35.3

[PATCH mptcp-next 4/4] selftests/bpf: Add mptcpify selftest

[Geliang Tang]

This patch extends the MPTCP test base, add a selftest test_mptcpify()
for the mptcpify case.

Open and load the mptcpify test prog to mptcpify the TCP sockets
dynamically, then use start_server() and connect_to_fd() to create a
TCP socket, but actually what's created is an MPTCP socket, which can
be verified through the stored local BSS variable.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index e430bebebcf0..4057ff07572d 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -6,6 +6,7 @@
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
 #include "mptcp_sock.skel.h"
+#include "mptcpify.skel.h"
 #include "mptcp_bpf_first.skel.h"
 #include "mptcp_bpf_bkup.skel.h"
 #include "mptcp_bpf_rr.skel.h"
@@ -13,6 +14,9 @@
 
 char NS_TEST[32];
 
+#ifndef IPPROTO_MPTCP
+#define IPPROTO_MPTCP 262
+#endif
 #ifndef TCP_CA_NAME_MAX
 #define TCP_CA_NAME_MAX	16
 #endif
@@ -200,6 +204,83 @@ static void test_base(void)
 	close(cgroup_fd);
 }
 
+static void send_byte(int fd)
+{
+	char b = 0x55;
+
+	ASSERT_EQ(write(fd, &b, sizeof(b)), 1, "send single byte");
+}
+
+static int run_mptcpify(int cgroup_fd)
+{
+	int server_fd, client_fd, prog_fd, err = 0;
+	struct mptcpify *mptcpify_skel;
+
+	mptcpify_skel = mptcpify__open_and_load();
+	if (!ASSERT_OK_PTR(mptcpify_skel, "skel_open_load"))
+		return -EIO;
+
+	err = mptcpify__attach(mptcpify_skel);
+	if (!ASSERT_OK(err, "skel_attach"))
+		goto out;
+
+	prog_fd = bpf_program__fd(mptcpify_skel->progs._sockops);
+	if (!ASSERT_GE(prog_fd, 0, "bpf_program__fd")) {
+		err = -EIO;
+		goto out;
+	}
+
+	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS,
+			      BPF_CGROUP_INET_SOCK_CREATE);
+	if (!ASSERT_OK(err, "bpf_prog_attach"))
+		goto out;
+
+	/* without MPTCP */
+	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
+	if (!ASSERT_GE(server_fd, 0, "start_server")) {
+		err = -EIO;
+		goto out;
+	}
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_GE(client_fd, 0, "connect to fd")) {
+		err = -EIO;
+		goto close_server;
+	}
+
+	send_byte(client_fd);
+	if (!ASSERT_EQ(mptcpify_skel->bss->protocol,
+		       IPPROTO_MPTCP, "unexpected protocol"))
+		err++;
+
+	close(client_fd);
+close_server:
+	close(server_fd);
+out:
+	mptcpify__destroy(mptcpify_skel);
+	return err;
+}
+
+static void test_mptcpify(void)
+{
+	struct nstoken *nstoken = NULL;
+	int cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/mptcpify");
+	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
+		return;
+
+	nstoken = create_netns();
+	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
+		goto fail;
+
+	ASSERT_OK(run_mptcpify(cgroup_fd), "run_mptcpify");
+
+fail:
+	cleanup_netns(nstoken);
+	close(cgroup_fd);
+}
+
 static const unsigned int total_bytes = 10 * 1024 * 1024;
 static int stop, duration;
 
@@ -459,6 +540,8 @@ void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
 		test_base();
+	if (test__start_subtest("mptcpify"))
+		test_mptcpify();
 	if (test__start_subtest("first"))
 		test_first();
 	if (test__start_subtest("bkup"))
-- 
2.35.3

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Matthieu Baerts]

Hi Geliang,

On 29/06/2023 04:12, Geliang Tang wrote:
> This patch implements a new struct bpf_func_proto bpf_mptcpify_proto. And
> define a new helper bpf_mptcpify() to mptcpify a TCP socket dynamically as
> an MPTCP one.

Nice feature, thank you for working on that!

Is it linked to what Nicolas Rybowski looked at a few years ago? I think
he put info in a Github ticket. Was there not an issue with this
technique? Did you see it and fix it?

I didn't look at the patchset in detail but I have one question, please
see below:

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 968139f4a1ac..e439f8b5f203 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
>  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
>  };
>  
> +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
> +{
> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
> +		sk->sk_protocol = IPPROTO_MPTCP;
> +		return (unsigned long)sk;
> +	}

How do we ensure such modifications are done at the right moment? I
mean: we can only change the protocol ID in very few places, before even
creating the socket (__sock_create()?). If we change it after, we will
break stuff: tcp ops, security labels, etc.

I thought it was not possible to hook at the right place when Nicolas
looked at that and/or ensure the restriction was done but I might be
mistaken and the situation has probably changed.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: selftests/bpf: Add mptcpify selftest: Tests Results

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

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

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

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/4831537535909888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4831537535909888/summary/summary.txt

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


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)

Re: selftests/bpf: Add mptcpify selftest: Tests Results

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

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

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

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/4831537535909888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4831537535909888/summary/summary.txt

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


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)

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Geliang Tang]

On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/06/2023 04:12, Geliang Tang wrote:
> > This patch implements a new struct bpf_func_proto bpf_mptcpify_proto. And
> > define a new helper bpf_mptcpify() to mptcpify a TCP socket dynamically as
> > an MPTCP one.
> 
> Nice feature, thank you for working on that!
> 
> Is it linked to what Nicolas Rybowski looked at a few years ago? I think
> he put info in a Github ticket. Was there not an issue with this
> technique? Did you see it and fix it?

Yes, this series addresses issue #79 "allow 'force to MPTCP' mode: BPF".
Nicolas looked at it before. I assigned it to myself a few days ago.

> 
> I didn't look at the patchset in detail but I have one question, please
> see below:
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 968139f4a1ac..e439f8b5f203 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
> >  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
> >  };
> >  
> > +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
> > +{
> > +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
> > +		sk->sk_protocol = IPPROTO_MPTCP;
> > +		return (unsigned long)sk;
> > +	}
> 
> How do we ensure such modifications are done at the right moment? I
> mean: we can only change the protocol ID in very few places, before even
> creating the socket (__sock_create()?). If we change it after, we will
> break stuff: tcp ops, security labels, etc.

This will be hooked in BPF_CGROUP_RUN_PROG_INET_SOCK() in inet_create().
See v2, a better version.

> 
> I thought it was not possible to hook at the right place when Nicolas
> looked at that and/or ensure the restriction was done but I might be
> mistaken and the situation has probably changed.

The selftest works, proving its ability to convert TCP to MPTCP.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Geliang Tang]

On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/06/2023 04:12, Geliang Tang wrote:
> > This patch implements a new struct bpf_func_proto bpf_mptcpify_proto. And
> > define a new helper bpf_mptcpify() to mptcpify a TCP socket dynamically as
> > an MPTCP one.
> 
> Nice feature, thank you for working on that!
> 
> Is it linked to what Nicolas Rybowski looked at a few years ago? I think
> he put info in a Github ticket. Was there not an issue with this
> technique? Did you see it and fix it?
> 
> I didn't look at the patchset in detail but I have one question, please
> see below:
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 968139f4a1ac..e439f8b5f203 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
> >  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
> >  };
> >  
> > +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
> > +{
> > +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
> > +		sk->sk_protocol = IPPROTO_MPTCP;
> > +		return (unsigned long)sk;
> > +	}
> 
> How do we ensure such modifications are done at the right moment? I
> mean: we can only change the protocol ID in very few places, before even
> creating the socket (__sock_create()?). If we change it after, we will
> break stuff: tcp ops, security labels, etc.

You're right, Matt, we need to do the modifications at the very beginning
of sys_socket(). In v3, a new wrapper socket_create() is added, it's the
right place to do the modifications.

The v3 works well now. We can get three MP_CAPABLEs in the log.

Thanks,
-Geliang

> 
> I thought it was not possible to hook at the right place when Nicolas
> looked at that and/or ensure the restriction was done but I might be
> mistaken and the situation has probably changed.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Matthieu Baerts]

Hi Geliang,

On 03/07/2023 08:54, Geliang Tang wrote:
> On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
>> On 29/06/2023 04:12, Geliang Tang wrote:
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 968139f4a1ac..e439f8b5f203 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
>>>  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
>>>  };
>>>  
>>> +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
>>> +{
>>> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
>>> +		sk->sk_protocol = IPPROTO_MPTCP;
>>> +		return (unsigned long)sk;
>>> +	}
>>
>> How do we ensure such modifications are done at the right moment? I
>> mean: we can only change the protocol ID in very few places, before even
>> creating the socket (__sock_create()?). If we change it after, we will
>> break stuff: tcp ops, security labels, etc.
> 
> You're right, Matt, we need to do the modifications at the very beginning
> of sys_socket(). In v3, a new wrapper socket_create() is added, it's the
> right place to do the modifications.

Thank you for this v3 and for your replies: it is clearer with your replies!

I hope such wrapper can be accepted upstream :)

It is then important to fail if bpf_mptcpify() is not executed at the
beginning of this new "socket_create()" wrapper. Will it be the case?

I guess the verifier will make sure bpf_mptcpify() is only called with a
function having "struct socket_args" as first argument so "by accident",
it will only be bpf_mptcpify(), right? Maybe there is a way to clearly
mark that bpf_mptcpify() cannot be called elsewhere? Or make sure it
fails if the socket has already been created? (maybe that's something we
need to ask to BPF devs)

One last thing: in your v3, I think you restrict the conversion to IPv4
only sockets, no?

  (args->family == AF_INET || args->family == AF_INET6)

(and I guess you saw that the kernel test robot complained about one of
the patch because the new wrapper is not declared in the "include" dir)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 0/4] BPF 'force to MPTCP'

[Paolo Abeni]

Hi Geliang,

On Thu, 2023-06-29 at 10:12 +0800, Geliang Tang wrote:
> This series depends on the two netns patches, should be inserted before
> the BPF scheduler series:
>   selftests/bpf: use random netns name for mptcp
>   selftests/bpf: add two mptcp netns helpers
>   bpf: Add bpf_mptcpify helper
>   selftests/bpf: Test bpf_mptcpify helper
>   selftests/bpf: Add mptcpify selftest
>   mptcp: refactor push_pending logic
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79

Again sorry for the possibly late feedback.

Before investing more time on this topic, are there any issues with the
current tools available (mainly: 'mptcpize') that need to be addressed?

Otherwise we would possibly better off not adding more code to the
kernel for such goal.

/P

Re: [PATCH mptcp-next 0/4] BPF 'force to MPTCP'

[Matthieu Baerts]

Hi Paolo,

On 03/07/2023 17:18, Paolo Abeni wrote:
> Hi Geliang,
> 
> On Thu, 2023-06-29 at 10:12 +0800, Geliang Tang wrote:
>> This series depends on the two netns patches, should be inserted before
>> the BPF scheduler series:
>>   selftests/bpf: use random netns name for mptcp
>>   selftests/bpf: add two mptcp netns helpers
>>   bpf: Add bpf_mptcpify helper
>>   selftests/bpf: Test bpf_mptcpify helper
>>   selftests/bpf: Add mptcpify selftest
>>   mptcp: refactor push_pending logic
>>
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79
> 
> Again sorry for the possibly late feedback.
> 
> Before investing more time on this topic, are there any issues with the
> current tools available (mainly: 'mptcpize') that need to be addressed?

I don't have specific use cases for having bpf_mptcpize but I know the
"original" mptcpize doesn't not work in some cases:

- if the application is not using libc (e.g. GoLang apps)
- in some envs, it might not be easy to set env vars, e.g. on Android
- it needs to be launched with all apps that wants MPTCP: we can have
more control from BPF to enable MPTCP only for some apps or all the ones
of a netns or a cgroup, etc.
- it is not in BPF, we cannot talk about it at netdev conf (:-D)

Even if I agree mptcpize can help a lot, having bpf_mptcpize seems to
help in some other cases.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Geliang Tang]

On Mon, Jul 03, 2023 at 02:58:33PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/07/2023 08:54, Geliang Tang wrote:
> > On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
> >> On 29/06/2023 04:12, Geliang Tang wrote:
> >>> diff --git a/net/core/filter.c b/net/core/filter.c
> >>> index 968139f4a1ac..e439f8b5f203 100644
> >>> --- a/net/core/filter.c
> >>> +++ b/net/core/filter.c
> >>> @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
> >>>  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
> >>>  };
> >>>  
> >>> +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
> >>> +{
> >>> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
> >>> +		sk->sk_protocol = IPPROTO_MPTCP;
> >>> +		return (unsigned long)sk;
> >>> +	}
> >>
> >> How do we ensure such modifications are done at the right moment? I
> >> mean: we can only change the protocol ID in very few places, before even
> >> creating the socket (__sock_create()?). If we change it after, we will
> >> break stuff: tcp ops, security labels, etc.
> > 
> > You're right, Matt, we need to do the modifications at the very beginning
> > of sys_socket(). In v3, a new wrapper socket_create() is added, it's the
> > right place to do the modifications.
> 
> Thank you for this v3 and for your replies: it is clearer with your replies!
> 
> I hope such wrapper can be accepted upstream :)

Hi Matt,

I just sent a v4, adding hooks in __sock_create(), instead of adding a
wrapper. I think v4 is much more acceptable by upstream.

> 
> It is then important to fail if bpf_mptcpify() is not executed at the
> beginning of this new "socket_create()" wrapper. Will it be the case?
> 
> I guess the verifier will make sure bpf_mptcpify() is only called with a
> function having "struct socket_args" as first argument so "by accident",
> it will only be bpf_mptcpify(), right? Maybe there is a way to clearly
> mark that bpf_mptcpify() cannot be called elsewhere? Or make sure it
> fails if the socket has already been created? (maybe that's something we
> need to ask to BPF devs)
> 
> One last thing: in your v3, I think you restrict the conversion to IPv4
> only sockets, no?
> 
>   (args->family == AF_INET || args->family == AF_INET6)

I updated this in v4.

Thanks,
-Geliang

> 
> (and I guess you saw that the kernel test robot complained about one of
> the patch because the new wrapper is not declared in the "include" dir)
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Matthieu Baerts]

Hi Geliang,

On 05/07/2023 10:15, Geliang Tang wrote:
> On Mon, Jul 03, 2023 at 02:58:33PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 03/07/2023 08:54, Geliang Tang wrote:
>>> On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
>>>> On 29/06/2023 04:12, Geliang Tang wrote:
>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>> index 968139f4a1ac..e439f8b5f203 100644
>>>>> --- a/net/core/filter.c
>>>>> +++ b/net/core/filter.c
>>>>> @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
>>>>>  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
>>>>>  };
>>>>>  
>>>>> +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
>>>>> +{
>>>>> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
>>>>> +		sk->sk_protocol = IPPROTO_MPTCP;
>>>>> +		return (unsigned long)sk;
>>>>> +	}
>>>>
>>>> How do we ensure such modifications are done at the right moment? I
>>>> mean: we can only change the protocol ID in very few places, before even
>>>> creating the socket (__sock_create()?). If we change it after, we will
>>>> break stuff: tcp ops, security labels, etc.
>>>
>>> You're right, Matt, we need to do the modifications at the very beginning
>>> of sys_socket(). In v3, a new wrapper socket_create() is added, it's the
>>> right place to do the modifications.
>>
>> Thank you for this v3 and for your replies: it is clearer with your replies!
>>
>> I hope such wrapper can be accepted upstream :)
> 
> Hi Matt,
> 
> I just sent a v4, adding hooks in __sock_create(), instead of adding a
> wrapper. I think v4 is much more acceptable by upstream.

Thank you for your reply. Yes, I think the v4 is more acceptable.

>> It is then important to fail if bpf_mptcpify() is not executed at the
>> beginning of this new "socket_create()" wrapper. Will it be the case?
>>
>> I guess the verifier will make sure bpf_mptcpify() is only called with a
>> function having "struct socket_args" as first argument so "by accident",
>> it will only be bpf_mptcpify(), right? Maybe there is a way to clearly
>> mark that bpf_mptcpify() cannot be called elsewhere? Or make sure it
>> fails if the socket has already been created? (maybe that's something we
>> need to ask to BPF devs)
>>
>> One last thing: in your v3, I think you restrict the conversion to IPv4
>> only sockets, no?
>>
>>   (args->family == AF_INET || args->family == AF_INET6)
> 
> I updated this in v4.

Thanks!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net