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

[Rishikesh Jethwani]

On Sat, Oct 24, 2026 at 12:55 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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

pw-bot: cr