[BUG] VPN broken in net-next

[BUG] VPN broken in net-next

[Stephen Hemminger]

My PPTP VPN is broken with net-next tree, it works fine with 2.6.38-rc6
but not in the next tree. Bisected the problem down to the following commit.

Debugging to see which address request is failing with the new code.

9435eb1cf0b76b323019cebf8d16762a50a12a19 is the first bad commit
commit 9435eb1cf0b76b323019cebf8d16762a50a12a19
Author: David S. Miller <davem@davemloft.net>
Date:   Fri Feb 18 12:43:09 2011 -0800

    ipv4: Implement __ip_dev_find using new interface address hash.
    
    Much quicker than going through the FIB tables.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

:040000 040000 265fa1882ee3430f74d59c9b38b6aca0858396d6 28a01a4941f99f7c266fd84f36cc27b933d426c1 M	net

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

On Wed, 2 Mar 2011 16:28:26 -0800
Stephen Hemminger <shemminger@vyatta.com> wrote:

> My PPTP VPN is broken with net-next tree, it works fine with 2.6.38-rc6
> but not in the next tree. Bisected the problem down to the following commit.
> 
> Debugging to see which address request is failing with the new code.
> 
> 9435eb1cf0b76b323019cebf8d16762a50a12a19 is the first bad commit
> commit 9435eb1cf0b76b323019cebf8d16762a50a12a19
> Author: David S. Miller <davem@davemloft.net>
> Date:   Fri Feb 18 12:43:09 2011 -0800
> 
>     ipv4: Implement __ip_dev_find using new interface address hash.
>     
>     Much quicker than going through the FIB tables.
>     
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> :040000 040000 265fa1882ee3430f74d59c9b38b6aca0858396d6 28a01a4941f99f7c266fd84f36cc27b933d426c1 M	net
> --
> 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

__ip_dev_find is being called with 10.250.0.105 and failing.
At the time the route tables are:

When VPN is up, the following is in the local table:

local 10.250.0.105 dev ppp0  proto kernel  scope host  src 10.250.0.105 

and it isn't getting matched...

In main table the difference is:
+10.255.254.0 dev ppp0  proto kernel  scope link  src 10.250.0.105 
-default via 192.168.1.1 dev eth0  proto static 

Re: [BUG] VPN broken in net-next

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 2 Mar 2011 16:41:14 -0800

> __ip_dev_find is being called with 10.250.0.105 and failing.

What is in the interface address lists at the time of the call?
Ie. "ip a l"?

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

The addresses (that matter) when VPN is up are:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether 00:23:54:91:08:c1 brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.11/24 brd 192.168.1.255 scope global eth0
    inet6 fe80::223:54ff:fe91:8c1/64 scope link 
       valid_lft forever preferred_lft forever
13: ppp0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1400 qdisc pfifo_fast state UNKNOWN qlen 3
    link/ppp 
    inet 10.250.0.105 peer 10.255.254.0/32 scope global ppp0

This is with working 2.6.37.2

Re: [BUG] VPN broken in net-next

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 2 Mar 2011 16:46:37 -0800

> The addresses (that matter) when VPN is up are:

I really need to know what addresses interfaces have the time of the
__ip_dev_find() call which, if I'm not mistaken, is before the VPN is
up.

Re: [BUG] VPN broken in net-next

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 02 Mar 2011 16:50:09 -0800 (PST)

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 2 Mar 2011 16:46:37 -0800
> 
>> The addresses (that matter) when VPN is up are:
> 
> I really need to know what addresses interfaces have the time of the
> __ip_dev_find() call which, if I'm not mistaken, is before the VPN is
> up.

Looking at pptp_connect(), it seems to be trying to do a route lookup
using the source address, before it registers the PPP channel and that
even gets brought up.

I suspect that was working by luck previously, and it was getting the
default route out perhaps another interface.  If that is the case,
removing the explicit "fl4_src" source address specification in the lookup
flow pptp_connect() uses should fix the problem.

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

On Wed, 02 Mar 2011 16:50:09 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 2 Mar 2011 16:46:37 -0800
> 
> > The addresses (that matter) when VPN is up are:
> 
> I really need to know what addresses interfaces have the time of the
> __ip_dev_find() call which, if I'm not mistaken, is before the VPN is
> up.


