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

[sashiko-bot@kernel.org]

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