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

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

[Jiri Bohac]

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@suse.cz>

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index c7a581a..bf89565 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -371,7 +371,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 	if (is_vlan_dev(dev))
 		__vlan_device_event(dev, event);
 
-	if ((event == NETDEV_UP) &&
+	if ((event == NETDEV_REGISTER) &&
 	    (dev->features & NETIF_F_HW_VLAN_FILTER) &&
 	    dev->netdev_ops->ndo_vlan_rx_add_vid) {
 		pr_info("8021q: adding VLAN 0 to HW filter on device %s\n",

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

[PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

[Jiri Bohac]

Since commit ad1afb00, bond_del_vlan() never restores
NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
empty once the 8021q module is loaded, because the special VLAN 0
is always kept registered on the bond interface. Change the
condition to check if bond->vlan_list contains exactly one item
instead of checking for an empty list.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 17b4dd9..4d317cd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 
 			kfree(vlan);
 
-			if (list_empty(&bond->vlan_list) &&
+			if (bond->vlan_list.next->next == &bond->vlan_list &&
 			    (bond->slave_cnt == 0)) {
-				/* Last VLAN removed and no slaves, so
+				/* Last VLAN removed (the only member of vlan_list
+				 * is the special vid == 0 vlan) and no slaves, so
 				 * restore block on adding VLANs. This will
 				 * be removed once new slaves that are not
 				 * VLAN challenged will be added.
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

[Jay Vosburgh]

Jiri Bohac <jbohac@suse.cz> wrote:

>Since commit ad1afb00, bond_del_vlan() never restores
>NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
>empty once the 8021q module is loaded, because the special VLAN 0
>is always kept registered on the bond interface. Change the
>condition to check if bond->vlan_list contains exactly one item
>instead of checking for an empty list.
>
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 17b4dd9..4d317cd 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>
> 			kfree(vlan);
>
>-			if (list_empty(&bond->vlan_list) &&
>+			if (bond->vlan_list.next->next == &bond->vlan_list &&
> 			    (bond->slave_cnt == 0)) {
>-				/* Last VLAN removed and no slaves, so
>+				/* Last VLAN removed (the only member of vlan_list
>+				 * is the special vid == 0 vlan) and no slaves, so
> 				 * restore block on adding VLANs. This will
> 				 * be removed once new slaves that are not
> 				 * VLAN challenged will be added.

	Could we do this instead in bond_release, when the last slave is
removed?  The CHALLENGED flag just prevents adding new VLANs; existing
ones would persist until a new slave was added.

	Since CHALLENGED slaves are in the minority these days (looks
like just IPoIB, one wimax and one obscure ethernet chipset) we could
even invert the logic: only assert CHALLENGED for the master when such a
slave is added.  We'd have to issue a NETDEV_CHANGEADDR when the master
picks up or releases its MAC address so the VLANs would pick it up, but
that's not a really big deal, and we probably ought to do that anyway.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

[Jiri Bohac]

Hi,

On Fri, Jun 03, 2011 at 05:26:51PM -0700, Jay Vosburgh wrote:
> Jiri Bohac <jbohac@suse.cz> wrote:
> 
> >Since commit ad1afb00, bond_del_vlan() never restores
> >NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
> >empty once the 8021q module is loaded, because the special VLAN 0
> >is always kept registered on the bond interface. Change the
> >condition to check if bond->vlan_list contains exactly one item
> >instead of checking for an empty list.
> >
> >Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 17b4dd9..4d317cd 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
> >
> > 			kfree(vlan);
> >
> >-			if (list_empty(&bond->vlan_list) &&
> >+			if (bond->vlan_list.next->next == &bond->vlan_list &&
> > 			    (bond->slave_cnt == 0)) {
> >-				/* Last VLAN removed and no slaves, so
> >+				/* Last VLAN removed (the only member of vlan_list
> >+				 * is the special vid == 0 vlan) and no slaves, so
> > 				 * restore block on adding VLANs. This will
> > 				 * be removed once new slaves that are not
> > 				 * VLAN challenged will be added.
> 
> 	Could we do this instead in bond_release, when the last slave is
> removed?  The CHALLENGED flag just prevents adding new VLANs; existing
> ones would persist until a new slave was added.

Actually, this has already happenned with commit b2a103e6. Sorry
I originally checked with an older kernel. This makes the above
patch obsolete and calls for a little cleanup (below).

b2a103e6 has changed the behaviour a little. Before, if you had a
vlan configured on a bond and removed the last slave, the
CHALLENGED flag would stay off, until you deleted the vlan.  Now,
CHALLENGED is turned on as soon as the last slave is deleted.

BTW, this can lead to a sort of inconsistent state, where CHALLENGED
is on and the bond has vlans.

> Since CHALLENGED slaves are in the minority these days (looks
> like just IPoIB, one wimax and one obscure ethernet chipset) we
> could even invert the logic: only assert CHALLENGED for the
> master when such a slave is added.

This sounds good. I don't understand why bonding currently tries
to prevent VLANs on an slave-less bond, and I might not be the
only one. Citing a comment in bond_fix_features():

	/* Disable adding VLANs to empty bond. But why? --mq */

Another comment, in bond_setup():

        /* At first, we block adding VLANs. That's the only way to
         * prevent problems that occur when adding VLANs over an
         * empty bond. The block will be removed once non-challenged
         * slaves are enslaved.
         */

Do we need to prevent this at all? What are the potential
problems?

Anyway a little cleanup:


bonding: clean up bond_del_vlan()

1) the setting of NETIF_F_VLAN_CHALLENGED in bond_del_vlan() is
useless since commit b2a103e6 because bond_fix_features() now
sets NETIF_F_VLAN_CHALLENGED whenever the last slave is being
removed.

2) the code never triggers anyway as vlan_list is never empty
since ad1afb00.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -329,16 +329,6 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 
 			kfree(vlan);
 
-			if (list_empty(&bond->vlan_list) &&
-			    (bond->slave_cnt == 0)) {
-				/* Last VLAN removed and no slaves, so
-				 * restore block on adding VLANs. This will
-				 * be removed once new slaves that are not
-				 * VLAN challenged will be added.
-				 */
-				bond->dev->features |= NETIF_F_VLAN_CHALLENGED;
-			}
-
 			res = 0;
 			goto out;
 		}

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

[Jay Vosburgh]

Jiri Bohac <jbohac@suse.cz> wrote:

>Hi,
>
>On Fri, Jun 03, 2011 at 05:26:51PM -0700, Jay Vosburgh wrote:
>> Jiri Bohac <jbohac@suse.cz> wrote:
>> 
>> >Since commit ad1afb00, bond_del_vlan() never restores
>> >NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
>> >empty once the 8021q module is loaded, because the special VLAN 0
>> >is always kept registered on the bond interface. Change the
>> >condition to check if bond->vlan_list contains exactly one item
>> >instead of checking for an empty list.
>> >
>> >Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 17b4dd9..4d317cd 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>> >
>> > 			kfree(vlan);
>> >
>> >-			if (list_empty(&bond->vlan_list) &&
>> >+			if (bond->vlan_list.next->next == &bond->vlan_list &&
>> > 			    (bond->slave_cnt == 0)) {
>> >-				/* Last VLAN removed and no slaves, so
>> >+				/* Last VLAN removed (the only member of vlan_list
>> >+				 * is the special vid == 0 vlan) and no slaves, so
>> > 				 * restore block on adding VLANs. This will
>> > 				 * be removed once new slaves that are not
>> > 				 * VLAN challenged will be added.
>> 
>> 	Could we do this instead in bond_release, when the last slave is
>> removed?  The CHALLENGED flag just prevents adding new VLANs; existing
>> ones would persist until a new slave was added.
>
>Actually, this has already happenned with commit b2a103e6. Sorry
>I originally checked with an older kernel. This makes the above
>patch obsolete and calls for a little cleanup (below).
>
>b2a103e6 has changed the behaviour a little. Before, if you had a
>vlan configured on a bond and removed the last slave, the
>CHALLENGED flag would stay off, until you deleted the vlan.  Now,
>CHALLENGED is turned on as soon as the last slave is deleted.
>
>BTW, this can lead to a sort of inconsistent state, where CHALLENGED
>is on and the bond has vlans.
>
>> Since CHALLENGED slaves are in the minority these days (looks
>> like just IPoIB, one wimax and one obscure ethernet chipset) we
>> could even invert the logic: only assert CHALLENGED for the
>> master when such a slave is added.
>
>This sounds good. I don't understand why bonding currently tries
>to prevent VLANs on an slave-less bond, and I might not be the
>only one. Citing a comment in bond_fix_features():
>
>	/* Disable adding VLANs to empty bond. But why? --mq */
>
>Another comment, in bond_setup():
>
>        /* At first, we block adding VLANs. That's the only way to
>         * prevent problems that occur when adding VLANs over an
>         * empty bond. The block will be removed once non-challenged
>         * slaves are enslaved.
>         */
>
>Do we need to prevent this at all? What are the potential
>problems?

	The problems have to do with the VLAN pseudo-device not getting
a MAC address if the bond master's MAC is all zeros when the VLAN is
added.

	The VLAN pseudo-device copies the underlying device's MAC at the
time the VLAN is added (in vlan_dev_init via register_netdevice), and
doesn't update it later if the underlying device's MAC address changes.
So we assert CHALLENGED until the bonding master has a MAC address (when
the first slave is added), to guarantee that there's a valid MAC in the
bond's dev_addr when the VLAN is added.

	If we relax this restriction (only enable CHALLENGED if an
actual CHALLENGED slave is added), then we also must add a CHANGEADDR
notifier when the bonding master picks up its MAC from the first slave
(which we do not currently do), and figure out how to get that initial
MAC assignment passed up to a VLAN device (or stack) atop bonding.
Currently, the vlan's event handler for CHANGEADDR, vlan_sync_address,
only tracks the underlying device's "real_dev_addr" but doesn't alter
the VLAN device's dev_addr.

	One obvious suggestion to resolve that is to have 8021q's
CHANGEADDR update the VLAN device's dev_addr to the underlying device's
dev_addr if the VLAN's is all zeroes.

>Anyway a little cleanup:
>
>
>bonding: clean up bond_del_vlan()
>
>1) the setting of NETIF_F_VLAN_CHALLENGED in bond_del_vlan() is
>useless since commit b2a103e6 because bond_fix_features() now
>sets NETIF_F_VLAN_CHALLENGED whenever the last slave is being
>removed.
>
>2) the code never triggers anyway as vlan_list is never empty
>since ad1afb00.
>
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>

	Just to be clear, this patch is unrelated to the discussion
above about changing the CHALLENGED behavior of bonding, and is really
just dead code removal.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

	-J

>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -329,16 +329,6 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>
> 			kfree(vlan);
>
>-			if (list_empty(&bond->vlan_list) &&
>-			    (bond->slave_cnt == 0)) {
>-				/* Last VLAN removed and no slaves, so
>-				 * restore block on adding VLANs. This will
>-				 * be removed once new slaves that are not
>-				 * VLAN challenged will be added.
>-				 */
>-				bond->dev->features |= NETIF_F_VLAN_CHALLENGED;
>-			}
>-
> 			res = 0;
> 			goto out;
> 		}
>
>-- 

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

[David Miller]

From: Jiri Bohac <jbohac@suse.cz>
Date: Fri, 10 Jun 2011 22:27:20 +0200

> bonding: clean up bond_del_vlan()
> 
> 1) the setting of NETIF_F_VLAN_CHALLENGED in bond_del_vlan() is
> useless since commit b2a103e6 because bond_fix_features() now
> sets NETIF_F_VLAN_CHALLENGED whenever the last slave is being
> removed.
> 
> 2) the code never triggers anyway as vlan_list is never empty
> since ad1afb00.
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