-- 
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast state DOWN qlen 1000
    link/ether 00:23:54:91:08:c2 brd ff:ff:ff:ff:ff:ff
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether 00:23:54:91:08:c1 brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.11/24 brd 192.168.1.255 scope global eth0
    inet6 fe80::223:54ff:fe91:8c1/64 scope link 
       valid_lft forever preferred_lft forever
 ... several offline interfaces ...
10: virbr0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN 
    link/ether 0a:2d:54:7c:09:96 brd ff:ff:ff:ff:ff:ff
    inet 192.168.100.1/24 brd 192.168.100.255 scope global virbr0
    inet6 fe80::82d:54ff:fe7c:996/64 scope link 
       valid_lft forever preferred_lft forever
11: virbr1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN 
    link/ether 46:2c:c7:fd:45:e5 brd ff:ff:ff:ff:ff:ff
    inet 192.168.99.1/24 brd 192.168.99.255 scope global virbr1
    inet6 fe80::442c:c7ff:fefd:45e5/64 scope link 
       valid_lft forever preferred_lft forever

Re: [BUG] VPN broken in net-next

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 2 Mar 2011 16:56:53 -0800

> On Wed, 02 Mar 2011 16:50:09 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Wed, 2 Mar 2011 16:46:37 -0800
>> 
>> > The addresses (that matter) when VPN is up are:
>> 
>> I really need to know what addresses interfaces have the time of the
>> __ip_dev_find() call which, if I'm not mistaken, is before the VPN is
>> up.
> 
> 
>     inet 127.0.0.1/8 scope host lo
>     inet 192.168.1.11/24 brd 192.168.1.255 scope global eth0
>     inet 192.168.100.1/24 brd 192.168.100.255 scope global virbr0
>     inet 192.168.99.1/24 brd 192.168.99.255 scope global virbr1

I see nothing providing 10.0.whatever that __ip_dev_find() is being
asked to resolve.

I think we were allowing the route lookup pptp is trying to do at
connect time erroneously, and it should elide the explicit source
address specification in the flow.

See my other email.

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

On Wed, 02 Mar 2011 17:03:46 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 2 Mar 2011 16:56:53 -0800
> 
> > On Wed, 02 Mar 2011 16:50:09 -0800 (PST)
> > David Miller <davem@davemloft.net> wrote:
> > 
> >> From: Stephen Hemminger <shemminger@vyatta.com>
> >> Date: Wed, 2 Mar 2011 16:46:37 -0800
> >> 
> >> > The addresses (that matter) when VPN is up are:
> >> 
> >> I really need to know what addresses interfaces have the time of the
> >> __ip_dev_find() call which, if I'm not mistaken, is before the VPN is
> >> up.
> > 
> > 
> >     inet 127.0.0.1/8 scope host lo
> >     inet 192.168.1.11/24 brd 192.168.1.255 scope global eth0
> >     inet 192.168.100.1/24 brd 192.168.100.255 scope global virbr0
> >     inet 192.168.99.1/24 brd 192.168.99.255 scope global virbr1
> 
> I see nothing providing 10.0.whatever that __ip_dev_find() is being
> asked to resolve.
> 
> I think we were allowing the route lookup pptp is trying to do at
> connect time erroneously, and it should elide the explicit source
> address specification in the flow.
> 

The VPN connection comes up the problem is that no packets pass over
it successfully.  The 10.X address is the other side of the VPN.

 I tried this, but it didn't work.

--- a/drivers/net/pptp.c	2011-03-02 17:01:55.353313682 -0800
+++ b/drivers/net/pptp.c	2011-03-02 17:02:05.381146980 -0800
@@ -473,7 +473,6 @@ static int pptp_connect(struct socket *s
 			.nl_u = {
 				.ip4_u = {
 					.daddr = opt->dst_addr.sin_addr.s_addr,
-					.saddr = opt->src_addr.sin_addr.s_addr,
 					.tos = RT_CONN_FLAGS(sk) } },
 			.proto = IPPROTO_GRE };
 		security_sk_classify_flow(sk, &fl);

Re: [BUG] VPN broken in net-next

[Julian Anastasov]

 	Hello,

On Wed, 2 Mar 2011, David Miller wrote:

>> I really need to know what addresses interfaces have the time of the
>> __ip_dev_find() call which, if I'm not mistaken, is before the VPN is
>> up.
>
> Looking at pptp_connect(), it seems to be trying to do a route lookup
> using the source address, before it registers the PPP channel and that
> even gets brought up.
>
> I suspect that was working by luck previously, and it was getting the
> default route out perhaps another interface.  If that is the case,
> removing the explicit "fl4_src" source address specification in the lookup
> flow pptp_connect() uses should fix the problem.

 	May be the problem is in inet_hash_insert(), it should
hash ifa_local, not ifa_address. May be they are equal for
the common case but not for peer addresses. In devinet_ioctl()
we can see they are equal initially:

ifa->ifa_address = ifa->ifa_local = sin->sin_addr.s_addr;

 	but later SIOCSIFDSTADDR can change ifa_address which
is destination address from the same prefix:

ifa->ifa_address = sin->sin_addr.s_addr;

 	fib_add_ifaddr() does more things but not for this
case with /32 mask, so the problem should be in using
ifa_address instead of ifa_local.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [BUG] VPN broken in net-next

[Julian Anastasov]

 	Hello,

On Thu, 3 Mar 2011, Julian Anastasov wrote:

> 	May be the problem is in inet_hash_insert(), it should
> hash ifa_local, not ifa_address. May be they are equal for

 	... and of course the new __ip_dev_find should use
ifa_local too.

> the common case but not for peer addresses. In devinet_ioctl()
> we can see they are equal initially:
>
> ifa->ifa_address = ifa->ifa_local = sin->sin_addr.s_addr;
>
> 	but later SIOCSIFDSTADDR can change ifa_address which
> is destination address from the same prefix:
>
> ifa->ifa_address = sin->sin_addr.s_addr;

 	While checking for ifa_address usage I see other
two places that look suspicious:

- inet_gifconf() exposes address from ifa_local but then
devinet_ioctl() matches by ifa_address in the
'if (tryaddrmatch)' block. I think, we should use ifa_local.

- IN_DEV_ARP_NOTIFY: announces ifa_address instead of ifa_local.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

>  	Hello,
> 
> On Thu, 3 Mar 2011, Julian Anastasov wrote:
> 
> > 	May be the problem is in inet_hash_insert(), it should
> > hash ifa_local, not ifa_address. May be they are equal for  
> 
>  	... and of course the new __ip_dev_find should use
> ifa_local too.
> 
> > the common case but not for peer addresses. In devinet_ioctl()
> > we can see they are equal initially:
> >
> > ifa->ifa_address = ifa->ifa_local = sin->sin_addr.s_addr;
> >
> > 	but later SIOCSIFDSTADDR can change ifa_address which
> > is destination address from the same prefix:
> >
> > ifa->ifa_address = sin->sin_addr.s_addr;  
> 
>  	While checking for ifa_address usage I see other
> two places that look suspicious:
> 
> - inet_gifconf() exposes address from ifa_local but then
> devinet_ioctl() matches by ifa_address in the
> 'if (tryaddrmatch)' block. I think, we should use ifa_local.
> 
> - IN_DEV_ARP_NOTIFY: announces ifa_address instead of ifa_local.

Julian you are on the right track with the ifa_local being the issue.

1. Bring up VPN 
   (no errors)

2. Ping one address gets connect error

3. Instrumentation triggers. I added code so if __ip_dev_find failed,
   it walked the hash table.

net=ffffffff81a12d80

[  393.224228] __ip_dev_find(ffffffff81a12d80, 10.250.0.104) hash=108 failed
[  393.224232] 138: ffffffff81a12d80 ifa_addr=127.0.0.1 ifa_local=127.0.0.1
[  393.224236] 150: ffffffff81a12d80 ifa_addr=192.168.1.11 ifa_local=192.168.1.11
[  393.224239] 249: ffffffff81a12d80 ifa_addr=192.168.100.1 ifa_local=192.168.100.1
[  393.224242] 254: ffffffff81a12d80 ifa_addr=192.168.99.1 ifa_local=192.168.99.1
[  393.224245] 255: ffffffff81a12d80 ifa_addr=10.255.254.0 ifa_local=10.250.0.10

Re: [BUG] VPN broken in net-next

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Thu, 3 Mar 2011 15:09:22 +0200 (EET)

> On Thu, 3 Mar 2011, Julian Anastasov wrote:
> 
>> 	May be the problem is in inet_hash_insert(), it should
>> hash ifa_local, not ifa_address. May be they are equal for
> 
> 	... and of course the new __ip_dev_find should use
> ifa_local too.

