[PATCH] net: Small bug on rxhash calculation

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

From: Chema Gonzalez <redacted>
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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..dcc673d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -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

On Wed, 2012-09-05 at 23:35 -0700, Chema Gonzalez wrote:

From 5c02179069826bfba9360383b88601b31ff05517 Mon Sep 17 00:00:00 2001
From: Chema Gonzalez <redacted>
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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---

Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks !

From: Chema Gonzalez <redacted>
Date: Wed, 5 Sep 2012 23:35:40 -0700

-       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.

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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..dcc673d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -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

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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b1e6d63..dcc673d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -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

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

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

-Chema

On Fri, Sep 7, 2012 at 4:50 PM, David Miller [off-list ref] wrote:

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

On Fri, 2012-09-07 at 16:40 -0700, Chema Gonzalez wrote:

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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---

Signed-off-by: Eric Dumazet <edumazet@google.com>

From: Eric Dumazet <redacted>
Date: Sat, 08 Sep 2012 10:18:40 +0200

On Fri, 2012-09-07 at 16:40 -0700, Chema Gonzalez wrote:

quoted

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 [off-list ref]

Signed-off-by: Chema Gonzalez <redacted>
---

Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks.