Patchwork udp: port starting location not random

login
register
mail settings
Submitter stephen hemminger
Date Oct. 4, 2012, 9:08 p.m.
Message ID <20121004140828.2d6f7bf9@nehalam.linuxnetplumber.net>
Download mbox | patch
Permalink /patch/189324/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

stephen hemminger - Oct. 4, 2012, 9:08 p.m.
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.



--
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 - Oct. 4, 2012, 9:12 p.m.
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.
--
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
stephen hemminger - Oct. 4, 2012, 9:28 p.m.
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.

--
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
Eric Dumazet - Oct. 4, 2012, 9:45 p.m.
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.




--
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 - Oct. 4, 2012, 9:50 p.m.
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'.
--
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
Eric Dumazet - Oct. 4, 2012, 9:57 p.m.
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


--
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
stephen hemminger - Oct. 4, 2012, 9:59 p.m.
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;


--
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
Stephen Hemminger - Oct. 4, 2012, 10:06 p.m.
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;
}
--
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

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