netdev - Re: [PATCH 1/2] vlan: only create special VLAN 0 once

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: <BANLkTikmNXK=i1HhtGtb1+81rGqGgBQ8mA@mail.gmail.com>
Date:	Tue, 7 Jun 2011 18:25:23 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Jiri Bohac <jbohac@...e.cz>
Cc:	David Miller <davem@...emloft.net>, kaber@...sh.net,
	netdev@...r.kernel.org, pedro.netdev@...devamos.com
Subject: Re: [PATCH 1/2] vlan: only create special VLAN 0 once

On Tue, Jun 7, 2011 at 9:18 AM, Jiri Bohac <jbohac@...e.cz> wrote:
> Hi David,
>
> On Sun, Jun 05, 2011 at 02:28:23PM -0700, David Miller wrote:
>> From: Jiri Bohac <jbohac@...e.cz>
>> Date: Fri, 3 Jun 2011 22:07:38 +0200
>>
>> > Commit ad1afb00 registers a VLAN with vid == 0 for every device to handle
>> > 802.1p frames.  This is currently done on every NETDEV_UP event and the special
>> > vlan is never unregistered.  This may have strange effects on drivers
>> > implementning ndo_vlan_rx_add_vid(). E.g. bonding will allocate a linked-list
>> > element each time, causing a memory leak.
>> >
>> > Only register the special VLAN once on NETDEV_REGISTER.
>> >
>> > Signed-off-by: Jiri Bohac <jbohac@...e.cz>
>>
>> I recognize the problem, but this solution isn't all that good.
>>
>> I am pretty sure that the hardware device driver methods that
>> implement ndo_vlan_rx_add_vid() assume that the device is up.
>> Because most drivers completely reset the chip when the
>> interface is brought up and this will likely clear out the
>> VLAN ID tables in the chip.
>
> Really? In that case, we have a much bigger problem: the vlan
> code allows registering a new vlan on an interface that is down.
> And it only registers the VID with ndo_vlan_rx_add_vid() in
> register_vlan_dev() during the registration of the new vlan
> interface -- it never re-registers the VIDs on a NETDEV_UP.

No, it's not true.  All drivers store the registered vlan filters in
some way so that they can restore them when the device is reset.  This
is currently done in one of two ways: storing a bitmap or iterating
over the devices currently registered in a group.

The vlan code is moving away from directly accessing groups and no new
drivers do this.  In fact, once all drivers are converted over groups
will not even be registered on devices.  This is because otherwise
there is quite a bit of vlan code in each driver, which leads to
inconsistent behavior and bugs.

Really, all a driver needs to know is whether it should add a given
vlan to its table, not what the upper layers plan to do with it.  So
when ndo_vlan_rx_add_vid() is called it should add it to its CAM table
and store it if it is needed to restore behavior after a reset, just
as is done with all other configuration state.
--
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