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

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

[Geliang Tang]

v2:
 - patch 2: we should address this resource leak by adding the missing
   close() calls on the server side, rather than by removing the correct
   existing ones on the client side.
 - patch 3: fix more error messages as Matt suggested.

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

Geliang Tang (3):
  selftests: mptcp: close server file descriptor
  selftests: mptcp: close IPC descriptor on server side
  selftests: mptcp: sockopt: fix error messages

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

-- 
2.48.1

[PATCH mptcp-net v2 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")
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
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 v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The client-side function 'connect_one_server()' correctly closes the IPC
descriptor (a pipe or UNIX socket) after use. However, the server-side
functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c'
were missing the corresponding 'close()' call for their IPC descriptors.

This omission could lead to resource leaks (file descriptors) in the test
server processes over time.

This patch adds the missing 'close(pipefd)' and 'close(unixfd)' calls in the
server-side code, ensuring symmetric and correct resource cleanup.

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 40f2a1b24763..6a282ec21fd7 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_inq.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_inq.c
@@ -462,6 +462,7 @@ static void process_one_client(int fd, int unixfd)
 	get_tcp_inq(&msg, &tcp_inq);
 	assert(tcp_inq == 1);
 
+	close(unixfd);
 	close(fd);
 }
 
diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b44b6c9b0550..b616af36c16f 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -685,6 +685,7 @@ static void process_one_client(int fd, int pipefd)
 			       s.last_sample.mptcpi_bytes_acked - ret2);
 	}
 
+	close(pipefd);
 	close(fd);
 }
 
-- 
2.48.1

[PATCH mptcp-net v2 3/3] selftests: mptcp: sockopt: fix error messages

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch fixes several issues in the error reporting of the MPTCP sockopt
selftest:

1. Add diff calculation: The error messages for counter mismatches now
   include the actual difference ('diff') between the expected and received
   values, making debugging significantly easier.

2. Fix variable usage: The error check for 'mptcpi_bytes_acked' incorrectly
   used 'ret2' (sent bytes) for both the expected value and the difference
   calculation. It now correctly uses 'ret' (received bytes), which is the
   expected value for bytes_acked.

3. Fix off-by-one in diff: The calculation for the 'mptcpi_rcv_delta' diff
   was 's.mptcpi_rcv_delta - ret', which is off-by-one. It has been corrected
   to 's.mptcpi_rcv_delta - (ret + 1)' to match the expected value in the
   condition above it.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b616af36c16f..56dfd02c5d01 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -667,22 +667,26 @@ static void process_one_client(int fd, int pipefd)
 
 	do_getsockopts(&s, fd, ret, ret2);
 	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
