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

[Rishikesh Jethwani]

On Mon, Nov 2, 2026 at 10:02 AM <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: 1 · Low: 2
>
> 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] The tls12_aes256 variant of test_tls_offload
>   (tls_hw_offload.py:280) requires HW offload in every DUT direction…
> - [Low] Two comments say both helpers' stdout pipes are only drained
>   after the helper exits: run_tls_test() in tls_hw_offload.py ("Both…
> - [Low] When 'Rx' is in tls_hw_directions, check_hw_crypto() asserts that
>   rx_tls_decrypted_packets grew by at least 1, but nothing makes 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 ends; the DUT runs the client and the peer
> echoes the data back. The test checks the TLS MIB counters and the NIC's
> ethtool -S TLS counters to confirm HW offload was used in each enabled
> DUT direction, and it adds handshake and alert record-type regression
> cases.
>
> > 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..78fa764a6ef3f
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/tls_hw_offload.py
> > @@ -0,0 +1,321 @@
>
> [ ... ]
>
> > +def check_hw_crypto(cfg, nic_before, nic_after, hw_directions):
> > +    """Check DUT NIC offload-path packet counts and initial key installs."""
> > +    for direction in hw_directions:
> > +        names = HW_NIC_COUNTERS[direction]
> > +
> > +        key = names['crypto']
> > +        if key in nic_after:
> > +            got = stat_diff(nic_before, nic_after, key)
> > +            ksft_ge(got, 1,
> > +                    comment=f"DUT {direction}: NIC reported no TLS "
> > +                            f"offload packets ({key}={got})")
>
> [Severity: Low]
> When Rx is in hw_directions, can rx_tls_decrypted_packets stay at 0 after
> a short run, even with working hardware?
>
> Nothing waits for the device RX context to become active before the
> peer's echoes arrive. On mlx5, mlx5e_ktls_add_rx() bumps rx_tls_ctx
> synchronously and returns after post_rx_param_wqes(). The steering rule
> that sends the flow to the TLS TIR is installed later, from a workqueue:
>
> mlx5e_ktls_handle_ctx_completion()
>   queue_work(rule->priv->tls->rx_wq, &rule->work)
>     accel_rule_handle_work()
>       mlx5e_accel_fs_add_sk()
>
> Until that rule is in place, records are decrypted in SW. TlsRxDevice
> still grows, but mlx5e_ktls_handle_rx_skb() only bumps the counter on
> the HW-decrypted path:
>
>         case CQE_TLS_OFFLOAD_DECRYPTED:
>                 skb->decrypted = 1;
>                 stats->tls_decrypted_packets++;
>
> If the rule lands mid-stream, a resync is also needed before HW
> decryption starts.
>
> client_connect_tls() starts sending as soon as TLS_RX is installed, and
> cfg.wait_hw_stats_settle() only runs after the traffic has finished.
> Could the small_buf variant (100 round trips of 512 bytes) finish
> entirely inside that window and report a false failure?
>
> This depends on firmware command and workqueue latency, and the commit
> message reports passing runs on ConnectX-6 Dx, so it may only show up as
> an occasional flake.

Agreed that on mlx5 the rule is installed asynchronously after the
context is created.
I haven't seen this on ConnectX-6 Dx across repeated runs.
If it does flake, the failure is visible and attributable, and I'd
rather not add a sleep or a skip.

> [ ... ]
>
> > +    # Both helpers run with their stdout captured in a pipe that is only
> > +    # drained once they exit, so the helper keeps its per-connection output
> > +    # small (see the note on verbose in tls_hw_offload.c); do not pass -V.
>
> [Severity: Low]
> Is this comment accurate for the local client?
>
> Only the remote server is started with bkg(), so its ssh pipe is not
> read until bkg.__exit__() calls process().
>
> The client runs in the foreground via cmd(client_cmd). In the non-background
> case cmd.__init__() calls self.process() right away. _process_terminate()
> then uses self.proc.communicate(timeout=timeout), which drains stdout and
> stderr while it waits for the child to exit.
>
> The comment on verbose in tls_hw_offload.c makes the same claim:
>
>   /* Per-iteration progress output (-V). Off by default: the
>    * harness captures stdout in a pipe that is not drained until the helper
>    * exits, ...
>
> The -V restriction still makes sense for the server. Should both comments
> say that only the background server's pipe goes undrained?

Good catch. The foreground cmd() drains through communicate(),
so only the bkg() server's pipe goes undrained. I'll fix both comments.

> > +    #
> > +    # Both helpers exit on their own within HELPER_TIMEOUT_S (see
> > +    # tls_hw_offload.c), so nothing outlives the variant even on failure.
> > +    with bkg(server_cmd, host=cfg.remote, exit_wait=True):
> > +        wait_port_listen(port, host=cfg.remote)
> > +        cmd(client_cmd)
>
> [ ... ]
>
> > +@ksft_variants([
> > +    KsftNamedVariant("tls12_aes128", "128", "1.2", "4"),
> > +    KsftNamedVariant("tls12_aes256", "256", "1.2", "4"),
>
> [Severity: Medium]
> Can the tls12_aes256 variant fail on hardware and a kernel that are both
> working correctly?
>
> check_tls_support() only looks for "tls-hw-tx-offload: on" and
> "tls-hw-rx-offload: on" in ethtool -k. Those flags don't say which
> ciphers the device accepts, and several in-tree drivers only handle
> AES-GCM-128:
>
>   nfp: nfp_net_cipher_supported() returns false for anything except
>   TLS_CIPHER_AES_GCM_128, so nfp_net_tls_add() returns -EOPNOTSUPP
>
>   chcr: chcr_ktls_save_keys() only handles TLS_CIPHER_AES_GCM_128
>
>   funeth: fun_ktls_add() returns -EOPNOTSUPP for anything except 128
>
>   mlx5: mlx5e_ktls_type_check() checks a separate firmware capability
>   for 256:
>
>         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);
>
> When the driver declines, do_tls_setsockopt_conf() falls back to SW
> without reporting an error:
>
> net/tls/tls_main.c:do_tls_setsockopt_conf() {
>     ...
>                 rc = tls_set_device_offload(sk);
>                 conf = TLS_HW;
>                 if (!rc) {
>                         TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
>                         TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
>                 } else {
>                         rc = tls_set_sw_offload(sk, 1,
>                                                 update ? crypto_info : NULL);
>     ...
>                                 TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
>     ...
> }
>
> check_path() then requires ksft_ge(dev, 1) for that direction and fails
> with "DUT Tx: HW offload not engaged (Device=0, Sw=1)".
>
> Would it make sense to skip this variant when the DUT falls back to SW
> for this cipher, rather than reporting a failure?

That's intended. A driver that doesn't support AES-GCM-256 offload
falls back to SW, and the test reports that as a failure.
Per Jakub's earlier feedback on this series, HW tests don't try to
skip for every device capability gap.
The automated triage separates regressions from failures caused by
missing support.

>
> > +    KsftNamedVariant("tls12_aes128_ip6", "128", "1.2", "6"),
> > +])
> > +def test_tls_offload(cfg, cipher, tls_version, ipver):
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029172302.2944955-1-rjethwani%40purestorage.com

pw-bot: cr