netdev - Re: linux-next: net->netns_ids is used after calling idr

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: <CAM_iQpXYvGnaf=AAD5EJYqQ7WApjRyUResN+0Z_Bq1Xt8G8zQQ@mail.gmail.com>
Date:   Tue, 15 Nov 2016 13:07:05 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Andrei Vagin <avagin@...il.com>
Cc:     Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: linux-next: net->netns_ids is used after calling idr_destroy for it

On Tue, Nov 15, 2016 at 12:48 PM, Andrei Vagin <avagin@...il.com> wrote:
> On Tue, Nov 15, 2016 at 10:50 AM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> On Tue, Nov 15, 2016 at 10:04 AM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>>> On Mon, Nov 14, 2016 at 10:23 PM, Andrei Vagin <avagin@...il.com> wrote:
>>>> Hi Nicolas,
>>>>
>>>> cleanup_net() calls idr_destroy(net->netns_ids) for network namespaces
>>>> and then it calls unregister_netdevice_many() which calls
>>>> idr_alloc(net0>netns_ids). It looks wrong, doesn't it?
>>>>
>>>
>>> netns id is designed to allocate lazily, but yeah it makes no sense
>>> to allocate id for the netns being destroyed, not to mention idr is freed.
>>>
>>> I will send a patch.
>>
>> Could you try the attached patch? I just did some quick netns creation/destroy
>> tests.
>
> Here is another fail:
>
> unreferenced object 0xffff94153912a0c0 (size 2096):
>   comm "ip", pid 29175, jiffies 4294954213 (age 137.624s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 b2 3b 1d 15 94 ff ff  ..........;.....
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffffac865c1a>] kmemleak_alloc+0x4a/0xa0
>     [<ffffffffac243b38>] kmem_cache_alloc+0x128/0x280
>     [<ffffffffac42f5ab>] idr_layer_alloc+0x2b/0x90
>     [<ffffffffac42f9cd>] idr_get_empty_slot+0x34d/0x370
>     [<ffffffffac42fa4e>] idr_alloc+0x5e/0x110
>     [<ffffffffac70ac3d>] __peernet2id_alloc+0x6d/0x90
>     [<ffffffffac70bda5>] peernet2id_alloc+0x55/0xb0
>     [<ffffffffac731246>] rtnl_fill_ifinfo+0xaa6/0x10a0
>     [<ffffffffac7330a3>] rtmsg_ifinfo_build_skb+0x73/0xd0
>     [<ffffffffac7125e1>] rollback_registered_many+0x2a1/0x3a0
>     [<ffffffffac712779>] __unregister_netdevice_many+0x29/0x80
>     [<ffffffffac7127e3>] unregister_netdevice_many+0x13/0x20
>     [<ffffffffc02dc4ce>] macvlan_device_event+0x13e/0x235 [macvlan]
>     [<ffffffffac0bef2a>] notifier_call_chain+0x4a/0x70
>     [<ffffffffac0bf066>] raw_notifier_call_chain+0x16/0x20
>     [<ffffffffac710205>] call_netdevice_notifiers_info+0x35/0x60
>

Oh, drivers send rtmsg in notifiers too, hmm.

>
> What do you think about calling idr_destroy() at the final step in
> cleanup_net()? In this case we can avoid this sort of problems in a
> future.

This was my first idea too, but it looks more risky than my approach.

Also, rtmsg is really not needed because the netns is being destroyed,
no one cares about it here.