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  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]
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

Powered by blists - more mailing lists