@@ -773,10 +773,8 @@ static inline void iph_to_flow_copy_v6addrs(struct flow_keys *flow,staticinline__be32ip6_make_flowlabel(structnet*net,structsk_buff*skb,__be32flowlabel,boolautolabel,-structflowi6*fl6)+structflowi6*fl6,u32hash){-u32hash;-/* @flowlabel may include more than a flow label, eg, the traffic class.*Herewewantonlytheflowlabelvalue.*/
@@ -814,7 +813,7 @@ static inline int ip6_default_np_autolabel(struct net *net)staticinlinevoidip6_set_txhash(structsock*sk){}staticinline__be32ip6_make_flowlabel(structnet*net,structsk_buff*skb,__be32flowlabel,boolautolabel,-structflowi6*fl6)+structflowi6*fl6,u32hash){returnflowlabel;}
From: Shaohua Li <shli@kernel.org> Date: 2017-07-31 22:19:27
From: Shaohua Li <redacted>
According to David Miller, the rotation doesn't really help avoid
security problem, so delte it.
Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Shaohua Li <redacted>
---
include/net/ipv6.h | 6 ------
1 file changed, 6 deletions(-)
@@ -790,12 +790,6 @@ static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,hash=skb_get_hash_flowi6(skb,fl6);-/* Since this is being sent on the wire obfuscate hash a bit-*tominimizepossbilitythatanyusefulinformationtoan-*attackerisleaked.Onlylower20bitsarerelevant.-*/-rol32(hash,16);-flowlabel=(__force__be32)hash&IPV6_FLOWLABEL_MASK;if(net->ipv6.sysctl.flowlabel_state_ranges)
From: Cong Wang <hidden> Date: 2017-07-31 22:35:24
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li [off-list ref] wrote:
quoted hunk
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
Why not just move skb_get_hash_flowi6() to its caller?
This check is not necessary. If you don't want to touch
existing callers, you can just introduce a wrapper:
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
struct flowi6 *fl6)
{
u32 hash = skb_get_hash_flowi6(skb, fl6);
return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
}
And your code can just call:
__ip6_make_flowlabel(net, flowlabel, autolabel, sk->sk_txhash);
From: Shaohua Li <shli@kernel.org> Date: 2017-07-31 23:00:04
On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li [off-list ref] wrote:
quoted
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
Why not just move skb_get_hash_flowi6() to its caller?
This check is not necessary. If you don't want to touch
existing callers, you can just introduce a wrapper:
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
struct flowi6 *fl6)
{
u32 hash = skb_get_hash_flowi6(skb, fl6);
return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
}
this will always call skb_get_hash_flowi6 for the fast path even auto flowlabel
is disabled. I thought we should avoid this.
And your code can just call:
__ip6_make_flowlabel(net, flowlabel, autolabel, sk->sk_txhash);
From: Cong Wang <hidden> Date: 2017-08-01 21:18:20
On Mon, Jul 31, 2017 at 4:00 PM, Shaohua Li [off-list ref] wrote:
On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
quoted
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li [off-list ref] wrote:
quoted
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
Why not just move skb_get_hash_flowi6() to its caller?
This check is not necessary. If you don't want to touch
existing callers, you can just introduce a wrapper:
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
struct flowi6 *fl6)
{
u32 hash = skb_get_hash_flowi6(skb, fl6);
return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
}
this will always call skb_get_hash_flowi6 for the fast path even auto flowlabel
is disabled. I thought we should avoid this.
Yeah, but you can move the check out too,
something like:
iph_to_flow_copy_v6addrs(struct flow_keys *flow,
#define IP6_DEFAULT_AUTO_FLOW_LABELS IP6_AUTO_FLOW_LABEL_OPTOUT
-static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
- __be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+static inline bool ip6_need_flowlabel(struct net *net, __be32
flowlabel, bool autolabel)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
- flowlabel &= IPV6_FLOWLABEL_MASK;
-
- if (flowlabel ||
+ if ((flowlabel & IPV6_FLOWLABEL_MASK) ||
net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
(!autolabel &&
net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
- return flowlabel;
-
- hash = skb_get_hash_flowi6(skb, fl6);
+ return false;
- /* Since this is being sent on the wire obfuscate hash a bit
- * to minimize possbility that any useful information to an
- * attacker is leaked. Only lower 20 bits are relevant.
- */
- rol32(hash, 16);
+ return true;
+}
+static inline __be32 __ip6_make_flowlabel(struct net *net, __be32
flowlabel, u32 hash)
+{
flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
if (net->ipv6.sysctl.flowlabel_state_ranges)
From: Shaohua Li <shli@kernel.org> Date: 2017-08-01 21:42:28
On Tue, Aug 01, 2017 at 02:17:58PM -0700, Cong Wang wrote:
On Mon, Jul 31, 2017 at 4:00 PM, Shaohua Li [off-list ref] wrote:
quoted
On Mon, Jul 31, 2017 at 03:35:02PM -0700, Cong Wang wrote:
quoted
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li [off-list ref] wrote:
quoted
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+ struct flowi6 *fl6, u32 hash)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
Why not just move skb_get_hash_flowi6() to its caller?
This check is not necessary. If you don't want to touch
existing callers, you can just introduce a wrapper:
static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
__be32 flowlabel, bool autolabel,
struct flowi6 *fl6)
{
u32 hash = skb_get_hash_flowi6(skb, fl6);
return __ip6_make_flowlabel(net, flowlabel, autolabel, hash);
}
this will always call skb_get_hash_flowi6 for the fast path even auto flowlabel
is disabled. I thought we should avoid this.
Yeah, but you can move the check out too,
something like:
Is this really better? I don't see any point. I'd use my original patch other
than this one. that said, there are just several lines of code, brutally
'abstract' them into a function doesn't make the code better.
iph_to_flow_copy_v6addrs(struct flow_keys *flow,
#define IP6_DEFAULT_AUTO_FLOW_LABELS IP6_AUTO_FLOW_LABEL_OPTOUT
-static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb,
- __be32 flowlabel, bool autolabel,
- struct flowi6 *fl6)
+static inline bool ip6_need_flowlabel(struct net *net, __be32
flowlabel, bool autolabel)
{
- u32 hash;
-
/* @flowlabel may include more than a flow label, eg, the traffic class.
* Here we want only the flow label value.
*/
- flowlabel &= IPV6_FLOWLABEL_MASK;
-
- if (flowlabel ||
+ if ((flowlabel & IPV6_FLOWLABEL_MASK) ||
net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF ||
(!autolabel &&
net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED))
- return flowlabel;
-
- hash = skb_get_hash_flowi6(skb, fl6);
+ return false;
- /* Since this is being sent on the wire obfuscate hash a bit
- * to minimize possbility that any useful information to an
- * attacker is leaked. Only lower 20 bits are relevant.
- */
- rol32(hash, 16);
+ return true;
+}
+static inline __be32 __ip6_make_flowlabel(struct net *net, __be32
flowlabel, u32 hash)
+{
flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK;
if (net->ipv6.sysctl.flowlabel_state_ranges)
From: Cong Wang <hidden> Date: 2017-08-03 01:33:42
On Tue, Aug 1, 2017 at 2:42 PM, Shaohua Li [off-list ref] wrote:
Is this really better? I don't see any point. I'd use my original patch other
than this one. that said, there are just several lines of code, brutally
'abstract' them into a function doesn't make the code better.
The current ip6_make_flowlabel() is already a mess, with your
change you just make it worse to read. I just want to make it suck
less.
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
Tom
From: David Miller <davem@davemloft.net> Date: 2017-08-09 17:55:47
From: Shaohua Li <shli@kernel.org>
Date: Wed, 9 Aug 2017 07:59:53 -0700
Could you please look at the patches?
If you actually looked in patchwork, the state of your patches is
"changes requested".
This means that you were given feedback asking to change something
about your patches, which means that a new submission is expected.
I think Cong Wang's function movement request is legitimate, so
please do it.
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
Thanks,
Shaohua
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Tom
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
Thanks,
Shaohua
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Tom
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Thanks,
Shaohua
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. In TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Tom
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. In TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Hi Tom,
Did you mean revert 877d1f6291f8(net: Set sk_txhash from a random number)? This
could fix the issue. So in normal case we calculate the sk_txhash using flow
dissector but in negative routing case we use the random hash, is this what you
want?
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
What's the 'src port for UDP encap'? I can't find the code setting skb->hash
to sk_txhash in UDP side.
Thanks,
Shaohua
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Hi Tom,
Did you mean revert 877d1f6291f8(net: Set sk_txhash from a random number)? This
could fix the issue. So in normal case we calculate the sk_txhash using flow
dissector but in negative routing case we use the random hash, is this what you
want?
No. What you'd want is something like a sysctl that sets an alternate
mode for sk_txhash processing. sk_txhash is derived from flow
dissector for the first TX packet and then it's never allowed to
change. Maybe this should be called persistent-hash mode.
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
What's the 'src port for UDP encap'? I can't find the code setting skb->hash
to sk_txhash in UDP side.
udp_flow_src_port is function call by UDP encaps to set source port.
This is call skb_get_hash. sk_set_txhash is function to set txhash
right now to random value. skb_set_hash_from_sk set skb->hash when
skbuff is owned by socket (skb_set_owner_w).
Thanks,
Tom
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Hi Tom,
Did you mean revert 877d1f6291f8(net: Set sk_txhash from a random number)? This
could fix the issue. So in normal case we calculate the sk_txhash using flow
dissector but in negative routing case we use the random hash, is this what you
want?
No. What you'd want is something like a sysctl that sets an alternate
mode for sk_txhash processing. sk_txhash is derived from flow
dissector for the first TX packet and then it's never allowed to
change. Maybe this should be called persistent-hash mode.
persistent-hash will almostly mean disabling auto flowlabel. We'd prefer
disabling auto flowlabel rather than the persistent-hash. But I think we do
want to use the auto flowlabel for load balancing, so the persistent-hash isn't
a go for us.
Let's take the other way. Your main concern is the multipath selection. I think
this can be fixed. We can still use the tw->tw_flowlabel to store the auto
created flowlabel like what my patch does, but we use an extra bit in tw_pad of
inet_timewait_sock to indicate this flowlabel is auto created. In the reset
packet send path, we don't use the auto created flowlabel for route slection,
but just for packet data. This will keep current multipath selection behavior
and fix the reset packet flowlabel issue.
quoted
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
I'll look at this issue later. It's not related to current flowlabel problem.
Thanks,
Shaohua
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Hi Tom,
Did you mean revert 877d1f6291f8(net: Set sk_txhash from a random number)? This
could fix the issue. So in normal case we calculate the sk_txhash using flow
dissector but in negative routing case we use the random hash, is this what you
want?
No. What you'd want is something like a sysctl that sets an alternate
mode for sk_txhash processing. sk_txhash is derived from flow
dissector for the first TX packet and then it's never allowed to
change. Maybe this should be called persistent-hash mode.
persistent-hash will almostly mean disabling auto flowlabel. We'd prefer
disabling auto flowlabel rather than the persistent-hash. But I think we do
want to use the auto flowlabel for load balancing, so the persistent-hash isn't
a go for us.
Those are two separate features here. Auto flow label means
automatically generating flow labels for packets based on some hash. A
persistent hash means that once a hash is determined for a connection
it never changes, so the flow label derived from the hash never
changes.
Let's take the other way. Your main concern is the multipath selection. I think
this can be fixed. We can still use the tw->tw_flowlabel to store the auto
created flowlabel like what my patch does, but we use an extra bit in tw_pad of
inet_timewait_sock to indicate this flowlabel is auto created. In the reset
packet send path, we don't use the auto created flowlabel for route slection,
but just for packet data. This will keep current multipath selection behavior
and fix the reset packet flowlabel issue.
quoted
quoted
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
I'll look at this issue later. It's not related to current flowlabel problem.
As I mentioned already, this is not a problem specific to flowlabels.
This is a problem about getting all the elements of the system work
together to ensure that all packets of flow are routed they same way
in order to meet a requirement imposed by middleboxes. You can solve
this problem just to make flow labels work, but IMO that's just
kicking the can down the road for someone else to have to debug and
solve when they hit the same underlying issue but in a different
context.
Tom
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
The #2 is exactly the problem we saw.
quoted
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
As far as I understood, we don't use sk_txhash for routing selection. The code
does routing selection with flowlabel user configured, at that time we don't
derive fl6.flowlabel from skb->hash (which is from sk_txhash). The code always
does routing selection first and then uses ip6_make_flowlabel to build packet
data where we derive flowlabel from skb->hash.
That is assuming one particular use case. Generally, if you want to
ensure all packets for a flow take the same path you'll need tx_hash
and make it persistent (disable flow bender). For instance, if you
were doing UDP encapsulation like in VXLAN the UDP source port
selection is unaffected by saved flow label for the lifetime of the
flow. So we would still hit #2 in that case and the stateful device
doesn't see whole flow. It might be just as easy to move tx_hash in
skc_common so that it's available in TW state for this purpose. Then
when moving to TW state just copy the tx_hash.
Hi Tom,
My original implementation is to add a tx_hash in tw sock, we then copy sock's
tx_hash to the tw tx_hash. This does makes things simplier. One concern from
Eric is this will increase the size of tw sock. If we move tx_hash to
skc_common, all sock size will increase, is this acceptable?
I think that can only be measured by how critical it is to
persistently route all packets the same exact way for every
connection. Page one of the IP book clearly states that IP packets can
be dropped, duplicated, or received out of order. Received OOO implies
that packet for the same flow are allowed to take different paths. The
requirement that packets for the same flow must always take the same
path through the network was created by stateful middleboxes-- it's
not inherent in the architecture of IP networking. Unfortunately,
we're seeing this become more and more of a problem as more devices
are multi-homed (like smart phones) and these network requirement
cripple our ability to take advantage of features like that.
Personally, I wish the middleboxes fix the problem they created, but I
suppose we need to be pragmatic at least in the short term.
Hmm, I still hesitate to add a new field in skc_common. Fixing current problem
looks propriate in current stage. I'd defer fixing the generic issue till it's
necessary.
Shaohua,
An alternative would be to not initialize sk_txhash, but instead defer
hash computation to use flow dissector in the TX path when the hash is
needed (to get flow label, src port for UDP encap, route for
multipath, etc.). At the first hash computation in TX path the result
in sk_txhash. TW state there is no socket so flow dissector is
always used but that should yield the same hash. No extra fields would
be needed and additional cost is negligible.
Hi Tom,
Did you mean revert 877d1f6291f8(net: Set sk_txhash from a random number)? This
could fix the issue. So in normal case we calculate the sk_txhash using flow
dissector but in negative routing case we use the random hash, is this what you
want?
No. What you'd want is something like a sysctl that sets an alternate
mode for sk_txhash processing. sk_txhash is derived from flow
dissector for the first TX packet and then it's never allowed to
change. Maybe this should be called persistent-hash mode.
Hi Tom,
We had been using the auto_flowlabels=1 (i.e. essentially enable flowlabel)
mainly because we want to take the benefit of dst_negative_advice() when
tcp_write_timeout() happens.
During our test, our system handles quite well with changing flowlabel.
The only exception we have hit is the TCP_RST sent from an inet_timewait_sock.
If we keep the flowlabel consistent (or persistent sk_txhash), there
is no practical usage for us to turn on flowlabel and the problem also goes
away. We have it off for now.
quoted
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
The localhost's mutlipath selection is another existing issue. AFAICT,
it does not take the sk_txhash (or skb->hash) into account and the following
dst_negative_advice() will also have no effect in the route selction.
It is another issue to be fixed and to be figured out how to pass the
sk_txhash down. (1)
Shaohua is proposing to record the 20 bits of the sk_txhash in the
tw_flowlabel of the 'struct inet_timewait_sock'. The tw_flowlabel could
potentially be used to do the multipath selection once we figured out
how to tackle (1).
Thanks,
Martin
quoted
What's the 'src port for UDP encap'? I can't find the code setting skb->hash
to sk_txhash in UDP side.
udp_flow_src_port is function call by UDP encaps to set source port.
This is call skb_get_hash. sk_set_txhash is function to set txhash
right now to random value. skb_set_hash_from_sk set skb->hash when
skbuff is owned by socket (skb_set_owner_w).
Thanks,
Tom
From: Tom Herbert <hidden> Date: 2017-08-18 14:50:04
We had been using the auto_flowlabels=1 (i.e. essentially enable flowlabel)
mainly because we want to take the benefit of dst_negative_advice() when
tcp_write_timeout() happens.
During our test, our system handles quite well with changing flowlabel.
The only exception we have hit is the TCP_RST sent from an inet_timewait_sock.
Martin,
That is interesting data. Have you determined why the middlebox has a
problem with flow label change in TW state but not other states?
Tom
If we keep the flowlabel consistent (or persistent sk_txhash), there
is no practical usage for us to turn on flowlabel and the problem also goes
away. We have it off for now.
quoted
quoted
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
The localhost's mutlipath selection is another existing issue. AFAICT,
it does not take the sk_txhash (or skb->hash) into account and the following
dst_negative_advice() will also have no effect in the route selction.
It is another issue to be fixed and to be figured out how to pass the
sk_txhash down. (1)
Shaohua is proposing to record the 20 bits of the sk_txhash in the
tw_flowlabel of the 'struct inet_timewait_sock'. The tw_flowlabel could
potentially be used to do the multipath selection once we figured out
how to tackle (1).
Thanks,
Martin
quoted
quoted
What's the 'src port for UDP encap'? I can't find the code setting skb->hash
to sk_txhash in UDP side.
udp_flow_src_port is function call by UDP encaps to set source port.
This is call skb_get_hash. sk_set_txhash is function to set txhash
right now to random value. skb_set_hash_from_sk set skb->hash when
skbuff is owned by socket (skb_set_owner_w).
Thanks,
Tom
From: Martin KaFai Lau <hidden> Date: 2017-08-18 20:51:42
On Fri, Aug 18, 2017 at 07:50:03AM -0700, Tom Herbert wrote:
quoted
We had been using the auto_flowlabels=1 (i.e. essentially enable flowlabel)
mainly because we want to take the benefit of dst_negative_advice() when
tcp_write_timeout() happens.
During our test, our system handles quite well with changing flowlabel.
The only exception we have hit is the TCP_RST sent from an inet_timewait_sock.
Martin,
That is interesting data. Have you determined why the middlebox has a
problem with flow label change in TW state but not other states?
Tom,
The problem of this middle box is specific to TCP_RST with a different
flowlabel from its previous packets. Sending TCP_RST from TW state hits
this pain point.
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
We are resilience to a small number of TCP_RST drop. However, this guarantee
flowlabel change on TCP_RST and then dropped is too much.
This flowlabel change does not look like intentional either when transitioning
from full sk to tw sk (tw->tw_flowlabel is inheriting the np->flow_label
in tcp_time_wait()). Currently, the tw_flowlabel is used in tcp_v6_timewait_ack()
but not in tcp_v6_send_reset(). Hence, shaohua is looking for a solution to solve
them together.
Thanks,
Martin
Tom
quoted
If we keep the flowlabel consistent (or persistent sk_txhash), there
is no practical usage for us to turn on flowlabel and the problem also goes
away. We have it off for now.
quoted
quoted
There seems to have other bug in this side. From my understanding, commit
265f94ff54d6(net: Recompute sk_txhash on negative routing advice) tries to
select a different route. But the multipath selection code
(rt6_multipath_select) doesn't use sk_txhash or skb->hash, it does use
fl6.flowlabel, but that is the flowlabel user sets. So looks like the commit
doesn't change anything.
The routing functions typically don't use sock of skbuff, but use flow
structs instead. It may be reasonable to add a hash to those.
The localhost's mutlipath selection is another existing issue. AFAICT,
it does not take the sk_txhash (or skb->hash) into account and the following
dst_negative_advice() will also have no effect in the route selction.
It is another issue to be fixed and to be figured out how to pass the
sk_txhash down. (1)
Shaohua is proposing to record the 20 bits of the sk_txhash in the
tw_flowlabel of the 'struct inet_timewait_sock'. The tw_flowlabel could
potentially be used to do the multipath selection once we figured out
how to tackle (1).
Thanks,
Martin
quoted
quoted
What's the 'src port for UDP encap'? I can't find the code setting skb->hash
to sk_txhash in UDP side.
udp_flow_src_port is function call by UDP encaps to set source port.
This is call skb_get_hash. sk_set_txhash is function to set txhash
right now to random value. skb_set_hash_from_sk set skb->hash when
skbuff is owned by socket (skb_set_owner_w).
Thanks,
Tom
From: David Miller <davem@davemloft.net> Date: 2017-08-18 22:27:10
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
Thank you.
From: Tom Herbert <hidden> Date: 2017-11-08 17:44:52
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Tom
From: Tom Herbert <hidden> Date: 2017-11-08 20:01:54
On Wed, Nov 8, 2017 at 9:44 AM, Tom Herbert [off-list ref] wrote:
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
quoted
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Sorry, I failed to give credit to Shaohua for submitting the initial
patch. Please take look!
From: Martin KaFai Lau <hidden> Date: 2017-11-08 21:41:44
On Wed, Nov 08, 2017 at 12:01:53PM -0800, Tom Herbert wrote:
On Wed, Nov 8, 2017 at 9:44 AM, Tom Herbert [off-list ref] wrote:
quoted
quoted
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Sorry, I failed to give credit to Shaohua for submitting the initial
patch. Please take look!
Hi Tom, thanks for the info. Shaohua will revisit this
when he returns next week.
Martin
From: Shaohua Li <shli@kernel.org> Date: 2017-11-14 18:24:57
On Wed, Nov 08, 2017 at 09:44:51AM -0800, Tom Herbert wrote:
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
quoted
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Hi Tom,
Do we really need to use the five-tupe hash? There are several places using
current random hash, which looks more lightweight. To fix issue, we only need
to make sure reset packet include the correct flowlabel. Like what my previous
patch did, we can set tw->tw_flowlabel in tcp_time_wait based on txhash and use
it reset packet. In this way we can use the random hash and not add extra field
in sock.
Thanks,
Shaohua
From: Tom Herbert <hidden> Date: 2017-11-14 19:13:11
On Tue, Nov 14, 2017 at 10:24 AM, Shaohua Li [off-list ref] wrote:
On Wed, Nov 08, 2017 at 09:44:51AM -0800, Tom Herbert wrote:
quoted
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
quoted
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Hi Tom,
Do we really need to use the five-tupe hash? There are several places using
current random hash, which looks more lightweight. To fix issue, we only need
to make sure reset packet include the correct flowlabel. Like what my previous
patch did, we can set tw->tw_flowlabel in tcp_time_wait based on txhash and use
it reset packet. In this way we can use the random hash and not add extra field
in sock.
Shaohua,
But that patch discards the full txhash in TW. So it's not just a
problem with the flow label. sk_tx_hash can also be used for route
selection in ECMP, port selection we're doing tunneling, etc. The
general solution should maintains tx_hash or be able to reconstruct it
in any state, flow label fix is a point solution.
Thanks,
Tom
From: Shaohua Li <shli@kernel.org> Date: 2017-11-14 21:59:05
On Tue, Nov 14, 2017 at 11:13:10AM -0800, Tom Herbert wrote:
On Tue, Nov 14, 2017 at 10:24 AM, Shaohua Li [off-list ref] wrote:
quoted
On Wed, Nov 08, 2017 at 09:44:51AM -0800, Tom Herbert wrote:
quoted
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
quoted
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Hi Tom,
Do we really need to use the five-tupe hash? There are several places using
current random hash, which looks more lightweight. To fix issue, we only need
to make sure reset packet include the correct flowlabel. Like what my previous
patch did, we can set tw->tw_flowlabel in tcp_time_wait based on txhash and use
it reset packet. In this way we can use the random hash and not add extra field
in sock.
Shaohua,
But that patch discards the full txhash in TW. So it's not just a
problem with the flow label. sk_tx_hash can also be used for route
selection in ECMP, port selection we're doing tunneling, etc. The
general solution should maintains tx_hash or be able to reconstruct it
in any state, flow label fix is a point solution.
Hi Tom,
do you want to keep sk_rethink_txhash() then? If we changed the hash to random
number, we can't reconstruct it for sure.
Thanks,
Shaohua
From: Martin KaFai Lau <hidden> Date: 2017-11-15 18:27:44
On Tue, Nov 14, 2017 at 01:59:03PM -0800, Shaohua Li wrote:
On Tue, Nov 14, 2017 at 11:13:10AM -0800, Tom Herbert wrote:
quoted
On Tue, Nov 14, 2017 at 10:24 AM, Shaohua Li [off-list ref] wrote:
quoted
On Wed, Nov 08, 2017 at 09:44:51AM -0800, Tom Herbert wrote:
quoted
On Fri, Aug 18, 2017 at 3:27 PM, David Miller [off-list ref] wrote:
quoted
From: Martin KaFai Lau <redacted>
Date: Fri, 18 Aug 2017 13:51:36 -0700
quoted
It seems like that middle box specifically drops TCP_RST if it
does not know anything about this flow. Since the flowlabel of the TCP_RST
(sent in TW state) is always different, it always lands to a different middle
box. All of these TCP_RST cannot be delivered.
This really is illegal behavior. The flow label is not a flow _KEY_
by any definition whatsoever.
Flow labels are an optimization, not a determinant for flow matching
particularly for proper TCP state processing.
I'd rather you invest all of this energy getting that vendor to fix
their kit.
We're now seeing several router vendors recommending people to not use
flow labels for ECMP hashing. This is precisely because when a flow
label changes, network devices that maintain state (firewalls, NAT,
load balancers) can't deal with packets being rerouted so connections
are dropped. Unfortunately, the need for packets of a flow to always
follow the same path has become an implicit requirement that I think
we need follow at least as the default behavior.
Martin: is there any change you could resurrect these patches? In
order to solve the general problem of making routing consistent, I
believe we want to keep sk_tx_hash consistent for the connection from
which a consistent flow label can be derived. To avoid the overhead of
a hash field in sk_common, maybe we could initially set a connection
hash to a five-tuple hash for a flow instead of a random value? So in
TW state the consistent hash can be computed on the fly.
Hi Tom,
Do we really need to use the five-tupe hash? There are several places using
current random hash, which looks more lightweight. To fix issue, we only need
to make sure reset packet include the correct flowlabel. Like what my previous
patch did, we can set tw->tw_flowlabel in tcp_time_wait based on txhash and use
it reset packet. In this way we can use the random hash and not add extra field
in sock.
Shaohua,
But that patch discards the full txhash in TW. So it's not just a
problem with the flow label. sk_tx_hash can also be used for route
selection in ECMP, port selection we're doing tunneling, etc. The
general solution should maintains tx_hash or be able to reconstruct it
in any state, flow label fix is a point solution.
Hi Tom,
do you want to keep sk_rethink_txhash() then? If we changed the hash to random
number, we can't reconstruct it for sure.
A followup question on rethink. Does it mean we need
a new sysctl (persistent_txhash) to avoid sk_rethink_txhash() together
such that it keeps the routing decision consistent (e.g. flowlabel) ?