Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 2
- [Medium] rtnl_dump_mcaddr()/rtnl_dump_mcaddr_dev()
(net/core/rtnetlink.c:4603-4728) resume a multi-part…
- [Medium] The commit message claims "AF_UNSPEC keeps its 'all families'
meaning from RTM_GETADDR".
- [Low] Incomplete submission claim: the new IFA_F_GLOBAL uAPI flag is
added only to include/uapi/linux/if_addr.h while the in-tree mirror…
- [Low] Claim that Documentation/netlink/specs/rt-addr.yaml is not
updated for the new AF_PACKET producer: the ifa-flags enum lacks a…
--
rtnetlink: add AF_PACKET multicast dumps
This adds an RTM_GETMULTICAST dump handler for ifa_family AF_PACKET which
walks dev->mc and reports each entry as an ifaddrmsg with IFA_MULTICAST,
IFA_MC_USERS and the new IFA_F_GLOBAL flag in IFA_FLAGS. The handler is
registered under PF_PACKET with RTNL_FLAG_DUMP_UNLOCKED and runs under RCU
plus netif_addr_lock_bh().
A note on the commit message first:
> This covers every column of /proc/net/dev_mcast. AF_PACKET is the
> family iproute2 already uses for link-layer addresses ("ip -0"), and
> AF_UNSPEC keeps its "all families" meaning from RTM_GETADDR.
[Severity: Medium]
Is the AF_UNSPEC part of this sentence accurate for RTM_GETMULTICAST?
RTM_GETMULTICAST only has PF_INET (net/ipv4/devinet.c), PF_INET6
(net/ipv6/addrconf.c) and the new PF_PACKET registration; rtnl_dump_all is
wired to RTM_GETADDR, RTM_GETROUTE and RTM_GETNETCONF only. In
rtnetlink_rcv_msg() the dump lookup falls back to PF_UNSPEC and then errors
out:
link = rtnl_get_link(family, type);
if (!link || !link->dumpit) {
family = PF_UNSPEC;
link = rtnl_get_link(family, type);
if (!link || !link->dumpit)
goto err_unlock;
}
so wouldn't an ifa_family == AF_UNSPEC RTM_GETMULTICAST dump return
-EOPNOTSUPP rather than dumping all families?
And even with a PF_UNSPEC handler wired to rtnl_dump_all(), that loop skips
PF_PACKET:
if (idx < s_idx || idx == PF_PACKET)
continue;
which would keep the new link-layer entries out of a combined dump. Could
this sentence be reworded to say userspace has to issue an explicit
AF_PACKET request?
> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index 7fb630b7fe31..0a1ad9ebb47b 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 the in-tree mirror tools/include/uapi/linux/if_addr.h
is listed in tools/perf/check-headers.sh and does not get the new define, so
a perf build will report the two copies as differing. The mirror already
lags at the baseline commit (it is missing IFA_MC_USERS), so this changes
nothing observable, and tools/ resyncs are usually separate commits. Was a
resync intended here?
The selftest side of the same concern is already handled later in the
series: "selftests: net: test AF_PACKET multicast dumps" adds
dump_mcaddr_l2_check() to tools/testing/selftests/net/rtnetlink.py covering
the ifa-index filter, mc-users, the global flag and target-netnsid.
>
> struct ifa_cacheinfo {
> __u32 ifa_prefered;
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 81c5a6104dea..f54e9cb3bf30 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4566,6 +4566,169 @@ 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)
> +{
> + u32 ifa_flags = ha->global_use ? IFA_F_GLOBAL : 0;
[ ... ]
> + if ((netnsid >= 0 &&
> + nla_put_s32(skb, IFA_TARGET_NETNSID, netnsid)) ||
> + nla_put(skb, IFA_MULTICAST, dev->addr_len, ha->addr) ||
> + nla_put_u32(skb, IFA_MC_USERS, ha->refcount) ||
> + nla_put_u32(skb, IFA_FLAGS, ifa_flags)) {
[Severity: Low]
This isn't a bug in the code, but at this commit
Documentation/netlink/specs/rt-addr.yaml has no "global" entry in the
ifa-flags enum for bit 0x1000, and the getmulticast operation still lists a
reply of {multicast, mc-users, cacheinfo} with an empty dump request
attribute list, while this function emits IFA_FLAGS and IFA_TARGET_NETNSID
and rtnl_valid_dump_mcaddr_req() accepts IFA_TARGET_NETNSID.
The following commit in the series, "netlink: specs: rt-addr: document
AF_PACKET multicast dumps", adds "- name: global" as the 13th ifa-flags
entry, adds flags and target-netnsid to the reply attributes and
target-netnsid to the dump request, and updates the op doc, so this only
matters for bisectability of the generated bindings.
> + nlmsg_cancel(skb, nlh);
> + return -EMSGSIZE;
> + }
> +
> + 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]
Can this skip or duplicate dev->mc entries across a multi-part dump without
telling userspace?
rtnl_dump_mcaddr_dev() resumes with a bare positional index and drops the
lock protecting dev->mc when it returns:
netif_addr_lock_bh(dev);
netdev_for_each_mc_addr(ha, dev) {
if (addr_idx < *s_addr_idx) {
...
netif_addr_unlock_bh(dev);
*s_addr_idx = err < 0 ? addr_idx : 0;
Between two netlink_dump() invocations dev->mc can be mutated by
__dev_mc_add()/__dev_mc_del()/dev_mc_flush() in net/core/dev_addr_lists.c,
which only take netif_addr_lock_bh(dev):
netif_addr_lock_bh(dev);
err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len, ...
and the handler is registered with RTNL_FLAG_DUMP_UNLOCKED, so RTNL does
not serialize this either. If an entry before *s_addr_idx is removed the
next entry is silently skipped; if one is inserted an entry is emitted
twice.
The sibling RTM_GETMULTICAST producers mark that case. in_dev_dump_ifmcaddr()
calls, per message:
nl_dump_check_consistent(cb, nlmsg_hdr(skb));
with cb->seq initialised from inet_base_seq(tgt_net) by the caller, so
userspace gets NLM_F_DUMP_INTR. rtnl_dump_ifinfo() and inet6_dump_addr() do
the same.
> +
[ ... ]
> +static int rtnl_dump_mcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{
[ ... ]
> + err = rtnl_valid_dump_mcaddr_req(cb->nlh, skb->sk, &filter,
> + cb->extack);
> + if (err < 0)
> + return err;
> +
> + rcu_read_lock();
> +
> + if (filter.ifindex) {
[Severity: Medium]
This is the spot where the sibling dumps set cb->seq from a generation
counter before iterating, for example inet6_dump_addr() and
inet_dump_addr(). Should rtnl_dump_mcaddr() do the same so that
nl_dump_check_consistent() in rtnl_dump_mcaddr_dev() can raise
NLM_F_DUMP_INTR?
struct netdev_hw_addr_list has no generation field today:
struct netdev_hw_addr_list {
struct list_head list;
int count;
...
};
so would this need a new counter bumped by __hw_addr_add_ex()/
__hw_addr_del_entry(), or would reusing something like dev_base_seq or a
dev_addr_genid style value be enough here?
> + cb->answer_flags |= NLM_F_DUMP_FILTERED;
> + flags |= NLM_F_DUMP_FILTERED;
> + dev = dev_get_by_index_rcu(filter.tgt_net, filter.ifindex);
[ ... ]
> + for_each_netdev_dump(filter.tgt_net, dev, ctx->ifindex) {
> + 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(filter.tgt_net);
> + return err;
> +}
> +
[ ... ]
> @@ -7251,6 +7414,8 @@ static const struct rtnl_msg_handler
> rtnetlink_rtnl_msg_handlers[] __initconst =
> {.msgtype = RTM_SETSTATS, .doit = rtnl_stats_set},
> {.msgtype = RTM_NEWLINKPROP, .doit = rtnl_newlinkprop},
> {.msgtype = RTM_DELLINKPROP, .doit = rtnl_dellinkprop},
> + {.protocol = PF_PACKET, .msgtype = RTM_GETMULTICAST,
> + .dumpit = rtnl_dump_mcaddr, .flags = RTNL_FLAG_DUMP_UNLOCKED},
[Severity: Medium]
This registration is PF_PACKET only, which is the basis of the commit
message question above about AF_UNSPEC keeping its "all families" meaning
for RTM_GETMULTICAST.
> {.protocol = PF_BRIDGE, .msgtype = RTM_GETLINK,
> .dumpit = rtnl_bridge_getlink},
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909133247.113923-1-sigefriedhyy%40gmail.com
