[PATCH] ipv4/route.c: respect prefsrc for local routes

[PATCH] ipv4/route.c: respect prefsrc for local routes

[Joel Sing]

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

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Eric Dumazet]

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

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Eric Dumazet]

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;

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Changli Gao]

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 "? :"

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Eric Dumazet]

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))

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Eric Dumazet]

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))

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Ben Hutchings]

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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[Joe Perches]

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)

Re: [PATCH] ipv4/route.c: respect prefsrc for local routes

[David Miller]

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.