Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.

From: David Ahern <dsahern@gmail.com>
To: Roopa Prabhu <roopa@cumulusnetworks.com>,
	David Miller <davem@davemloft.net>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	Hangbin Liu <liuhangbin@gmail.com>,
	network dev <netdev@vger.kernel.org>
Subject: Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.
Date: Sat, 29 Jul 2017 08:41:00 -0600	[thread overview]
Message-ID: <cabe22e9-f300-ffa5-46bc-c8b48ea91b66@gmail.com> (raw)
In-Reply-To: <CAJieiUiqFTAuPpUZF1_v7fbfGPiqdj+wagKwUKbvtfUJognT1w@mail.gmail.com>

On 7/28/17 1:52 PM, Roopa Prabhu wrote:
> On Fri, Jul 28, 2017 at 10:39 AM, David Ahern <dsahern@gmail.com> wrote:
>> IPv4 does not have the notion of null_entry or prohibit route entries
>> which makes IPv4 and IPv6 inconsistent - something we really need to be
>> avoiding from a user experience.
>>
>> We have the following cases:
>>
>> # ip -4 rule  add to 172.16.60.0/24 prohibit
>> # ip -4 route add prohibit 172.16.50.0/24
>> # ip -6 rule  add to 6000::/120 prohibit
>> # ip -6 route add prohibit 5000::/120
>>
>>
>> Behavior before Roopa's patch set:
>>   Rule match:
>>     # ip ro get 172.16.60.1
>>     RTNETLINK answers: Permission denied
>>
>>     # ip -6 ro get 6000::1
>>     prohibit 6000::1 from :: dev lo proto kernel src 2001:db8::3 metric
>> 4294967295  error -13 pref medium
>>
>>   Route match:
>>     # ip ro get 172.16.50.1
>>     RTNETLINK answers: Permission denied
>>
>>     # ip -6 ro get 5000::1
>>     prohibit 5000::1 from :: dev lo table red src 2001:db8::3 metric
>> 1024  error -13 pref medium
>>
>>
>> Behavior after Roopa's patch set:
>>   Rule match:
>>     # ip ro get 172.16.60.1
>>     RTNETLINK answers: Permission denied
>>
>>     # ip -6 ro get 6000::1
>>     RTNETLINK answers: Permission denied
>>
>>   Route match:
>>     # ip ro get 172.16.50.1
>>     RTNETLINK answers: Permission denied
>>
>>     # ip -6 ro get 5000::1
>>     RTNETLINK answers: Permission denied
>>
>>
>> So Roopa's fibmatch patches brings consistency between IPv4 and IPv6 at
>> the cost of breaking backwards compatibility for IPv6 when the prohibit
>> or blackhole routes are hit.
>>
>> If that is not acceptable, then let's wrap the change in 'if (fibmatch)'
>> so that when fibmatch is requested we have consistency between IPv4 and
>> IPv6 when it is set.
> 
> 
> David, Thanks for listing all the cases and options.
> 
> for the route match fibmatch case, if a prohibit route entry exists
> (added by user), I was hoping fibmatch can return that entry...
> 
>  # ip -6 ro get fibmatch 5000::1
>     prohibit 5000::1 from :: dev lo
> 
> because the semantics of fibmatch is to return the matching route
> entry if exists.
> I am assuming that is possible with appropriate checks around the
> dst.error check for fibmatch. what do you say ?
> I need to verify if this can work for ipv4 the same way.
> 

Routes such as prohibit, blackhole and unreachable cause
fib_table_lookup to return an error (see the fib_props reference in
fib_table_lookup) and the IPv4 code does not generate an rtable for
them. IMO it does not make sense to complicate the general IPv4 lookup
code to create an rtable for these 'special' routes just for the
fibmatch getroute case. Furthermore, I think consistency between IPv4
and IPv6 is important from a programmability perspective which is what
we have now.

DaveM: as I outlined above, Roopa's fibmatch patches caused a change in
user behavior in IPv6 getroute for prohibit, blackhole and unreachable
route entries. Opinions on whether we should limit that new behavior to
just the fibmatch lookup in which case a patch is needed or take the new
behavior and consistency in which case nothing is needed?