RE: [PATCH net-next] hv_netvsc: don't make assumptions on struct flow_keys layout

-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: Thursday, January 14, 2016 5:08 PM
To: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Tom Herbert <redacted>; One Thousand Gnomes
[off-list ref]; David Miller [off-list ref];
vkuznets@redhat.com; netdev@vger.kernel.org; KY Srinivasan
[off-list ref]; devel@linuxdriverproject.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH net-next] hv_netvsc: don't make assumptions on
struct flow_keys layout

On Thu, 2016-01-14 at 20:23 +0000, Haiyang Zhang wrote:

quoted

quoted

For non-random inputs, I used the port selection of iperf that

increases

quoted

the port number by 2 for each connection. Only send-port numbers are
different, other values are the same. I also tested some other fixed
increment, Toeplitz spreads the connections evenly. For real

applications,

quoted

if the load came from local area, then the IP/port combinations are
likely to have some non-random patterns.

We are not putting code in core networking stack favoring non secure
behavior.

The +2 behavior for connections from A to B:<fixed port> is something
that we will eventually remove in the future. It used to be +1 not a
long time ago...

Say if we implement the following,

https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2ftools.i
etf.org%2fhtml%2frfc6056%23section-
3.3.4&data=01%7c01%7chaiyangz%40microsoft.com%7ced5f98ae23a843df05c408d3
1d2f3028%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=uPo0Rdme20vZX%2b%2
frcwe1iE0mKGZYl%2fMdeaF1wld%2fgbQ%3d


The fact that Toeplitz hash has this linear property should not be a
valid reason to help hackers to exploit vulnerabilities.

In my tests I was using netperf, which randomizes both source &
destination ports.

This is why I could not reproduce your results based on iperf, which
generates 5-tuple in a totally predictable way.

This reminds me some drivers had a well known Toeplitz RSS key, allowing
attackers to direct their attack on a single queue.

I guess we could replace sk_txhash generator by a simple linear
allocator and boom, your driver will be pleased.

But this is only for a very specific workload.

diff --git a/include/net/sock.h b/include/net/sock.h
index e830c1006935..949527413cfb 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

@@ -1689,7 +1689,8 @@ unsigned long sock_i_ino(struct sock *sk);

 static inline u32 net_tx_rndhash(void)
 {
-       u32 v = prandom_u32();
+       static u32 last_hash;
+       u32 v = ++last_hash; // do not care about SMP races.

        return v ?: 1;
 }