[PATCH mptcp-net 0/3] misc fixes for sockopt selftests

[PATCH mptcp-net 0/3] misc fixes for sockopt selftests

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch series addresses several resource management issues in the MPTCP
sockopt selftests:

Patch 1 fixes a resource leak by ensuring the server's listening socket
is properly closed after processing clients, rather than leaving it open.

Patch 2 eliminates double-closing of pipe descriptors by removing redundant
close operations in functions where these descriptors are already managed
by the main function.

Patch 3 corrects an error reporting issue where the wrong variable was
being checked in debug output, ensuring accurate debugging information
when assertions fail.

These changes improve the robustness and correctness of the MPTCP selftests
by ensuring proper resource cleanup and accurate error reporting.

Geliang Tang (3):
  selftests: mptcp: close server file descriptor
  selftests: mptcp: avoid double closing pipe descriptor
  selftests: mptcp: sockopt: fix variable check in error reporting

 tools/testing/selftests/net/mptcp/mptcp_inq.c     | 3 +--
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 6 +++---
 2 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.48.1

[PATCH mptcp-net 1/3] selftests: mptcp: close server file descriptor

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The server file descriptor ('fd') is opened in server() but never closed.
While accepted connections are properly closed in process_one_client(),
the main listening socket remains open, causing a resource leak.

This patch ensures the server fd is properly closed after processing
clients, bringing the sockopt and inq test cases in line with proper
resource cleanup practices.

Fixes: ce9979129a0b ("selftests: mptcp: add mptcp getsockopt test cases")
Fixes: b51880568f20 ("selftests: mptcp: add inq test case")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_inq.c     | 1 +
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_inq.c b/tools/testing/selftests/net/mptcp/mptcp_inq.c
index f3bcaa48df8f..40f2a1b24763 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_inq.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_inq.c
@@ -502,6 +502,7 @@ static int server(int unixfd)
 
 	process_one_client(r, unixfd);
 
+	close(fd);
 	return 0;
 }
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index e934dd26a59d..b44b6c9b0550 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -722,6 +722,7 @@ static int server(int pipefd)
 
 	process_one_client(r, pipefd);
 
+	close(fd);
 	return 0;
 }
 
-- 
2.48.1

[PATCH mptcp-net 2/3] selftests: mptcp: avoid double closing pipe descriptor

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The pipe descriptor (pipefds[0]) is already closed in main() after
the client() function completes, making the explicit close in
connect_one_server() redundant and potentially harmful.

This patch removes the unnecessary close operations in both the sockopt
and inq test cases to prevent double-closing of file descriptors, which
could lead to undefined behavior if the descriptor is reused.

Fixes: ce9979129a0b ("selftests: mptcp: add mptcp getsockopt test cases")
Fixes: b51880568f20 ("selftests: mptcp: add inq test case")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_inq.c     | 2 --
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_inq.c b/tools/testing/selftests/net/mptcp/mptcp_inq.c
index 40f2a1b24763..5e2b5913121f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_inq.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_inq.c
@@ -314,8 +314,6 @@ static void connect_one_server(int fd, int unixfd)
 	close(fd);
 	ret = write(unixfd, "closed", 6);
 	assert(ret == 6);
-
-	close(unixfd);
 }
 
 static void get_tcp_inq(struct msghdr *msgh, unsigned int *inqv)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b44b6c9b0550..8d590629a8e9 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -593,7 +593,6 @@ static void connect_one_server(int fd, int pipefd)
 	/* un-block server */
 	ret = read(pipefd, buf2, 4);
 	assert(ret == 4);
-	close(pipefd);
 
 	assert(strncmp(buf2, "xmit", 4) == 0);
 
-- 
2.48.1

[PATCH mptcp-net 3/3] selftests: mptcp: sockopt: fix variable check in error reporting

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The error message for mptcpi_bytes_acked comparison incorrectly
references 'ret2' instead of 'ret'. Since the comparison is between
mptcpi_bytes_acked and 'ret', the error output should consistently
use 'ret' for both the expected value and difference calculation.

