* [PATCH] udp: port starting location not random
@ 2012-10-04 21:08 Stephen Hemminger
2012-10-04 21:12 ` David Miller
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2012-10-04 21:08 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: netdev
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
*/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
[not found] ` <b400428e-ffe4-42ee-b656-99b0314d64f4@tahiti.vyatta.com>
@ 2012-10-04 21:12 ` David Miller
0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2012-10-04 21:12 UTC (permalink / raw)
To: stephen.hemminger; +Cc: dada1, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:08 Stephen Hemminger
@ 2012-10-04 21:12 ` David Miller
2012-10-04 21:28 ` Stephen Hemminger
0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2012-10-04 21:12 UTC (permalink / raw)
To: shemminger; +Cc: dada1, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:12 ` David Miller
@ 2012-10-04 21:28 ` Stephen Hemminger
2012-10-04 21:45 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2012-10-04 21:28 UTC (permalink / raw)
To: David Miller; +Cc: dada1, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:28 ` Stephen Hemminger
@ 2012-10-04 21:45 ` Eric Dumazet
2012-10-04 21:50 ` David Miller
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-10-04 21:45 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, dada1, netdev
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:45 ` Eric Dumazet
@ 2012-10-04 21:50 ` David Miller
2012-10-04 21:57 ` Eric Dumazet
2012-10-04 21:59 ` Stephen Hemminger
0 siblings, 2 replies; 9+ messages in thread
From: David Miller @ 2012-10-04 21:50 UTC (permalink / raw)
To: eric.dumazet; +Cc: shemminger, dada1, netdev
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'.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:50 ` David Miller
@ 2012-10-04 21:57 ` Eric Dumazet
2012-10-04 22:06 ` Stephen Hemminger
2012-10-04 21:59 ` Stephen Hemminger
1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-10-04 21:57 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, dada1, netdev
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:50 ` David Miller
2012-10-04 21:57 ` Eric Dumazet
@ 2012-10-04 21:59 ` Stephen Hemminger
1 sibling, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2012-10-04 21:59 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, dada1, netdev
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] udp: port starting location not random
2012-10-04 21:57 ` Eric Dumazet
@ 2012-10-04 22:06 ` Stephen Hemminger
0 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2012-10-04 22:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller
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;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-10-04 22:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20121004.170103.1493867833710136820.davem@davemloft.net>
[not found] ` <b400428e-ffe4-42ee-b656-99b0314d64f4@tahiti.vyatta.com>
2012-10-04 21:12 ` [PATCH] udp: port starting location not random David Miller
2012-10-04 21:08 Stephen Hemminger
2012-10-04 21:12 ` David Miller
2012-10-04 21:28 ` Stephen Hemminger
2012-10-04 21:45 ` Eric Dumazet
2012-10-04 21:50 ` David Miller
2012-10-04 21:57 ` Eric Dumazet
2012-10-04 22:06 ` Stephen Hemminger
2012-10-04 21:59 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).