Re: [PATCH net-next v4 3/9] tls: Honor O_NONBLOCK in tls_sw_splice_read()

[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 commit message of "tls: Honor O_NONBLOCK in
  tls_sw_splice_read()" describes a poll-then-splice stall and names a…
- [Low] At this commit no selftest covers the new behavior in
  tls_sw_splice_read(): splice(2) without SPLICE_F_NONBLOCK on a TLS…

Pre-existing issues:
- [Medium] At this commit, tls_sw_splice_read() splices a zero-length
  decrypted DATA record as chunk = 0.

--

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. The combined value is
passed to tls_rx_reader_lock() and tls_rx_rec_wait(). As a result,
splice(2) and sendfile(2) on a nonblocking TLS socket return -EAGAIN
instead of sleeping, the same way tcp_splice_read() does.

> 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.
>
> 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.

[Severity: Low]
This isn't a bug, but do these paragraphs describe code that exists at
this commit?

At this commit tls_sw_splice_read() has no retry loop. It splices a
zero-length record as zero bytes, consumes it, and returns 0. That means
there is no second wait in tls_rx_rec_wait() that could stall.

Also, tools/testing/selftests/net/tls.c has no zero_len_splice fixture
yet.

Both come later in the series:

- "tls: Consume empty data records in tls_sw_splice_read()" adds the
  retry loop.
- "selftests: tls: Extend coverage for zero-length records" adds
  zero_len_splice.

The current wording ("once ...", "With empty records consumed ...")
already hints at this. Would it be clearer to say that the stall only
appears once the following patch is applied?

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f2062e141512c..f5e329b75976b 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2024,10 +2024,14 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	struct tls_msg *tlm;
>  	struct sk_buff *skb;
>  	ssize_t copied = 0;
> +	bool nonblock;
>  	int chunk;
>  	int err;
>  
> -	err = tls_rx_reader_lock(sk, ctx, flags & SPLICE_F_NONBLOCK);
> +	nonblock = (flags & SPLICE_F_NONBLOCK) ||
> +		   (sock->file->f_flags & O_NONBLOCK);

[Severity: Low]
This isn't a bug, but does any selftest at this commit cover the new
condition? That would be splice(2) without SPLICE_F_NONBLOCK on a TLS
socket that has O_NONBLOCK set.

TEST_F(tls, nonblocking) sets O_NONBLOCK on cfd but only calls recv().
TEST_F(zero_len, test) never calls splice().

Later in the series, "selftests: tls: Cover splice on a nonblocking
socket" adds TEST_F(zero_len_splice, nonblock_socket). That test sets
O_NONBLOCK on cfd and calls zero_len_do_splice() with splice_flags = 0,
which closes this gap.

> +
> +	err = tls_rx_reader_lock(sk, ctx, nonblock);
>  	if (err < 0)
>  		return err;
>  
> @@ -2041,8 +2045,7 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	} else {
>  		struct tls_decrypt_arg darg;
>  
> -		err = tls_rx_rec_wait(sk, flags & SPLICE_F_NONBLOCK,
> -				      true, false);
> +		err = tls_rx_rec_wait(sk, nonblock, true, false);
>  		if (err <= 0)
>  			goto splice_read_end;
>  

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
happens later in tls_sw_splice_read() when the decrypted DATA record has
zero length?

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);
	if (copied < 0)
		goto splice_requeue;

	if (copied < rxm->full_len) {
    ...
	consume_skb(skb);

splice_read_end:
	tls_rx_reader_unlock(sk, ctx);
	return copied ? : err;
}

When rxm->full_len == 0, chunk and copied are both 0. The record is
consumed and the function returns 0.

Will splice(2) and sendfile(2) callers read that 0 as EOF? RFC 5246 and
RFC 8446 both allow empty application_data records, so a remote peer
could send one and make a live connection look closed to the reader.

The next patch in the series, "tls: Consume empty data records in
tls_sw_splice_read()", fixes this by consuming the zero-length record
and jumping back to retry.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-tls-follow-on-v4-0-8cea429565da%40kernel.org