Message-ID: <0bae4a20d499c23a80065f5cb7ba1e6a1d95417b.camel@wdc.com>
Date: Thu, 25 Sep 2025 05:16:44 +0000
From: Wilfred Mallawa <wilfred.mallawa@....com>
To: "sd@...asysnail.net" <sd@...asysnail.net>
CC: "corbet@....net" <corbet@....net>, "davem@...emloft.net"
	<davem@...emloft.net>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>, "john.fastabend@...il.com"
	<john.fastabend@...il.com>, "shuah@...nel.org" <shuah@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "horms@...nel.org" <horms@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] selftests: tls: add tls record_size_limit test

On Wed, 2025-09-24 at 19:50 +0200, Sabrina Dubroca wrote:
> [got a bit distracted while writing this so Simon got to the process
> stuff before me, but I'll leave it in:]
> 
> BTW, a few details about process: since this is a new feature, the
> subject prefix should be [PATCH net-next v4 n/m] (new stuff targets
> the net-next tree), and the patches should be based on the net-next
> tree [1] (I'm not sure what you based this on, git am complained on
> both net and net-next for this patch). More info about this in the
> docs [2].
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
> [2] https://docs.kernel.org/process/maintainer-netdev.html
>     (in case you're not aware: also note the bits about "merge
> window"
>     which will quite likely become relevant in a few days)
> 
Thanks! I will rebase this on [1] for V5 with the changes you
specified.
> 
> 2025-09-23, 15:32:07 +1000, Wilfred Mallawa wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@....com>
> > 
> > Test that outgoing plaintext records respect the tls
> > record_size_limit
> > set using setsockopt(). The record size limit is set to be 128,
> > thus,
> > in all received records, the plaintext must not exceed this amount.
> > 
> > Also test that setting a new record size limit whilst a pending
> > open
> > record exists is handled correctly by discarding the request.
> > 
> > Suggested-by: Sabrina Dubroca <sd@...asysnail.net>
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@....com>
> 
> Thanks for adding this patch.
> (and for the tag :))
> 
> > ---
> >  tools/testing/selftests/net/tls.c | 149
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 149 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/tls.c
> > b/tools/testing/selftests/net/tls.c
> > index 0f5640d8dc7f..c5bd431d5af3 100644
> > --- a/tools/testing/selftests/net/tls.c
> > +++ b/tools/testing/selftests/net/tls.c
> > @@ -24,6 +24,7 @@
> >  #include "../kselftest_harness.h"
> >  
> >  #define TLS_PAYLOAD_MAX_LEN 16384
> > +#define TLS_TX_RECORD_SIZE_LIM 5
> 
> nit: That should not be needed if you run `make headers_install`
> before compiling the selftest:
> 
> make -s headers_install ; make -C tools/testing/selftests/net tls
> make: Entering directory
> '/home/sab/linux/net/tools/testing/selftests/net'
> gcc -Wall -Wl,--no-as-needed -O2 -g -I../../../../usr/include/ -
> isystem
> /home/sab/linux/net/tools/testing/selftests/../../../usr/include -
> I../ -D_GNU_SOURCE=     tls.c   -o tls
> 
> and that will find the new constant defined in the previous patch
> using the headers from the current kernel tree, instead of those in
> the system.
> 
Thanks!
> 
> [...]
> > +TEST(tx_record_size)
> > +{
> > +	struct tls_crypto_info_keys tls12;
> > +	int cfd, ret, fd, rx_len, overhead;
> > +	size_t total_plaintext_rx = 0;
> > +	__u8 tx[1024], rx[2000];
> > +	__u8 *rec;
> > +	__u16 limit = 128;
> > +	__u16 opt = 0;
> > +	__u8 rec_header_len = 5;
> 
> gcc complains about unused variables, I guess leftovers from
> extracting parse_tls_records:
> 
> tls.c: In function ‘tx_record_size’:
> tls.c:2840:14: warning: unused variable ‘rec_header_len’ [-Wunused-
> variable]
>  2840 |         __u8 rec_header_len = 5;
>       |              ^~~~~~~~~~~~~~
> tls.c:2837:15: warning: unused variable ‘rec’ [-Wunused-variable]
>  2837 |         __u8 *rec;
>       |               ^~~
> tls.c: In function ‘tx_record_size_open_rec’:
> tls.c:2893:14: warning: unused variable ‘rec_header_len’ [-Wunused-
> variable]
>  2893 |         __u8 rec_header_len = 5;
>       |              ^~~~~~~~~~~~~~
> tls.c:2891:15: warning: unused variable ‘rec’ [-Wunused-variable]
>  2891 |         __u8 *rec;
>       |               ^~~
> 
> 
> > +	unsigned int optlen = sizeof(opt);
> > +	bool notls;
> > +
> > +	tls_crypto_info_init(TLS_1_2_VERSION,
> > TLS_CIPHER_AES_CCM_128,
> > +			     &tls12, 0);
> > +
> > +	ulp_sock_pair(_metadata, &fd, &cfd, &notls);
> > +
> > +	if (notls)
> > +		exit(KSFT_SKIP);
> > +
> > +	/* Don't install keys on fd, we'll parse raw records */
> > +	ret = setsockopt(cfd, SOL_TLS, TLS_TX, &tls12, tls12.len);
> > +	ASSERT_EQ(ret, 0);
> > +
> > +	ret = setsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM,
> > &limit, sizeof(limit));
> > +	ASSERT_EQ(ret, 0);
> > +
> > +	ret = getsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM,
> > &opt, &optlen);
> > +	ASSERT_EQ(ret, 0);
> > +	ASSERT_EQ(limit, opt);
> > +	ASSERT_EQ(optlen, sizeof(limit));
> 
> nit: Maybe a few of those should be EXPECT_EQ? (ASSERT_* stops the
> test, EXPECT_* will run the rest of the test)
> 
> Getting the wrong value back from this getsockopt is worth noting but
> there's value in running the traffic through anyway?
> 
> > +
> > +	memset(tx, 0, sizeof(tx));
> > +	EXPECT_EQ(send(cfd, tx, sizeof(tx), 0), sizeof(tx));
> 
> But this one should maybe be an ASSERT because trying to parse
> records
> from whatever data we managed to send (if any) may not make much
> sense?
> 
> (just some thoughts, this is not a "strict requirement" to change
> anything in the patch)
Good points, I think that makes more sense.

Regards,
Wilfred

Powered by blists - more mailing lists