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: <20250411193347.47836-1-kuniyu@amazon.com>
Date: Fri, 11 Apr 2025 12:33:46 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <horms@...nel.org>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v2 net-next 10/14] ipv6: Factorise ip6_route_multipath_add().

From: Simon Horman <horms@...nel.org>
Date: Fri, 11 Apr 2025 11:34:04 +0100
> > +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,

Yes, it never happens by patch 1 as rtm_to_fib6_multipath_config()
returns an error in such a case.

> 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;

If we want to return 0 above, we need to duplicate list_splice() at
err: and return err; there.  Or initialise err = 0, but this looks
worse to me.

Btw, was this caught by Smatch, Coverity, or something ?  I don't
see such a report at CI.
https://patchwork.kernel.org/project/netdevbpf/patch/20250409011243.26195-11-kuniyu@amazon.com/

If so, I'm just curious if we have an official guideline for
false-positives flagged by such tools, like we should care about it
while writing a code and should try to be safer to make it happy.

We are also running Coverity for the mainline kernel and have tons
of false-positive reports due to lack of contexts.

Thanks!

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