Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)

[Jesper Dangaard Brouer <jdb@comx.dk>]

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

Hi Patrick,

Bisected me down to one of you changes
 commit 6c78dcbd47a68a
 [VLAN]: Fix promiscous/allmulti synchronization races

Description:
------------
 All other VLAN interfaces stop working, if a vlan is taken down
 (ifconfig eth1.1025 down) _while_ there is a tcpdump running on that
 interface.

 The problem is a result of promisc mode being removed on the
 real-device (eth1), when the vlan interface is taken down.  This
 should not happen as other vlan devices exists that still need
 promisc mode on the real-device (eth1).

Setup:
------
  eth1      - the real-device
  eth1.1013 - VLAN device
  eth1.1025 - VLAN device

 Both VLAN devices has assigned ether address 00:11:22:33:44:55 (which
 is different from the real-device).

Reproduce / test:
-----------------
Lookfor "promiscuous mode" changes in kern.log
# tail -n 40 -f /var/log/kern.log | grep "promiscuous mode"

Start a tcpdump on VLAN device
# tcpdump -ni eth1.1025

# LOG:
#   kernel: device eth1.1025 entered promiscuous mode

Take VLAN device down
# ifconfig eth1.1025 down

Tcpdump dies, the problem is that promisc mode is removed from both
the real-device and the VLAN device.  That should NOT happen, as there
are other VLAN devices up (eth1.1013).

# LOG:
#   kernel: device eth1.1025 left promiscuous mode
#   kernel: device eth1 left promiscuous mode

Bisect log: Attached

Commit 6c78dcbd47: Attached


See you in Paris!
-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk
  LinkedIn: http://www.linkedin.com/in/brouer

[-- Attachment #2: bisect03-found-bug.log --]
[-- Type: text/x-log, Size: 1706 bytes --]

git-bisect start
# good: [de46c33745f5e2ad594c72f2cf5f490861b16ce1] Linux 2.6.21
git-bisect good de46c33745f5e2ad594c72f2cf5f490861b16ce1
# bad: [bbf25010f1a6b761914430f5fca081ec8c7accd1] Linux 2.6.23
git-bisect bad bbf25010f1a6b761914430f5fca081ec8c7accd1
# good: [71ba22fa739029bb158144813b9e82c00326497c] Merge branch 'upstream-linus' of master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6
git-bisect good 71ba22fa739029bb158144813b9e82c00326497c
# bad: [2d9ce177e68645945e3366cfe2d66ee3c28cd4f2] i386: Allow KVM on i386 nonpae
git-bisect bad 2d9ce177e68645945e3366cfe2d66ee3c28cd4f2
# bad: [da58a1617343e345d435953a0f32024997a95164] /proc/*/environ: wrong placing of ptrace_may_attach() check
git-bisect bad da58a1617343e345d435953a0f32024997a95164
# good: [79e15ae424afa0a40b1a0c4478046d6ba0b71e20] dm: bio_list prefetch removal
git-bisect good 79e15ae424afa0a40b1a0c4478046d6ba0b71e20
# good: [e46662be7fddde3464bf208317542c2f8df13d0b] net/9p: change net/9p module name to 9pnet
git-bisect good e46662be7fddde3464bf208317542c2f8df13d0b
# bad: [b3b0b681b12478a7afa7d1f3d58be96830e16c7d] [TCP]: tcp probe add back ssthresh field
git-bisect bad b3b0b681b12478a7afa7d1f3d58be96830e16c7d
# bad: [6c78dcbd47a68a7d25d2bee7a6c74b9136cb5fde] [VLAN]: Fix promiscous/allmulti synchronization races
git-bisect bad 6c78dcbd47a68a7d25d2bee7a6c74b9136cb5fde
# good: [24023451c8df726692e2f52288a20870d13b501f] [NET]: Add net_device change_rx_mode callback
git-bisect good 24023451c8df726692e2f52288a20870d13b501f
# good: [a0a400d79e3dd7843e7e81baa3ef2957bdc292d0] [NET]: dev_mcast: add multicast list synchronization helpers
git-bisect good a0a400d79e3dd7843e7e81baa3ef2957bdc292d0

[-- Attachment #3: bisect03-found-bug.patch --]
[-- Type: text/x-patch, Size: 4902 bytes --]

commit 6c78dcbd47a68a7d25d2bee7a6c74b9136cb5fde
Author: Patrick McHardy <kaber@trash.net>
Date:   Sat Jul 14 18:52:56 2007 -0700

    [VLAN]: Fix promiscous/allmulti synchronization races
    
    The set_multicast_list function may be called without holding the rtnl
    mutex, resulting in races when changing the underlying device's promiscous
    and allmulti state. Use the change_rx_mode hook, which is always invoked
    under the rtnl.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 61a57dc..7f71df4 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -132,8 +132,6 @@ struct vlan_dev_info {
                                            * made, in order to feed the right changes down
                                            * to the real hardware...
                                            */
-	int old_allmulti;               /* similar to above. */
-	int old_promiscuity;            /* similar to above. */
 	struct net_device *real_dev;    /* the underlying device/interface */
 	unsigned char real_dev_addr[ETH_ALEN];
 	struct proc_dir_entry *dent;    /* Holds the proc data */
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index abb9900..39bdcc2 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -373,6 +373,7 @@ void vlan_setup(struct net_device *new_dev)
 	new_dev->open = vlan_dev_open;
 	new_dev->stop = vlan_dev_stop;
 	new_dev->set_multicast_list = vlan_dev_set_multicast_list;
+	new_dev->change_rx_flags = vlan_change_rx_flags;
 	new_dev->destructor = free_netdev;
 	new_dev->do_ioctl = vlan_dev_ioctl;
 
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index 62ce1c5..7df5b29 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -69,6 +69,7 @@ int vlan_dev_set_vlan_flag(const struct net_device *dev,
 			   u32 flag, short flag_val);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char *result);
 void vlan_dev_get_vid(const struct net_device *dev, unsigned short *result);
+void vlan_change_rx_flags(struct net_device *dev, int change);
 void vlan_dev_set_multicast_list(struct net_device *vlan_dev);
 
 int vlan_check_real_dev(struct net_device *real_dev, unsigned short vlan_id);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index d4a62d1..dec7e62 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -712,6 +712,11 @@ int vlan_dev_open(struct net_device *dev)
 	}
 	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
 
+	if (dev->flags & IFF_ALLMULTI)
+		dev_set_allmulti(real_dev, 1);
+	if (dev->flags & IFF_PROMISC)
+		dev_set_promiscuity(real_dev, 1);
+
 	return 0;
 }
 
