netdev - Re: Refcount mismatch when unregistering netdevice from kernel

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_D+diS7jnpoGk6cncWL8qiAGod2EAp=Vcnc-zWNPg04Jg@mail.gmail.com>
Date:   Tue, 8 Dec 2020 10:09:39 -0800
From:   Wei Wang <weiwan@...gle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     stranche@...eaurora.org, David Ahern <dsahern@...il.com>,
        Martin KaFai Lau <kafai@...com>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Subject: Re: Refcount mismatch when unregistering netdevice from kernel

Hi Sean,

Thanks for reporting the issue. Like Eric said, if you could provide
the kernel version you are using, that would be great. And if you have
a reproducer, that would be even better.
I have a few comments inline below.

On Tue, Dec 8, 2020 at 7:08 AM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
>
> On 12/8/20 4:55 AM, stranche@...eaurora.org wrote:
> > Hi everyone,
> >
> > We've recently been investigating a refcount problem when unregistering a netdevice from the kernel. It seems that the IPv6 module is still holding various references to the inet6_dev associated with the main netdevice struct that are not being released, preventing the unregistration from completing.
> >
> > After tracing the various locations that take/release refcounts to these two structs, we see that there are mismatches in the refcount for the inet6_dev in the DST path when there are routes flushed with the rt6_uncached_list_flush_dev() function during rt6_disable_ip() when the device is unregistering. It seems that usually these references are freed via ip6_dst_ifdown() in the dst_ops->ifdown callback from dst_dev_put(), but this callback is not executed in the rt6_uncached_list_flush_dev() function. Instead, a reference to the inet6_dev is simply dropped to account for the inet6_dev_get() in ip6_rt_copy_init().
> >

rt6_uncached_list_flush_dev() actually tries to replace the inet6_dev
with loopback_dev, and release the reference to the previous inet6_dev
by calling in6_dev_put(), which is actually doing the same thing as
ip6_dst_ifdown(). I don't understand why you say " a reference to the
inet6_dev is simply dropped".

> > Unfortunately, this does not seem to be sufficient, as these uncached routes have an additional refcount on the inet6_device attached to them from the DST allocation. In the normal case, this reference from the DST allocation will happen in the dst_ops->destroy() callback in the dst_destroy() function when the DST is being freed. However, since rt6_uncached_list_flush_dev() changes the inet6_device stored in the DST to the loopback device, the dst_ops->destroy() callback doesn't decrement the refcount on the correct inet6_dev struct.
> >

The additional refcount to the DST is also released by doing the following:
                        if (rt_dev == dev) {
                                rt->dst.dev = blackhole_netdev;
                                dev_hold(rt->dst.dev);
                                dev_put(rt_dev);
                        }
Am I missing something?


> > We're wondering if it would be appropriate to put() the refcount additionally for the uncached routes when flushing out the list for the unregistering device. Perhaps something like the following?
> >
>
> dev refcount imbalances are quite common, particularly on old kernel versions, lacking few fixes.
>
> First thing is to let us know on which kernel version you see this, and how you reproduce it.
>