netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
@ 2008-09-26 14:00 Jesper Dangaard Brouer
  2008-09-26 16:14 ` Patrick McHardy
  0 siblings, 1 reply; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2008-09-26 14:00 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev@vger.kernel.org

[-- 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)) {

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 14:00 Bisect'ed BUG in VLAN promisc mode (6c78dcbd47) Jesper Dangaard Brouer
@ 2008-09-26 16:14 ` Patrick McHardy
  2008-09-26 19:22   ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 12+ messages in thread
From: Patrick McHardy @ 2008-09-26 16:14 UTC (permalink / raw)
  To: jdb; +Cc: netdev@vger.kernel.org

Jesper Dangaard Brouer wrote:
> 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).

I'm pretty sure we fixed this already in commit 0ed21b32.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 16:14 ` Patrick McHardy
@ 2008-09-26 19:22   ` Jesper Dangaard Brouer
  2008-09-26 19:24     ` Patrick McHardy
  0 siblings, 1 reply; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2008-09-26 19:22 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev@vger.kernel.org

On Fri, 2008-09-26 at 18:14 +0200, Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
> > 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).
> 
> I'm pretty sure we fixed this already in commit 0ed21b32.

I think that I have tested including this commit.  The first thing I did
was to test with latest DaveM net-2.6 git tree.

I'll test it later... perhaps during the hacking days...

Need to pack.. See you soon!
-- 
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:22   ` Jesper Dangaard Brouer
@ 2008-09-26 19:24     ` Patrick McHardy
  2008-09-26 19:28       ` Patrick McHardy
  0 siblings, 1 reply; 12+ messages in thread
From: Patrick McHardy @ 2008-09-26 19:24 UTC (permalink / raw)
  To: jdb; +Cc: netdev@vger.kernel.org

Jesper Dangaard Brouer wrote:
> On Fri, 2008-09-26 at 18:14 +0200, Patrick McHardy wrote:
>> Jesper Dangaard Brouer wrote:
>>> 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).
>> I'm pretty sure we fixed this already in commit 0ed21b32.
> 
> I think that I have tested including this commit.  The first thing I did
> was to test with latest DaveM net-2.6 git tree.

Let me see if I can reproduce it ...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:24     ` Patrick McHardy
@ 2008-09-26 19:28       ` Patrick McHardy
  2008-09-26 19:34         ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 12+ messages in thread
From: Patrick McHardy @ 2008-09-26 19:28 UTC (permalink / raw)
  To: jdb; +Cc: netdev@vger.kernel.org

Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
>> On Fri, 2008-09-26 at 18:14 +0200, Patrick McHardy wrote:
>>> Jesper Dangaard Brouer wrote:
>>>> 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).
>>> I'm pretty sure we fixed this already in commit 0ed21b32.
>>
>> I think that I have tested including this commit.  The first thing I did
>> was to test with latest DaveM net-2.6 git tree.
> 
> Let me see if I can reproduce it ...

Actually - one question: you're saying you're using different MAC
addresses on the VLAN devices, so I guess thats why you're expecting
the underlying device to still be in promiscous mode after you set
eth1.1025 down. For devices that support multiple unicast addresses
in hardware, we don't put the device in promiscous mode anymore.
So the question is: is something actually not working, or did you
just notice that the real device is no longer in promiscous mode?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:28       ` Patrick McHardy
@ 2008-09-26 19:34         ` Jesper Dangaard Brouer
  2008-09-26 19:41           ` Patrick McHardy
  0 siblings, 1 reply; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2008-09-26 19:34 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev@vger.kernel.org

On Fri, 2008-09-26 at 21:28 +0200, Patrick McHardy wrote:
> Patrick McHardy wrote:
> > Jesper Dangaard Brouer wrote:
> >> On Fri, 2008-09-26 at 18:14 +0200, Patrick McHardy wrote:
> >>> Jesper Dangaard Brouer wrote:
> >>>> 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).
> >>> I'm pretty sure we fixed this already in commit 0ed21b32.
> >>
> >> I think that I have tested including this commit.  The first thing I did
> >> was to test with latest DaveM net-2.6 git tree.
> > 
> > Let me see if I can reproduce it ...
> 
> Actually - one question: you're saying you're using different MAC
> addresses on the VLAN devices, so I guess thats why you're expecting
> the underlying device to still be in promiscous mode after you set
> eth1.1025 down. For devices that support multiple unicast addresses
> in hardware, we don't put the device in promiscous mode anymore.
> So the question is: is something actually not working, or did you
> just notice that the real device is no longer in promiscous mode?

It stopped working!  In the test setup I do have a machine connected to
eth1.1013, where I have a ping running, that stop working...

