mac80211: Fix TX after monitor interface is converted to managed

PATCH In review Daniel Drake 10 participants 21 messages Oct 4, 2007 – Oct 6, 2007 7 messages not in the archive

Subsystems (3)

Networking Network drivers mac80211

lore

Share & tools

lore

Daniel Drake

author

2007-10-04 11:33 UTC #0 lore raw

PATCH 1/1 mac80211: Fix TX after monitor interface is converted to managed

This sequence of events causes loss of connectivity:

<plug in>
<associate as normal in managed mode>
ifconfig eth7 down
iwconfig eth7 mode monitor
ifconfig eth7 up
ifconfig eth7 down
iwconfig eth7 mode managed
<associate as normal>

At this point you are associated but TX does not work. This is because the eth7 hard_start_xmit is still ieee80211_monitor_start_xmit.

Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.

Signed-off-by Daniel Drake <[email protected]>

---
 net/mac80211/ieee80211_iface.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

net/mac80211/ieee80211_iface.c +3 -0

index 08c1e18..40d4b63 100644
@@ -242,6 +242,9 @@ void ieee80211_if_reinit(struct net_device *dev)
 
 	ieee80211_if_sdata_deinit(sdata);
 
+	BUG_ON(netif_running(dev));
+	dev->hard_start_xmit = NULL;
+
 	switch (sdata->type) {
 	case IEEE80211_IF_TYPE_MGMT:
 		/* nothing to do */

Signature · 1 line

1.5.3.3

Michael Wu

2007-10-04 14:36 UTC #1 lore raw

Quoted 4 lines · Daniel Drake

Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.

Well.. this kinda sucks, but we can clean up the logic here later.

net/mac80211/ieee80211_iface.c void ieee80211_if_reinit(struct net_device *dev) · line 245

Quoted 1 line

net/mac80211/ieee80211_iface.c +1 -0
+	BUG_ON(netif_running(dev));

This will never happen, so there's no point.

ACK with that bit removed.

Thanks,
-Michael Wu

Michael Buesch

2007-10-04 15:06 UTC #2 lore raw

Quoted 9 lines · On Thursday 04 October 2007 16:34:43 Michael Wu wrote:

Quoted 4 lines · Daniel Drake
Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.
Well.. this kinda sucks, but we can clean up the logic here later.
net/mac80211/ieee80211_iface.c void ieee80211_if_reinit(struct net_device *dev) · line 245
Quoted 1 line
net/mac80211/ieee80211_iface.c +1 -0
+	BUG_ON(netif_running(dev));
This will never happen, so there's no point.

The reason why BUG_ON exists is to catch bugs that happen, although they Should Never Happen (tm) ;)

Signature · 1 line

Greetings Michael.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael Wu

2007-10-04 15:14 UTC #4 lore raw

Quoted 2 lines · Michael Buesch

The reason why BUG_ON exists is to catch bugs that happen, although they Should Never Happen (tm) ;)

This is just paranoia. There's plenty of other BUG_ONs which we use to catch bugs caused by drivers doing silly things. We can verify that this condition will never occur within the mac80211 layer, so there's no need to have it. The only thing this can catch is someone deciding to manually invoke dev->uninit, which only the unregister code should be doing.

-Michael Wu

John W. Linville

2007-10-04 15:38 UTC #5 lore raw

In reply to Michael Buesch

Quoted 6 lines · Michael Buesch

Quoted 5 lines · On Thursday 04 October 2007 16:34:43 Michael Wu wrote:
Quoted 4 lines · Daniel Drake
Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.

Quoted 7 lines

Quoted 4 lines
Well.. this kinda sucks, but we can clean up the logic here later.
net/mac80211/ieee80211_iface.c void ieee80211_if_reinit(struct net_device *dev) · line 245
Quoted 1 line
net/mac80211/ieee80211_iface.c +1 -0
+	BUG_ON(netif_running(dev));
This will never happen, so there's no point.
The reason why BUG_ON exists is to catch bugs that happen, although they Should Never Happen (tm) ;)

Precisely.

Signature · 2 lines

John W. Linville
[email protected]

Michael Wu

2007-10-04 17:13 UTC #6 lore raw

Quoted 4 lines · John W. Linville

Quoted 2 lines
The reason why BUG_ON exists is to catch bugs that happen, although they Should Never Happen (tm) ;)
Precisely.

