diff mbox series

[mptcp-next,v13,4/4] selftests/bpf: Add mptcpify test

Message ID 15a618b03f65177166adf2850d4159cd4b77dfb1.1691808484.git.geliang.tang@suse.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series bpf: Force to MPTCP | expand

Commit Message

Geliang Tang Aug. 12, 2023, 2:54 a.m. UTC 
Implement a new test program mptcpify: if the family is AF_INET or
AF_INET6, the type is SOCK_STREAM, and the protocol ID is 0 or
IPPROTO_TCP, set it to IPPROTO_MPTCP. It will be hooked in
update_socket_protocol().

Extend 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 'getsockopt(SOL_PROTOCOL)'
and 'nstat' commands.

Acked-by: Yonghong Song <yonghong.song@linux.dev>
Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 102 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/mptcpify.c  |  20 ++++
 2 files changed, 122 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcpify.c

Comments

Martin KaFai Lau Aug. 15, 2023, 6:23 a.m. UTC | #1 
On 8/11/23 7:54 PM, Geliang Tang wrote:
> +static int verify_mptcpify(int server_fd)
> +{
> +	socklen_t optlen;
> +	char cmd[256];
> +	int protocol;
> +	int err = 0;
> +
> +	optlen = sizeof(protocol);
> +	if (!ASSERT_OK(getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen),
> +		       "getsockopt(SOL_PROTOCOL)"))
> +		return -1;
> +
> +	if (!ASSERT_EQ(protocol, IPPROTO_MPTCP, "protocol isn't MPTCP"))
> +		err++;
> +
> +	/* Output of nstat:
> +	 *
> +	 * #kernel
> +	 * MPTcpExtMPCapableSYNACKRX       1                  0.0
> +	 */
> +	snprintf(cmd, sizeof(cmd),
> +		 "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'",
> +		 NS_TEST, "MPTcpExtMPCapableSYNACKRX",
> +		 "NR==1 {next} {print $2}", "1");

Is the mp-capable something that the regular mptcp user want to learn from a fd 
also? Does it have a simpler way like to learn this, eg. getsockopt(fd, 
SOL_MPTCP, MPTCP_xxx), instead of parsing text output?

> +	if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!"))
Geliang Tang Aug. 15, 2023, 10:08 a.m. UTC | #2 
On Mon, Aug 14, 2023 at 11:23:49PM -0700, Martin KaFai Lau wrote:
> On 8/11/23 7:54 PM, Geliang Tang wrote:
> > +static int verify_mptcpify(int server_fd)
> > +{
> > +	socklen_t optlen;
> > +	char cmd[256];
> > +	int protocol;
> > +	int err = 0;
> > +
> > +	optlen = sizeof(protocol);
> > +	if (!ASSERT_OK(getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen),
> > +		       "getsockopt(SOL_PROTOCOL)"))
> > +		return -1;
> > +
> > +	if (!ASSERT_EQ(protocol, IPPROTO_MPTCP, "protocol isn't MPTCP"))
> > +		err++;
> > +
> > +	/* Output of nstat:
> > +	 *
> > +	 * #kernel
> > +	 * MPTcpExtMPCapableSYNACKRX       1                  0.0
> > +	 */
> > +	snprintf(cmd, sizeof(cmd),
> > +		 "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'",
> > +		 NS_TEST, "MPTcpExtMPCapableSYNACKRX",
> > +		 "NR==1 {next} {print $2}", "1");
> 
> Is the mp-capable something that the regular mptcp user want to learn from a
> fd also? Does it have a simpler way like to learn this, eg. getsockopt(fd,
> SOL_MPTCP, MPTCP_xxx), instead of parsing text output?

Thanks Martin. Yes, you're right. A better one is using getsockopt
(MPTCP_INFO) to get the mptcpi_flags, then test the FALLBACK bit to make
sure this MPTCP connection didn't fallback. This is, in other word, this
MPTCP connection has been established correctly. Something like this:

