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: <CANaxB-ypW4cFxNTAaS-n6ehNHxGyxe9x0G8SJap6PBp8UJX9gA@mail.gmail.com>
Date:   Tue, 15 Nov 2016 14:45:25 -0800
From:   Andrei Vagin <avagin@...il.com>
To:     Cong Wang <xiyou.wangcong@...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 2:21 PM, Andrei Vagin <avagin@...il.com> wrote:
> On Tue, Nov 15, 2016 at 1:07 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> 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.
>
> I would like to agree with you here, but looks like sockets with
> NETLINK_F_LISTEN_ALL_NSID are able to catch these messages.

Actually I found that I was wrong.

do_one_broadcast() sends a notification only if a device network
namespace has an id in a socket netns. But cleanup_net() removes all
id-s to a target namespace, so just ignore my last comment.

Thanks,
Andrei