Re: [PATCH net v7 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 10 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 7 · Low: 3

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 new 'busy-handler rollback' case (rollback_macvlan() in
  hsr_gro_superpacket.py) needs a macvlan device, but…
- [Medium] set_gro() accepts a rejected GRO request only when ethtool's
  stdout is exactly "Actual changes:\nrx-gro: off [requested on]\n" and…
- [Medium] decode() treats any non-HSR frame whose last two bytes are
  0x88fb as PRP (`elif end >= off + 6 and raw[-2:] == b"\x88\xfb":`) and…
- [Medium] Capture.__init__ creates the AF_PACKET socket with protocol
  htons(ETH_P_ALL) (line 120), then calls setsockopt twice, and only…
- [Medium] SIGINT/SIGTERM are handled by interrupt(), which raises
  InterruptedError in the main thread at the next bytecode boundary.
- [Medium] Lab.finish() decides transmission is complete after 100 ms
  with no captured frame (`if empty and time.monotonic() - quiet >= 0.1:…
- [Medium] Lab.finish() removes the capture hooks one capture at a time
  (`for cap in active: ... cap.socket.bind(("lo", 0))`).
- [Low] Lab.finish() takes every capture in self.captures that is not yet
  complete, so a case that raises Skip after creating captures but…
- [Low] hsr_gro_superpacket.py contains about 1088 lines of its own
  framework: a hand-written TAP emitter, ctypes setns() namespace entry,…
- [Low] In ordered_concurrent(), if a producer fails before reaching the
  barrier (for example, os.sched_setaffinity() raises), its except block…

--

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

This adds a Python helper with two shell entry points. It tests HSR/PRP
member GRO handling across attach, detach and failed attachment, the
master's GSO type mask, and TCP/UDP GSO segmentation through HSR and PRP.
It also uses AF_PACKET captures on every lane to check ordered forwarding
of local, interlink, supervision and concurrent traffic, plus sequence
number wrap.

[ ... ]

> diff --git a/tools/testing/selftests/net/hsr/config b/tools/testing/selftests/net/hsr/config
> index 205cc4d3d64bd..724fbe3a6eab7 100644
> --- a/tools/testing/selftests/net/hsr/config
> +++ b/tools/testing/selftests/net/hsr/config
> @@ -2,5 +2,6 @@ CONFIG_BRIDGE=y
>  CONFIG_HSR=y
>  CONFIG_IPV6=y
>  CONFIG_NET_SCH_NETEM=m
> +CONFIG_PACKET=y
>  CONFIG_VETH=y
>  CONFIG_VLAN_8021Q=m

[Severity: Medium]
Should CONFIG_MACVLAN be added here too?

The busy-handler rollback case in rollback_macvlan() creates a macvlan on
top of m:

    result = lab.ip(ns, "link", "add", "link", "m", "name", "mv",
                    "type", "macvlan", "mode", "bridge", check=False)

On a kernel built from this fragment, that command fails with "Unknown
device type", and rollback_macvlan() turns the failure into a SKIP:

    if "unknown device type" in error or "operation not supported" in error:
        raise Skip("macvlan device kind unavailable: " + result.stderr.strip())

No other case reaches the -EBUSY path in hsr_portdev_setup(), where
netdev_rx_handler_register() fails because macvlan already owns the
rx_handler. CI kernels built from net/hsr/config would lose that coverage
and still report success.

> 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..d39bf1af291f8
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_gro_superpacket.py
> @@ -0,0 +1,1088 @@

[ ... ]

> +def decode(raw, ancillary=()):

[ ... ]

> +    end = len(raw)
> +    if proto == 0x892f:
> +        require(off + 6 <= end, "short HSR tag")
> +        field, seq, proto = struct.unpack_from("!HHH", raw, off)
> +        row.update(seq=seq, path=field >> 12, tag=True, encoding="hsr")
> +        expected = len(raw) - 14 - (4 if raw[12:14] == b"\x81\x00" else 0)
> +        require(field & 0xfff == expected, "HSR LSDU size differs")
> +        off += 6
> +    elif end >= off + 6 and raw[-2:] == b"\x88\xfb":
> +        seq, field, suffix = struct.unpack_from("!HHH", raw, end - 6)
> +        expected = len(raw) - 14 - (4 if raw[12:14] == b"\x81\x00" else 0)
> +        require(field & 0xfff == expected, "PRP LSDU size differs")
> +        row.update(seq=seq, path=field >> 12, tag=True, encoding="prp")
> +        end -= 6

[Severity: Medium]
Can an ordinary untagged frame that happens to end in 88:fb fail the whole
run here?

decode() treats any non-HSR frame whose last two bytes are 0x88fb as PRP,
and an LSDU mismatch then raises an error. The kernel handles a mismatched
LSDU size by falling back to a plain frame, in prp_fill_frame_info():

    if (rct &&
        prp_check_lsdu_size(skb, rct, frame->is_supervision)) {
        ...
        return 0;
    }
    return HSR_FRAME_PLAIN;

Capture.drain() calls decode() on every frame that matches pkttype. That
includes captures on non-PRP links such as dil, t0 and m/mv.

A pure TCP ACK ends in the low 16 bits of TSecr. An IPv6 NS/RS/MLD frame
can end in the last two bytes of a random interface MAC. In either case
drain() raises ValueError, the case fails, and every later case is
reported as not ok.

Would it be better to treat an LSDU mismatch as a plain frame, as the
kernel does?

[ ... ]

> +class Capture:
> +    def __init__(self, lab, ns, dev, pkttype=None):
> +        self.lab, self.rows, self.seen = lab, [], 0
> +        self.ns, self.dev, self.packets = ns, dev, None
> +        self.pkttype, self.name = pkttype, ns + "/" + dev
> +        self.complete, self.drops, self.truncated = False, None, 0
> +        self.last_seen = time.monotonic()
> +        self.socket = lab.sock(ns, socket.AF_PACKET, socket.SOCK_RAW, socket.htons(3))
> +        self.socket.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 8 * 1024 * 1024)
> +        self.socket.setsockopt(263, 8, 1)
> +        self.socket.bind((dev, 0))

[Severity: Medium]
Can frames from other interfaces end up in this capture?

Creating the socket with htons(ETH_P_ALL) registers the hook right away in
packet_create(), with no device restriction:

    if (proto) {
        po->prot_hook.type = proto;
        __register_prot_hook(sk);
    }

Frames that arrive on any device in the namespace before bind((dev, 0))
stay in the socket queue. Capture.drain() checks only addr[2] (pkttype)
and never addr[0] (ifname):

    if self.pkttype is None or addr[2] == self.pkttype:
        self.rows.append(decode(raw, anc))

For example, while ordered_lanes() is creating the db capture, a
supervision or ND frame sent on da can be recorded as db traffic.
verify_supervision() then fails with "wrong supervision tag kind/path", a
protocol_order() error, or "data/supervision LAN order differs".

Would either of these avoid it: creating the socket with protocol 0 and
passing ETH_P_ALL to bind(), or filtering on addr[0] in drain()?

[ ... ]

> +class Lab:
> +    def __init__(self, deadline):

[ ... ]

> +    def ns(self, label):
> +        name = "hsrp4-%s-%d" % (self.token, len(self.names))

[Severity: Low]
This isn't a bug, but this file carries its own test framework: a
hand-written TAP emitter, namespace entry through ctypes setns(), a
manager for namespace/socket/capture lifetimes, and ip/ethtool wrappers.

tools/testing/selftests/net/lib/py already provides ksft_run, NetNS,
NetNSEnter, defer, ip, ethtool and cmd. Could this use net/lib/py
instead? The custom layer has no per-case teardown, which leads to the
cross-case coupling in Lab.finish() noted below.

Some of the code is unused. Lab.ns() ignores label, and tcp_transfer()
never uses its captures argument.

[ ... ]

> +    @contextmanager
> +    def enter(self, ns):
> +        descriptor = os.open("/var/run/netns/" + ns, os.O_RDONLY)
> +        try:
> +            self.setns(descriptor)
> +            yield
> +        finally:
> +            try:
> +                self.setns(self.original)
> +            except BaseException:
> +                self.restore_failed = True
> +                raise
> +            finally:
> +                os.close(descriptor)

[Severity: Medium]
Can a SIGINT or SIGTERM here leak every namespace the test owns?

interrupt() raises InterruptedError at the next bytecode boundary. If that
happens during or just after self.setns(self.original), restore_failed is
set even though the libc setns() call already succeeded.

Lab.close() then skips every ip netns del and never retries the restore:

    if self.restore_failed:
        errors.append("namespace restoration failed; no namespace commands attempted")

All hsrp4-* namespaces are left behind, along with their veths and HSR
devices.

main()'s finally block has a similar window:

    finally:
        for sig in (signal.SIGINT, signal.SIGTERM):
            signal.signal(sig, signal.SIG_IGN)
        try:
            lab.close()

The handlers are switched one at a time. A second signal that arrives
before both are ignored raises InterruptedError inside the finally block,
and lab.close() never runs.

The commit message says:

    INT/TERM release owned resources and preserve failures.

Would blocking the signals with signal.pthread_sigmask() around these
sections make that statement hold?

[ ... ]

> +    def finish(self):
> +        active = [cap for cap in self.captures if not cap.complete]

[Severity: Low]
This collects every incomplete capture from the whole run, not only those
of the current case. If a case raised Skip after creating captures but
before calling finish(), the next case's finish() would run the cutoff and
validate() on them.

As far as I can tell, the only such path today is the Skip in
ordered_data():

    except OSError as error:
        if error.errno in (errno.ENOPROTOOPT, errno.EOPNOTSUPP):
            raise Skip("UDP GSO unavailable") from error

udp_lib_setsockopt() cannot return those errnos for UDP_SEGMENT:

    case UDP_SEGMENT:
        if (val < 0 || val > USHRT_MAX)
            return -EINVAL;
        WRITE_ONCE(up->gso_size, val);

So the path can't be reached in the current tree. Would it still be safer
to limit captures to the case that created them?

> +        end = min(self.deadline, time.monotonic() + 2)
> +        quiet = time.monotonic()
> +        while time.monotonic() < end:
> +            self.observe(0.01)
> +            empty = True
> +            for cap in active:
> +                count, drained = cap.drain()
> +                if count or not drained:
> +                    empty = False
> +            quiet = max([quiet] + [cap.last_seen for cap in active])
> +            if empty and time.monotonic() - quiet >= 0.1:
> +                break

[Severity: Medium]
Is 100 ms with no captured frame enough to conclude that the kernel has
finished transmitting?

In this tree, contended master TX, interlink RX and supervision frames are
queued and drained by a BH work item. In net/hsr/hsr_forward_queue.c,
hsr_queue_owned_release() does:

    queue_work(system_bh_wq, &hsr->fwd_work);

hsr_queue_work() requeues itself when its budget runs out.

Suppose that worker is delayed by more than about 100 ms after the last
captured frame, for example under load on PREEMPT_RT. The remaining
frames then go out after the hooks have been moved to lo, and verify_udp()
fails with "UDP payload/ID set differs". ordered_send() also allows only
lab.observe(0.1) before returning.

The packets == seen + drops check only shows that the socket queue was
drained. Could finish() instead wait until every expected payload has
been seen on both lanes, with the deadline as the upper bound?

> +        else:
> +            raise ValueError("capture failed to drain within fixed deadline")

[ ... ]

> +        for cap in active:
> +            require(time.monotonic() < end, "capture cutoff/drain deadline expired")
> +            require(cap.dev != "lo", "capture already uses the cutoff device")
> +            cap.socket.bind(("lo", 0))
> +            require(cap.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR) == errno.ENETDOWN,
> +                    cap.name + ": capture cutoff did not report ENETDOWN")

[Severity: Medium]
Can the da and db captures stop at different points in the stream?

The hooks are removed one capture at a time. Each bind() of a running
socket goes through packet_do_bind()->__unregister_prot_hook(sk, true),
which calls synchronize_net().

In ordered_supervision(), the HSR master and interlink are still up when
finish() runs:

    lab.observe(10)
    lab.finish()

hsr_announce() and hsr_proxy_announce() keep sending supervision frames
during the cutoff. A frame sent after the da hook is removed, but before
the db hook is removed, is captured only on db.

Both captures pass their completeness checks, but verify_supervision()
fails:

    require(observed[0] == observed[1], "supervision LAN copies differ")
    require(all_frames[0] == all_frames[1], "data/supervision LAN order differs")

Would it help to stop the supervision senders before the cutoff, or to
compare only the prefix that both lanes have in common?

[ ... ]

> +def set_gro(lab, ns, dev, wanted, active=None):
> +    result = lab.cmd(ns, "ethtool", "-K", dev, "gro", wanted, check=False)
> +    forced = (active == "off" and wanted == "on" and result.returncode == 1
> +              and result.stdout == "Actual changes:\nrx-gro: off [requested on]\n"
> +              and result.stderr == "Could not change any device features\n")
> +    require(result.returncode == 0 or forced,
> +            "%s: GRO setter rc%d %s" % (dev, result.returncode, result.stderr.strip()))

[Severity: Medium]
Does this depend on the exact output format of ethtool?

A rejected request is accepted only when stdout and stderr match these
strings byte for byte. The code right below already reads back the actual
state with lab.features() and checks it against active.

forced becomes false if an ethtool build reports the change differently,
for example:

- the ioctl fallback, which uses legacy names such as
  generic-receive-offload
- an extra line for a dependent feature

policy() goes through this path on every iteration:

    for wanted in ("on", "off", last):
        set_gro(lab, ns, "m", wanted, active="off")

When it fails here, every remaining gro-mode case is reported as not ok.

Could this rely on the read-back state, and check only the return code
here?

[ ... ]

> +    def producer(index):
> +        try:
> +            # Socket namespace binding was completed before threads existed.
> +            os.sched_setaffinity(0, {cpus[index]})
> +            barrier.wait(timeout=2)

[ ... ]

> +        except Exception as error:
> +            errors.append(error)
> +            stop.set()
> +            barrier.abort()
> +
> +    threads = [threading.Thread(target=producer, args=(index,))
> +               for index in range(2)]
> +    unfinished = []
> +    try:
> +        for thread in threads:
> +            lab.threads.append(thread)
> +            thread.start()
> +        barrier.wait(timeout=2)

[ ... ]

> +    if errors:
> +        raise errors[0]

[Severity: Low]
What happens when a producer fails before reaching the barrier, for
example in os.sched_setaffinity()?

The producer calls barrier.abort(), and the main thread's barrier.wait()
raises threading.BrokenBarrierError. That exception propagates through the
finally block, so the recorded producer error is never raised.

main() then prints "not ok N - two-functional-producers: " with an empty
message, and the real cause is lost.

[ ... ]

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