Re: [PATCH net-next v5 2/4] rtnetlink: add AF_PACKET multicast dumps
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-15 16:47:33
Also in:
linux-kselftest, lkml
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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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 {[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ unsigned long s_ifindex; + struct net_device *dev; + int err; +
[ ... ]
quoted hunk ↗ jump to hunk
+ 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