Re: [PATCH] udp: port starting location not random

Re: [PATCH] udp: port starting location not random

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Thu, 04 Oct 2012 14:05:26 -0700 (PDT)

>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Thu, 4 Oct 2012 13:56:56 -0700
>> 
>> > -		first = (((u64)rand * remaining) >> 32) + low;
>> > +		first = rand % remaining + low;
>> 
>> I really don't get it, either a modulus via multiplcation
>> works or it doesn't.  We have this construct all over the
>> place.
>> 
>> 
> 
> Try out of kernel:
> 
> #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;
> 	rand = atoi(av[3]);
> 
> 	first = (((uint64_t)rand * remaining) >> 32) + low;
> 
> 	printf("%d %d %u => %u\n", low, high, rand, first);
> 	return 0;
> }
> 
> $ port-test 32768 61000 $RANDOM
> 32768 61000 7027 => 32768

It just shows that we need to shift "remaining" up into the top N
bits instead of the bottom N bits.

[PATCH] udp: port starting location not random

[Stephen Hemminger]

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

commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
Author: Eric Dumazet <dada1@cosmosbay.com>
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 <shemminger@vyatta.com>

---
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
 		 */

Re: [PATCH] udp: port starting location not random

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 4 Oct 2012 14:08:28 -0700

> While working on VXLAN, noticed a bug in UDP introduced by:
> 
> commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
> Author: Eric Dumazet <dada1@cosmosbay.com>
> 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 <shemminger@vyatta.com>
> 
> ---
> 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.

Re: [PATCH] udp: port starting location not random

[Stephen Hemminger]

On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 4 Oct 2012 14:08:28 -0700
> 
> > While working on VXLAN, noticed a bug in UDP introduced by:
> > 
> > commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
> > Author: Eric Dumazet <dada1@cosmosbay.com>
> > 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 <shemminger@vyatta.com>
> > 
> > ---
> > 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.

Re: [PATCH] udp: port starting location not random

[Eric Dumazet]

On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:
> On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Stephen Hemminger <shemminger@vyatta.com>
> > Date: Thu, 4 Oct 2012 14:08:28 -0700
> > 
> > > While working on VXLAN, noticed a bug in UDP introduced by:
> > > 
> > > commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
> > > Author: Eric Dumazet <dada1@cosmosbay.com>
> > > 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 <shemminger@vyatta.com>
> > > 
> > > ---
> > > 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.

Re: [PATCH] udp: port starting location not random

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 04 Oct 2012 23:45:53 +0200

> On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:
>> On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
>> David Miller <davem@davemloft.net> wrote:
>> 
>> > From: Stephen Hemminger <shemminger@vyatta.com>
>> > Date: Thu, 4 Oct 2012 14:08:28 -0700
>> > 
>> > > While working on VXLAN, noticed a bug in UDP introduced by:
>> > > 
>> > > commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
>> > > Author: Eric Dumazet <dada1@cosmosbay.com>
>> > > 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 <shemminger@vyatta.com>
>> > > 
>> > > ---
>> > > 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'.

Re: [PATCH] udp: port starting location not random

[Eric Dumazet]

On Thu, 2012-10-04 at 17:50 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 04 Oct 2012 23:45:53 +0200
> 
> > On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:
> >> On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
> >> David Miller <davem@davemloft.net> wrote:
> >> 
> >> > From: Stephen Hemminger <shemminger@vyatta.com>
> >> > Date: Thu, 4 Oct 2012 14:08:28 -0700
> >> > 
> >> > > While working on VXLAN, noticed a bug in UDP introduced by:
> >> > > 
> >> > > commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
> >> > > Author: Eric Dumazet <dada1@cosmosbay.com>
> >> > > 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 <shemminger@vyatta.com>
> >> > > 
> >> > > ---
> >> > > 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

Re: [PATCH] udp: port starting location not random

[Stephen Hemminger]

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;
}

Re: [PATCH] udp: port starting location not random

[Stephen Hemminger]

On Thu, 04 Oct 2012 17:50:09 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 04 Oct 2012 23:45:53 +0200
> 
> > On Thu, 2012-10-04 at 14:28 -0700, Stephen Hemminger wrote:
> >> On Thu, 04 Oct 2012 17:12:46 -0400 (EDT)
> >> David Miller <davem@davemloft.net> wrote:
> >> 
> >> > From: Stephen Hemminger <shemminger@vyatta.com>
> >> > Date: Thu, 4 Oct 2012 14:08:28 -0700
> >> > 
> >> > > While working on VXLAN, noticed a bug in UDP introduced by:
> >> > > 
> >> > > commit 9088c5609584684149f3fb5b065aa7f18dcb03ff
> >> > > Author: Eric Dumazet <dada1@cosmosbay.com>
> >> > > 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 <shemminger@vyatta.com>
> >> > > 
> >> > > ---
> >> > > 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;