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

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.

[PATCHv2] 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>
---
v2: simplify patch using julian anastasov's suggestion.

 net/ipv4/route.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 234a43e..b09fda8 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.0

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

[David Miller]

From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
Date: Mon, 15 Sep 2014 12:28:13 +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>
> ---
> v2: simplify patch using julian anastasov's suggestion.

This also disables caching for the cases of a simple fib lookup failure.

Handle cached route invalidation the way it's meant to be, by bumping
the rt_genid.

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 214882e..aa4e63c 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1965,6 +1965,8 @@ static void inet_forward_change(struct net *net)
 		}
 		rcu_read_unlock();
 	}
+
+	rt_cache_flush(net);
 }
 
 static int devinet_conf_ifindex(struct net *net, struct ipv4_devconf *cnf)

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

[Nicolas Cavallari]

On 16/09/2014 20:54, David Miller wrote:
> From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> Date: Mon, 15 Sep 2014 12:28:13 +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>
>> ---
>> v2: simplify patch using julian anastasov's suggestion.
> 
> This also disables caching for the cases of a simple fib lookup failure.

Caching is already not done on a fib lookup failure. On which fib_info
would you cache anyway ? res.fi is already NULL in this case.

> Handle cached route invalidation the way it's meant to be, by bumping
> the rt_genid.
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 214882e..aa4e63c 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1965,6 +1965,8 @@ static void inet_forward_change(struct net *net)
>  		}
>  		rcu_read_unlock();
>  	}
> +
> +	rt_cache_flush(net);
>  }
>  
>  static int devinet_conf_ifindex(struct net *net, struct ipv4_devconf *cnf)

This doesn't solve the problem. Flushing the cache only changes how the
bug manifests:

ip route flush cache
ip route get 10.0.0.1 from 10.0.0.2 iif A
# unreachable (because forwarding on A is disabled). This is cached.
ip route get 10.0.0.1 from 10.0.0.2 iif B
# unreachable (at first fib_lookup finds it reachable and forwarding on
B is enabled. And then there is a valid cache entry, which is reused
blindly even if it says "unreachable").

ip route flush cache
ip route get 10.0.0.1 from 10.0.0.2 iif B
# reachable (because forwarding on B is enabled).  This is cached
ip route get 10.0.0.1 from 10.0.0.2 iif A
# reachable (forwarding on A is disabled, so it is unreachable
initially.  But there is a valid cache entry, which says "reachable",
which is reused)

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

[Julian Anastasov]

	Hello,

On Mon, 15 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
> 
> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>

	Looks good to me,

Reviewed-by: Julian Anastasov <ja@ssi.bg>

	Still, I fail to see how the compiler optimizes
the jump, 'goto local_input' still jumps to res.fi check.
I tried even likely() after 'local_input:" checks for
res.fi and !itag but the 'if (res.fi) {' block is still
moved below and reached with jnz. I expected likely() to
prefer the res.fi != NULL path.

> ---
> v2: simplify patch using julian anastasov's suggestion.
> 
>  net/ipv4/route.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 234a43e..b09fda8 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.0

Regards

--
Julian Anastasov <ja@ssi.bg>

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

[Nicolas Cavallari]

On 18/09/2014 07:17, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 15 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
>>
>> Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
> 
> 	Looks good to me,
> 
> Reviewed-by: Julian Anastasov <ja@ssi.bg>
> 
> 	Still, I fail to see how the compiler optimizes
> the jump, 'goto local_input' still jumps to res.fi check.
> I tried even likely() after 'local_input:" checks for
> res.fi and !itag but the 'if (res.fi) {' block is still
> moved below and reached with jnz. I expected likely() to
> prefer the res.fi != NULL path.

Different compiler/arch/options perhaps ?  I'm using Debian's GCC
4.9.1-11 on amd64 with a defconfig + CONFIG_DEBUG_INFO. It does this:

.LBE2727:
        .loc 1 1800 0
        movb    $7, -102(%rbp)  #, res.type
        .loc 1 1801 0
        movq    $0, -96(%rbp)   #, res.fi
        xorl    %ecx, %ecx      # D.60248
        .loc 1 1653 0
        xorl    %r12d, %r12d    # flags
.LVL737:
        .loc 1 1749 0
        xorl    %r15d, %r15d    # do_cache
.LVL738:
        jmp     .L784   #

where .L784 is :
        .loc 1 1762 0
        movl    324(%r11), %edi # in_dev_74->cnf.data,
        xorl    %esi, %esi      # D.60250
.LVL601:
        movq    %r10, -128(%rbp)        # skb, %sfp
        testl   %edi, %edi      #
        movq    264(%r14), %rdi # _75->loopback_dev, _75->loopback_dev
        setne   %sil    #, D.60250
        xorl    %edx, %edx      #