This patch corrects the variable usage in the error output to ensure
accurate debugging information when the assertion fails.

Fixes: 5dcff89e1455 ("selftests: mptcp: explicitly tests aggregate counters")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index 8d590629a8e9..a61508270a5c 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -680,8 +680,8 @@ static void process_one_client(int fd, int pipefd)
 			       s.last_sample.mptcpi_bytes_received - ret);
 		if (s.last_sample.mptcpi_bytes_acked != ret)
 			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64,
-			       s.last_sample.mptcpi_bytes_acked, ret2,
-			       s.last_sample.mptcpi_bytes_acked - ret2);
+			       s.last_sample.mptcpi_bytes_acked, ret,
+			       s.last_sample.mptcpi_bytes_acked - ret);
 	}
 
 	close(fd);
-- 
2.48.1

Re: [PATCH mptcp-net 0/3] misc fixes for sockopt selftests

[MPTCP CI]

Hi Geliang,

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: Unstable: 5 failed test(s): packetdrill_mp_join packetdrill_sockopts packetdrill_syscalls selftest_mptcp_connect_checksum selftest_mptcp_join 🔴
- 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/17391054770

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


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)

Re: [PATCH mptcp-net 1/3] selftests: mptcp: close server file descriptor

[Matthieu Baerts]

Hi Geliang,

On 02/09/2025 03:38, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The server file descriptor ('fd') is opened in server() but never closed.
> While accepted connections are properly closed in process_one_client(),
> the main listening socket remains open, causing a resource leak.
> 
> This patch ensures the server fd is properly closed after processing
> clients, bringing the sockopt and inq test cases in line with proper
> resource cleanup practices.

Thanks!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

Re: [PATCH mptcp-net 2/3] selftests: mptcp: avoid double closing pipe descriptor

[Matthieu Baerts]

Hi Geliang,

Thank you for the fixes.

On 02/09/2025 03:38, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The pipe descriptor (pipefds[0]) is already closed in main() after
> the client() function completes, making the explicit close in
> connect_one_server() redundant and potentially harmful.

Mmh, the close() are done from different processes, after fork(), no?

If yes, each process should close the fd, and what was done was correct.
Or did I miss something?

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

Re: [PATCH mptcp-net 3/3] selftests: mptcp: sockopt: fix variable check in error reporting

[Matthieu Baerts]

Hi Geliang,

On 02/09/2025 03:38, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The error message for mptcpi_bytes_acked comparison incorrectly
> references 'ret2' instead of 'ret'. Since the comparison is between
> mptcpi_bytes_acked and 'ret', the error output should consistently
> use 'ret' for both the expected value and difference calculation.
> 
> This patch corrects the variable usage in the error output to ensure
> accurate debugging information when the assertion fails.
> 
> Fixes: 5dcff89e1455 ("selftests: mptcp: explicitly tests aggregate counters")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index 8d590629a8e9..a61508270a5c 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -680,8 +680,8 @@ static void process_one_client(int fd, int pipefd)
>  			       s.last_sample.mptcpi_bytes_received - ret);
>  		if (s.last_sample.mptcpi_bytes_acked != ret)
>  			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64,
> -			       s.last_sample.mptcpi_bytes_acked, ret2,
> -			       s.last_sample.mptcpi_bytes_acked - ret2);
> +			       s.last_sample.mptcpi_bytes_acked, ret,
> +			       s.last_sample.mptcpi_bytes_acked - ret);

Good catch! But I also just noticed there is one extra argument: you
have 2 "%", but 3 arguments after the string.

I guess the error should be:

  "mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64 ", diff %" PRIu64,
  (...)

(Same for the others above.) If yes, can you add this as part of the
same commit?

  selftests: mptcp: sockopt: fix error messages

(with a note about the fact the wrong variable was used)

>  	}
>  
>  	close(fd);

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