Re: [PATCH v3 net-next 14/15] selftests: drv-net: test AF_XDP zero-copy with an SKB peer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: drv-net: system state leaked on exception in xsk.py
--

commit 800c14b8cb37c2773db542cb20c9fa28d14761b7
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

selftests: drv-net: test AF_XDP zero-copy with an SKB peer

This commit adds a new test script to run AF_XDP zero-copy tests against
an SKB-mode peer using pairs of xskxceiver processes. It sets up ntuple
filters, reserves huge pages, configures queues out of RSS, and cleans up
the configuration when the tests are complete.

> diff --git a/tools/testing/selftests/drivers/net/hw/xsk.py b/tools/testing/selftests/drivers/net/hw/xsk.py
> new file mode 100755
> index 0000000000000..fd708bb3420db
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/xsk.py
[ ... ]
> +def _steer_udp(cfg, ifname, port, queue, host=None):
> +    """Steer the test flow to queue, return False if ifname cannot."""
> +    ntuple = ethtool(f"-k {ifname}", json=True, host=host)[0]["ntuple-filters"]
> +    if not ntuple["active"]:
> +        if ntuple["fixed"]:
> +            print(f"1..0 # SKIP {ifname} does not support ntuple-filters")
> +            return False
> +
> +        ethtool(f"-K {ifname} ntuple-filters on", host=host)
> +
> +    out = ethtool(f"-N {ifname} flow-type udp4 dst-port {port} action {queue}",
> +                  host=host).stdout

[Severity: Medium]
If the ethtool command fails here, cmd() will raise a CmdExitFailure
exception since fail=True is the default in net/lib/py. Does this leave
ntuple-filters on permanently enabled on the physical NIC, because the
exception prevents appending the rule to cfg.rules for later cleanup?

> +    rule = re.search(r"ID (\d+)", out).group(1)
> +    cfg.rules.append((host, ifname, rule, ntuple["active"]))
> +    return True
[ ... ]
> +def main():
> +    # ksft_run() exits on -l and -h before main() can undo the setup.
> +    if "-l" in sys.argv or "-h" in sys.argv:
> +        sys.exit(f"{sys.argv[0]}: -l and -h are not supported")
> +
> +    with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
[ ... ]
> +        cases = _get_cases(cfg)
> +
> +        # _fixup_args() sets the options that differ between the cases.
> +        cfg.args = ["--hw",
> +                    "-p", cfg.remote_addr_v["4"],
[ ... ]
> +        cfg.local_args = [str(cfg.bin_local),
> +                          "-i", cfg.ifname,
> +                          "-m", "zc",
> +                          "--queue", str(cfg.queue),
> +                          "--peer-mac", cfg.remote_dev["address"]]
> +
> +        ksft_run([ksft_variants(cases)(test_xsk),
> +                  ksft_variants(cases)(test_xsk_busy_poll)], args=(cfg,))
> +
> +        xsk_release_hugepages(saved_hugepages)
> +        _unsteer_udp(cfg)
> +        ethtool(f"-X {cfg.ifname} default")

[Severity: Medium]
If an exception is raised earlier in main(), such as during _get_cases() or
other setup commands, will execution exit the function immediately and bypass
these cleanup calls?

Since this can leak system-wide configurations like hugepage allocations and
hardware NIC RSS/ntuple settings, would it make sense to protect the cleanup
sequence with a try...finally block?

> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014184539.774708-1-maciej.fijalkowski@intel.com?part=14