linux-kernel - Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEA6p_BtBdPviFVRC4UoJchbz7eH9fM=KDihYA3zUQebmNBpdA@mail.gmail.com>
Date:   Sun, 13 Aug 2017 13:56:30 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Ido Schimmel <idosch@...sch.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        John Stultz <john.stultz@...aro.org>,
        Martin KaFai Lau <kafai@...com>,
        lkml <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>
Subject: Re: unregister_netdevice: waiting for eth0 to become free. Usage
 count = 1

> Looking at my patch to move host routes from loopback to device with the
> address, I have this:
>
> @@ -2789,7 +2808,8 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
>         const struct arg_dev_net *adn = arg;
>         const struct net_device *dev = adn->dev;
>
> -       if ((rt->dst.dev == dev || !dev) &&
> +       if ((rt->dst.dev == dev || !dev ||
> +            (netdev_unregistering(dev) && rt->rt6i_idev->dev == dev)) &&
>             rt != adn->net->ipv6.ip6_null_entry &&
>             (rt->rt6i_nsiblings == 0 ||
>              (dev && netdev_unregistering(dev)) ||

As you explained earlier, after your patch, all entries in the fib6
tree will have rt->dst.dev be the same as rt->rt6i_idev->dev except
those ones created by p6_rt_cache_alloc() and ip6_rt_pcpu_alloc().
Then the above newly added check is mainly to catch those cached dst
entries (created by ip6_rt_cached_alloc()). right?
And it is required because __ipv6_ifa_notify() -> ip6_del_rt() won't
take care of those cached dst entries.

Then I think I should wait for your patches to get merged before
submitting my patch?

Thanks.
Wei


On Sun, Aug 13, 2017 at 9:24 AM, David Ahern <dsahern@...il.com> wrote:
> On 8/12/17 1:42 PM, Wei Wang wrote:
>> Hi Ido,
>>
>>>> -     if ((rt->dst.dev == dev || !dev) &&
>>>> +     if ((rt->dst.dev == dev || !dev ||
>>>> +          rt->rt6i_idev->dev == dev) &&
>>>
>>> Can you please explain why this line is needed? While host routes aren't
>>> removed from the FIB by rt6_ifdown() (when dst.dev goes down), they are
>>> removed later on in addrconf_ifdown().
>>>
>>
>> Yes.. Agree. But one difference is that if the route is removed from
>> addrconf_ifdown(), dst_dev_put() won't be called to release the
>> devices before doing dst_release(). It is OK if dst_release() sees the
>> refcnt on dst already drops to 0 and directly destroys the dst. But I
>> think it will cause problem if at the time, the dst is still held by
>> some other users because then the refcnt on the device going down will
>> not get released.
>> That's why I think we should remove the dst with either dst->dev ==
>> going down dev or rt6->rt6i_idev->dev == going down dev from the fib6
>> tree always because there, we always call dst_dev_put() to release the
>> device.
>>
>>> With your patch, if I check the return value of ip6_del_rt() in
>>> __ipv6_ifa_notify() I see that -ENONET is returned. Because the host
>>> route was already removed by rt6_ifdown(). When the line in question is
>>> removed from the patch I don't get the error anymore.
>>>
>>
>> Right. That is expected as the route is already removed from the tree.
>>
>>> Is it possible that in John's case the host route was correctly removed
>>> from the FIB and that the unreleased reference was due to a wrong check
>>> in ip6_dst_ifdown() (which you patched correctly AFAICT)?
>>>
>>
>> Yes. possible. But as I explained earlier, I still think we should
>> also remove routes with rt6->rt6i_idev->dev == going down dev from the
>> tree.
>
> Looking at my patch to move host routes from loopback to device with the
> address, I have this:
>
> @@ -2789,7 +2808,8 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
>         const struct arg_dev_net *adn = arg;
>         const struct net_device *dev = adn->dev;
>
> -       if ((rt->dst.dev == dev || !dev) &&
> +       if ((rt->dst.dev == dev || !dev ||
> +            (netdev_unregistering(dev) && rt->rt6i_idev->dev == dev)) &&
>             rt != adn->net->ipv6.ip6_null_entry &&
>             (rt->rt6i_nsiblings == 0 ||
>              (dev && netdev_unregistering(dev)) ||
>
>