| Message ID | 1294122260-13245-1-git-send-email-jsing@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 04 janvier 2011 à 17:24 +1100, Joel Sing a écrit : > The preferred source address is currently ignored for local routes, > which results in all local connections having a src address that is the > same as the local dst address. Fix this by respecting the preferred source > address when it is provided for local routes. > > This bug can be demonstrated as follows: > > # ifconfig dummy0 192.168.0.1 > # ip route show table local | grep local.*dummy0 > local 192.168.0.1 dev dummy0 proto kernel scope host src 192.168.0.1 > # ip route change table local local 192.168.0.1 dev dummy0 \ > proto kernel scope host src 127.0.0.1 > # ip route show table local | grep local.*dummy0 > local 192.168.0.1 dev dummy0 proto kernel scope host src 127.0.0.1 > > We now establish a local connection and verify the source IP > address selection: > > # nc -l 192.168.0.1 3128 & > # nc 192.168.0.1 3128 & > # netstat -ant | grep 192.168.0.1:3128.*EST > tcp 0 0 192.168.0.1:3128 192.168.0.1:33228 ESTABLISHED > tcp 0 0 192.168.0.1:33228 192.168.0.1:3128 ESTABLISHED > > Signed-off-by: Joel Sing <jsing@google.com> > --- > net/ipv4/route.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index df948b0..93bfd95 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -2649,8 +2649,12 @@ static int ip_route_output_slow(struct net *net, struct rtable **rp, > } > > if (res.type == RTN_LOCAL) { > - if (!fl.fl4_src) > - fl.fl4_src = fl.fl4_dst; > + if (!fl.fl4_src) { > + if (res.fi->fib_prefsrc) > + fl.fl4_src = res.fi->fib_prefsrc; > + else > + fl.fl4_src = fl.fl4_dst; > + } > dev_out = net->loopback_dev; > fl.oif = dev_out->ifindex; > res.fi = NULL; Please use FIB_RES_PREFSRC(res) as we do a few lines after ;) Thanks -- 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
Le mardi 04 janvier 2011 à 08:24 +0100, Eric Dumazet a écrit : > Please use FIB_RES_PREFSRC(res) > Hmm no, this is not applicable, but this could be shorter : fl.fl4_src = res.fi->fib_prefsrc ? : fl.fl4_dst; -- 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 Tue, Jan 4, 2011 at 3:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 04 janvier 2011 à 08:24 +0100, Eric Dumazet a écrit : > >> Please use FIB_RES_PREFSRC(res) >> > > Hmm no, this is not applicable, but this could be shorter : > > fl.fl4_src = res.fi->fib_prefsrc ? : fl.fl4_dst; > > I think Joe may object the use of "? :"
Le mardi 04 janvier 2011 à 16:07 +0800, Changli Gao a écrit : > On Tue, Jan 4, 2011 at 3:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Le mardi 04 janvier 2011 à 08:24 +0100, Eric Dumazet a écrit : > > > >> Please use FIB_RES_PREFSRC(res) > >> > > > > Hmm no, this is not applicable, but this could be shorter : > > > > fl.fl4_src = res.fi->fib_prefsrc ? : fl.fl4_dst; > > > > > > I think Joe may object the use of "? :" > Ternary operator is standard C idiom, used in networking stuff, for example in FIB_RES_PREFSRC() ;) This could be properly done using another macro in include/net/ip_fib.h to centralize this ternary op in one point : #define __FIB_RES_PREFSRC(res, default) ((res).fi->fib_prefsrc ? : default) #define FIB_RES_PREFSRC(res) __FIB_RES_PREFSRC(res, default, __fib_res_prefsrc(&res)) -- 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
Le mardi 04 janvier 2011 à 09:38 +0100, Eric Dumazet a écrit : > This could be properly done using another macro in include/net/ip_fib.h > to centralize this ternary op in one point : > > #define __FIB_RES_PREFSRC(res, default) ((res).fi->fib_prefsrc ? : default) > #define FIB_RES_PREFSRC(res) __FIB_RES_PREFSRC(res, default, __fib_res_prefsrc(&res) I meant #define FIB_RES_PREFSRC(res) __FIB_RES_PREFSRC(res, __fib_res_prefsrc(&res)) -- 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 Tue, 2011-01-04 at 09:38 +0100, Eric Dumazet wrote: > Le mardi 04 janvier 2011 à 16:07 +0800, Changli Gao a écrit : > > On Tue, Jan 4, 2011 at 3:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > Le mardi 04 janvier 2011 à 08:24 +0100, Eric Dumazet a écrit : > > > > > >> Please use FIB_RES_PREFSRC(res) > > >> > > > > > > Hmm no, this is not applicable, but this could be shorter : > > > > > > fl.fl4_src = res.fi->fib_prefsrc ? : fl.fl4_dst; > > > > > > > > > > I think Joe may object the use of "? :" > > > > Ternary operator is standard C idiom, used in networking stuff, for > example in FIB_RES_PREFSRC() ;) [...] However, the option to omit the second operand is a GNU extension. Ben.
On Tue, 2011-01-04 at 14:16 +0000, Ben Hutchings wrote: > On Tue, 2011-01-04 at 09:38 +0100, Eric Dumazet wrote: > > Le mardi 04 janvier 2011 à 16:07 +0800, Changli Gao a écrit : > > > On Tue, Jan 4, 2011 at 3:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > Le mardi 04 janvier 2011 à 08:24 +0100, Eric Dumazet a écrit : > > > >> Please use FIB_RES_PREFSRC(res) > > > > Hmm no, this is not applicable, but this could be shorter : > > > > fl.fl4_src = res.fi->fib_prefsrc ? : fl.fl4_dst; > > > I think Joe may object the use of "? :" > > Ternary operator is standard C idiom, used in networking stuff, for > > example in FIB_RES_PREFSRC() ;) > However, the option to omit the second operand is a GNU extension. I don't object to using GNU extensions. The ?: extension is used in most every major subsystem. $ grep -rP --include=*.[ch] "\?\s*:" * | wc -l 515 (with some false positives) -- 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: Joel Sing <jsing@google.com> Date: Tue, 4 Jan 2011 17:24:20 +1100 > The preferred source address is currently ignored for local routes, > which results in all local connections having a src address that is the > same as the local dst address. Fix this by respecting the preferred source > address when it is provided for local routes. > > This bug can be demonstrated as follows: > > # ifconfig dummy0 192.168.0.1 > # ip route show table local | grep local.*dummy0 > local 192.168.0.1 dev dummy0 proto kernel scope host src 192.168.0.1 > # ip route change table local local 192.168.0.1 dev dummy0 \ > proto kernel scope host src 127.0.0.1 > # ip route show table local | grep local.*dummy0 > local 192.168.0.1 dev dummy0 proto kernel scope host src 127.0.0.1 > > We now establish a local connection and verify the source IP > address selection: > > # nc -l 192.168.0.1 3128 & > # nc 192.168.0.1 3128 & > # netstat -ant | grep 192.168.0.1:3128.*EST > tcp 0 0 192.168.0.1:3128 192.168.0.1:33228 ESTABLISHED > tcp 0 0 192.168.0.1:33228 192.168.0.1:3128 ESTABLISHED > > Signed-off-by: Joel Sing <jsing@google.com> Applied to net-2.6, thanks Joel. If you guys want to mess with ternary operators and new macros, please do that in net-next-2.6 the next time I merge or similar. Thanks. -- 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
diff --git a/net/ipv4/route.c b/net/ipv4/route.c index df948b0..93bfd95 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -2649,8 +2649,12 @@ static int ip_route_output_slow(struct net *net, struct rtable **rp, } if (res.type == RTN_LOCAL) { - if (!fl.fl4_src) - fl.fl4_src = fl.fl4_dst; + if (!fl.fl4_src) { + if (res.fi->fib_prefsrc) + fl.fl4_src = res.fi->fib_prefsrc; + else + fl.fl4_src = fl.fl4_dst; + } dev_out = net->loopback_dev; fl.oif = dev_out->ifindex; res.fi = NULL;
The preferred source address is currently ignored for local routes, which results in all local connections having a src address that is the same as the local dst address. Fix this by respecting the preferred source address when it is provided for local routes. This bug can be demonstrated as follows: # ifconfig dummy0 192.168.0.1 # ip route show table local | grep local.*dummy0 local 192.168.0.1 dev dummy0 proto kernel scope host src 192.168.0.1 # ip route change table local local 192.168.0.1 dev dummy0 \ proto kernel scope host src 127.0.0.1 # ip route show table local | grep local.*dummy0 local 192.168.0.1 dev dummy0 proto kernel scope host src 127.0.0.1 We now establish a local connection and verify the source IP address selection: # nc -l 192.168.0.1 3128 & # nc 192.168.0.1 3128 & # netstat -ant | grep 192.168.0.1:3128.*EST tcp 0 0 192.168.0.1:3128 192.168.0.1:33228 ESTABLISHED tcp 0 0 192.168.0.1:33228 192.168.0.1:3128 ESTABLISHED Signed-off-by: Joel Sing <jsing@google.com> --- net/ipv4/route.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)