Re: [PATCH mptcp-next v4 5/5] selftests/bpf: Update sockmap tests for MPTCP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete test conversion for `test_sockmap_with_mptcp` that contradicts the commit message.

Pre-existing issues:
- [High] Unchecked `skb_pull()` in `mptcp_read_skb()` causes a kernel panic if the partial read offset exceeds the linear data length.
--

--- Patch [5]: [PATCH mptcp-next v4 5/5] selftests/bpf: Update sockmap tests for MPTCP ---
commit a45d03b16d520c3cfcda9a50c975aef36b416a66
Author: Geliang Tang <geliang@kernel.org>

selftests/bpf: Update sockmap tests for MPTCP

This commit updates sockmap tests to reflect new MPTCP support. It allows
MPTCP sockets in sockmap and adjusts the test expectations. It also adds
explicit map updates for server_fd1 and server_fd2 since sockops fires on TCP
subflows which return -EOPNOTSUPP for MPTCP connections.

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index d77c9f8c53c75..4f8727dbcbfd0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
[ ... ]
> @@ -500,30 +500,36 @@ static void test_subflow(void)
[ ... ]
>  	server_fd1 = accept(listen_fd, NULL, 0);
> +	err = bpf_map_update_elem(bpf_map__fd(skel->maps.sock_map),
> +				  &zero, &server_fd1, BPF_NOEXIST);
> +	if (!ASSERT_OK(err, "sockmap:add server_fd1"))
> +		goto end;
> +
>  	skel->bss->sk_index = 1;
>  	client_fd2 = connect_to_fd_opts(listen_fd, NULL);
> -	if (!ASSERT_OK_FD(client_fd2, "sockmap-fb:connect_to_fd"))
> +	if (!ASSERT_OK_FD(client_fd2, "sockmap:connect_to_fd"))
>  		goto end;
>  
>  	server_fd2 = accept(listen_fd, NULL, 0);

[Severity: Medium]
Does this code miss the changes for client_fd2 and server_fd2 that were
mentioned in the commit message?

The commit message states:
"Add server_fd1 and server_fd2 to the sockmap explicitly from userspace via
bpf_map_update_elem() instead of relying on the sockops program."

However, that map update appears to be missing here for server_fd2.

Additionally, client_fd2 is still created using connect_to_fd_opts() with
NULL options, creating a TCP fallback socket rather than an MPTCP socket.
Should client_fd2 also be converted to use connect_to_fd(listen_fd, 0) to
fully test the MPTCP support for the second socket pair?


[Severity: High]
This is a pre-existing issue, but can the unchecked skb_pull() in
mptcp_read_skb() located in net/mptcp/protocol.c cause a kernel panic?

If a user creates an MPTCP socket, receives a packet where the payload is
stored in page fragments, reads a partial amount of data, and then attaches
the socket to a sockmap, mptcp_read_skb() can be invoked.

net/mptcp/protocol.c:mptcp_read_skb() {
    ...
        if (offset)
            skb_pull(skb, offset);
    ...
}

If the partial read offset exceeds the linear data length, the skb_pull() call
without prior bounds checking will trigger a BUG() inside __skb_pull(). Should
this offset be validated against the available linear region before pulling?

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