Re: [PATCH net-next v10 3/6] net: add a generation counter for dev->mc changes

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] netlink: AF_PACKET multicast dump misses changes due to
  reading sequence after loop
- [Low] __hw_addr_changed() (net/core/dev_addr_lists.c) does
  atomic_inc(&dev_net(list->mc_dev)->dev_mc_genid).

--

net: add a generation counter for dev->mc changes

This commit adds a per netns generation counter, dev_mc_genid, that is
bumped whenever an entry is added to or removed from any dev->mc list.
To find the netns, netdev_hw_addr_list gets an mc_dev pointer, which is
set only for dev->mc, and the count helpers bump
dev_net(mc_dev)->dev_mc_genid. The counter will be used by a following
patch for the AF_PACKET multicast dump.

> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index d615192d1c3b1..3ab053cbf8b99 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -16,9 +16,19 @@
>  
>  #include "dev.h"
>  
> +/* RTM_GETMULTICAST dumps use the netns generation counter to detect
> + * changes of dev->mc between dump rounds.
> + */
> +static void __hw_addr_changed(struct netdev_hw_addr_list *list)
> +{
> +	if (list->mc_dev)
> +		atomic_inc(&dev_net(list->mc_dev)->dev_mc_genid);

[Severity: Medium]
This counter only catches changes if the dump samples it before it
walks the lists, but the user in the following patch seems to sample it
afterwards. dev_mc_dump() assigns cb->seq from
dev_mc_base_seq(filter.tgt_net) only after the for_each_netdev_dump()
loop has finished.

The reader holds netif_addr_lock_bh() while it walks each dev->mc, so
the ordering of the atomic_inc() against the list update is not the
problem. The problem is the window between releasing the address lock
of the last device dumped in a round and taking the sample. For
example:

  - a round fills the skb partway through eth0's dev->mc and stops,
    recording the resume position
  - eth0's addr_list_lock is released
  - another CPU deletes an eth0 entry that sits before the resume
    position and bumps dev_mc_genid
  - dev_mc_dump() then stores the already bumped value in cb->seq
  - the next round resumes eth0 at the stored position and skips one
    entry, but the genid it sees matches the value stamped at the end
    of the previous round, so nl_dump_check_consistent() has nothing
    to flag and NLM_F_DUMP_INTR is never set

An add in the same window gives a duplicated entry instead. Either way
user space receives an inconsistent dump with no hint that it should
retry, which is what this counter is meant to prevent.

The commit message names inet_base_seq() as the model. In
inet_dump_ifaddr() it is sampled under rcu_read_lock() before
for_each_netdev_dump() is entered, so any change made during or after
the traversal shows up as a different value in the next round. Should
dev_mc_dump() take its sample before the loop in the same way?

[Severity: Low]
Is dev_net() safe to use here? __hw_addr_changed() runs with only the
device's addr_list_lock held. __dev_change_net_namespace() switches
dev->nd_net under RTNL and netdev_lock(dev), and it never takes
addr_list_lock:

net/core/dev.c:__dev_change_net_namespace() {
    ...
	netdev_lock(dev);
	dev_net_set(dev, net);
	netdev_unlock(dev);
    ...
}

So a dev->mc writer that holds neither RTNL nor the instance lock can
run at the same time as a netns move. The rxe driver looks like one such
caller:

rxe_attach_mcast()
  rxe_get_mcg()
    rxe_mcast_add()
      dev_mc_add(ndev)
        __dev_mc_add()
          __hw_addr_add_ex()
            __hw_addr_count_inc()
              __hw_addr_changed()
                dev_net(ndev)

rxe_mcast_add() only holds the reference it gets from
ib_device_get_netdev(). On NETDEV_UNREGISTER, rxe_notify() only queues
the ib device unregistration, and rxe_net_del() does not clear the port
netdev. That means this path can still run while something like
"ip link set <ndev> netns X" executes.

dev_net() is read_pnet(), which does a plain load through
rcu_dereference_protected(pnet->net, true). That load races with the
rcu_assign_pointer() in dev_net_set().

Would KCSAN report this as a data race? And could the increment then
land on the old netns's dev_mc_genid instead of the new one?

The old struct net can't be freed under the BH-disabled spinlock, and
dev_base_seq covers the device appearing in the new netns. So the impact
looks limited to the race report and one bump going to the wrong netns.

Would dev_net_rcu() be a better fit for this reader?

> +}
> +
>  static void __hw_addr_count_add(struct netdev_hw_addr_list *list, int value)
>  {
>  	list->_count += value;
> +	__hw_addr_changed(list);
>  }

[ ... ]

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