Patchwork ipv4/route.c: respect prefsrc for local routes

login
register
mail settings
Submitter Joel Sing
Date Jan. 4, 2011, 6:24 a.m.
Message ID <1294122260-13245-1-git-send-email-jsing@google.com>
Download mbox | patch
Permalink /patch/77390/
State Accepted
Delegated to: David Miller
Headers show

Comments

Joel Sing - Jan. 4, 2011, 6:24 a.m.
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(-)
Eric Dumazet - Jan. 4, 2011, 7:24 a.m.
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
Eric Dumazet - Jan. 4, 2011, 7:33 a.m.
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
Changli Gao - Jan. 4, 2011, 8:07 a.m.
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 "? :"
Eric Dumazet - Jan. 4, 2011, 8:38 a.m.
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
Eric Dumazet - Jan. 4, 2011, 8:40 a.m.
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
Ben Hutchings - Jan. 4, 2011, 2:16 p.m.
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.
Joe Perches - Jan. 4, 2011, 3:32 p.m.
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
David Miller - Jan. 4, 2011, 7:35 p.m.
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

Patch

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;