Re: [PATCH net v2 2/2] selftests: net: add TLS 1.2 hardware offload tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] test_tls_offload() in
  tools/testing/selftests/drivers/net/hw/tls_hw_offload.py skips the…
- [Medium] main() in tls_hw_offload.py raises KsftSkipEx outside
  ksft_run().
- [Low] The comment on HW_NIC_COUNTERS says '*_packets increment once per
  packet the NIC actually encrypted or decrypted ... so they prove the…

--

selftests: net: add TLS 1.2 hardware offload tests

This adds a two-node kTLS hardware offload selftest. A C helper installs
fixed AES-GCM keys on both endpoints, and a Python harness runs echo
traffic over IPv4/IPv6 with different payload sizes, checking the TLS MIB
and NIC ethtool counters to confirm HW offload in each enabled DUT
direction. It also adds handshake and alert record-type regression cases
for the push_pending_record fix.

> 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 0000000000000..37f26e21a7b78
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py
> @@ -0,0 +1,402 @@

[ ... ]

> +# TlsTxDevice/TlsRxDevice in /proc/net/tls_stat only prove tls_dev_add()
> +# accepted the offload:
> +#  - *_packets increment once per packet the NIC actually encrypted or
> +#    decrypted (mlx5 counts gso_segs, not records), so they prove the HW
> +#    crypto path was exercised.

[Severity: Low]
Is this description accurate for the TX side? Documentation/networking/
tls-offload.rst defines the counter as:

  tx_tls_encrypted_packets - number of TX packets passed to the device
  for encryption of their TLS payload.

mlx5 increments it in the driver xmit path while it builds the
descriptor. It does not wait for a hardware completion:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c:mlx5e_ktls_handle_tx_skb() {
    ...
    stats->tls_encrypted_packets += skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
    ...
}

So the ksft_ge() on tx_tls_encrypted_packets in check_hw_crypto() shows
that the driver handed TLS packets to the offload path. It does not show
that the NIC encrypted them. A broken HW encrypt would most likely still
be caught by the peer's TlsDecryptError check or the echo memcmp(). The
gap is only in what this comment and the "NIC reported no HW crypto"
message claim.

Could the comment be reworded to match the documented semantics?

> +#  - *_ctx increment once per tls_dev_add().
> +# Missing NIC crypto counters remain optional.

[ ... ]

> +# Drivers whose tls_dev_add() accepts only AES-GCM-128: nfp
> +# (nfp_net_cipher_supported()), cxgb4 (chcr_ktls), funeth.
> +GCM128_ONLY_DRIVERS = {"nfp", "nfp_netvf", "cxgb4", "funeth"}

[ ... ]

> +def test_tls_offload(cfg, cipher, tls_version, ipver):
> +    """Cipher/version matrix over the HW offload data path."""
> +    # The tls-hw-{tx,rx}-offload feature bits do not advertise cipher
> +    # support, and there is no uAPI to query it. Skip AES-GCM-256 on drivers
> +    # whose tls_dev_add() accepts only AES-GCM-128; keep the HW assertions
> +    # for every other driver/cipher pair.
> +    if tls_version == "1.2" and cipher == "256":
> +        info = cmd(f"ethtool -i {cfg.ifname}", fail=False).stdout
> +        for line in info.splitlines():
> +            name, _, value = line.partition(':')
> +            driver = value.strip()
> +            if name.strip() == 'driver' and driver in GCM128_ONLY_DRIVERS:
> +                raise KsftSkipEx(f"{driver} TLS offload supports only AES-GCM-128")
> +
> +    run_tls_test(cfg, cipher=cipher, tls_version=tls_version, ipver=ipver)

[Severity: Medium]
Can the tls12_aes256 variant report a false failure on mlx5 devices that
lack the AES-GCM-256 capability?

On mlx5, AES-GCM-256 support is a per-device capability bit, not a
property of the driver. mlx5e_is_ktls_device() turns on the tls-hw
offload features when the device reports either tls_1_2_aes_gcm_128 or
tls_1_2_aes_gcm_256. mlx5e_ktls_add() then rejects any cipher the device
lacks:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.h:mlx5e_ktls_type_check() {
    ...
    case TLS_CIPHER_AES_GCM_256:
        if (crypto_info->version == TLS_1_2_VERSION)
            return MLX5_CAP_TLS(mdev,  tls_1_2_aes_gcm_256);
    ...
}

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c:mlx5e_ktls_add() {
    ...
    if (!mlx5e_ktls_type_check(mdev, crypto_info))
        return -EOPNOTSUPP;
    ...
}

When that happens, do_tls_setsockopt_conf() in net/tls/tls_main.c falls
back to tls_set_sw_offload() and bumps TlsTxSw/TlsRxSw. setsockopt()
still succeeds, so the C helper sees no error.

mlx5_core is not in GCM128_ONLY_DRIVERS, so check_path() with
require_hw=True then fails with "HW offload not engaged", even though the
device is behaving as designed.

Would it be more robust to detect this cipher-specific SW fallback
(Device delta of 0, Sw delta of at least 1, no TlsDecryptError) and raise
KsftSkipEx, rather than relying on a list of driver names?

[ ... ]

> +def main() -> None:
> +    """Set up the DUT/peer environment and run the offload test suites."""
> +    with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
> +        cfg.bin_local = cfg.test_dir / "tls_hw_offload"
> +        if not cfg.bin_local.exists():
> +            raise KsftSkipEx(f"tls_hw_offload binary not found at {cfg.bin_local}")
> +        cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
> +        check_tls_support(cfg)
> +        cfg.cleanup_failed = False
> +
> +        ksft_run([test_tls_offload, test_tls_offload_sizes,
> +                  test_tls_offload_record_type], args=(cfg, ))
> +    ksft_exit()

[Severity: Medium]
Does raising KsftSkipEx here, outside ksft_run(), turn the expected skip
cases into failures?

The missing-binary check raises directly. check_tls_support() also
raises KsftSkipEx when /proc/net/tls_stat is missing on either host, when
ethtool -k fails, or when the DUT has neither tls-hw-tx-offload nor
tls-hw-rx-offload enabled.

KsftSkipEx only becomes a SKIP result inside the per-case loop of
ksft_run():

tools/testing/selftests/net/lib/py/ksft.py:ksft_run() {
    ...
        except KsftSkipEx as e:
            comment = "SKIP " + str(e)
            cnt_key = 'skip'
    ...
}

NetDrvEnvBase.__exit__() only calls self.__del__() and returns None, so
the exception reaches the interpreter. The result is a traceback, exit
status 1 and no KTAP output, and ksft_exit() is never reached.

Most NICs have no kTLS HW offload, and on all of them the kselftest
runner would record this test as FAIL instead of SKIP.

Could these checks move into the test cases? Alternatively, main() could
catch KsftSkipEx and exit with KSFT_SKIP (4).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020181147.939575-1-rjethwani%40purestorage.com