Re: [PATCH net-next v3 4/4] net: remove rcu_barrier() from __dev_change_net_namespace()

[netdev-bot+sashiko]

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: UAF in rt6_remove_exception() after
  __dev_change_net_namespace()

--

> diff --git a/net/core/dev.c b/net/core/dev.c
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -13044,7 +13044,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]
The commit message says the IPv6 exceptions of a nexthop that leaves a
group were already flushed when it left, but I don't think that holds.
When remove_nexthop_from_groups() drops a single IPv6 nexthop from a
multi-member group, nh_rt_cache_flush() is called with replaced_nh ==
nh. It bumps fib6_sernum through rt_genid_bump_ipv6() and then returns
early on !replaced_nh->is_group, so fib6_nh_release_dsts() is never
called. Any rt6_ex created through a route using the group stays in
nh->nh_info->fib6_nh.rt6i_exception_bucket until nexthop_free_rcu()
runs.

That nexthop_free_rcu() is not always queued directly from the
NETDEV_UNREGISTER handler. If nh is also used directly by another IPv6
route, __remove_nexthop_fib() deletes that fib6_info. Its
fib6_purge_rt() only flushes exceptions created from itself, and the
reference on nh is only dropped by nexthop_put() in
fib6_info_destroy_rcu(). nexthop_free_rcu() is therefore queued from
inside an RCU callback. Before this patch, the rcu_barrier() here plus
the one in netdev_run_todo() drained that two-stage chain before the
old netns could go on with its pernet exit handlers.

Now consider dev being moved out of a netns that is about to exit, or
default_device_exit_net() moving it back to init_net from
default_device_exit_batch(). The moved device is not on the old
netns's todo list, so netdev_run_todo() issues a single rcu_barrier(),
which only runs fib6_info_destroy_rcu() and queues nexthop_free_rcu().
netdev_wait_allrefs_any() then sees the loopback refcount already at 1
and returns without another barrier. ops_undo_list() then runs
fib6_net_exit(), freeing net->ipv6.rt6_stats, and ip6_route_net_exit(),
calling dst_entries_destroy() on net->ipv6.ip6_dst_ops, both before
the final rcu_barrier() in cleanup_net().

When nexthop_free_rcu() finally runs, fib6_nh_release() ->
fib6_nh_release_dsts() -> fib6_nh_flush_exceptions() ->
rt6_remove_exception() decrements net->ipv6.rt6_stats->fib_rt_cache in
freed memory, and dst_release() reaches dst_entries_add() on the
destroyed pcpuc_entries counter. Nothing beyond CAP_NET_ADMIN in a
user-owned netns is needed to set this up with RTM_NEWNEXTHOP,
RTM_NEWROUTE and RTM_NEWLINK.

Chains with more stages, such as a one-member group or two members of
the same group on the moved device, look like they can outlast even
the two barriers the old code had. Removing this barrier makes the
simple case reachable as well.

Should the leaving nexthop's exceptions be flushed explicitly in
remove_nexthop_from_groups() or nh_rt_cache_flush(), so that the claim
in the commit message actually holds? Or would you rather keep the
barrier until the netns exit ordering is sorted out?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109-bug-net-netns-move-rcu-barrier-v3-0-6084b02610b3%40linux.dev