Re: [PATCH v2 net-next 10/14] ipv6: Factorise ip6_route_multipath_add().

[Simon Horman <horms@kernel.org>]

On Tue, Apr 08, 2025 at 06:12:18PM -0700, Kuniyuki Iwashima wrote:
> We will get rid of RTNL from RTM_NEWROUTE and SIOCADDRT and rely
> on RCU to guarantee dev and nexthop lifetime.
> 
> Then, the RCU section will start before ip6_route_info_create_nh()
> in ip6_route_multipath_add(), but ip6_route_info_create() is called
> in the same loop and will sleep.
> 
> Let's split the loop into ip6_route_mpath_info_create() and
> ip6_route_mpath_info_create_nh().
> 
> Note that ip6_route_info_append() is now integrated into
> ip6_route_mpath_info_create_nh() because we need to call different
> free functions for nexthops that passed ip6_route_info_create_nh().
> 
> In case of failure, the remaining nexthops that ip6_route_info_create_nh()
> has not been called for will be freed by ip6_route_mpath_info_cleanup().
> 
> OTOH, if a nexthop passes ip6_route_info_create_nh(), it will be linked
> to a local temporary list, which will be spliced back to rt6_nh_list.
> In case of failure, these nexthops will be released by fib6_info_release()
> in ip6_route_multipath_add().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv6/route.c | 205 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 130 insertions(+), 75 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c

...

> +static int ip6_route_mpath_info_create_nh(struct list_head *rt6_nh_list,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct rt6_nh *nh, *nh_next, *nh_tmp;
> +	LIST_HEAD(tmp);
> +	int err;
> +
> +	list_for_each_entry_safe(nh, nh_next, rt6_nh_list, next) {
> +		struct fib6_info *rt = nh->fib6_info;
> +
> +		err = ip6_route_info_create_nh(rt, &nh->r_cfg, extack);
> +		if (err) {
> +			nh->fib6_info = NULL;
> +			goto err;
> +		}
> +
> +		rt->fib6_nh->fib_nh_weight = nh->weight;
> +
> +		list_move_tail(&nh->next, &tmp);
> +
> +		list_for_each_entry(nh_tmp, rt6_nh_list, next) {
> +			/* check if fib6_info already exists */
> +			if (rt6_duplicate_nexthop(nh_tmp->fib6_info, rt)) {
> +				err = -EEXIST;
> +				goto err;
> +			}
> +		}
> +	}
> +out:
> +	list_splice(&tmp, rt6_nh_list);
> +	return err;

Hi Kuniyuki-san,

Perhaps it can't happen in practice, but if the loop above iterates zero
times then err will be used uninitialised. As it's expected that err is 0
here, perhaps it would be simplest to just:

	return 0;

> +err:
> +	ip6_route_mpath_info_cleanup(rt6_nh_list);
> +	goto out;
>  }

...