diff mbox

[v2] net: Fix behaviour of unreachable, blackhole and prohibit routes

Message ID 1441271331.3360.36.camel@redhat.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Nikola Forró Sept. 3, 2015, 9:08 a.m. UTC
Man page of ip-route(8) says following about route types:

  unreachable - these destinations are unreachable.  Packets are dis‐
  carded and the ICMP message host unreachable is generated.  The local
  senders get an EHOSTUNREACH error.

  blackhole - these destinations are unreachable.  Packets are dis‐
  carded silently.  The local senders get an EINVAL error.

  prohibit - these destinations are unreachable.  Packets are discarded
  and the ICMP message communication administratively prohibited is
  generated.  The local senders get an EACCES error.

In the inet6 address family, this was correct, except the local senders
got ENETUNREACH error instead of EHOSTUNREACH in case of unreachable route.
In the inet address family, all three route types generated ICMP message
net unreachable, and the local senders got ENETUNREACH error.

In both address families all three route types now behave consistently
with documentation.

Signed-off-by: Nikola Forró <nforro@redhat.com>
---
 include/net/ip_fib.h | 21 ++++++++++++++++-----
 net/ipv4/route.c     |  6 ++++--
 net/ipv6/route.c     |  4 +++-
 3 files changed, 23 insertions(+), 8 deletions(-)

Comments

Florian Westphal Sept. 3, 2015, 9:13 a.m. UTC | #1
Nikola Forró <nforro@redhat.com> wrote:
> Man page of ip-route(8) says following about route types:
> 
>   unreachable - these destinations are unreachable.  Packets are dis‐
>   carded and the ICMP message host unreachable is generated.  The local
>   senders get an EHOSTUNREACH error.
> 
>   blackhole - these destinations are unreachable.  Packets are dis‐
>   carded silently.  The local senders get an EINVAL error.
> 
>   prohibit - these destinations are unreachable.  Packets are discarded
>   and the ICMP message communication administratively prohibited is
>   generated.  The local senders get an EACCES error.
> 
> In the inet6 address family, this was correct, except the local senders
> got ENETUNREACH error instead of EHOSTUNREACH in case of unreachable route.
> In the inet address family, all three route types generated ICMP message
> net unreachable, and the local senders got ENETUNREACH error.
> 
> In both address families all three route types now behave consistently
> with documentation.
> 
> Signed-off-by: Nikola Forró <nforro@redhat.com>
> ---
>  include/net/ip_fib.h | 21 ++++++++++++++++-----
>  net/ipv4/route.c     |  6 ++++--
>  net/ipv6/route.c     |  4 +++-
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 5fa643b..cf025107 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -233,8 +233,10 @@ static inline int fib_lookup(struct net *net, const struct flowi4 *flp,
>  	rcu_read_lock();
>  
>  	tb = fib_get_table(net, RT_TABLE_MAIN);
> -	if (tb && !fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF))
> -		err = 0;
> +	if (tb)
> +		err = fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF);
> +		if (err == -EAGAIN)
> +			err = -ENETUNREACH;

Missing { } ?
--
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
Nikola Forró Sept. 3, 2015, 10:01 a.m. UTC | #2
Florian Westphal wrote:

> Missing { } ?
> 
I should really pay more attention to what I'm submitting.
Thanks Florian.

--
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 Sept. 3, 2015, 5:29 p.m. UTC | #3
From: Nikola Forró <nforro@redhat.com>
Date: Thu, 03 Sep 2015 11:08:51 +0200

> @@ -233,8 +233,10 @@ static inline int fib_lookup(struct net *net, const struct flowi4 *flp,
>  	rcu_read_lock();
>  
>  	tb = fib_get_table(net, RT_TABLE_MAIN);
> -	if (tb && !fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF))
> -		err = 0;
> +	if (tb)
> +		err = fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF);
> +		if (err == -EAGAIN)
> +			err = -ENETUNREACH;

