net: Small bug on rxhash calculation

PATCH In review Chema Gonzalez 4 participants 9 messages Sep 6, 2012 – Sep 8, 2012

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Chema Gonzalez

author

2012-09-06 06:35 UTC #0 lore raw

PATCH 1/1 net: Small bug on rxhash calculation

Quoted 1 line

From 5c02179069826bfba9360383b88601b31ff05517 Mon Sep 17 00:00:00 2001

From: Chema Gonzalez <[email protected]>
Date: Wed, 5 Sep 2012 17:05:48 -0700
Subject: [PATCH] net: Small bug on rxhash calculation

While the (current) sorting of the ports/addresses is coherent (you get the same rxhash for packets sharing the same (unsorted) 4-tuple), ports and addresses should not be sorted independently, which currently are: For traffic between A=S:l and B=L:s, packets in both directions would get their rxhash from hashing {L, S, {s, l}}. The same rxhash is obtained from packets between C=S:s and D=L:l.

This patch ensures traffic between A and B gets their rxhash from {S, L, {s, l}}, and traffic between C and D from {S, L, {l, s}}

The patch is co-written with Eric Dumazet <[email protected]>

Signed-off-by Chema Gonzalez <[email protected]>

---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

net/core/dev.c +6 -5

index b1e6d63..dcc673d 100644
@@ -2655,15 +2655,16 @@ void __skb_get_rxhash(struct sk_buff *skb)
        if (!skb_flow_dissect(skb, &keys))
                return;
 
-       if (keys.ports) {
-               if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
-                       swap(keys.port16[0], keys.port16[1]);
+       if (keys.ports)
                skb->l4_rxhash = 1;
-       }
 
        /* get a consistent hash (same value on both flow directions) */
