Re: [PATCH v14 9/9] selftests: net: add TLS hardware offload test

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: add TLS hardware offload test

This commit adds a two-node kTLS HW offload selftest under
tools/testing/selftests/drivers/net/hw/. A C helper binary acts as TLS
client or server, and a Python harness drives it and verifies TLS stat
counters across TLS 1.2/1.3 and AES-GCM-128/256 with rekey variants.

> 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..2b82e6af55ef
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.c

[ ... ]

> +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)
> +{
> +	int i;
> +
> +	for (i = 0; i < key_size; i++)
> +		key[i] ^= generation;
> +	for (i = 0; i < iv_size; i++)
> +		iv[i] ^= generation;
> +	for (i = 0; i < salt_size; i++)
> +		salt[i] ^= generation;
> +	memset(rec_seq, 0, rec_seq_size);
> +}

[Low]
Does this XOR truncate the generation to 8 bits? key[] is unsigned char,
so key[i] ^= generation only mixes in the low byte of the int. Generations
that differ only in their high bytes (0 vs 256, 1 vs 257, ...) end up
producing the same key, iv, and salt:

	for (i = 0; i < key_size; i++)
		key[i] ^= generation;

Currently the Python harness caps rekeys at 50, so this is not exercised,
but parse_int_arg accepts -k up to INT_MAX. A manual run with -k 256 or
beyond would silently install duplicate keys across generations. Should
the per-byte mix incorporate more of the generation value, or should the
accepted range be bounded to keep generations distinct?

[ ... ]

> +static int recv_tls_message(int fd, char *buf, size_t buflen, int *record_type,
> +			    int flags)
> +{
> +	char cmsg_buf[CMSG_SPACE(sizeof(unsigned char))];
> +	struct msghdr msg = {0};
> +	struct cmsghdr *cmsg;
> +	struct iovec iov;
> +	int ret;
> +
> +	iov.iov_base = buf;
> +	iov.iov_len = buflen;
> +
> +	msg.msg_iov = &iov;
> +	msg.msg_iovlen = 1;
> +	msg.msg_control = cmsg_buf;
> +	msg.msg_controllen = sizeof(cmsg_buf);
> +
> +	ret = recvmsg(fd, &msg, flags);
> +	if (ret <= 0)
> +		return ret;
> +
> +	cmsg = CMSG_FIRSTHDR(&msg);
> +	if (cmsg && cmsg->cmsg_level == SOL_TLS &&
> +	    cmsg->cmsg_type == TLS_GET_RECORD_TYPE)
> +		*record_type = *((unsigned char *)CMSG_DATA(cmsg));
> +
> +	return ret;
> +}

[ ... ]

> +static int check_ekeyexpired(int fd)
> +{
> +	char buf[MIN_BUF_SIZE];
> +	int ret;
> +
> +	ret = recv(fd, buf, sizeof(buf), MSG_DONTWAIT);
> +	if (ret == -1 && errno == EKEYEXPIRED) {
> +		printf("recv() returned EKEYEXPIRED as expected\n");
> +		return 0;
> +	} else if (ret == -1 && errno == EAGAIN) {
> +		printf("recv() returned EAGAIN (no pending data)\n");
> +		return 0;
> +	} else if (ret > 0) {
> +		printf("FAIL: recv() returned %d bytes, expected EKEYEXPIRED\n",
> +		       ret);
> +		return -1;
> +	} else {
> +		printf("FAIL: recv() returned unexpected error: %s\n",
> +		       strerror(errno));
> +		return -1;
> +	}
> +}

[Low]
Is this errno set complete for burst mode? check_ekeyexpired() is called
from server_rekey() (burst mode included) and only accepts EKEYEXPIRED and
EAGAIN. The comment in server_verify_burst() and the comment in
verify_tls_counters() in tls_hw_offload.py both describe records straddling
the rekey boundary as causing a transient EBADMSG stashed on sk_err:

	/* A short data return under MSG_WAITALL means a follow-on
	 * record failed to decrypt mid-recv (kTLS returns the prior
	 * decrypted bytes and stashes -EBADMSG on sk_err). Probe
	 * with one more recv to surface the underlying error.
	 */

If kTLS surfaces EBADMSG (or another errno) on the boundary recv() in
check_ekeyexpired(), wouldn't the burst rekey path FAIL spuriously even
though the harness elsewhere acknowledges this is expected?

[ ... ]

