* Re: [PATCH mptcp-net] selftests: mptcp: sockopt: echo until EOF on partial reads
2026-10-17 11:22 [PATCH mptcp-net] selftests: mptcp: sockopt: echo until EOF on partial reads Quanye Yang via B4 Relay
@ 2026-10-17 11:29 ` sashiko-bot
2026-10-19 0:15 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-17 11:29 UTC (permalink / raw)
To: Quanye Yang, MPTCP Linux
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: mptcp: connect_one_server() passes partial ret to do_getsockopts()
--
commit 522b0ff16267ee948ab4943b8089644ce3f33c32
Author: Quanye Yang <quanyeyang@proton.me>
selftests: mptcp: sockopt: echo until EOF on partial reads
This commit updates process_one_client() in mptcp_sockopt.c to loop over
read() and write() until EOF, preventing a hang when payload segmentation
causes a short read on the server.
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index b6e58d936ebe5..5e1ec33368590 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -638,7 +638,7 @@ static void connect_one_server(int fd, int pipefd)
>
> static void process_one_client(int fd, int pipefd)
> {
> - ssize_t ret, ret2, ret3;
> + ssize_t ret, ret2, n, w;
> struct so_state s;
> char buf[4096];
>
> @@ -648,23 +648,27 @@ 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)
> - die_perror("read");
> + ret = 0;
> + ret2 = 0;
> + while ((n = read(fd, buf, sizeof(buf))) > 0) {
> + if (!ret) {
> + assert(s.mptcpi_rcv_delta <= (uint64_t)n);
>
> - assert(s.mptcpi_rcv_delta <= (uint64_t)ret);
> -
> - if (s.tcpi_rcv_delta)
> - assert(s.tcpi_rcv_delta == (uint64_t)ret);
> + if (s.tcpi_rcv_delta)
> + assert(s.tcpi_rcv_delta <= (uint64_t)n);
> + }
>
> - ret2 = write(fd, buf, ret);
> - if (ret2 < 0)
> - die_perror("write");
> + w = write(fd, buf, n);
> + if (w < 0)
> + die_perror("write");
> + if (w != n)
> + xerror("short write");
>
> - /* wait for hangup */
> - ret3 = read(fd, buf, 1);
> - if (ret3 != 0)
> - xerror("expected EOF, got %lu", ret3);
> + ret += n;
> + ret2 += w;
> + }
[Severity: Medium]
When a partial read occurs in process_one_client(), it immediately echoes
the partial chunk n back to the client before reading and echoing the rest
of the payload in subsequent loop iterations.
If the client in connect_one_server() reads the echoed data across more
than one read() call, ret only holds the byte count of the final chunk
while total accumulates the full length:
tools/testing/selftests/net/mptcp/mptcp_sockopt.c: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);
}
Because connect_one_server() passes ret instead of total to
do_getsockopts(), won't connect_one_server() -> do_getsockopts() ->
do_getsockopt_mptcp_info() fail on:
assert(s->mi.mptcpi_write_seq + w == i.mptcpi_write_seq);
and do_getsockopt_tcp_info() fail when checking tcpi_bytes_sent and
tcpi_bytes_received against r and w whenever the echoed response is read in
multiple chunks? Should connect_one_server() pass total to do_getsockopts()
as well?
> + if (n < 0)
> + die_perror("read");
>
> do_getsockopts(&s, fd, ret, ret2);
> if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-fix-mptcp-issue-639-v1-1-0554d07ab8f8@proton.me?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH mptcp-net] selftests: mptcp: sockopt: echo until EOF on partial reads
2026-10-17 11:22 [PATCH mptcp-net] selftests: mptcp: sockopt: echo until EOF on partial reads Quanye Yang via B4 Relay
2026-10-17 11:29 ` sashiko-bot
@ 2026-10-19 0:15 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 0:15 UTC (permalink / raw)
To: quanyeyang; +Cc: mptcp, kuba
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0 (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] The fix is incomplete: the segmented-payload case still fails.
- [Medium] The commit message describes a failure the old code could not
produce.
Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the xerror()
calls in process_one_client() pass `ssize_t` values (`ret + 1`,…
--
selftests: mptcp: sockopt: echo until EOF on partial reads
This commit changes process_one_client() in the mptcp_sockopt selftest.
The old code did a single read() and write(), then a one-byte read to wait
for hangup. The new code loops on read()/write() until EOF and passes the
accumulated byte counts to the existing sockopt checks.
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/639
> 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.
[Severity: Medium]
Is this description of the old behavior accurate?
In the old process_one_client(), the server did this after a short first
read and the partial echo:
/* wait for hangup */
ret3 = read(fd, buf, 1);
if (ret3 != 0)
xerror("expected EOF, got %lu", ret3);
By then connect_one_server() has already written all len bytes, since it
calls xerror() on a short write. So the remaining bytes reach the server,
read(fd, buf, 1) returns 1, and the server exits through xerror().
The server is a separate forked process. When it exits, its socket is
closed and the client's blocked read() returns. That is a quick failure,
not both sides waiting on each other until the alarm fires.
The old read(fd, buf, 1) could only block forever if the rest of the data
never arrived. In that case the new read() loop would block in the same
way.
If issue #639 really shows a timeout, is the root cause somewhere else?
If not, could the commit message describe the failure as the "expected
EOF" error instead of a hang?
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index b6e58d936ebe5..5e1ec33368590 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
[ ... ]
> @@ -648,23 +648,27 @@ 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)
> - die_perror("read");
> + ret = 0;
> + ret2 = 0;
> + while ((n = read(fd, buf, sizeof(buf))) > 0) {
> + if (!ret) {
> + assert(s.mptcpi_rcv_delta <= (uint64_t)n);
>
> - assert(s.mptcpi_rcv_delta <= (uint64_t)ret);
> -
> - if (s.tcpi_rcv_delta)
> - assert(s.tcpi_rcv_delta == (uint64_t)ret);
> + if (s.tcpi_rcv_delta)
> + assert(s.tcpi_rcv_delta <= (uint64_t)n);
> + }
>
> - ret2 = write(fd, buf, ret);
> - if (ret2 < 0)
> - die_perror("write");
> + w = write(fd, buf, n);
> + if (w < 0)
> + die_perror("write");
> + if (w != n)
> + xerror("short write");
[Severity: High]
When the payload is segmented, the server now sends the echo as several
write() calls. Can the client still fail in connect_one_server() in that
case?
The client read loop adds up total, but then passes ret, which holds only
the size of the last read() chunk:
total = 0;
do {
ret = read(fd, buf2 + total, sizeof(buf2) - total);
...
total += ret;
} while (total < len);
...
do_getsockopts(&s, fd, ret, ret);
For a plain read(), mptcp_recvmsg() uses a sock_rcvlowat() target of 1.
So if the echo arrives as n1 and then n2, the client can return n1
without waiting for n2. That leaves ret equal to n2, which is less than
len.
do_getsockopt_mptcp_info() then checks:
assert(s->mi.mptcpi_write_seq + w == i.mptcpi_write_seq);
Here w is n2 but write_seq has advanced by len, so the client aborts.
Without that assert, do_getsockopt_tcp_info() would still fail. It
compares tcpi_bytes_sent and tcpi_bytes_received (both len) against n2,
never matches, and ends in xerror() after the retries.
Before this patch the echo was a single write(), so ret == len usually
held. Should connect_one_server() pass total (or len) to do_getsockopts()
as part of this fix?
>
> - /* wait for hangup */
> - ret3 = read(fd, buf, 1);
> - if (ret3 != 0)
> - xerror("expected EOF, got %lu", ret3);
> + ret += n;
> + ret2 += w;
> + }
> + if (n < 0)
> + die_perror("read");
>
> do_getsockopts(&s, fd, ret, ret2);
> if (s.mptcpi_rcv_delta != (uint64_t)ret + 1)
[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The
xerror() calls that follow pass ssize_t values to PRIu64 conversions:
xerror("mptcpi_rcv_delta %" PRIu64 ", expect %" PRIu64 ", diff %" PRId64,
s.mptcpi_rcv_delta, ret + 1, s.mptcpi_rcv_delta - (ret + 1));
ret2 and ret are passed the same way in the mptcpi_bytes_sent,
mptcpi_bytes_received and mptcpi_bytes_acked messages.
On 32-bit builds ssize_t is 32 bits, so will these messages print
garbage values? The declaration of ret and ret2 is being rewritten
anyway. Could they be cast to uint64_t in these calls, or declared with a
64-bit type?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017-fix-mptcp-issue-639-v1-1-0554d07ab8f8%40proton.me
^ permalink raw reply [flat|nested] 3+ messages in thread