Re: [PATCH net v8 4/4] selftests: net: hsr: verify GRO policy and ordered forwarding

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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] topo() in
  tools/testing/selftests/net/hsr/hsr_gro_superpacket.py treats a…
- [Low] ordered_wrap() in
  tools/testing/selftests/net/hsr/hsr_gro_superpacket.py checks the…

--

selftests: net: hsr: verify GRO policy and ordered forwarding

This commit adds two HSR/PRP selftests. Each is a shell entry point that
execs a shared Python helper. hsr_gro_superpacket.sh covers the GRO policy
on member ports, refusal of forwarding port types, and TCP over the
interlink and PRP LANs. hsr_ordered_forwarding.sh covers the ordering of
local and interlink data, supervision frames, concurrent producers and the
sequence number wrap.

> diff --git a/tools/testing/selftests/net/hsr/hsr_gro_superpacket.py b/tools/testing/selftests/net/hsr/hsr_gro_superpacket.py
> new file mode 100755
> index 0000000000000..23feb0ce4df4c
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_gro_superpacket.py
> @@ -0,0 +1,1376 @@

[ ... ]

> +def ordered_wrap(lab):
> +    """The interlink sequence number wraps with both lanes intact."""
> +    topology = topo(lab)
> +    caps = ordered_lanes(lab, topology)
> +    sender = ordered_socket(lab, topology["tx"], "100.64.2.1")
> +    probe = ordered_send(lab, sender, 4, 0, 1, 5130)
> +    # This only selects the warmup count; final loss-free captures certify it.
> +    rows = [udp_rows(cap, {5130}) for cap in caps]
> +    require(all(len(lane) == 1 for lane in rows) and
> +            rows[0][0]["seq"] == rows[1][0]["seq"], "wrap probe missing or unequal")

[Severity: Low]

Could this check run before the probe has reached the da/db captures?
The only wait between sending the probe and reading the captures is the
fixed drain at the end of ordered_send(), and it has no completion
condition:

    ordered_send() {
        ...
        lab.observe(0.1)
        return expected
    }

The probe goes from t0 over the veth to dil, through the HSR rx_handler in
NET_RX softirq, and then out on da/db. That softirq usually runs
synchronously inside sendto(). If net_rx_action() runs out of budget, the
remaining work is handed to ksoftirqd.

On a loaded CI VM, can ksoftirqd take longer than 100 ms to get scheduled?
If so, the case would fail with "wrap probe missing or unequal" even though
the kernel behaves correctly.

The other packet checks in the script wait through lab.finish() with a
completion predicate. Would polling lab.observe() until both lanes hold the
probe, bounded by lab.remaining(), close this window?

> +    start = rows[0][0]["seq"]

[ ... ]

> +def topo(lab):
> +    """SAN - interlink - HSR DUT - two LANs - receiver namespaces."""
> +    tx, dut, rx = lab.ns(), lab.ns(), lab.ns()
> +    for ns, dev, peer_ns, peer in ((tx, "t0", dut, "dil"),
> +                                   (dut, "da", rx, "ra"), (dut, "db", rx, "rb")):
> +        lab.veth(ns, dev, peer_ns, peer)
> +        for owner, link in ((ns, dev), (peer_ns, peer)):
> +            lab.ip(owner, "link", "set", "dev", link, "up")
> +    hsr_attach(lab, dut, "hm", "da", "db", "dil")
> +    attributes = link_state(lab, dut, "hm")["linkinfo"]["info_data"]
> +    require(attributes.get("proto") == 0 and attributes.get("version") == 1 and
> +            all(attributes.get(key) == value for key, value in
> +                (("slave1", "da"), ("slave2", "db"), ("interlink", "dil"))),
> +            "HSR topology has unexpected protocol/version")

[Severity: Medium]

What happens here with an iproute2 that does not print the HSR version
attribute?

The kernel has only sent IFLA_HSR_VERSION for non-PRP devices since commit
16a2206354d169 ("net/hsr: add protocol version to fill_info output"), in
hsr_fill_info():

    else if (nla_put_u8(skb, IFLA_HSR_VERSION, hsr->prot_version))

Whether a "version" key shows up in the ip -j -d link show output then
depends on the iproute2 HSR print code. Older iproute2 releases print
slave1, slave2, interlink, seq_nr, supervision and proto, but not version.
In that case attributes.get("version") returns None and require() raises
ValueError.

ValueError is in CASE_ERRORS, so main() reports the case as not ok. It then
marks every later case as failed too:

    for later, (other, _) in enumerate(cases[number:], number + 1):
        print(f"not ok {later} - {other}: stopped after prior failure", flush=True)

In gro mode, "natural TCP interlink" is the first caller of topo(). The six
"PRP LAN" cases after it never call topo(), but they would still be
reported as not ok. In ordered mode all five cases call topo(), so the
whole test would fail.

A missing interlink option in iproute2 is already reported as SKIP through
interlink_unsupported(). Could a missing "version" key get the same
treatment, or simply not be required?

> +    lab.ip(tx, "addr", "add", "100.64.2.1/24", "dev", "t0")

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107220544.20-1-xiexinet%40gmail.com