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: the commit message for "tls: Honor O_NONBLOCK in
tls_sw_splice_read()" justifies the patch partly with an event-loop…
--
tls: Honor O_NONBLOCK in tls_sw_splice_read()
This commit makes tls_sw_splice_read() treat the socket's O_NONBLOCK flag
as nonblocking, in addition to SPLICE_F_NONBLOCK. Splice and sendfile from
a nonblocking TLS socket then return -EAGAIN instead of sleeping in
tls_rx_rec_wait(), as they already do on a plain TCP socket.
> Polling first does not avoid the sleep once tls_sw_splice_read()
> consumes zero-length records. tls_sw_sock_is_readable() reports
> the socket readable while any record sits on rx_list, including a
> zero-length record that delivers no bytes to the pipe. The
> strparser announces a record before decryption, when the record's
> plaintext length is unknown, so the readiness test cannot screen
> such a record out. A splice that consumes the record and waits for
> the next stalls every connection an event loop multiplexes.
[Severity: Low]
This isn't a bug, but could the commit message say that this paragraph
describes behavior added later in the series?
At this commit, tls_sw_splice_read() has no retry loop. An empty record is
spliced as 0 bytes and consumed, and the function returns 0 without waiting
again:
net/tls/tls_sw.c:tls_sw_splice_read() {
...
chunk = min_t(unsigned int, rxm->full_len, len);
copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
...
}
The "consumes the record and waits for the next" path is added by the next
patch, "tls: Consume empty data records in tls_sw_splice_read()".
> An LLM audit of the TLS read paths found this defect. With empty
> records consumed, the zero_len_splice selftest, run on a
> nonblocking socket, reproduces the sleep.
The zero_len_splice selftest also doesn't exist in
tools/testing/selftests/net/tls.c at this commit. The only zero_len test
here uses recv. zero_len_splice is added later by "selftests: tls: Extend
coverage for zero-length records".
Both claims are worded conditionally ("once ... consumes zero-length
records", "With empty records consumed"). Putting this patch first in the
series also looks deliberate. The O_NONBLOCK problem in the first paragraph
exists on its own since c46234ebb4d1, so the Fixes: tag looks right.
Since this patch carries a Fixes: tag and may be backported alone, would it
help to say that the stall scenario and the zero_len_splice reference
depend on later patches in the series?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-tls-follow-on-v5-0-e76c301d4f05%40kernel.org
