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

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

[Hangbin Liu]

After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
result when requested"). When we get a prohibit ertry, we will return
-EACCES directly.

Before:
+ ip netns exec client ip -6 route get 2003::1
prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
4294967295 error -13

After:
+ ip netns exec server ip -6 route get 2002::1
RTNETLINK answers: Network is unreachable

Since we will check the ip6_null_entry later. There is not sense
to check the dst.error after get rt.

Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/route.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..8fc52de 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		dst = ip6_route_lookup(net, &fl6, 0);
 
 	rt = container_of(dst, struct rt6_info, dst);
-	if (rt->dst.error) {
-		err = rt->dst.error;
-		ip6_rt_put(rt);
-		goto errout;
-	}
-
 	if (rt == net->ipv6.ip6_null_entry) {
 		err = rt->dst.error;
 		ip6_rt_put(rt);
-- 
2.5.5

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

[Hangbin Liu]

Hi Roopa, Cong,

2017-07-20 22:51 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly.
>
> Before:
> + ip netns exec client ip -6 route get 2003::1
> prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> 4294967295 error -13
>
> After:
> + ip netns exec server ip -6 route get 2002::1
> RTNETLINK answers: Network is unreachable
>
> Since we will check the ip6_null_entry later. There is not sense
> to check the dst.error after get rt.
>
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/route.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96..8fc52de 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
>
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }

hmm... or instead of remove this check, should we check all the entry? Like
if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
net->ipv6.ip6_blk_hole_entry) ||
     rt == net->ipv6.ip6_null_entry )

What do you think?

Thanks
Hangbin

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

[Hangbin Liu]

2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
>> +++ b/net/ipv6/route.c
>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>>                 dst = ip6_route_lookup(net, &fl6, 0);
>>
>>         rt = container_of(dst, struct rt6_info, dst);
>> -       if (rt->dst.error) {
>> -               err = rt->dst.error;
>> -               ip6_rt_put(rt);
>> -               goto errout;
>> -       }
>
> hmm... or instead of remove this check, should we check all the entry? Like
> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
                                                    ^^ mistake here
> net->ipv6.ip6_blk_hole_entry) ||
>      rt == net->ipv6.ip6_null_entry )

Sorry,  there should be no need to check ip6_null_entry since the
error is already
-ENETUNREACH. So how about

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..c290aa4 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,13 +3637,8 @@ static int inet6_rtm_getroute(struct sk_buff
*in_skb, struct nlmsghdr *nlh,
                dst = ip6_route_lookup(net, &fl6, 0);

        rt = container_of(dst, struct rt6_info, dst);
-       if (rt->dst.error) {
-               err = rt->dst.error;
-               ip6_rt_put(rt);
-               goto errout;
-       }
-
-       if (rt == net->ipv6.ip6_null_entry) {
+       if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
+           rt != net->ipv6.ip6_blk_hole_entry) {
                err = rt->dst.error;
                ip6_rt_put(rt);
                goto errout;

Thanks
Hangbin

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

[David Ahern]

On 7/20/17 9:23 AM, Hangbin Liu wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96..c290aa4 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,13 +3637,8 @@ static int inet6_rtm_getroute(struct sk_buff
> *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
> 
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }

The above 5 lines introduced the change in behavior, so removing them
should be sufficient.

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

[Cong Wang]

On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
>>> +++ b/net/ipv6/route.c
>>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>>>                 dst = ip6_route_lookup(net, &fl6, 0);
>>>
>>>         rt = container_of(dst, struct rt6_info, dst);
>>> -       if (rt->dst.error) {
>>> -               err = rt->dst.error;
>>> -               ip6_rt_put(rt);
>>> -               goto errout;
>>> -       }
>>
>> hmm... or instead of remove this check, should we check all the entry? Like
>> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
>                                                     ^^ mistake here
>> net->ipv6.ip6_blk_hole_entry) ||
>>      rt == net->ipv6.ip6_null_entry )
>
> Sorry,  there should be no need to check ip6_null_entry since the
> error is already
> -ENETUNREACH. So how about

Hmm? All of these 3 entries have error set, right??
So we should only check dst.error...

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

[Roopa Prabhu]

On Fri, Jul 21, 2017 at 11:42 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
>>>> +++ b/net/ipv6/route.c
>>>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>>>>                 dst = ip6_route_lookup(net, &fl6, 0);
>>>>
>>>>         rt = container_of(dst, struct rt6_info, dst);
>>>> -       if (rt->dst.error) {
>>>> -               err = rt->dst.error;
>>>> -               ip6_rt_put(rt);
>>>> -               goto errout;
>>>> -       }
>>>
>>> hmm... or instead of remove this check, should we check all the entry? Like
>>> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
>>                                                     ^^ mistake here
>>> net->ipv6.ip6_blk_hole_entry) ||
>>>      rt == net->ipv6.ip6_null_entry )
>>
>> Sorry,  there should be no need to check ip6_null_entry since the
>> error is already
>> -ENETUNREACH. So how about
>
> Hmm? All of these 3 entries have error set, right??
> So we should only check dst.error...

removing the check seems ok to me. We can also make the check
conditional to fibmatch code only
to eliminate any change in behavior introduced by fibmatch.

ie if (fibmatch && rt->dst.error).

Hangbin, can you also pls check that fibmatch works ok for such routes
with your patch applied ?.

ip netns exec client ip -6 route get fibmatch 2003::1     (with latest iproute2)

thank you.

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

[Roopa Prabhu]

On Fri, Jul 21, 2017 at 2:53 PM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> On Fri, Jul 21, 2017 at 11:42 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
>>> 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
>>>>> +++ b/net/ipv6/route.c
>>>>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>>>>>                 dst = ip6_route_lookup(net, &fl6, 0);
>>>>>
>>>>>         rt = container_of(dst, struct rt6_info, dst);
>>>>> -       if (rt->dst.error) {
>>>>> -               err = rt->dst.error;
>>>>> -               ip6_rt_put(rt);
>>>>> -               goto errout;
>>>>> -       }
>>>>
>>>> hmm... or instead of remove this check, should we check all the entry? Like
>>>> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
>>>                                                     ^^ mistake here
>>>> net->ipv6.ip6_blk_hole_entry) ||
>>>>      rt == net->ipv6.ip6_null_entry )
>>>
>>> Sorry,  there should be no need to check ip6_null_entry since the
>>> error is already
>>> -ENETUNREACH. So how about
>>
>> Hmm? All of these 3 entries have error set, right??
>> So we should only check dst.error...
>
> removing the check seems ok to me. We can also make the check
> conditional to fibmatch code only
> to eliminate any change in behavior introduced by fibmatch.
>
> ie if (fibmatch && rt->dst.error).
>
> Hangbin, can you also pls check that fibmatch works ok for such routes
> with your patch applied ?.
>
> ip netns exec client ip -6 route get fibmatch 2003::1     (with latest iproute2)
>
> thank you.

I tried this with your patch and this works for the fibmatch case as well.

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

[Hangbin Liu]

Hi Cong,
On Fri, Jul 21, 2017 at 11:42:46AM -0700, Cong Wang wrote:
> On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> > 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> >>> +++ b/net/ipv6/route.c
> >>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
> >>>                 dst = ip6_route_lookup(net, &fl6, 0);
> >>>
> >>>         rt = container_of(dst, struct rt6_info, dst);
> >>> -       if (rt->dst.error) {
> >>> -               err = rt->dst.error;
> >>> -               ip6_rt_put(rt);
> >>> -               goto errout;
> >>> -       }
> >>
> >> hmm... or instead of remove this check, should we check all the entry? Like
> >> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt !=
> >                                                     ^^ mistake here
> >> net->ipv6.ip6_blk_hole_entry) ||
> >>      rt == net->ipv6.ip6_null_entry )
> >
> > Sorry,  there should be no need to check ip6_null_entry since the
> > error is already
> > -ENETUNREACH. So how about
> 
> Hmm? All of these 3 entries have error set, right??
> So we should only check dst.error...