Applied, thanks.

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

[David Miller]

From: Jiri Bohac <jbohac@suse.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@suse.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.

Second, now even devices which don't ever get brought up will
have the VLAN ID 0 thing allocated.

Probably the thing to do is to remove the VLAN ID 0 entry on
NETDEV_DOWN.

Something like:

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index c7a581a..135019d 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -379,6 +379,14 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		dev->netdev_ops->ndo_vlan_rx_add_vid(dev, 0);
 	}
 
+	if ((event == NETDEV_DOWN) &&
+	    (dev->features & NETIF_F_HW_VLAN_FILTER) &&
+	    dev->netdev_ops->ndo_vlan_rx_kill_vid) {
+		pr_info("8021q: removing VLAN 0 from HW filter on device %s\n",
+			dev->name);
+		dev->netdev_ops->ndo_vlan_rx_kill_vid(dev, 0);
+	}
+
 	grp = rtnl_dereference(dev->vlgrp);
 	if (!grp)
 		goto out;

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

[Patrick McHardy]

On 05.06.2011 23:28, David Miller wrote:
> From: Jiri Bohac <jbohac@suse.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@suse.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.
> 

Good point.

I don't think this approach works very well at all since
some drivers don't do incremental updates, but iterate over
the registered VLAN group when constructing filters. The
group is not created until the first real VLAN device is
registered however.

