[PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[Quanye Yang]

process_one_client() used a single read() and write() before waiting
for hangup. A SOCK_STREAM read() can return a short count when the
payload is segmented, so the server echoed only part of the data and
then blocked on FIN.

The client keeps reading until it has the full length and only then
closes the socket. The two sides wait on each other and the test
hangs until SIGALRM.

The server does not know the payload length, so echo each chunk until
read() returns 0. Retry short writes, and accumulate nread/nwritten for
the existing sockopt checks.

Fixes: ce9979129a0b ("selftests: mptcp: add mptcp getsockopt test cases")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/639
Signed-off-by: Quanye Yang <quanyeyang@proton.me>
---
Changes in v2:
- target mptcp-next instead of mptcp-net
- echo until hangup with goto; retry short writes; use nread/nwritten
- Link to v1: https://patch.msgid.link/20261017-fix-mptcp-issue-639-v1-1-0554d07ab8f8@proton.me
---
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 60 +++++++++++++----------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b6e58d936ebe..deb239058e79 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -638,7 +638,8 @@ static void connect_one_server(int fd, int pipefd)
 
 static void process_one_client(int fd, int pipefd)
 {
-	ssize_t ret, ret2, ret3;
+	size_t nread = 0, nwritten = 0;
+	ssize_t ret, n, w, off;
 	struct so_state s;
 	char buf[4096];
 
@@ -648,46 +649,53 @@ static void process_one_client(int fd, int pipefd)
 	ret = write(pipefd, "xmit", 4);
 	assert(ret == 4);
 
-	ret = read(fd, buf, sizeof(buf));
-	if (ret < 0)
+read_more:
+	n = read(fd, buf, sizeof(buf));
+	if (n < 0)
 		die_perror("read");
+	if (n == 0)
+		goto hangup;
+
+	for (off = 0; off < n; off += w) {
+		w = write(fd, buf + off, n - off);
+		if (w < 0)
+			die_perror("write");
+		if (w == 0)
+			xerror("write returned 0");
+	}
+	nread += n;
+	nwritten += n;
+	goto read_more;
 
-	assert(s.mptcpi_rcv_delta <= (uint64_t)ret);
+hangup:
+	assert(s.mptcpi_rcv_delta <= nread);
 
 	if (s.tcpi_rcv_delta)
-		assert(s.tcpi_rcv_delta == (uint64_t)ret);
-
-	ret2 = write(fd, buf, ret);
-	if (ret2 < 0)
-		die_perror("write");
-
-	/* wait for hangup */
-	ret3 = read(fd, buf, 1);
-	if (ret3 != 0)
-		xerror("expected EOF, got %lu", ret3);
+		assert(s.tcpi_rcv_delta <= nread);
 
-	do_getsockopts(&s, fd, ret, ret2);
-	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
+	do_getsockopts(&s, fd, nread, nwritten);
+	if (s.mptcpi_rcv_delta != (uint64_t)nread + 1)
 		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64 ", diff %" PRId64,
-		       s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - (ret + 1));
+		       s.mptcpi_rcv_delta, (uint64_t)nread + 1,
+		       s.mptcpi_rcv_delta - ((uint64_t)nread + 1));
 
 	/* be nice when running on top of older kernel */
 	if (s.pkt_stats_avail) {
-		if (s.last_sample.mptcpi_bytes_sent != ret2)
+		if (s.last_sample.mptcpi_bytes_sent != nwritten)
 			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)
+			       s.last_sample.mptcpi_bytes_sent, nwritten,
+			       s.last_sample.mptcpi_bytes_sent - nwritten);
+		if (s.last_sample.mptcpi_bytes_received != nread)
 			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)
+			       s.last_sample.mptcpi_bytes_received, nread,
+			       s.last_sample.mptcpi_bytes_received - nread);
+		if (s.last_sample.mptcpi_bytes_acked != nread)
 			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64
 			       ", diff %" PRId64,
