[PATCH 0/3] selftests: mptcp: Fix various issues in main_loop

[PATCH 0/3] selftests: mptcp: Fix various issues in main_loop

[Cong Liu]

Fix several issues in the mptcp connect test's main_loop function.

 - Fix a bug where the wrong file descriptor was being checked for errors
 - Fix the input file descriptor lifecycle in the reconnection loop to
   prevent use of invalid fd
 - Add proper resource cleanup in error paths

Cong Liu (3):
  selftests: mptcp: Fix incorrect file descriptor check in main_loop
  selftests: mptcp: Fix input fd lifecycle in reconnection loop
  selftests: mptcp: Clean up resources properly in main_loop

 .../selftests/net/mptcp/mptcp_connect.c        | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)


base-commit: 2b88851f583d3c4e40bcd40cfe1965241ec229dd
-- 
2.43.0

[PATCH 1/3] selftests: mptcp: Fix incorrect file descriptor check in main_loop

[Cong Liu]

Fix a bug where the code was checking the wrong file descriptor
when opening the input file. The code was checking 'fd' instead
of 'fd_in', which could lead to incorrect error handling.

Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4209b9569039..31f4c5618569 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1249,7 +1249,7 @@ int main_loop(void)
 
 	if (cfg_input && cfg_sockopt_types.mptfo) {
 		fd_in = open(cfg_input, O_RDONLY);
-		if (fd < 0)
+		if (fd_in < 0)
 			xerror("can't open %s:%d", cfg_input, errno);
 	}
 
@@ -1272,7 +1272,7 @@ int main_loop(void)
 
 	if (cfg_input && !cfg_sockopt_types.mptfo) {
 		fd_in = open(cfg_input, O_RDONLY);
-		if (fd < 0)
+		if (fd_in < 0)
 			xerror("can't open %s:%d", cfg_input, errno);
 	}
 
-- 
2.43.0

[PATCH 2/3] selftests: mptcp: Fix input fd lifecycle in reconnection loop

[Cong Liu]

When both cfg_input and cfg_sockopt_types.mptfo are set, the input file
descriptor (fd_in) is opened before the reconnection loop but closed
within the loop. However, when mptfo is enabled, the descriptor is not
reopened in the loop, causing subsequent iterations to use an invalid
file descriptor.

Move the file open operation into the loop to ensure fd_in is always
valid when needed, regardless of mptfo setting.

Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 31f4c5618569..4d4ea4627daa 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1247,12 +1247,6 @@ int main_loop(void)
 	struct addrinfo *peer;
 	struct wstate winfo;
 
-	if (cfg_input && cfg_sockopt_types.mptfo) {
-		fd_in = open(cfg_input, O_RDONLY);
-		if (fd_in < 0)
-			xerror("can't open %s:%d", cfg_input, errno);
-	}
-
 	memset(&winfo, 0, sizeof(winfo));
 	fd = sock_connect_mptcp(cfg_host, cfg_port, cfg_sock_proto, &peer, fd_in, &winfo);
 	if (fd < 0)
@@ -1270,7 +1264,7 @@ int main_loop(void)
 	if (cfg_cmsg_types.cmsg_enabled)
 		apply_cmsg_types(fd, &cfg_cmsg_types);
 
