Re: [PATCH mptcp-next v5 16/16] selftests/bpf: add MPTCP coverage to sockmap_listen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Unconditional loading of MPTCP BPF program breaks the entire test suite on kernels without CONFIG_MPTCP.
- [Medium] MPTCP redirect tests silently test standard TCP instead due to hardcoded protocol in create_socket_pairs().
- [Medium] The test suite unconditionally executes MPTCP socket tests and will report them as failures if the kernel does not support MPTCP.
- [Low] The SOTYPE_PROTO macro uses a bitwise AND on the SOCK_STREAM enum value.
--

commit 94803c56b754dd47ba0a9cd2f2a41576b9144b10
Author: Geliang Tang <geliang@kernel.org>

selftests/bpf: add MPTCP coverage to sockmap_listen

Extend sockmap_listen tests to cover MPTCP sockets. Run the full test
suite (ops, redir, reuseport) in both TCP and MPTCP modes, using
IPPROTO_MPTCP for stream sockets when in MPTCP mode. Skip UDP-specific
tests in MPTCP mode. Add a dedicated BPF reuseport program
(prog_reuseport_mptcp) that uses bpf_mptcp_sk_select_reuseport() for
MPTCP-aware reuseport selection. Prefix subtest names with the protocol
(tcp/mptcp) for clarity.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> index 1c96a3cf4b979..d31f3eb661d65 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -31,6 +31,10 @@
>  
>  #define NO_FLAGS 0
>  
> +static bool mptcp;
> +#define SOTYPE_PROTO(st) \
> +	(mptcp && ((st) & SOCK_STREAM) ? IPPROTO_MPTCP : 0)

[Severity: Low]
Does this bitwise AND on the socket type work correctly for all socket types?

Since SOCK_STREAM is defined as 1, ((st) & SOCK_STREAM) coincidentally
evaluates to true for SOCK_STREAM (1) and false for SOCK_DGRAM (2). However,
it evaluates to true for other odd-numbered socket types like SOCK_RAW (3) or
SOCK_SEQPACKET (5).

Could this be rewritten as ((st) == SOCK_STREAM)?

[ ... ]

> @@ -1359,7 +1375,8 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
>  	map_name = map_type_str(map);
>  
>  	for (t = tests; t < tests + ARRAY_SIZE(tests); t++) {
> -		snprintf(s, sizeof(s), "%s %s %s", map_name, family_name,
> +		snprintf(s, sizeof(s), "%s %s %s %s",
> +			 mptcp ? "mptcp" : "tcp", map_name, family_name,
>  			 t->name);

[Severity: Medium]
Are the MPTCP redirect tests silently testing standard TCP instead?

The test_redir() function loops through tests like redir_to_connected() which
use create_socket_pairs() to establish connections.

However, create_socket_pairs() (defined in socket_helpers.h) hardcodes
proto = 0, which resolves to IPPROTO_TCP for SOCK_STREAM. It completely ignores
the SOTYPE_PROTO macro logic intended to create IPPROTO_MPTCP sockets.

[ ... ]

> @@ -1429,13 +1455,17 @@ void serial_test_sockmap_listen(void)
>  		return;
>  	}
>  
> -	skel->bss->test_sockmap = true;
> -	run_tests(skel, skel->maps.sock_map, AF_INET);
> -	run_tests(skel, skel->maps.sock_map, AF_INET6);
> +	for (int i = 0; i < 2; i++) {
> +		mptcp = i;
>  
> -	skel->bss->test_sockmap = false;
> -	run_tests(skel, skel->maps.sock_hash, AF_INET);
> -	run_tests(skel, skel->maps.sock_hash, AF_INET6);
> +		skel->bss->test_sockmap = true;
> +		run_tests(skel, skel->maps.sock_map, AF_INET);
> +		run_tests(skel, skel->maps.sock_map, AF_INET6);

[Severity: Medium]
Will this unconditionally execute MPTCP socket tests and report them as failures
if the kernel lacks MPTCP support?

When mptcp is set to 1, functions like socket_loopback_proto() call xsocket(),
which invokes FAIL_ERRNO("socket") if the socket creation fails (e.g., returning
-1 with EPROTONOSUPPORT).

Should the test check for MPTCP support first and gracefully skip the MPTCP
iterations?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> index b7250eb9c30cc..46e9929ba7c4d 100644
> --- a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> @@ -137,4 +140,26 @@ int prog_reuseport(struct sk_reuseport_md *reuse)
>  	return verdict;
>  }
>  
> +SEC("sk_reuseport")
> +int prog_reuseport_mptcp(struct sk_reuseport_md *reuse)
> +{
> +	unsigned int *count;
> +	int err, verdict;
> +	__u32 zero = 0;
> +
> +	if (test_sockmap)
> +		err = bpf_mptcp_sk_select_reuseport(reuse, &sock_map,
> +						    &zero, 0);

[Severity: Medium]
Will this break the entire test suite on kernels without CONFIG_MPTCP?

The bpf_mptcp_sk_select_reuseport helper is unknown to the verifier when
CONFIG_MPTCP is disabled.

Since test_sockmap_listen__open_and_load() unconditionally loads all programs
in the skeleton, this will cause the load to fail and skip all standard TCP
and UDP tests.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789294365.git.tanggeliang@kylinos.cn?part=16