MPTCP Join "fastclose server" selftest is sometimes failing because the
client output file doesn't have the expected size, e.g. 296B instead of
1024B.

When looking at a packet trace when this happens, the server sent the
expected 1024B in two parts -- 100B, then 924B -- then the MP_FASTCLOSE.
It is then strange to see the client only receiving 296B, which would
mean it only got a part of the second packet. The problem is then not on
the networking side, but rather on the data reception side.

When mptcp_connect is launched with '-f -1', it means the connection
might stop before having sent everything, because a reset has been
received. When this happens, the program was directly stopped. But it is
also possible there are still some data to read, simply because the
previous 'read' step was done with a buffer smaller than the pending
data, see do_rnd_read(). In this case, it is important to read what's
left in the kernel buffers before stopping without error like before.

SIGPIPE is now ignored, not to quit the app before having read
everything.

Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2:
 - Also catch EPIPE, and ignore SIGPIPE
---
 tools/testing/selftests/net/mptcp/mptcp_connect.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
index c030b08a7195..404a77bf366a 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
@@ -710,8 +710,14 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 
 				bw = do_rnd_write(peerfd, winfo->buf + winfo->off, winfo->len);
 				if (bw < 0) {
-					if (cfg_rcv_trunc)
-						return 0;
+					/* expected reset, continue to read */
+					if (cfg_rcv_trunc &&
+					    (errno == ECONNRESET ||
+					     errno == EPIPE)) {
+						fds.events &= ~POLLOUT;
+						continue;
+					}
+
 					perror("write");
 					return 111;
 				}
