Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1 (1)
- [Low] The new `getmulticast` doc text (Documentation/netlink/specs/rt-
addr.yaml:173-179) documents only the ifa-family selector.
Pre-existing issues:
- [Medium] Documentation/netlink/specs/rt-addr.yaml describes a `do`
(single-message get) variant of `getmulticast` (lines 182-193), and…
- [Low] The patch adds `target-netnsid` to the shared `getmulticast`
reply attribute set (&mcaddr-attrs, Documentation/netlink/specs/rt-…
--
netlink: specs: rt-addr: document AF_PACKET multicast dumps
This adds the global address flag to the rt-addr spec, extends the
getmulticast reply attribute list with flags and target-netnsid, and
rewrites the operation doc to explain how ifa-family selects IPv4, IPv6
or link-layer output.
> diff --git a/Documentation/netlink/specs/rt-addr.yaml
> b/Documentation/netlink/specs/rt-addr.yaml
> index 17ead22034515..1a17e55d1cca0 100644
> --- a/Documentation/netlink/specs/rt-addr.yaml
> +++ b/Documentation/netlink/specs/rt-addr.yaml
[ ... ]
> @@ -168,7 +170,13 @@ operations:
> attributes: *ifaddr-all
> -
> name: getmulticast
> - doc: Get / dump IPv4/IPv6 multicast addresses.
> + doc: |
> + Get / dump multicast addresses. ifa-family must select the address
> + family: AF_INET or AF_INET6 for the IP multicast groups joined on
> + a device, AF_PACKET for the link-layer multicast addresses in the
> + device filter. Link-layer entries added explicitly, e.g. with
> + SIOCADDMULTI or "bridge fdb add ... self", rather than by a
> + protocol join are reported with the global flag set.
[Severity: Low]
This isn't a bug, but the new doc text describes only the ifa-family
selector, while the AF_PACKET dump path added by the sibling commit
"rtnetlink: add AF_PACKET multicast dumps" enforces two further
request-side rules in rtnl_valid_dump_mcaddr_req():
net/core/rtnetlink.c:rtnl_valid_dump_mcaddr_req() {
if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) {
NL_SET_ERR_MSG(extack,
"Invalid values in multicast dump header");
return -EINVAL;
}
...
filter->ifindex = ifm->ifa_index;
}
Should the doc mention that ifa_prefixlen, ifa_flags and ifa_scope must
be zero for AF_PACKET, which is stricter than the AF_INET/AF_INET6
behaviour of the same operation?
A caller reusing an ifaddrmsg from a getaddr reply, with a non-zero
ifa_scope still in it, gets -EINVAL with nothing in the spec explaining
why.
Should the doc also mention that ifa-index restricts the dump to a single
device, and that the reply then carries NLM_F_DUMP_FILTERED? Right now
that filter is not discoverable from the spec.
> attribute-set: addr-attrs
> fixed-header: ifaddrmsg
> do:
> @@ -181,10 +189,13 @@ operations:
> - multicast
> - mc-users
> - cacheinfo
> + - flags
> + - target-netnsid
[Severity: Medium]
This isn't a problem introduced by this patch, but since the patch
extends this block it may be worth resolving here: the spec advertises a
do (single-message get) variant of getmulticast, yet no family registers
a doit handler for RTM_GETMULTICAST.
net/ipv4/devinet.c:
{.owner = THIS_MODULE, .protocol = PF_INET, .msgtype = RTM_GETMULTICAST,
.dumpit = inet_dump_ifmcaddr, .flags = RTNL_FLAG_DUMP_UNLOCKED},
net/ipv6/addrconf.c:
{.owner = THIS_MODULE, .protocol = PF_INET6, .msgtype =
RTM_GETMULTICAST,
.dumpit = inet6_dump_ifmcaddr,
.flags = RTNL_FLAG_DUMP_UNLOCKED},
net/core/rtnetlink.c (the new AF_PACKET registration):
{.protocol = PF_PACKET, .msgtype = RTM_GETMULTICAST,
.dumpit = rtnl_dump_mcaddr, .flags = RTNL_FLAG_DUMP_UNLOCKED},
All three provide .dumpit only, and there is no PF_UNSPEC entry, so a
non-dump request ends up here:
net/core/rtnetlink.c:rtnetlink_rcv_msg() {
link = rtnl_get_link(family, type);
if (!link || !link->doit) {
family = PF_UNSPEC;
link = rtnl_get_link(PF_UNSPEC, type);
if (!link || !link->doit)
goto out_unlock;
}
}
and returns -EOPNOTSUPP. Would it make sense to drop the do: block
instead of extending its reply list, so ynl-generated clients do not
emit a single-object get that can never succeed?
[Severity: Low]
This is a pre-existing kernel-side gap rather than something this patch
introduces, but target-netnsid is being added to the shared
&mcaddr-attrs anchor used by all three families. The AF_PACKET and
AF_INET6 emitters do put the attribute:
net/core/rtnetlink.c:rtnl_fill_mcaddr() {
if ((netnsid >= 0 &&
nla_put_s32(skb, IFA_TARGET_NETNSID, netnsid)) ||
}
net/ipv6/addrconf.c:inet6_fill_ifmcaddr() {
if (args->netnsid >= 0 &&
nla_put_s32(skb, IFA_TARGET_NETNSID, args->netnsid)) {
}
The AF_INET emitter does not, even though inet_valid_dump_ifaddr_req()
accepts IFA_TARGET_NETNSID and stores it in fillargs.netnsid:
net/ipv4/igmp.c:inet_fill_ifmcaddr() {
if (nla_put_in_addr(skb, IFA_MULTICAST, im->multiaddr) < 0 ||
nla_put_u32(skb, IFA_MC_USERS, READ_ONCE(im->users)) < 0 ||
nla_put(skb, IFA_CACHEINFO, sizeof(ci), &ci) < 0) {
}
So an AF_INET multicast dump against another namespace never reports the
netnsid that its AF_INET6 and AF_PACKET peers report. Would a one-line
follow-up in inet_fill_ifmcaddr() be worth doing so the three families
agree with what the spec now lists?
> dump:
> request:
> value: 58
> - attributes: []
> + attributes:
> + - target-netnsid
> reply:
> value: 58
> attributes: *mcaddr-attrs
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914014539.5015-1-sigefriedhyy%40gmail.com