+       optlen = sizeof(info);
+       if (!ASSERT_OK(getsockopt(fd, SOL_MPTCP, MPTCP_INFO, &info, &optlen),
+                      "getsockopt(MPTCP_INFO)"))
+               return -1;
+
+       if (!ASSERT_FALSE(info.mptcpi_flags & MPTCP_INFO_FLAG_FALLBACK,
+                         "MPTCP fallback"))
+               err++;

It's necessary to add this further check after the MPTCP protocol check
using getsockopt(SOL_PROTOCOL). Since in some cases, the MPTCP protocol
check is not enough. Say, if we change TCP protocol into MPTCP using
"cgroup/sock_create", the hook of BPF_CGROUP_RUN_PROG_INET_SOCK in
inet_create(), this place is too late to change the protocol. Although
sk->sk_protocol is set to MPTCP correctly, and the MPTCP protocol check
using getsockopt(SOL_PROTOCOL) will pass. This MPTCP connection will
fallback to TCP connection. So this further check is needed.

-Geliang

> 
> > +	if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!"))
> 
>
Martin KaFai Lau Aug. 15, 2023, 6:57 p.m. UTC | #3 
On 8/15/23 3:08 AM, Geliang Tang wrote:
> On Mon, Aug 14, 2023 at 11:23:49PM -0700, Martin KaFai Lau wrote:
>> On 8/11/23 7:54 PM, Geliang Tang wrote:
>>> +static int verify_mptcpify(int server_fd)
>>> +{
>>> +	socklen_t optlen;
>>> +	char cmd[256];
>>> +	int protocol;
>>> +	int err = 0;
>>> +
>>> +	optlen = sizeof(protocol);
>>> +	if (!ASSERT_OK(getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen),
>>> +		       "getsockopt(SOL_PROTOCOL)"))
>>> +		return -1;
>>> +
>>> +	if (!ASSERT_EQ(protocol, IPPROTO_MPTCP, "protocol isn't MPTCP"))
>>> +		err++;
>>> +
>>> +	/* Output of nstat:
>>> +	 *
>>> +	 * #kernel
>>> +	 * MPTcpExtMPCapableSYNACKRX       1                  0.0
>>> +	 */
>>> +	snprintf(cmd, sizeof(cmd),
>>> +		 "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'",
>>> +		 NS_TEST, "MPTcpExtMPCapableSYNACKRX",
>>> +		 "NR==1 {next} {print $2}", "1");
>>
>> Is the mp-capable something that the regular mptcp user want to learn from a
>> fd also? Does it have a simpler way like to learn this, eg. getsockopt(fd,
>> SOL_MPTCP, MPTCP_xxx), instead of parsing text output?
> 
> Thanks Martin. Yes, you're right. A better one is using getsockopt
> (MPTCP_INFO) to get the mptcpi_flags, then test the FALLBACK bit to make
> sure this MPTCP connection didn't fallback. This is, in other word, this
> MPTCP connection has been established correctly. Something like this:
> 
> +       optlen = sizeof(info);
> +       if (!ASSERT_OK(getsockopt(fd, SOL_MPTCP, MPTCP_INFO, &info, &optlen),
> +                      "getsockopt(MPTCP_INFO)"))
> +               return -1;
> +
> +       if (!ASSERT_FALSE(info.mptcpi_flags & MPTCP_INFO_FLAG_FALLBACK,
> +                         "MPTCP fallback"))
> +               err++;
> 
> It's necessary to add this further check after the MPTCP protocol check
> using getsockopt(SOL_PROTOCOL). Since in some cases, the MPTCP protocol
> check is not enough. Say, if we change TCP protocol into MPTCP using
> "cgroup/sock_create", the hook of BPF_CGROUP_RUN_PROG_INET_SOCK in
> inet_create(), this place is too late to change the protocol. Although
> sk->sk_protocol is set to MPTCP correctly, and the MPTCP protocol check
> using getsockopt(SOL_PROTOCOL) will pass. This MPTCP connection will
> fallback to TCP connection. So this further check is needed.

