| Message ID | 20220513224827.662254-6-mathew.j.martineau@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: mptcp: Support for mptcp_sock | expand |
On Fri, May 13, 2022 at 3:48 PM Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > > From: Geliang Tang <geliang.tang@suse.com> > > This patch verifies the struct member token of struct mptcp_sock. Add a > new function get_msk_token() to parse the msk token from the output of > the command 'ip mptcp monitor', and verify it in verify_msk(). > > v4: > - use ASSERT_* instead of CHECK_FAIL (Andrii) > - skip the test if 'ip mptcp monitor' is not supported (Mat) > > Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Signed-off-by: Geliang Tang <geliang.tang@suse.com> > Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > --- > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 1 + > .../testing/selftests/bpf/prog_tests/mptcp.c | 64 +++++++++++++++++++ > .../testing/selftests/bpf/progs/mptcp_sock.c | 5 ++ > 3 files changed, 70 insertions(+) > [...] > + fd = open(monitor_log_path, O_RDONLY); > + if (!ASSERT_GE(fd, 0, "Failed to open monitor_log_path")) > + return token; > + > + len = read(fd, buf, sizeof(buf)); > + if (!ASSERT_GT(len, 0, "Failed to read monitor_log_path")) > + goto err; > + > + if (strncmp(buf, prefix, strlen(prefix))) { ASSERT_STRNEQ ? > + log_err("Invalid prefix %s", buf); > + goto err; > + } > + > + token = strtol(buf + strlen(prefix), NULL, 16); > + > +err: > + close(fd); > + return token; > +} > + > static int verify_msk(int map_fd, int client_fd) > { > char *msg = "MPTCP subflow socket"; > int err, cfd = client_fd; > struct mptcp_storage val; > + __u32 token; > + > + token = get_msk_token(); > + if (!ASSERT_GT(token, 0, "Unexpected token")) > + return -1; > > err = bpf_map_lookup_elem(map_fd, &cfd, &val); > if (!ASSERT_OK(err, "bpf_map_lookup_elem")) > @@ -58,6 +102,12 @@ static int verify_msk(int map_fd, int client_fd) > err++; > } > > + if (val.token != token) { ASSERT_NEQ > + log_err("Unexpected mptcp_sock.token %x != %x", > + val.token, token); > + err++; > + } > + > return err; > } > [...]
On Fri, May 13, 2022 at 03:48:25PM -0700, Mat Martineau wrote: [ ... ] > +/* > + * Parse the token from the output of 'ip mptcp monitor': > + * > + * [ CREATED] token=3ca933d3 remid=0 locid=0 saddr4=127.0.0.1 ... > + * [ CREATED] token=2ab57040 remid=0 locid=0 saddr4=127.0.0.1 ... How stable is the string format ? If it happens to have some unrelated mptcp connection going on, will the test break ? > + */ > +static __u32 get_msk_token(void) > +{ > + char *prefix = "[ CREATED] token="; > + char buf[BUFSIZ] = {}; > + __u32 token = 0; > + ssize_t len; > + int fd; > + > + sync(); > + > + fd = open(monitor_log_path, O_RDONLY); > + if (!ASSERT_GE(fd, 0, "Failed to open monitor_log_path")) > + return token; > + > + len = read(fd, buf, sizeof(buf)); > + if (!ASSERT_GT(len, 0, "Failed to read monitor_log_path")) > + goto err; > + > + if (strncmp(buf, prefix, strlen(prefix))) { > + log_err("Invalid prefix %s", buf); > + goto err; > + } > + > + token = strtol(buf + strlen(prefix), NULL, 16); > + > +err: > + close(fd); > + return token; > +} > +
Martin KaFai Lau <kafai@fb.com> 于2022年5月17日周二 09:32写道： > > On Fri, May 13, 2022 at 03:48:25PM -0700, Mat Martineau wrote: > [ ... ] > > > +/* > > + * Parse the token from the output of 'ip mptcp monitor': > > + * > > + * [ CREATED] token=3ca933d3 remid=0 locid=0 saddr4=127.0.0.1 ... > > + * [ CREATED] token=2ab57040 remid=0 locid=0 saddr4=127.0.0.1 ... > How stable is the string format ? > > If it happens to have some unrelated mptcp connection going on, will the test > break ? Hi Martin, Yes, it will break in that case. How can I fix this? Should I run the test in a special net namespace? 'ip mptcp monitor' can easily run in a special net namespace: ip -net ns1 mptcp monitor But I don't know how to run start_server() and connect_to_fd() in a special namespace. Could you please give me some suggestions about this? Thanks, -Geliang > > > + */ > > +static __u32 get_msk_token(void) > > +{ > > + char *prefix = "[ CREATED] token="; > > + char buf[BUFSIZ] = {}; > > + __u32 token = 0; > > + ssize_t len; > > + int fd; > > + > > + sync(); > > + > > + fd = open(monitor_log_path, O_RDONLY); > > + if (!ASSERT_GE(fd, 0, "Failed to open monitor_log_path")) > > + return token; > > + > > + len = read(fd, buf, sizeof(buf)); > > + if (!ASSERT_GT(len, 0, "Failed to read monitor_log_path")) > > + goto err; > > + > > + if (strncmp(buf, prefix, strlen(prefix))) { > > + log_err("Invalid prefix %s", buf); > > + goto err; > > + } > > + > > + token = strtol(buf + strlen(prefix), NULL, 16); > > + > > +err: > > + close(fd); > > + return token; > > +} > > + >
On Tue, May 17, 2022 at 01:19:38PM +0800, Geliang Tang wrote: > Martin KaFai Lau <kafai@fb.com> 于2022年5月17日周二 09:32写道： > > > > On Fri, May 13, 2022 at 03:48:25PM -0700, Mat Martineau wrote: > > [ ... ] > > > > > +/* > > > + * Parse the token from the output of 'ip mptcp monitor': > > > + * > > > + * [ CREATED] token=3ca933d3 remid=0 locid=0 saddr4=127.0.0.1 ... > > > + * [ CREATED] token=2ab57040 remid=0 locid=0 saddr4=127.0.0.1 ... > > How stable is the string format ? > > > > If it happens to have some unrelated mptcp connection going on, will the test > > break ? > > Hi Martin, > > Yes, it will break in that case. How can I fix this? Should I run the > test in a special net namespace? > > 'ip mptcp monitor' can easily run in a special net namespace: > > ip -net ns1 mptcp monitor > > But I don't know how to run start_server() and connect_to_fd() in a > special namespace. Could you please give me some suggestions about > this? You can take a look at the open_netns() usages under prog_tests/. For example, tc_redirect.c. I would also avoid string matching from "ip mptcp monitor" if possible considering the command may not have support (test skip) and the string format may change also. One option, you can consider to directly trace some of the mptcp functions by using bpf fentry/fexit prog to obtain the token and save it in a global bpf variable. Search 'SEC("fentry' or 'SEC("fexit' for some examples. Then the iproute2 support testing and the test skip logic can go away. Suggesting them here because the test will have better chance to catch issue if the test is not skipped or failed because of string format not match. I won't insist on not using "ip mptcp monitor" though.
diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h index 90fecafc493d..422491872619 100644 --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h @@ -229,6 +229,7 @@ extern void tcp_cong_avoid_ai(struct tcp_sock *tp, __u32 w, __u32 acked) __ksym; struct mptcp_sock { struct inet_connection_sock sk; + __u32 token; } __attribute__((preserve_access_index)); #endif diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c index 02e7fd8918e6..ac98aa314123 100644 --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -9,8 +9,11 @@ struct mptcp_storage { __u32 invoked; __u32 is_mptcp; + __u32 token; }; +static char monitor_log_path[64]; + static int verify_tsk(int map_fd, int client_fd) { char *msg = "plain TCP socket"; @@ -36,11 +39,52 @@ static int verify_tsk(int map_fd, int client_fd) return err; } +/* + * Parse the token from the output of 'ip mptcp monitor': + * + * [ CREATED] token=3ca933d3 remid=0 locid=0 saddr4=127.0.0.1 ... + * [ CREATED] token=2ab57040 remid=0 locid=0 saddr4=127.0.0.1 ... + */ +static __u32 get_msk_token(void) +{ + char *prefix = "[ CREATED] token="; + char buf[BUFSIZ] = {}; + __u32 token = 0; + ssize_t len; + int fd; + + sync(); + + fd = open(monitor_log_path, O_RDONLY); + if (!ASSERT_GE(fd, 0, "Failed to open monitor_log_path")) + return token; + + len = read(fd, buf, sizeof(buf)); + if (!ASSERT_GT(len, 0, "Failed to read monitor_log_path")) + goto err; + + if (strncmp(buf, prefix, strlen(prefix))) { + log_err("Invalid prefix %s", buf); + goto err; + } + + token = strtol(buf + strlen(prefix), NULL, 16); + +err: + close(fd); + return token; +} + static int verify_msk(int map_fd, int client_fd) { char *msg = "MPTCP subflow socket"; int err, cfd = client_fd; struct mptcp_storage val; + __u32 token; + + token = get_msk_token(); + if (!ASSERT_GT(token, 0, "Unexpected token")) + return -1; err = bpf_map_lookup_elem(map_fd, &cfd, &val); if (!ASSERT_OK(err, "bpf_map_lookup_elem")) @@ -58,6 +102,12 @@ static int verify_msk(int map_fd, int client_fd) err++; } + if (val.token != token) { + log_err("Unexpected mptcp_sock.token %x != %x", + val.token, token); + err++; + } + return err; } @@ -123,6 +173,7 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp) void test_base(void) { + char cmd[256], tmp_dir[] = "/tmp/XXXXXX"; int server_fd, cgroup_fd; cgroup_fd = test__join_cgroup("/mptcp"); @@ -140,6 +191,17 @@ void test_base(void) with_mptcp: /* with MPTCP */ + if (system("ip mptcp help 2>&1 | grep -q monitor")) { + test__skip(); + goto close_cgroup_fd; + } + if (!ASSERT_OK_PTR(mkdtemp(tmp_dir), "mkdtemp")) + goto close_cgroup_fd; + snprintf(monitor_log_path, sizeof(monitor_log_path), + "%s/ip_mptcp_monitor", tmp_dir); + snprintf(cmd, sizeof(cmd), "ip mptcp monitor > %s &", monitor_log_path); + if (!ASSERT_OK(system(cmd), "ip mptcp monitor")) + goto close_cgroup_fd; server_fd = start_mptcp_server(AF_INET, NULL, 0, 0); if (!ASSERT_GE(server_fd, 0, "start_mptcp_server")) goto close_cgroup_fd; @@ -147,6 +209,8 @@ void test_base(void) ASSERT_OK(run_test(cgroup_fd, server_fd, true), "run_test mptcp"); close(server_fd); + snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir); + system(cmd); close_cgroup_fd: close(cgroup_fd); diff --git a/tools/testing/selftests/bpf/progs/mptcp_sock.c b/tools/testing/selftests/bpf/progs/mptcp_sock.c index 3feb7ff578e2..4890130826c6 100644 --- a/tools/testing/selftests/bpf/progs/mptcp_sock.c +++ b/tools/testing/selftests/bpf/progs/mptcp_sock.c @@ -12,6 +12,7 @@ extern bool CONFIG_MPTCP __kconfig; struct mptcp_storage { __u32 invoked; __u32 is_mptcp; + __u32 token; }; struct { @@ -48,6 +49,8 @@ int _sockops(struct bpf_sock_ops *ctx) BPF_SK_STORAGE_GET_F_CREATE); if (!storage) return 1; + + storage->token = 0; } else { if (!CONFIG_MPTCP) return 1; @@ -60,6 +63,8 @@ int _sockops(struct bpf_sock_ops *ctx) BPF_SK_STORAGE_GET_F_CREATE); if (!storage) return 1; + + storage->token = msk->token; } storage->invoked++; storage->is_mptcp = is_mptcp;