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

On Tue, Jun 7, 2011 at 9:18 AM, Jiri Bohac [off-list ref] wrote:

Hi David,

On Sun, Jun 05, 2011 at 02:28:23PM -0700, David Miller wrote:

quoted

From: Jiri Bohac <redacted>
Date: Fri, 3 Jun 2011 22:07:38 +0200

quoted

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

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.