> +static int do_server(void)
> +{
> +	int lsk = -1, csk = -1;
> +	ssize_t n, total = 0;
> +	int test_result = -1;
> +	int current_gen = 0;
> +	int recv_count = 0;
> +	int send_iter = 1;
> +	char *buf = NULL;
> +	int record_type;
> +	int recv_flags;
> +	int buf_size;

[Low]
Should record_type be initialized here? recv_tls_message() only writes
*record_type when CMSG_FIRSTHDR is non-NULL and matches SOL_TLS /
TLS_GET_RECORD_TYPE; the sibling recv_tls_keyupdate() declares it as
int record_type = 0. In do_server(), record_type is left uninitialized
and the main loop branches on:

	if (record_type == TLS_RECORD_TYPE_HANDSHAKE) {

If the cmsg ever arrives truncated or absent, would the read of
record_type be undefined and potentially misclassify a data record as a
KeyUpdate (or vice versa)?

[ ... ]

> +	buf_size = send_size;
> +	if (buf_size < MIN_BUF_SIZE)
> +		buf_size = MIN_BUF_SIZE;
> +	buf = malloc(buf_size);

[ ... ]

> +static int server_verify_burst(int fd, char *buf, int buf_size,
> +			       ssize_t n, int send_iter)
> +{
> +	unsigned char expect = send_iter & 0xFF;
> +	int j;
> +
> +	if (n != send_size) {
> +		int record_type = 0;
> +		ssize_t n2;
> +
> +		/* A short data return under MSG_WAITALL means a follow-on
> +		 * record failed to decrypt mid-recv (kTLS returns the prior
> +		 * decrypted bytes and stashes -EBADMSG on sk_err). Probe
> +		 * with one more recv to surface the underlying error.
> +		 */
> +		n2 = recv_tls_message(fd, buf, buf_size, &record_type, 0);
> +		printf("FAIL: short recv in burst mode: got %zd, expected %d (iter %d)\n",
> +		       n, send_size, send_iter);
> +		printf("      follow-up recv: %zd errno=%s\n",
> +		       n2, n2 < 0 ? strerror(errno) : "ok");
> +		return -1;
> +	}

[Low]
Can this cause a spurious failure when -b is small and -B is set? The
recv() target under MSG_WAITALL is buf_size, and do_server() raises
buf_size to MIN_BUF_SIZE (16) when send_size is smaller:

	buf_size = send_size;
	if (buf_size < MIN_BUF_SIZE)
		buf_size = MIN_BUF_SIZE;

With for example -b 1 -B, the kernel waits for 16 bytes (aggregating
multiple 1-byte data records) and recv returns n=16, but
server_verify_burst() compares against send_size (1):

	if (n != send_size) { ... return -1; }

Should the recv length be capped at send_size in burst mode, or should
send_size < MIN_BUF_SIZE be rejected at parse time so the two cannot
diverge?

[ ... ]

> 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..94dd9d692bb1
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py

[ ... ]

> +def check_tls_support(cfg):
> +    try:
> +        cmd("test -f /proc/net/tls_stat")
> +        cmd("test -f /proc/net/tls_stat", host=cfg.remote)
> +    except CmdExitFailure as e:
> +        raise KsftSkipEx(f"kTLS not supported: {e}")

[Medium]
Should this also gate on the device's TLS HW offload features? The test
lives under drivers/net/hw/, which is a generic NIC test directory used
in CI across many vendors. check_tls_support() only checks for
/proc/net/tls_stat (a kernel-config gate for kTLS itself). It does not
look at NETIF_F_HW_TLS_TX/NETIF_F_HW_TLS_RX or the ethtool features
tls-hw-tx-offload/tls-hw-rx-offload.

verify_tls_counters() then unconditionally requires HW offload on the
DUT:

	dev = stat_diff(before, after, f'Tls{direction}Device')
	sw = stat_diff(before, after, f'Tls{direction}Sw')
	if require_hw:
		if dev < 1:
			ksft_pr(f"FAIL: ...")
			return 1

On any DUT where kTLS runs in software, would this test FAIL rather than
SKIP? Sibling tests (e.g. tso.py) raise KsftSkipEx based on
cfg.hw_features; could the same pattern be used here?

[ ... ]

> +def verify_tls_counters(stats_before, stats_after, expected_rekeys,
> +                        tls_role, is_dut, burst=False):

[ ... ]

> +    # In burst mode, records straddling the rekey boundary cause a transient
> +    # EBADMSG in tls_decrypt_sw() before tls_rx_rekey_retry() succeeds,
> +    # so TlsDecryptError increments are expected.
> +    if not burst:
> +        errors += check_zero(stats_before, stats_after, 'TlsDecryptError', role)
> +
> +    return errors

[Low]
The commit message says the harness verifies "TLS stat counters (RekeyOk,
RekeyReceived, RekeyFallback, RekeyInProgress, DecryptError)". Is
DecryptError still verified in burst mode? The check is skipped entirely
under burst rather than bounded (e.g. by expected_rekeys * N).

Would a regression that produces hundreds of decrypt errors in a burst
run go undetected, given the listed counter set in the commit message?