Re: [PATCH net-next v2 1/3] rtnetlink: add AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

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