Re: [PATCH v12 6/6] selftests: net: add TLS hardware offload test

[Sabrina Dubroca <sd@queasysnail.net>]

@Jakub [top-posting so you don't have to scroll through the rest of my
comments to find some global questions about this patch]

tools/testing/selftests/drivers/net/README.rst mentions "Local
host is the DUT", but this test does rekeys on both sides and sends a
bit of traffic back and forth. Is that acceptable?

Another thought: is there a "standard" for stdout vs stderr, as well as
verbosity of "test progress"/"debug" type messages ("sent
keyupdate"/"received keyupdate"/"server listening"/"setup complete"
etc) for those test programs? Any expectation for a --{debug,verbose}
option to only display all this stuff on request?

Output for 1 test:
-------- 8< --------
TLS Version: TLS 1.3
Cipher: AES-GCM-128
Buffer size: 16384
Connecting to 192.168.13.1:4433...
Connected!
Installing TLS_TX AES-GCM-128 gen 0...
TLS_TX AES-GCM-128 gen 0 installed
Installing TLS_RX AES-GCM-128 gen 0...
TLS_RX AES-GCM-128 gen 0 installed
TLS setup complete.
Sending 100 messages of 16384 bytes...
Sent 16384 bytes (iteration 1)
Received echo 16384 bytes (ok)
[...repeated]
Sent 16384 bytes (iteration 100)
Received echo 16384 bytes (ok)
-------- 8< --------

With some rekeys I get 300L of output on each side.


If not, I guess we can fall back to what makes the most sense for
NIPA?



2026-04-02, 17:55:11 -0600, Rishikesh Jethwani wrote:
> diff --git a/tools/testing/selftests/drivers/net/hw/tls_hw_offload.c b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.c
> new file mode 100644
> index 000000000000..788891890ec8
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.c

I think it would make sense to add the new files to the
"NETWORKING [TLS]" entry in MAINTAINERS. We already have the existing
selftest listed.

[...]
> +#define TEST_ITERATIONS	100
> +#define MAX_REKEYS	99

Making TEST_ITERATIONS a test argument would allow a test that lasts
more than half a second. Then MAX_REKEYS can be removed and the
argument validation switched to num_rekeys < num_iterations. (doing
rekeys without a send() in between [other than the send_keyupdate]
should be fine, but it makes the logic for "do we send a rekey and/or
application_data this round?" more complex, so let's keep the check on
num_rekeys)

In general, this test is a good start, but it doesn't cover enough
(see my reply to 5/6). How did you test the "unacked records" part of
the rekey patch? It's the most complex part of the series.


[...]
> +static void derive_key_fields(unsigned char *key, int key_size,
> +			      unsigned char *iv, int iv_size,
> +			      unsigned char *salt, int salt_size,
> +			      unsigned char *rec_seq, int rec_seq_size,
> +			      int generation)
> +{
> +	unsigned char pattern;
> +	int i;
> +
> +	if (generation == 0)
> +		return;
> +
> +	pattern = (unsigned char)((generation * 0x1B) ^ 0x63);
> +	for (i = 0; i < key_size; i++) {
> +		key[i] ^= pattern;
> +		pattern = (pattern << 1) | (pattern >> 7);
> +	}
> +
> +	pattern = (unsigned char)((generation * 0x2D) ^ 0x7C);
> +	for (i = 0; i < iv_size; i++) {
> +		iv[i] ^= pattern;
> +		pattern = (pattern << 1) | (pattern >> 7);
> +	}
> +
> +	for (i = 0; i < salt_size; i++)
> +		salt[i] ^= (unsigned char)(generation & 0xFF);
> +
> +	memset(rec_seq, 0, rec_seq_size);
> +}

This isn't wrong (so I'm not requesting to change it), but it seems
way overkill for "we want to fill tls12_crypto_info_aes_gcm_* with
some predictable data based only on key_generation".