@@ -737,8 +743,10 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
 		}
 
 		if (fds.revents & (POLLERR | POLLNVAL)) {
-			if (cfg_rcv_trunc)
-				return 0;
+			if (cfg_rcv_trunc) {
+				fds.events &= ~(POLLERR | POLLNVAL);
+				continue;
+			}
 			fprintf(stderr, "Unexpected revents: "
 				"POLLERR/POLLNVAL(%x)\n", fds.revents);
 			return 5;
@@ -1441,7 +1449,7 @@ static void parse_opts(int argc, char **argv)
 			 */
 			if (cfg_truncate < 0) {
 				cfg_rcv_trunc = true;
-				signal(SIGPIPE, handle_signal);
+				signal(SIGPIPE, SIG_IGN);
 			}
 			break;
 		case 'j':

-- 
2.51.0

Hi Matt,

On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
> MPTCP Join "fastclose server" selftest is sometimes failing because
> the
> client output file doesn't have the expected size, e.g. 296B instead
> of
> 1024B.

Thanks for this fix. After repeatedly running the fastclose tests,
patches 5 and 6 have indeed eliminated the flaky failures.

> 
> When looking at a packet trace when this happens, the server sent the
> expected 1024B in two parts -- 100B, then 924B -- then the
> MP_FASTCLOSE.
> It is then strange to see the client only receiving 296B, which would
> mean it only got a part of the second packet. The problem is then not
> on
> the networking side, but rather on the data reception side.
> 
> When mptcp_connect is launched with '-f -1', it means the connection
> might stop before having sent everything, because a reset has been
> received. When this happens, the program was directly stopped. But it
> is
> also possible there are still some data to read, simply because the
> previous 'read' step was done with a buffer smaller than the pending
> data, see do_rnd_read(). In this case, it is important to read what's
> left in the kernel buffers before stopping without error like before.
> 
> SIGPIPE is now ignored, not to quit the app before having read
> everything.
> 
> Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose
> test-cases")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> v2:
>  - Also catch EPIPE, and ignore SIGPIPE
> ---
>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 18
> +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index c030b08a7195..404a77bf366a 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -710,8 +710,14 @@ static int copyfd_io_poll(int infd, int peerfd,
> int outfd,
>  
>  				bw = do_rnd_write(peerfd, winfo->buf
> + winfo->off, winfo->len);
>  				if (bw < 0) {
> -					if (cfg_rcv_trunc)
> -						return 0;
> +					/* expected reset, continue
> to read */
> +					if (cfg_rcv_trunc &&
> +					    (errno == ECONNRESET ||
> +					     errno == EPIPE)) {
> +						fds.events &=
> ~POLLOUT;
> +						continue;
> +					}

We're adjusting the behavior of the falseclose tests here and need to
ensure it doesn't affect other tests. I'm considering adding a new
config like cfg_rcv_ign to encapsulate these changes within an if
(cfg_rcv_ign) block, and only enable this new config in the falseclose
tests.

WDYT?

Thanks,
-Geliang

> +
>  					perror("write");
>  					return 111;
>  				}
> @@ -737,8 +743,10 @@ static int copyfd_io_poll(int infd, int peerfd,
> int outfd,
>  		}
>  
>  		if (fds.revents & (POLLERR | POLLNVAL)) {
> -			if (cfg_rcv_trunc)
> -				return 0;
> +			if (cfg_rcv_trunc) {
> +				fds.events &= ~(POLLERR | POLLNVAL);
> +				continue;
> +			}
>  			fprintf(stderr, "Unexpected revents: "
>  				"POLLERR/POLLNVAL(%x)\n",
> fds.revents);
>  			return 5;
> @@ -1441,7 +1449,7 @@ static void parse_opts(int argc, char **argv)
>  			 */
>  			if (cfg_truncate < 0) {
>  				cfg_rcv_trunc = true;
> -				signal(SIGPIPE, handle_signal);
> +				signal(SIGPIPE, SIG_IGN);
>  			}
>  			break;
>  		case 'j':
> 

Hi Geliang,

On 03/11/2025 08:13, Geliang Tang wrote:
> Hi Matt,
> 
> On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
>> MPTCP Join "fastclose server" selftest is sometimes failing because
>> the
>> client output file doesn't have the expected size, e.g. 296B instead
>> of
>> 1024B.
> 
> Thanks for this fix. After repeatedly running the fastclose tests,
> patches 5 and 6 have indeed eliminated the flaky failures.

Thank you for the tests.

>> When looking at a packet trace when this happens, the server sent the
>> expected 1024B in two parts -- 100B, then 924B -- then the
>> MP_FASTCLOSE.
>> It is then strange to see the client only receiving 296B, which would
>> mean it only got a part of the second packet. The problem is then not
>> on
>> the networking side, but rather on the data reception side.
>>
>> When mptcp_connect is launched with '-f -1', it means the connection
>> might stop before having sent everything, because a reset has been
>> received. When this happens, the program was directly stopped. But it
>> is
>> also possible there are still some data to read, simply because the
>> previous 'read' step was done with a buffer smaller than the pending
>> data, see do_rnd_read(). In this case, it is important to read what's
>> left in the kernel buffers before stopping without error like before.
>>
>> SIGPIPE is now ignored, not to quit the app before having read
>> everything.
>>
>> Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose
>> test-cases")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> v2:
>>  - Also catch EPIPE, and ignore SIGPIPE
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 18
>> +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> index c030b08a7195..404a77bf366a 100644
>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> @@ -710,8 +710,14 @@ static int copyfd_io_poll(int infd, int peerfd,
>> int outfd,
>>  
>>  				bw = do_rnd_write(peerfd, winfo->buf
>> + winfo->off, winfo->len);
>>  				if (bw < 0) {
>> -					if (cfg_rcv_trunc)
>> -						return 0;
>> +					/* expected reset, continue
>> to read */
>> +					if (cfg_rcv_trunc &&
>> +					    (errno == ECONNRESET ||
>> +					     errno == EPIPE)) {
>> +						fds.events &=
>> ~POLLOUT;
>> +						continue;
>> +					}
> 
> We're adjusting the behavior of the falseclose tests here and need to
> ensure it doesn't affect other tests. I'm considering adding a new
> config like cfg_rcv_ign to encapsulate these changes within an if
> (cfg_rcv_ign) block, and only enable this new config in the falseclose
> tests.
> 
> WDYT?

I don't think we need an extra option. For the moment, when one side
truncates the output ("-f XXX", which will cause a fastclose), the other
side will be launched with "-f -1". In this case, "cfg_rcv_trunc" will
be set, and we only change the behaviour for that.

It looks like this "-f" option is only use for the fastclose tests, and
both peers have a "-f". So we can keep this as it is, no?

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

Hi Matt,

On Mon, 2025-11-03 at 13:12 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/11/2025 08:13, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
> > > MPTCP Join "fastclose server" selftest is sometimes failing
> > > because
> > > the
> > > client output file doesn't have the expected size, e.g. 296B
> > > instead
> > > of
> > > 1024B.
> > 
> > Thanks for this fix. After repeatedly running the fastclose tests,
> > patches 5 and 6 have indeed eliminated the flaky failures.
> 
> Thank you for the tests.
> 
> > > When looking at a packet trace when this happens, the server sent
> > > the
> > > expected 1024B in two parts -- 100B, then 924B -- then the
> > > MP_FASTCLOSE.
> > > It is then strange to see the client only receiving 296B, which
> > > would
> > > mean it only got a part of the second packet. The problem is then
> > > not
> > > on
> > > the networking side, but rather on the data reception side.
> > > 
> > > When mptcp_connect is launched with '-f -1', it means the
> > > connection
> > > might stop before having sent everything, because a reset has
> > > been
> > > received. When this happens, the program was directly stopped.
> > > But it
> > > is
> > > also possible there are still some data to read, simply because
> > > the
> > > previous 'read' step was done with a buffer smaller than the
> > > pending
> > > data, see do_rnd_read(). In this case, it is important to read
> > > what's
> > > left in the kernel buffers before stopping without error like
> > > before.
> > > 
> > > SIGPIPE is now ignored, not to quit the app before having read
> > > everything.
> > > 
> > > Fixes: 6bf41020b72b ("selftests: mptcp: update and extend
> > > fastclose
> > > test-cases")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > v2:
> > >  - Also catch EPIPE, and ignore SIGPIPE
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_connect.c | 18
> > > +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > index c030b08a7195..404a77bf366a 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > @@ -710,8 +710,14 @@ static int copyfd_io_poll(int infd, int
> > > peerfd,
> > > int outfd,
> > >  
> > >  				bw = do_rnd_write(peerfd, winfo-
> > > >buf
> > > + winfo->off, winfo->len);
> > >  				if (bw < 0) {
> > > -					if (cfg_rcv_trunc)
> > > -						return 0;
> > > +					/* expected reset,
> > > continue
> > > to read */
> > > +					if (cfg_rcv_trunc &&
> > > +					    (errno == ECONNRESET
> > > ||
> > > +					     errno == EPIPE)) {
> > > +						fds.events &=
> > > ~POLLOUT;
> > > +						continue;
> > > +					}
> > 
> > We're adjusting the behavior of the falseclose tests here and need
> > to
> > ensure it doesn't affect other tests. I'm considering adding a new
> > config like cfg_rcv_ign to encapsulate these changes within an if
> > (cfg_rcv_ign) block, and only enable this new config in the
> > falseclose
> > tests.
> > 
> > WDYT?
> 
> I don't think we need an extra option. For the moment, when one side
> truncates the output ("-f XXX", which will cause a fastclose), the
> other
> side will be launched with "-f -1". In this case, "cfg_rcv_trunc"
> will
> be set, and we only change the behaviour for that.
> 
> It looks like this "-f" option is only use for the fastclose tests,
> and
> both peers have a "-f". So we can keep this as it is, no?

Yes, you're right. Let's keep this patch as is.

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> 
> Cheers,
> Matt