[PATCH] udp: port starting location not random

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;
 		/*
 		 * force rand to be an odd multiple of UDP_HTABLE_SIZE
 		 */

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller [off-list ref] wrote:

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

quoted

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

The standalone program gets same result.

On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller [off-list ref] wrote:

quoted

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

quoted

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

The standalone program gets same result.

Hey, I hope you understand random32() does allocate a 32bit value, not a
15bit one...

If really we had such a bug, I am pretty sure we would have noticed.

From: Eric Dumazet <redacted>
Date: Thu, 04 Oct 2012 23:45:53 +0200

On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:

quoted

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller [off-list ref] wrote:

quoted

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

quoted

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

The standalone program gets same result.

Hey, I hope you understand random32() does allocate a 32bit value, not a
15bit one...

If really we had such a bug, I am pretty sure we would have noticed.

But the issue is is "remaining" that's of limited range, not rand.
I didn't say to shift up 'rand', but rather 'remaining'.

On Thu, 2012-10-04 at 17:50 -0400, David Miller wrote:

From: Eric Dumazet <redacted>
Date: Thu, 04 Oct 2012 23:45:53 +0200

quoted

On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:

quoted

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller [off-list ref] wrote:

quoted

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

quoted

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

The standalone program gets same result.

Hey, I hope you understand random32() does allocate a 32bit value, not a
15bit one...

If really we had such a bug, I am pretty sure we would have noticed.

But the issue is is "remaining" that's of limited range, not rand.
I didn't say to shift up 'rand', but rather 'remaining'.

Sorry I see no issue.

remaining is the delta between high and low, and its right,
unless your compiler or cpu has a nice bug.

remaining = (high - low) + 1;

And I see no bug at all here.

The only assumption is that rand is a random number between 0 and
0xFFFFFFFF

$RANDOM is between 0 and 0x7FFF

Thats the difference

On Thu, 04 Oct 2012 17:50:09 -0400 (EDT)
David Miller [off-list ref] wrote:

From: Eric Dumazet <redacted>
Date: Thu, 04 Oct 2012 23:45:53 +0200

quoted

On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:

quoted

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller [off-list ref] wrote:

quoted

From: Stephen Hemminger <redacted>
Date: Thu, 4 Oct 2012 14:08:28 -0700

quoted

While working on VXLAN, noticed a bug in UDP introduced by:

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet [off-list ref]
Date:   Wed Oct 8 11:44:17 2008 -0700

    udp: Improve port randomization
    

The logic for choosing where to start for port randomization incorrectly
calculates the starting port number. It is always ends up using
the low end of the range independent of the value of random.
This causes all UDP port searches to start at the same port.

Doing the following fixes it but at the cost of doing a real divide.

Signed-off-by: Stephen Hemminger <redacted>

---
Resend, previous send was not going to netdev.

Not sure if worth fixing for stable, because only has performance impact
and some application might be depending on current broken behaviour.

--- a/net/ipv4/udp.c	2012-10-01 17:06:53.107427436 -0700
+++ b/net/ipv4/udp.c	2012-10-04 13:43:21.278960379 -0700

@@ -216,7 +216,7 @@ int udp_lib_get_port(struct sock *sk, un
 		remaining = (high - low) + 1;
 
 		rand = net_random();
-		first = (((u64)rand * remaining) >> 32) + low;
+		first = rand % remaining + low;

Try replacing "remaining" with "(remaining << (64 - 16))" in
the expression instead.

The standalone program gets same result.

Hey, I hope you understand random32() does allocate a 32bit value, not a
15bit one...

If really we had such a bug, I am pretty sure we would have noticed.

But the issue is is "remaining" that's of limited range, not rand.
I didn't say to shift up 'rand', but rather 'remaining'.

I think the issue is the Gcc is deciding to truncate the math.
If it is done as separate steps, then the right answer happens:

Good:
	t = ((uint64_t) rand * remaining) >> 32;
	first = t + low;

Bad:
	first = (((uint64_t)rand * remaining) >> (64-16)) + low;

Nevermind, it depends on random being big enough. This shows that it
works with large enough random(). The hash I was getting from VXLAN
was too small...

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

int main(int ac, char **av) {
	int low, high, remaining;
	unsigned int rand;
	unsigned short first;
	
	low = atoi(av[1]);
	high = atoi(av[2]);
	remaining = (high - low) + 1;
	srandom(getpid());
	rand = (uint32_t) random();
	
	first = (((uint64_t)rand * remaining) >> 32) + low;

	printf("%d %d %u => %u\n", low, high, rand, first);
	return 0;
}