If I read it correctly, it sounds like the "ip netns... nstat.... awk...grep..." 
can be replaced with the getsockopt(MPTCP_INFO)?  If that is the case, could you 
respin one more time to do getsockopt(MPTCP_INFO) instead? I would like the test 
to avoid parsing text output and have less dependency on external binary/library 
if possible (On top of 'ip', the above uses nstat, awk, grep...). This will make 
the bpf CI and other developers' environment tricky to maintain. eg. fwiw, 
although unrelated to the commands used above, my dev environment suddenly like 
to give this text output when I run "e"grep: "egrep: warning: egrep is 
obsolescent; using grep -E"

>>
>>> +	if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!"))
>>
>>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 3d3999067e27..a6bbc734a876 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -2,13 +2,19 @@ 
 /* Copyright (c) 2020, Tessares SA. */
 /* Copyright (c) 2022, SUSE. */
 
+#include <netinet/in.h>
 #include <test_progs.h>
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
 #include "mptcp_sock.skel.h"
+#include "mptcpify.skel.h"
 
 #define NS_TEST "mptcp_ns"
 
+#ifndef IPPROTO_MPTCP
+#define IPPROTO_MPTCP 262
+#endif
+
 #ifndef TCP_CA_NAME_MAX
 #define TCP_CA_NAME_MAX	16
 #endif
@@ -183,8 +189,104 @@  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 verify_mptcpify(int server_fd)
+{
+	socklen_t optlen;
+	char cmd[256];
+	int protocol;
+	int err = 0;
+
+	optlen = sizeof(protocol);
+	if (!ASSERT_OK(getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen),
+		       "getsockopt(SOL_PROTOCOL)"))
+		return -1;
+
+	if (!ASSERT_EQ(protocol, IPPROTO_MPTCP, "protocol isn't MPTCP"))
+		err++;
+
+	/* Output of nstat:
+	 *
+	 * #kernel
+	 * MPTcpExtMPCapableSYNACKRX       1                  0.0
+	 */
+	snprintf(cmd, sizeof(cmd),
+		 "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'",
+		 NS_TEST, "MPTcpExtMPCapableSYNACKRX",
+		 "NR==1 {next} {print $2}", "1");
+	if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!"))
+		err++;
+
+	return err;
+}
+
+static int run_mptcpify(int cgroup_fd)
+{
+	int server_fd, client_fd, err = 0;
+	struct mptcpify *mptcpify_skel;
+
+	mptcpify_skel = mptcpify__open_and_load();
+	if (!ASSERT_OK_PTR(mptcpify_skel, "skel_open_load"))
+		return libbpf_get_error(mptcpify_skel);
+
+	err = mptcpify__attach(mptcpify_skel);
+	if (!ASSERT_OK(err, "skel_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);
+	err = verify_mptcpify(server_fd);
+
+	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);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
 		test_base();
+	if (test__start_subtest("mptcpify"))
+		test_mptcpify();
 }
diff --git a/tools/testing/selftests/bpf/progs/mptcpify.c b/tools/testing/selftests/bpf/progs/mptcpify.c
new file mode 100644
index 000000000000..53301ae8a8f7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcpify.c
@@ -0,0 +1,20 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023, SUSE. */
+
+#include "vmlinux.h"
+#include <bpf/bpf_tracing.h>
+#include "bpf_tracing_net.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("fmod_ret/update_socket_protocol")
+int BPF_PROG(mptcpify, int family, int type, int protocol)
+{
+	if ((family == AF_INET || family == AF_INET6) &&
+	    type == SOCK_STREAM &&
+	    (!protocol || protocol == IPPROTO_TCP)) {
+		return IPPROTO_MPTCP;
+	}
+
+	return protocol;
+}