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

[Alexander Duyck <alexander.duyck@gmail.com>]

On 09/03/2015 03:05 AM, Nikola Forró 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 | 22 +++++++++++++++++-----
>   net/ipv4/route.c     |  6 ++++--
>   net/ipv6/route.c     |  4 +++-
>   3 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index 5fa643b..8e7b3e1 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -233,8 +233,11 @@ 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();
>   

The likelihood of tb being NULL is next to 0%.  You would probably be 
better off pulling out the EAGAIN check from the if statement and just 
placing it before the rcu_read_unlock with the correct indentation.

> @@ -267,11 +270,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;
>   	}
>   

The loop stuff can just be dropped.  This code is now getting a bit too 
tangled up to justify it.  Probably just use some goto labels instead.  
I would probably just initialize err to -ENETUNREACH and drop the check 
for err at the end and just handle the -EAGAIN case.

I would also probably just pull the -EAGAIN check and place it at the 
end before the unlock and after whatever label it is you add. No point 
in optimizing for unlikely cases.

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