[PATCH] mac80211: Prevent unregistering of unregistered hw

[PATCH] mac80211: Prevent unregistering of unregistered hw

[Ivo van Doorn]

At the moment it is possible to call ieee80211_unregister_hw()
for an unregistered hw structure. This will cause a big panic.
This patch will add a check to check if IEEE80211_DEV_REGISTERED
has been set before attempting to unregister hw.

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>

---

diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 577dbe3..7494280 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -4765,6 +4765,12 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	/* TODO: skb_queue should be empty here, no need to do anything? */
 
 	rtnl_lock();
+
+	if (local->reg_state != IEEE80211_DEV_REGISTERED) {
+		rtnl_unlock();
+		return;
+	}
+
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;
 	if (local->apdev)
 		ieee80211_if_del_mgmt(local);

Re: [PATCH] mac80211: Prevent unregistering of unregistered hw

[Michael Buesch]

On Friday 09 March 2007 16:14, Ivo van Doorn wrote:
> At the moment it is possible to call ieee80211_unregister_hw()
> for an unregistered hw structure. This will cause a big panic.
> This patch will add a check to check if IEEE80211_DEV_REGISTERED
> has been set before attempting to unregister hw.

For which reason would a driver call unregister, but not register before?
I smell design problems in the driver ;)

> Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
> 
> ---
> 
> diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
> index 577dbe3..7494280 100644
> --- a/net/mac80211/ieee80211.c
> +++ b/net/mac80211/ieee80211.c
> @@ -4765,6 +4765,12 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
>  	/* TODO: skb_queue should be empty here, no need to do anything? */
>  
>  	rtnl_lock();
> +
> +	if (local->reg_state != IEEE80211_DEV_REGISTERED) {
> +		rtnl_unlock();
> +		return;
> +	}
> +
>  	local->reg_state = IEEE80211_DEV_UNREGISTERED;
>  	if (local->apdev)
>  		ieee80211_if_del_mgmt(local);

-- 
Greetings Michael.

Re: [PATCH] mac80211: Prevent unregistering of unregistered hw

[Ivo van Doorn]

On Friday 09 March 2007 16:46, Michael Buesch wrote:
> On Friday 09 March 2007 16:14, Ivo van Doorn wrote:
> > At the moment it is possible to call ieee80211_unregister_hw()
> > for an unregistered hw structure. This will cause a big panic.
> > This patch will add a check to check if IEEE80211_DEV_REGISTERED
> > has been set before attempting to unregister hw.
> 
> For which reason would a driver call unregister, but not register before?

Well it occurred in rt2x00 during errors during initialization,
instead of using goto's to step by step free all allocated memory and unregistration
the main free_dev() was called. And there isn't a way for the driver to check
if the hw has been registered or not without creating new flags for it.
But since mac80211 already is using such a flag internally, it would sound like
more sense to make the check in there.

> I smell design problems in the driver ;)

True, there are different approaches to take care of this inside the driver,
but I think this check is sane enough as well. ;)

Ivo

Re: [PATCH] mac80211: Prevent unregistering of unregistered hw

[Johannes Berg]

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

On Fri, 2007-03-09 at 17:30 +0100, Ivo van Doorn wrote:

> Well it occurred in rt2x00 during errors during initialization,
> instead of using goto's to step by step free all allocated memory and unregistration
> the main free_dev() was called.

Change that then, we like to see a proper error handling strategy that
one can understand w/o checking all possible flags.

>  And there isn't a way for the driver to check
> if the hw has been registered or not without creating new flags for it.
> But since mac80211 already is using such a flag internally, it would sound like
> more sense to make the check in there.

No, the driver should know from its code flow whether it has registered
it or not, it should be obvious by looking at it.

> > I smell design problems in the driver ;)
> 
> True, there are different approaches to take care of this inside the driver,
> but I think this check is sane enough as well. ;)

I disagree. In fact, I'd prefer putting
 BUG_ON(local->reg_state != IEEE80211_DEV_REGISTERED)
into ieee80211_unregister_hw.

johannes

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

Re: [PATCH] mac80211: Prevent unregistering of unregistered hw

[Ivo Van Doorn]

> > Well it occurred in rt2x00 during errors during initialization,
> > instead of using goto's to step by step free all allocated memory and
> unregistration
> > the main free_dev() was called.
>
> Change that then, we like to see a proper error handling strategy that
> one can understand w/o checking all possible flags.

Ok will do that.

Ivo

Re: [PATCH] mac80211: Prevent unregistering of unregistered hw

[Jiri Benc]

On Thu, 22 Mar 2007 11:37:54 +0100, Johannes Berg wrote:
> I disagree. In fact, I'd prefer putting
>  BUG_ON(local->reg_state != IEEE80211_DEV_REGISTERED)
> into ieee80211_unregister_hw.

There's a WARN_ON in uregister_netdevice when the device wasn't
previously registered, so I'd agree with Johannes.

 Jiri

-- 
Jiri Benc
SUSE Labs

[PATCH] mac80211: BUG_ON unregistering unregistered device

[Johannes Berg]

This adds a BUG_ON when you unregister a device that wasn't registered.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
Applies on top of Michael's tasklet fixes or something. Maybe applies
anyway, fuzz should be minimal in any case.

--- wireless-dev.orig/net/mac80211/ieee80211.c	2007-03-23 21:07:00.354239808 +0100
+++ wireless-dev/net/mac80211/ieee80211.c	2007-03-23 21:07:33.184239808 +0100
@@ -4729,6 +4729,8 @@ void ieee80211_unregister_hw(struct ieee
 	struct ieee80211_sub_if_data *sdata, *tmp;
 	int i;
 
+	BUG_ON(local->reg_state != IEEE80211_DEV_REGISTERED);
+
 	tasklet_disable(&local->tasklet);
 	/* TODO: skb_queue should be empty here, no need to do anything? */