Patchwork net: small bug on rxhash calculation

login
register
mail settings
Submitter Chema Gonzalez
Date Sept. 7, 2012, 11:40 p.m.
Message ID <1347061251-2019-1-git-send-email-chema@google.com>
Download mbox | patch
Permalink /patch/182490/
State Accepted
Delegated to: David Miller
Headers show

Comments

Chema Gonzalez - Sept. 7, 2012, 11:40 p.m.
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 <edumazet@google.com>

Signed-off-by: Chema Gonzalez <chema@google.com>
---
 net/core/dev.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
Eric Dumazet - Sept. 8, 2012, 8:18 a.m.
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 <edumazet@google.com>
> 
> Signed-off-by: Chema Gonzalez <chema@google.com>
> ---

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



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Sept. 8, 2012, 10:43 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 08 Sep 2012 10:18:40 +0200

> 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 <edumazet@google.com>
>> 
>> Signed-off-by: Chema Gonzalez <chema@google.com>
>> ---
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,