Re: [PATCH mptcp-next v4 3/3] mptcp: kunit: add sockopt test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: kunit: mptcp_sockopt_test() fails in kernel_connect() when lo is down
--

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

mptcp: kunit: add sockopt test

This commit adds a KUnit test for MPTCP socket options (SO_REUSEADDR,
SO_BINDTODEVICE, and SO_BINDTOIFINDEX) and lockdep class separation between
MPTCP sockets and their subflows across IPv4 and IPv6.

> diff --git a/net/mptcp/sockopt_test.c b/net/mptcp/sockopt_test.c
> new file mode 100644
> index 0000000000000..f8ce69911cafd
> --- /dev/null
> +++ b/net/mptcp/sockopt_test.c
> @@ -0,0 +1,184 @@

[ ... ]

> +static void mptcp_sockopt_test(struct kunit *test, sa_family_t family)
> +{
> +	struct socket *server_sock, *client_sock, *accepted_sock;
> +	struct msghdr send_msg = { 0 }, recv_msg = { 0 };
> +	struct sockaddr_storage addr = { 0 };
> +	struct kvec send_vec, recv_vec;
> +	char send_buf[] = "MPTCP";
> +	char recv_buf[32] = { 0 };
> +	int err, addrlen;
> +
> +	if (family == AF_INET6) {
> +		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&addr;
> +
> +		addr6->sin6_family = AF_INET6;
> +		addr6->sin6_port = htons(0);
> +		addr6->sin6_addr = in6addr_any;
> +		addrlen = sizeof(struct sockaddr_in6);
> +	} else {
> +		struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr;
> +
> +		addr4->sin_family = AF_INET;
> +		addr4->sin_port = htons(0);
> +		addr4->sin_addr.s_addr = htonl(INADDR_ANY);
> +		addrlen = sizeof(struct sockaddr_in);
> +	}
> +
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &server_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	err = kunit_add_action_or_reset(test, sock_release_wrapper,
> +					server_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	mptcp_test_reuseaddr(test, server_sock);
> +
> +	mptcp_test_bindtodevice(test, server_sock);
> +
> +	err = kernel_bind(server_sock, (struct sockaddr_unsized *)&addr,
> +			  addrlen);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = kernel_getsockname(server_sock, (struct sockaddr *)&addr);
> +	KUNIT_ASSERT_EQ(test, err, addrlen);
> +
> +	err = kernel_listen(server_sock, 5);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	err = sock_create_kern(&init_net, family, SOCK_STREAM, IPPROTO_MPTCP,
> +			       &client_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	err = kunit_add_action_or_reset(test, sock_release_wrapper,
> +					client_sock);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +
> +	mptcp_test_bindtoifindex(test, client_sock);
> +
> +	err = kernel_connect(client_sock,
> +			     (struct sockaddr_unsized *)&addr,
> +			     addrlen, 0);
> +	KUNIT_ASSERT_EQ(test, err, 0);

[Severity: Medium]
The commit message notes that INADDR_ANY is used instead of INADDR_LOOPBACK
to avoid failures when running during early boot before the loopback device
is up. However, will kernel_connect() in mptcp_sockopt_test() still fail
when CONFIG_MPTCP_KUNIT_TEST=y runs built-in at early boot?

When kunit_run_all_tests() executes from kernel_init_freeable(), the "lo"
device in init_net has been registered by loopback_net_init() without
IFF_UP set, so the NETDEV_UP handlers in inetdev_event() and
addrconf_notify() -> init_loopback() have not yet configured 127.0.0.1/8 or
::1/128.

While binding server_sock to INADDR_ANY or in6addr_any allows kernel_bind()
and kernel_listen() to succeed, client_sock is bound to lo->ifindex via
mptcp_test_bindtoifindex() before calling kernel_connect():

For AF_INET, mptcp_connect() -> tcp_v4_connect() -> ip_route_connect() ->
ip_route_output_key_hash_rcu() checks fl4->flowi4_oif in net/ipv4/route.c:

	if (fl4->flowi4_oif) {
		dev_out = dev_get_by_index_rcu(net, fl4->flowi4_oif);
		rth = ERR_PTR(-ENODEV);
		if (!dev_out)
			goto out;

		/* RACE: Check return value of inet_select_addr instead. */
		if (!(dev_out->flags & IFF_UP) || !__in_dev_get_rcu(dev_out)) {
			rth = ERR_PTR(-ENETUNREACH);
			goto out;
		}

Because "lo" does not have IFF_UP set, route lookup fails with
-ENETUNREACH.

For AF_INET6, mptcp_connect() -> tcp_v6_connect() rewrites in6addr_any to
in6addr_loopback in net/ipv6/tcp_ipv6.c:

	if (ipv6_addr_any(&usin->sin6_addr)) {
		if (ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr))
			ipv6_addr_set_v4mapped(htonl(INADDR_LOOPBACK),
					       &usin->sin6_addr);
		else
			usin->sin6_addr = in6addr_loopback;
	}

Subsequent route and source-address lookup via ip6_dst_lookup_flow() ->
ip6_dst_lookup_tail() -> ip6_route_get_saddr() -> ipv6_dev_get_saddr() then
fails with -EADDRNOTAVAIL or -ENETUNREACH because "lo" has no IPv6 address
or route while down (and __dev_queue_xmit() also drops packets with
-ENETDOWN when !(dev->flags & IFF_UP)).

Should the test run in a dedicated network namespace with its loopback
device brought up, or ensure "lo" is up before connecting?

[ ... ]

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("KUnit tests for MPTCP sockopt");

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