About caching unreachable routes when not forwarding

About caching unreachable routes when not forwarding

[Nicolas Cavallari]

I have some weird routing problem on a seemingly simple setup on a
3.12-3.16 kernel and I suspect that net-next is also affected.

I have two interfaces: A, B, with forwarding disabled on A and
enabled on B.  I also have another interface V.

Both interfaces receive a packet that must be routed though V:
- A receives the packet first, do some fib lookup and cache a "route
unreachable" rt_iif=0 because forwarding is disabled.
- B receives the packet, do some fib lookup (and reverse path filtering, whatever) and succeeds, then finds the recently cached rth and use it. This cached rth is of course "route unreachable" and forwarding is broken.

This simple script in a network namespace is enough to show it:

#!/bin/sh -eu
setup_iface () {
        ip link add name "$1" type dummy
        ip link set dev "$1" up
        [ -n "${2:-}" ] && ip route add "$2" dev "$1"
        ip4conf="/proc/sys/net/ipv4/conf/$1"
        echo "$3" > "$ip4conf/forwarding"
        echo "$4" > "$ip4conf/rp_filter"
}
ip link set lo up
setup_iface "A" ""              0       0
setup_iface "B" "10.0.0.2/32"   1       1
setup_iface "C" "10.0.0.1/32"   1       0
set -x +e
ip route get 10.0.0.1 from 10.0.0.2 iif A
# unreachable
ip route get 10.0.0.1 from 10.0.0.2 iif B
# unreachable, but should be reachable

ip route flush cache
ip route get 10.0.0.1 from 10.0.0.2 iif B
# reachable
ip route get 10.0.0.1 from 10.0.0.2 iif A
# reachable, but should be unreachable

I would suggest that we shouldn't cache unreachable results due to
forwarding being disabled, but I'm not aware of all use-case of this
code nor what should actually be cached.  The following patch fix
my use case.  I don't know if it breaks others.

[RFC] ipv4: Do not cache routing failures due to disabled forwarding.

[Nicolas Cavallari]

If we cache them, the kernel will reuse them, independently of
whether forwarding is enabled or not.  Which means that if forwarding is
disabled on the input interface where the first routing request comes
from, then that unreachable result will be cached and reused for
other interfaces, even if forwarding is enabled on them.

This can be verified with two interfaces A and B and an output interface
C, where B has forwarding enabled, but not A and trying
ip route get $dst iif A from $src && ip route get $dst iif B from $src

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
based on net-next, but not really tested on top of it.

 net/ipv4/route.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 234a43e..b537997 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1655,7 +1655,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	struct rtable	*rth;
 	int		err = -EINVAL;
 	struct net    *net = dev_net(dev);
-	bool do_cache;
+	bool do_cache = true;
 
 	/* IP on this device is disabled. */
 
@@ -1723,6 +1723,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	if (!IN_DEV_FORWARD(in_dev)) {
 		err = -EHOSTUNREACH;
+		do_cache = false;
 		goto no_route;
 	}
 	if (res.type != RTN_UNICAST)
@@ -1746,16 +1747,14 @@ brd_input:
 	RT_CACHE_STAT_INC(in_brd);
 
 local_input:
-	do_cache = false;
-	if (res.fi) {
-		if (!itag) {
-			rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
-			if (rt_cache_valid(rth)) {
-				skb_dst_set_noref(skb, &rth->dst);
-				err = 0;
-				goto out;
-			}
-			do_cache = true;
+	if (!res.fi || itag) {
+		do_cache = false;
+	} else if (do_cache) {
+		rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
+		if (rt_cache_valid(rth)) {
+			skb_dst_set_noref(skb, &rth->dst);
+			err = 0;
+			goto out;
 		}
 	}
 
-- 
2.1.0

Re: [RFC] ipv4: Do not cache routing failures due to disabled forwarding.

[Julian Anastasov]

	Hello,

On Fri, 12 Sep 2014, Nicolas Cavallari wrote:

> If we cache them, the kernel will reuse them, independently of
> whether forwarding is enabled or not.  Which means that if forwarding is
> disabled on the input interface where the first routing request comes
> from, then that unreachable result will be cached and reused for
> other interfaces, even if forwarding is enabled on them.
> 
> This can be verified with two interfaces A and B and an output interface
> C, where B has forwarding enabled, but not A and trying
> ip route get $dst iif A from $src && ip route get $dst iif B from $src

	Correct. While failed fib_lookup() does not set
res.fi in net/ipv4/fib_trie.c:check_leaf(), on fib_lookup()
success we have res.fi != NULL and it remains for the
!IN_DEV_FORWARD case (the 2nd 'goto no_route').

> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
> based on net-next, but not really tested on top of it.
> 
>  net/ipv4/route.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 234a43e..b537997 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1655,7 +1655,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>  	struct rtable	*rth;
>  	int		err = -EINVAL;
>  	struct net    *net = dev_net(dev);
> -	bool do_cache;
> +	bool do_cache = true;
>  
>  	/* IP on this device is disabled. */
>  
> @@ -1723,6 +1723,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>  
>  	if (!IN_DEV_FORWARD(in_dev)) {
>  		err = -EHOSTUNREACH;
> +		do_cache = false;
>  		goto no_route;
>  	}
>  	if (res.type != RTN_UNICAST)
> @@ -1746,16 +1747,14 @@ brd_input:
>  	RT_CACHE_STAT_INC(in_brd);
>  
>  local_input:
> -	do_cache = false;
> -	if (res.fi) {
> -		if (!itag) {
> -			rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
> -			if (rt_cache_valid(rth)) {
> -				skb_dst_set_noref(skb, &rth->dst);
> -				err = 0;
> -				goto out;
> -			}
> -			do_cache = true;
> +	if (!res.fi || itag) {
> +		do_cache = false;
> +	} else if (do_cache) {
> +		rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
> +		if (rt_cache_valid(rth)) {
> +			skb_dst_set_noref(skb, &rth->dst);
> +			err = 0;
> +			goto out;
>  		}
>  	}
>  
> -- 
> 2.1.0

	Two alternatives are possible:

1. set res.fi = NULL after 'no_route:' label

or better

2. set do_cache = false after 'no_route:' label,
then instead of 'goto local_input;' jump to a new
label 'create_rt:' just before rt_dst_alloc.

	Not sure, they may generate less code in the fast path.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [RFC] ipv4: Do not cache routing failures due to disabled forwarding.

[David Miller]

From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
Date: Fri, 12 Sep 2014 16:14:20 +0200

> If we cache them, the kernel will reuse them, independently of
> whether forwarding is enabled or not.  Which means that if forwarding is
> disabled on the input interface where the first routing request comes
> from, then that unreachable result will be cached and reused for
> other interfaces, even if forwarding is enabled on them.
> 
> This can be verified with two interfaces A and B and an output interface
> C, where B has forwarding enabled, but not A and trying
> ip route get $dst iif A from $src && ip route get $dst iif B from $src
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> ---
> based on net-next, but not really tested on top of it.

Sorry Nicolas, this seems to have fallen on the floor.  Could you please
resubmit your most uptodate version of this patch so I can apply it?

Thanks.

[PATCH RESEND v2] ipv4: Do not cache routing failures due to disabled forwarding.

[Nicolas Cavallari]

If we cache them, the kernel will reuse them, independently of
whether forwarding is enabled or not.  Which means that if forwarding is
disabled on the input interface where the first routing request comes
from, then that unreachable result will be cached and reused for
other interfaces, even if forwarding is enabled on them.  The opposite
is also true.

This can be verified with two interfaces A and B and an output interface
C, where B has forwarding enabled, but not A and trying
ip route get $dst iif A from $src && ip route get $dst iif B from $src

Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
Reviewed-by: Julian Anastasov <ja@ssi.bg>
---
> Sorry Nicolas, this seems to have fallen on the floor.  Could you please
> resubmit your most uptodate version of this patch so I can apply it?

Here you are.

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 2d4ae46..6a2155b 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1798,6 +1798,7 @@ local_input:
 no_route:
 	RT_CACHE_STAT_INC(in_no_route);
 	res.type = RTN_UNREACHABLE;
+	res.fi = NULL;
 	goto local_input;
 
 	/*
-- 
2.1.1

Re: [PATCH RESEND v2] ipv4: Do not cache routing failures due to disabled forwarding.

[David Miller]

From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
Date: Thu, 30 Oct 2014 10:09:53 +0100

> If we cache them, the kernel will reuse them, independently of
> whether forwarding is enabled or not.  Which means that if forwarding is
> disabled on the input interface where the first routing request comes
> from, then that unreachable result will be cached and reused for
> other interfaces, even if forwarding is enabled on them.  The opposite
> is also true.
> 
> This can be verified with two interfaces A and B and an output interface
> C, where B has forwarding enabled, but not A and trying
> ip route get $dst iif A from $src && ip route get $dst iif B from $src
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> Reviewed-by: Julian Anastasov <ja@ssi.bg>

Applied and queued up for -stable, thanks.

Re: [RFC] ipv4: Do not cache routing failures due to disabled forwarding.

[Nicolas Cavallari]

[Resending to netdev instead of linux-netdev, sorry again.]

For whatever reason, I didn't receive your reply and I'm not subscribed
to the list but I saw it on archives.

> 	Two alternatives are possible:
> 
> 1. set res.fi = NULL after 'no_route:' label
> 
> or better
> 
> 2. set do_cache = false after 'no_route:' label,
> then instead of 'goto local_input;' jump to a new
> label 'create_rt:' just before rt_dst_alloc.
> 
> 	Not sure, they may generate less code in the fast path.

If I implement the first alternative, GCC will optimize it to the
second. And it does not do that same optimization with my patch...

Will submit alternative 1 if there are no further issues/comments.