diff mbox series

[bpf-next,v3,5/8] selftests: bpf: test bpf_skc_to_mptcp_sock

Message ID 20220502211235.142250-6-mathew.j.martineau@linux.intel.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf: mptcp: Support for mptcp_sock and is_mptcp | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-2 fail Logs for Kernel LATEST on z15 + selftests
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest + selftests
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: songliubraving@fb.com shuah@kernel.org kafai@fb.com linux-kselftest@vger.kernel.org yhs@fb.com john.fastabend@gmail.com kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: externs should be avoided in .c files
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Mat Martineau May 2, 2022, 9:12 p.m. UTC 
From: Geliang Tang <geliang.tang@suse.com>

This patch extends the MPTCP test base, to test the new helper
bpf_skc_to_mptcp_sock().

Define struct mptcp_sock in bpf_tcp_helpers.h, use bpf_skc_to_mptcp_sock
to get the msk socket in progs/mptcp_sock.c and store the infos in
socket_storage_map.

Get the infos from socket_storage_map in prog_tests/mptcp.c. Add a new
function verify_msk() to verify the infos of MPTCP socket, and rename
verify_sk() to verify_tsk() to verify TCP socket only.

v2: Add CONFIG_MPTCP check for clearer error messages

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>
---
 MAINTAINERS                                   |  1 +
 .../testing/selftests/bpf/bpf_mptcp_helpers.h | 14 ++++++++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 36 +++++++++++++++----
 .../testing/selftests/bpf/progs/mptcp_sock.c  | 24 ++++++++++---
 4 files changed, 65 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_mptcp_helpers.h

Comments

Andrii Nakryiko May 6, 2022, 10:26 p.m. UTC | #1 
On Mon, May 2, 2022 at 2:12 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> From: Geliang Tang <geliang.tang@suse.com>
>
> This patch extends the MPTCP test base, to test the new helper
> bpf_skc_to_mptcp_sock().
>
> Define struct mptcp_sock in bpf_tcp_helpers.h, use bpf_skc_to_mptcp_sock
> to get the msk socket in progs/mptcp_sock.c and store the infos in
> socket_storage_map.
>
> Get the infos from socket_storage_map in prog_tests/mptcp.c. Add a new
> function verify_msk() to verify the infos of MPTCP socket, and rename
> verify_sk() to verify_tsk() to verify TCP socket only.
>
> v2: Add CONFIG_MPTCP check for clearer error messages
>
> 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>
> ---
>  MAINTAINERS                                   |  1 +
>  .../testing/selftests/bpf/bpf_mptcp_helpers.h | 14 ++++++++
>  .../testing/selftests/bpf/prog_tests/mptcp.c  | 36 +++++++++++++++----
>  .../testing/selftests/bpf/progs/mptcp_sock.c  | 24 ++++++++++---
>  4 files changed, 65 insertions(+), 10 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 359afc617b92..d48d3cb6abbc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13780,6 +13780,7 @@ F:      include/net/mptcp.h
>  F:     include/trace/events/mptcp.h
>  F:     include/uapi/linux/mptcp.h
>  F:     net/mptcp/
> +F:     tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>  F:     tools/testing/selftests/bpf/*/*mptcp*.c
>  F:     tools/testing/selftests/net/mptcp/
>
> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> new file mode 100644
> index 000000000000..18da4cc65e89
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2022, SUSE. */
> +
> +#ifndef __BPF_MPTCP_HELPERS_H
> +#define __BPF_MPTCP_HELPERS_H
> +
> +#include "bpf_tcp_helpers.h"
> +
> +struct mptcp_sock {
> +       struct inet_connection_sock     sk;
> +
> +} __attribute__((preserve_access_index));

why can't all this live in bpf_tcp_helpers.h? why do we need extra header?

> +
> +#endif
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index cd548bb2828f..4b40bbdaf91f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -10,14 +10,12 @@ struct mptcp_storage {
>         __u32 is_mptcp;
>  };
>

[...]
Matthieu Baerts May 9, 2022, 9 a.m. UTC | #2 
Hi Andrii,

Thank you for the review!

On 07/05/2022 00:26, Andrii Nakryiko wrote:
> On Mon, May 2, 2022 at 2:12 PM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:

(...)

