netdev - Re: [PATCH v2 net-next 10/14] ipv6: Factorise ip6_route_multipath

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250411103404.GY395307@horms.kernel.org>
Date: Fri, 11 Apr 2025 11:34:04 +0100
From: Simon Horman <horms@...nel.org>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 10/14] ipv6: Factorise
 ip6_route_multipath_add().

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@...zon.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;
>  }

...