No really, this bug will never happen. This is function is merely a helper function which is called from interface removal code (where the interface *has* to be down) or from changing the interface type (which ensures that the interface is down first). There are an unlimited number of bugs which Should Never Happen. That doesn't mean we should start adding BUG_ONs for every single one of them. That gives some sort of protection against cosmic rays flipping bits, but down here on earth, it's bloat.

-Michael Wu

John W. Linville

2007-10-04 18:38 UTC #7 lore raw

Quoted 6 lines · Michael Wu

Quoted 4 lines · John W. Linville
Quoted 2 lines
The reason why BUG_ON exists is to catch bugs that happen, although they Should Never Happen (tm) ;)
Precisely.

Quoted 7 lines

No really, this bug will never happen. This is function is merely a helper function which is called from interface removal code (where the interface *has* to be down) or from changing the interface type (which ensures that the interface is down first). There are an unlimited number of bugs which Should Never Happen. That doesn't mean we should start adding BUG_ONs for every single one of them. That gives some sort of protection against cosmic rays flipping bits, but down here on earth, it's bloat.

Falling back on bloat as an argument against a BUG_ON in a configuration path seems a bit weak. :-)

Programming with assertions (and BUG_ON is a form of that) is generally a good practice. Almost any book or other source on good programming practices will agree. Yes, it can be overdone. But I don't really think that is the case here, since the check is relatively inexpensive and the consequence should it ever *somehow* happen could be a something wierd (crash, corruption, etc) w/o any other indication of what occured.

Anyway, the point is probably moot in this case if there is no great objection to the alternative patch I proposed.

John

Signature · 2 lines

John W. Linville
[email protected]

Michael Wu

2007-10-04 21:18 UTC #8 lore raw

Quoted 3 lines · John W. Linville

Falling back on bloat as an argument against a BUG_ON in a configuration path seems a bit weak. :-)

Seems strong to me. Bloat slows me down and distracts me from what code really needs to do. Bloat is an indication that one does not understand what the code really needs to do.

Quoted 8 lines

Programming with assertions (and BUG_ON is a form of that) is generally a good practice. Almost any book or other source on good programming practices will agree. Yes, it can be overdone. But I don't really think that is the case here, since the check is relatively inexpensive and the consequence should it ever *somehow* happen could be a something wierd (crash, corruption, etc) w/o any other indication of what occured.

A line has to be drawn somewhere. There's about a billion places where we can add checks like this because if an assumption should ever break, the code will break into pieces. However, we don't, and there's no reason to do so here other than to needlessly add code just because it makes you feel safer.

-Michael Wu

Roland Dreier

2007-10-04 21:31 UTC #9 lore raw

In reply to John W. Linville

Quoted 7 lines

Programming with assertions (and BUG_ON is a form of that) is generally a good practice. Almost any book or other source on good programming practices will agree. Yes, it can be overdone. But I don't really think that is the case here, since the check is relatively inexpensive and the consequence should it ever *somehow* happen could be a something wierd (crash, corruption, etc) w/o any other indication of what occured.

The problem with BUG_ON is that it kills the whole system. So every time you add a BUG_ON into code, you have to weigh whether the problem you detected is so severe that the right response is to panic. For example, I can see panicking on something fundamental like corrupted page tables. However I would submit that the wireless stack should *never* use BUG_ON -- printing a warning and trying to limp on seems preferable to me.

- R.

John W. Linville

2007-10-04 22:39 UTC #10 lore raw

Quoted 10 lines · Roland Dreier

Quoted 2 lines
Programming with assertions (and BUG_ON is a form of that) is generally a good practice. Almost any book or other source on
The problem with BUG_ON is that it kills the whole system. So every time you add a BUG_ON into code, you have to weigh whether the problem you detected is so severe that the right response is to panic. For example, I can see panicking on something fundamental like corrupted page tables. However I would submit that the wireless stack should *never* use BUG_ON -- printing a warning and trying to limp on seems preferable to me.

OK, I'll buy that as an argument to use WARN_ON instead of BUG_ON. But it doesn't invalidate the desire to have some sort of assertion.

John

Signature · 2 lines

John W. Linville
[email protected]

Johannes Berg

maintainer

2007-10-04 18:12 UTC #11 lore raw

In reply to Michael Wu

Quoted 6 lines · Michael Wu