-			       s.last_sample.mptcpi_bytes_acked, ret,
-			       s.last_sample.mptcpi_bytes_acked - ret);
+			       s.last_sample.mptcpi_bytes_acked, nread,
+			       s.last_sample.mptcpi_bytes_acked - nread);
 	}
 
 	close(fd);

---
base-commit: a22035be824449c4921ec406f6383089016a628c
change-id: 20261017-fix-mptcp-issue-639-96339a3d921a

Best regards,
--  
Quanye Yang <quanyeyang@proton.me>

[PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[Quanye Yang via B4 Relay]

From: Quanye Yang <quanyeyang@proton.me>

process_one_client() used a single read() and write() before waiting
for hangup. A SOCK_STREAM read() can return a short count when the
payload is segmented, so the server echoed only part of the data and
then blocked on FIN.

The client keeps reading until it has the full length and only then
closes the socket. The two sides wait on each other and the test
hangs until SIGALRM.

The server does not know the payload length, so echo each chunk until
read() returns 0. Retry short writes, and accumulate nread/nwritten for
the existing sockopt checks.

Fixes: ce9979129a0b ("selftests: mptcp: add mptcp getsockopt test cases")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/639
Signed-off-by: Quanye Yang <quanyeyang@proton.me>
---
Changes in v2:
- target mptcp-next instead of mptcp-net
- echo until hangup with goto; retry short writes; use nread/nwritten
- Link to v1: https://patch.msgid.link/20261017-fix-mptcp-issue-639-v1-1-0554d07ab8f8@proton.me
---
 tools/testing/selftests/net/mptcp/mptcp_sockopt.c | 60 +++++++++++++----------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
index b6e58d936ebe..deb239058e79 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
+++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
@@ -638,7 +638,8 @@ static void connect_one_server(int fd, int pipefd)
 
 static void process_one_client(int fd, int pipefd)
 {
-	ssize_t ret, ret2, ret3;
+	size_t nread = 0, nwritten = 0;
+	ssize_t ret, n, w, off;
 	struct so_state s;
 	char buf[4096];
 
@@ -648,46 +649,53 @@ static void process_one_client(int fd, int pipefd)
 	ret = write(pipefd, "xmit", 4);
 	assert(ret == 4);
 
-	ret = read(fd, buf, sizeof(buf));
-	if (ret < 0)
+read_more:
+	n = read(fd, buf, sizeof(buf));
+	if (n < 0)
 		die_perror("read");
+	if (n == 0)
+		goto hangup;
+
+	for (off = 0; off < n; off += w) {
+		w = write(fd, buf + off, n - off);
+		if (w < 0)
+			die_perror("write");
+		if (w == 0)
+			xerror("write returned 0");
+	}
+	nread += n;
+	nwritten += n;
+	goto read_more;
 
-	assert(s.mptcpi_rcv_delta <= (uint64_t)ret);
+hangup:
+	assert(s.mptcpi_rcv_delta <= nread);
 
 	if (s.tcpi_rcv_delta)
-		assert(s.tcpi_rcv_delta == (uint64_t)ret);
-
-	ret2 = write(fd, buf, ret);
-	if (ret2 < 0)
-		die_perror("write");
-
-	/* wait for hangup */
-	ret3 = read(fd, buf, 1);
-	if (ret3 != 0)
-		xerror("expected EOF, got %lu", ret3);
+		assert(s.tcpi_rcv_delta <= nread);
 
-	do_getsockopts(&s, fd, ret, ret2);
-	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
+	do_getsockopts(&s, fd, nread, nwritten);
+	if (s.mptcpi_rcv_delta != (uint64_t)nread + 1)
 		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64 ", diff %" PRId64,
-		       s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - (ret + 1));
+		       s.mptcpi_rcv_delta, (uint64_t)nread + 1,
+		       s.mptcpi_rcv_delta - ((uint64_t)nread + 1));
 
 	/* be nice when running on top of older kernel */
 	if (s.pkt_stats_avail) {
-		if (s.last_sample.mptcpi_bytes_sent != ret2)
+		if (s.last_sample.mptcpi_bytes_sent != nwritten)
 			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)
