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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Wilfred Mallawa <[email protected]> > > 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, ¬ls); > > + > > + 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
