Re: [PATCH net-next v3 3/4] nexthop: flush cached routes when a nexthop leaves a group

[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] In remove_nexthop_from_groups() (net/ipv4/nexthop.c), the new
  loop calls nh_rt_cache_flush(net, nhge->nh_parent, nh) once for every…

--

nexthop: flush cached routes when a nexthop leaves a group

When a nexthop is removed and other members keep its group alive, dsts
cached through the removed nexthop could stay in use until the nexthop is
freed from an RCU callback. This patch calls nh_rt_cache_flush() for each
affected group in remove_nexthop_from_groups(), after synchronize_net().

> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 42e55b5a755ee..e0089a6450e75 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c

[ ... ]

> @@ -2130,6 +2133,13 @@ remove_nexthop_from_groups(struct net *net, struct nexthop *nh,
>  	/* make sure all see the newly published array before releasing rtnl */
>  	synchronize_net();
>  
> +	/* Routes through the groups can still hold dsts cached through @nh,
> +	 * which are only released when @nh is freed. Invalidate them now that
> +	 * no reader can pick @nh from the old groups any more.
> +	 */
> +	list_for_each_entry(nhge, &deferred_free, nh_list)
> +		nh_rt_cache_flush(net, nhge->nh_parent, nh);

[Severity: High]
This loop runs a netns-wide flush once for every surviving group. Is
that needed, or would one flush after the loop be enough?

remove_nexthop_from_groups() is only reached from the !nh->is_group
branch of __remove_nexthop(), so @nh is never a group here. That means
each nh_rt_cache_flush() call only does the two global invalidations:

net/ipv4/nexthop.c:nh_rt_cache_flush() {
	if (!list_empty(&nh->fi_list))
		rt_cache_flush(net);

	spin_lock_bh(&nh->lock);
	have_f6i = !list_empty(&nh->f6i_list);
	spin_unlock_bh(&nh->lock);
	if (have_f6i)
		rt_genid_bump_ipv6(net);

	if (!replaced_nh->is_group)
		return;
	...
}

rt_cache_flush() is just a genid bump. rt_genid_bump_ipv6(), however,
calls fib6_flush_trees(), and __fib6_clean_all() then walks every IPv6
table in the netns while holding tb6_lock with BH disabled:

net/ipv6/ip6_fib.c:__fib6_clean_all() {
	...
			spin_lock_bh(&table->tb6_lock);
			fib6_clean_tree(net, &table->tb6_root,
					func, sernum, arg, skip_notify);
			spin_unlock_bh(&table->tb6_lock);
	...
}

The comment above __fib6_flush_trees in addrconf_core.c describes this
as "fairly expensive, be careful".

Every route through a group puts its fib6_info on the group's f6i_list
(fib6_add() does list_add(&rt->nh_list, &rt->nh->f6i_list)). So any
group that carries IPv6 routes triggers a full walk. After the first
walk, the walks for the remaining groups have no additional effect.

Before this patch, removing a nexthop from G groups did no fib6 walk.
Now it does G full walks back to back under RTNL, with no
cond_resched().

On link down or unregister, nexthop_flush_dev() calls remove_nexthop()
for every nexthop on the device, which multiplies the cost again. With
FRR using kernel nexthop groups, one peer nexthop is often a member of
many ECMP groups, and a full IPv6 table can hold around 200k routes.

CAP_NET_ADMIN in a user namespace is enough to set this up. Could that
lead to long RTNL hold times, or soft-lockup or RCU stall warnings on
!PREEMPT kernels?

The replace_nexthop() precedent cited in the commit message calls
nh_rt_cache_flush() once per operation. For groups, it uses the targeted
fib6_nh_release_dsts() instead of repeated global walks. The IPv4 side
of this file also batches through need_flush and a single fib_flush().

Would it work to collect the fi_list and f6i_list checks into two
booleans in this loop, and then call rt_cache_flush() and
rt_genid_bump_ipv6() at most once afterwards? Another option is to
release only the removed nexthop's own dsts with fib6_nh_release_dsts(),
which does not need a tree walk.

> +
>  	/* Now safe to free percpu stats — all RCU readers have finished */
>  	list_for_each_entry_safe(nhge, tmp, &deferred_free, nh_list) {
>  		list_del(&nhge->nh_list);

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