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