Re: [PATCH net 2/7] selftests: tls: add test for data loss on small pipe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Test logic flaw on architectures with large page sizes defeats the purpose of the test.
- [Medium] Unhandled failure paths and use of EXPECT_EQ cause uninitialized memory reads and test hangs.
--

commit 52c9391392d3ea9a9876d20000032a6a0985383b
Author: Jakub Kicinski <kuba@kernel.org>

selftests: tls: add test for data loss on small pipe

This commit adds a new selftest splice_to_pipe_small to the TLS test suite.
The test verifies that data is not lost when using splice to move data from
a TLS socket into a small pipe that requires multiple iterations to drain.

> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c

[ ... ]

> +TEST_F(tls, splice_to_pipe_small)
> +{
> +	int send_len = TLS_PAYLOAD_MAX_LEN;
> +	char mem_send[TLS_PAYLOAD_MAX_LEN];
> +	char mem_recv[TLS_PAYLOAD_MAX_LEN];
> +	size_t total = 0;
> +	int p[2];
> +
> +	memrnd(mem_send, sizeof(mem_send));
> +
> +	ASSERT_GE(pipe(p), 0);
> +
> +	/* Shrink pipe to 1 page (typically 4096 bytes) to force multiple
> +	 * splice iterations for a 16384-byte TLS record.
> +	 */
> +	EXPECT_GE(fcntl(p[1], F_SETPIPE_SZ, 4096), 4096);

Does this test behave as expected on architectures with large page sizes?

The kernel rounds pipe capacities up to the system page size. On architectures
with a 64KB page size (such as ARM64 or PowerPC), the pipe size will be set
to 64KB instead of 4096 bytes.

Since the 16KB payload easily fits within a 64KB pipe, the splice call would
transfer the entire payload in a single iteration, bypassing the intended
multiple-iteration behavior.

> +	EXPECT_EQ(send(self->fd, mem_send, send_len, 0), send_len);

Will this test hang on a short send?

Unlike ASSERT_EQ, EXPECT_EQ allows execution to continue on failure. If send()
transmits a partial payload, total will not advance enough relative to the
data on the socket. The subsequent splice() could then block indefinitely
waiting for data that will never arrive.

> +	while (total < (size_t)send_len) {
> +		ssize_t spliced, drained;
> +
> +		spliced = splice(self->cfd, NULL, p[1], NULL,
> +				 send_len - total, 0);
> +		EXPECT_GT(spliced, 0);
> +		if (spliced <= 0)
> +			break;
> +
> +		drained = read(p[0], mem_recv + total, spliced);
> +		EXPECT_EQ(drained, spliced);
> +		if (drained <= 0)
> +			break;
> +
> +		total += drained;
> +	}
> +
> +	EXPECT_EQ(total, (size_t)send_len);
> +	EXPECT_EQ(memcmp(mem_send, mem_recv, send_len), 0);

Could this read uninitialized memory?

If splice() or read() return <= 0, the loop breaks early with total < send_len.

Because mem_recv is uninitialized stack memory, the memcmp() call will
read past mem_recv + total into uninitialized memory, triggering undefined
behavior and potential memory sanitizer failures.

> +
> +	close(p[0]);
> +	close(p[1]);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429222944.2139041-1-kuba@kernel.org?part=2