* [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too
@ 2017-02-28 18:44 Cong Wang
2017-02-28 19:01 ` David Ahern
0 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2017-02-28 18:44 UTC (permalink / raw)
To: netdev; +Cc: andreyknvl, Cong Wang, David Ahern
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);
--
2.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too
2017-02-28 18:44 [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too Cong Wang
@ 2017-02-28 19:01 ` David Ahern
2017-02-28 19:48 ` Cong Wang
0 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2017-02-28 19:01 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: andreyknvl
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too
2017-02-28 19:01 ` David Ahern
@ 2017-02-28 19:48 ` Cong Wang
2017-02-28 22:35 ` David Ahern
0 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2017-02-28 19:48 UTC (permalink / raw)
To: David Ahern; +Cc: Linux Kernel Network Developers, Andrey Konovalov
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...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too
2017-02-28 19:48 ` Cong Wang
@ 2017-02-28 22:35 ` David Ahern
2017-03-01 17:51 ` Cong Wang
0 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2017-02-28 22:35 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Andrey Konovalov
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too
2017-02-28 22:35 ` David Ahern
@ 2017-03-01 17:51 ` Cong Wang
0 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2017-03-01 17:51 UTC (permalink / raw)
To: David Ahern; +Cc: Linux Kernel Network Developers, Andrey Konovalov
On Tue, Feb 28, 2017 at 2:35 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> 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
>
Yeah, I am not sure if we really want to "return" the null entry here,
since we ignore it in dump anyway. If we really want, an alternative
patch is probably something like:
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 25590d1..e60dc1c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3322,7 +3322,7 @@ static int rt6_nexthop_info(struct sk_buff *skb,
struct rt6_info *rt,
{
if (!netif_running(rt->dst.dev) || !netif_carrier_ok(rt->dst.dev)) {
*flags |= RTNH_F_LINKDOWN;
- if (rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
+ if (rt->rt6i_idev &&
rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
*flags |= RTNH_F_DEAD;
}
> 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.
>
Makes sense, I will change the errno and update changelog.
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-01 17:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-28 18:44 [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too Cong Wang
2017-02-28 19:01 ` David Ahern
2017-02-28 19:48 ` Cong Wang
2017-02-28 22:35 ` David Ahern
2017-03-01 17:51 ` Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.