Quoted 4 lines · Daniel Drake
Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.
Well.. this kinda sucks, but we can clean up the logic here later.

I kinda agree, but the hack in 50e1f4d76a9d45c940733f05ccd42c5bbe6ca132 which caused this was is hack too. Somebody really need to rewrite the whole interface handling, it's a total mess with the master being specially treated but initialised by the same functions etc. I think we should start having a IEEE80211_IF_TYPE_MASTER for this, maybe then things will fall into place nicer.

johannes

Stephen Hemminger

2007-10-04 15:56 UTC #12 lore raw

In reply to Daniel Drake

Quoted 11 lines · On Thu, 4 Oct 2007 12:33:43 +0100 (BST) Daniel Drake <[email protected]> wrote:

This sequence of events causes loss of connectivity:
<plug in>
<associate as normal in managed mode>
ifconfig eth7 down
iwconfig eth7 mode monitor
ifconfig eth7 up
ifconfig eth7 down
iwconfig eth7 mode managed
<associate as normal>

Quoted 8 lines

At this point you are associated but TX does not work. This is because the eth7 hard_start_xmit is still ieee80211_monitor_start_xmit.
Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.
Signed-off-by Daniel Drake <[email protected]>

Playing with the function pointer is a awkward way to do this. Shouldn't the state management flags be used instead (dormant, running, stop/wake)... I am concerned about races and dereferencing the NULL ptr.

Signature · 1 line

Stephen Hemminger <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20071004085440.008216c6-s08KbqtN0aBORcJjwVk881hTQxXnIo14@public.gmane.org

Michael Wu

2007-10-04 16:56 UTC #14 lore raw

Quoted 3 lines · Stephen Hemminger

Playing with the function pointer is a awkward way to do this. Shouldn't the state management flags be used instead (dormant, running, stop/wake)... I am concerned about races and dereferencing the NULL ptr.

This is all under RTNL and the pointer is only modified while the interface is down.

-Michael Wu

John W. Linville

2007-10-04 18:38 UTC #15 lore raw

In reply to Daniel Drake

Quoted 3 lines · Daniel Drake

Fix this by unsetting the hard_start_xmit handler in ieee80211_if_reinit. It will then be reinitialised to the default (ieee80211_subif_start_xmit) in ieee80211_if_set_type.

I think I'd rather fix this by making the check in ieee80211_if_set_type more explicit for master devices. Patch to follow.

John

Signature · 2 lines

John W. Linville
[email protected]

John W. Linville

2007-10-04 18:38 UTC #16 lore raw

PATCH 1/1 ieee80211_if_set_type: make check for master dev more explicit

In reply to Daniel Drake

Problem description by Daniel Drake <[email protected]>:

"This sequence of events causes loss of connectivity:

<plug in>
<associate as normal in managed mode>
ifconfig eth7 down
iwconfig eth7 mode monitor
ifconfig eth7 up
ifconfig eth7 down
iwconfig eth7 mode managed
<associate as normal>

At this point you are associated but TX does not work. This is because the eth7 hard_start_xmit is still ieee80211_monitor_start_xmit."

The problem is caused by ieee80211_if_set_type checking for a non-zero hard_start_xmit pointer value in order to avoid changing that value for master devices. The fix is to make that check more explicitly linked to master devices rather than simply checking if the value has been previously set.

CC Daniel Drake <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
 net/mac80211/ieee80211_iface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/mac80211/ieee80211_iface.c +1 -1

