Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 3
- [Medium] test_psp_ver_diff() in
tools/testing/selftests/drivers/net/hw/psp_gro.py gates on the *count*…
- [Medium] test_psp_after_reconfig()
(tools/testing/selftests/drivers/net/hw/psp_gro.py:132-141) disables…
- [Low] The new HAVE_OPENSSL probe in
tools/testing/selftests/drivers/net/hw/Makefile (lines 16-28) decides…
- [Low] test_psp_ver_diff()
(tools/testing/selftests/drivers/net/hw/psp_gro.py:116-122) claims to…
- [Low] The psp_gro.py module docstring (line 7: "This reuses the gro
binary in --psp mode:") and the new Makefile comment ("psp_gro.py uses…
--
selftests: drv-net: Add PSP HW GRO conformance tests
This adds psp_gro.py, which drives the existing gro helper binary with PSP
associations obtained from a real PSP device, so the sender encrypts in
software and the receiver relies on the NIC for decrypt, decap and HW GRO.
It also plumbs a common_args parameter through gro_lib.run_test() and adds a
libcrypto probe to the hw Makefile.
No Link: tags in the commit header.
> Add PSP conformance tests using the same gro helper binary as other gro
> tests , but in --psp mode. SPIs are procured by psp_gro.py from a real
[Severity: Low]
The commit message, the new Makefile comment ("psp_gro.py uses the gro
binary in PSP mode") and the psp_gro.py docstring all describe a "--psp
mode", but the gro helper has no such option.
In tools/testing/selftests/net/lib/gro.c the only PSP entry in
long_options[] is:
{ "psp-assoc", required_argument, NULL, OPT_PSP_ASSOC },
and psp_enabled is just a side effect of psp_parse_assoc(). _psp_args()
in psp_gro.py correctly emits --psp-assoc ver,spi,key. Could the wording
be changed to --psp-assoc so nobody tries to pass --psp and gets an
unknown-option error from getopt_long()?
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile
> b/tools/testing/selftests/drivers/net/hw/Makefile
> index 6105be8e590f..daa7e52f236e 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -13,6 +13,20 @@ else
> $(warning excluding iouring tests, liburing not installed or too old)
> endif
>
> +# psp_gro.py uses the gro binary in PSP mode, which requires OpenSSL.
> +PKG_CONFIG ?= pkg-config
> +HAVE_OPENSSL := $(shell echo 'int main(void) { return 0; }' | \
> + $(CC) -x c - -include openssl/evp.h \
> + $(shell $(PKG_CONFIG) --cflags --libs libcrypto 2>/dev/null \
> + || echo -lcrypto) \
> + -o /dev/null >/dev/null 2>&1 && echo 1)
> +
> +ifeq ($(HAVE_OPENSSL),1)
> +COND_PROGS += psp_gro.py
> +else
> +$(warning excluding PSP GRO tests, libcrypto not installed)
> +endif
[Severity: Low]
Is the build host the right place to test this condition? The PSP
software encryption runs in the gro binary on the remote endpoint, not on
the build host:
gro_lib.py:_run_gro_bin()
cfg.bin_local = cfg.net_lib_dir / "gro"
cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
...
tx_cmd = f"{cfg.bin_remote} {args} --iface {cfg.remote_ifname}"
...
cmd(tx_cmd, host=cfg.remote)
gro is dynamically linked against libcrypto:
tools/testing/selftests/net/lib/Makefile
$(OUTPUT)/gro: LDLIBS += $(LIBCRYPTO_LIBS)
and Remote.deploy() only scp's the executable:
tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
cmd(f"scp {what} {self.name}:{file_name}")
So on a setup where the build host has libcrypto but the remote endpoint
lacks the runtime library, psp_gro.py is installed and every case fails
with a dynamic linker error on the sender instead of skipping. Would a
runtime check on the remote be preferable here?
[ ... ]
> diff --git a/tools/testing/selftests/drivers/net/hw/psp_gro.py
> b/tools/testing/selftests/drivers/net/hw/psp_gro.py
> new file mode 100755
> index 000000000000..3a069c944ae1
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/psp_gro.py
> @@ -0,0 +1,157 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +"""
> +PSP HW GRO conformance tests.
> +
> +This reuses the gro binary in --psp mode:
[Severity: Low]
Same --psp naming question as in the commit message: the implemented
option in gro.c is --psp-assoc.
> +The sender crafts encapsulated & SW-encrypted PSP packets with receiver's PSP
> +rx-assoc, and the receiver's device decrypts and decapsulates the packets
> +before an AF_PACKET tap gets to analyze them.
> +
> +All GRO conformance tests which could run with PSP are included.
> +"""
[ ... ]
> +def _setup(cfg, version):
> + """Enables PSP on the device under test."""
> + init_psp_dev(cfg)
> + require_version(cfg, version)
> +
> +
> +def _psp_args(cfg, versions):
> + """Produces PSP associations as gro binary --psp-assoc arguments."""
> + keys = [_psp_assoc(cfg, ver) for ver in versions]
> + return [f"--psp-assoc {ver},{key['spi']:x},{key['key'].hex()}"
> + for ver, key in zip(versions, keys)]
> +
> +
> +def _run(cfg, test_name, protocol, versions):
> + """Sets up, associates and runs one gro test case under PSP + HW GRO."""
> + _setup(cfg, max(versions))
> +
> + run_test(cfg, "hw", protocol, test_name,
> + common_args=_psp_args(cfg, versions))
[ ... ]
> +@ksft_variants(_ip_variants())
> +def test_psp_ver_diff(cfg, protocol):
> + """Frames from two different PSP versions must not coalesce."""
> + init_psp_dev(cfg)
> + if len(cfg.psp_info['psp-versions-cap']) < 2:
> + raise KsftSkipEx("Device supports a single PSP version")
> +
> + _run(cfg, "psp_ver_diff", protocol, [0, 1])
[Severity: Medium]
Does the guard here match what the test then asks for? It counts how
many versions the device advertises, but the following call hardcodes
versions 0 and 1.
psp-versions-cap is an independent flag bitmask in
Documentation/netlink/specs/psp.yaml:
- name: psp-versions-cap
doc: Bitmask of PSP versions supported by the device.
type: u32
enum: version
enum-as-flags: true
so the supported set is not required to be a contiguous prefix starting
at 0. On a device advertising, say, hdr0-aes-gcm-128 (0) and
hdr0-aes-gmac-128 (2), the length check passes and then:
psp_gro.py:_run()
_setup(cfg, max([0, 1])) /* == 1 */
psp_lib.py:require_version()
name = cfg.pspnl.consts["version"].entries_by_val[version].name
if name in cfg.psp_info['psp-versions-cap']:
return
...
raise KsftSkipEx("PSP version not supported", name)
which skips with a reason unrelated to the guard, and the cross-version
non-coalescing assertion never runs on a device that could run it.
The existing peer test picks the versions from the actual caps instead:
tools/testing/selftests/drivers/net/psp.py:assoc_version_mismatch()
versions = [cfg.pspnl.consts["version"].entries[v].value for v in
versions]
Would deriving the two versions from cfg.psp_info['psp-versions-cap'] be
better here? Today mlx5 happens to advertise exactly versions 0 and 1
(psp_supported_versions[] = { GCM_128, GCM_256 }), so this only bites the
next driver that advertises a GMAC version.
[Severity: Low]
This isn't a bug, but does psp_ver_diff assert anything that psp_spi_diff
does not? The two associations passed here differ in version and in SPI,
since each rx_assoc allocates a fresh device SPI
(psp_nl_rx_assoc_doit() -> psd->ops->rx_spi_alloc()).
On the gro.c side both cases share the same sender helper, which only
flips psp_idx:
send_psp_assoc_switch()
create_packet(buf, PAYLOAD_LEN, 0, PAYLOAD_LEN, 0);
psp_idx = 1;
write_packet(fd, buf, pkt_size, daddr);
psp_idx = 0;
and the receive expectations are byte-for-byte the same as psp_spi_diff
(correct_payload[0] = correct_payload[1] = PAYLOAD_LEN, then
check_recv_pkts(rxfd, correct_payload, 2)); only the printf text differs.
A version-only difference does not look constructible from user space, so
this may be the closest achievable approximation - it is just worth noting
that the case would pass even if the GRO path ignored the version field.
[ ... ]
> +@ksft_variants(_ip_variants())
> +def test_psp_after_reconfig(cfg, protocol):
> + """Verifies that decap still works after PSP off + on."""
> + _setup(cfg, 0)
> +
> + cap = cfg.psp_info['psp-versions-cap']
> + cfg.pspnl.dev_set({'id': cfg.psp_dev_id, 'psp-versions-ena': []})
> + cfg.pspnl.dev_set({'id': cfg.psp_dev_id, 'psp-versions-ena': cap})
[Severity: Medium]
Should the disable here be paired with a defer() for the restore? As
written, the only thing that re-enables PSP is the very next dev_set()
call, with nothing covering an NlError from it or a kill between the two
calls.
init_psp_dev() only registers a rollback when the cached psp_info says the
enabled set differs from the caps:
tools/testing/selftests/drivers/net/psp_lib.py:init_psp_dev()
if cap != ena:
cfg.pspnl.dev_set({'id': cfg.psp_dev_id, 'psp-versions-ena':
cap})
defer(cfg.pspnl.dev_set, {'id': cfg.psp_dev_id,
'psp-versions-ena': ena})
so on a device discovered with PSP already fully enabled (cap == ena)
there is no defer at all. If the re-enable does not happen, the NIC is
left with PSP offload off while cfg.psp_info still caches the old enabled
set, and later init_psp_dev() calls compare against that stale snapshot
and conclude nothing needs enabling - subsequent PSP tests then fail on
rx-assoc/decap in a way that looks like a driver problem.
Would registering the re-enable via defer() right after the disable, and
re-reading the device state rather than trusting the cached psp_info,
address that?
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908065244.3799142-1-tariqt%40nvidia.com
