Re: [PATCH v2] net/tls: support maximum record size limit

[Sabrina Dubroca <sd@queasysnail.net>]

2025-09-04, 23:31:23 +0000, Wilfred Mallawa wrote:
> On Wed, 2025-09-03 at 10:21 +0200, Sabrina Dubroca wrote:
> > 2025-09-02, 22:50:53 +0000, Wilfred Mallawa wrote:
> > > On Tue, 2025-09-02 at 18:07 +0200, Sabrina Dubroca wrote:
> > > > 2025-09-02, 13:38:10 +1000, Wilfred Mallawa wrote:
> > > > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > Hey Sabrina,
> > > > A selftest would be nice (tools/testing/selftests/net/tls.c), but
> > > > I'm
> > > > not sure what we could do on the "RX" side to check that we are
> > > > respecting the size restriction. Use a basic TCP socket and try
> > > > to
> > > > parse (and then discard without decrypting) records manually out
> > > > of
> > > > the stream and see if we got the length we wanted?
> > > > 
> > > So far I have just been using an NVMe TCP Target with TLS enabled
> > > and
> > > checking that the targets RX record sizes are <= negotiated size in
> > > tls_rx_one_record(). I didn't check for this patch and the bug
> > > below
> > > got through...my bad!
> > > 
> > > Is it possible to get the exact record length into the testing
> > > layer?
> > 
> > Not really, unless we come up with some mechanism using probes. I
> > wouldn't go that route unless we don't have any other choice.
> > 
> > > Wouldn't the socket just return N bytes received which doesn't
> > > necessarily correlate to a record size?
> > 
> > Yes. That's why I suggested only using ktls on one side of the test,
> > and parsing the records out of the raw stream of bytes on the RX
> > side.
> > 
> Ah okay I see.
> > Actually, control records don't get aggregated on read, so sending a
> > large non-data buffer should result in separate limit-sized reads.
> > But
> > this makes me wonder if this limit is supposed to apply to control
> > records, and how the userspace library/application is supposed to
> > deal
> > with the possible splitting of those records?
> > 
> Good point, from the spec, "When the "record_size_limit" extension is
> negotiated, an endpoint MUST NOT generate a protected record with
> plaintext that is larger than the RecordSizeLimit value it receives
> from its peer. Unprotected messages are not subject to this limit." [1]
> 
> From what I understand, as long as it in encrypted. It must respect the
> record size limit?

Yes, and the kernel will make sure to split all the data it sends over
records of the maximum acceptable length (currently
TLS_MAX_PAYLOAD_SIZE, with your patch tx_record_size_limit). The
question was more about what happens if userspace does a send(!DATA,
length > tx_record_size_limit). The kernel will happily split that
over N consecutive records of tx_record_size_limit (or fewer) bytes,
and the peer will receive N separate messages. But this could already
happen with a non-DATA record larger than TLS_MAX_PAYLOAD_SIZE, so
it's not really something we need to worry about here. It's a concern
for the userspace library (reconstructing the original message from
consecutive records read separately from the ktls socket). So, my
comment here was pretty much noise, sorry.

> In regards to user-space, do you mean for TX or RX? For TX, there
> shouldn't need to be any changes as record splitting occurs in the
> kernel. For RX, I am not too sure, but this patch shouldn't change
> anything for that case?

Yes, I'm talking about TX here.

-- 
Sabrina