dev->promiscuity can become negative in specific bridge + vlan configuration

dev->promiscuity can become negative in specific bridge + vlan configuration

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

(Please CC me, I'm not subscribed)

Hi folks,

while debugging an issue on a wireless access point running Linux, I
encountered an issue with the promiscuity of interfaces when combining
vlans and bridges. When configuring them in a specific order, the
dev->promiscuity can become negative and the IFF_PROMISC flag will be
wrong.

I've reproduced this on an old 2.6.26 kernel as well as an
3.1.0-rc10-ish kernel from misc.git.

When a VLAN device is configured to be promiscuous, the underlying
physical device must be promiscuous as well. This is achieved in two
ways:

1. When a vlan device is brought up and its IFF_PROMISC flag is set, the
   promiscuity of the underlying interface is increased. When a vlan
   device is brought down and its IFF_PROMISC flag is set, the
   promiscuity of the underlying interface is decreased. This happens in
   vlan_dev_open() and vlan_dev_stop().
2. When the IFF_PROMISC flag changes on the vlan device, the promiscuity
   value of the underlying device is increased or decreased, depending
   on the value of the flag. This happens in vlan_dev_change_rx_flags().

However, 2. also happens when the interface is not up, which is
incorrect AFAICS. If a VLAN interface, which is promiscuous, is first
brought down and then has its promiscious flag reset, the promiscuity of
the underlying physical interface will be decreased twice.

This problem can be demonstrated with the following commands. It should
happen on any hardware and all recent (andn not-so-recent) kernels,
AFAICS. I've added the relevant kernel output and some comments inline
below.

$ vconfig add eth0 10
Added VLAN with VID == 10 to IF -:eth0:-
$ brctl addbr br0
$ ifconfig eth0.10 up
$ brctl addif br0 eth0.10
[14288.817391] device eth0.10 entered promiscuous mode
[14288.817397] device eth0 entered promiscuous mode
# eth0->promiscuity is now 1, as expected

$ ifconfig eth0.10 down
[14335.533019] device eth0 left promiscuous mode
# eth0->promiscuity is now 0, as expected

$ brctl delif br0 eth0.10
[14351.037666] device eth0.10 left promiscuous mode
# eth0->promiscuit is now -1!

$ brctl addif br0 eth0.10
[14383.549998] device eth0.10 entered promiscuous mode
# eth0->promiscuity is now 0, so eth0 is not entering promisciuous mode
# as it should

I've confirmed that the promiscuity actually gets set to -1 by some
added kernel prints on the 2.6.26 kernel, but the above behaviour is
also shown on the 3.1.0-rc10 kernel (which is consistent with
promiscuity diving below 0).

From looking at the code, I assume the same story applies for the
IFF_ALLMULTI flag, but I've not tested this.


I'm working on a (simple) patch to fix this issue, by simply not
updating the promiscuity of the underlying interface if the vlan
interface is down. I'll reply to this message with the patch after I've
finished and tested it.

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] vlan: Don't propagate flag changes on down interfaces.

[Matthijs Kooijman]

When (de)configuring a vlan interface, the IFF_ALLMULTI ans IFF_PROMISC
flags are cleared or set on the underlying interface. So, if these flags
are changed on a vlan interface that is not up, the flags underlying
interface might be set or cleared twice.

Only propagating flag changes when a device is up makes sure this does
not happen. It also makes sure that an underlying device is not set to
promiscuous or allmulti mode for a vlan device that is down.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 net/8021q/vlan_dev.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 9d40a07..0e72689 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -470,10 +470,12 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
 {
 	struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
 
-	if (change & IFF_ALLMULTI)
-		dev_set_allmulti(real_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
-	if (change & IFF_PROMISC)
-		dev_set_promiscuity(real_dev, dev->flags & IFF_PROMISC ? 1 : -1);
+	if (dev->flags & IFF_UP) {
+		if (change & IFF_ALLMULTI)
+			dev_set_allmulti(real_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
+		if (change & IFF_PROMISC)
+			dev_set_promiscuity(real_dev, dev->flags & IFF_PROMISC ? 1 : -1);
+	}
 }
 
 static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
-- 
1.7.7

Re: [PATCH] vlan: Don't propagate flag changes on down interfaces.

[David Miller]

From: Matthijs Kooijman <matthijs@stdin.nl>
Date: Mon, 31 Oct 2011 15:53:13 +0100

> When (de)configuring a vlan interface, the IFF_ALLMULTI ans IFF_PROMISC
> flags are cleared or set on the underlying interface. So, if these flags
> are changed on a vlan interface that is not up, the flags underlying
> interface might be set or cleared twice.
> 
> Only propagating flag changes when a device is up makes sure this does
> not happen. It also makes sure that an underlying device is not set to
> promiscuous or allmulti mode for a vlan device that is down.
> 
> Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Applied, thanks.