-	if (cfg_input && !cfg_sockopt_types.mptfo) {
+	if (cfg_input) {
 		fd_in = open(cfg_input, O_RDONLY);
 		if (fd_in < 0)
 			xerror("can't open %s:%d", cfg_input, errno);
-- 
2.43.0

Re: [PATCH 2/3] selftests: mptcp: Fix input fd lifecycle in reconnection loop

[Geliang Tang]

cc mptcp only.

On Mon, 2025-01-13 at 16:52 +0800, Cong Liu wrote:
> When both cfg_input and cfg_sockopt_types.mptfo are set, the input
> file
> descriptor (fd_in) is opened before the reconnection loop but closed
> within the loop. However, when mptfo is enabled, the descriptor is
> not
> reopened in the loop, causing subsequent iterations to use an invalid
> file descriptor.
> 
> Move the file open operation into the loop to ensure fd_in is always
> valid when needed, regardless of mptfo setting.
> 
> Signed-off-by: Cong Liu <liucong2@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 31f4c5618569..4d4ea4627daa 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1247,12 +1247,6 @@ int main_loop(void)
>  	struct addrinfo *peer;
>  	struct wstate winfo;
>  
> -	if (cfg_input && cfg_sockopt_types.mptfo) {
> -		fd_in = open(cfg_input, O_RDONLY);
> -		if (fd_in < 0)
> -			xerror("can't open %s:%d", cfg_input,
> errno);
> -	}
> -
>  	memset(&winfo, 0, sizeof(winfo));
>  	fd = sock_connect_mptcp(cfg_host, cfg_port, cfg_sock_proto,
> &peer, fd_in, &winfo);

Is this 'fd_in' here valid if the previous 'fd_in = open(cfg_input,
O_RDONLY);' is deleted?

>  	if (fd < 0)
> @@ -1270,7 +1264,7 @@ int main_loop(void)
>  	if (cfg_cmsg_types.cmsg_enabled)
>  		apply_cmsg_types(fd, &cfg_cmsg_types);
>  
> -	if (cfg_input && !cfg_sockopt_types.mptfo) {
> +	if (cfg_input) {
>  		fd_in = open(cfg_input, O_RDONLY);
>  		if (fd_in < 0)
>  			xerror("can't open %s:%d", cfg_input,
> errno);

[PATCH 3/3] selftests: mptcp: Clean up resources properly in main_loop

[Cong Liu]

Add proper cleanup of resources (file descriptors and address info)
in error paths to prevent resource leaks.

Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4d4ea4627daa..e82fde0411b2 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1271,8 +1271,13 @@ int main_loop(void)
 	}
 
 	ret = copyfd_io(fd_in, fd, 1, 0, &winfo);
-	if (ret)
+	if (ret) {
+		close(fd);
+		if (cfg_input)
+			close(fd_in);
+		freeaddrinfo(peer);
 		return ret;
+	}
 
 	if (cfg_truncate > 0) {
 		xdisconnect(fd, peer->ai_addrlen);
@@ -1291,6 +1296,7 @@ int main_loop(void)
 		goto again;
 	} else {
 		close(fd);
+		freeaddrinfo(peer);
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH 3/3] selftests: mptcp: Clean up resources properly in main_loop

[Geliang Tang]

cc mptcp only.

On Mon, 2025-01-13 at 16:52 +0800, Cong Liu wrote:
> Add proper cleanup of resources (file descriptors and address info)
> in error paths to prevent resource leaks.
> 
> Signed-off-by: Cong Liu <liucong2@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 4d4ea4627daa..e82fde0411b2 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1271,8 +1271,13 @@ int main_loop(void)
>  	}
>  
>  	ret = copyfd_io(fd_in, fd, 1, 0, &winfo);
> -	if (ret)
> +	if (ret) {
> +		close(fd);
> +		if (cfg_input)
> +			close(fd_in);

Should 'listensock' be closed here?

> +		freeaddrinfo(peer);
>  		return ret;
> +	}
>  
>  	if (cfg_truncate > 0) {
>  		xdisconnect(fd, peer->ai_addrlen);
> @@ -1291,6 +1296,7 @@ int main_loop(void)
>  		goto again;
>  	} else {
>  		close(fd);

Should 'fd_in' be closed here? If so, is it possible to use a 'goto
out;' to reduce duplication of code?

Thanks,
-Geliang

> +		freeaddrinfo(peer);
>  	}
>  
>  	return 0;

Re: [PATCH 0/3] selftests: mptcp: Fix various issues in main_loop

[MPTCP CI]

Hi Cong,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 5 failed test(s): mptcp_connect_mmap selftest_mptcp_connect selftest_mptcp_join selftest_mptcp_sockopt selftest_simult_flows 🔴
- KVM Validation: debug: Unstable: 5 failed test(s): mptcp_connect_mmap selftest_mptcp_connect selftest_mptcp_join selftest_mptcp_sockopt selftest_simult_flows 🔴
- 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/12751035845

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d2149d21eec6
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=924794


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 0/3] selftests: mptcp: Fix various issues in main_loop

