Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2
- [Medium] rtnl_dump_mcaddr()/rtnl_dump_mcaddr_dev()
(net/core/rtnetlink.c:4598-4623, 4655-4694) resume a multi-part…
- [Low] The commit message claims 'AF_UNSPEC keeps its "all families"
meaning from RTM_GETADDR'.
- [Low] Concern (validated as resolved within this series): after this
patch alone, Documentation/netlink/specs/rt-addr.yaml still described…
Pre-existing issues:
- [Medium] rtnl_fill_mcaddr() emits IFA_MULTICAST with a length read from
dev->addr_len at dump time (net/core/rtnetlink.c:4588), while struct…
--
rtnetlink: add AF_PACKET multicast dumps
This adds a PF_PACKET RTM_GETMULTICAST dump handler that walks dev->mc and
reports each netdev_hw_addr in the existing ifaddrmsg format, so the
link-layer multicast filter no longer has to be read from
/proc/net/dev_mcast. The handler runs unlocked with respect to RTNL, under
RCU and netif_addr_lock_bh().
A few questions below.
> 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: Low]
Is the AF_UNSPEC part of this sentence accurate for RTM_GETMULTICAST?
Grepping the tree, RTM_GETMULTICAST has a dumpit registered only for
PF_INET (net/ipv4/devinet.c), PF_INET6 (net/ipv6/addrconf.c) and, with this
patch, PF_PACKET. There is no PF_UNSPEC handler, so an AF_UNSPEC dump ends
up in the fallback in rtnetlink_rcv_msg():
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;
and returns -EOPNOTSUPP.
Additionally, rtnl_dump_all() explicitly skips PF_PACKET:
if (idx < s_idx || idx == PF_PACKET)
continue;
so even if a PF_UNSPEC RTM_GETMULTICAST handler were added later, these new
link-layer entries would not appear in an all-families dump. Could the
changelog drop or reword that claim?
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 81c5a6104dea..b2febdb6915e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4566,6 +4566,133 @@ 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)
> +{
> + struct ifaddrmsg *ifm;
> + struct nlmsghdr *nlh;
> +
> + nlh = nlmsg_put(skb, portid, seq, RTM_GETMULTICAST, sizeof(*ifm),
> + flags);
> + if (!nlh)
> + return -EMSGSIZE;
> +
> + ifm = nlmsg_data(nlh);
> + ifm->ifa_family = AF_PACKET;
> + ifm->ifa_prefixlen = 0;
> + ifm->ifa_flags = ha->global_use ? IFA_F_PERMANENT : 0;
> + ifm->ifa_scope = RT_SCOPE_LINK;
> + ifm->ifa_index = dev->ifindex;
> +
> + if (nla_put(skb, IFA_MULTICAST, dev->addr_len, ha->addr) ||
[Severity: Medium]
This isn't a bug introduced by this patch, but can this copy expose
uninitialized bytes from ha->addr?
The length comes from dev->addr_len as read at dump time, while the entry
was filled in at insertion time by __hw_addr_create() in
net/core/dev_addr_lists.c:
ha = kmalloc(alloc_size, GFP_ATOMIC);
if (!ha)
return NULL;
memcpy(ha->addr, addr, addr_len);
The allocation is not zeroed and only addr_len bytes of
ha->addr[MAX_ADDR_LEN] are written.
If dev->addr_len grows after the entry was created, the tail is
uninitialized heap. One such path is tun's TUNSETLINK, which does:
tun->dev->type = (int) arg;
tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
without flushing dev->mc, so a 6-byte entry added with SIOCADDMULTI while
the device was ARPHRD_ETHER would later be dumped as 16 bytes.
The same disclosure already exists through the world-readable
/proc/net/dev_mcast, where dev_mc_seq_show() prints with (int)dev->addr_len
over the same buffer, so this is not new here. Would it make sense to
bound the copy (min_t(unsigned int, dev->addr_len, MAX_ADDR_LEN)) and/or
zero the allocation in __hw_addr_create()?
[Severity: Low]
With this patch alone, Documentation/netlink/specs/rt-addr.yaml still
describes operation getmulticast as:
name: getmulticast
doc: Get / dump IPv4/IPv6 multicast addresses.
while this reply carries a link-layer IFA_MULTICAST of dev->addr_len bytes,
no IFA_CACHEINFO, ifa_scope of RT_SCOPE_LINK and IFA_F_PERMANENT with a
different meaning. The following patch in the series, "netlink: specs:
rt-addr: document AF_PACKET multicast dumps", updates that doc, so the
divergence only exists at this point in the series and needs no action.
> + nla_put_u32(skb, IFA_MC_USERS, ha->refcount)) {
> + 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)
> +{
> + 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);
> + if (err < 0)
> + break;
> + addr_idx++;
> + }
> + netif_addr_unlock_bh(dev);
> +
> + *s_addr_idx = err < 0 ? addr_idx : 0;
[Severity: Medium]
Can this positional resume cursor silently skip dev->mc entries?
Resume state is the ordinal position in the list, and both
netif_addr_lock_bh() and rcu_read_lock() are dropped before the callback
returns, so between netlink dump rounds a writer can unlink an entry that
sits before the saved index:
net/core/dev_addr_lists.c:__hw_addr_del_entry() {
...
list_del_rcu(&ha->list);
kfree_rcu(ha, rcu_head);
list->count--;
...
}
Writers reaching that only need netif_addr_lock_bh() (dev_mc_del() from an
IGMP/MLD leave, SIOCDELMULTI, driver or bonding rx_mode churn), and RTNL
holders are free to run as well since the handler is registered with
RTNL_FLAG_DUMP_UNLOCKED. Removing N entries ahead of the saved addr_idx
shifts the survivors down and drops N entries that were never reported;
device add/remove similarly shifts the for_each_netdev_dump() cursor.
Neither cb->seq nor nl_dump_check_consistent() is used here, so
NLM_F_DUMP_INTR can never be set and user space sees a truncated dump that
looks complete.
The sibling handler for the same message type does maintain that:
net/ipv4/devinet.c:inet_dump_addr() {
...
cb->seq = inet_base_seq(tgt_net);
...
}
net/ipv4/devinet.c:in_dev_dump_ifmcaddr() {
...
nl_dump_check_consistent(cb, nlmsg_hdr(skb));
...
}
Would setting cb->seq from a generation counter and calling
nl_dump_check_consistent() in rtnl_dump_mcaddr_dev() be appropriate here
too?
> +
> + return err;
> +}
> +
[ ... ]
> +static int rtnl_dump_mcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> + struct net *net = sock_net(skb->sk);
> + unsigned int flags = NLM_F_MULTI;
> + struct {
> + unsigned long ifindex;
> + int addr_idx;
> + } *ctx = (void *)cb->ctx;
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909013401.14408-1-sigefriedhyy%40gmail.com
