diff mbox

[net] ipv6: ignore null_entry in inet6_rtm_getroute() too

Message ID 1488307478-10081-1-git-send-email-xiyou.wangcong@gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Cong Wang Feb. 28, 2017, 6:44 p.m. UTC 
Like commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route dumps"),
we need to ignore null entry in inet6_rtm_getroute() too.

Return -ENOENT here because we return the same errno when deleting
the null entry.

Fixes: a1a22c1206 ("net: ipv6: Keep nexthop of multipath route on admin down")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ipv6/route.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

David Ahern Feb. 28, 2017, 7:01 p.m. UTC | #1 
On 2/28/17 10:44 AM, Cong Wang wrote:
> Like commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route dumps"),
> we need to ignore null entry in inet6_rtm_getroute() too.
> 
> Return -ENOENT here because we return the same errno when deleting
> the null entry.
> 
> Fixes: a1a22c1206 ("net: ipv6: Keep nexthop of multipath route on admin down")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/ipv6/route.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f54f426..25590d1 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3627,6 +3627,12 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
>  		rt = (struct rt6_info *)ip6_route_output(net, NULL, &fl6);
>  	}
>  
> +	if (rt == net->ipv6.ip6_null_entry) {
> +		ip6_rt_put(rt);
> +		err = -ENOENT;
> +		goto errout;
> +	}
> +
>  	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>  	if (!skb) {
>  		ip6_rt_put(rt);
> 

hold on. That test exposed something else, not just a getroute problem.
I accidentally ran 'unsahre -n; ip -6 ro ls' on my host machine instead
of a VM, so took some time to recover. dumproute already covers the null
route.

I'll get back to this in the afternoon.
Cong Wang Feb. 28, 2017, 7:48 p.m. UTC | #2 
On Tue, Feb 28, 2017 at 11:01 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 2/28/17 10:44 AM, Cong Wang wrote:
>> Like commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route dumps"),
>> we need to ignore null entry in inet6_rtm_getroute() too.
>>
>> Return -ENOENT here because we return the same errno when deleting
>> the null entry.
>>
>> Fixes: a1a22c1206 ("net: ipv6: Keep nexthop of multipath route on admin down")
>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>> Cc: David Ahern <dsa@cumulusnetworks.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>>  net/ipv6/route.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f54f426..25590d1 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -3627,6 +3627,12 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
>>               rt = (struct rt6_info *)ip6_route_output(net, NULL, &fl6);
>>       }
>>
>> +     if (rt == net->ipv6.ip6_null_entry) {
>> +             ip6_rt_put(rt);
>> +             err = -ENOENT;
>> +             goto errout;
>> +     }
>> +
>>       skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>       if (!skb) {
>>               ip6_rt_put(rt);
>>
>
> hold on. That test exposed something else, not just a getroute problem.
> I accidentally ran 'unsahre -n; ip -6 ro ls' on my host machine instead
> of a VM, so took some time to recover. dumproute already covers the null
> route.
>

Of course, you already stated it in your commit:

    ip6_null_entry is the root of all ipv6 fib tables making it integrated
    into the table and hence passed to the ipv6 route dump code. The
    null_entry route uses the loopback device for dst.dev but may not have
    rt6i_idev set because of the order in which initializations are done --
    ip6_route_net_init is run before addrconf_init has initialized the
    loopback device. Fixing the initialization order is a much bigger problem
    with no obvious solution thus far.

    The BUG is triggered when the loopback is set down and the netif_running
    check added by a1a22c1206 fails. The fill_node descends to checking
    rt->rt6i_idev for ignore_routes_with_linkdown and since rt6i_idev is
    NULL it faults.

    The null_entry route should not be processed in a dump request. Catch
    and ignore. This check is done in rt6_dump_route as it is the highest
    place in the callchain with knowledge of both the route and the network
    namespace.

which is why I omit it.

The rt->rt6i_idev = in6_dev_get(loopback_dev) is apparently not correct,
at that time loopback_dev is just registered and not up or running, its
in6_dev pointer should be NULL, we need to listen to inet6addr event to
make it non-NULL. I thought you apparently knew this...
David Ahern Feb. 28, 2017, 10:35 p.m. UTC | #3 
On 2/28/17 11:48 AM, Cong Wang wrote:
> On Tue, Feb 28, 2017 at 11:01 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> On 2/28/17 10:44 AM, Cong Wang wrote:
>>> Like commit 1f17e2f2c8a8 ("net: ipv6: ignore null_entry on route dumps"),
>>> we need to ignore null entry in inet6_rtm_getroute() too.
>>>
>>> Return -ENOENT here because we return the same errno when deleting
>>> the null entry.
>>>
>>> Fixes: a1a22c1206 ("net: ipv6: Keep nexthop of multipath route on admin down")
>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>> Cc: David Ahern <dsa@cumulusnetworks.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> ---
>>>  net/ipv6/route.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index f54f426..25590d1 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -3627,6 +3627,12 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
>>>               rt = (struct rt6_info *)ip6_route_output(net, NULL, &fl6);
>>>       }
>>>
>>> +     if (rt == net->ipv6.ip6_null_entry) {
>>> +             ip6_rt_put(rt);
>>> +             err = -ENOENT;
>>> +             goto errout;
>>> +     }
>>> +
>>>       skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
>>>       if (!skb) {
>>>               ip6_rt_put(rt);
>>>
>>
>> hold on. That test exposed something else, not just a getroute problem.
>> I accidentally ran 'unsahre -n; ip -6 ro ls' on my host machine instead
>> of a VM, so took some time to recover. dumproute already covers the null
>> route.

My host was running a slightly older kernel (did not have the null_entry
check in the dump route path for one).

As for trapping null_entry on getroute, this changes user experience.
Right now you always get a route response for IPv6 with the error set as
rta_error. This patch changes that. I am fine with it since it makes
IPv6 more like IPv4:

# ip -6 ro get 2001:db8:12::1
RTNETLINK answers: Network is unreachable

But, if we are going to do this then err should be set based on
rt->dst.error (ENOENT is not the right error) and the commit message
should state the change.
diff mbox

Patch

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f54f426..25590d1 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3627,6 +3627,12 @@  static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
 		rt = (struct rt6_info *)ip6_route_output(net, NULL, &fl6);
 	}
 
+	if (rt == net->ipv6.ip6_null_entry) {
+		ip6_rt_put(rt);
+		err = -ENOENT;
+		goto errout;
+	}
+
 	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
 	if (!skb) {
 		ip6_rt_put(rt);