netdev - Re: [PATCH v3 net-next] net: vxlan: when lower dev unregisters remove vxlan dev as well

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: <52D19FB6.50906@redhat.com>
Date:	Sat, 11 Jan 2014 20:47:02 +0100
From:	Daniel Borkmann <dborkman@...hat.com>
To:	Cong Wang <cwang@...pensource.com>
CC:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH v3 net-next] net: vxlan: when lower dev unregisters remove
 vxlan dev as well

On 01/11/2014 08:03 PM, Cong Wang wrote:
> On Sat, Jan 11, 2014 at 12:46 AM, Daniel Borkmann <dborkman@...hat.com> wrote:
>> On 01/11/2014 01:49 AM, Cong Wang wrote:
>>>
>>> On Fri, Jan 10, 2014 at 2:35 PM, Daniel Borkmann <dborkman@...hat.com>
>>> wrote:
>>>>
>>>> @@ -2673,13 +2712,14 @@ static __net_init int vxlan_init_net(struct net
>>>> *net)
>>>>    static __net_exit void vxlan_exit_net(struct net *net)
>>>>    {
>>>>           struct vxlan_net *vn = net_generic(net, vxlan_net_id);
>>>> -       struct vxlan_dev *vxlan;
>>>> -       LIST_HEAD(list);
>>>> +       struct vxlan_dev *vxlan, *next;
>>>> +       LIST_HEAD(list_kill);
>>>>
>>>>           rtnl_lock();
>>>> -       list_for_each_entry(vxlan, &vn->vxlan_list, next)
>>>> -               unregister_netdevice_queue(vxlan->dev, &list);
>>>> -       unregister_netdevice_many(&list);
>>>> +       list_for_each_entry_safe(vxlan, next, &vn->vxlan_list, next)
>>>> +               vxlan_dellink(vxlan->dev, &list_kill);
>>>> +       unregister_netdevice_many(&list_kill);
>>>> +       list_del(&list_kill);
>>>
>>>
>>> The last list_del() looks suspicous... Since list_kill is a local list
>>> head,
>>> why do we need to delete the head at the end??
>>
>>
>> Cong, maybe I'm missing something, but we're doing this rtnl_dellink()
>> and elsewehere, e.g. commit 226bd341147 ("net: use batched device unregister
>> in veth and macvlan").
>
> list_kill is a list *head* (not a node) on stack. unregister_netdevice_many()
> should remove all the nodes in this list after it finishes. So, list_kill is
> supposed to be empty after that.
>
> Either unregister_netdevice_many() is mis-designed, i.e. it is not like what I
> said above, or list_del() is completely unnecessary.
>
> BTW, there are many places calling unregister_netdevice_many() without
> a following list_del(). For example, bond_net_exit().

Basically I agree with you; I'll wait if there are more comments coming up
that address this question, if not I'll respin, remove that and split the patch
into two parts by the beginning of next week.

Cheers,

Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html