| Message ID | 20121004140828.2d6f7bf9@nehalam.linuxnetplumber.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
--- 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 */
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