A question, since you have fixed the ip6_null_entry->rt6i_idev issue via

  ipv6: initialize route null entry in addrconf_init()
  ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf
  ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER
  ipv6: avoid unregistering inet6_dev for loopback

Do we still need this net->ipv6.ip6_null_entry check? How about remove all
the checks?

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..b5e3fe0 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,17 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
                dst = ip6_route_lookup(net, &fl6, 0);

        rt = container_of(dst, struct rt6_info, dst);
-       if (rt->dst.error) {
-               err = rt->dst.error;
-               ip6_rt_put(rt);
-               goto errout;
-       }
-
-       if (rt == net->ipv6.ip6_null_entry) {
-               err = rt->dst.error;
-               ip6_rt_put(rt);
-               goto errout;
-       }

        skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
        if (!skb) {


Before this patch:
+ ip netns exec client ip -6 route get 2003::1
RTNETLINK answers: Network is unreachable

After this patch:
+ ip netns exec client ip -6 route get 2003::1
unreachable 2003::1 dev lo table unspec proto kernel src 2001::1 metric 4294967295 error -101


Thanks
Hangbin

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

[Cong Wang]

On Sun, Jul 23, 2017 at 8:09 PM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> Do we still need this net->ipv6.ip6_null_entry check? How about remove all
> the checks?

I believe you only need to check for rt->dst.error, no need to check against
NULL or ip6_null_entry.

Take a look at other ip6_route_lookup() callers.

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

[Hangbin Liu]

On Mon, Jul 24, 2017 at 12:57:43PM -0700, Cong Wang wrote:
> On Sun, Jul 23, 2017 at 8:09 PM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> > Do we still need this net->ipv6.ip6_null_entry check? How about remove all
> > the checks?
> 
> I believe you only need to check for rt->dst.error, no need to check against
> NULL or ip6_null_entry.
> 
> Take a look at other ip6_route_lookup() callers.

Yes, I saw it. That why I send v2 patch to check both rt->dst.error and
ip6_null_entry.

The question is the other two caller are rpfilter_lookup_reverse6() and
nft_fib6_eval(). From the code it looks these two caller only care about
device match.

         if (rt->rt6i_idev->dev == dev || (flags & XT_RPFILTER_LOOSE))
                 ret = true;

And the device would be lo if it is ip6_null_entry. So they just discard it.
I'm not familiar with netfilter, Please correct me if I make any mistake.

But what we want in inet6_rtm_getroute() and rt6_dump_route() is to
get/dump the route info. So we should get the info even it's unreachable or
prohibit.

That's why I think we should remove both rt->dst.error and ip6_null_entry
check in inet6_rtm_getroute(). And even further, remove the ip6_null_entry
check in rt6_dump_route().

What do you think?

Thanks
Hangbin

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

[David Ahern]

On 7/24/17 6:08 PM, Hangbin Liu wrote:
> That's why I think we should remove both rt->dst.error and ip6_null_entry
> check in inet6_rtm_getroute(). And even further, remove the ip6_null_entry
> check in rt6_dump_route().

git blame net/ipv6/route.c

find the commits and review.

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

[Hangbin Liu]

On Mon, Jul 24, 2017 at 09:28:07PM -0600, David Ahern wrote:
> On 7/24/17 6:08 PM, Hangbin Liu wrote:
> > That's why I think we should remove both rt->dst.error and ip6_null_entry
> > check in inet6_rtm_getroute(). And even further, remove the ip6_null_entry
> > check in rt6_dump_route().
> 
> git blame net/ipv6/route.c
> 
> find the commits and review.

Hi David,

Sorry, would you like to give me more hints?

I saw your commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route
dumps"). But I think this issue has been fixed by

2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")

I use a reproducer which add unreachable route in netns with lo down. And I
could not trigger Panic in the fixed kernel. That's why I think we could
remove ip6_null_entry check in rt6_dump_route().

Please correct me if I make any mistake.

Thanks
Hangbin

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

[David Ahern]

On 7/25/17 1:32 AM, Hangbin Liu wrote:
> On Mon, Jul 24, 2017 at 09:28:07PM -0600, David Ahern wrote:
>> On 7/24/17 6:08 PM, Hangbin Liu wrote:
>>> That's why I think we should remove both rt->dst.error and ip6_null_entry
>>> check in inet6_rtm_getroute(). And even further, remove the ip6_null_entry
>>> check in rt6_dump_route().
>>
>> git blame net/ipv6/route.c
>>
>> find the commits and review.
> 
> Hi David,
> 
> Sorry, would you like to give me more hints?
> 
> I saw your commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route
> dumps"). But I think this issue has been fixed by
> 
> 2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
> 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
> 
> I use a reproducer which add unreachable route in netns with lo down. And I
> could not trigger Panic in the fixed kernel. That's why I think we could
> remove ip6_null_entry check in rt6_dump_route().

Understood. Cong's patch to fix the intialization order (lo device
before route init) makes sure the idev is not null. That said, the
null_entry route is internal ipv6 logic and is not a route entry to be
returned to userspace.

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

[Roopa Prabhu]

On Wed, Jul 26, 2017 at 10:18 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/25/17 1:32 AM, Hangbin Liu wrote:
>> On Mon, Jul 24, 2017 at 09:28:07PM -0600, David Ahern wrote:
>>> On 7/24/17 6:08 PM, Hangbin Liu wrote:
>>>> That's why I think we should remove both rt->dst.error and ip6_null_entry
>>>> check in inet6_rtm_getroute(). And even further, remove the ip6_null_entry
>>>> check in rt6_dump_route().
>>>
>>> git blame net/ipv6/route.c
>>>
>>> find the commits and review.
>>
>> Hi David,
>>
>> Sorry, would you like to give me more hints?
>>
>> I saw your commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route
>> dumps"). But I think this issue has been fixed by
>>
>> 2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
>> 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
>>
>> I use a reproducer which add unreachable route in netns with lo down. And I
>> could not trigger Panic in the fixed kernel. That's why I think we could
>> remove ip6_null_entry check in rt6_dump_route().
>
> Understood. Cong's patch to fix the intialization order (lo device
> before route init) makes sure the idev is not null. That said, the
> null_entry route is internal ipv6 logic and is not a route entry to be
> returned to userspace.

agreed...so looks like the check in v3 should be


