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] Concern: TEST_F(zero_len_splice, test) in
tools/testing/selftests/net/tls.c passes SPLICE_F_NONBLOCK to every…
--
selftests: tls: Extend coverage for zero-length records
This adds two fixtures to tls.c, zero_len_splice and zero_len_peek. They
check splice() and MSG_PEEK against runs of zero-length application_data
records. It also adds a data_0data_data variant to zero_len, which checks
that the first recv() stops at the empty record.
> diff --git a/tools/testing/selftests/net/tls.c
> b/tools/testing/selftests/net/tls.c
> index 0bc43728262e0..8ce5a90329600 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
[ ... ]
> @@ -2671,6 +2698,177 @@ TEST_F(zero_len, test)
[ ... ]
> +TEST_F(zero_len_splice, test)
> +{
> + struct pollfd pfd = { .fd = self->cfd, .events = POLLIN };
> + const struct raw_rec *payload;
> + unsigned char buf[128];
> + ssize_t ret;
> + int p[2];
> +
> + if (self->notls)
> + SKIP(return, "no TLS support");
> +
> + ASSERT_GE(pipe(p), 0);
> +
> + payload = zero_len_send_recs(_metadata, self->fd, variant->recs);
> +
> + if (variant->splice_ret < 0) {
> + ret = splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
> + SPLICE_F_NONBLOCK);
> + EXPECT_EQ(ret, -1);
> + EXPECT_EQ(errno, -variant->splice_ret);
> + } else {
> + /* ASSERT so a zero return stops the test here; the read
> + * below would block until the harness timeout.
> + */
> + ASSERT_EQ(splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
> + SPLICE_F_NONBLOCK), variant->splice_ret);
[Severity: Low]
This isn't a bug, but it is a gap in test coverage. Both splice() calls in
TEST_F(zero_len_splice, test) pass SPLICE_F_NONBLOCK, and nothing sets
O_NONBLOCK on self->cfd.
Can this test reach the socket O_NONBLOCK check in tls_sw_splice_read()?
net/tls/tls_sw.c:tls_sw_splice_read() {
...
nonblock = (flags & SPLICE_F_NONBLOCK) ||
(sock->file->f_flags & O_NONBLOCK);
...
}
The commit "tls: Honor O_NONBLOCK in tls_sw_splice_read()" adds the
sock->file->f_flags check. Its message gives "the zero_len_splice selftest,
run on a nonblocking socket" as the reproducer. In this version, every
splice() call in the test is nonblocking because of SPLICE_F_NONBLOCK alone.
A later patch in the series, "selftests: tls: Cover splice on a nonblocking
socket", moves this body into zero_len_do_splice(), which takes a
splice_flags argument. That patch also adds TEST_F(zero_len_splice,
nonblock_socket), which sets O_NONBLOCK on self->cfd and passes 0 as the
splice flags. The pipe comes from a plain pipe(), so the socket's O_NONBLOCK
is the only thing that makes those calls nonblocking.
So the path is covered by the end of the series. The gap only shows up when
the series is applied up to this patch and no further.
> + ret = read(p[0], buf, sizeof(buf));
> + EXPECT_EQ(ret, variant->splice_ret);
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org