@@ -721,6 +726,11 @@ int vlan_dev_stop(struct net_device *dev)
 
 	vlan_flush_mc_list(dev);
 
+	if (dev->flags & IFF_ALLMULTI)
+		dev_set_allmulti(real_dev, -1);
+	if (dev->flags & IFF_PROMISC)
+		dev_set_promiscuity(real_dev, -1);
+
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
 		dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
 
@@ -754,34 +764,26 @@ int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return err;
 }
 
+void vlan_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);
+}
+
 /** Taken from Gleb + Lennert's VLAN code, and modified... */
 void vlan_dev_set_multicast_list(struct net_device *vlan_dev)
 {
 	struct dev_mc_list *dmi;
 	struct net_device *real_dev;
-	int inc;
 
 	if (vlan_dev && (vlan_dev->priv_flags & IFF_802_1Q_VLAN)) {
 		/* Then it's a real vlan device, as far as we can tell.. */
 		real_dev = VLAN_DEV_INFO(vlan_dev)->real_dev;
 
-		/* compare the current promiscuity to the last promisc we had.. */
-		inc = vlan_dev->promiscuity - VLAN_DEV_INFO(vlan_dev)->old_promiscuity;
-		if (inc) {
-			printk(KERN_INFO "%s: dev_set_promiscuity(master, %d)\n",
-			       vlan_dev->name, inc);
-			dev_set_promiscuity(real_dev, inc); /* found in dev.c */
-			VLAN_DEV_INFO(vlan_dev)->old_promiscuity = vlan_dev->promiscuity;
-		}
-
-		inc = vlan_dev->allmulti - VLAN_DEV_INFO(vlan_dev)->old_allmulti;
-		if (inc) {
-			printk(KERN_INFO "%s: dev_set_allmulti(master, %d)\n",
-			       vlan_dev->name, inc);
-			dev_set_allmulti(real_dev, inc); /* dev.c */
-			VLAN_DEV_INFO(vlan_dev)->old_allmulti = vlan_dev->allmulti;
-		}
-
 		/* looking for addresses to add to master's list */
 		for (dmi = vlan_dev->mc_list; dmi != NULL; dmi = dmi->next) {
 			if (vlan_should_add_mc(dmi, VLAN_DEV_INFO(vlan_dev)->old_mc_list)) {