netdev - Re: [PATCH] 8021q: fix vlan 0 inconsistencies

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]

Date:	Thu, 27 Jun 2013 22:27:49 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	nikolay@...hat.com
Cc:	netdev@...r.kernel.org, kaber@...sh.net
Subject: Re: [PATCH] 8021q: fix vlan 0 inconsistencies

From: Nikolay Aleksandrov <nikolay@...hat.com>
Date: Thu, 20 Jun 2013 16:08:34 +0200

> On 06/20/2013 02:24 PM, nikolay@...hat.com wrote:
>> From: Nikolay Aleksandrov <nikolay@...hat.com>
>> 
>> The first part of the patch stops the addition of VLAN 0 to bonding
>> devices because we use an internal vlan_list to keep the added vlans and
>> after that when checking if we're using vlans on the bond
>> (bond_vlan_used) it evaluates to true always, which leads to different
>> problems. Since this is intended for HW vlan filters, it's not needed
>> for the bonding, and for its slaves it will still get added upon
>> NETDEV_UP.
>> The second part that does unconditional vlan_vid_del is needed because
>> when we add vlan 0 to a bonding device, it can never be removed
>> completely (it will always stay in the local vlan_list). Since there's
>> refcounting, I don't think this will change the behaviour because if a
>> real device is UP then vlan 0 will have at least refcnt == 1 so
>> ndo_vlan_rx_kill_vid won't get called until the device is down, but in
>> the bonding case we need it while the device is up so we can cleanup
>> properly after vlan 0 removal.
>> As an addition I'd like to say that I tried many different fixes of this
>> issue from inside the bonding, but they all have shortcomings and fixing
>> the root cause would be much better. For example I can't filter out vlan
>> 0 in the bond's ndo_vlan_rx_add_vid because bond_has_this_ip() (and others)
>> rely on being able to check the vlan devices on top through the local
>> vlan_list. Also there's no way to differentiate between addition of vlan 0
>> from vlan_device_event and from register_vlan_dev.
>> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.com>
> In fact I think there's a deeper issue with vlan 0 because if you add it to any
> device its refcount will only be incremented (unconditional vlan_vid_add in
> register_vlan_dev) and never decremented. And this issue is also fixed by this
> patch.

I don't think I can apply this patch, it seems to revert very much intentional
behavior.

If you have the 8021q module available, and you bring a device up, it gets
VLAN 0 by default, and if necessary programmed into the HW filters of the
device.

This VLAN 0 entry is not treated like a real VLAN, it is just there to be
decapsulated for the sake of 802.1p Priority Code Points (QoS).

If the user explicitly configures other VLAN entries, then removes them all,
that conditional check on vlan_id in the delete path retains this default
VLAN 0 configuration and is very much intended to behave that way.

Your patch breaks this, so I cannot apply it.

If bonding is so broken that it cannot cope with this default 802.1p behavior,
that is really bonding's problem.  It seemingly needs logic to handle 802.1p,
and that default VID 0, properly.

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