>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 359afc617b92..d48d3cb6abbc 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13780,6 +13780,7 @@ F:      include/net/mptcp.h
>>  F:     include/trace/events/mptcp.h
>>  F:     include/uapi/linux/mptcp.h
>>  F:     net/mptcp/
>> +F:     tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>>  F:     tools/testing/selftests/bpf/*/*mptcp*.c
>>  F:     tools/testing/selftests/net/mptcp/
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>> new file mode 100644
>> index 000000000000..18da4cc65e89
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2022, SUSE. */
>> +
>> +#ifndef __BPF_MPTCP_HELPERS_H
>> +#define __BPF_MPTCP_HELPERS_H
>> +
>> +#include "bpf_tcp_helpers.h"
>> +
>> +struct mptcp_sock {
>> +       struct inet_connection_sock     sk;
>> +
>> +} __attribute__((preserve_access_index));
> 
> why can't all this live in bpf_tcp_helpers.h? why do we need extra header?

The main reason is related to the maintenance: to have MPTCP ML being
cc'd for all patches modifying this file.

Do you prefer if all these specific MPTCP structures and macros and
mixed with TCP ones?

Cheers,
Matt
Andrii Nakryiko May 9, 2022, 9 p.m. UTC | #3 
On Mon, May 9, 2022 at 2:00 AM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Andrii,
>
> Thank you for the review!
>
> On 07/05/2022 00:26, Andrii Nakryiko wrote:
> > On Mon, May 2, 2022 at 2:12 PM Mat Martineau
> > <mathew.j.martineau@linux.intel.com> wrote:
>
> (...)
>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 359afc617b92..d48d3cb6abbc 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -13780,6 +13780,7 @@ F:      include/net/mptcp.h
> >>  F:     include/trace/events/mptcp.h
> >>  F:     include/uapi/linux/mptcp.h
> >>  F:     net/mptcp/
> >> +F:     tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> >>  F:     tools/testing/selftests/bpf/*/*mptcp*.c
> >>  F:     tools/testing/selftests/net/mptcp/
> >>
> >> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> >> new file mode 100644
> >> index 000000000000..18da4cc65e89
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
> >> @@ -0,0 +1,14 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright (c) 2022, SUSE. */
> >> +
> >> +#ifndef __BPF_MPTCP_HELPERS_H
> >> +#define __BPF_MPTCP_HELPERS_H
> >> +
> >> +#include "bpf_tcp_helpers.h"
> >> +
> >> +struct mptcp_sock {
> >> +       struct inet_connection_sock     sk;
> >> +
> >> +} __attribute__((preserve_access_index));
> >
> > why can't all this live in bpf_tcp_helpers.h? why do we need extra header?
>
> The main reason is related to the maintenance: to have MPTCP ML being
> cc'd for all patches modifying this file.
>
> Do you prefer if all these specific MPTCP structures and macros and
> mixed with TCP ones?
>

These definitions don't even have to be 1:1 w/ whatever is kernel
defining in terms of having all the fields, or their order, etc. So I
think it won't require active maintenance and thus can be merged into
bpf_tcp_helpers.h to keep it in one place.


> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
Matthieu Baerts May 10, 2022, 1:48 p.m. UTC | #4 
Hi Andrii,

On 09/05/2022 23:00, Andrii Nakryiko wrote:
> On Mon, May 9, 2022 at 2:00 AM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Andrii,
>>
>> Thank you for the review!
>>
>> On 07/05/2022 00:26, Andrii Nakryiko wrote:
>>> On Mon, May 2, 2022 at 2:12 PM Mat Martineau
>>> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> (...)
>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 359afc617b92..d48d3cb6abbc 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -13780,6 +13780,7 @@ F:      include/net/mptcp.h
>>>>  F:     include/trace/events/mptcp.h
>>>>  F:     include/uapi/linux/mptcp.h
>>>>  F:     net/mptcp/
>>>> +F:     tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>>>>  F:     tools/testing/selftests/bpf/*/*mptcp*.c
>>>>  F:     tools/testing/selftests/net/mptcp/
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>>>> new file mode 100644
>>>> index 000000000000..18da4cc65e89
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
>>>> @@ -0,0 +1,14 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright (c) 2022, SUSE. */
>>>> +
>>>> +#ifndef __BPF_MPTCP_HELPERS_H
>>>> +#define __BPF_MPTCP_HELPERS_H
>>>> +
>>>> +#include "bpf_tcp_helpers.h"
>>>> +
>>>> +struct mptcp_sock {
>>>> +       struct inet_connection_sock     sk;
>>>> +
>>>> +} __attribute__((preserve_access_index));
>>>
>>> why can't all this live in bpf_tcp_helpers.h? why do we need extra header?
>>
>> The main reason is related to the maintenance: to have MPTCP ML being
>> cc'd for all patches modifying this file.
>>
>> Do you prefer if all these specific MPTCP structures and macros and
>> mixed with TCP ones?
>>
> 
> These definitions don't even have to be 1:1 w/ whatever is kernel
> defining in terms of having all the fields, or their order, etc. So I
> think it won't require active maintenance and thus can be merged into
> bpf_tcp_helpers.h to keep it in one place.

