Re: [PATCH net 4/5] selftests: tls: Extend coverage for zero-length records

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new MSG_PEEK test, TEST_F(zero_len_peek, test) in
  tools/testing/selftests/net/tls.c, cannot catch a regression of the…

--

selftests: tls: Extend coverage for zero-length records

This commit adds two TLS selftest fixtures, zero_len_peek and
zero_len_splice. Both inject pre-encrypted zero-length application_data
records and check that MSG_PEEK and splice() reach the payload behind
them, or return EAGAIN/EINVAL when there is no payload. It also moves the
zero_len socket setup and record send loop into shared helpers.

> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index ca47576220d3e..ed7feeac1cc41 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c

[ ... ]

> +/* Peeking past a run of empty data records must reach the payload
> + * behind them, and a run with no payload behind it must report EAGAIN.
> + */
> +TEST_F(zero_len_peek, test)
> +{
> +	const struct raw_rec *payload;
> +	unsigned char buf[128];
> +	ssize_t ret;
> +
> +	if (self->notls)
> +		SKIP(return, "no TLS support");
> +
> +	payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
> +
> +	if (variant->peek_ret < 0) {
> +		ret = recv(self->cfd, buf, sizeof(buf),
> +			   MSG_DONTWAIT | MSG_PEEK);
> +		EXPECT_EQ(ret, -1);
> +		EXPECT_EQ(errno, -variant->peek_ret);
> +		return;
> +	}

[Severity: Low]

Can this test catch a regression of the peek change made earlier in this
series by "tls: break the connection after a burst of empty data records"?

That change replaced "if (is_peek) goto put_on_rx_list;" in
tls_sw_recvmsg() with:

net/tls/tls_sw.c:tls_sw_recvmsg() {
    ...
			/* Consume empty records, even when peeking, so that
			 * they cannot pile up on rx_list.
			 */
    ...
			if (is_peek && chunk) {
				peeked += chunk;
				goto put_on_rx_list;
			}
    ...
}

On a sync-crypto kernel with the old behaviour, both variants still seem
to pass.

For 0data_0data_data, the peek queues all three skbs on rx_list, including
the two empty ones, and returns the 11 payload bytes. The next non-peek
recv() goes through process_rx_list(). There, each empty skb has chunk == 0
and rxm->full_len - skip == 0, so it is unlinked and freed, and then the
payload is copied. The last recv() still gets EAGAIN.

For 0data_0data_0data, the three empty skbs stay on ctx->rx_list.
tls_rx_rec_wait() returns -EAGAIN for the nonblocking peek, and the branch
above returns after checking only ret and errno.

tls_sw_sock_is_readable() reports readable while rx_list is non-empty:

	return tls_strp_msg_ready(ctx) ||
		!skb_queue_empty(&ctx->rx_list);

Would a zero-timeout poll() on cfd that expects 0 after the EAGAIN peek
tell the fixed and unfixed kernels apart?

zero_len_splice already has that check, under the comment "The empty
records were consumed, not left queued". It only runs after a successful
splice, though, and tls_sw_splice_read() frees empty records instead of
requeueing them.

The later patch "selftests: tls: Cover the cap on consecutive empty
records" adds zero_len_run with recv and splice cases but no MSG_PEEK
case. So at the end of the series, it looks like reverting the peek half
of the kernel fix would not make any selftest fail.

> +
> +	ret = recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT | MSG_PEEK);
> +	EXPECT_EQ(ret, variant->peek_ret);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109175836.1246840-1-kuba%40kernel.org