Re: [PATCH net-next v2 3/8] tls: consume empty data records in tls_sw_splice_read()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This commit introduces a new blocking point in
  tls_sw_splice_read() (net/tls/tls_sw.c); a later patch in the series…
- [Medium] The new `goto retry` loop in tls_sw_splice_read()
  (net/tls/tls_sw.c:2069-2079 at this commit, 2088-2095 at the end of…

--

tls: consume empty data records in tls_sw_splice_read()

This patch makes tls_sw_splice_read() consume zero-length
application_data records and fetch the next record, so that callers no
longer see a false EOF. A signal_pending() check before each retry is
meant to keep the splicing task killable.

Closes: https://lore.kernel.org/netdev/akaoXcfamBp8_mYe@krikkit/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index ee50b9028264c..79a807e51bc7d 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2031,13 +2032,14 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  	if (err)
>  		goto splice_read_end;
>  
> +retry:
>  	if (!skb_queue_empty(&ctx->rx_list)) {
>  		skb = __skb_dequeue(&ctx->rx_list);
>  	} else {
>  		struct tls_decrypt_arg darg;
>  
>  		err = tls_rx_rec_wait(sk, flags & SPLICE_F_NONBLOCK,
> -				      true, false);
> +				      released, false);

[Severity: Medium]
With the new goto retry below, can this wait now block on a socket that
has O_NONBLOCK set?

Here the only input to the nonblocking decision is
flags & SPLICE_F_NONBLOCK. On the socket-to-pipe path, do_splice() sets
that flag only from the output pipe:

fs/splice.c:do_splice() {
    ...
		if (out->f_flags & O_NONBLOCK)
			flags |= SPLICE_F_NONBLOCK;

		ret = splice_file_to_pipe(in, opipe, &offset, len, flags);
    ...
}

tcp_splice_read(), by contrast, honours the socket's own flag:

	timeo = sock_rcvtimeo(sk, sock->file->f_flags & O_NONBLOCK);

Before this patch, an empty record made splice return 0 right away. Now
the empty record is consumed and tls_rx_rec_wait() is called again with
a blocking timeout. It then sleeps until the peer sends another record,
and SO_RCVTIMEO has no limit by default.

Take an event loop with an O_NONBLOCK kTLS socket and a blocking pipe.
Could it hang in splice() after getting EPOLLIN for a single empty
record?

The later patch in this series, "tls: honor O_NONBLOCK in
tls_sw_splice_read()", appears to fix this. It computes nonblock from
both SPLICE_F_NONBLOCK and sock->file->f_flags. Could that patch be
ordered before this one, so the hang does not exist in the bisect
window? Or could the commit message at least mention it?

>  		if (err <= 0)
>  			goto splice_read_end;
>  

[ ... ]

> @@ -2060,6 +2065,19 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		goto splice_requeue;
>  	}
>  
> +	/* Splicing zero bytes reads as EOF to the caller. */
> +	if (rxm->full_len == 0) {
> +		consume_skb(skb);
> +		if (signal_pending(current)) {
> +			long timeo;
> +
> +			timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK);
> +			err = sock_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;

[Severity: Medium]
How many consecutive empty records can this loop consume while it holds
the socket lock?

After each record, tls_rx_rec_done()->tls_strp_check_rcv() parses the
next one. On retry, tls_rx_rec_wait() sees tls_strp_msg_ready() return
true and skips its wait loop entirely.

When the receive queue runs dry, sk_flush_backlog() refills it, and the
loop breaks out before reaching sk_wait_event():

net/tls/tls_sw.c:tls_rx_rec_wait() {
	while (!tls_strp_msg_ready(ctx)) {
		...
		if (sk_flush_backlog(sk))
			released = true;
		if (!skb_queue_empty(&sk->sk_receive_queue)) {
			...
			tls_strp_check_rcv(&ctx->strp, false);
			if (tls_strp_msg_ready(ctx))
				break;
		}
		...
		ret = sk_wait_event(sk, &timeo,
				    tls_strp_msg_ready(ctx), &wait);
		...
}

sk_wait_event() is the only place where socket ownership is released.
__release_sock() drops only the slock spinlock, and it calls
cond_resched() only once every 16 backlog skbs.

The only exit is signal_pending(current), and that helps only the
splicing task.

Suppose a peer streams minimal empty application_data records (under 30
bytes each) at least as fast as they are decrypted. Could this:

  - leave other tasks that call lock_sock() on this socket (sendmsg,
    setsockopt, shutdown, close) stuck in uninterruptible sleep?

  - make the CPU busy-loop through back-to-back AEAD decrypts on
    !PREEMPT kernels?

  - pin a nonblocking event loop inside a single splice() call? The
    O_NONBLOCK fix later in the series does not seem to help, because
    nonblock only matters when no record is ready.

Before this patch, each splice(2) call handled at most one record and
then released the lock.

In the same series, "tls: bound consecutive no-data records in
tls_sw_read_sock()" caps this same pattern with TLS_RX_NODATA_LIMIT.
Should the splice path get a similar cap? At the end of the series this
loop still appears to have no limit.

> +	}
> +
>  	chunk = min_t(unsigned int, rxm->full_len, len);
>  	copied = skb_splice_bits(skb, sk, rxm->offset, pipe, chunk, flags);
>  	if (copied < 0)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-tls-follow-on-v2-0-2dd1947bb642%40kernel.org