-       if ((__force u32)keys.dst < (__force u32)keys.src)
+       if (((__force u32)keys.dst < (__force u32)keys.src) ||
+           (((__force u32)keys.dst == (__force u32)keys.src) &&
+            ((__force u16)keys.port16[1] < (__force u16)keys.port16[0]))) {
                swap(keys.dst, keys.src);
+               swap(keys.port16[0], keys.port16[1]);
+       }
 
        hash = jhash_3words((__force u32)keys.dst,
                            (__force u32)keys.src,

--
1.7.7.3

Eric Dumazet

2012-09-06 06:42 UTC #1 lore raw

Quoted 13 lines · Chema Gonzalez

From 5c02179069826bfba9360383b88601b31ff05517 Mon Sep 17 00:00:00 2001
From: Chema Gonzalez <[email protected]>
Date: Wed, 5 Sep 2012 17:05:48 -0700
Subject: [PATCH] net: Small bug on rxhash calculation
While the (current) sorting of the ports/addresses is coherent (you get the same rxhash for packets sharing the same (unsorted) 4-tuple), ports and addresses should not be sorted independently, which currently are: For traffic between A=S:l and B=L:s, packets in both directions would get their rxhash from hashing {L, S, {s, l}}. The same rxhash is obtained from packets between C=S:s and D=L:l.

Quoted 8 lines

This patch ensures traffic between A and B gets their rxhash from {S, L, {s, l}}, and traffic between C and D from {S, L, {l, s}}
The patch is co-written with Eric Dumazet <[email protected]>
Signed-off-by Chema Gonzalez <[email protected]>
---

Signed-off-by Eric Dumazet <[email protected]>

Thanks !

David Miller

maintainer

2012-09-07 16:57 UTC #2 lore raw

In reply to Chema Gonzalez

From: Chema Gonzalez <[email protected]>
Date: Wed, 5 Sep 2012 23:35:40 -0700

net/core/dev.c void __skb_get_rxhash(struct sk_buff *skb) · line 2658

Quoted 4 lines

net/core/dev.c +1 -3

-       if (keys.ports) {
-               if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
-                       swap(keys.port16[0], keys.port16[1]);
+       if (keys.ports)

This patch was corrupted by your email client, it transformed TAB characters into spaces, making your patch completely useless to us.

Please fix this, send a test patch to yourself, and make sure that the patch you receive in that test email will apply cleanly and properly.

Only after you are able to get such an email test patch to work should you submit it here again.

Thanks.

Chema Gonzalez

2012-09-07 23:40 UTC #3 lore raw

PATCH 1/1 net: small bug on rxhash calculation

In the current rxhash calculation function, while the sorting of the ports/addrs is coherent (you get the same rxhash for packets sharing the same 4-tuple, in both directions), ports and addrs are sorted independently. This implies packets from a connection between the same addresses but crossed ports hash to the same rxhash.

For example, traffic between A=S:l and B=L:s is hashed (in both directions) from {L, S, {s, l}}. The same rxhash is obtained for packets between C=S:s and D=L:l.

This patch ensures that you either swap both addrs and ports, or you swap none. Traffic between A and B, and traffic between C and D, get their rxhash from different sources ({L, S, {l, s}} for A<->B, and {L, S, {s, l}} for C<->D)

The patch is co-written with Eric Dumazet <[email protected]>

Signed-off-by Chema Gonzalez <[email protected]>

---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

net/core/dev.c +6 -5

index b1e6d63..dcc673d 100644
@@ -2655,15 +2655,16 @@ void __skb_get_rxhash(struct sk_buff *skb)
 	if (!skb_flow_dissect(skb, &keys))
 		return;
 
-	if (keys.ports) {
-		if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
-			swap(keys.port16[0], keys.port16[1]);
+	if (keys.ports)
 		skb->l4_rxhash = 1;
-	}
 
 	/* get a consistent hash (same value on both flow directions) */
-	if ((__force u32)keys.dst < (__force u32)keys.src)
+	if (((__force u32)keys.dst < (__force u32)keys.src) ||
+	    (((__force u32)keys.dst == (__force u32)keys.src) &&
+	     ((__force u16)keys.port16[1] < (__force u16)keys.port16[0]))) {
 		swap(keys.dst, keys.src);
+		swap(keys.port16[0], keys.port16[1]);
+	}
 
 	hash = jhash_3words((__force u32)keys.dst,
 			    (__force u32)keys.src,

Signature · 1 line

1.7.7.3

Chema Gonzalez

2012-09-07 23:41 UTC #4 lore raw

PATCH 1/1 net: small bug on rxhash calculation

For example, traffic between A=S:l and B=L:s is hashed (in both directions) from {L, S, {s, l}}. The same rxhash is obtained for packets between C=S:s and D=L:l.

The patch is co-written with Eric Dumazet <[email protected]>

Signed-off-by Chema Gonzalez <[email protected]>

---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

net/core/dev.c +6 -5

index b1e6d63..dcc673d 100644
@@ -2655,15 +2655,16 @@ void __skb_get_rxhash(struct sk_buff *skb)
 	if (!skb_flow_dissect(skb, &keys))
 		return;
 
-	if (keys.ports) {
-		if ((__force u16)keys.port16[1] < (__force u16)keys.port16[0])
-			swap(keys.port16[0], keys.port16[1]);
+	if (keys.ports)
 		skb->l4_rxhash = 1;
-	}
 
 	/* get a consistent hash (same value on both flow directions) */
-	if ((__force u32)keys.dst < (__force u32)keys.src)
+	if (((__force u32)keys.dst < (__force u32)keys.src) ||
+	    (((__force u32)keys.dst == (__force u32)keys.src) &&
+	     ((__force u16)keys.port16[1] < (__force u16)keys.port16[0]))) {
 		swap(keys.dst, keys.src);
+		swap(keys.port16[0], keys.port16[1]);
+	}
 
 	hash = jhash_3words((__force u32)keys.dst,
 			    (__force u32)keys.src,

Signature · 1 line

1.7.7.3

David Miller

maintainer

2012-09-07 23:50 UTC #5 lore raw

Re: [PATCH] net: small bug on rxhash calculation

You just sent two copies of this patch, why did you do this and which one should I use?

Chema Gonzalez

2012-09-08 00:24 UTC #6 lore raw

Re: [PATCH] net: small bug on rxhash calculation

The patches are exactly the same. I added the patch name twice to git-send-email :(

-Chema

Quoted 3 lines · David Miller

You just sent two copies of this patch, why did you do this and which one should I use?

Eric Dumazet

2012-09-08 08:18 UTC #7 lore raw

Re: [PATCH] net: small bug on rxhash calculation

In reply to Chema Gonzalez

Quoted 17 lines · Chema Gonzalez

In the current rxhash calculation function, while the sorting of the ports/addrs is coherent (you get the same rxhash for packets sharing the same 4-tuple, in both directions), ports and addrs are sorted independently. This implies packets from a connection between the same addresses but crossed ports hash to the same rxhash.
For example, traffic between A=S:l and B=L:s is hashed (in both directions) from {L, S, {s, l}}. The same rxhash is obtained for packets between C=S:s and D=L:l.
This patch ensures that you either swap both addrs and ports, or you swap none. Traffic between A and B, and traffic between C and D, get their rxhash from different sources ({L, S, {l, s}} for A<->B, and {L, S, {s, l}} for C<->D)

Quoted 4 lines

The patch is co-written with Eric Dumazet <[email protected]>
Signed-off-by Chema Gonzalez <[email protected]>
---

Signed-off-by Eric Dumazet <[email protected]>

David Miller

maintainer

2012-09-08 22:43 UTC #8 lore raw

Re: [PATCH] net: small bug on rxhash calculation

From: Eric Dumazet <[email protected]>
Date: Sat, 08 Sep 2012 10:18:40 +0200

Quoted 18 lines · Chema Gonzalez

Quoted 17 lines
In the current rxhash calculation function, while the sorting of the ports/addrs is coherent (you get the same rxhash for packets sharing the same 4-tuple, in both directions), ports and addrs are sorted independently. This implies packets from a connection between the same addresses but crossed ports hash to the same rxhash.
For example, traffic between A=S:l and B=L:s is hashed (in both directions) from {L, S, {s, l}}. The same rxhash is obtained for packets between C=S:s and D=L:l.
This patch ensures that you either swap both addrs and ports, or you swap none. Traffic between A and B, and traffic between C and D, get their rxhash from different sources ({L, S, {l, s}} for A<->B, and {L, S, {s, l}} for C<->D)

Quoted 6 lines

Quoted 4 lines
The patch is co-written with Eric Dumazet <[email protected]>
Signed-off-by Chema Gonzalez <[email protected]>
---
Signed-off-by Eric Dumazet <[email protected]>

Applied and queued up for -stable, thanks.