[PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Tom Herbert]

On IPv6 transmit if there is no flow label preset use the skb_hash
or skb->sk->sk_hash as a flow label to support the transmit side of
RFC6438. Added an IPv6 sysctl auto_flow_labels to enable/disable this
behavior.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 include/net/ipv6.h         | 14 ++++++++++++++
 include/net/netns/ipv6.h   |  1 +
 net/ipv6/af_inet6.c        |  1 +
 net/ipv6/ip6_gre.c         |  7 +++++--
 net/ipv6/ip6_output.c      |  5 +++--
 net/ipv6/ip6_tunnel.c      |  3 ++-
 net/ipv6/sysctl_net_ipv6.c |  8 ++++++++
 7 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index d640925..9354ae2 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -664,6 +664,20 @@ void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
 
 int ip6_dst_hoplimit(struct dst_entry *dst);
 
+static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
+					__be32 flowlabel)
+{
+	__be32 hash;
+
+	if (flowlabel || !net->ipv6.sysctl.auto_flowlabels)
+		return flowlabel;
+
+	hash = skb->sk && skb->sk->sk_hash ? skb->sk->sk_hash :
+					     skb_get_hash_raw(skb);
+
+	return hash & IPV6_FLOWLABEL_MASK;
+}
+
 /*
  *	Header manipulation
  */
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index 21edaf1..de5b01d 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -28,6 +28,7 @@ struct netns_sysctl_ipv6 {
 	int ip6_rt_mtu_expires;
 	int ip6_rt_min_advmss;
 	int flowlabel_consistency;
+	int auto_flowlabels;
 	int icmpv6_time;
 	int anycast_src_echo_reply;
 };
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index d935889..74d19ab 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -776,6 +776,7 @@ static int __net_init inet6_net_init(struct net *net)
 	net->ipv6.sysctl.bindv6only = 0;
 	net->ipv6.sysctl.icmpv6_time = 1*HZ;
 	net->ipv6.sysctl.flowlabel_consistency = 1;
+	net->ipv6.sysctl.auto_flowlabels = 1;
 	atomic_set(&net->ipv6.rt_genid, 0);
 
 	err = ipv6_init_mibs(net);
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 75277b7..2ae6e21 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -733,7 +733,8 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 	 *	Push down and install the IP header.
 	 */
 	ipv6h = ipv6_hdr(skb);
-	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield), fl6->flowlabel);
+	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
+		     ip6_make_flowlabel(net, skb, fl6->flowlabel));
 	ipv6h->hop_limit = tunnel->parms.hop_limit;
 	ipv6h->nexthdr = proto;
 	ipv6h->saddr = fl6->saddr;
@@ -1183,8 +1184,10 @@ static int ip6gre_header(struct sk_buff *skb, struct net_device *dev,
 	struct ip6_tnl *t = netdev_priv(dev);
 	struct ipv6hdr *ipv6h = (struct ipv6hdr *)skb_push(skb, t->hlen);
 	__be16 *p = (__be16 *)(ipv6h+1);
+	struct net *net = dev_net(dev);
 
-	ip6_flow_hdr(ipv6h, 0, t->fl.u.ip6.flowlabel);
+	ip6_flow_hdr(ipv6h, 0,
+		     ip6_make_flowlabel(net, skb, t->fl.u.ip6.flowlabel));
 	ipv6h->hop_limit = t->parms.hop_limit;
 	ipv6h->nexthdr = NEXTHDR_GRE;
 	ipv6h->saddr = t->parms.laddr;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 40e7581..4bde649 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -205,7 +205,7 @@ int ip6_xmit(struct sock *sk, struct sk_buff *skb, struct flowi6 *fl6,
 	if (hlimit < 0)
 		hlimit = ip6_dst_hoplimit(dst);
 
-	ip6_flow_hdr(hdr, tclass, fl6->flowlabel);
+	ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel));
 
 	hdr->payload_len = htons(seg_len);
 	hdr->nexthdr = proto;
@@ -1553,7 +1553,8 @@ int ip6_push_pending_frames(struct sock *sk)
 	skb_reset_network_header(skb);
 	hdr = ipv6_hdr(skb);
 
-	ip6_flow_hdr(hdr, np->cork.tclass, fl6->flowlabel);
+	ip6_flow_hdr(hdr, np->cork.tclass,
+		     ip6_make_flowlabel(net, skb, fl6->flowlabel));
 	hdr->hop_limit = np->cork.hop_limit;
 	hdr->nexthdr = proto;
 	hdr->saddr = fl6->saddr;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index b05b609..9595f96 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1045,7 +1045,8 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 	skb_push(skb, sizeof(struct ipv6hdr));
 	skb_reset_network_header(skb);
 	ipv6h = ipv6_hdr(skb);