Thank you for your reply!

New structures and macros[1] are going to be added later but I see your
point: there is nothing requiring an active maintenance. We can move
them all to bpf_tcp_helpers.h.

[1]
https://github.com/multipath-tcp/mptcp_net-next/blob/export/20220510T054929/tools/testing/selftests/bpf/bpf_mptcp_helpers.h

Cheers,
Matt
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 359afc617b92..d48d3cb6abbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13780,6 +13780,7 @@  F:	include/net/mptcp.h
 F:	include/trace/events/mptcp.h
 F:	include/uapi/linux/mptcp.h
 F:	net/mptcp/
+F:	tools/testing/selftests/bpf/bpf_mptcp_helpers.h
 F:	tools/testing/selftests/bpf/*/*mptcp*.c
 F:	tools/testing/selftests/net/mptcp/
 
diff --git a/tools/testing/selftests/bpf/bpf_mptcp_helpers.h b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
new file mode 100644
index 000000000000..18da4cc65e89
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_mptcp_helpers.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2022, SUSE. */
+
+#ifndef __BPF_MPTCP_HELPERS_H
+#define __BPF_MPTCP_HELPERS_H
+
+#include "bpf_tcp_helpers.h"
+
+struct mptcp_sock {
+	struct inet_connection_sock	sk;
+
+} __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 cd548bb2828f..4b40bbdaf91f 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -10,14 +10,12 @@  struct mptcp_storage {
 	__u32 is_mptcp;
 };
 
-static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
+static int verify_tsk(int map_fd, int client_fd)
 {
+	char *msg = "plain TCP socket";
 	int err = 0, cfd = client_fd;
 	struct mptcp_storage val;
 
-	if (is_mptcp == 1)
-		return 0;
-
 	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
 		perror("Failed to read socket storage");
 		return -1;
@@ -38,6 +36,32 @@  static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 	return err;
 }
 
+static int verify_msk(int map_fd, int client_fd)
+{
+	char *msg = "MPTCP subflow socket";
+	int err = 0, cfd = client_fd;
+	struct mptcp_storage val;
+
+	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
+		perror("Failed to read socket storage");
+		return -1;
+	}
+
+	if (val.invoked != 1) {
+		log_err("%s: unexpected invoked count %d != 1",
+			msg, val.invoked);
+		err++;
+	}
+
+	if (val.is_mptcp != 1) {
+		log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != 1",
+			msg, val.is_mptcp);
+		err++;
+	}
+
+	return err;
+}
+
 static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 {
 	int client_fd, prog_fd, map_fd, err;
@@ -88,8 +112,8 @@  static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 		goto out;
 	}
 
-	err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1) :
-			  verify_sk(map_fd, client_fd, "plain TCP socket", 0);
+	err += is_mptcp ? verify_msk(map_fd, client_fd) :
+			  verify_tsk(map_fd, client_fd);
 
 	close(client_fd);
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp_sock.c b/tools/testing/selftests/bpf/progs/mptcp_sock.c
index 0d65fb889d03..7b6a25e37de8 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_sock.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_sock.c
@@ -3,9 +3,11 @@ 
 
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_mptcp_helpers.h"
 
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;
+extern bool CONFIG_MPTCP __kconfig;
 
 struct mptcp_storage {
 	__u32 invoked;
@@ -24,6 +26,7 @@  int _sockops(struct bpf_sock_ops *ctx)
 {
 	struct mptcp_storage *storage;
 	struct bpf_tcp_sock *tcp_sk;
+	struct mptcp_sock *msk;
 	int op = (int)ctx->op;
 	struct bpf_sock *sk;
 
@@ -38,11 +41,24 @@  int _sockops(struct bpf_sock_ops *ctx)
 	if (!tcp_sk)
 		return 1;
 
-	storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
-				     BPF_SK_STORAGE_GET_F_CREATE);
-	if (!storage)
-		return 1;
+	if (!tcp_sk->is_mptcp) {
+		storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+					     BPF_SK_STORAGE_GET_F_CREATE);
+		if (!storage)
+			return 1;
+	} else {
+		if (!CONFIG_MPTCP)
+			return 1;
+
+		msk = bpf_skc_to_mptcp_sock(sk);
+		if (!msk)
+			return 1;
 
+		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
+					     BPF_SK_STORAGE_GET_F_CREATE);
+		if (!storage)
+			return 1;
+	}
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;