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

["Chuck Lever" <cel@kernel.org>]

On Thu, Jul 30, 2026, at 1:21 PM, Sabrina Dubroca wrote:
> 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".

The consumer can, but only by killing the connection. During a run of
empty records, read_actor is never called and nothing decrements
desc->count, so the consumer never regains control. The socket lock is
held throughout, and __sk_flush_backlog() takes only sk_lock.slock, so
sk_lock.owned stays set. Anything needing lock_sock() blocks behind the
reader, kernel_sock_shutdown() included. The one channel left is a
store to sk->sk_err, which tls_rx_rec_wait() tests first in its loop.
That works, but it is terminal for the connection.

To be non-lethal, the bound has to be inside ktls. v2 of this series
will use a count rather than a deadline, as Jakub requested.


> 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.
  
Well, but consume_skb() has already run, and the break leaves the loop
for read_sock_end, which does not requeue. read_actor is never reached
for an empty record.


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

It depends on which callback was installed first, and unfortunately
two current in-tree read_sock consumers differ in that order.

svcsock (to become a read_sock consumer soon) installs svc_data_ready
at socket setup and the handshake runs later, so ktls sits on top:
sk_data_ready is tls_data_ready and saved_data_ready is svc_data_ready.
The release-time announce already reaches that consumer.

But nvme-tcp is the reverse. It starts TLS in nvme_tcp_alloc_queue() and
installs its own callback afterward in nvme_tcp_start_queue().
saved_data_ready is sock_def_readable and the announce never queues its
io_work.

The explicit poke is for the second case. v2 of this series will rewrite
the comment to state the condition rather than name a function.


-- 
Chuck Lever