I use the tg3 driver for both netcards. Don't know if it supports it?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:34         ` Jesper Dangaard Brouer
@ 2008-09-26 19:41           ` Patrick McHardy
  2008-09-26 20:10             ` Patrick McHardy
  2008-10-02 16:38             ` Jesper Dangaard Brouer
  0 siblings, 2 replies; 12+ messages in thread
From: Patrick McHardy @ 2008-09-26 19:41 UTC (permalink / raw)
  To: jdb; +Cc: netdev@vger.kernel.org

Jesper Dangaard Brouer wrote:
> On Fri, 2008-09-26 at 21:28 +0200, Patrick McHardy wrote:
>> Actually - one question: you're saying you're using different MAC
>> addresses on the VLAN devices, so I guess thats why you're expecting
>> the underlying device to still be in promiscous mode after you set
>> eth1.1025 down. For devices that support multiple unicast addresses
>> in hardware, we don't put the device in promiscous mode anymore.
>> So the question is: is something actually not working, or did you
>> just notice that the real device is no longer in promiscous mode?
> 
> It stopped working!  In the test setup I do have a machine connected to
> eth1.1013, where I have a ping running, that stop working...

Found it in the bugreport :) OK, I'll try to reproduce it now.

> I use the tg3 driver for both netcards. Don't know if it supports it?

The driver doesn't support it.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:41           ` Patrick McHardy
@ 2008-09-26 20:10             ` Patrick McHardy
  2008-10-02 16:38             ` Jesper Dangaard Brouer
  1 sibling, 0 replies; 12+ messages in thread
From: Patrick McHardy @ 2008-09-26 20:10 UTC (permalink / raw)
  To: jdb; +Cc: netdev@vger.kernel.org

Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
>> On Fri, 2008-09-26 at 21:28 +0200, Patrick McHardy wrote:
>>> Actually - one question: you're saying you're using different MAC
>>> addresses on the VLAN devices, so I guess thats why you're expecting
>>> the underlying device to still be in promiscous mode after you set
>>> eth1.1025 down. For devices that support multiple unicast addresses
>>> in hardware, we don't put the device in promiscous mode anymore.
>>> So the question is: is something actually not working, or did you
>>> just notice that the real device is no longer in promiscous mode?
>>
>> It stopped working!  In the test setup I do have a machine connected to
>> eth1.1013, where I have a ping running, that stop working...
> 
> Found it in the bugreport :) OK, I'll try to reproduce it now.

Found it without reproduing, but unfortunately I also have to leave
now, will look at it later again.

Anyways, the problem appears to be that the promiscous count is
decremented twice for the VLAN device, once in vlan_stop() because
the device is still in promiscous mode, once when af_packet takes
the VLAN device out of promisc in the NETDEV_UNREGISTER notifier
chain, which triggers the VLAN ->change_rx_mode callback and
removes another promiscous count from the real device.

I think the correct fix would be to not invoke ->change_rx_flags
while the device is down, similar to ->set_multicast_list and
->set_rx_mode, but I need to check the other drivers first.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-09-26 19:41           ` Patrick McHardy
  2008-09-26 20:10             ` Patrick McHardy
@ 2008-10-02 16:38             ` Jesper Dangaard Brouer
  2008-10-06 11:03               ` Patrick McHardy
  1 sibling, 1 reply; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2008-10-02 16:38 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: Jesper Dangaard Brouer, netdev@vger.kernel.org


Here is a quick fix for the bug...
Patrick promised he would do a more clean fix later ;-)

commit 356d09baea53abb7f6a6aabf2264ba104cdb9b5b
Author: Jesper Dangaard Brouer <hawk@comx.dk>
Date:   Thu Oct 2 12:24:19 2008 +0200

     Quick fix from Patrick to solve the VLAN promisc mode bug.

      Bisect'ed BUG in VLAN promisc mode

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

      Need promisc mode because VLAN devices uses another ether address.

     Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4bf014e..a79daee 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -551,6 +551,8 @@ 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 (!netif_running(dev))
+		return;
  	if (change & IFF_ALLMULTI)
  		dev_set_allmulti(real_dev, dev->flags & IFF_ALLMULTI ? 1 : -1);
  	if (change & IFF_PROMISC)

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-10-02 16:38             ` Jesper Dangaard Brouer
@ 2008-10-06 11:03               ` Patrick McHardy
  2008-10-07 11:03                 ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 12+ messages in thread
From: Patrick McHardy @ 2008-10-06 11:03 UTC (permalink / raw)
  To: Jesper Dangaard Brouer; +Cc: Jesper Dangaard Brouer, netdev@vger.kernel.org

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

Jesper Dangaard Brouer wrote:
> 
> Here is a quick fix for the bug...
> Patrick promised he would do a more clean fix later ;-)

And here it is. Could you verify that it also fixes the problem?
Thanks!



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2701 bytes --]

