Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Wu]

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

On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> 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.

> +	BUG_ON(netif_running(dev));
This will never happen, so there's no point.

ACK with that bit removed.

Thanks,
-Michael Wu

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Buesch]

On Thursday 04 October 2007 16:34:43 Michael Wu wrote:
> On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > 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.
> 
> > +	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) ;)

-- 
Greetings Michael.

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Wu]

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

On Thursday 04 October 2007 11:06, Michael Buesch wrote:
> 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

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[John W. Linville]

On Thu, Oct 04, 2007 at 05:06:05PM +0200, Michael Buesch wrote:
> On Thursday 04 October 2007 16:34:43 Michael Wu wrote:
> > On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > > 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.
> > 
> > > +	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.

-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Wu]

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

On Thursday 04 October 2007 11:19, John W. Linville wrote:
> > 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

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[John W. Linville]

On Thu, Oct 04, 2007 at 01:11:33PM -0400, Michael Wu wrote:
> On Thursday 04 October 2007 11:19, John W. Linville wrote:
> > > 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.

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
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Wu]

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

On Thursday 04 October 2007 14:15, John W. Linville wrote:
> 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.

> 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

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Roland Dreier]

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[John W. Linville]

On Thu, Oct 04, 2007 at 02:31:26PM -0700, Roland Dreier wrote:
>  > 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
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Johannes Berg]

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

On Thu, 2007-10-04 at 10:34 -0400, Michael Wu wrote:
> On Thursday 04 October 2007 07:33, Daniel Drake wrote:
> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Stephen Hemminger]

On Thu,  4 Oct 2007 12:33:43 +0100 (BST)
Daniel Drake <dsd@gentoo.org> 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>
> 
> 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 <dsd@gentoo.org>

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.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[Michael Wu]

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

On Thursday 04 October 2007 11:54, Stephen Hemminger wrote:
> 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

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

Re: [PATCH] mac80211: Fix TX after monitor interface is converted to managed

[John W. Linville]

On Thu, Oct 04, 2007 at 12:33:43PM +0100, Daniel Drake wrote:

> 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
-- 
John W. Linville
linville@tuxdriver.com

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

[John W. Linville]

Problem description by Daniel Drake <dsd@gentoo.org>:

"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 <dsd@gentoo.org>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/ieee80211_iface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index be7e77f..6607b80 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -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;
 
 	/*
-- 
1.5.2.4

-- 
John W. Linville
linville@tuxdriver.com

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

[Johannes Berg]

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

On Thu, 2007-10-04 at 14:09 -0400, John W. Linville wrote:

> --- a/net/mac80211/ieee80211_iface.c
> +++ b/net/mac80211/ieee80211_iface.c
> @@ -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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[John W. Linville]

On Thu, Oct 04, 2007 at 08:44:48PM +0200, Johannes Berg wrote:
> On Thu, 2007-10-04 at 14:09 -0400, John W. Linville wrote:
> 
> > --- a/net/mac80211/ieee80211_iface.c
> > +++ b/net/mac80211/ieee80211_iface.c
> > @@ -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?

-- 
John W. Linville
linville@tuxdriver.com

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

[Johannes Berg]

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

On Thu, 2007-10-04 at 15:13 -0400, John W. Linville wrote:

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[Michael Wu]

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

On Thursday 04 October 2007 14:09, John W. Linville wrote:
> diff --git a/net/mac80211/ieee80211_iface.c
> b/net/mac80211/ieee80211_iface.c index be7e77f..6607b80 100644
> --- a/net/mac80211/ieee80211_iface.c
> +++ b/net/mac80211/ieee80211_iface.c
> @@ -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)
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

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

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

[John W. Linville]

On Thu, Oct 04, 2007 at 05:26:11PM -0400, Michael Wu wrote:
> On Thursday 04 October 2007 14:09, John W. Linville wrote:
> > diff --git a/net/mac80211/ieee80211_iface.c
> > b/net/mac80211/ieee80211_iface.c index be7e77f..6607b80 100644
> > --- a/net/mac80211/ieee80211_iface.c
> > +++ b/net/mac80211/ieee80211_iface.c
> > @@ -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)
> 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.

Alright...better?

---

From: John W. Linville <linville@tuxdriver.com>
Subject: [PATCH] ieee80211_if_set_type: make check for master dev more explicit

Problem description by Daniel Drake <dsd@gentoo.org>:

"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 <dsd@gentoo.org>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/mac80211/ieee80211_iface.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index be7e77f..43e505d 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -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;
 
 	/*
-- 
1.5.2.4


-- 
John W. Linville
linville@tuxdriver.com

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

[Michael Wu]

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

On Thursday 04 October 2007 19:02, John W. Linville wrote:
> Alright...better?
>
Yup, thanks.

> 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 <dsd@gentoo.org>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
Acked-by: Michael Wu <flamingice@sourmilk.net>

-Michael Wu

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