+			       s.last_sample.mptcpi_bytes_sent, nwritten,
+			       s.last_sample.mptcpi_bytes_sent - nwritten);
+		if (s.last_sample.mptcpi_bytes_received != nread)
 			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)
+			       s.last_sample.mptcpi_bytes_received, nread,
+			       s.last_sample.mptcpi_bytes_received - nread);
+		if (s.last_sample.mptcpi_bytes_acked != nread)
 			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64
 			       ", diff %" PRId64,
-			       s.last_sample.mptcpi_bytes_acked, ret,
-			       s.last_sample.mptcpi_bytes_acked - ret);
+			       s.last_sample.mptcpi_bytes_acked, nread,
+			       s.last_sample.mptcpi_bytes_acked - nread);
 	}
 
 	close(fd);

---
base-commit: a22035be824449c4921ec406f6383089016a628c
change-id: 20261017-fix-mptcp-issue-639-96339a3d921a

Best regards,
--  
Quanye Yang <quanyeyang@proton.me>

Re: [PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: mptcp: connect_one_server() passes chunk size to do_getsockopts()
- [Low] selftests: mptcp: process_one_client() passes size_t to PRIu64 in xerror()
--

commit 22c1d412b84bbca0005369450144b95d047ed072
Author: Quanye Yang <quanyeyang@proton.me>

selftests: mptcp: sockopt: echo until EOF on partial reads

This commit updates process_one_client() to loop and echo chunks until EOF
rather than issuing a single read() and write(), avoiding a hang when the
client payload arrives in multiple segments.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index b6e58d936ebe5..deb239058e796 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -638,7 +638,8 @@ static void connect_one_server(int fd, int pipefd)
>  
>  static void process_one_client(int fd, int pipefd)
>  {
> -	ssize_t ret, ret2, ret3;
> +	size_t nread = 0, nwritten = 0;
> +	ssize_t ret, n, w, off;
>  	struct so_state s;
>  	char buf[4096];
>  
> @@ -648,46 +649,53 @@ static void process_one_client(int fd, int pipefd)
>  	ret = write(pipefd, "xmit", 4);
>  	assert(ret == 4);
>  
> -	ret = read(fd, buf, sizeof(buf));
> -	if (ret < 0)
> +read_more:
> +	n = read(fd, buf, sizeof(buf));
> +	if (n < 0)
>  		die_perror("read");
> +	if (n == 0)
> +		goto hangup;
> +
> +	for (off = 0; off < n; off += w) {
> +		w = write(fd, buf + off, n - off);
> +		if (w < 0)
> +			die_perror("write");
> +		if (w == 0)
> +			xerror("write returned 0");
> +	}
> +	nread += n;
> +	nwritten += n;
> +	goto read_more;

[Severity: Medium]
When read() in process_one_client() returns a partial chunk n < len, the
server immediately echoes that partial chunk in a separate write() before
looping back to read_more for the remaining bytes.

When the client receives those echoed chunks across multiple read()
iterations in connect_one_server(), won't connect_one_server() then fail in
do_getsockopts()?

Looking at connect_one_server():

	total = 0;
	do {
		ret = read(fd, buf2 + total, sizeof(buf2) - total);
		if (ret < 0)
			die_perror("read");
		if (ret == 0) {
			eof = true;
			break;
		}

		total += ret;
	} while (total < len);
	...
	do_getsockopts(&s, fd, ret, ret);

When the loop takes more than one iteration, ret only holds the byte count
from the final read() call rather than the accumulated total. Passing ret to
do_getsockopts() causes do_getsockopt_mptcp_info() to abort on:

	assert(s->mi.mptcpi_write_seq + w == i.mptcpi_write_seq);

and causes do_getsockopt_tcp_info() to check tcpi_bytes_sent and
tcpi_bytes_received against the last chunk size ret instead of total. Should
connect_one_server() pass total instead of ret to do_getsockopts()?

> -
> -	assert(s.mptcpi_rcv_delta <= (uint64_t)ret);
> +hangup:
> +	assert(s.mptcpi_rcv_delta <= nread);
>  
>  	if (s.tcpi_rcv_delta)
> -		assert(s.tcpi_rcv_delta == (uint64_t)ret);
> -
> -	ret2 = write(fd, buf, ret);
> -	if (ret2 < 0)
> -		die_perror("write");
> -
> -	/* wait for hangup */
> -	ret3 = read(fd, buf, 1);
> -	if (ret3 != 0)
> -		xerror("expected EOF, got %lu", ret3);
> +		assert(s.tcpi_rcv_delta <= nread);
>  
> -	do_getsockopts(&s, fd, ret, ret2);
> -	if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
> +	do_getsockopts(&s, fd, nread, nwritten);
> +	if (s.mptcpi_rcv_delta != (uint64_t)nread + 1)
>  		xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64 ", diff %" PRId64,
> -		       s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - (ret + 1));
> +		       s.mptcpi_rcv_delta, (uint64_t)nread + 1,
> +		       s.mptcpi_rcv_delta - ((uint64_t)nread + 1));
>  
>  	/* be nice when running on top of older kernel */
>  	if (s.pkt_stats_avail) {
> -		if (s.last_sample.mptcpi_bytes_sent != ret2)
> +		if (s.last_sample.mptcpi_bytes_sent != nwritten)
>  			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)
> +			       s.last_sample.mptcpi_bytes_sent, nwritten,
> +			       s.last_sample.mptcpi_bytes_sent - nwritten);
> +		if (s.last_sample.mptcpi_bytes_received != nread)
>  			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)
> +			       s.last_sample.mptcpi_bytes_received, nread,
> +			       s.last_sample.mptcpi_bytes_received - nread);
> +		if (s.last_sample.mptcpi_bytes_acked != nread)
>  			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64
>  			       ", diff %" PRId64,
> -			       s.last_sample.mptcpi_bytes_acked, ret,
> -			       s.last_sample.mptcpi_bytes_acked - ret);
> +			       s.last_sample.mptcpi_bytes_acked, nread,
> +			       s.last_sample.mptcpi_bytes_acked - nread);
>  	}

