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] This patch adds TEST_F(zero_len_splice, test) in
tools/testing/selftests/net/tls.c.
--
selftests: tls: Extend coverage for zero-length records
This adds the zero_len_splice and zero_len_peek fixtures to the TLS
selftests. They check that splice() and MSG_PEEK get past runs of
zero-length application_data records. It also adds a data_0data_data
variant to zero_len, which checks that recv() does not read through an
empty record sitting between two data records.
> diff --git a/tools/testing/selftests/net/tls.c
> b/tools/testing/selftests/net/tls.c
> index 0bc43728262e..aa66d0bbd1b5 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
[ ... ]
> +/* Splicing must skip a run of empty data records to reach the payload
> + * behind it, since a zero-byte splice reads as EOF. Splice reports
> + * EAGAIN for a run with no payload behind it, and EINVAL for a control
> + * record behind the run.
> + */
> +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 before the read
> + * below blocks until the harness timeout.
> + */
> + ASSERT_EQ(splice(self->cfd, NULL, p[1], NULL, sizeof(buf),
> + SPLICE_F_NONBLOCK), variant->splice_ret);
[Severity: Low]
Both splice() calls in TEST_F(zero_len_splice, test) pass
SPLICE_F_NONBLOCK, and nothing sets O_NONBLOCK on cfd. Is the socket
O_NONBLOCK path in tls_sw_splice_read() ever covered by this test?
net/tls/tls_sw.c:tls_sw_splice_read() {
...
nonblock = (flags & SPLICE_F_NONBLOCK) ||
(sock->file->f_flags & O_NONBLOCK);
...
}
The earlier commit "tls: Honor O_NONBLOCK in tls_sw_splice_read()"
added the f_flags check above. It names this selftest, run on a
nonblocking socket, as its reproducer.
As of this patch, SPLICE_F_NONBLOCK on its own makes the wait return
-EAGAIN. A kernel that lost the O_NONBLOCK term would still pass every
zero_len_splice variant.
This looks resolved later in the series. "selftests: tls: Cover splice
on a nonblocking socket" moves the body into zero_len_do_splice() with
a splice_flags parameter. It also adds TEST_F(zero_len_splice,
nonblock_socket), which sets O_NONBLOCK on cfd with fcntl(F_SETFL) and
then splices with flags 0.
Would it make sense to fold that coverage into this patch, so the
reproducer named by the fix is in place from the start?
> + ret = read(p[0], buf, sizeof(buf));
> + EXPECT_EQ(ret, variant->splice_ret);
> + if (ret == variant->splice_ret)
> + EXPECT_EQ(memcmp(buf, payload->plain_data,
> + variant->splice_ret), 0);
> +
> + /* The empty records were consumed, not left queued */
> + EXPECT_EQ(poll(&pfd, 1, 0), 0);
> + }
[ ... ]
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-tls-follow-on-v6-0-ab01c4ecca0c%40kernel.org
