netdev - Re: [PATCH RFC net-next 5/7] net/ipv6: Add support for path selection using hash of 5-tuple

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] [day] [month] [year] [list]

Date:   Wed, 21 Feb 2018 10:54:33 -0700
From:   David Ahern <dsahern@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        nikolay@...ulusnetworks.com, idosch@...lanox.com
Subject: Re: [PATCH RFC net-next 5/7] net/ipv6: Add support for path selection
 using hash of 5-tuple

On 2/21/18 9:22 AM, Ido Schimmel wrote:

>> +u32 rt6_multipath_hash(const struct net *net, const struct flowi6 *fl6,
>> +		       const struct sk_buff *skb)
>>  {
>>  	struct flow_keys hash_keys;
>>  	u32 mhash;
>>  
>> -	memset(&hash_keys, 0, sizeof(hash_keys));
>> -	hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>> -	if (skb) {
>> -		ip6_multipath_l3_keys(skb, &hash_keys);
>> -	} else {
>> -		hash_keys.addrs.v6addrs.src = fl6->saddr;
>> -		hash_keys.addrs.v6addrs.dst = fl6->daddr;
>> -		hash_keys.tags.flow_label = (__force u32)fl6->flowlabel;
>> -		hash_keys.basic.ip_proto = fl6->flowi6_proto;
>> +	switch (net->ipv6.sysctl.multipath_hash_policy) {
>> +	case 0:
>> +		memset(&hash_keys, 0, sizeof(hash_keys));
>> +		hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>> +		if (skb) {
>> +			ip6_multipath_l3_keys(skb, &hash_keys);
>> +		} else {
>> +			hash_keys.addrs.v6addrs.src = fl6->saddr;
>> +			hash_keys.addrs.v6addrs.dst = fl6->daddr;
>> +			hash_keys.tags.flow_label = (__force u32)fl6->flowlabel;
>> +			hash_keys.basic.ip_proto = fl6->flowi6_proto;
>> +		}
>> +		break;
>> +	case 1:
>> +		if (skb) {
>> +			unsigned int flag = FLOW_DISSECTOR_F_STOP_AT_ENCAP;
>> +			struct flow_keys keys;
>> +
>> +			/* short-circuit if we already have L4 hash present */
>> +			if (skb->l4_hash)
>> +				return skb_get_hash_raw(skb) >> 1;
>> +
>> +			memset(&hash_keys, 0, sizeof(hash_keys));
>> +
>> +			skb_flow_dissect_flow_keys(skb, &keys, flag);
>> +
>> +			hash_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
>> +			hash_keys.addrs.v6addrs.dst = keys.addrs.v6addrs.dst;
> 
> Shouldn't you add:
> 
> hash_keys.control.addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> 
> ?
> 
> Otherwise flow_hash_from_keys() will not consistentify the ports and the
> addresses and it will also not use the addresses for the hash
> computation.
> 
> It's missing from fib_multipath_hash() as well, so I might be missing
> something.

Yes, I think you are correct. The oversight here is based on the missing
set in the ipv4 variant.

> 
>> +			hash_keys.ports.src = keys.ports.src;
>> +			hash_keys.ports.dst = keys.ports.dst;
>> +			hash_keys.tags.flow_label = keys.tags.flow_label;
> 
> Why are you using the flow label?

will remove. It is supposed to be an L4 hash.