Re: [PATCH net] ipv4: stop PMTU walk when nexthop group shrinks

[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] The fix is incomplete.

--

ipv4: stop PMTU walk when nexthop group shrinks

The PMTU walk in __ip_rt_update_pmtu() reads the path count with
fib_info_num_path() and then each path with fib_info_nhc(). For nexthop
objects, these two calls load nh->nh_grp separately, so a group
replacement in between can make fib_info_nhc() return NULL. This patch
adds a NULL check and stops the walk at that point.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 37674d76f90f0..5f2197874bebc 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1082,6 +1082,8 @@ static void __ip_rt_update_pmtu(struct rtable *rt, struct flowi4 *fl4, u32 mtu)
>  
>  			for (nhsel = 0; nhsel < fib_info_num_path(res.fi); nhsel++) {
>  				nhc = fib_info_nhc(res.fi, nhsel);
> +				if (!nhc)
> +					break;

[Severity: High]
Is this fix complete? fib_dump_info_fnhe(), in the same file, has the
same fib_info_num_path()/fib_info_nhc() loop and does not check nhc for
NULL:

net/ipv4/route.c:fib_dump_info_fnhe() {
    ...
	for (nhsel = 0; nhsel < fib_info_num_path(fi); nhsel++) {
		struct fib_nh_common *nhc = fib_info_nhc(fi, nhsel);
		struct fnhe_hash_bucket *bucket;
		int err;

		if (nhc->nhc_flags & RTNH_F_DEAD)
			continue;
    ...
}

For a route that uses a nexthop object, nexthop_num_path() can see a
two-member group and accept nhsel = 1. A writer can then publish a
smaller group on the same struct nexthop, in either of two ways:

replace_nexthop_grp()
    rcu_assign_pointer(old->nh_grp, newg);

remove_nh_grp_entry()
    rcu_assign_pointer(nhp->nh_grp, newg);

After that, nexthop_fib_nhc() loads the new group, and
nexthop_mpath_select() returns NULL because nhsel >= num_nh. That makes
the nhc->nhc_flags read a NULL pointer dereference.

The reader path holds only RCU:

inet_dump_fib()
  fib_table_dump()
    fn_trie_dump_leaf()
      fib_dump_info_fnhe()

inet_dump_fib() is registered with RTNL_FLAG_DUMP_UNLOCKED and takes only
rcu_read_lock(), so nothing stops the RTNL-held nexthop writers from
running at the same time.

The dump_exceptions filter is set by default for non-strict dumps. For
strict dumps it stays set when RTM_F_CLONED is requested, for example by
ip route show cache. The dump itself needs no privilege. The writer
needs CAP_NET_ADMIN in the netns, which the commit message notes is
reachable through an unprivileged user namespace.

If only this patch goes to stable, the same NULL dereference is still
reachable through the route dump with the same writer. Should the same
guard also go into fib_dump_info_fnhe()? Alternatively, could each walk
take a single snapshot of nh_grp, so the count and the per-index lookup
always use the same group?

>  				update_or_create_fnhe(nhc, fl4->daddr, 0, mtu, lock,
>  						      jiffies + net->ipv4.ip_rt_mtu_expires);
>  			}

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