Re: [PATCH net-next v2 1/6] net/tls: Bound consecutive no-data records in tls_sw_read_sock()

2026-07-23, 10:29:55 -0400, Chuck Lever wrote:
> 
> 
> On Thu, Jul 23, 2026, at 10:23 AM, Sabrina Dubroca wrote:
> > 2026-07-23, 09:24:47 -0400, Chuck Lever wrote:
> >> 
> >> 
> >> On Thu, Jul 23, 2026, at 3:11 AM, Hannes Reinecke wrote:
> >> > On 7/20/26 4:27 PM, Chuck Lever wrote:
> >> >> A record that delivers no payload -- an empty TLS 1.3 data record
> >> >> today, a control record once read_sock_rectype() lands -- 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 the records
> >> >> arrive.
> >> >> 
> >> >> Cap the number of consecutive no-data records consumed per call. The
> >> >> count resets on any record that delivers bytes, so a normal stream
> >> >> is unaffected; a peer supplying only empty records is bounded to
> >> >> TLS_RX_NODATA_LIMIT iterations before the call returns 0. read_sock
> >
> > Would a time-based limit be "better" than a packet count? For example
> > that's what net_rx_action() (net/core/dev.c) does.
> >
> >
> >> >> consumers treat that as "no progress, re-poll" rather than EOF, so
> >> >> the connection stays up and makes progress once real data arrives.
> >> >> 
> >> >> Only tls_sw_read_sock() needs this cap. Its consumers drive the receive
> >> >> loop from kernel context -- a work item or service thread holding the
> >> >> socket lock across the whole call with no return to userspace -- so an
> >> >> unbounded empty-record stream keeps that context and the lock pinned
> >> >> for as long as the flood lasts. The cap supplies the return boundary
> >> >> that a system call would otherwise provide. tls_sw_splice_read()
> >> >> and tls_sw_recvmsg() already have one: they run in the calling task's
> >> >> context, reschedule while draining the socket backlog (cond_resched()
> >> >> in __release_sock()),
> >
> > tls_sw_read_sock() would also do that via tls_rx_rec_wait(), no?
> >
> >> >> and drop the socket lock when the call returns. A
> >> >> flood there costs the caller only its own scheduler time, so the cap
> >> >> would add nothing.
> >> >> 
> >> >> Signed-off-by: Chuck Lever <[email protected]>
> >> >> ---
> >> >>   net/tls/tls_sw.c | 13 +++++++++++++
> >> >>   1 file changed, 13 insertions(+)
> >> >> 
> >> > This is technically a fix, so it might be worthwhile sending it
> >> > on its own.
> >> 
> >> My impression is that the issue this patch addresses is not
> >> reachable until the subsequent patches in this series have
> >> been applied. Thus I positioned it as a pre-requisite patch
> >> in this series, and not as part of the earlier "fixes" series.
> >
> > It looks to me like a follow-up for 3be28e2c9cd0 ("net/tls: Consume
> > empty data records in tls_sw_read_sock()"). Control records only
> > become a problem after the rest of the series, but TLS 1.3 empty data
> > records would get there with the current code, no?
> 
> It sounds like a change in presentation strategy might be
> needed: I have the other patches you suggested before but
> was planning on submitting them after this series goes in.
> 
> But it could be sensible to post those first, along with
> 1/6 of this series, destined for "net". Thoughts?

That makes sense. 2..6/6 from this series can still be around for
review (maybe with a [RFC] prefix if you resend the net-next patches
before net gets merged into net-next, to help the maintainers by
avoiding conflicts).

-- 
Sabrina