-	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield), fl6->flowlabel);
+	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield,
+		     ip6_make_flowlabel(net, skb, fl6->flowlabel)));
 	ipv6h->hop_limit = t->parms.hop_limit;
 	ipv6h->nexthdr = proto;
 	ipv6h->saddr = fl6->saddr;
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index 7f405a1..93bdee8 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -38,6 +38,13 @@ static struct ctl_table ipv6_table_template[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
+	{
+		.procname	= "auto_flowlabels",
+		.data		= &init_net.ipv6.sysctl.auto_flowlabels,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
 	{ }
 };
 
@@ -67,6 +74,7 @@ static int __net_init ipv6_sysctl_net_init(struct net *net)
 	ipv6_table[0].data = &net->ipv6.sysctl.bindv6only;
 	ipv6_table[1].data = &net->ipv6.sysctl.anycast_src_echo_reply;
 	ipv6_table[2].data = &net->ipv6.sysctl.flowlabel_consistency;
+	ipv6_table[3].data = &net->ipv6.sysctl.auto_flowlabels;
 
 	ipv6_route_table = ipv6_route_sysctl_init(net);
 	if (!ipv6_route_table)
-- 
1.9.1.423.g4596e3a

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Eric Dumazet]

On Sat, 2014-04-26 at 16:00 -0700, Tom Herbert wrote:
>  
> +static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
> +					__be32 flowlabel)
> +{
> +	__be32 hash;
> +
> +	if (flowlabel || !net->ipv6.sysctl.auto_flowlabels)
> +		return flowlabel;
> +
> +	hash = skb->sk && skb->sk->sk_hash ? skb->sk->sk_hash :
> +					     skb_get_hash_raw(skb);
> +
> +	return hash & IPV6_FLOWLABEL_MASK;
> +}

Hmm... 

We use jhash and a random perturb to generate sk_hash which serves as
our input to locate sockets in hash table.

If we leak sk_hash, an attacker can easily target one hash slot.

You could simply use :

hash = skb->sk ? hash_ptr(skb->sk, 20) : skb_get_hash_raw(skb);

But even with this, we could leak sensitive information...

So I would rather use a separate sk->sk_flowhash value, and put a
completely random value in it.

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Tom Herbert]

On Sat, Apr 26, 2014 at 4:40 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2014-04-26 at 16:00 -0700, Tom Herbert wrote:
>>
>> +static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
>> +                                     __be32 flowlabel)
>> +{
>> +     __be32 hash;
>> +
>> +     if (flowlabel || !net->ipv6.sysctl.auto_flowlabels)
>> +             return flowlabel;
>> +
>> +     hash = skb->sk && skb->sk->sk_hash ? skb->sk->sk_hash :
>> +                                          skb_get_hash_raw(skb);
>> +
>> +     return hash & IPV6_FLOWLABEL_MASK;
>> +}
>
> Hmm...
>
> We use jhash and a random perturb to generate sk_hash which serves as
> our input to locate sockets in hash table.
>
> If we leak sk_hash, an attacker can easily target one hash slot.
>
Interesting point, but flow label is only 20 bits so it might not be
trivial to create such an attack?

> You could simply use :
>
> hash = skb->sk ? hash_ptr(skb->sk, 20) : skb_get_hash_raw(skb);
>
> But even with this, we could leak sensitive information...
>
> So I would rather use a separate sk->sk_flowhash value, and put a
> completely random value in it.
>
If we do that, I presume you'd want to use this value to seed
skb->hash on xmit also?

>
>

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Sat, 26 Apr 2014 16:00:30 -0700 (PDT)

> -	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield), fl6->flowlabel);
> +	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield,
> +		     ip6_make_flowlabel(net, skb, fl6->flowlabel)));

This doesn't compile on the planet on which I live.

net/ipv6/ip6_tunnel.c: In function ‘ip6_tnl_xmit2’:
net/ipv6/ip6_tunnel.c:1049:8: error: too many arguments to function ‘INET_ECN_encapsulate’
        ip6_make_flowlabel(net, skb, fl6->flowlabel)));
        ^
In file included from include/net/ip_tunnels.h:11:0,
                 from net/ipv6/ip6_tunnel.c:50:
include/net/inet_ecn.h:43:20: note: declared here
 static inline __u8 INET_ECN_encapsulate(__u8 outer, __u8 inner)
                    ^