You didn't test this.
--
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
Alexander H Duyck Sept. 3, 2015, 10:22 p.m. UTC | #4
On 09/03/2015 10:29 AM, David Miller wrote:
> From: Nikola Forró <nforro@redhat.com>
> Date: Thu, 03 Sep 2015 11:08:51 +0200
>
>> @@ -233,8 +233,10 @@ static inline int fib_lookup(struct net *net, const struct flowi4 *flp,
>>   	rcu_read_lock();
>>   
>>   	tb = fib_get_table(net, RT_TABLE_MAIN);
>> -	if (tb && !fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF))
>> -		err = 0;
>> +	if (tb)
>> +		err = fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF);
>> +		if (err == -EAGAIN)
>> +			err = -ENETUNREACH;
> You didn't test this.

Actually the way the code is structured is still functional this way.  
The indentation is all that is really wrong.

I suspect this actually results in smaller code that may be faster for 
the standard case since tb will almost always have a value anyway, and 
if tb doesn't exist then err would equal -ENETUNREACH which would just 
mean the err == -EAGAIN would be ignored.

- Alex


--
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
diff mbox

Patch

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 5fa643b..cf025107 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -233,8 +233,10 @@  static inline int fib_lookup(struct net *net, const struct flowi4 *flp,
 	rcu_read_lock();
 
 	tb = fib_get_table(net, RT_TABLE_MAIN);
-	if (tb && !fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF))
-		err = 0;
+	if (tb)
+		err = fib_table_lookup(tb, flp, res, flags | FIB_LOOKUP_NOREF);
+		if (err == -EAGAIN)
+			err = -ENETUNREACH;
 
 	rcu_read_unlock();
 
@@ -267,11 +269,20 @@  static inline int fib_lookup(struct net *net, struct flowi4 *flp,
 
 	for (err = 0; !err; err = -ENETUNREACH) {
 		tb = rcu_dereference_rtnl(net->ipv4.fib_main);
-		if (tb && !fib_table_lookup(tb, flp, res, flags))
-			break;
+		if (tb) {
+			err = fib_table_lookup(tb, flp, res, flags);
+			if (!err)
+				break;
+		}
 
 		tb = rcu_dereference_rtnl(net->ipv4.fib_default);
-		if (tb && !fib_table_lookup(tb, flp, res, flags))
+		if (tb) {
+			err = fib_table_lookup(tb, flp, res, flags);
+			if (!err)
+				break;
+		}
+
+		if (err && err != -EAGAIN)
 			break;
 	}
 
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e681b85..4ce3f87 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2020,6 +2020,7 @@  struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 	struct fib_result res;
 	struct rtable *rth;
 	int orig_oif;
+	int err = ENETUNREACH;
 
 	res.tclassid	= 0;
 	res.fi		= NULL;
@@ -2123,7 +2124,8 @@  struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 		goto make_route;
 	}
 
-	if (fib_lookup(net, fl4, &res, 0)) {
+	err = fib_lookup(net, fl4, &res, 0);
+	if (err) {
 		res.fi = NULL;
 		res.table = NULL;
 		if (fl4->flowi4_oif) {
@@ -2151,7 +2153,7 @@  struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 			res.type = RTN_UNICAST;
 			goto make_route;
 		}
-		rth = ERR_PTR(-ENETUNREACH);
+		rth = ERR_PTR(err);
 		goto out;
 	}
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index d155864..d33a6a5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1847,9 +1847,11 @@  int ip6_route_add(struct fib6_config *cfg)
 			rt->dst.input = ip6_pkt_prohibit;
 			break;
 		case RTN_THROW:
+		case RTN_UNREACHABLE:
 		default:
 			rt->dst.error = (cfg->fc_type == RTN_THROW) ? -EAGAIN
-					: -ENETUNREACH;
+					: (cfg->fc_type == RTN_UNREACHABLE)
+					? -EHOSTUNREACH : -ENETUNREACH;
 			rt->dst.output = ip6_pkt_discard_out;
 			rt->dst.input = ip6_pkt_discard;
 			break;