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: <20250414180731.26130-1-kuniyu@amazon.com>
Date: Mon, 14 Apr 2025 11:06:58 -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: Mon, 14 Apr 2025 15:52:26 +0100
> 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.

Thanks for confirming!

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

Makes sense.

But looks like the series was marked Changes Requested, not sure
if it's accidental or intentional, so I'll resend v2 to see others'
opinion.

Thanks!