-		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64, s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - ret);
+		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64 ", diff %" PRId64,
+		       s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - (ret + 1));
 
 	/* be nice when running on top of older kernel */
 	if (s.pkt_stats_avail) {
 		if (s.last_sample.mptcpi_bytes_sent != ret2)
-			xerror("mptcpi_bytes_sent %" PRIu64 ", expect %" PRIu64,
+			xerror("mptcpi_bytes_sent %" PRIu64 ", expect %" PRIu64
+			       ", diff %" PRId64,
 			       s.last_sample.mptcpi_bytes_sent, ret2,
 			       s.last_sample.mptcpi_bytes_sent - ret2);
 		if (s.last_sample.mptcpi_bytes_received != ret)
-			xerror("mptcpi_bytes_received %" PRIu64 ", expect %" PRIu64,
+			xerror("mptcpi_bytes_received %" PRIu64 ", expect %" PRIu64
+			       ", diff %" PRId64,
 			       s.last_sample.mptcpi_bytes_received, ret,
 			       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);
+			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64
+			       ", diff %" PRId64,
+			       s.last_sample.mptcpi_bytes_acked, ret,
+			       s.last_sample.mptcpi_bytes_acked - ret);
 	}
 
 	close(pipefd);
-- 
2.48.1

Re: [PATCH mptcp-net v2 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: 1 failed test(s): packetdrill_dss 🔴
- 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/17423040759

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


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 v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Matthieu Baerts]

Hi Geliang,

On 03/09/2025 06:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The client-side function 'connect_one_server()' correctly closes the IPC
> descriptor (a pipe or UNIX socket) after use. However, the server-side
> functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c'
> were missing the corresponding 'close()' call for their IPC descriptors.
> 
> This omission could lead to resource leaks (file descriptors) in the test
> server processes over time.
> 
> This patch adds the missing 'close(pipefd)' and 'close(unixfd)' calls in the
> server-side code, ensuring symmetric and correct resource cleanup.

I don't know if we need such patch. I mean: yes, that's better to close
such FD before closing the application, but then:

- it is strange to close it in the middle of a function
(process_one_client()), and not where it has been created (main())

- if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
server process.

So if you really want to fix that, I think it would be better to close
all these unix FD in the 'main()' function, no?

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

Re: [PATCH mptcp-net v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Matthieu Baerts]

Hi Geliang,

On 03/09/2025 13:57, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/09/2025 06:08, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> The client-side function 'connect_one_server()' correctly closes the IPC
>> descriptor (a pipe or UNIX socket) after use. However, the server-side
>> functions 'process_one_client()' in both 'mptcp_sockopt.c' and 'mptcp_inq.c'

FYI, checkpatch is complaining that this line is too long.

https://github.com/multipath-tcp/mptcp_net-next/actions/runs/17423040753

Because it is not an output of a log, can you go to the next line after
max 72 chars please?

(I didn't check, but there are other similar errors reported by
checkpatch. *If they make sense*, don't hesitate to fix them as well)

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

Re: [PATCH mptcp-net v2 3/3] selftests: mptcp: sockopt: fix error messages

[Matthieu Baerts]

Hi Geliang,

On 03/09/2025 06:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch fixes several issues in the error reporting of the MPTCP sockopt
> selftest:
> 
> 1. Add diff calculation: The error messages for counter mismatches now
>    include the actual difference ('diff') between the expected and received
>    values, making debugging significantly easier.

This could be interpreted as a new feature while it is a fix. What about:

  Fix diff not printed: The error messages for counter mismatches had
  the actual difference ('diff') as argument, but it was missing in the
  format string. Displaying it makes the debugging easier.

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

Re: [PATCH mptcp-net v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Geliang Tang]

Hi Matt,

On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/09/2025 06:08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The client-side function 'connect_one_server()' correctly closes
> > the IPC
> > descriptor (a pipe or UNIX socket) after use. However, the server-
> > side
> > functions 'process_one_client()' in both 'mptcp_sockopt.c' and
> > 'mptcp_inq.c'
> > were missing the corresponding 'close()' call for their IPC
> > descriptors.
> > 
> > This omission could lead to resource leaks (file descriptors) in
> > the test
> > server processes over time.
> > 
> > This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
> > calls in the
> > server-side code, ensuring symmetric and correct resource cleanup.
> 
> I don't know if we need such patch. I mean: yes, that's better to
> close
> such FD before closing the application, but then:
> 
> - it is strange to close it in the middle of a function
> (process_one_client()), and not where it has been created (main())
> 

Yes, close it in main() is much better. I added close(pipefds[1]) after
calling server(pipefds[1]) in the new version instead of adding
close(pipefd) in process_one_client().

> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
> server process.

Yes, indeed. close(pipefds[0]) is missing in the server process too.

> So if you really want to fix that, I think it would be better to
> close
> all these unix FD in the 'main()' function, no?

I also moved 'close(pipefd)' from connect_one_server() to main(), just
after calling client(pipefds[0]). And here's the new version:

        e1 = pipe(pipefds);
        if (e1 < 0)
                die_perror("pipe");

        s = xfork();
        if (s == 0) {
                close(pipefds[0]);
                ret = server(pipefds[1]);
                close(pipefds[1]);
                return ret;
        }

        close(pipefds[1]);

        /* wait until server bound a socket */
        e1 = read(pipefds[0], &e1, 4); 
        assert(e1 == 4); 

        c = xfork();
        if (c == 0) {
                ret = client(pipefds[0]);
                close(pipefds[0]);
                return ret;
        }

        close(pipefds[0]);

Is this better?

Thanks,
-Geliang

> 
> Cheers,
> Matt