Re: [PATCH net-next v3 4/9] tls: Consume empty data records in tls_sw_splice_read()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] tls_sw_splice_read() (net/tls/tls_sw.c) now has a `goto retry`
  loop that a peer can keep running.

--

tls: Consume empty data records in tls_sw_splice_read()

Before this commit, tls_sw_splice_read() returned zero when it hit a
zero-length application_data record, and callers read that as EOF. It
now consumes the empty record and jumps back to fetch the next one. A
signal_pending() check before each retry keeps 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 6c21897b03ee5..ff82e2f4e9e52 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -2038,12 +2039,13 @@ 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, nonblock, true, false);
> +		err = tls_rx_rec_wait(sk, nonblock, released, false);
>  		if (err <= 0)
>  			goto splice_read_end;
>  

[ ... ]

> @@ -2066,6 +2071,16 @@ 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)) {
> +			err = sock_intr_errno(sock_rcvtimeo(sk, nonblock));
> +			goto splice_read_end;
> +		}
> +		goto retry;
> +	}

[Severity: Medium]
Can a peer keep this goto retry loop running for as long as it likes?

Nothing limits how many empty records one call consumes, and nonblock is
not checked between retries. The loop only exits on one of these:

  - a non-empty data record
  - a control record
  - tls_rx_rec_wait() returning <= 0
  - a pending signal

Suppose the peer sends authenticated empty records at least as fast as
they are decrypted. Then the next record is always ready, through one of
two paths:

  - tls_rx_rec_done()->tls_strp_check_rcv()
  - sk_flush_backlog() followed by tls_strp_check_rcv() inside
    tls_rx_rec_wait()

In that case tls_rx_rec_wait() either skips its wait loop or breaks out
before it reaches the -EAGAIN exit:

net/tls/tls_sw.c:tls_rx_rec_wait() {
    ...
	timeo = sock_rcvtimeo(sk, nonblock);

	while (!tls_strp_msg_ready(ctx)) {
		...
		if (!skb_queue_empty(&sk->sk_receive_queue)) {
			tls_strp_check_rcv(&ctx->strp, false);
			if (tls_strp_msg_ready(ctx))
				break;
		}
		...
		if (!timeo)
			return -EAGAIN;
    ...
}

If so, would a splice(2) or sendfile(2) caller using SPLICE_F_NONBLOCK or
O_NONBLOCK never see -EAGAIN, and never get back to its event loop while
the stream continues? The earlier patch in this series, "tls: Honor
O_NONBLOCK in tls_sw_splice_read()", was meant to prevent this kind of
event loop stall.

The task also holds lock_sock() and the TLS reader lock for the whole
run. Other users of the socket, such as tls_sw_sendmsg(), setsockopt()
and recvmsg(), would be blocked during that time.

Blocking callers have a similar problem. Each call to tls_rx_rec_wait()
computes a fresh timeo from sock_rcvtimeo(). Could one empty record
arriving just before each timeout expires keep a splice with SO_RCVTIMEO
waiting forever?

Elsewhere in this series, tls_sw_read_sock() caps the same pattern:

	if (++nodata >= TLS_RX_NODATA_LIMIT) {

At the end of the series, tls_sw_splice_read() still has no cap. Would it
make sense to cap consecutive empty records here as well? One option is
to reuse TLS_RX_NODATA_LIMIT and return -EAGAIN to nonblocking callers.
Another is to carry a single remaining-timeout budget across retries.

tls_sw_recvmsg() has the same unbounded behaviour. The patch "tls:
Consume empty data records in tls_sw_recvmsg()" acknowledges this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-tls-follow-on-v3-0-6d52d98389b3%40kernel.org