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: <20250414145226.GS395307@horms.kernel.org>
Date: Mon, 14 Apr 2025 15:52:26 +0100
From: Simon Horman <horms@...nel.org>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
	kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org,
	pabeni@...hat.com
Subject: Re: [PATCH v2 net-next 10/14] ipv6: Factorise
 ip6_route_multipath_add().

On Fri, Apr 11, 2025 at 12:33:46PM -0700, Kuniyuki Iwashima wrote:
> 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.

Thanks. I should have dug a bit deeper to determine that this
is a false-positive.

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

Sorry for not mentioning that it was flagged by Smatch,
I certainly should have done so.


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

I think that the current non-guideline is that we don't change
code just to keep the tools happy. Perhaps we should add something
about that to the process document?