Re: [PATCH net 2/9] net/tls: Consume empty data records in tls_sw_splice_read()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-07-26, 20:33:30 -0400, Chuck Lever wrote:
> +/* TLS 1.2 and TLS 1.3 both permit a zero-length application_data
> + * record as a traffic-analysis countermeasure (RFC 5246, Section
> + * 6.2.1; RFC 8446, Section 5.1).
> + */
> +static bool tls_rx_empty_data_rec(int len, unsigned char control)
> +{
> +	return !len && control == TLS_RECORD_TYPE_DATA;
> +}

I'm not convinced by this helper. The record type check is redundant
for splice and read_sock, and it doesn't save much for recvmsg.

If we're going to keep it, I'd rather pass it the skb and fetch the
length and record type directly in the helper, since that's anyway
what all callers are passing.

[...]
> @@ -2017,6 +2037,11 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  
>  		tls_rx_rec_done(ctx);
>  		skb = darg.skb;
> +
> +		/* The socket lock stays held to the retry, so the
> +		 * anchor this wait loaded survives it.

I'm quite confused by this comment. Do you mean "We haven't released
the lock, so don't tell tls_rx_rec_wait that we have if we retry" ?

Either way, I don't think we should be leaking mentions of the
"anchor" outside of strp.c. Whatever tls_rx_rec_wait() does with the
"released" argument isn't tls_sw_splice_read()'s business.

> +		 */
> +		released = false;
>  	}
>  
>  	rxm = strp_msg(skb);
> @@ -2028,6 +2053,21 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
>  		goto splice_requeue;
>  	}
>  
> +	/* Splicing an empty data record delivers zero bytes, which the
> +	 * caller reads as EOF. tls_rx_rec_wait() skips its signal check
> +	 * while a record is parsed, so test for a signal here.
> +	 */
> +	if (tls_rx_empty_data_rec(rxm->full_len, tlm->control)) {
> +		long timeo = sock_rcvtimeo(sk, flags & SPLICE_F_NONBLOCK);
> +
> +		consume_skb(skb);
> +		if (signal_pending(current)) {
> +			err = tls_rx_intr_errno(timeo);
> +			goto splice_read_end;
> +		}
> +		goto retry;

This looping (and the existing one in the other RX handlers) is making
rcvtimeo a bit pointless AFAICT:

 - we apply rcvtimeo to tls_rx_reader_acquire/tls_rx_reader_lock
 - if that worked (maybe consuming almost the full duration), we keep going
 - we apply rcvtimeo (from "0") it tls_rx_rec_wait
 - keep going again, so maybe we've already consumed close to 2*rcvtimeo
 - decrypt does its thing
 - if we're getting a bunch of 0-length records spaced "just right",
   we keep waiting ~rcvtimeo and never stop. with recvmsg(), if we're
   getting some data but not enough to fill the user's buffer, we'll
   also keep going "too long".

Am I reading this wrong?

If not, that behavior doesn't look desirable. At least it doesn't seem
to match the doc for SO_RCVTIMEO:

    If an input or output function blocks for this period of time, and
    data has been sent or received, the return value of that function
    will be the amount of data transferred; if no data has been
    transferred and the timeout has been reached, then -1 is returned
    with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS (for
    connect(2)) just as if the socket was specified to be nonblocking.


-- 
Sabrina