> +/* Send TLS 1.3 KeyUpdate handshake message */
> +static int send_tls_key_update(int fd, int request_update)
> +{
> +	char cmsg_buf[CMSG_SPACE(sizeof(unsigned char))];
> +	unsigned char key_update_msg[5];
> +	struct msghdr msg = {0};
> +	struct cmsghdr *cmsg;
> +	struct iovec iov;
> +
> +	key_update_msg[0] = TLS_HANDSHAKE_KEY_UPDATE;
> +	key_update_msg[1] = 0;
> +	key_update_msg[2] = 0;
> +	key_update_msg[3] = 1;
> +	key_update_msg[4] = request_update ? KEY_UPDATE_REQUESTED
> +					   : KEY_UPDATE_NOT_REQUESTED;

I'm still really not convinced we need to bother with things that
would only affect a "proper" userspace.

[...]
> +static int validate_keyupdate(const char *buf, int len)
> +{
> +	if (len != 5) {
> +		printf("KeyUpdate: expected 5 bytes, got %d\n", len);
> +		return -1;
> +	}
> +
> +	if ((unsigned char)buf[0] != TLS_HANDSHAKE_KEY_UPDATE) {
> +		printf("Expected KeyUpdate (0x%02x), got 0x%02x\n",
> +		       TLS_HANDSHAKE_KEY_UPDATE, (unsigned char)buf[0]);
> +		return -1;
> +	}
> +
> +	if (buf[1] != 0 || buf[2] != 0 || buf[3] != 1) {
> +		printf("KeyUpdate: bad length field %02x%02x%02x\n",
> +		       (unsigned char)buf[1], (unsigned char)buf[2],
> +		       (unsigned char)buf[3]);
> +		return -1;
> +	}

And same here.

> +	if ((unsigned char)buf[4] != KEY_UPDATE_NOT_REQUESTED &&
> +	    (unsigned char)buf[4] != KEY_UPDATE_REQUESTED) {
> +		printf("KeyUpdate: invalid request_update value %u\n",
> +		       (unsigned char)buf[4]);
> +		return -1;
> +	}

And here.

What type of issue do you expect to identify by validating the
contents of the key update message that your test program is sending
to itself?


> +	printf("Received TLS KeyUpdate (request_update=%u)\n",
> +	       (unsigned char)buf[4]);
> +	return 0;
> +}
> +

...
> +static int do_client(void)
> +{
> +	char *buf = NULL, *echo_buf = NULL;
> +	int max_size, rekey_interval;
> +	ssize_t echo_total, echo_n;
> +	int csk = -1, ret, i, j;
> +	struct sockaddr_in sa;
> +	int test_result = -1;
> +	int current_gen = 0;
> +	int next_rekey_at;
> +	ssize_t n;
> +
> +	if (!server_ip) {
> +		printf("ERROR: Client requires -s <ip> option\n");
> +		return -1;
> +	}
> +
> +	max_size = random_size_max > 0 ? random_size_max : send_size;
> +	if (max_size < MIN_BUF_SIZE)
> +		max_size = MIN_BUF_SIZE;
> +	buf = malloc(max_size);
> +	echo_buf = malloc(max_size);
> +	if (!buf || !echo_buf) {
> +		printf("failed to allocate buffers\n");
> +		goto out;
> +	}
> +
> +	csk = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> +	if (csk < 0) {
> +		printf("failed to create socket: %s\n", strerror(errno));

Those failures (malloc, socket, connect, maybe also initial key setup)
could use a common prefix (maybe "SETUP ERROR") to differentate them
from "invalid arguments" and "actual test failure".


[...]
> +	rekey_interval = TEST_ITERATIONS / (num_rekeys + 1);
> +	if (rekey_interval < 1)
> +		rekey_interval = 1;

nit: should never happen since num_rekeys < TEST_ITERATIONS

> +	next_rekey_at = rekey_interval;
> +
> +	for (i = 0; i < TEST_ITERATIONS; i++) {

nit: i itself is never used, only i+1, so iterating from 1 up to
i <= TEST_ITERATIONS would be a tiny bit more readable.

> +		int this_size;
> +
> +		if (random_size_max > 0)
> +			this_size = (rand() % random_size_max) + 1;
> +		else
> +			this_size = send_size;
> +
> +		for (j = 0; j < this_size; j++)
> +			buf[j] = rand() & 0xFF;
> +
> +		n = send(csk, buf, this_size, 0);
> +		if (n != this_size) {
> +			printf("FAIL: send failed: %s\n", strerror(errno));

The test failures messages in the client consistently use "FAIL:" as a
prefix...

[...]
> +static int do_server(void)
> +{
[...]
> +	/* Main receive loop */
> +	while (1) {
[...]
> +			printf("recv failed: %s\n", strerror(errno));
[...]
> +				printf("Failed to send KeyUpdate\n");
[...]
> +				printf("Echo send failed: %s\n",
> +				       strerror(errno));

...but not in the server.


[...]
> +int main(int argc, char *argv[])
> +{
[...]
> +	if (tls_version == TLS_1_2_VERSION && num_rekeys) {
> +		printf("ERROR: TLS 1.2 does not support rekey\n");
> +		return -1;
> +	}

Maybe also a check to make setting random_size_max incompatible with
setting send_size. And probably at least a warning when client-only
options (server_ip, random_size, num_rekeys) are passed to the server
and will be ignored.

The "Client requires -s <ip> option" check would also fit better here,
with the rest of the options checking.



> diff --git a/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py
> new file mode 100755
> index 000000000000..66c5ddfd8125
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py
[...]
> +def verify_tls_counters(stats_before, stats_after, expected_rekeys, is_server):

This is much cleaner now, thanks.


> +def run_tls_test(cfg, cipher="128", tls_version="1.3", rekey=0, buffer_size=None, random_max=None):
> +    port = rand_port()
> +    send_size = random_max or buffer_size
> +
> +    server_args = f"{cfg.bin_remote} server -p {port} -c {cipher} -v {tls_version}"

These variables were called {server,client}_cmd in the previous
version, I wonder why you renamed them to something less accurate
(since it's actually the full command, not just the arguments).

> +    if send_size:
> +        server_args += f" -b {send_size}"
> +
> +    client_args = (f"{cfg.bin_local} client -s {cfg.remote_addr_v['4']} "
> +                   f"-p {port} -c {cipher} -v {tls_version}")
> +    if rekey:
> +        client_args += f" -k {rekey}"
> +    if random_max:
> +        client_args += f" -r {random_max}"
> +    elif send_size:
> +        client_args += f" -b {send_size}"

nit: I find the use of send_size here (instead of directly
buffer_size) slightly confusing, since we just took care of
"random_max was set".

-- 
Sabrina