[Severity: Low]
In process_one_client(), nread and nwritten are declared as size_t, which is
32 bits wide on 32-bit architectures, but they are passed directly without a
cast to xerror() for the PRIu64 format specifier before the 64-bit diff
argument.

Could these format strings use %zu for nwritten and nread (or cast them to
uint64_t as done above for mptcpi_rcv_delta) so vfprintf() does not read a
mismatched argument size on 32-bit targets?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-fix-mptcp-issue-639-v2-1-14a3d3746d2d@proton.me?part=1

Re: [PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[MPTCP CI]

Hi Quanye,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/39975743414

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


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-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This is an incomplete fix: connect_one_server() on the client
  side still has the same partial-read bug that the patch fixes in…
- [Medium] The commit message's account of the pre-patch failure does not
  match the pre-patch code.
- [Low] selftests: mptcp: process_one_client() passes size_t to PRIu64 in
  xerror()

Pre-existing issues:
- [Low] The patch did not introduce this problem, but it rewrote these
  exact argument lists.

--

selftests: mptcp: sockopt: echo until EOF on partial reads

This commit changes process_one_client() in the mptcp_sockopt selftest so
the server echoes data in a loop until read() returns 0, retrying short
writes. The accumulated nread/nwritten counts replace the single read and
write sizes in the existing sockopt checks.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/639

> The client keeps reading until it has the full length and only then
> closes the socket. The two sides wait on each other and the test
> hangs until SIGALRM.

[Severity: Medium]

Does this match what the old code would do? Before this patch,
process_one_client() ran this after the partial echo:

	/* wait for hangup */
	ret3 = read(fd, buf, 1);
	if (ret3 != 0)
		xerror("expected EOF, got %lu", ret3);

The client had already queued all len bytes with one write(). Once the
remaining bytes reached the server, this read() would return 1 and the
server would exit through xerror().

That closes the socket, so the client's read() returns. Both processes
would be reaped well before alarm(15).

A mutual hang seems possible only if the remaining bytes never reach the
server's receive queue. In that case the new loop would block in the
same way here:

	n = read(fd, buf, sizeof(buf));

and the client would keep waiting for len bytes.

Could the hang reported in issue #639 have a different cause, perhaps in
the client side accounting in connect_one_server() or in the kernel? If
so, is the Closes: tag right for this patch?

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index b6e58d936ebe5..deb239058e796 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -638,7 +638,8 @@ static void connect_one_server(int fd, int pipefd)
>  
>  static void process_one_client(int fd, int pipefd)
>  {
> -	ssize_t ret, ret2, ret3;
> +	size_t nread = 0, nwritten = 0;
> +	ssize_t ret, n, w, off;
>  	struct so_state s;
>  	char buf[4096];
>  
> @@ -648,46 +649,53 @@ static void process_one_client(int fd, int pipefd)
>  	ret = write(pipefd, "xmit", 4);
>  	assert(ret == 4);
>  
> -	ret = read(fd, buf, sizeof(buf));
> -	if (ret < 0)
> +read_more:
> +	n = read(fd, buf, sizeof(buf));
> +	if (n < 0)
>  		die_perror("read");
> +	if (n == 0)
> +		goto hangup;
> +
> +	for (off = 0; off < n; off += w) {
> +		w = write(fd, buf + off, n - off);
> +		if (w < 0)
> +			die_perror("write");
> +		if (w == 0)
> +			xerror("write returned 0");
> +	}
> +	nread += n;
> +	nwritten += n;
> +	goto read_more;

[Severity: High]

Does connect_one_server() on the client side still have the same
short-read problem? Its read loop adds the echoed bytes up in total, but
the sockopt check after it passes the size of the last read() chunk:

tools/testing/selftests/net/mptcp/mptcp_sockopt.c:connect_one_server() {
    ...
	total = 0;
	do {
		ret = read(fd, buf2 + total, sizeof(buf2) - total);
		...
		total += ret;
	} while (total < len);
    ...
	do_getsockopts(&s, fd, ret, ret);
    ...
}

With this loop, a short first read() on the server makes the echo go out
in several write() calls. The client is then likely to receive it in
more than one read(), which leaves ret holding only the final chunk.

do_getsockopt_mptcp_info() would then run this check with w smaller than
the len bytes the client wrote:

	assert(s->mi.mptcpi_write_seq + w == i.mptcpi_write_seq);

That assert fails and the client dies with SIGABRT. Without the assert,
do_getsockopt_tcp_info() compares tcpi_bytes_sent/received against w/r
and would call xerror() after its retries.

So the segmented-payload case from the commit message still looks like
it fails the test, only in a different way.

Should connect_one_server() call do_getsockopts(&s, fd, total, total)
instead? The other checks in that function already use total.

[ ... ]

>  	/* be nice when running on top of older kernel */
>  	if (s.pkt_stats_avail) {
> -		if (s.last_sample.mptcpi_bytes_sent != ret2)
> +		if (s.last_sample.mptcpi_bytes_sent != nwritten)
>  			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)
> +			       s.last_sample.mptcpi_bytes_sent, nwritten,
> +			       s.last_sample.mptcpi_bytes_sent - nwritten);
> +		if (s.last_sample.mptcpi_bytes_received != nread)
>  			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)
> +			       s.last_sample.mptcpi_bytes_received, nread,
> +			       s.last_sample.mptcpi_bytes_received - nread);
> +		if (s.last_sample.mptcpi_bytes_acked != nread)
>  			xerror("mptcpi_bytes_acked %" PRIu64 ", expect %" PRIu64
>  			       ", diff %" PRId64,
> -			       s.last_sample.mptcpi_bytes_acked, ret,
> -			       s.last_sample.mptcpi_bytes_acked - ret);
> +			       s.last_sample.mptcpi_bytes_acked, nread,
> +			       s.last_sample.mptcpi_bytes_acked - nread);

