Re: [PATCH net-next v2 3/3] net: remove rcu_barrier() from __dev_change_net_namespace()

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] net: stale IPv6 nexthop exception dst and wrong-netns stats
  after netns move

--

> diff --git a/net/core/dev.c b/net/core/dev.c
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12994,7 +12994,6 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
>  	 * the device is just moving and can keep their slaves up.
>  	 */
>  	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
> -	rcu_barrier();
>  
>  	new_nsid = peernet2id_alloc(dev_net(dev), net, GFP_KERNEL);
>  
>

[Severity: High]
Removing the rcu_barrier() here looks like it opens a window for IPv6
nexthop exception routes that the changelog does not cover.

Consider a route in net_old that uses a nexthop group with more than
one member, where one member, nh1, is bound to dev. When dev is moved,
nexthop_flush_dev() calls remove_nexthop() on nh1. Because the group
still has other members, remove_nh_grp_entry() only drops nh1 from the
group. The fib6_info stays, its fn_sernum is not bumped, and nh1's
exception bucket is not flushed synchronously.

That flush happens later, in this chain:

  nexthop_free_rcu()
    -> nexthop_free_single()
    -> fib6_nh_release()
    -> fib6_nh_flush_exceptions()

remove_nexthop() queues it through nexthop_put() and call_rcu_hurry().

Under RTNL, synchronize_net() is synchronize_rcu_expedited(), which
does not wait for that callback. Without the barrier,
dev_net_set(dev, net) can therefore run first. Two things then go
wrong:

- RTF_CACHE exceptions created by ip6_rt_cache_alloc() have
  rt->sernum == 0. ip6_dst_check() then skips rt6_is_valid() and goes
  through rt6_check(), which still passes because rt->from is alive in
  net_old with an unchanged cookie. A socket in net_old can keep
  transmitting through dev after it has moved to the new netns, until
  the callback finally calls dst_dev_put(). The fib6_sernum argument
  in the changelog does not seem to hold for these entries.

- rt6_remove_exception() uses dev_net(rt6_ex->rt6i->dst.dev) to pick
  which rt6_stats->fib_rt_cache to decrement. When it runs after the
  move, it decrements the new netns counter instead of net_old's,
  leaving both counters wrong.

Was this case considered? If the barrier goes, it seems the nexthop
code would need to flush nh1's exceptions synchronously when nh1 is
removed from a group because its device is going away, instead of
relying on the RCU callback. Alternatively, rt6_remove_exception()
could avoid deriving the netns from the dst device. Which direction
would you prefer?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-bug-net-netns-move-rcu-barrier-v2-0-b6eb7780c492%40gmail.com