[v2] selftests: mptcp: avoid spurious errors on TCP disconnect

[PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

Posted by Matthieu Baerts (NGI0) 4 days, 11 hours ago

IO errors were correctly printed to stderr, and propagated up to the
main loop for the server side, but the returned value was ignored. As a
consequence, the program for the listener side was no longer exiting
with an error code in case of IO issues.

Because of that, some issues might not have been seen. But very likely,
most issues either had an effect on the client side, or the file
transfer was not the expected one, e.g. the connection got reset before
the end. Still, it is better to fix this.

The main consequence of this issue is the error that was reported by the
selftests: the received and sent files were different, and the MIB
counters were not printed. Also, when such errors happened during the
'disconnect' tests, the program tried to continue until the timeout.

Now when an IO error is detected, the program exits directly with an
error.

Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd624b98f0bfd18 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
 	salen = sizeof(ss);
 	remotesock = accept(listensock, (struct sockaddr *)&ss, &salen);
 	if (remotesock >= 0) {
+		int err;
+
 		maybe_close(listensock);
 		check_sockaddr(pf, &ss, salen);
 		check_getpeername(remotesock, &ss, salen);
@@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
 		SOCK_TEST_TCPULP(remotesock, 0);
 
 		memset(&winfo, 0, sizeof(winfo));
-		copyfd_io(fd, remotesock, 1, true, &winfo);
+		err = copyfd_io(fd, remotesock, 1, true, &winfo);
+		if (err)
+			return err;
 	} else {
 		perror("accept");
 		return 1;

-- 
2.51.0

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

Posted by Geliang Tang 4 days, 5 hours ago

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> IO errors were correctly printed to stderr, and propagated up to the
> main loop for the server side, but the returned value was ignored. As
> a
> consequence, the program for the listener side was no longer exiting
> with an error code in case of IO issues.
> 
> Because of that, some issues might not have been seen. But very
> likely,
> most issues either had an effect on the client side, or the file
> transfer was not the expected one, e.g. the connection got reset
> before
> the end. Still, it is better to fix this.
> 
> The main consequence of this issue is the error that was reported by
> the
> selftests: the received and sent files were different, and the MIB
> counters were not printed. Also, when such errors happened during the
> 'disconnect' tests, the program tried to continue until the timeout.
> 
> Now when an IO error is detected, the program exits directly with an
> error.
> 
> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index
> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd
> 624b98f0bfd18 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>  	salen = sizeof(ss);
>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
> &salen);
>  	if (remotesock >= 0) {
> +		int err;
> +
>  		maybe_close(listensock);
>  		check_sockaddr(pf, &ss, salen);
>  		check_getpeername(remotesock, &ss, salen);
> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>  		SOCK_TEST_TCPULP(remotesock, 0);
>  
>  		memset(&winfo, 0, sizeof(winfo));
> -		copyfd_io(fd, remotesock, 1, true, &winfo);
> +		err = copyfd_io(fd, remotesock, 1, true, &winfo);
> +		if (err)
> +			return err;

The file descriptor (fd) should be closed before returning.

Thanks,
-Geliang

>  	} else {
>  		perror("accept");
>  		return 1;

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

Posted by Matthieu Baerts 3 days, 15 hours ago

Hi Geliang,

On 06/09/2025 01:49, Geliang Tang wrote:
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> IO errors were correctly printed to stderr, and propagated up to the
>> main loop for the server side, but the returned value was ignored. As
>> a
>> consequence, the program for the listener side was no longer exiting
>> with an error code in case of IO issues.
>>
>> Because of that, some issues might not have been seen. But very
>> likely,
>> most issues either had an effect on the client side, or the file
>> transfer was not the expected one, e.g. the connection got reset
>> before
>> the end. Still, it is better to fix this.
>>
>> The main consequence of this issue is the error that was reported by
>> the
>> selftests: the received and sent files were different, and the MIB
>> counters were not printed. Also, when such errors happened during the
>> 'disconnect' tests, the program tried to continue until the timeout.
>>
>> Now when an IO error is detected, the program exits directly with an
>> error.
>>
>> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> index
>> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd
>> 624b98f0bfd18 100644
>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>>  	salen = sizeof(ss);
>>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
>> &salen);
>>  	if (remotesock >= 0) {
>> +		int err;
>> +
>>  		maybe_close(listensock);
>>  		check_sockaddr(pf, &ss, salen);
>>  		check_getpeername(remotesock, &ss, salen);
>> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>>  		SOCK_TEST_TCPULP(remotesock, 0);
>>  
>>  		memset(&winfo, 0, sizeof(winfo));
>> -		copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		err = copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		if (err)
>> +			return err;
> 
> The file descriptor (fd) should be closed before returning.

I do agree it would be better to do so, but when you look in this file,
most errors paths don't close the FD, because that's the exit path.

'fd' will need to be closed, 'remotesock' as well, but same when it was
not possible to create 'fd'. Also, I guess 'listensock' is never closed.
Honestly, I don't think we need to increase the complexity, and close
them, just for the tests. It would be different if the FD were reused
later on, but I don't think we need to spend time on these details when
this test program is about to close.

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

Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

Posted by Geliang Tang 3 hours ago

Hi Matt,

On Sat, 2025-09-06 at 15:54 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/09/2025 01:49, Geliang Tang wrote:
> > On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > IO errors were correctly printed to stderr, and propagated up to
> > > the
> > > main loop for the server side, but the returned value was
> > > ignored. As
> > > a
> > > consequence, the program for the listener side was no longer
> > > exiting
> > > with an error code in case of IO issues.
> > > 
> > > Because of that, some issues might not have been seen. But very
> > > likely,
> > > most issues either had an effect on the client side, or the file
> > > transfer was not the expected one, e.g. the connection got reset
> > > before
> > > the end. Still, it is better to fix this.
> > > 
> > > The main consequence of this issue is the error that was reported
> > > by
> > > the
> > > selftests: the received and sent files were different, and the
> > > MIB
> > > counters were not printed. Also, when such errors happened during
> > > the
> > > 'disconnect' tests, the program tried to continue until the
> > > timeout.
> > > 
> > > Now when an IO error is detected, the program exits directly with
> > > an
> > > error.
> > > 
> > > Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > index
> > > 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac3
> > > 19fd
> > > 624b98f0bfd18 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
> > >  	salen = sizeof(ss);
> > >  	remotesock = accept(listensock, (struct sockaddr *)&ss,
> > > &salen);
> > >  	if (remotesock >= 0) {
> > > +		int err;
> > > +
> > >  		maybe_close(listensock);
> > >  		check_sockaddr(pf, &ss, salen);
> > >  		check_getpeername(remotesock, &ss, salen);
> > > @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
> > >  		SOCK_TEST_TCPULP(remotesock, 0);
> > >  
> > >  		memset(&winfo, 0, sizeof(winfo));
> > > -		copyfd_io(fd, remotesock, 1, true, &winfo);
> > > +		err = copyfd_io(fd, remotesock, 1, true,
> > > &winfo);
> > > +		if (err)
> > > +			return err;
> > 
> > The file descriptor (fd) should be closed before returning.
> 
> I do agree it would be better to do so, but when you look in this
> file,
> most errors paths don't close the FD, because that's the exit path.
> 
> 'fd' will need to be closed, 'remotesock' as well, but same when it
> was
> not possible to create 'fd'. Also, I guess 'listensock' is never
> closed.
> Honestly, I don't think we need to increase the complexity, and close
> them, just for the tests. It would be different if the FD were reused
> later on, but I don't think we need to spend time on these details
> when
> this test program is about to close.

What I mean here is not to return directly, something like:

'''
int err = 0;

if (remotesock >= 0) {
    ... ...
    err = copyfd_io(fd, remotesock, 1, true, &winfo);
}

if (cfg_input)
    close(fd);

if (!err && --cfg_repeat > 0)
    goto again;

return err;
'''

WDYT?

I will send a patch to close remotesock and listensock later.

Thanks,
-Geliang

> 
> Cheers,
> Matt

[PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible
[PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side
[PATCH mptcp-net v2 3/5] selftests: mptcp: avoid spurious errors on TCP disconnect
[PATCH mptcp-net v2 4/5] selftests: mptcp: print trailing bytes with od
[PATCH mptcp-net v2 5/5] selftests: mptcp: connect: print pcap suffix