Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 2
- [Medium] AF_PACKET RTM_GETMULTICAST dump
(rtnl_dump_mcaddr()/rtnl_dump_mcaddr_dev(),…
- [Low] Commit "netlink: specs: rt-addr: fix the type of target-netnsid"
(first patch of this series) changes Documentation/netlink/specs/rt-…
- [Low] Reported concern (validated as resolved within this series): the
rt-addr YAML spec did not describe the new AF_PACKET RTM_GETMULTICAST…
--
rtnetlink: add AF_PACKET multicast dumps
This adds handling for RTM_GETMULTICAST dumps with ifa_family set to
AF_PACKET, reporting every entry of dev->mc in the existing ifaddrmsg
format. A new IFA_F_GLOBAL uAPI flag reports netdev_hw_addr::global_use,
and the dump runs under RCU plus netif_addr_lock_bh() without RTNL.
> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index 7fb630b7fe311..0a1ad9ebb47be 100644
> --- a/include/uapi/linux/if_addr.h
> +++ b/include/uapi/linux/if_addr.h
> @@ -57,6 +57,7 @@ enum {
> #define IFA_F_NOPREFIXROUTE 0x200
> #define IFA_F_MCAUTOJOIN 0x400
> #define IFA_F_STABLE_PRIVACY 0x800
> +#define IFA_F_GLOBAL 0x1000
[Severity: Low]
This isn't a bug, but at this commit Documentation/netlink/specs/rt-addr.yaml
has no "global" entry in the ifa-flags enum for bit 12, and the getmulticast
op still lists an empty dump-request attribute list plus a reply set without
"flags" or "target-netnsid". Spec-driven decoders such as ynl therefore
cannot name the new flag or validate the new AF_PACKET reply.
The next patch in the series ("netlink: specs: rt-addr: document AF_PACKET
multicast dumps") adds all of that, so this looks like nothing more than a
transient state inside the series. Would it be worth folding the two
patches, or does the ordering matter for something else?
>
> struct ifa_cacheinfo {
> __u32 ifa_prefered;
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index be9d1625bac31..5f3e692a828b8 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4571,6 +4571,178 @@ static int rtnl_dump_all(struct sk_buff *skb, struct
> netlink_callback *cb)
> return skb->len ? : ret;
> }
>
> +static int rtnl_fill_mcaddr(struct sk_buff *skb, const struct net_device
> *dev,
> + const struct netdev_hw_addr *ha, u32 portid,
> + u32 seq, unsigned int flags, int netnsid)
> +{
[ ... ]
> + nlmsg_end(skb, nlh);
> + return 0;
> +}
> +
> +static int rtnl_dump_mcaddr_dev(struct net_device *dev, struct sk_buff *skb,
> + struct netlink_callback *cb, int *s_addr_idx,
> + unsigned int flags, int netnsid)
> +{
> + struct netdev_hw_addr *ha;
> + int addr_idx = 0;
> + int err = 0;
> +
> + netif_addr_lock_bh(dev);
> + netdev_for_each_mc_addr(ha, dev) {
> + if (addr_idx < *s_addr_idx) {
> + addr_idx++;
> + continue;
> + }
> + err = rtnl_fill_mcaddr(skb, dev, ha, NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq, flags, netnsid);
> + if (err < 0)
> + break;
> + addr_idx++;
> + }
> + netif_addr_unlock_bh(dev);
> +
> + *s_addr_idx = err < 0 ? addr_idx : 0;
> +
> + return err;
> +}
[Severity: Medium]
The only resume state for a multi-part dump is a bare positional index into
dev->mc, and both netif_addr_lock_bh(dev) here and the rcu_read_lock() in
rtnl_dump_mcaddr() are dropped between dump rounds.
Can this silently skip or repeat entries of dev->mc?
Round N fills the skb until rtnl_fill_mcaddr() returns -EMSGSIZE and stores
the position:
*s_addr_idx = err < 0 ? addr_idx : 0;
Round N+1 then skips exactly *s_addr_idx entries from the head of the list
again:
netdev_for_each_mc_addr(ha, dev) {
if (addr_idx < *s_addr_idx) {
addr_idx++;
continue;
}
If a __dev_mc_del() or __dev_mc_add() runs in between (an IGMP/MLD join or
leave, SIOCADDMULTI, "bridge fdb add ... self", or a lower device sync), the
list shifts under the cursor. A removal before the cursor means the entry
that moved into that slot is never emitted; an insertion means an
already-emitted entry is emitted a second time.
> +
> +struct rtnl_mcaddr_dump_filter {
[ ... ]
> +static int rtnl_dump_mcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> + struct rtnl_mcaddr_dump_filter filter = {
> + .tgt_net = sock_net(skb->sk),
> + .netnsid = -1,
> + };
> + unsigned int flags = NLM_F_MULTI;
> + struct {
> + unsigned long ifindex;
> + int addr_idx;
> + } *ctx = (void *)cb->ctx;
[Severity: Medium]
Related to the cursor question above: cb->seq is never set to a generation
counter for dev->mc, and rtnl_fill_mcaddr() does not call
nl_dump_check_consistent(), so NLM_F_DUMP_INTR can never appear on an
emitted message or on NLMSG_DONE. Does that leave user space unable to tell
a torn listing from a consistent one?
The IPv4 RTM_GETMULTICAST dump that this mirrors does both, in
net/ipv4/devinet.c:
cb->seq = inet_base_seq(tgt_net);
...
nl_dump_check_consistent(cb, nlmsg_hdr(skb));
and net/ipv6/addrconf.c does the same with inet6_base_seq(). Is there
something about dev->mc that makes the stamping unnecessary here, or should
this dump grow an equivalent counter?
> + unsigned long s_ifindex;
> + struct net_device *dev;
> + int err;
> +
[ ... ]
> + s_ifindex = ctx->ifindex;
> + for_each_netdev_dump(filter.tgt_net, dev, ctx->ifindex) {
> + /* The device the dump stopped at is gone, do not skip
> + * entries of the next one.
> + */
> + if (dev->ifindex != s_ifindex)
> + ctx->addr_idx = 0;
> + err = rtnl_dump_mcaddr_dev(dev, skb, cb, &ctx->addr_idx, flags,
> + filter.netnsid);
> + if (err < 0)
> + break;
> + }
> +out:
> + rcu_read_unlock();
> + if (filter.netnsid >= 0)
> + put_net_track(filter.tgt_net, &filter.ns_tracker);
> + return err;
> +}
One more note, on the first patch of the series rather than on this one:
[Severity: Low]
"netlink: specs: rt-addr: fix the type of target-netnsid" changes
addr-attrs.target-netnsid in Documentation/netlink/specs/rt-addr.yaml from
type: binary
to s32, matching what the kernel actually parses in ifa_ipv4_policy
(net/ipv4/devinet.c) and ifa_ipv6_policy (net/ipv6/addrconf.c):
[IFA_TARGET_NETNSID] = { .type = NLA_S32 },
That is a fix to a machine-readable ABI description consumed by ynl and
generated bindings, but the patch carries no Fixes: tag. git blame points
the "type: binary" line at dfb0f7d9d979 ("doc/netlink: Add spec for rt addr
messages"), and a prior spec fix, 0c8e30252d9fe ("netlink: specs: rt_addr:
pull the ifa- prefix out of the names"), tagged the same commit. Should the
patch pick up:
Fixes: dfb0f7d9d979 ("doc/netlink: Add spec for rt addr messages")
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914014539.5015-1-sigefriedhyy%40gmail.com
