Re: [PATCH net-next 3/3] selftests: net: test AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] dump_mcaddr_l2_check() in
  tools/testing/selftests/net/rtnetlink.py dereferences dictionary keys…

--

selftests: net: test AF_PACKET multicast dumps

This adds dump_mcaddr_l2_check() to the rtnetlink.py selftest, dumping the
link-layer multicast addresses of a dummy device over AF_PACKET and
checking the ifa-index filter, the all-hosts entry flags and the
SIOCADDMULTI entry flags plus mc-users.  The case skips when the kernel
answers the dump with EOPNOTSUPP.

> diff --git a/tools/testing/selftests/net/rtnetlink.py b/tools/testing/selftests/net/rtnetlink.py
> index 5cc3ebdcf08de..94340dac0e217 100755
> --- a/tools/testing/selftests/net/rtnetlink.py
> +++ b/tools/testing/selftests/net/rtnetlink.py

[ ... ]

> @@ -105,6 +109,53 @@ def dump_mcaddr6_check() -> None:
>                  s2.close()
>  
>  
> +def dump_mcaddr_l2_check() -> None:
> +    """
> +    Verify link-layer multicast addresses in an AF_PACKET RTM_GETMULTICAST
> +    dump: the ifa-index filter, mc-users and the permanent flag.
> +    """
> +
> +    with NetNS() as ns:
> +        with NetNSEnter(str(ns)):
> +            for ifname in ("dummy1", "dummy2"):
> +                ip(f"link add name {ifname} type dummy")
> +                ip(f"link set {ifname} up")
> +            dev_idx = socket.if_nametoindex("dummy1")
> +            ip(f"maddr add {ETH_TEST_MULTICAST_STR} dev dummy1")
> +
> +            rtnl = RtnlAddrFamily()
> +            try:
> +                addresses = rtnl.getmulticast(
> +                    {"ifa-family": socket.AF_PACKET, "ifa-index": dev_idx},
> +                    dump=True)
> +            except NlError as e:
> +                if e.error == errno.EOPNOTSUPP:
> +                    raise KsftSkipEx(
> +                        "kernel does not support AF_PACKET multicast dump")
> +                raise
> +
> +            # dummy2 has entries as well, only dummy1 may be listed
> +            ksft_eq({addr['ifa-index'] for addr in addresses}, {dev_idx},
> +                    "AF_PACKET multicast dump ignored ifa-index filter")
> +
> +            entries = {addr['multicast']: addr for addr in addresses}
> +
> +            # Bringing an Ethernet device up joins 224.0.0.1, which maps
> +            # to 01:00:5e:00:00:01 in the device multicast list.
> +            ksft_in(ETH_ALL_HOSTS_MULTICAST, entries,
> +                    "dummy1 does not have the all-hosts link-layer address")
> +            ksft_not_in('permanent',
> +                        entries[ETH_ALL_HOSTS_MULTICAST]['ifa-flags'],
> +                        "protocol entry is permanent")
> +
> +            ksft_in(ETH_TEST_MULTICAST, entries,
> +                    "dummy1 does not have the SIOCADDMULTI address")
> +            ksft_eq(entries[ETH_TEST_MULTICAST]['mc-users'], 1,
> +                    "unexpected mc-users for the SIOCADDMULTI address")
> +            ksft_in('permanent', entries[ETH_TEST_MULTICAST]['ifa-flags'],
> +                    "SIOCADDMULTI entry is not permanent")

[Severity: Low]
Can the entries[] lookups here raise KeyError in exactly the case the
preceding checks are meant to report?

ksft_in() in tools/testing/selftests/net/lib/py/ksft.py only records the
failure and returns:

    def ksft_in(a, b, comment=""):
        if a not in b:
            _fail("Check failed", a, "not in", b, comment)

So when the dump does not contain 01:00:5e:00:00:01, execution continues
into:

    entries[ETH_ALL_HOSTS_MULTICAST]['ifa-flags']

and the same applies to entries[ETH_TEST_MULTICAST] for the mc-users and
permanent checks if the SIOCADDMULTI address is missing.

ksft_run() catches this with "except BaseException" and prints a
traceback, so the case is reported as a failure, but the remaining
independent checks (mc-users == 1 and the permanent flag on the
SIOCADDMULTI entry) are never executed.  Note also that the earlier
ksft_eq() on the ifa-index set does not stop the case either, so
addresses may legitimately be empty at this point.

Would it be worth following the convention already used elsewhere in this
file, where _users_for() and dump_mcaddr_check() test membership before
dereferencing:

    if 'mc-users' not in matches[0]:
        raise KsftSkipEx(...)

and ipv6_route_del_reason_expired() guards with ksft_not_none(attrs)
before touching attrs?

[ ... ]

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