Re: [PATCH net-next v9 4/6] net: add AF_PACKET multicast dumps

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Concurrent device removal can end an unfiltered AF_PACKET
  RTM_GETMULTICAST dump early without setting NLM_F_DUMP_INTR.
- [Low] The commit message gives a false reason for not extending the FDB
  dump.
- [Low] At this commit, Documentation/netlink/specs/rt-addr.yaml does not
  describe the new AF_PACKET multicast dump: - The ifa-flags enum stops…
- [Low] dev_mc_genid is only bumped through the __hw_addr_count_*
  helpers, i.e. when a dev->mc entry is added or removed.

--

net: add AF_PACKET multicast dumps

This adds an unlocked RTM_GETMULTICAST dump handler for ifa_family
AF_PACKET. For each device it reports every dev->mc entry in the ifaddrmsg
format, with IFA_MULTICAST, IFA_MC_USERS and a new IFA_F_GLOBAL flag. Dump
consistency comes from sampling dev_mc_genid plus dev_base_seq into cb->seq,
so NLM_F_DUMP_INTR gets set when either changes.

> The default FDB dump also walks dev->mc, but only for Ethernet devices
> without an ndo_fdb_dump of their own, so bridge, vxlan or macvlan
> devices never show their multicast filter there, and it has no users
> count or global_use bit.

[Severity: Low]
Is this accurate for bridge and macvlan?

macvlan uses the default helper directly:

drivers/net/macvlan.c:
	.ndo_fdb_dump		= ndo_dflt_fdb_dump,

br_fdb_dump() also falls back to it for unfiltered dumps:

net/bridge/br_fdb.c:br_fdb_dump() {
	...
	if (!filter_dev) {
		err = ndo_dflt_fdb_dump(skb, cb, dev, NULL, idx);
	...
}

rtnl_fdb_dump() calls a device's own ndo_fdb_dump with filter_dev == NULL.
ndo_dflt_fdb_dump() then walks both dev->uc and dev->mc for ARPHRD_ETHER
devices.

So "bridge fdb show" seems to already list dev->mc for bridge masters and
macvlan devices. Only vxlan fits the description.

The other reasons (no users count, no global_use bit, changing the
"bridge fdb show" output) may still hold. Could this paragraph be
corrected?

> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 783c62895249b..948dc25635938 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c

[ ... ]

> @@ -1219,6 +1223,197 @@ void dev_mc_init(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(dev_mc_init);
>  
> +static int dev_mc_fill_addr(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]
At this commit, Documentation/netlink/specs/rt-addr.yaml doesn't yet
describe this dump:

  - the ifa-flags enum ends at stable-privacy, so bit 12 (IFA_F_GLOBAL),
    emitted here in IFA_FLAGS, has no name
  - the getmulticast op doc still says "Get / dump IPv4/IPv6 multicast
    addresses."
  - the reply attributes do not include flags or target-netnsid, and the
    dump request declares no attributes even though target-netnsid is
    accepted

The next patch in the series, "netlink: specs: rt-addr: document AF_PACKET
multicast dumps", fixes all of this. It adds global after stable-privacy,
adds flags and target-netnsid to the reply and target-netnsid to the
request, and rewrites the op doc. Nothing is left over at the end of the
series.

Would it make sense to fold the spec update into this patch, so the spec
and the kernel match at every commit?

> +	    nla_put_u32(skb, IFA_FLAGS, ifa_flags)) {
> +		nlmsg_cancel(skb, nlh);
> +		return -EMSGSIZE;
> +	}
> +
> +	nlmsg_end(skb, nlh);
> +	return 0;
> +}
> +
> +/* Combine dev_mc_genid and dev_base_seq to detect changes, like
> + * inet_base_seq().
> + */
> +static u32 dev_mc_base_seq(const struct net *net)
> +{
> +	u32 res = atomic_read(&net->dev_mc_genid) +
> +		  READ_ONCE(net->dev_base_seq);

[Severity: Low]
This isn't a bug, but dev_mc_genid is only bumped from the __hw_addr_count_*
helpers, i.e. when a dev->mc entry is added or removed. In-place updates to
the two fields exported here don't bump it:

net/core/dev_addr_lists.c:__hw_addr_add_ex() {
	...
			if (global) {
				/* check if addr is already used as global */
				if (ha->global_use)
					return 0;
				else
					ha->global_use = true;
			}
	...
			ha->refcount++;
			return 0;
	...
}

net/core/dev_addr_lists.c:__hw_addr_del_entry() {
	...
	if (global)
		ha->global_use = false;
	...
	if (--ha->refcount)
		return 0;
	...
}

The dump resumes by list position, and these updates never add, remove or
reorder entries. So nothing is skipped or repeated, which matches the IPv4
dev_addr_genid convention.

However, the struct net comment ("bumped on dev->mc changes") and the
commit message ("so a change since the previous device or dump round sets
NLM_F_DUMP_INTR") describe more than that. Could they say membership
changes instead?

[ ... ]

> +	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 = dev_mc_dump_dev(dev, skb, cb, &ctx->addr_idx, flags,
> +				      filter.netnsid);
> +		if (err < 0)
> +			break;
> +	}
> +out:
> +	/* A round that dumps no device, e.g. the one it stopped at is gone,
> +	 * still needs the NLMSG_DONE check to see the change.
> +	 */
> +	cb->seq = dev_mc_base_seq(filter.tgt_net);

[Severity: Medium]
Can a concurrent unregister still end this dump early without
NLM_F_DUMP_INTR?

unlist_netdevice() removes the device from the xarray before it bumps
dev_base_seq:

net/core/dev.c:unlist_netdevice() {
	...
	xa_erase(&net->dev_by_index, dev->ifindex);

	netdev_for_each_altname(dev, name_node)
		netdev_name_node_del(name_node);

	/* Unlink dev from the device chain */
	list_del_rcu(&dev->dev_list);
	netdev_name_node_del(dev->name_node);
	hlist_del_rcu(&dev->index_hlist);

	dev_base_seq_inc(dev_net(dev));
}

The handler is registered with RTNL_FLAG_DUMP_UNLOCKED. So the next dump
round can run while the RTNL holder is preempted between xa_erase() and
dev_base_seq_inc().

Take a round that returns -EMSGSIZE partway through device D. That leaves
ctx->ifindex == D->ifindex, ctx->addr_idx == k, and
cb->prev_seq == cb->seq == G.

D, which is down, is then unregistered or moved to another netns. dev->mc
isn't flushed until later in unregister_netdevice_many_notify(), so
dev_mc_genid doesn't change either.

In the next round, for_each_netdev_dump() no longer finds D. The
dev_mc_dump_dev() calls on later devices and the sample at out: both read
G again. netlink_dump_done()->nl_dump_check_consistent() then sees
seq == prev_seq, and NLMSG_DONE is sent without NLM_F_DUMP_INTR.

Userspace would get entries 0..k-1 of D and no sign that the rest are
missing. The unlocked IPv4 and IPv6 dumps rely on the same dev_base_seq
ordering, but this path is new in this patch.

The commit message says this case is covered ("also when the last round
dumps nothing because the device it stopped at is gone"). Does that fully
hold with this ordering?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930112842.21323-1-sigefriedhyy%40gmail.com