[PATCH] net: Fix behavior of unreachable, blackhole and prohibit routes

[PATCH] net: Fix behavior of unreachable, blackhole and prohibit routes

[Nikola Forró]

Man page of ip-route(8) says the 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 consistent
with documentation.

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

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 5fa643b..8d174a7 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -233,8 +233,8 @@ 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);
 
 	rcu_read_unlock();
 
@@ -267,11 +267,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)
 			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;
-- 
2.4.3

Re: [PATCH] net: Fix behavior of unreachable, blackhole and prohibit routes

[Alexander Duyck]

On 09/01/2015 03:13 AM, Nikola Forró wrote:
> Man page of ip-route(8) says the 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 consistent
> with documentation.

Generally updating kernel code to match user-space documentation isn't 
always the best way to go.  The question I would have is if there are 
any other user-space applications out there that might be expecting this 
behaviour now?

Also your changes don't seem to match up with what you have described. 
You are returning the error code from fib_table_lookup, but 
fib_table_lookup can return -EAGAIN if there is no matching entry found. 
  I don't see you describing how you would deal with that case.  You 
might try testing your code after deleting the default route to see what 
behaviour it is you get.

> Signed-off-by: Nikola Forró <nforro@redhat.com>
> ---
>   include/net/ip_fib.h | 19 ++++++++++++++-----
>   net/ipv4/route.c     |  6 ++++--
>   net/ipv6/route.c     |  4 +++-
>   3 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 5fa643b..8d174a7 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -233,8 +233,8 @@ 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);
>
>   	rcu_read_unlock();
>
> @@ -267,11 +267,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)
>   			break;
>   	}
>

Without a default route set these functions are going to return -EAGAIN 
when it should probably be returning -ENETUNREACH.

> 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;
>   	}
>

This bit appears to overlook the fact that fib_rules_lookup could also 
be the function used to return the error via a call to fib_lookup.  In 
which case that also throws -ESRCH into the mix for return error codes.

> 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;
>

Re: [PATCH] net: Fix behavior of unreachable, blackhole and prohibit routes

[Nikola Forró]

Hello Alexander,
thank you for your comments.

On 09/01/2015 10:53 AM, Alexander Duyck wrote:

> Generally updating kernel code to match user-space documentation
> isn't 
> always the best way to go.  The question I would have is if there are
> any other user-space applications out there that might be expecting
> this 
> behaviour now?
> 
Well, any application which uses connect or sendto syscalls is getting
wrong error codes. If not wrong, than at least different for ipv4 and
ipv6. I think errors in fib_props are defined for a reason.

But I think bigger issue are incorrect ICMP messages being returned to
sender, e.g. packet going to blackhole route is not silently discarded,
instead it generates ICMP net unreachable message. I think that kind of
breaks the purpose of blackhole route.

> Also your changes don't seem to match up with what you have
> described. 
> You are returning the error code from fib_table_lookup, but 
> fib_table_lookup can return -EAGAIN if there is no matching entry
> found. 
>   I don't see you describing how you would deal with that case.  You 
> might try testing your code after deleting the default route to see
> what 
> behaviour it is you get.
> 
You are right, I need to handle -EAGAIN and return -ENETUNREACH
instead.

> This bit appears to overlook the fact that fib_rules_lookup could
> also 
> be the function used to return the error via a call to fib_lookup. 
>  In 
> which case that also throws -ESRCH into the mix for return error
> codes.
> 
I don't think it does. In __fib_lookup -ESRCH returned from
fib_rules_lookup is being replaced by -ENETUNREACH.


I will submit corrected patch.

Kind regards,
Nikola