[Severity: Low]
nread and nwritten are declared as size_t, but these three xerror()
calls pass them straight to an "expect %" PRIu64 conversion. On 32-bit
(ILP32) builds size_t is 4 bytes while PRIu64 consumes 8. That means
vfprintf() in xerror() reads past nwritten/nread and picks up the wrong
bytes for the PRId64 diff that follows.

The diff argument itself is fine. Subtracting a size_t from a uint64_t
promotes the result to 64 bits, so only the bare nread/nwritten
argument is the wrong width.

The mptcpi_rcv_delta xerror() just above already casts with
(uint64_t)nread + 1, so the patch is not consistent here. xerror() has
no printf format attribute, so the compiler does not warn about this.

The old code had the same mismatch with ret/ret2, so this is not a new
problem. But these argument lists are being rewritten anyway, and when
one of the mptcpi_bytes_sent/received/acked checks fails on a 32-bit
machine, the expect and diff values would be garbage. That is exactly
when the output matters.

Could you either cast nwritten and nread to (uint64_t) in these calls,
or declare the two counters as uint64_t? Is it also worth adding a
format(printf, 1, 2) attribute to xerror() so the compiler catches
this kind of mismatch?

[Severity: Low]

This isn't a bug introduced by this patch, but these argument lists are
being rewritten anyway. nwritten and nread are size_t, while the format
strings use PRIu64.

