Skip to contentSkip to conversation

mac80211: Prevent unregistering of unregistered hw

PATCH Superseded Ivo van Doorn 4 participants 7 messages Mar 9, 2007 – Mar 23, 2007 mac80211

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Ivo van Doorn

author
#0 lore raw

PATCH 1/1 mac80211: Prevent unregistering of unregistered hw

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 <[email protected]>

---

net/mac80211/ieee80211.c +6 -0
index 577dbe3..7494280 100644
@@ -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);
#1 lore raw
Quoted 4 lines · 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.

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

Quoted 21 lines
Signed-off-by Ivo van Doorn <[email protected]>

---

net/mac80211/ieee80211.c +6 -0
index 577dbe3..7494280 100644
@@ -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);
Signature · 1 line
Greetings Michael.
author
#2 lore raw
Quoted 7 lines · Michael Buesch
Quoted 4 lines · 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.

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.

Quoted 1 line

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

maintainer
#3 lore raw
Quoted 3 lines · 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.

Quoted 4 lines

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.

Quoted 4 lines
Quoted 1 line

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

author
#4 lore raw
Quoted 7 lines
Quoted 2 lines

Well it occurred in rt2x00 during errors during initialization, instead of using goto's to step by step free all allocated memory and

unregistration

Quoted 1 line

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

#5 lore raw
In reply to Johannes Berg
Quoted 3 lines · Johannes Berg

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

Signature · 2 lines
Jiri Benc
SUSE Labs
maintainer
#6 lore raw

PATCH 1/1 mac80211: BUG_ON unregistering unregistered device

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

Signed-off-by Johannes Berg <[email protected]>

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

net/mac80211/ieee80211.c +2 -0
@@ -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? */