Re: [PATCH net] ipv6: omit traffic class when calculating flow hash

[David Ahern]

On 6/1/18 4:34 AM, Michal Kubecek wrote:
> Some of the code paths calculating flow hash for IPv6 use flowlabel member
> of struct flowi6 which, despite its name, encodes both flow label and
> traffic class. If traffic class changes within a TCP connection (as e.g.
> ssh does), ECMP route can switch between path. It's also incosistent with
> other code paths where ip6_flowlabel() (returning only flow label) is used
> to feed the key.
> 
> Use only flow label everywhere, including one place where hash key is set
> using ip6_flowinfo().
> 
> Fixes: 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)")
> Fixes: f70ea018da06 ("net: Add functions to get skb->hash based on flow structures")
> Signed-off-by: Michal Kubecek <mkubecek@xxxxxxx>
> ---
>  net/core/flow_dissector.c | 3 ++-
>  net/ipv6/route.c          | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index d29f09bc5ff9..441d3db76e8e 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1334,7 +1334,8 @@ __u32 __get_hash_from_flowi6(const struct flowi6 *fl6, struct flow_keys *keys)
>  	keys->ports.src = fl6->fl6_sport;
>  	keys->ports.dst = fl6->fl6_dport;
>  	keys->keyid.keyid = fl6->fl6_gre_key;
> -	keys->tags.flow_label = (__force u32)fl6->flowlabel;
> +	keys->tags.flow_label = (__force u32)(fl6->flowlabel &
> +					      IPV6_FLOWLABEL_MASK);
>  	keys->basic.ip_proto = fl6->flowi6_proto;
>  
>  	return flow_hash_from_keys(keys);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f4d61736c41a..fcbacf1677f8 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1868,7 +1868,7 @@ static void ip6_multipath_l3_keys(const struct sk_buff *skb,
>  	} else {
>  		keys->addrs.v6addrs.src = key_iph->saddr;
>  		keys->addrs.v6addrs.dst = key_iph->daddr;
> -		keys->tags.flow_label = ip6_flowinfo(key_iph);
> +		keys->tags.flow_label = ip6_flowlabel(key_iph);
>  		keys->basic.ip_proto = key_iph->nexthdr;
>  	}
>  }
> @@ -1889,7 +1889,8 @@ u32 rt6_multipath_hash(const struct net *net, const struct flowi6 *fl6,
>  		} 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.tags.flow_label = (__force u32)(fl6->flowlabel &
> +								  IPV6_FLOWLABEL_MASK);
>  			hash_keys.basic.ip_proto = fl6->flowi6_proto;
>  		}
>  		break;
> 

Can you make an inline for the flowlabel conversion. Something like this:

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 798558fd1681..e36eca2f8531 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -284,6 +284,11 @@ struct ip6_flowlabel {
 #define IPV6_FLOWLABEL_MASK            cpu_to_be32(0x000FFFFF)
 #define IPV6_FLOWLABEL_STATELESS_FLAG  cpu_to_be32(0x00080000)

+static inline u32 flowi6_get_flowlabel(const struct flowi6 *fl6)
+{
+       return (__force u32)(fl6->flowlabel & IPV6_FLOWLABEL_MASK);
+}
+
 #define IPV6_TCLASS_MASK (IPV6_FLOWINFO_MASK & ~IPV6_FLOWLABEL_MASK)
 #define IPV6_TCLASS_SHIFT      20

>From there we can fix the flow struct to have flowinfo instead of
flowlabel and use the macro to hide the conversion.