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

[David Ahern <dsahern@gmail.com>]

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.