.LVL602:
        call    rt_dst_alloc    #

As for the res.fi and itag check in local_input, it jumps only when the
test fails.

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

[Julian Anastasov]

	Hello,

On Thu, 18 Sep 2014, Nicolas Cavallari wrote:

> On 18/09/2014 07:17, Julian Anastasov wrote:
> > 
> > 	Still, I fail to see how the compiler optimizes
> > the jump, 'goto local_input' still jumps to res.fi check.
> > I tried even likely() after 'local_input:" checks for
> > res.fi and !itag but the 'if (res.fi) {' block is still
> > moved below and reached with jnz. I expected likely() to
> > prefer the res.fi != NULL path.
> 
> Different compiler/arch/options perhaps ?  I'm using Debian's GCC
> 4.9.1-11 on amd64 with a defconfig + CONFIG_DEBUG_INFO. It does this:

	I see, new compilers can do better, I'm on x86 32-bit:

# rpm -qi gcc | grep -e Version -e Date
Version     : 4.8.3
Install Date: Sat 06 Sep 2014 05:56:15 PM EEST
Build Date  : Tue 24 Jun 2014 10:18:10 PM EEST

# make net/ipv4/route.s

---> local_input:
.L1031:
        movl    -56(%ebp), %edx # res.fi, D.55132
        testl   %edx, %edx      # D.55132

---> jump for likely(res.fi != NULL):

        jne     .L1219  #,
.L1095:
        movb    $0, -88(%ebp)   #, %sfp
.L1059:
        movzbl  -88(%ebp), %eax # %sfp, D.55118
        xorl    %edx, %edx      # D.55127
        cmpl    $0, 236(%edi)   #, in_dev_129->cnf.data
        pushl   %eax    # D.55118
        movl    init_net+172, %eax      # init_net.loopback_dev,
        setne   %dl     #, D.55127
        xorl    %ecx, %ecx      #
        call    rt_dst_alloc    #

...

---> no_route:
.L1027:
        movb    $7, -62(%ebp)   #, res.type
        movl    $0, -56(%ebp)   #, res.fi
#APP
# 1773 "net/ipv4/route.c" 1
        incl rt_cache_stat+8    # rt_cache_stat.in_no_route
# 0 "" 2
#NO_APP
.L1214:
        movl    $0, -92(%ebp)   #, %sfp

---> jump to local_input:

        jmp     .L1031  #

Regards

--
Julian Anastasov <ja@ssi.bg>

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

[Nicolas Cavallari]

On 16/09/2014 21:52, Nicolas Cavallari wrote:
> On 16/09/2014 20:54, David Miller wrote:
>> From: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
>> Date: Mon, 15 Sep 2014 12:28:13 +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>
>>> ---
>>> v2: simplify patch using julian anastasov's suggestion.
>>
>> This also disables caching for the cases of a simple fib lookup failure.
> 
> Caching is already not done on a fib lookup failure. On which fib_info
> would you cache anyway ? res.fi is already NULL in this case.
> 
>> Handle cached route invalidation the way it's meant to be, by bumping
>> the rt_genid.
>>
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index 214882e..aa4e63c 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -1965,6 +1965,8 @@ static void inet_forward_change(struct net *net)
>>  		}
>>  		rcu_read_unlock();
>>  	}
>> +
>> +	rt_cache_flush(net);
>>  }
>>  
>>  static int devinet_conf_ifindex(struct net *net, struct ipv4_devconf *cnf)
> 
> This doesn't solve the problem. Flushing the cache only changes how the
> bug manifests:
> 
> ip route flush cache
> ip route get 10.0.0.1 from 10.0.0.2 iif A
> # unreachable (because forwarding on A is disabled). This is cached.
> ip route get 10.0.0.1 from 10.0.0.2 iif B
> # unreachable (at first fib_lookup finds it reachable and forwarding on
> B is enabled. And then there is a valid cache entry, which is reused
> blindly even if it says "unreachable").
> 
> ip route flush cache
> ip route get 10.0.0.1 from 10.0.0.2 iif B
> # reachable (because forwarding on B is enabled).  This is cached
> ip route get 10.0.0.1 from 10.0.0.2 iif A
> # reachable (forwarding on A is disabled, so it is unreachable
> initially.  But there is a valid cache entry, which says "reachable",
> which is reused)

Any news on this patch ? Is there anything I can do so that this bug
is finally fixed ?

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

[David Miller]

From: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
Date: Tue, 23 Sep 2014 10:34:08 +0200

> Any news on this patch ? Is there anything I can do so that this bug
> is finally fixed ?

Getting back to this is on my TODO list, I haven't forgotten about
it, thanks :)

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.

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>

[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