[MPTCP CI]

Hi Cong,

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): Unstable: 1 failed test(s): bpftest_test_progs_mptcp 🔴
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12750992738

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ae5a93603d19
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=924794


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 0/3] selftests: mptcp: Fix various issues in main_loop

[Matthieu Baerts]

Hi Cong Liu,

On 13/01/2025 09:52, Cong Liu wrote:
> Fix several issues in the mptcp connect test's main_loop function.
> 
>  - Fix a bug where the wrong file descriptor was being checked for errors
>  - Fix the input file descriptor lifecycle in the reconnection loop to
>    prevent use of invalid fd
>  - Add proper resource cleanup in error paths

Thank you for these fixes!

Please note that when sending patches to the Netdev mailing list, it is
asked to follow some specific rules, e.g.

- designate your patch to a tree - [PATCH net] or [PATCH net-next]
- for fixes the Fixes: tag is required, regardless of the tree

https://docs.kernel.org/process/maintainer-netdev.html

If I'm not mistaken, it looks like you have here fixes for net, and the
Fixes tag is missing.

Do you mind sending a v2 with this being fixed please? Because these
fixes are not urgent, do you mind sending them only to the MPTCP ML for
the moment, and not to the other ones (and no other people in CC).

> Cong Liu (3):
>   selftests: mptcp: Fix incorrect file descriptor check in main_loop
>   selftests: mptcp: Fix input fd lifecycle in reconnection loop
>   selftests: mptcp: Clean up resources properly in main_loop
> 
>  .../selftests/net/mptcp/mptcp_connect.c        | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> 
> base-commit: 2b88851f583d3c4e40bcd40cfe1965241ec229dd

Note that this base-commit doesn't seem to exist. Because of that, our
MPTCP CI was not able to automatically apply this commit.

Talking about our CI, it looks like that 'mptcp_connect' now crashes in
some cases with:

  free(): double free detected in tcache 2

Do you mind checking this please?

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

pw-bot: cr

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

[PATCH v2] selftests: mptcp: Fix incorrect file descriptor check in main_loop

[Cong Liu]

Fix a bug where the code was checking the wrong file descriptor
when opening the input file. The code was checking 'fd' instead
of 'fd_in', which could lead to incorrect error handling.

Fixes: ca7ae8916043 ("selftests: mptcp: mptfo Initiator/Listener")
Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4209b9569039..31f4c5618569 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1249,7 +1249,7 @@ int main_loop(void)
 
 	if (cfg_input && cfg_sockopt_types.mptfo) {
 		fd_in = open(cfg_input, O_RDONLY);
-		if (fd < 0)
+		if (fd_in < 0)
 			xerror("can't open %s:%d", cfg_input, errno);
 	}
 
@@ -1272,7 +1272,7 @@ int main_loop(void)
 
 	if (cfg_input && !cfg_sockopt_types.mptfo) {
 		fd_in = open(cfg_input, O_RDONLY);
-		if (fd < 0)
+		if (fd_in < 0)
 			xerror("can't open %s:%d", cfg_input, errno);
 	}
 
-- 
2.43.0


When I tried to solve the problem where fd_in was closed in the again
code block but not reopened in certain scenarios, I encountered some 
issues. Here's my code, but it causes the disconnect test to fail and I'm
not sure how to fix this. 

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 31f4c5618569..fbcb6bf6500c 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1247,7 +1247,7 @@ int main_loop(void)
        struct addrinfo *peer;
        struct wstate winfo;
 
