Re: [PATCH v4 net-next 09/15] selftests: xsk: run one test case per xskxceiver invocation

[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: 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] test_xsk.py reserves the 2 MiB pool only
  (/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages), on the…
- [Low] In xsk_wait_endpoint_exit()
  (tools/testing/selftests/net/lib/py/xsk.py), the TimeoutExpired…
- [Low] Concern: xsk_reserve_hugepages()/xsk_release_hugepages() read,
  modify and restore the global 2 MiB nr_hugepages with no protection…

--

selftests: xsk: run one test case per xskxceiver invocation

This replaces test_xsk.sh and xsk_prereqs.sh with test_xsk.py, which runs
xskxceiver once per mode and case on a fresh veth pair. It moves the huge
page, busy-poll and endpoint wait helpers into net/lib/py/xsk.py.
xskxceiver now requires -m and -t and runs exactly one case.

> diff --git a/tools/testing/selftests/net/lib/py/xsk.py b/tools/testing/selftests/net/lib/py/xsk.py
> new file mode 100644
> index 0000000000000..ffaf4b2d35b9d
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/py/xsk.py
> @@ -0,0 +1,73 @@

[ ... ]

> +NR_HUGEPAGES = 32
> +# xskxceiver maps the UMEMs of the UNALIGNED cases with MAP_HUGE_2MB.
> +HUGEPAGES = "/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages"

[Severity: Medium]
This comment, and the commit message line "test_xsk.py reserves 32 huge
pages of 2 MiB, the size that xskxceiver maps the UNALIGNED UMEMs with",
hold for thread_common_ops(). Do they also hold for the probe that decides
whether the UNALIGNED cases can run?

hugepages_present() in lib/xsk/test_xsk.c does:

	bufs = mmap(NULL, mmap_sz, PROT_READ | PROT_WRITE,
		    MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, MAP_HUGE_2MB);

Here MAP_HUGE_2MB goes in as the offset and is not OR-ed into the flags.
That leaves no size bits in flags, so the kernel uses the default hstate.

Take a host booted with default_hugepagesz=1G, or arm64 with 64K pages.
The probe checks the wrong pool there. The 0x54000000 offset is also not
aligned to the default huge page size, so the mmap fails with -EINVAL.

On such a host the sequence is:

  xsk_reserve_hugepages() fills the 2 MiB pool and returns True
  _get_cases() keeps the UNALIGNED cases
  detect_ifobj_caps()->hugepages_present() fails
  XSK_CAP_UNALIGNED stays clear
  testapp_validate_traffic() prints "No huge pages present." and skips

Every UNALIGNED case then skips, even though the 2 MiB pages were reserved.
The docstring says these cases are left out only without huge pages.

The probe was already wrong before this patch, but this patch is the first
to rely on xskxceiver using the 2 MiB pool. Should hugepages_present() put
MAP_HUGE_2MB in the flags, the way thread_common_ops() does?

> +
> +
> +def xsk_reserve_hugepages(saved, host=None):

[ ... ]

> +    have = cmd(f"cat {HUGEPAGES}", fail=False, host=host)
> +    if have.ret:
> +        return False
> +    have = int(have.stdout)
> +    if have >= NR_HUGEPAGES:
> +        return True
> +    saved.append((host, have))
> +    cmd(f"echo {NR_HUGEPAGES} > {HUGEPAGES}", shell=True, fail=False,
> +        host=host)

[ ... ]

> +def xsk_release_hugepages(saved):
> +    """Restore the counts that xsk_reserve_hugepages() added to saved, the
> +    last one first, as two hosts can share the pool."""
> +    for host, have in reversed(saved):
> +        cmd(f"echo {have} > {HUGEPAGES}", shell=True, fail=False, host=host)

[Severity: Low]
This probably isn't a problem in practice. The kselftest runner runs tests
one at a time, and saving and restoring nr_hugepages is the usual pattern
(mm's run_vmtests.sh does the same).

Still, the 2 MiB nr_hugepages count is global, and it is read, changed and
restored here with no coordination. If something else on the host changes
it during the run, would this restore overwrite that change?

[ ... ]

> +def xsk_wait_endpoint_exit(endpoint, label, timeout):

[ ... ]

> +    try:
> +        endpoint.process(terminate=False, fail=False, timeout=timeout)
> +    except subprocess.TimeoutExpired:
> +        endpoint.proc.kill()
> +        endpoint.process(terminate=False, fail=False)

[Severity: Low]
Can this second process() call time out too? That could happen if
xskxceiver is in uninterruptible sleep, for example blocked in the kernel on
a hung page_pool or XDP teardown, so the SIGKILL does not take effect.

cmd.process() defaults to timeout=20, so this is another bounded
proc.communicate(timeout=20). A TimeoutExpired raised inside this handler
is not caught by the except BaseException clause next to it.

The exception would then escape without the "killed after" log, without the
captured stdout and stderr, and with the child left unreaped. ksft_run()
would mark the case failed with a traceback.

The case would also take about 2 x CASE_TIMEOUT. The commit message says:

    A case is killed and fails after 20s, so that the run ends within
    the net timeout even if all cases hang.

> +        ksft_pr(f"killed after {timeout}s", line_pfx=f"{label}|")
> +    except BaseException:
> +        endpoint.proc.kill()
> +        endpoint.proc.wait()
> +        raise

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016174338.787135-1-maciej.fijalkowski%40intel.com