Re: [PATCH net 1/9] net/tls: Bound time spent on no-data records in tls_sw_read_sock()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-07-26, 20:33:29 -0400, Chuck Lever wrote:
> An empty TLS 1.3 data record delivers no payload, so it leaves
> tls_sw_read_sock() in its loop without advancing the caller's read
> descriptor. A peer that streams such records keeps the receive loop
> running, and the socket lock held, for as long as they arrive.

BTW, should such a peer be considered malicious and
disconnected/blocked? Of course the logic for that should be left to
whatever is calling read_sock, not to TLS itself.

> Bound a run of such records, as net_rx_action() bounds a softirq
> poll. The first record that delivers no bytes arms a deadline
> TLS_RX_NODATA_NS ahead; any record that delivers bytes disarms it,
> so a normal stream never trips it. Breaking out with nothing copied

Another thought here: I think a peer that sends "some" 0-length data
records followed by one (possibly very small) data record, and then
repeats that sequence, will not hit this "rate-limiting" of no-data
records. Is that right? And if so, is that a problem?
[I'm guessing that the caller is doing something equivalent to
recvmsg(WAITALL), so it could be stuck for a long time even with this
new bound]

> returns zero, which a read_sock consumer reads as "no progress"
> rather than EOF, so the connection stays up. Records left queued
> draw no fresh sk_data_ready() of their own, so fire the socket's
> current callback before returning.

[...]
> @@ -2122,7 +2129,19 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>  		 * here instead.
>  		 */
>  		if (rxm->full_len == 0) {
> +			err = 0;
>  			consume_skb(skb);
> +			if (!nodata_deadline) {
> +				nodata_deadline = ktime_get_ns() +
> +						  TLS_RX_NODATA_NS;
> +			} else if (ktime_get_ns() >= nodata_deadline) {
> +				/* Queued records raise no new sk_data_ready(),
> +				 * and tls_rx_reader_release() announces only to
> +				 * saved_data_ready(), not the consumer's own.

sk->sk_data_ready is tls_data_ready at this point, no? I'm confused by
"the consumer" here.

> +				 */
> +				sk->sk_data_ready(sk);
> +				break;
> +			}
>  			continue;
>  		}

-- 
Sabrina