netdev - Re: [Patch net] ipv6: ignore null_entry in inet6_rtm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM_iQpV-kRgPXCWzkoxJcJfyj=thdgUFeLy+NhM=6G34vzhspA@mail.gmail.com>
Date:   Wed, 1 Mar 2017 09:51:54 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [Patch net] ipv6: ignore null_entry in inet6_rtm_getroute() too

On Tue, Feb 28, 2017 at 2:35 PM, David Ahern <dsa@...ulusnetworks.com> wrote:
> On 2/28/17 11:48 AM, Cong Wang wrote:
>> On Tue, Feb 28, 2017 at 11:01 AM, David Ahern <dsa@...ulusnetworks.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@...gle.com>
>>>> Cc: David Ahern <dsa@...ulusnetworks.com>
>>>> Signed-off-by: Cong Wang <xiyou.wangcong@...il.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.