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
