From: Gang Yan <yangang@kylinos.cn>

Through code coverage analysis, it has been identified that the
'mptcp_diag_dump_one' function lacks test coverage on the testing front.

This patch introduces a function in mptcp_sockopt.c, which is built upon
the 'inet_diag' module, and integrates it into the 'mptcp_sockopt.sh' to
execute the 'mptcp_diag_dump_one' function when server bound a socket.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/524
Signed-off-by: Gang Yan <yangang@kylinos.cn>
---
 .../selftests/net/mptcp/mptcp_sockopt.c       | 76 ++++++++++++++++++-
 .../selftests/net/mptcp/mptcp_sockopt.sh      |  6 ++
 2 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index 926b0be87c99..caa0466d827f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -25,6 +25,10 @@
 #include <netinet/in.h>
 
 #include <linux/tcp.h>
+#include <linux/netlink.h>
+#include <linux/inet_diag.h>
+#include <linux/rtnetlink.h>
+#include <linux/sock_diag.h>
 
 static int pf = AF_INET;
 
@@ -250,11 +254,77 @@ static int sock_connect_mptcp(const char * const remoteaddr,
 	return sock;
 }
 
+static void send_query(int fd)
+{
+	struct sockaddr_nl nladdr = {
+		.nl_family = AF_NETLINK
+	};
+	struct {
+		struct nlmsghdr nlh;
+		struct inet_diag_req_v2 r;
+	} req = {
+		.nlh = {
+			.nlmsg_len = sizeof(req),
+			.nlmsg_type = SOCK_DIAG_BY_FAMILY,
+			.nlmsg_flags = NLM_F_REQUEST | NLM_F_ATOMIC
+		},
+		.r = {
+			.sdiag_family = AF_INET,
+			.sdiag_protocol = IPPROTO_MPTCP,
+		}
+	};
+	struct rtattr rta_proto;
+	struct iovec iov[6];
+	int iovlen = 1;
+	__u32 proto;
+
+	proto = IPPROTO_MPTCP;
+	rta_proto.rta_type = INET_DIAG_REQ_PROTOCOL;
+	rta_proto.rta_len = RTA_LENGTH(sizeof(proto));
+
+	iov[0] = (struct iovec) {
+		.iov_base = &req,
+		.iov_len = sizeof(req)
+	};
+	iov[iovlen] = (struct iovec){ &rta_proto, sizeof(rta_proto)};
+	iov[iovlen+1] = (struct iovec){ &proto, sizeof(proto)};
+	req.nlh.nlmsg_len += RTA_LENGTH(sizeof(proto));
+	iovlen += 2;
+
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = iov,
+		.msg_iovlen = iovlen
+	};
+
+	for (;;) {
+		if (sendmsg(fd, &msg, 0) < 0) {
+			if (errno == EINTR)
+				continue;
+			die_perror("sendmsg");
+		}
+		return;
+	}
+}
+
+void diag(void)
+{
+	int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
+
+	if (fd < 0)
+		die_perror("Netlink socket");
+
+	send_query(fd);
+
+	close(fd);
+}
+
 static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "h6")) != -1) {
+	while ((c = getopt(argc, argv, "h6d")) != -1) {
 		switch (c) {
 		case 'h':
 			die_usage(0);
@@ -262,6 +332,10 @@ static void parse_opts(int argc, char **argv)
 		case '6':
 			pf = AF_INET6;
 			break;
+		case 'd':
+			diag();
+			exit(0);
+			break;
 		default:
 			die_usage(1);
 			break;
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
index 5e8d5b83e2d0..3f74100bc023 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
@@ -278,6 +278,11 @@ do_mptcp_sockopt_tests()
 	mptcp_lib_result_pass "sockopt v6"
 }
 
+do_diag_tests()
+{
+	ip netns exec "$ns_sbox" ./mptcp_sockopt -d
+}
+
 run_tests()
 {
 	local listener_ns="$1"
@@ -356,6 +361,7 @@ run_tests $ns1 $ns2 dead:beef:1::1
 
 do_mptcp_sockopt_tests
 do_tcpinq_tests
+do_diag_tests
 
 mptcp_lib_result_print_all_tap
 exit $ret
-- 
2.25.1

Hi Gang Yan,

On 17/01/2025 07:44, Gang Yan wrote:
> From: Gang Yan <yangang@kylinos.cn>
> 
> Through code coverage analysis, it has been identified that the
> 'mptcp_diag_dump_one' function lacks test coverage on the testing front.
> 
> This patch introduces a function in mptcp_sockopt.c, which is built upon
> the 'inet_diag' module, and integrates it into the 'mptcp_sockopt.sh' to
> execute the 'mptcp_diag_dump_one' function when server bound a socket.

Thank you for increasing the code coverage!

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/524
> Signed-off-by: Gang Yan <yangang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/mptcp_sockopt.c       | 76 ++++++++++++++++++-
>  .../selftests/net/mptcp/mptcp_sockopt.sh      |  6 ++
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index 926b0be87c99..caa0466d827f 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c

(...)

> +void diag(void)
> +{
> +	int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
> +
> +	if (fd < 0)
> +		die_perror("Netlink socket");
> +
> +	send_query(fd);

From what I see, you are sending a request, but you don't look at the
reply. I understand the objective is to increase the code coverage, but
while at it, it sounds better to verify the kernel is doing the right
thing: the reply looks correct.

Do you mind checking the reply? e.g. create a connection, request info
about it, and compare (a part of) the reply with info you already have.

(Also, is your code here covering mptcp_diag_dump_one() function?)

> +
> +	close(fd);
> +}
> +
>  static void parse_opts(int argc, char **argv)
>  {
>  	int c;
>  
> -	while ((c = getopt(argc, argv, "h6")) != -1) {
> +	while ((c = getopt(argc, argv, "h6d")) != -1) {
>  		switch (c) {
>  		case 'h':
>  			die_usage(0);
> @@ -262,6 +332,10 @@ static void parse_opts(int argc, char **argv)
>  		case '6':
>  			pf = AF_INET6;
>  			break;
> +		case 'd':
> +			diag();
> +			exit(0);
> +			break;
>  		default:
>  			die_usage(1);

(you didn't update the 'usage' menu)

>  			break;
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 5e8d5b83e2d0..3f74100bc023 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh

(...)

> @@ -356,6 +361,7 @@ run_tests $ns1 $ns2 dead:beef:1::1
>  
>  do_mptcp_sockopt_tests
>  do_tcpinq_tests
> +do_diag_tests

It sounds strange to do this Netlink request in the MPTCP Socket Option
test program.

Ideally, this should be done from diag.sh. I guess you modified
mptcp_sockopt.c because you needed to do some operations in C, right?

Here are some ideas:

- Create a new diag.c file to do these operations, and call the new
program from diag.sh. In diag.sh, you already have connections that are
created, you can do an extra check there, e.g. compare info from ss and
from diag.c.

- Modify 'ss' to be able to query one MPTCP connection having a given
token. Then use this feature (if available) in diag.sh.

- Modify mptcp_sockopt.c to query info about one connection via netlink,
e.g. to compare them with info retrieved via getsockopt(MPTCP_INFO) from
do_getsockopt_mptcp_info(). You could compare them in
process_one_client() I suppose (to be checked where it sounds better to
do that).

- ...


The first 2 options might make more sense: it is all about 'diag'. Maybe
the 3rd option can be justified if the goal is to compare more data from
getsockopt(MPTCP_INFO).


Note that modifying 'ss' to query info about a specific connection seems
useful anyway. In other words, we could have both option 1 and 2 in
place. Or only option 1 or 3 + the support in 'ss' but not used in the
selftests.

WDYT?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Hi Gang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12824293005

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ee9495d612db
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=926359


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)