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

2026-07-30, 09:05:17 -0400, Chuck Lever wrote:
> 
> 
> On Thu, Jul 30, 2026, at 5:12 AM, Sabrina Dubroca wrote:
> > 2026-07-26, 20:33:29 -0400, Chuck Lever wrote:
> 
> >> 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?
> 
> That occurred to me too. It's right on the cusp between still
> making progress and gumming things up. Neither the packet-count
> limit nor the time-bound address this case.
> 
> I don't have a good answer.

I'm not sure that can be addressed in a generic way within ktls. Maybe
the caller needs to do its own accounting of "this read_sock/read_actor
dance has been going on for too long now, let's stop".

> > [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]
> 
> Still digesting your other feedback. I plan to send a v2 to
> address it.

Thanks. There was also some from sashiko but I couldn't force myself
to read it.

BTW I'm not requesting you to also fix the existing misbehavior I've
mentioned (EOR and timeo).


One small thing I've just noticed in this patch:

>                 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(sk);
> +                               break;
> +                       }
>                         continue;
>                 }
>  
>                 used = read_actor(desc, skb, rxm->offset, rxm->full_len);
>                 if (used <= 0) {
>                         if (!copied)
>                                 err = used;
>                         goto read_sock_requeue;
>                 }

If we hit the nodata_deadline, we break out of the loop, read_actor
returns 0 since it's an empty record, and we jump to
read_sock_requeue. It doesn't make much sense to requeue the empty
record? Well, we'll start the next read_sock with a record that
immediately arms the deadline, and it avoids a small additional diff
to handle that case in the "used <= 0" block. Nothing serious, I just
thought I'd mention it. I can live with the current change.

-- 
Sabrina