-       if (cfg_input && cfg_sockopt_types.mptfo) {
+       if (cfg_input) {
                fd_in = open(cfg_input, O_RDONLY);
                if (fd_in < 0)
                        xerror("can't open %s:%d", cfg_input, errno);
@@ -1270,12 +1270,6 @@ int main_loop(void)
        if (cfg_cmsg_types.cmsg_enabled)
                apply_cmsg_types(fd, &cfg_cmsg_types);
 
-       if (cfg_input && !cfg_sockopt_types.mptfo) {
-               fd_in = open(cfg_input, O_RDONLY);
-               if (fd_in < 0)
-                       xerror("can't open %s:%d", cfg_input, errno);
-       }
-
        ret = copyfd_io(fd_in, fd, 1, 0, &winfo);
        if (ret)
                return ret;
@@ -1291,8 +1285,6 @@ int main_loop(void)
                set_nonblock(fd, false);
                if (connect(fd, peer->ai_addr, peer->ai_addrlen))
                        xerror("can't reconnect: %d", errno);
-               if (cfg_input)
-                       close(fd_in);
                memset(&winfo, 0, sizeof(winfo));
                goto again;
        } else {




INFO: disconnect
63 ns1 MPTCP -> ns1 (10.0.1.1:20001      ) MPTCP     (duration   103ms) [FAIL] file received by server does not match (in, out):
-rw-r--r-- 1 root root 18548982  1月 16 16:18 /tmp/tmp.mBbh37L7rj.disconnect
Trailing bytes are: 
�ǫ��r�_���m:�!d��v��Pv��-rw------- 1 root root 6182994  1月 16 16:18 /tmp/tmp.mJRvCLPpUi
Trailing bytes are: 
�ǫ��r�_���m:�!d��v��Pv��64 ns1 MPTCP -> ns1 (dead:beef:1::1:20002) MPTCP     (duration   149ms) [FAIL] file received by server does not match (in, out):
-rw-r--r-- 1 root root 18548982  1月 16 16:18 /tmp/tmp.mBbh37L7rj.disconnect
Trailing bytes are: 
�ǫ��r�_���m:�!d��v��Pv��-rw------- 1 root root 6182994  1月 16 16:18 /tmp/tmp.mJRvCLPpUi
Trailing bytes are: 
�ǫ��r�_���m:�!d��v��Pv��[FAIL] Tests of the full disconnection have failed
Time: 49 seconds

TAP version 13

Re: [PATCH v2] selftests: mptcp: Fix incorrect file descriptor check in main_loop

[Matthieu Baerts]

Hi Cong Liu,

(-Cc everybody, except the MPTCP ML)

Thank you for the v2.

On 16/01/2025 09:54, Cong Liu wrote:
> Fix a bug where the code was checking the wrong file descriptor
> when opening the input file. The code was checking 'fd' instead
> of 'fd_in', which could lead to incorrect error handling.
> 
> Fixes: ca7ae8916043 ("selftests: mptcp: mptfo Initiator/Listener")

I think the first issue has been introduced in 05be5e273c84 ("selftests:
mptcp: add disconnect tests"), then in the one you mentioned. Because it
is just in the selftests, feel free to add the older one, or both.

> Signed-off-by: Cong Liu <liucong2@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 4209b9569039..31f4c5618569 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1249,7 +1249,7 @@ int main_loop(void)
>  
>  	if (cfg_input && cfg_sockopt_types.mptfo) {
>  		fd_in = open(cfg_input, O_RDONLY);
> -		if (fd < 0)
> +		if (fd_in < 0)
>  			xerror("can't open %s:%d", cfg_input, errno);
>  	}
>  
> @@ -1272,7 +1272,7 @@ int main_loop(void)
>  
>  	if (cfg_input && !cfg_sockopt_types.mptfo) {
>  		fd_in = open(cfg_input, O_RDONLY);
> -		if (fd < 0)
> +		if (fd_in < 0)
>  			xerror("can't open %s:%d", cfg_input, errno);
>  	}

The CI is not able to apply your patch. On which base are you?
Do you mind using our 'export' branch from our GitHub repository?

> When I tried to solve the problem where fd_in was closed in the again
> code block but not reopened in certain scenarios, I encountered some 
> issues. Here's my code, but it causes the disconnect test to fail and I'm
> not sure how to fix this. 

Some fixes have been recently applied around the 'disconnect' tests,
maybe this issue has already been fixed?

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