+       if ( rt == net->ipv6.ip6_null_entry ||
+            (rt->dst.error &&
+ #ifdef CONFIG_IPV6_MULTIPLE_TABLES
+              rt != net->ipv6.ip6_prohibit_entry &&
+              rt != net->ipv6.ip6_blk_hole_entry &&
+#endif
+             )) {
                err = rt->dst.error;
                ip6_rt_put(rt);
                goto errout;

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

[David Ahern]

On 7/26/17 12:27 PM, Roopa Prabhu wrote:
> agreed...so looks like the check in v3 should be
> 
> 
> +       if ( rt == net->ipv6.ip6_null_entry ||
> +            (rt->dst.error &&
> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> +              rt != net->ipv6.ip6_prohibit_entry &&
> +              rt != net->ipv6.ip6_blk_hole_entry &&
> +#endif
> +             )) {
>                 err = rt->dst.error;
>                 ip6_rt_put(rt);
>                 goto errout;
> 

I don't think so. If I add a prohibit route and use the fibmatch
attribute, I want to see the route from the FIB that was matched.

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

[Roopa Prabhu]

On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>> agreed...so looks like the check in v3 should be
>>
>>
>> +       if ( rt == net->ipv6.ip6_null_entry ||
>> +            (rt->dst.error &&
>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>> +              rt != net->ipv6.ip6_prohibit_entry &&
>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>> +#endif
>> +             )) {
>>                 err = rt->dst.error;
>>                 ip6_rt_put(rt);
>>                 goto errout;
>>
>
> I don't think so. If I add a prohibit route and use the fibmatch
> attribute, I want to see the route from the FIB that was matched.


yes, exactly. wouldn't  'rt != net->ipv6.ip6_prohibit_entry' above let
it fall through to the route fill code ?

ah...but i guess you are saying that they will have rt6_info's of
their own and will not match. got it. ack.

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

[David Ahern]

On 7/26/17 12:55 PM, Roopa Prabhu wrote:
> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>>> agreed...so looks like the check in v3 should be
>>>
>>>
>>> +       if ( rt == net->ipv6.ip6_null_entry ||
>>> +            (rt->dst.error &&
>>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>>> +              rt != net->ipv6.ip6_prohibit_entry &&
>>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>>> +#endif
>>> +             )) {
>>>                 err = rt->dst.error;
>>>                 ip6_rt_put(rt);
>>>                 goto errout;
>>>
>>
>> I don't think so. If I add a prohibit route and use the fibmatch
>> attribute, I want to see the route from the FIB that was matched.
> 
> 
> yes, exactly. wouldn't  'rt != net->ipv6.ip6_prohibit_entry' above let
> it fall through to the route fill code ?
> 
> ah...but i guess you are saying that they will have rt6_info's of
> their own and will not match. got it. ack.
> 

This:

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96a819d..24de81c804c2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,11 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
*in_skb, struct nlmsghdr *nlh,
                dst = ip6_route_lookup(net, &fl6, 0);

        rt = container_of(dst, struct rt6_info, dst);
-       if (rt->dst.error) {
-               err = rt->dst.error;
-               ip6_rt_put(rt);
-               goto errout;
-       }

        if (rt == net->ipv6.ip6_null_entry) {
                err = rt->dst.error;

Puts back the original behavior. In that case, only rt == null_entry
drops to the error path which is correct. All other rt values will drop
to rt6_fill_node and return rt data.

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

[Roopa Prabhu]

On Wed, Jul 26, 2017 at 12:00 PM, David Ahern <dsahern@gmail.com> wrote:
> On 7/26/17 12:55 PM, Roopa Prabhu wrote:
>> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
>>> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>>>> agreed...so looks like the check in v3 should be
>>>>
>>>>
>>>> +       if ( rt == net->ipv6.ip6_null_entry ||
>>>> +            (rt->dst.error &&
>>>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>>>> +              rt != net->ipv6.ip6_prohibit_entry &&
>>>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>>>> +#endif
>>>> +             )) {
>>>>                 err = rt->dst.error;
>>>>                 ip6_rt_put(rt);
>>>>                 goto errout;
>>>>
>>>
>>> I don't think so. If I add a prohibit route and use the fibmatch
>>> attribute, I want to see the route from the FIB that was matched.
>>
>>
>> yes, exactly. wouldn't  'rt != net->ipv6.ip6_prohibit_entry' above let
>> it fall through to the route fill code ?
>>
>> ah...but i guess you are saying that they will have rt6_info's of
>> their own and will not match. got it. ack.
>>
>
> This:
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96a819d..24de81c804c2 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,11 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
> *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
>
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }
>
>         if (rt == net->ipv6.ip6_null_entry) {
>                 err = rt->dst.error;
>
> Puts back the original behavior. In that case, only rt == null_entry
> drops to the error path which is correct. All other rt values will drop
> to rt6_fill_node and return rt data.

yes, i thought so too and hence acked v1. But, following congs
comment, realized that it may mask some real errors for fibmatch ?

I just tested a case of unreachable route with just the above patch
you posted, and I do get the error correctly.

so, I guess you are saying all real errors for fibmatch will have "rt
== net->ipv6.ip6_null_entry" and we should be ok.
sounds good to me.

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

[Hangbin Liu]

Hi David,

On Wed, Jul 26, 2017 at 01:00:26PM -0600, David Ahern wrote:
> >> I don't think so. If I add a prohibit route and use the fibmatch
> >> attribute, I want to see the route from the FIB that was matched.
> > 
> > 
> > yes, exactly. wouldn't  'rt != net->ipv6.ip6_prohibit_entry' above let
> > it fall through to the route fill code ?
> > 
> > ah...but i guess you are saying that they will have rt6_info's of
> > their own and will not match. got it. ack.
> > 
> 
> This:
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96a819d..24de81c804c2 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,11 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
> *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
> 
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }
> 
>         if (rt == net->ipv6.ip6_null_entry) {
>                 err = rt->dst.error;
> 
> Puts back the original behavior. In that case, only rt == null_entry
> drops to the error path which is correct. All other rt values will drop
> to rt6_fill_node and return rt data.

Thanks for your explains. Now I know where I made the mistake. I mis-looked
FR_ACT_UNREACHABLE to RTN_UNREACHABLE and thought we return rt =
net->ipv6.ip6_null_entry in fib6_rule_action().

With you help I know we just set rt->dst.error to -EACCES for prohibit
entry in ip6_route_info_create. So remove the rt->dst.error check is enought.

Thanks
Hangbin

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

[Cong Wang]

On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>> agreed...so looks like the check in v3 should be
>>
>>
>> +       if ( rt == net->ipv6.ip6_null_entry ||
>> +            (rt->dst.error &&
>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>> +              rt != net->ipv6.ip6_prohibit_entry &&
>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>> +#endif
>> +             )) {
>>                 err = rt->dst.error;
>>                 ip6_rt_put(rt);
>>                 goto errout;
>>
>
> I don't think so. If I add a prohibit route and use the fibmatch
> attribute, I want to see the route from the FIB that was matched.

But net->ipv6.ip6_prohibit_entry is not the prohibit route you can
add in user-space, it is only used by rule actions. So do you really
want to dump it?? My gut feeling is no, but I am definitely not sure.

When you add a prohibit route, a new rt is allocated dynamically,
net->ipv6.ip6_prohibit_entry is relatively static, internal and is the
only one per netns. (Same for net->ipv6.ip6_blk_hole_entry)

I think Hangbin's example doesn't have ip rules, so this case
is not shown up.

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

[Hangbin Liu]

On Thu, Jul 27, 2017 at 09:56:08PM -0700, Cong Wang wrote:
> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
> > On 7/26/17 12:27 PM, Roopa Prabhu wrote:
> >> agreed...so looks like the check in v3 should be
> >>
> >>
> >> +       if ( rt == net->ipv6.ip6_null_entry ||
> >> +            (rt->dst.error &&
> >> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> >> +              rt != net->ipv6.ip6_prohibit_entry &&
> >> +              rt != net->ipv6.ip6_blk_hole_entry &&
> >> +#endif
> >> +             )) {
> >>                 err = rt->dst.error;
> >>                 ip6_rt_put(rt);
> >>                 goto errout;
> >>
> >
> > I don't think so. If I add a prohibit route and use the fibmatch
> > attribute, I want to see the route from the FIB that was matched.
> 
> But net->ipv6.ip6_prohibit_entry is not the prohibit route you can
> add in user-space, it is only used by rule actions. So do you really
> want to dump it?? My gut feeling is no, but I am definitely not sure.
> 
> When you add a prohibit route, a new rt is allocated dynamically,
> net->ipv6.ip6_prohibit_entry is relatively static, internal and is the
> only one per netns. (Same for net->ipv6.ip6_blk_hole_entry)
> 
> I think Hangbin's example doesn't have ip rules, so this case
> is not shown up.

I mixed the rule entry and route entry these days. And with your help I can
separate them now.

When first time I find the rt->dst.error return directly issue, I was testing
ip rule actually.

e.g.
+ ip netns exec client ip -6 rule add to 2003::1/64 table 100 unreachable
+ ip netns exec server ip -6 rule add to 2001::1/64 table 100 prohibit
+ ip netns exec client ip -6 route get 2003::1
RTNETLINK answers: Network is unreachable
+ ip netns exec client ip -6 route get 2001::1
RTNETLINK answers: Permission denied

After check I thought we returned net->ipv6.ip6_null_entry /
net->ipv6.ip6_blk_hole_entry in function fib6_rule_action().

That's the reason I want to delete both rt->dst.error and
net->ipv6.ip6_null_entry check in patch v2 and v3.

Then with David's comments I realise we also need to take care about ip route
entrys.

my last mail's comment:
> Thanks for your explains. Now I know where I made the mistake. I mis-looked
> FR_ACT_UNREACHABLE to RTN_UNREACHABLE and thought we return rt =
> net->ipv6.ip6_null_entry in fib6_rule_action().

But then I fall in to the code logic and get lost... And thought
FR_ACT_UNREACHABLE and RTN_UNREACHABLE are not same. Today I re-check the
code and realise RTN_UNREACHABLE is defined in user space and FR_ACT_UNREACHABLE
is in kernel. Actually they are the same.

So after PATCH v4, we fixed the route side. And part of ip rule(prohibit and
blk hole). I will think over of this.

Thanks
Hangbin

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

[David Ahern]

On 7/27/17 10:56 PM, Cong Wang wrote:
> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>>> agreed...so looks like the check in v3 should be
>>>
>>>
>>> +       if ( rt == net->ipv6.ip6_null_entry ||
>>> +            (rt->dst.error &&
>>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>>> +              rt != net->ipv6.ip6_prohibit_entry &&
>>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>>> +#endif
>>> +             )) {
>>>                 err = rt->dst.error;
>>>                 ip6_rt_put(rt);
>>>                 goto errout;
>>>
>>
>> I don't think so. If I add a prohibit route and use the fibmatch
>> attribute, I want to see the route from the FIB that was matched.
> 
> But net->ipv6.ip6_prohibit_entry is not the prohibit route you can
> add in user-space, it is only used by rule actions. So do you really
> want to dump it?? My gut feeling is no, but I am definitely not sure.
> 
> When you add a prohibit route, a new rt is allocated dynamically,
> net->ipv6.ip6_prohibit_entry is relatively static, internal and is the
> only one per netns. (Same for net->ipv6.ip6_blk_hole_entry)
> 
> I think Hangbin's example doesn't have ip rules, so this case
> is not shown up.
> 

Understood. The v4 patch returns getroute to the original behavior. The
original behavior returned a route entry not just an error code. The
following is at 5dafc87f40d7 which the commit before roopa's patch set:

# ip -6 ru add to 6000::/120 prohibit
# ip -6 ro get 6000::1
prohibit 6000::1 from :: dev lo proto kernel src 2001:db8::3 metric
4294967295  error -13 pref medium

# ip -6 ro add vrf red prohibit 5000::1/120
# ip -6 ro get vrf red 5000::1
prohibit 5000::1 from :: dev lo table red src 2001:db8::3 metric 1024
error -13 pref medium

Generically, the only time you get just an error response is when the
lookup fails to find a match and returns the null_entry which has
dst.error = -ENETUNREACH.


Now to your point about the new fibmatch option I have gone back and
forth but in the end I think returning the route associated with the FIB
rule is better than just failing with an error code.

Roopa?

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

[Roopa Prabhu]

On Fri, Jul 28, 2017 at 8:10 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/27/17 10:56 PM, Cong Wang wrote:
>> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@gmail.com> wrote:
>>> On 7/26/17 12:27 PM, Roopa Prabhu wrote:
>>>> agreed...so looks like the check in v3 should be
>>>>
>>>>
>>>> +       if ( rt == net->ipv6.ip6_null_entry ||
>>>> +            (rt->dst.error &&
>>>> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>>>> +              rt != net->ipv6.ip6_prohibit_entry &&
>>>> +              rt != net->ipv6.ip6_blk_hole_entry &&
>>>> +#endif
>>>> +             )) {
>>>>                 err = rt->dst.error;
>>>>                 ip6_rt_put(rt);
>>>>                 goto errout;
>>>>
>>>
>>> I don't think so. If I add a prohibit route and use the fibmatch
>>> attribute, I want to see the route from the FIB that was matched.
>>
>> But net->ipv6.ip6_prohibit_entry is not the prohibit route you can
>> add in user-space, it is only used by rule actions. So do you really
>> want to dump it?? My gut feeling is no, but I am definitely not sure.
>>
>> When you add a prohibit route, a new rt is allocated dynamically,
>> net->ipv6.ip6_prohibit_entry is relatively static, internal and is the
>> only one per netns. (Same for net->ipv6.ip6_blk_hole_entry)
>>
>> I think Hangbin's example doesn't have ip rules, so this case
>> is not shown up.
>>
>
> Understood. The v4 patch returns getroute to the original behavior. The
> original behavior returned a route entry not just an error code. The
> following is at 5dafc87f40d7 which the commit before roopa's patch set:
>
> # ip -6 ru add to 6000::/120 prohibit
> # ip -6 ro get 6000::1
> prohibit 6000::1 from :: dev lo proto kernel src 2001:db8::3 metric
> 4294967295  error -13 pref medium
>
> # ip -6 ro add vrf red prohibit 5000::1/120
> # ip -6 ro get vrf red 5000::1
> prohibit 5000::1 from :: dev lo table red src 2001:db8::3 metric 1024
> error -13 pref medium
>
> Generically, the only time you get just an error response is when the
> lookup fails to find a match and returns the null_entry which has
> dst.error = -ENETUNREACH.
>
>
> Now to your point about the new fibmatch option I have gone back and
> forth but in the end I think returning the route associated with the FIB
> rule is better than just failing with an error code.
>
> Roopa?

for fibmatch, my original intent was to return with an error code.
This is similar
to the ipv4 behavior. One option is to keep the check in there and put
the 'fibmatch'
condition around it. But, i do want to make sure that for the fibmatch case,
it does not return an error directly on an existing prohibit route
entry in the fib.
This is probably doable by checking for appropriate
net->ipv6.ip6_prohibit_entry entries.

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

[David Ahern]

On 7/28/17 11:13 AM, Roopa Prabhu wrote:
> for fibmatch, my original intent was to return with an error code.
> This is similar
> to the ipv4 behavior. One option is to keep the check in there and put
> the 'fibmatch'
> condition around it. But, i do want to make sure that for the fibmatch case,
> it does not return an error directly on an existing prohibit route
> entry in the fib.
> This is probably doable by checking for appropriate
> net->ipv6.ip6_prohibit_entry entries.
> 

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.

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

[Roopa Prabhu]

On Fri, Jul 28, 2017 at 10:39 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/28/17 11:13 AM, Roopa Prabhu wrote:
>> for fibmatch, my original intent was to return with an error code.
>> This is similar
>> to the ipv4 behavior. One option is to keep the check in there and put
>> the 'fibmatch'
>> condition around it. But, i do want to make sure that for the fibmatch case,
>> it does not return an error directly on an existing prohibit route
>> entry in the fib.
>> This is probably doable by checking for appropriate
>> net->ipv6.ip6_prohibit_entry entries.
>>
>
> 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.

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

[David Ahern]

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?

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

[Cong Wang]

On Fri, Jul 28, 2017 at 10:39 AM, David Ahern <dsahern@gmail.com> wrote:
> On 7/28/17 11:13 AM, Roopa Prabhu wrote:
>> for fibmatch, my original intent was to return with an error code.
>> This is similar
>> to the ipv4 behavior. One option is to keep the check in there and put
>> the 'fibmatch'
>> condition around it. But, i do want to make sure that for the fibmatch case,
>> it does not return an error directly on an existing prohibit route
>> entry in the fib.
>> This is probably doable by checking for appropriate
>> net->ipv6.ip6_prohibit_entry entries.
>>
>
> 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
>

There must be a reason why we allocate prohibit entries
dynamically for IPv6 despite we already have a (relatively)
static one.

>From this point of view, we need to dump them, that is,
restore the behavior before Roopa's patch.


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

There are already many differences between IPv4 and
IPv6 behaviors, I don't see why this one is so special
that we have to make it consistent.

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

[David Ahern]

On 7/31/17 12:37 PM, Cong Wang wrote:
> 
>> 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.
>>
> There are already many differences between IPv4 and
> IPv6 behaviors, I don't see why this one is so special
> that we have to make it consistent.

yes, and I have sent a number of patches closing the gap.

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

[Cong Wang]

On Mon, Jul 24, 2017 at 5:08 PM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> But what we want in inet6_rtm_getroute() and rt6_dump_route() is to
> get/dump the route info. So we should get the info even it's unreachable or
> prohibit.


If you want to dump prohibit/blackhole entry, then you have to check
for null_entry, and rt->dst.error check is still needed because we
could return error on other normal entries too, IOW, your v2 is correct
if dumping prohibit/blackhole is expected.

I thought we don't dump them. I am not sure about the behavior either.

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

[Hangbin Liu]

On Tue, Jul 25, 2017 at 10:49:05AM -0700, Cong Wang wrote:
> On Mon, Jul 24, 2017 at 5:08 PM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> > But what we want in inet6_rtm_getroute() and rt6_dump_route() is to
> > get/dump the route info. So we should get the info even it's unreachable or
> > prohibit.
> 
> If you want to dump prohibit/blackhole entry, then you have to check
> for null_entry, and rt->dst.error check is still needed because we

I could not reproduce the NULL rt6i_idev issue after you route init fix, So
I think it's also safe to dump null_entry now. There is a v3 patch. After
the patch, we could dump unreachable route info like:

+ ip netns exec client ip -6 route get 2003::1
unreachable 2003::1 dev lo table unspec proto kernel metric 4294967295 error -101

> could return error on other normal entries too, IOW, your v2 is correct
> if dumping prohibit/blackhole is expected.

Would you hlep review the v3 patch? I prepare not touch the check in
function rt6_dump_route() now.

Thanks
Hangbin

[PATCHv2 net] ipv6: should not return rt->dst.error if it is prohibit or blk hole entry.

[Hangbin Liu]

After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
result when requested"). When we get a prohibit ertry, we will return
-EACCES directly.

Before:
+ ip netns exec client ip -6 route get 2003::1
prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
4294967295 error -13

After:
+ ip netns exec server ip -6 route get 2002::1
RTNETLINK answers: Permission denied

Fix this by add prohibit and blk hole check. Since ip6_null_entry's
error is already -ENETUNREACH. Merge the ip6_null_entry check and error
check together.

Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/route.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..c290aa4 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,13 +3637,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		dst = ip6_route_lookup(net, &fl6, 0);
 
 	rt = container_of(dst, struct rt6_info, dst);
-	if (rt->dst.error) {
-		err = rt->dst.error;
-		ip6_rt_put(rt);
-		goto errout;
-	}
-
-	if (rt == net->ipv6.ip6_null_entry) {
+	if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
+	    rt != net->ipv6.ip6_blk_hole_entry) {
 		err = rt->dst.error;
 		ip6_rt_put(rt);
 		goto errout;
-- 
2.5.5

Re: [PATCHv2 net] ipv6: should not return rt->dst.error if it is prohibit or blk hole entry.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4517 bytes --]

Hi Hangbin,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Hangbin-Liu/ipv6-should-not-return-rt-dst-error-if-it-is-prohibit-or-blk-hole-entry/20170721-204554
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   net//ipv6/route.c: In function 'inet6_rtm_getroute':
>> net//ipv6/route.c:3640:38: error: 'struct netns_ipv6' has no member named 'ip6_prohibit_entry'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
                                         ^
>> net//ipv6/route.c:3641:21: error: 'struct netns_ipv6' has no member named 'ip6_blk_hole_entry'
         rt != net->ipv6.ip6_blk_hole_entry) {
                        ^

vim +3640 net//ipv6/route.c

  3558	
  3559	static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
  3560				      struct netlink_ext_ack *extack)
  3561	{
  3562		struct net *net = sock_net(in_skb->sk);
  3563		struct nlattr *tb[RTA_MAX+1];
  3564		int err, iif = 0, oif = 0;
  3565		struct dst_entry *dst;
  3566		struct rt6_info *rt;
  3567		struct sk_buff *skb;
  3568		struct rtmsg *rtm;
  3569		struct flowi6 fl6;
  3570		bool fibmatch;
  3571	
  3572		err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_policy,
  3573				  extack);
  3574		if (err < 0)
  3575			goto errout;
  3576	
  3577		err = -EINVAL;
  3578		memset(&fl6, 0, sizeof(fl6));
  3579		rtm = nlmsg_data(nlh);
  3580		fl6.flowlabel = ip6_make_flowinfo(rtm->rtm_tos, 0);
  3581		fibmatch = !!(rtm->rtm_flags & RTM_F_FIB_MATCH);
  3582	
  3583		if (tb[RTA_SRC]) {
  3584			if (nla_len(tb[RTA_SRC]) < sizeof(struct in6_addr))
  3585				goto errout;
  3586	
  3587			fl6.saddr = *(struct in6_addr *)nla_data(tb[RTA_SRC]);
  3588		}
  3589	
  3590		if (tb[RTA_DST]) {
  3591			if (nla_len(tb[RTA_DST]) < sizeof(struct in6_addr))
  3592				goto errout;
  3593	
  3594			fl6.daddr = *(struct in6_addr *)nla_data(tb[RTA_DST]);
  3595		}
  3596	
  3597		if (tb[RTA_IIF])
  3598			iif = nla_get_u32(tb[RTA_IIF]);
  3599	
  3600		if (tb[RTA_OIF])
  3601			oif = nla_get_u32(tb[RTA_OIF]);
  3602	
  3603		if (tb[RTA_MARK])
  3604			fl6.flowi6_mark = nla_get_u32(tb[RTA_MARK]);
  3605	
  3606		if (tb[RTA_UID])
  3607			fl6.flowi6_uid = make_kuid(current_user_ns(),
  3608						   nla_get_u32(tb[RTA_UID]));
  3609		else
  3610			fl6.flowi6_uid = iif ? INVALID_UID : current_uid();
  3611	
  3612		if (iif) {
  3613			struct net_device *dev;
  3614			int flags = 0;
  3615	
  3616			dev = __dev_get_by_index(net, iif);
  3617			if (!dev) {
  3618				err = -ENODEV;
  3619				goto errout;
  3620			}
  3621	
  3622			fl6.flowi6_iif = iif;
  3623	
  3624			if (!ipv6_addr_any(&fl6.saddr))
  3625				flags |= RT6_LOOKUP_F_HAS_SADDR;
  3626	
  3627			if (!fibmatch)
  3628				dst = ip6_route_input_lookup(net, dev, &fl6, flags);
  3629		} else {
  3630			fl6.flowi6_oif = oif;
  3631	
  3632			if (!fibmatch)
  3633				dst = ip6_route_output(net, NULL, &fl6);
  3634		}
  3635	
  3636		if (fibmatch)
  3637			dst = ip6_route_lookup(net, &fl6, 0);
  3638	
  3639		rt = container_of(dst, struct rt6_info, dst);
> 3640		if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
> 3641		    rt != net->ipv6.ip6_blk_hole_entry) {
  3642			err = rt->dst.error;
  3643			ip6_rt_put(rt);
  3644			goto errout;
  3645		}
  3646	
  3647		skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
  3648		if (!skb) {
  3649			ip6_rt_put(rt);
  3650			err = -ENOBUFS;
  3651			goto errout;
  3652		}
  3653	
  3654		skb_dst_set(skb, &rt->dst);
  3655		if (fibmatch)
  3656			err = rt6_fill_node(net, skb, rt, NULL, NULL, iif,
  3657					    RTM_NEWROUTE, NETLINK_CB(in_skb).portid,
  3658					    nlh->nlmsg_seq, 0);
  3659		else
  3660			err = rt6_fill_node(net, skb, rt, &fl6.daddr, &fl6.saddr, iif,
  3661					    RTM_NEWROUTE, NETLINK_CB(in_skb).portid,
  3662					    nlh->nlmsg_seq, 0);
  3663		if (err < 0) {
  3664			kfree_skb(skb);
  3665			goto errout;
  3666		}
  3667	
  3668		err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
  3669	errout:
  3670		return err;
  3671	}
  3672	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12059 bytes --]

Re: [PATCHv2 net] ipv6: should not return rt->dst.error if it is prohibit or blk hole entry.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 12502 bytes --]

Hi Hangbin,

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Hangbin-Liu/ipv6-should-not-return-rt-dst-error-if-it-is-prohibit-or-blk-hole-entry/20170721-204554
config: x86_64-randconfig-x003-07211556 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/uapi/linux/capability.h:16,
                    from include/linux/capability.h:15,
                    from net/ipv6/route.c:29:
   net/ipv6/route.c: In function 'inet6_rtm_getroute':
   net/ipv6/route.c:3640:38: error: 'struct netns_ipv6' has no member named 'ip6_prohibit_entry'; did you mean 'ip6_null_entry'?
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
                                         ^
   include/linux/compiler.h:156:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c:3641:21: error: 'struct netns_ipv6' has no member named 'ip6_blk_hole_entry'; did you mean 'ip6_null_entry'?
         rt != net->ipv6.ip6_blk_hole_entry) {
                        ^
   include/linux/compiler.h:156:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c:3640:38: error: 'struct netns_ipv6' has no member named 'ip6_prohibit_entry'; did you mean 'ip6_null_entry'?
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
                                         ^
   include/linux/compiler.h:156:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c:3641:21: error: 'struct netns_ipv6' has no member named 'ip6_blk_hole_entry'; did you mean 'ip6_null_entry'?
         rt != net->ipv6.ip6_blk_hole_entry) {
                        ^
   include/linux/compiler.h:156:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c:3640:38: error: 'struct netns_ipv6' has no member named 'ip6_prohibit_entry'; did you mean 'ip6_null_entry'?
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
                                         ^
   include/linux/compiler.h:167:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c:3641:21: error: 'struct netns_ipv6' has no member named 'ip6_blk_hole_entry'; did you mean 'ip6_null_entry'?
         rt != net->ipv6.ip6_blk_hole_entry) {
                        ^
   include/linux/compiler.h:167:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> net/ipv6/route.c:3640:2: note: in expansion of macro 'if'
     if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
     ^~
   net/ipv6/route.c: At top level:
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'strcpy' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:390:2: note: in expansion of macro 'if'
     if (p_size == (size_t)-1 && q_size == (size_t)-1)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:380:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:378:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:369:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:367:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:358:2: note: in expansion of macro 'if'
     if (p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:356:2: note: in expansion of macro 'if'
     if (__builtin_constant_p(size) && p_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:348:2: note: in expansion of macro 'if'
     if (p_size < size || q_size < size)
     ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:345:3: note: in expansion of macro 'if'
      if (q_size < size)
      ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:343:3: note: in expansion of macro 'if'
      if (p_size < size)
      ^~
   include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
       ______f = {     \
       ^
   include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
    #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
                          ^~~~~~~~~~
   include/linux/string.h:342:2: note: in expansion of macro 'if'

vim +/if +3640 net/ipv6/route.c

  3558	
  3559	static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
  3560				      struct netlink_ext_ack *extack)
  3561	{
  3562		struct net *net = sock_net(in_skb->sk);
  3563		struct nlattr *tb[RTA_MAX+1];
  3564		int err, iif = 0, oif = 0;
  3565		struct dst_entry *dst;
  3566		struct rt6_info *rt;
  3567		struct sk_buff *skb;
  3568		struct rtmsg *rtm;
  3569		struct flowi6 fl6;
  3570		bool fibmatch;
  3571	
  3572		err = nlmsg_parse(nlh, sizeof(*rtm), tb, RTA_MAX, rtm_ipv6_policy,
  3573				  extack);
  3574		if (err < 0)
  3575			goto errout;
  3576	
  3577		err = -EINVAL;
  3578		memset(&fl6, 0, sizeof(fl6));
  3579		rtm = nlmsg_data(nlh);
  3580		fl6.flowlabel = ip6_make_flowinfo(rtm->rtm_tos, 0);
  3581		fibmatch = !!(rtm->rtm_flags & RTM_F_FIB_MATCH);
  3582	
  3583		if (tb[RTA_SRC]) {
  3584			if (nla_len(tb[RTA_SRC]) < sizeof(struct in6_addr))
  3585				goto errout;
  3586	
  3587			fl6.saddr = *(struct in6_addr *)nla_data(tb[RTA_SRC]);
  3588		}
  3589	
  3590		if (tb[RTA_DST]) {
  3591			if (nla_len(tb[RTA_DST]) < sizeof(struct in6_addr))
  3592				goto errout;
  3593	
  3594			fl6.daddr = *(struct in6_addr *)nla_data(tb[RTA_DST]);
  3595		}
  3596	
  3597		if (tb[RTA_IIF])
  3598			iif = nla_get_u32(tb[RTA_IIF]);
  3599	
  3600		if (tb[RTA_OIF])
  3601			oif = nla_get_u32(tb[RTA_OIF]);
  3602	
  3603		if (tb[RTA_MARK])
  3604			fl6.flowi6_mark = nla_get_u32(tb[RTA_MARK]);
  3605	
  3606		if (tb[RTA_UID])
  3607			fl6.flowi6_uid = make_kuid(current_user_ns(),
  3608						   nla_get_u32(tb[RTA_UID]));
  3609		else
  3610			fl6.flowi6_uid = iif ? INVALID_UID : current_uid();
  3611	
  3612		if (iif) {
  3613			struct net_device *dev;
  3614			int flags = 0;
  3615	
  3616			dev = __dev_get_by_index(net, iif);
  3617			if (!dev) {
  3618				err = -ENODEV;
  3619				goto errout;
  3620			}
  3621	
  3622			fl6.flowi6_iif = iif;
  3623	
  3624			if (!ipv6_addr_any(&fl6.saddr))
  3625				flags |= RT6_LOOKUP_F_HAS_SADDR;
  3626	
  3627			if (!fibmatch)
  3628				dst = ip6_route_input_lookup(net, dev, &fl6, flags);
  3629		} else {
  3630			fl6.flowi6_oif = oif;
  3631	
  3632			if (!fibmatch)
  3633				dst = ip6_route_output(net, NULL, &fl6);
  3634		}
  3635	
  3636		if (fibmatch)
  3637			dst = ip6_route_lookup(net, &fl6, 0);
  3638	
  3639		rt = container_of(dst, struct rt6_info, dst);
> 3640		if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry &&
  3641		    rt != net->ipv6.ip6_blk_hole_entry) {
  3642			err = rt->dst.error;
  3643			ip6_rt_put(rt);
  3644			goto errout;
  3645		}
  3646	
  3647		skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
  3648		if (!skb) {
  3649			ip6_rt_put(rt);
  3650			err = -ENOBUFS;
  3651			goto errout;
  3652		}
  3653	
  3654		skb_dst_set(skb, &rt->dst);
  3655		if (fibmatch)
  3656			err = rt6_fill_node(net, skb, rt, NULL, NULL, iif,
  3657					    RTM_NEWROUTE, NETLINK_CB(in_skb).portid,
  3658					    nlh->nlmsg_seq, 0);
  3659		else
  3660			err = rt6_fill_node(net, skb, rt, &fl6.daddr, &fl6.saddr, iif,
  3661					    RTM_NEWROUTE, NETLINK_CB(in_skb).portid,
  3662					    nlh->nlmsg_seq, 0);
  3663		if (err < 0) {
  3664			kfree_skb(skb);
  3665			goto errout;
  3666		}
  3667	
  3668		err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
  3669	errout:
  3670		return err;
  3671	}
  3672	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31411 bytes --]

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

[Roopa Prabhu]

On Thu, Jul 20, 2017 at 7:51 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly.
>
> Before:
> + ip netns exec client ip -6 route get 2003::1
> prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> 4294967295 error -13
>
> After:
> + ip netns exec server ip -6 route get 2002::1
> RTNETLINK answers: Network is unreachable
>
> Since we will check the ip6_null_entry later. There is not sense
> to check the dst.error after get rt.
>
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>


thanks.

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

[Hangbin Liu]

Hi Roopa,
On Sat, Jul 22, 2017 at 09:55:51PM -0700, Roopa Prabhu wrote:
> On Thu, Jul 20, 2017 at 7:51 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> > After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> > result when requested"). When we get a prohibit ertry, we will return
> > -EACCES directly.
> >
> > Before:
> > + ip netns exec client ip -6 route get 2003::1
> > prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> > 4294967295 error -13
> >
> > After:
> > + ip netns exec server ip -6 route get 2002::1
> > RTNETLINK answers: Network is unreachable

Sorry, I have a copy/paste error here, the return error should be

+ ip netns exec server ip -6 route get 2002::1
RTNETLINK answers: Permission denied

I fixed it in v2 patch, but forgot to add
#ifdef CONFIG_IPV6_MULTIPLE_TABLES before net->ipv6.ip6_prohibit_entry.

Since you acked this patch. I will post a v3 patch with fixed comment.

> >
> > Since we will check the ip6_null_entry later. There is not sense
> > to check the dst.error after get rt.
> >
> > Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> 
> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Thanks
Hangbin

[PATCHv3 net] ipv6: no need to return rt->dst.error if it is prohibit entry

[Hangbin Liu]

After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
result when requested"). When we get a prohibit ertry, we will return
-EACCES directly.

Before:
+ ip netns exec client ip -6 route get 2003::1
prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
4294967295 error -13

After:
+ ip netns exec server ip -6 route get 2002::1
RTNETLINK answers: Permission denied

Fix this by add prohibit and blk hole check.

At the same time, after commit
2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
We will init rt6i_idev correctly. So we could dump ip6_null_entry
(unreachable route entry) safely now.

Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/route.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..b05da74 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,13 +3637,12 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		dst = ip6_route_lookup(net, &fl6, 0);
 
 	rt = container_of(dst, struct rt6_info, dst);
-	if (rt->dst.error) {
-		err = rt->dst.error;
-		ip6_rt_put(rt);
-		goto errout;
-	}
-
-	if (rt == net->ipv6.ip6_null_entry) {
+	if (rt->dst.error &&
+#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+	    rt != net->ipv6.ip6_prohibit_entry &&
+	    rt != net->ipv6.ip6_blk_hole_entry &&
+#endif
+	    rt != net->ipv6.ip6_null_entry) {
 		err = rt->dst.error;
 		ip6_rt_put(rt);
 		goto errout;
-- 
2.5.5

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

[David Ahern]

On 7/26/17 3:20 AM, Hangbin Liu wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly.
> 
> Before:

Do you mean "Before commit 18c3a61c4264?"

> + ip netns exec client ip -6 route get 2003::1
> prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> 4294967295 error -13
> 
> After:

And "After commit 18c3a61c4264?"

> + ip netns exec server ip -6 route get 2002::1
> RTNETLINK answers: Permission denied
> 
> Fix this by add prohibit and blk hole check.
> 
> At the same time, after commit
> 2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
> 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
> We will init rt6i_idev correctly. So we could dump ip6_null_entry
> (unreachable route entry) safely now.
> 
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/route.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96..b05da74 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,13 +3637,12 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  		dst = ip6_route_lookup(net, &fl6, 0);
>  
>  	rt = container_of(dst, struct rt6_info, dst);
> -	if (rt->dst.error) {
> -		err = rt->dst.error;
> -		ip6_rt_put(rt);
> -		goto errout;
> -	}
> -
> -	if (rt == net->ipv6.ip6_null_entry) {
> +	if (rt->dst.error &&
> +#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> +	    rt != net->ipv6.ip6_prohibit_entry &&
> +	    rt != net->ipv6.ip6_blk_hole_entry &&
> +#endif
> +	    rt != net->ipv6.ip6_null_entry) {
>  		err = rt->dst.error;
>  		ip6_rt_put(rt);
>  		goto errout;
> 

This is what I see with your patch:

# ip -6 ro ls vrf red
2001:db8:1::/120 dev eth1 proto kernel metric 256  pref medium
prohibit 5000::/120 dev lo metric 1024  error -13 pref medium
fe80::/64 dev eth1 proto kernel metric 256  pref medium
ff00::/8 dev eth1 metric 256  pref medium
unreachable default dev lo metric 8192  error -113 pref medium

ie., I added a prohibit route for 5000:/120

and then running:
# ip -6 ro get vrf red 5000::1
RTNETLINK answers: Permission denied

Which is the behavior without your patch.

Now if I delete just the first bit:

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96a819d..8fc52de40175 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
*in_skb, struct nlmsghdr *nlh,
                dst = ip6_route_lookup(net, &fl6, 0);

        rt = container_of(dst, struct rt6_info, dst);
-       if (rt->dst.error) {
-               err = rt->dst.error;
-               ip6_rt_put(rt);
-               goto errout;
-       }
-
        if (rt == net->ipv6.ip6_null_entry) {
                err = rt->dst.error;
                ip6_rt_put(rt);

Then I get:

# ip -6 ro get vrf red 5000::1
prohibit 5000::1 from :: dev lo table red src 2001:db8::2 metric 1024
error -13 pref medium

which seems to be your objective.

I don't understand why you are focused on the built-in null and prohibit
route entries. When I add a default unreachable or prohibit route those
are different rt6_info entries. Take a look at ip6_route_info_create.

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

[David Ahern]

On 7/26/17 11:09 AM, David Ahern wrote:
> I don't understand why you are focused on the built-in null and prohibit
> route entries. 

I see. You are using fib rules for the prohibit entry; I am using an
explicit route entry.

If I run 'ip ro get fibmatch' for the latter I want to see that route
entry since it is a route in the FIB:

# ip -6 ro get fibmatch vrf red 5000::1
prohibit 5000::/120 dev lo table red metric 1024 error -13 pref medium

So there are multiple cases to verify.

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

[Hangbin Liu]

On Wed, Jul 26, 2017 at 11:09:39AM -0600, David Ahern wrote:
> On 7/26/17 3:20 AM, Hangbin Liu wrote:
> > After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> > result when requested"). When we get a prohibit ertry, we will return
> > -EACCES directly.
> > 
> > Before:
> 
> Do you mean "Before commit 18c3a61c4264?"
> 
> > + ip netns exec client ip -6 route get 2003::1
> > prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> > 4294967295 error -13
> > 
> > After:
> 
> And "After commit 18c3a61c4264?"

Yes. Sorry I didn't make it clear.

> 
> > + ip netns exec server ip -6 route get 2002::1
> > RTNETLINK answers: Permission denied
> > 
> > Fix this by add prohibit and blk hole check.
> > 
> > At the same time, after commit
> > 2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
> > 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
> > We will init rt6i_idev correctly. So we could dump ip6_null_entry
> > (unreachable route entry) safely now.
> > 
> > Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> This is what I see with your patch:
> 
> # ip -6 ro ls vrf red
> 2001:db8:1::/120 dev eth1 proto kernel metric 256  pref medium
> prohibit 5000::/120 dev lo metric 1024  error -13 pref medium
> fe80::/64 dev eth1 proto kernel metric 256  pref medium
> ff00::/8 dev eth1 metric 256  pref medium
> unreachable default dev lo metric 8192  error -113 pref medium
> 
> ie., I added a prohibit route for 5000:/120
> 
> and then running:
> # ip -6 ro get vrf red 5000::1
> RTNETLINK answers: Permission denied
> 
> Which is the behavior without your patch.
> 
> Now if I delete just the first bit:
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96a819d..8fc52de40175 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
> *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
> 
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }
> -
>         if (rt == net->ipv6.ip6_null_entry) {
>                 err = rt->dst.error;
>                 ip6_rt_put(rt);
> 
> Then I get:
> 
> # ip -6 ro get vrf red 5000::1
> prohibit 5000::1 from :: dev lo table red src 2001:db8::2 metric 1024
> error -13 pref medium
> 
> which seems to be your objective.

Yes
>
>> I don't understand why you are focused on the built-in null and prohibit
>> route entries.
>
>I see. You are using fib rules for the prohibit entry; I am using an
>explicit route entry.

Yes, This is my fault. I should put my steps in the commit.

# ip -6 rule add to 2003::1/64 table 100 prohibit
# ip -6 route get 2003::1
RTNETLINK answers: Permission denied

>
>If I run 'ip ro get fibmatch' for the latter I want to see that route
>entry since it is a route in the FIB:
>
># ip -6 ro get fibmatch vrf red 5000::1
>prohibit 5000::/120 dev lo table red metric 1024 error -13 pref medium
>
>So there are multiple cases to verify.

Yeah... there are more stuff I need to learn.

Thanks
Hangbin

[PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[Hangbin Liu]

After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
result when requested"). When we get a prohibit ertry, we will return
-EACCES directly instead of dump route info.

Fix it by remove the rt->dst.error check.

Before fix:
\# ip -6 route add prohibit 2003::/64 dev eth1
\# ip -6 route get fibmatch 2003::1
RTNETLINK answers: Permission denied
\# ip -6 route add unreachable 2004::/64 dev eth1
\# ip -6 route get fibmatch 2004::1
RTNETLINK answers: No route to host

After fix:
\# ip -6 route add prohibit 2003::/64 dev eth1
\# ip -6 route get fibmatch 2003::1
prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
\# ip -6 route add unreachable 2004::/64 dev eth1
\# ip -6 route get fibmatch 2004::1
unreachable 2004::/64 dev lo metric 1024 error -113 pref medium

Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/route.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d30c96..8fc52de 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		dst = ip6_route_lookup(net, &fl6, 0);
 
 	rt = container_of(dst, struct rt6_info, dst);
-	if (rt->dst.error) {
-		err = rt->dst.error;
-		ip6_rt_put(rt);
-		goto errout;
-	}
-
 	if (rt == net->ipv6.ip6_null_entry) {
 		err = rt->dst.error;
 		ip6_rt_put(rt);
-- 
2.5.5

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[David Ahern]

On 7/27/17 10:25 AM, Hangbin Liu wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly instead of dump route info.
> 
> Fix it by remove the rt->dst.error check.
> 
> Before fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> RTNETLINK answers: Permission denied
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> RTNETLINK answers: No route to host
> 
> After fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> unreachable 2004::/64 dev lo metric 1024 error -113 pref medium
> 
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/route.c | 6 ------
>  1 file changed, 6 deletions(-)
> 

Acked-by: David Ahern <dsahern@gmail.com>

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[David Ahern]

On 7/27/17 12:03 PM, David Ahern wrote:
> On 7/27/17 10:25 AM, Hangbin Liu wrote:
>> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
>> result when requested"). When we get a prohibit ertry, we will return
>> -EACCES directly instead of dump route info.
>>
>> Fix it by remove the rt->dst.error check.
>>
>> Before fix:
>> \# ip -6 route add prohibit 2003::/64 dev eth1
>> \# ip -6 route get fibmatch 2003::1
>> RTNETLINK answers: Permission denied
>> \# ip -6 route add unreachable 2004::/64 dev eth1
>> \# ip -6 route get fibmatch 2004::1
>> RTNETLINK answers: No route to host
>>
>> After fix:
>> \# ip -6 route add prohibit 2003::/64 dev eth1
>> \# ip -6 route get fibmatch 2003::1
>> prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
>> \# ip -6 route add unreachable 2004::/64 dev eth1
>> \# ip -6 route get fibmatch 2004::1
>> unreachable 2004::/64 dev lo metric 1024 error -113 pref medium
>>
>> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> ---
>>  net/ipv6/route.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
> 
> Acked-by: David Ahern <dsahern@gmail.com>
> 

Dave: please hold off on applying this patch.

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[Roopa Prabhu]

On Thu, Jul 27, 2017 at 9:25 AM, Hangbin Liu <liuhangbin@gmail.com> wrote:
> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly instead of dump route info.
>
> Fix it by remove the rt->dst.error check.
>
> Before fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> RTNETLINK answers: Permission denied
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> RTNETLINK answers: No route to host
>
> After fix:
> \# ip -6 route add prohibit 2003::/64 dev eth1
> \# ip -6 route get fibmatch 2003::1
> prohibit 2003::/64 dev lo metric 1024 error -13 pref medium
> \# ip -6 route add unreachable 2004::/64 dev eth1
> \# ip -6 route get fibmatch 2004::1
> unreachable 2004::/64 dev lo metric 1024 error -113 pref medium
>
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Fri, 28 Jul 2017 00:25:36 +0800

> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> result when requested"). When we get a prohibit ertry, we will return
> -EACCES directly instead of dump route info.
> 
> Fix it by remove the rt->dst.error check.
...
> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

David A., where are we on this?

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[David Ahern]

On 7/31/17 5:22 PM, David Miller wrote:
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Fri, 28 Jul 2017 00:25:36 +0800
> 
>> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
>> result when requested"). When we get a prohibit ertry, we will return
>> -EACCES directly instead of dump route info.
>>
>> Fix it by remove the rt->dst.error check.
> ...
>> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> David A., where are we on this?
> 

Dizzy from running in circles.


Question I posed to you Saturday morning, 8:41 MDT [1]:

"... 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?"

Personally, after all the discussion I think the behavior as it is right
now is best.

[1] https://www.spinics.net/lists/netdev/msg446571.html

Re: [PATCHv4 net] ipv6: no need to check rt->dst.error when get route info

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Mon, 31 Jul 2017 17:34:09 -0600

> On 7/31/17 5:22 PM, David Miller wrote:
>> From: Hangbin Liu <liuhangbin@gmail.com>
>> Date: Fri, 28 Jul 2017 00:25:36 +0800
>> 
>>> After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
>>> result when requested"). When we get a prohibit ertry, we will return
>>> -EACCES directly instead of dump route info.
>>>
>>> Fix it by remove the rt->dst.error check.
>> ...
>>> Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
>>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> 
>> David A., where are we on this?
>> 
> 
> Dizzy from running in circles.

:-)

> Question I posed to you Saturday morning, 8:41 MDT [1]:
> 
> "... 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?"
> 
> Personally, after all the discussion I think the behavior as it is right
> now is best.
> 
> [1] https://www.spinics.net/lists/netdev/msg446571.html

I agree with you that we should keep the behavior as is.