Based on a quick grep (may have missed some):

- via_velocity, mlx4, starfire: will do nothing

- benet, igb, vxge, igbvf, ixgbevf, e1000e: would oops on
  rx_kill_vid due to unnecessary vlan_group_set_device()

The assumption of the drivers that a VLAN group exists
before the first VID is configured is reasonable in my
opinion, a lot of them also don't even configure VLAN
filtering until the VLAN group is registered.

So I think a good solution would be to make sure all
drivers don't enable VLAN filtering before the first
VLAN is actually registered and do the automatic
registration of VID 0 once the first real VLAN device
is created.

Also the code currently doesn't handle module unload:
regulary registered VLAN devices are removed through
rtnl_link, the manually registered VIDs need to be
removed manually.

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

[Jiri Bohac]

On Tue, Jun 07, 2011 at 05:17:27PM +0200, Patrick McHardy wrote:
> On 05.06.2011 23:28, David Miller wrote:
> > 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.
> > 
> 
> Good point.
> 
> I don't think this approach works very well at all since
> some drivers don't do incremental updates, but iterate over
> the registered VLAN group when constructing filters. The
> group is not created until the first real VLAN device is
> registered however.
> 
> Based on a quick grep (may have missed some):
> 
> - via_velocity, mlx4, starfire: will do nothing
> 
> - benet, igb, vxge, igbvf, ixgbevf, e1000e: would oops on
>   rx_kill_vid due to unnecessary vlan_group_set_device()