Thanks for looking into this Julian.  I will look at the other
cases you found later.

Stephen, is this sufficient to fix your problem?  I suspect it is
not because fib_add_addr() adds prefixes with RTN_LOCAL to the
local routing table too :-/

I suspect that even if we need to handle prefixes, we can still use
the hash for optimistic lookup, and fallback to a local table FIB
inspection if that fails.

--------------------
ipv4: Fix __ip_dev_find() to use ifa_local instead of ifa_address.

Reported-by: Stephen Hemminger <shemminger@vyatta.com>
Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 9038928..ff53860 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -111,7 +111,7 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr)
 
 static void inet_hash_insert(struct net *net, struct in_ifaddr *ifa)
 {
-	unsigned int hash = inet_addr_hash(net, ifa->ifa_address);
+	unsigned int hash = inet_addr_hash(net, ifa->ifa_local);
 
 	spin_lock(&inet_addr_hash_lock);
 	hlist_add_head_rcu(&ifa->hash, &inet_addr_lst[hash]);
@@ -146,7 +146,7 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 
 		if (!net_eq(dev_net(dev), net))
 			continue;
-		if (ifa->ifa_address == addr) {
+		if (ifa->ifa_local == addr) {
 			result = dev;
 			break;
 		}

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

On Thu, 03 Mar 2011 11:23:28 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Julian Anastasov <ja@ssi.bg>
> Date: Thu, 3 Mar 2011 15:09:22 +0200 (EET)
> 
> > On Thu, 3 Mar 2011, Julian Anastasov wrote:
> > 
> >> 	May be the problem is in inet_hash_insert(), it should
> >> hash ifa_local, not ifa_address. May be they are equal for
> > 
> > 	... and of course the new __ip_dev_find should use
> > ifa_local too.
> 
> Thanks for looking into this Julian.  I will look at the other
> cases you found later.
> 
> Stephen, is this sufficient to fix your problem?  I suspect it is
> not because fib_add_addr() adds prefixes with RTN_LOCAL to the
> local routing table too :-/
> 
> I suspect that even if we need to handle prefixes, we can still use
> the hash for optimistic lookup, and fallback to a local table FIB
> inspection if that fails.
> 
> --------------------
> ipv4: Fix __ip_dev_find() to use ifa_local instead of ifa_address.
> 
> Reported-by: Stephen Hemminger <shemminger@vyatta.com>
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 

VPN works now with this patch on net-next.

Re: [BUG] VPN broken in net-next

[Julian Anastasov]

 	Hello,

On Thu, 3 Mar 2011, David Miller wrote:

> I suspect that even if we need to handle prefixes, we can still use
> the hash for optimistic lookup, and fallback to a local table FIB
> inspection if that fails.

 	Yes, as ip_route_output_slow uses __ip_dev_find for
fl4_src there should be some kind of fallback to local table,
so that traffic from 127.0.0.2 to 127.0.0.3 or other local
subnets on loopback can work. Another option is to use
inet_addr_onlink but I suspect people can add many addresses
on loopback: inet_addr_onlink(loopback_indev, addr, 0)

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [BUG] VPN broken in net-next

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Thu, 3 Mar 2011 15:09:22 +0200 (EET)

> 	While checking for ifa_address usage I see other
> two places that look suspicious:
> 
> - inet_gifconf() exposes address from ifa_local but then
> devinet_ioctl() matches by ifa_address in the
> 'if (tryaddrmatch)' block. I think, we should use ifa_local.
> 
> - IN_DEV_ARP_NOTIFY: announces ifa_address instead of ifa_local.

Thanks Julian, I'll push the following into net-2.6:

--------------------
ipv4: Fix erroneous uses of ifa_address.

In usual cases ifa_address == ifa_local, but in the case where
SIOCSIFDSTADDR sets the destination address on a point-to-point
link, ifa_address gets set to that destination address.

Therefore we should use ifa_local when we want the local interface
address.

There were two cases where the selection was done incorrectly:

1) When devinet_ioctl() does matching, it checks ifa_address even
   though gifconf correct reported ifa_local to the user

2) IN_DEV_ARP_NOTIFY handling sends a gratuitous ARP using
   ifa_address instead of ifa_local.

Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/devinet.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index df4616f..036652c 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -670,7 +670,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 			     ifap = &ifa->ifa_next) {
 				if (!strcmp(ifr.ifr_name, ifa->ifa_label) &&
 				    sin_orig.sin_addr.s_addr ==
-							ifa->ifa_address) {
+							ifa->ifa_local) {
 					break; /* found */
 				}
 			}