net/ipv6/ip6_tunnel.c:1049:8: error: too few arguments to function ‘ip6_flow_hdr’
        ip6_make_flowlabel(net, skb, fl6->flowlabel)));
        ^
In file included from include/net/inetpeer.h:15:0,
                 from include/net/route.h:28,
                 from include/net/ip.h:31,
                 from net/ipv6/ip6_tunnel.c:49:
include/net/ipv6.h:684:20: note: declared here
 static inline void ip6_flow_hdr(struct ipv6hdr *hdr, unsigned int tclass,
                    ^
make[1]: *** [net/ipv6/ip6_tunnel.o] Error 1
make: *** [net/ipv6/ip6_tunnel.o] Error 2

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Florent Fourcot]

Le 27/04/2014 01:00, Tom Herbert a écrit :
>  	net->ipv6.sysctl.flowlabel_consistency = 1;
> +	net->ipv6.sysctl.auto_flowlabels = 1;

I do not like setting this option to one by default. The idea of
flowlabel_consistency above was to be sure that all labels given by the
host are in the control of the flow label manager.

By enabling auto_flowlabels, we can have conflict between the flow label
manager policy, and a randomly generated label.

And I do not have a strong opinion on it, but perhaps should
auto_flowlabels depends of a disabled flowlabel_consistency, to have a
coherent behaviour for users.

Regards,

Florent.

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Florent Fourcot]

I forgot to signal the lack of documentation for the new sysctl.

Regards,

Florent.

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Tom Herbert]

On Mon, Apr 28, 2014 at 11:12 AM, Florent Fourcot
<florent.fourcot@enst-bretagne.fr> wrote:
> Le 27/04/2014 01:00, Tom Herbert a écrit :
>>       net->ipv6.sysctl.flowlabel_consistency = 1;
>> +     net->ipv6.sysctl.auto_flowlabels = 1;
>
> I do not like setting this option to one by default. The idea of
> flowlabel_consistency above was to be sure that all labels given by the
> host are in the control of the flow label manager.
>
> By enabling auto_flowlabels, we can have conflict between the flow label
> manager policy, and a randomly generated label.
>
What would be the consequences of such a conflict? Per RFC6437
stateless and stateful flow labels are selected from the same space,
the probability of collision between them is assumed to be non-zero.
We obviously can't make any assumptions on RX.

Thanks,
Tom

> And I do not have a strong opinion on it, but perhaps should
> auto_flowlabels depends of a disabled flowlabel_consistency, to have a
> coherent behaviour for users.
>
> Regards,
>
> Florent.

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Florent Fourcot]

>>
>> By enabling auto_flowlabels, we can have conflict between the flow label
>> manager policy, and a randomly generated label.
>>
> What would be the consequences of such a conflict? Per RFC6437
> stateless and stateful flow labels are selected from the same space,
> the probability of collision between them is assumed to be non-zero.
> We obviously can't make any assumptions on RX.
> 

In the new RFC world, perhaps. But the flow label manager is older than
that, and implement a strong policy on flow labels.
Since we cannot know who is using it, and how there are using it, I'm
against this new behaviour. Disabling the manager is ok for me, but not
by default.

As second point, the name of auto_flowlabel is in my opinion not a
perfect choice if you set a label it only on tunnels. This is exactly
the same name that a BSD sysctl, setting a random label on all sockets
(TCP, UDP...). The flow label world is already hard enough to
understand, thanks to various implementations.

Regards,

-- 
Florent.

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Tom Herbert]

On Mon, Apr 28, 2014 at 12:37 PM, Florent Fourcot
<florent.fourcot@enst-bretagne.fr> wrote:
>
>>>
>>> By enabling auto_flowlabels, we can have conflict between the flow label
>>> manager policy, and a randomly generated label.
>>>
>> What would be the consequences of such a conflict? Per RFC6437
>> stateless and stateful flow labels are selected from the same space,
>> the probability of collision between them is assumed to be non-zero.
>> We obviously can't make any assumptions on RX.
>>
>
> In the new RFC world, perhaps. But the flow label manager is older than
> that, and implement a strong policy on flow labels.
>
> Since we cannot know who is using it, and how there are using it, I'm
> against this new behaviour. Disabling the manager is ok for me, but not
> by default.
>
But the RFC defines the on-the-wire protocol and we can never assume
that a site only uses Linux. As you pointed out BSD already has this
functionality, and we cannot assume any stateful association in
received flow labels. If someone is implementing a strong policy for
flow labels (like dropping based on it), it would be incorrect.
RFC6437 is clear on this:

  "A node that sets the flow label MAY also take part in a flow state
   establishment method that results in assigning specific treatments to
   specific flows, possibly including signaling.  Any such method MUST
   NOT disturb nodes taking part in the stateless scenario just
   described.  Thus, any node that sets flow label values according to a
   stateful scheme MUST choose labels that conform to Section 3 of this
   specification.  Further details are not discussed in this document."

