Re: [PATCH net-next v5 4/4] 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: 1 · Low: 0

- [Medium] In `dump_mcaddr_l2_check()`
  (tools/testing/selftests/net/rtnetlink.py:161-173) the…

--

selftests: net: test AF_PACKET multicast dumps

This adds dump_mcaddr_l2_check() to tools/testing/selftests/net/rtnetlink.py,
dumping the link-layer multicast list of a dummy device and checking the
ifa-index filter, the global flag, mc-users, and a dump of another netns via
target-netnsid. The test skips when the kernel returns EOPNOTSUPP for the
AF_PACKET dump.

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

[ ... ]

> @@ -105,6 +109,70 @@ 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, the global flag and
> +    target-netnsid.
> +    """
> +
> +    with NetNS() as ns, 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")
> +

[ ... ]

> +        # target-netnsid dumps another netns, ifa-index is relative to it
> +        with NetNS() as peer:
> +            ip(f"netns set {peer} 5")
> +            ip("link add name dummy3 type dummy", ns=peer)
> +            ip("link set dummy3 up", ns=peer)
> +            peer_idx = ip("link show dummy3", json=True, ns=peer)[0]['ifindex']
> +
> +            addresses = rtnl.getmulticast(
> +                {"ifa-family": socket.AF_PACKET, "target-netnsid": 5,
> +                 "ifa-index": peer_idx}, dump=True)
> +            ksft_eq({(addr['ifa-index'], addr['target-netnsid'])
> +                     for addr in addresses}, {(peer_idx, 5)},
> +                    "target-netnsid did not dump the peer netns")

[Severity: Medium]
Can this final assertion actually distinguish a dump of the peer netns from
a dump of the socket's own netns?

The test body runs inside a fresh netns, so lo is 1, dummy1 is 2 and dummy2
is 3, making dev_idx equal to 2.  The peer netns is created by NetNS(), which
just does:

tools/testing/selftests/net/lib/py/netns.py:NetNS.__init__() {
    ...
    ip('netns add ' + self.name)
    ...
}

so the peer contains only lo (1) plus dummy3, and peer_idx ends up as 2 as
well - the same ifindex as dummy1 in the enclosing netns.

On the kernel side, rtnl_dump_mcaddr() starts with the socket's namespace:

net/core/rtnetlink.c:rtnl_dump_mcaddr() {
    ...
    struct rtnl_mcaddr_dump_filter filter = {
        .tgt_net = sock_net(skb->sk),
    ...
    dev = dev_get_by_index_rcu(filter.tgt_net, filter.ifindex);
    ...
}

If filter.tgt_net were left at sock_net(skb->sk) instead of the resolved peer
net, the lookup of ifindex 2 returns the local dummy1, and the entries are
emitted with ifa_index 2.  IFA_TARGET_NETNSID is then echoed straight back
from the request value:

net/core/rtnetlink.c:rtnl_fill_mcaddr() {
    ...
    if ((netnsid >= 0 &&
         nla_put_s32(skb, IFA_TARGET_NETNSID, netnsid)) ||
    ...
}

so the reply set is {(2, 5)}, which is exactly what ksft_eq() expects here.
Would a kernel that parsed target-netnsid but iterated the wrong struct net
still pass this check?

The commit message says the test verifies "that IFA_TARGET_NETNSID dumps
another netns" - is that claim accurate given the above?

Would it help to make the peer namespace distinguishable, for example by
adding a unique address with ip maddr add ... dev dummy3 in the peer netns
and asserting that address shows up, and/or by creating extra devices so peer
ifindices cannot collide with the local ones?

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