@@ -1040,8 +1040,8 @@ static void inetdev_send_gratuitous_arp(struct net_device *dev,
 		return;
 
 	arp_send(ARPOP_REQUEST, ETH_P_ARP,
-		 ifa->ifa_address, dev,
-		 ifa->ifa_address, NULL,
+		 ifa->ifa_local, dev,
+		 ifa->ifa_local, NULL,
 		 dev->dev_addr, NULL);
 }
 
-- 
1.7.4.1

Re: [BUG] VPN broken in net-next

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Fri, 4 Mar 2011 10:39:55 +0200 (EET)

> On Thu, 3 Mar 2011, David Miller wrote:
> 
>> I suspect that even if we need to handle prefixes, we can still use
>> the hash for optimistic lookup, and fallback to a local table FIB
>> inspection if that fails.
> 
> 	Yes, as ip_route_output_slow uses __ip_dev_find for
> fl4_src there should be some kind of fallback to local table,
> so that traffic from 127.0.0.2 to 127.0.0.3 or other local
> subnets on loopback can work. Another option is to use
> inet_addr_onlink but I suspect people can add many addresses
> on loopback: inet_addr_onlink(loopback_indev, addr, 0)

I just got back to this, sorry for taking so long :-)

Here is the patch I've come up with and will commit to
net-2.6, thanks!

--------------------
ipv4: Fallback to FIB local table in __ip_dev_find().

In commit 9435eb1cf0b76b323019cebf8d16762a50a12a19
("ipv4: Implement __ip_dev_find using new interface address hash.")
we reimplemented __ip_dev_find() so that it doesn't have to
do a full FIB table lookup.

Instead, it consults a hash table of addresses configured to
interfaces.

This works identically to the old code in all except one case,
and that is for loopback subnets.

The old code would match the loopback device for any IP address
that falls within a subnet configured to the loopback device.

Handle this corner case by doing the FIB lookup.

We could implement this via inet_addr_onlink() but:

1) Someone could configure many addresses to loopback and
   inet_addr_onlink() is a simple list traversal.

2) We know the old code works.

Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/devinet.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d5a4553..5345b0b 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -64,6 +64,8 @@
 #include <net/rtnetlink.h>
 #include <net/net_namespace.h>
 
+#include "fib_lookup.h"
+
 static struct ipv4_devconf ipv4_devconf = {
 	.data = {
 		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
@@ -151,6 +153,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 			break;
 		}
 	}
+	if (!result) {
+		struct flowi4 fl4 = { .daddr = addr };
+		struct fib_result res = { 0 };
+		struct fib_table *local;
+
+		/* Fallback to FIB local table so that communication
+		 * over loopback subnets work.
+		 */
+		local = fib_get_table(net, RT_TABLE_LOCAL);
+		if (local &&
+		    !fib_table_lookup(local, &fl4, &res, FIB_LOOKUP_NOREF) &&
+		    res.type == RTN_LOCAL)
+			result = FIB_RES_DEV(res);
+	}
 	if (result && devref)
 		dev_hold(result);
 	rcu_read_unlock();
-- 
1.7.4.1

Re: [BUG] VPN broken in net-next

[Julian Anastasov]

 	Hello,

On Tue, 22 Mar 2011, David Miller wrote:

> I just got back to this, sorry for taking so long :-)
>
> Here is the patch I've come up with and will commit to
> net-2.6, thanks!
>
> --------------------
> ipv4: Fallback to FIB local table in __ip_dev_find().
>
> In commit 9435eb1cf0b76b323019cebf8d16762a50a12a19
> ("ipv4: Implement __ip_dev_find using new interface address hash.")
> we reimplemented __ip_dev_find() so that it doesn't have to
> do a full FIB table lookup.
>
> Instead, it consults a hash table of addresses configured to
> interfaces.
>
> This works identically to the old code in all except one case,
> and that is for loopback subnets.
>
> The old code would match the loopback device for any IP address
> that falls within a subnet configured to the loopback device.
>
> Handle this corner case by doing the FIB lookup.
>
> We could implement this via inet_addr_onlink() but:
>
> 1) Someone could configure many addresses to loopback and
>   inet_addr_onlink() is a simple list traversal.
>
> 2) We know the old code works.
>
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> net/ipv4/devinet.c |   16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index d5a4553..5345b0b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -64,6 +64,8 @@
> #include <net/rtnetlink.h>
> #include <net/net_namespace.h>
>
> +#include "fib_lookup.h"
> +
> static struct ipv4_devconf ipv4_devconf = {
> 	.data = {
> 		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
> @@ -151,6 +153,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
> 			break;
> 		}
> 	}
> +	if (!result) {
> +		struct flowi4 fl4 = { .daddr = addr };
> +		struct fib_result res = { 0 };
> +		struct fib_table *local;
> +
> +		/* Fallback to FIB local table so that communication
> +		 * over loopback subnets work.
> +		 */
> +		local = fib_get_table(net, RT_TABLE_LOCAL);
> +		if (local &&
> +		    !fib_table_lookup(local, &fl4, &res, FIB_LOOKUP_NOREF) &&
> +		    res.type == RTN_LOCAL)
> +			result = FIB_RES_DEV(res);
> +	}
> 	if (result && devref)
> 		dev_hold(result);
> 	rcu_read_unlock();
> -- 
> 1.7.4.1

 	Not tested but looks ok for me.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [BUG] VPN broken in net-next

[Stephen Hemminger]

On Tue, 22 Mar 2011 21:56:55 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Julian Anastasov <ja@ssi.bg>
> Date: Fri, 4 Mar 2011 10:39:55 +0200 (EET)
> 
> > On Thu, 3 Mar 2011, David Miller wrote:
> > 
> >> I suspect that even if we need to handle prefixes, we can still use
> >> the hash for optimistic lookup, and fallback to a local table FIB
> >> inspection if that fails.
> > 
> > 	Yes, as ip_route_output_slow uses __ip_dev_find for
> > fl4_src there should be some kind of fallback to local table,
> > so that traffic from 127.0.0.2 to 127.0.0.3 or other local
> > subnets on loopback can work. Another option is to use
> > inet_addr_onlink but I suspect people can add many addresses
> > on loopback: inet_addr_onlink(loopback_indev, addr, 0)
> 
> I just got back to this, sorry for taking so long :-)
> 
> Here is the patch I've come up with and will commit to
> net-2.6, thanks!
> 
> --------------------
> ipv4: Fallback to FIB local table in __ip_dev_find().
> 
> In commit 9435eb1cf0b76b323019cebf8d16762a50a12a19
> ("ipv4: Implement __ip_dev_find using new interface address hash.")
> we reimplemented __ip_dev_find() so that it doesn't have to
> do a full FIB table lookup.
> 
> Instead, it consults a hash table of addresses configured to
> interfaces.
> 
> This works identically to the old code in all except one case,
> and that is for loopback subnets.
> 
> The old code would match the loopback device for any IP address
> that falls within a subnet configured to the loopback device.
> 
> Handle this corner case by doing the FIB lookup.
> 
> We could implement this via inet_addr_onlink() but:
> 
> 1) Someone could configure many addresses to loopback and
>    inet_addr_onlink() is a simple list traversal.
> 
> 2) We know the old code works.
> 
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/ipv4/devinet.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index d5a4553..5345b0b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -64,6 +64,8 @@
>  #include <net/rtnetlink.h>
>  #include <net/net_namespace.h>
>  
> +#include "fib_lookup.h"
> +
>  static struct ipv4_devconf ipv4_devconf = {
>  	.data = {
>  		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
> @@ -151,6 +153,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
>  			break;
>  		}
>  	}
> +	if (!result) {
> +		struct flowi4 fl4 = { .daddr = addr };
> +		struct fib_result res = { 0 };
> +		struct fib_table *local;
> +
> +		/* Fallback to FIB local table so that communication
> +		 * over loopback subnets work.
> +		 */
> +		local = fib_get_table(net, RT_TABLE_LOCAL);
> +		if (local &&
> +		    !fib_table_lookup(local, &fl4, &res, FIB_LOOKUP_NOREF) &&
> +		    res.type == RTN_LOCAL)
> +			result = FIB_RES_DEV(res);
> +	}
>  	if (result && devref)
>  		dev_hold(result);
>  	rcu_read_unlock();

Acked-by: Stephen Hemminger <shemminger@vyatta.com>



--