> As second point, the name of auto_flowlabel is in my opinion not a
> perfect choice if you set a label it only on tunnels. This is exactly
> the same name that a BSD sysctl, setting a random label on all sockets
> (TCP, UDP...). The flow label world is already hard enough to
> understand, thanks to various implementations.
>
This is not just for tunnels, but would apply to transports and
potentially obviates the need for a lot of UDP encapsulation (like
ESP/UDP). So it would appear that this is pretty much the same thing
as what the BSD implementation does and same sysctl name (a happy
coincidence). Looks like the default in BSD is on btw.

Thanks,
Tom

> Regards,
>
> --
> Florent.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Florent Fourcot]

> If someone is implementing a strong policy for
> flow labels (like dropping based on it), it would be incorrect.
> RFC6437 is clear on this:
> 

And correct for RFC 3697:

>    A source node MUST ensure that it does not unintentionally reuse Flow
>    Label values it is currently using or has recently used when creating
>    new flows.

The situation is very uncomfortable, but the choice is to break old
implementations based on a rule valid between (at least) 2004 and 2011,
or to follow the newer RFC. If we not set it by default, the
distributions still have the choice to enable it.


>> As second point, the name of auto_flowlabel is in my opinion not a
>> perfect choice if you set a label it only on tunnels. This is exactly
>> the same name that a BSD sysctl, setting a random label on all sockets
>> (TCP, UDP...). The flow label world is already hard enough to
>> understand, thanks to various implementations.
>>
> This is not just for tunnels, but would apply to transports and
> potentially obviates the need for a lot of UDP encapsulation (like
> ESP/UDP). So it would appear that this is pretty much the same thing
> as what the BSD implementation does and same sysctl name (a happy
> coincidence). Looks like the default in BSD is on btw.
> 

Oups sorry, I missed the line in the patch for the transports. If you
follow the BSD implementation, what about the IPV6_AUTOFLOWLABEL socket
option, to disable/enable it only on some sockets? (the enable is
actually already available thanks to the manager).

Re: [PATCH 2/2] ipv6: Set flow label to skb_hash on transmit

[Tom Herbert]

On Mon, Apr 28, 2014 at 1:49 PM, Florent Fourcot
<florent.fourcot@enst-bretagne.fr> wrote:
>
>> If someone is implementing a strong policy for
>> flow labels (like dropping based on it), it would be incorrect.
>> RFC6437 is clear on this:
>>
>
> And correct for RFC 3697:
>
>>    A source node MUST ensure that it does not unintentionally reuse Flow
>>    Label values it is currently using or has recently used when creating
>>    new flows.
>
> The situation is very uncomfortable, but the choice is to break old
> implementations based on a rule valid between (at least) 2004 and 2011,
> or to follow the newer RFC. If we not set it by default, the
> distributions still have the choice to enable it.
>
Agreed, following robustness principle I will concede that the
conservative approach is to not enable auto_flowlabel by default. But
for RX purposes, the implementation needs to be consistent with 6437--
again we should not enforce that a received flow label corresponds to
a stateful connection (BSD would apparently break that). This might be
something that needs to be considered with flowlabel manager?

>
>>> As second point, the name of auto_flowlabel is in my opinion not a
>>> perfect choice if you set a label it only on tunnels. This is exactly
>>> the same name that a BSD sysctl, setting a random label on all sockets
>>> (TCP, UDP...). The flow label world is already hard enough to
>>> understand, thanks to various implementations.
>>>
>> This is not just for tunnels, but would apply to transports and
>> potentially obviates the need for a lot of UDP encapsulation (like
>> ESP/UDP). So it would appear that this is pretty much the same thing
>> as what the BSD implementation does and same sysctl name (a happy
>> coincidence). Looks like the default in BSD is on btw.
>>
>
> Oups sorry, I missed the line in the patch for the transports. If you
> follow the BSD implementation, what about the IPV6_AUTOFLOWLABEL socket
> option, to disable/enable it only on some sockets? (the enable is
> actually already available thanks to the manager).

I doubt a socket option would be very useful. The goal is to affect
aggregate behavior in the network which means we'd want it to be
generally used. As I mentioned, my primary use case would be to use
this with ESP (transport or tunnel mode) so we can avoid an extra UDP
encap with IPv6 for getting things like RPS and ECMP.

Thanks,
Tom