The mptcpi_rcv_delta xerror() above gets a (uint64_t) cast, but these
three calls do not. xerror() has no printf format attribute, so the
compiler gives no warning.

On 32-bit builds, would the expect and diff values print as garbage when
one of these checks fails? The old code had the same mismatch with
ret/ret2.

>  	}
>  
>  	close(fd);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-fix-mptcp-issue-639-v2-1-14a3d3746d2d%40proton.me

Re: [PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[quanyeyang]

Hi,

On Fri, 23 Oct 2026, sashiko-bot wrote:
> [Medium] connect_one_server() passes chunk size to do_getsockopts()
> ...
> Should connect_one_server() pass total instead of ret to do_getsockopts()?

Yes. That is pre-existing in connect_one_server() and is not part of this
change. It is tracked as issue 638 and already sent separately:

  https://lore.kernel.org/all/20261015-fix-mptcp-issue-638-v1-1-5de999ad601c@proton.me/

This patch only updates process_one_client(), which already accumulates
nread/nwritten.

> [Low] process_one_client() passes size_t to PRIu64 in xerror()

Agreed on 32-bit. The existing sockopt checks already pass ssize_t the
same way. I can add the uint64_t casts in a follow-up if wanted.

Thanks,
Quanye

Re: [PATCH mptcp-next v2] selftests: mptcp: sockopt: echo until EOF on partial reads

[Matthieu Baerts]

On 25/10/2026 13:50, quanyeyang wrote:
> Hi,
> 
> On Fri, 23 Oct 2026, sashiko-bot wrote:
>> [Medium] connect_one_server() passes chunk size to do_getsockopts()
>> ...
>> Should connect_one_server() pass total instead of ret to do_getsockopts()?
> 
> Yes. That is pre-existing in connect_one_server() and is not part of this
> change. It is tracked as issue 638 and already sent separately:
> 
>   https://lore.kernel.org/all/20261015-fix-mptcp-issue-638-v1-1-5de999ad601c@proton.me/
> 
> This patch only updates process_one_client(), which already accumulates
> nread/nwritten.

This patch has already been applied. It looks like Sashiko and Clashiko
used the "base-commit" tag added to your patch as a base, and thus found
this other issue already applied in our tree.

Please next time base all your MPTCP patches on top of our "export"
branch, and use the last version, that's easier. If you have dependences
between patches, feel free to either send a series with multiple
patches, or use the Based-on tag (or wait for the dependences to be
applied first).

https://github.com/multipath-tcp/mptcp_net-next/wiki/Git-Branches#which-branch-to-use

>> [Low] process_one_client() passes size_t to PRIu64 in xerror()
> 
> Agreed on 32-bit. The existing sockopt checks already pass ssize_t the
> same way. I can add the uint64_t casts in a follow-up if wanted.

Maybe good to fix it while at it, no?

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