commit 79a95e393b63c26f270c676075b95c9421d3faba
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Oct 6 13:01:26 2008 +0200

    net: only invoke dev->change_rx_flags when device is UP
    
    Jesper Dangaard Brouer <hawk@comx.dk> reported a bug when setting a VLAN
    device down that is in promiscous mode:
    
    When the VLAN device is set down, the promiscous count on the real
    device is decremented by one by vlan_dev_stop(). When removing the
    promiscous flag from the VLAN device afterwards, the promiscous
    count on the real device is decremented a second time by the
    vlan_change_rx_flags() callback.
    
    The root cause for this is that the ->change_rx_flags() callback is
    invoked while the device is down. The synchronization is meant to mirror
    the behaviour of the ->set_rx_mode callbacks, meaning the ->open function
    is responsible for doing a full sync on open, the ->close() function is
    responsible for doing full cleanup on ->stop() and ->change_rx_flags()
    is meant to do incremental changes while the device is UP.
    
    Only invoke ->change_rx_flags() while the device is UP to provide the
    intended behaviour.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/core/dev.c b/net/core/dev.c
index e8eb2b4..fd992c0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2918,6 +2918,12 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
 	return 0;
 }
 
+static void dev_change_rx_flags(struct net_device *dev, int flags)
+{
+	if (dev->flags & IFF_UP && dev->change_rx_flags)
+		dev->change_rx_flags(dev, flags);
+}
+
 static int __dev_set_promiscuity(struct net_device *dev, int inc)
 {
 	unsigned short old_flags = dev->flags;
@@ -2955,8 +2961,7 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc)
 				current->uid, current->gid,
 				audit_get_sessionid(current));
 
-		if (dev->change_rx_flags)
-			dev->change_rx_flags(dev, IFF_PROMISC);
+		dev_change_rx_flags(dev, IFF_PROMISC);
 	}
 	return 0;
 }
@@ -3022,8 +3027,7 @@ int dev_set_allmulti(struct net_device *dev, int inc)
 		}
 	}
 	if (dev->flags ^ old_flags) {
-		if (dev->change_rx_flags)
-			dev->change_rx_flags(dev, IFF_ALLMULTI);
+		dev_change_rx_flags(dev, IFF_ALLMULTI);
 		dev_set_rx_mode(dev);
 	}
 	return 0;
@@ -3347,8 +3351,8 @@ int dev_change_flags(struct net_device *dev, unsigned flags)
 	 *	Load in the correct multicast list now the flags have changed.
 	 */
 
-	if (dev->change_rx_flags && (old_flags ^ flags) & IFF_MULTICAST)
-		dev->change_rx_flags(dev, IFF_MULTICAST);
+	if ((old_flags ^ flags) & IFF_MULTICAST)
+		dev_change_rx_flags(dev, IFF_MULTICAST);
 
 	dev_set_rx_mode(dev);
 

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-10-06 11:03               ` Patrick McHardy
@ 2008-10-07 11:03                 ` Jesper Dangaard Brouer
  2008-10-07 11:10                   ` Patrick McHardy
  0 siblings, 1 reply; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2008-10-07 11:03 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: Jesper Dangaard Brouer, netdev@vger.kernel.org

On Mon, 2008-10-06 at 13:03 +0200, Patrick McHardy wrote:
> Jesper Dangaard Brouer wrote:
> > 
> > Patrick promised he would do a more clean fix later ;-)
> 
> And here it is. Could you verify that it also fixes the problem?

It works and fixes the problem -- thanks!

Tested-by: Jesper Dangaard Brouer <jdb@comx.dk>

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Bisect'ed BUG in VLAN promisc mode (6c78dcbd47)
  2008-10-07 11:03                 ` Jesper Dangaard Brouer
@ 2008-10-07 11:10                   ` Patrick McHardy
  0 siblings, 0 replies; 12+ messages in thread
From: Patrick McHardy @ 2008-10-07 11:10 UTC (permalink / raw)
  To: jdb; +Cc: Jesper Dangaard Brouer, netdev@vger.kernel.org

Jesper Dangaard Brouer wrote:
> On Mon, 2008-10-06 at 13:03 +0200, Patrick McHardy wrote:
>> Jesper Dangaard Brouer wrote:
>>> Patrick promised he would do a more clean fix later ;-)
>> And here it is. Could you verify that it also fixes the problem?
> 
> It works and fixes the problem -- thanks!
> 
> Tested-by: Jesper Dangaard Brouer <jdb@comx.dk>

Thanks Jesper.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2008-10-07 11:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-26 14:00 Bisect'ed BUG in VLAN promisc mode (6c78dcbd47) Jesper Dangaard Brouer
2008-09-26 16:14 ` Patrick McHardy
2008-09-26 19:22   ` Jesper Dangaard Brouer
2008-09-26 19:24     ` Patrick McHardy
2008-09-26 19:28       ` Patrick McHardy
2008-09-26 19:34         ` Jesper Dangaard Brouer
2008-09-26 19:41           ` Patrick McHardy
2008-09-26 20:10             ` Patrick McHardy
2008-10-02 16:38             ` Jesper Dangaard Brouer
2008-10-06 11:03               ` Patrick McHardy
2008-10-07 11:03                 ` Jesper Dangaard Brouer
2008-10-07 11:10                   ` Patrick McHardy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).