which approach do you mean? David's or mine? I suppose you mean
David's, because I did not call rx_kill_vid().

> The assumption of the drivers that a VLAN group exists
> before the first VID is configured is reasonable in my
> opinion, a lot of them also don't even configure VLAN
> filtering until the VLAN group is registered.

So this is broken already since ad1afb00 :(
The assumption broke the bonding driver and this got fixed by
f35188fa, btw.

> So I think a good solution would be to make sure all
> drivers don't enable VLAN filtering before the first
> VLAN is actually registered and do the automatic
> registration of VID 0 once the first real VLAN device
> is created.

But this behaviour is not what was intended by ad1afb00.
The VID 0 needs to be registered by default, to make 8021p work.
Even without any real VLAN devices created.

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

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

[Patrick McHardy]

On 07.06.2011 18:41, Jiri Bohac wrote:
> On Tue, Jun 07, 2011 at 05:17:27PM +0200, Patrick McHardy wrote:
>> On 05.06.2011 23:28, David Miller wrote:
>>> 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.
>>>
>>
>> Good point.
>>
>> I don't think this approach works very well at all since
>> some drivers don't do incremental updates, but iterate over
>> the registered VLAN group when constructing filters. The
>> group is not created until the first real VLAN device is
>> registered however.
>>
>> Based on a quick grep (may have missed some):
>>
>> - via_velocity, mlx4, starfire: will do nothing
>>
>> - benet, igb, vxge, igbvf, ixgbevf, e1000e: would oops on
>>   rx_kill_vid due to unnecessary vlan_group_set_device()
> 
> which approach do you mean? David's or mine? I suppose you mean
> David's, because I did not call rx_kill_vid().

Neither, the approach how adding VID 0 was implemented. At
least the first list of drivers mentioned above will do
nothing, the second one would oops if we removed the VID
on NETDEV_DOWN without adding a real VLAN device.

>> The assumption of the drivers that a VLAN group exists
>> before the first VID is configured is reasonable in my
>> opinion, a lot of them also don't even configure VLAN
>> filtering until the VLAN group is registered.
> 
> So this is broken already since ad1afb00 :(

Exactly.

> The assumption broke the bonding driver and this got fixed by
> f35188fa, btw.
> 
>> So I think a good solution would be to make sure all
>> drivers don't enable VLAN filtering before the first
>> VLAN is actually registered and do the automatic
>> registration of VID 0 once the first real VLAN device
>> is created.
> 
> But this behaviour is not what was intended by ad1afb00.
> The VID 0 needs to be registered by default, to make 8021p work.
> Even without any real VLAN devices created.

I'm not sure what exactly you're referring to. HW VLAN filters
are (or should be) usually only activated when the first
VLAN device is registered, so this change has no effect.

If drivers behave that way *and* the VID 0 is only registered
automatically when the first real device is configured,
this should provide the desired behaviour.

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

[Jiri Bohac]

Hi David,

On Sun, Jun 05, 2011 at 02:28:23PM -0700, David Miller wrote:
> From: Jiri Bohac <jbohac@suse.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@suse.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.

That would mean doing:

	ip link set down eth0
	ip link add link eth0 name eth0.1 type vlan id 1
	ip link set up eth0

... should result in a non-working setup, right? I would expect
an -EINVAL somewhere along the way.
However, at least for e1000, I just tested the above setup works.

Could you be wrong about this? Or is this supposed to fail with
other chips?  In that case, the vlan code or the drivers need
fixing. Something should either disallow adding vlans when down
and disallow putting the interface down then vlans are
configured, or it should re-register the VIDs on every NETDEV_UP.

> Second, now even devices which don't ever get brought up will
> have the VLAN ID 0 thing allocated.

Why is this a problem?
 
> Probably the thing to do is to remove the VLAN ID 0 entry on
> NETDEV_DOWN.

OK, if you prefer fixing it this way, why not...

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

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

[Jesse Gross]

On Tue, Jun 7, 2011 at 9:18 AM, Jiri Bohac <jbohac@suse.cz> wrote:
> Hi David,
>
> On Sun, Jun 05, 2011 at 02:28:23PM -0700, David Miller wrote:
>> From: Jiri Bohac <jbohac@suse.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@suse.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.

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

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Tue, 7 Jun 2011 18:25:23 -0700

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

Thanks for clearing all of this up Jesse.