Re: [PATCH net-next v10 4/6] net: 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: 2 · Low: 1

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] dev_mc_dump() (net/core/dev_addr_lists.c) calls
  nl_dump_check_consistent(cb, nlmsg_hdr(skb)) once at the end of every…
- [Medium] dev_mc_dump() reads the generation (cb->seq =
  dev_mc_base_seq()) only after the walk, once dev_mc_dump_dev() has…
- [Low] At this commit, Documentation/netlink/specs/rt-addr.yaml is not
  updated for the new AF_PACKET RTM_GETMULTICAST dump.

--

net: add AF_PACKET multicast dumps

This adds RTM_GETMULTICAST dump support for ifa_family AF_PACKET. Each
dev->mc entry is reported in ifaddrmsg format with its link-layer
address, refcount and global_use flag. The dump runs under RCU and the
device addr_list_lock without RTNL, and uses a dev->mc generation counter
to set NLM_F_DUMP_INTR.

> 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 this commit does not update
Documentation/netlink/specs/rt-addr.yaml for the new flag or the
AF_PACKET dump:

  - the ifa-flags enum has no entry for IFA_F_GLOBAL
  - the getmulticast doc still says "Get / dump IPv4/IPv6 multicast
    addresses."
  - the reply attribute list does not include flags or target-netnsid
  - the dump request lists no attributes

The next patch in the series, "netlink: specs: rt-addr: document
AF_PACKET multicast dumps", appears to fix all of these. The final patch,
"selftests: net: test AF_PACKET multicast dumps", adds a selftest. So the
mismatch only affects ynl decoding at this intermediate commit.

>  
>  struct ifa_cacheinfo {
>  	__u32	ifa_prefered;
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 3ab053cbf8b99..7b9abeca431f6 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c

[ ... ]

> +/* 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);

[ ... ]

> +static int dev_mc_dump_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 = dev_mc_fill_addr(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;
> +}

[ ... ]

> +int dev_mc_dump(struct sk_buff *skb, struct netlink_callback *cb)
> +{

[ ... ]

> +	rcu_read_lock();
> +
> +	if (filter.ifindex) {
> +		cb->answer_flags |= NLM_F_DUMP_FILTERED;
> +		flags |= NLM_F_DUMP_FILTERED;
> +		dev = dev_get_by_index_rcu(filter.tgt_net, filter.ifindex);
> +		if (dev)
> +			err = dev_mc_dump_dev(dev, skb, cb, &ctx->addr_idx,
> +					      flags, filter.netnsid);
> +		else
> +			err = -ENODEV;
> +	} else {
> +		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;
> +		}
> +	}
> +	cb->seq = dev_mc_base_seq(filter.tgt_net);

[Severity: Medium]
Is the generation read too late for the first round?

cb->seq is read here, after dev_mc_dump_dev() has already dropped
addr_list_lock with netif_addr_unlock_bh(). On the first round
cb->prev_seq is 0, so the value read here becomes the baseline.

Suppose another CPU changes dev->mc on the device where round 1 stopped,
between that unlock and the atomic_read() in dev_mc_base_seq(). That
change then becomes part of the baseline:

  round 1 stops at index 5 of dev A
  netif_addr_unlock_bh(A)
                                 other CPU deletes entry 2 of A
                                 __hw_addr_changed() bumps dev_mc_genid
  round 1 reads G1, cb->prev_seq = G1
  round 2 resumes at index 5, old entry 5 (now index 4) is skipped
  round 2 reads G1, no NLM_F_DUMP_INTR

An addition in the same window would produce a duplicate entry instead.
The window covers local_bh_enable() in the unlock, which can run
softirqs. Under PREEMPT_RCU the task can also be preempted there.

This ordering matches rtnl_dump_ifinfo(), but that dump resumes by
ifindex, not by list position. inet_dump_addr() in net/ipv4/devinet.c,
which resumes by position, reads the generation before the walk:

	cb->seq = inet_base_seq(tgt_net);

	for_each_netdev_dump(tgt_net, dev, ctx->ifindex) {

Should cb->seq be read before the walk here too?

> +	nl_dump_check_consistent(cb, nlmsg_hdr(skb));

[Severity: Medium]
Can NLM_F_DUMP_INTR be lost when a round emits no messages?

This check runs on every round, even when nothing was added to the skb.
Dumps resume by list position through ctx->addr_idx. Consider this
sequence:

  round N-1 returns -EMSGSIZE at the last dev->mc entry of device D,
  where D is the last device with entries or the one selected by
  ifa_index

  before the next recvmsg(), an earlier entry of D is deleted, for
  example by a socket leaving an IP multicast group, which reaches
  dev_mc_del() and bumps dev_mc_genid

  round N resumes at the saved addr_idx, which is now past the end of
  D's list, so it emits nothing and returns 0

In round N, cb->seq differs from cb->prev_seq, so
nl_dump_check_consistent() sets NLM_F_DUMP_INTR in nlmsg_hdr(skb). With
skb->len == 0 that address is unused tailroom, not a message. The call
then sets cb->prev_seq to cb->seq.

Next, netlink_dump() calls netlink_dump_done():

net/netlink/af_netlink.c:netlink_dump_done() {
    ...
	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(nlk->dump_done_errno),
			       NLM_F_MULTI | cb->answer_flags);
    ...
	nl_dump_check_consistent(cb, nlh);
    ...
}

NLMSG_DONE is written at the same address, and __nlmsg_put() overwrites
nlmsg_flags. The second check then finds seq == prev_seq and sets
nothing.

Wouldn't the entry that moved into the deleted slot be missing from the
dump, with no NLM_F_DUMP_INTR reported? That seems to contradict the
commit message:

  "so an entry added or removed since the previous round sets
  NLM_F_DUMP_INTR"

in_dev_dump_ifaddr() in net/ipv4/devinet.c only calls the check after it
has filled a message:

		err = inet_fill_ifaddr(skb, ifa, fillargs);
		if (err < 0)
			goto done;

		nl_dump_check_consistent(cb, nlmsg_hdr(skb));

Would the same approach work here?

> +
> +	rcu_read_unlock();
> +	if (filter.netnsid >= 0)
> +		put_net_track(filter.tgt_net, &filter.ns_tracker);
> +	return err;
> +}

[ ... ]

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