netdev - Re: [PATCH v2 net-next] net: ipv4: Consider unreachable nexthops in multipath routes

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 for Android: free password hash cracker in your pocket

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

Message-ID: <56FE8822.3080900@cumulusnetworks.com>
Date:	Fri, 1 Apr 2016 08:39:30 -0600
From:	David Ahern <dsa@...ulusnetworks.com>
To:	Julian Anastasov <ja@....bg>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next] net: ipv4: Consider unreachable nexthops in
 multipath routes

On 4/1/16 2:09 AM, Julian Anastasov wrote:
>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>> index d97268e8ff10..6d423faff0ce 100644
>> --- a/net/ipv4/fib_semantics.c
>> +++ b/net/ipv4/fib_semantics.c
>> @@ -1559,17 +1559,45 @@ int fib_sync_up(struct net_device *dev, unsigned int nh_flags)
>>   }
>>
>>   #ifdef CONFIG_IP_ROUTE_MULTIPATH
>> +static bool fib_good_nh(const struct fib_nh *nh, struct net_device *dev)
>> +{
>> +	struct neighbour *n = NULL;
>> +	int state = NUD_NONE;
>
> 	Looks like we can do it even better.
> If we use NUD_REACHABLE here...
>
>> +
>> +	if (nh->nh_scope == RT_SCOPE_LINK) {
>> +		rcu_read_lock_bh();
>> +
>> +		n = __neigh_lookup_noref(&arp_tbl, &nh->nh_gw, dev);
>> +		if (n)
>> +			state = n->nud_state;
>> +
>> +		rcu_read_unlock_bh();
>> +	}
>> +
>> +	/* outside of rcu locking using n only as a boolean
>> +	 * on whether a neighbor entry existed
>> +	 */
>> +	if (!n || (state & NUD_VALID))
>
> 	then check for '!n' is not needed. For bool type
> 'return state & NUD_VALID;' should work.

good simplification.

>
>> +		return true;
>> +
>> +	return false;
>> +}
>>
>>   void fib_select_multipath(struct fib_result *res, int hash)
>>   {
>>   	struct fib_info *fi = res->fi;
>> +	struct net_device *dev = fi->fib_dev;
>> +	struct net *net = fi->fib_net;
>>
>>   	for_nexthops(fi) {
>>   		if (hash > atomic_read(&nh->nh_upper_bound))
>>   			continue;
>>
>> -		res->nh_sel = nhsel;
>> -		return;
>> +		if (!net->ipv4.sysctl_fib_multipath_use_neigh ||
>> +		    fib_good_nh(nh, dev)) {
>
> 	This dev is from first nexthop. Better fib_good_nh
> to use nh->nh_dev instead, it is present for all nexthops
> in multipath route. We should not copy the bugs from
> fib_detect_death.

ack. Will fix.

>
>> +			res->nh_sel = nhsel;
>> +			return;
>> +		}
>>   	} endfor_nexthops(fi);
>
> 	So, you dropped the idea to give full chance for
> fallback? Now if last nexthop fails we do not fallback at all.
> We promised to prefer reachable nexthops.

I'll save the first nhsel not selected and if all fails return it. That 
keeps the fallback inline with what happens today.