index be7e77f..6607b80 100644
@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
 	 * which already has a hard_start_xmit routine assigned
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev->type != ARPHRD_IEEE80211)
 		dev->hard_start_xmit = ieee80211_subif_start_xmit;
 
 	/*

Signature · 5 lines

1.5.2.4

-- 
John W. Linville
[email protected]

Johannes Berg

maintainer

2007-10-04 18:44 UTC #17 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

net/mac80211/ieee80211_iface.c void ieee80211_if_set_type(struct net_device *dev, int type) · line 110

Quoted 9 lines · John W. Linville

net/mac80211/ieee80211_iface.c +1 -1

@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
 	 * which already has a hard_start_xmit routine assigned
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev->type != ARPHRD_IEEE80211)
 		dev->hard_start_xmit = ieee80211_subif_start_xmit;

That should work as well although I think you should update the comment above :)

johannes

A message that is not in the archive (sent off-list or not yet received): [email protected]

John W. Linville

2007-10-04 19:13 UTC #19 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Quoted 14 lines · Johannes Berg

net/mac80211/ieee80211_iface.c void ieee80211_if_set_type(struct net_device *dev, int type) · line 110
Quoted 9 lines · John W. Linville
net/mac80211/ieee80211_iface.c +1 -1
@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
 	 * which already has a hard_start_xmit routine assigned
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev->type != ARPHRD_IEEE80211)
 		dev->hard_start_xmit = ieee80211_subif_start_xmit;
That should work as well although I think you should update the comment above :)

The comment still seem applicable. How would you word it?

Signature · 2 lines

John W. Linville
[email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johannes Berg

2007-10-05 12:01 UTC #21 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Quoted 4 lines · John W. Linville

Quoted 2 lines
That should work as well although I think you should update the comment above :)
The comment still seem applicable. How would you word it?

Whoops, you're right, I misremembered explaining the use of the pointer.

johannes

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael Wu

2007-10-04 21:26 UTC #23 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

net/mac80211/ieee80211_iface.c void ieee80211_if_set_type(struct net_device *dev, int type) · line 109

Quoted 10 lines · John W. Linville

(quoted diff) +0 -0
b/net/mac80211/ieee80211_iface.c index be7e77f..6607b80 100644
net/mac80211/ieee80211_iface.c +0 -0
@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int
type) * which already has a hard_start_xmit routine assigned
net/mac80211/ieee80211_iface.c +1 -1
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev->type != ARPHRD_IEEE80211)

The standard way of checking for the master device is dev == sdata->local->mdev

wme.c doesn't quite follow this but that code needs to die anyway.

This does look nicer than the other patch.

-Michael Wu

A message that is not in the archive (sent off-list or not yet received): 200710041726.11744.flamingice-R9e9/[email protected]

John W. Linville

2007-10-04 23:02 UTC #25 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Quoted 14 lines · Michael Wu

net/mac80211/ieee80211_iface.c
Quoted 10 lines · John W. Linville
(quoted diff) +0 -0
b/net/mac80211/ieee80211_iface.c index be7e77f..6607b80 100644
net/mac80211/ieee80211_iface.c +0 -0
@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int
type) * which already has a hard_start_xmit routine assigned
net/mac80211/ieee80211_iface.c +1 -1
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev->type != ARPHRD_IEEE80211)
The standard way of checking for the master device is dev == sdata->local->mdev

Quoted 3 lines

wme.c doesn't quite follow this but that code needs to die anyway.
This does look nicer than the other patch.

Alright...better?

---

From: John W. Linville <[email protected]>
Subject: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Problem description by Daniel Drake <[email protected]>:

"This sequence of events causes loss of connectivity:

<plug in>
<associate as normal in managed mode>
ifconfig eth7 down
iwconfig eth7 mode monitor
ifconfig eth7 up
ifconfig eth7 down
iwconfig eth7 mode managed
<associate as normal>

At this point you are associated but TX does not work. This is because the eth7 hard_start_xmit is still ieee80211_monitor_start_xmit."

CC Daniel Drake <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
 net/mac80211/ieee80211_iface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/mac80211/ieee80211_iface.c +1 -1

index be7e77f..43e505d 100644
@@ -106,7 +106,7 @@ void ieee80211_if_set_type(struct net_device *dev, int type)
 	 * which already has a hard_start_xmit routine assigned
 	 * which must not be changed.
 	 */
-	if (!dev->hard_start_xmit)
+	if (dev != sdata->local->mdev)
 		dev->hard_start_xmit = ieee80211_subif_start_xmit;
 
 	/*

Signature · 6 lines

1.5.2.4


-- 
John W. Linville
[email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael Wu

Acked-by

2007-10-06 02:23 UTC #27 lore raw

Re: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Quoted 2 lines · John W. Linville

Alright...better?

Yup, thanks.

Quoted 8 lines

The problem is caused by ieee80211_if_set_type checking for a non-zero hard_start_xmit pointer value in order to avoid changing that value for master devices. The fix is to make that check more explicitly linked to master devices rather than simply checking if the value has been previously set.
CC Daniel Drake <[email protected]>
Signed-off-by John W. Linville <[email protected]>

Acked-by Michael Wu <flamingice-R9e9/[email protected]>

-Michael Wu