mac80211: fix resuming when device is gone

RFC In review Stanislaw Gruszka 2 participants 14 messages Aug 8, 2011 – Aug 9, 2011

Subsystems (2)

mac80211 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

Stanislaw Gruszka

author

2011-08-08 14:19 UTC #0 lore raw

RFC 1/1 mac80211: fix resuming when device is gone

Is possible that usb hardware can be unplugged during or before resume. If so do not call ieee80211_reconfig(), which among other things arm sta_cleanup timer. Timer callback then operate on freed memory.

WARNING: at lib/debugobjects.c:262 debug_print_object+0x85/0xa0()
Hardware name: 6369CTO
ODEBUG: free active (active state 0) object type: timer_list hint:
sta_info_cleanup+0x0/0x1f0 [mac80211]
Modules linked in: rt73usb crc_itu_t rt2x00usb rt2x00lib mac80211

cfg80211 aes_i586 aes_generic fuse bridge stp llc autofs4 sunrpc cpufreq_ondemand acpi_cpufreq mperf ext2 uinput sg arc4 i2c_i801 iTCO_wdt iTCO_vendor_support e1000e thinkpad_acpi hwmon ext4 mbcache jbd2 sd_mod crc_t10dif sr_mod cdrom yenta_socket ahci libahci pata_acpi ata_generic ata_piix i915 drm_kms_helper drm i2c_algo_bit video [last
unloaded: cfg80211]
Pid: 8251, comm: pm-hibernate Tainted: G W 3.0.0-wl+ #7

Call Trace:
 [<c04510fd>] warn_slowpath_common+0x6d/0xa0
 [<c05ef1b5>] ? debug_print_object+0x85/0xa0
 [<c05ef1b5>] ? debug_print_object+0x85/0xa0
 [<c04511ae>] warn_slowpath_fmt+0x2e/0x30
 [<c05ef1b5>] debug_print_object+0x85/0xa0
 [<f8d1dac0>] ? sta_info_alloc+0x230/0x230 [mac80211]
 [<c05ef7b2>] debug_check_no_obj_freed+0xe2/0x180
 [<c051a80d>] kfree+0x9d/0x180
 [<f8cb368e>] cfg80211_dev_free+0x9e/0xb0 [cfg80211]
 [<f8cb4cbd>] wiphy_dev_release+0xd/0x10 [cfg80211]
 [<c06a3f09>] device_release+0x19/0x80
 [<c048858c>] ? trace_hardirqs_on_caller+0x12c/0x170
 [<c05e027a>] kobject_release+0x7a/0x1c0
 [<c06ad77c>] ? dpm_resume+0xcc/0x190
 [<c05e0200>] ? kobject_del+0x30/0x30
 [<c05e167d>] kref_put+0x2d/0x60
 [<c05e012d>] kobject_put+0x1d/0x50
 [<c081dc52>] ? mutex_lock_nested+0x42/0x50
 [<c06ad77c>] ? dpm_resume+0xcc/0x190
 [<c06a3b9f>] put_device+0xf/0x20
 [<c06ad7aa>] dpm_resume+0xfa/0x190
 [<c04975fd>] hibernation_snapshot+0xcd/0x270
 [<c049673f>] ? freeze_processes+0x3f/0x90
 [<c049786b>] hibernate+0xcb/0x1e0

I have this warning with possible fallow up crash without physically unplugging device, but usb core rebind rt73usb with message:

"rt73usb 1-2:1.0: no reset_resume for driver rt73usb?"

What probably also need to be fixed in rt2x00. But I think fix in mac80211 is needed for possibility of physical remove. Not sure if this is best possible fix, through. Maybe just preventing arming sta_cleanup would be better, other things in ieee80211_reconfig() seems to work.

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 net/mac80211/ieee80211_i.h |    6 ++++++
 net/mac80211/main.c        |    2 ++
 net/mac80211/util.c        |    3 +++
 3 files changed, 11 insertions(+), 0 deletions(-)

net/mac80211/ieee80211_i.h +6 -0

index 400c09b..ce8201f 100644
@@ -804,6 +804,12 @@ struct ieee80211_local {
 	/* wowlan is enabled -- don't reconfig on resume */
 	bool wowlan;
 
+	/*
+	 * true between ieee80211_register_hw() and ieee80211_unregister_hw()
+	 * calls, protected by rtnl_lock(), used as hw gone check when resuming
+	 */
+	bool registered;
+
 	int tx_headroom; /* required headroom for hardware/radiotap */
 
 	/* Tasklet and skb queue to process calls from IRQ mode. All frames

net/mac80211/main.c +2 -0

index 866f269..7ef2698 100644
@@ -929,6 +929,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 				   "Failed to add default virtual iface\n");
 	}
 
+	local->registered = true;
 	rtnl_unlock();
 
 	ieee80211_led_init(local);

@@ -1000,6 +1001,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	 */
 	ieee80211_remove_interfaces(local);
 
+	local->registered = false;
 	rtnl_unlock();
 
 	/*

net/mac80211/util.c +3 -0

index ddeb1b9..05da753 100644
@@ -1135,6 +1135,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 	struct sta_info *sta;
 	int res, i;
 
+	if (!local->registered)
+		return -ENODEV;
+
 #ifdef CONFIG_PM
 	if (local->suspended)
 		local->resuming = true;

Signature · 1 line

1.7.1

Johannes Berg

maintainer

2011-08-08 15:58 UTC #1 lore raw

Quoted 7 lines · Stanislaw Gruszka

Is possible that usb hardware can be unplugged during or before resume. If so do not call ieee80211_reconfig(), which among other things arm sta_cleanup timer. Timer callback then operate on freed memory.
I have this warning with possible fallow up crash without physically unplugging device, but usb core rebind rt73usb with message:

Quoted 6 lines

"rt73usb 1-2:1.0: no reset_resume for driver rt73usb?"
What probably also need to be fixed in rt2x00. But I think fix in mac80211 is needed for possibility of physical remove. Not sure if this is best possible fix, through. Maybe just preventing arming sta_cleanup would be better, other things in ieee80211_reconfig() seems to work.

But ... if sta_cleanup timer operates on freed memory, why doesn't "local->registered"?

johannes

Stanislaw Gruszka

author

2011-08-09 09:23 UTC #2 lore raw

Quoted 10 lines · Johannes Berg

Quoted 9 lines · Stanislaw Gruszka
Is possible that usb hardware can be unplugged during or before resume. If so do not call ieee80211_reconfig(), which among other things arm sta_cleanup timer. Timer callback then operate on freed memory.
I have this warning with possible fallow up crash without physically unplugging device, but usb core rebind rt73usb with message:
"rt73usb 1-2:1.0: no reset_resume for driver rt73usb?"

Quoted 7 lines

Quoted 4 lines
What probably also need to be fixed in rt2x00. But I think fix in mac80211 is needed for possibility of physical remove. Not sure if this is best possible fix, through. Maybe just preventing arming sta_cleanup would be better, other things in ieee80211_reconfig() seems to work.
But ... if sta_cleanup timer operates on freed memory, why doesn't "local->registered"?

I think I was unclear. The sta_cleanup timer callback, namely sta_info_cleanup(), can operate on freed memory. On ieee80211_unregister_hw() -> sta_info_stop() we delete this timer, but rdev/wiphy/local/hw structure is not freed. It's keep by reference counter. Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.

Stanislaw

Johannes Berg

maintainer

2011-08-09 09:28 UTC #3 lore raw

Quoted 8 lines · Stanislaw Gruszka

Quoted 2 lines
But ... if sta_cleanup timer operates on freed memory, why doesn't "local->registered"?
I think I was unclear. The sta_cleanup timer callback, namely sta_info_cleanup(), can operate on freed memory. On ieee80211_unregister_hw() -> sta_info_stop() we delete this timer, but rdev/wiphy/local/hw structure is not freed. It's keep by reference counter.

You mean by device_del(&rdev->wiphy.dev) right?

Quoted 3 lines

Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.

Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.

johannes

Johannes Berg

maintainer

2011-08-09 09:29 UTC #4 lore raw

Quoted 6 lines · Johannes Berg

Quoted 5 lines
I think I was unclear. The sta_cleanup timer callback, namely sta_info_cleanup(), can operate on freed memory. On ieee80211_unregister_hw() -> sta_info_stop() we delete this timer, but rdev/wiphy/local/hw structure is not freed. It's keep by reference counter.

Quoted 8 lines

You mean by device_del(&rdev->wiphy.dev) right?
Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.

Or .. what else called ieee80211_reconfig()?

johannes

Stanislaw Gruszka

author

2011-08-09 09:43 UTC #5 lore raw

Quoted 10 lines · Johannes Berg

Quoted 8 lines · Johannes Berg
Quoted 5 lines
I think I was unclear. The sta_cleanup timer callback, namely sta_info_cleanup(), can operate on freed memory. On ieee80211_unregister_hw() -> sta_info_stop() we delete this timer, but rdev/wiphy/local/hw structure is not freed. It's keep by reference counter.
You mean by device_del(&rdev->wiphy.dev) right?

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.
Or .. what else called ieee80211_reconfig()?

Only ieee80211_restart_work().

Note, I assumed that ieee80211_reconfig() have no sense after ieee80211_unregister_hw()?

Stanislaw

Stanislaw Gruszka

author

2011-08-09 09:39 UTC #6 lore raw

In reply to Johannes Berg

Quoted 5 lines · Johannes Berg

Quoted 3 lines · Stanislaw Gruszka
Quoted 2 lines
But ... if sta_cleanup timer operates on freed memory, why doesn't "local->registered"?

Quoted 7 lines

Quoted 5 lines
I think I was unclear. The sta_cleanup timer callback, namely sta_info_cleanup(), can operate on freed memory. On ieee80211_unregister_hw() -> sta_info_stop() we delete this timer, but rdev/wiphy/local/hw structure is not freed. It's keep by reference counter.
You mean by device_del(&rdev->wiphy.dev) right?

Yes.

Quoted 6 lines

Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.

No, ieee80211_reconfig() is called before device_del() (but can be called right after ieee80211_unregister_hw() and perhaps ieee80211_free_hw).

Stanislaw

Johannes Berg

maintainer

2011-08-09 09:45 UTC #7 lore raw

Quoted 2 lines · Stanislaw Gruszka

Note, I assumed that ieee80211_reconfig() have no sense after ieee80211_unregister_hw()?

Yeah.

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.
No, ieee80211_reconfig() is called before device_del() (but can be called right after ieee80211_unregister_hw() and perhaps ieee80211_free_hw).

Ah so it's racy.

But I think you meant to say resume() is called before device_del()?

johannes

Stanislaw Gruszka

author

2011-08-09 11:36 UTC #8 lore raw

Quoted 7 lines · Johannes Berg

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.

Quoted 7 lines

Quoted 3 lines
No, ieee80211_reconfig() is called before device_del() (but can be called right after ieee80211_unregister_hw() and perhaps ieee80211_free_hw).
Ah so it's racy.
But I think you meant to say resume() is called before device_del()?

Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().

Stanislaw

Johannes Berg

maintainer

2011-08-09 11:43 UTC #9 lore raw

Quoted 12 lines · Stanislaw Gruszka

Quoted 11 lines · Johannes Berg
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Then if ieee80211_reconfig() is called, we schedule sta_cleanup timer. After that, when sysfs drop reference counter we free rdev. Then sta_info_cleanup() crash kernel.
Ok let me get this straight -- even after device_del() we get a resume callback from the core subsystem? That doesn't seem right.
No, ieee80211_reconfig() is called before device_del() (but can be called right after ieee80211_unregister_hw() and perhaps ieee80211_free_hw).

Quoted 5 lines

Quoted 3 lines
Ah so it's racy.
But I think you meant to say resume() is called before device_del()?
Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().

Sure. Just trying to make sure I understand where it's coming from. So wiphy_unregister() is still running, before device_del(), but then we get the resume from the core...

Shouldn't we implement this logic you just did in cfg80211 instead?

johannes

Johannes Berg

maintainer

2011-08-09 11:45 UTC #10 lore raw

Quoted 7 lines · Johannes Berg

Quoted 1 line
Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().
Sure. Just trying to make sure I understand where it's coming from. So wiphy_unregister() is still running, before device_del(), but then we get the resume from the core...
Shouldn't we implement this logic you just did in cfg80211 instead?

Although I guess until wiphy_unregister() returns, drivers must still expect callbacks, which clearly mac80211 didn't handle correctly here with the timer...

johannes

Johannes Berg

maintainer

2011-08-09 11:46 UTC #11 lore raw

Quoted 8 lines · Johannes Berg

Quoted 6 lines · Johannes Berg
Quoted 1 line
Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().
Sure. Just trying to make sure I understand where it's coming from. So wiphy_unregister() is still running, before device_del(), but then we get the resume from the core...

Quoted 5 lines

Quoted 1 line
Shouldn't we implement this logic you just did in cfg80211 instead?
Although I guess until wiphy_unregister() returns, drivers must still expect callbacks, which clearly mac80211 didn't handle correctly here with the timer...

Actually...

Doesn't that just mean we need to reorder the code in _free_hw()? If we do the timer stuff after wiphy_unregister(), we should be OK, and prevent other, similar problems in the future too.

johannes

Johannes Berg

maintainer

2011-08-09 11:47 UTC #12 lore raw

Quoted 11 lines · Johannes Berg

Quoted 10 lines · Johannes Berg
Quoted 7 lines · Johannes Berg
Quoted 1 line
Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().
Sure. Just trying to make sure I understand where it's coming from. So wiphy_unregister() is still running, before device_del(), but then we get the resume from the core...
Shouldn't we implement this logic you just did in cfg80211 instead?

Quoted 7 lines

Quoted 3 lines
Although I guess until wiphy_unregister() returns, drivers must still expect callbacks, which clearly mac80211 didn't handle correctly here with the timer...
Actually...
Doesn't that just mean we need to reorder the code in _free_hw()?

I mean ieee80211_unregister_hw().

Quoted 11 lines

If we
do the timer stuff after wiphy_unregister(), we should be OK, and prevent other, similar problems in the future too.
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stanislaw Gruszka

author

2011-08-09 15:30 UTC #13 lore raw

Quoted 16 lines · Johannes Berg

Quoted 15 lines · Johannes Berg
Quoted 13 lines · Johannes Berg
Quoted 7 lines · Johannes Berg
Quoted 1 line
Yes, but __ieee80211_resume() calls directly ieee80211_reconfig().
Sure. Just trying to make sure I understand where it's coming from. So wiphy_unregister() is still running, before device_del(), but then we get the resume from the core...
Shouldn't we implement this logic you just did in cfg80211 instead?
Although I guess until wiphy_unregister() returns, drivers must still expect callbacks, which clearly mac80211 didn't handle correctly here with the timer...

Quoted 9 lines

Quoted 3 lines
Actually...
Doesn't that just mean we need to reorder the code in _free_hw()?
I mean ieee80211_unregister_hw().
Quoted 3 lines
If we
do the timer stuff after wiphy_unregister(), we should be OK, and prevent other, similar problems in the future too.

There are some debugfs dependencies, so moving wiphy_unregister() before del_timer_sync(&local->work_timer); in ieee80211_unregister_hw() gave me a crash. I moved sta_info_stop(local); after wiphy_unregister(local->hw.wiphy); but this alone do not solve the problem, ->resume callback can be called after wiphy_unregister(). Below patch fixes the problem (it also protect ->suspend, in case device gone during suspend process):

net/mac80211/main.c +1 -1

index 866f269..acb4423 100644
@@ -1012,7 +1012,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	cancel_work_sync(&local->reconfig_filter);
 
 	ieee80211_clear_tx_pending(local);
-	sta_info_stop(local);
 	rate_control_deinitialize(local);
 
 	if (skb_queue_len(&local->skb_queue) ||

@@ -1024,6 +1023,7 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 
 	destroy_workqueue(local->workqueue);
 	wiphy_unregister(local->hw.wiphy);
+	sta_info_stop(local);
 	ieee80211_wep_free(local);
 	ieee80211_led_exit(local);
 	kfree(local->int_scan_req);

net/wireless/core.c +8 -0

index 645437c..453fcc6 100644
@@ -616,6 +616,9 @@ int wiphy_register(struct wiphy *wiphy)
 	if (res)
 		goto out_rm_dev;
 
+	rtnl_lock();
+	rdev->wiphy.registered = true;
+	rtnl_unlock();
 	return 0;
 
 out_rm_dev:

@@ -647,6 +650,10 @@ void wiphy_unregister(struct wiphy *wiphy)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
 
+	rtnl_lock();
+	rdev->wiphy.registered = false;
+	rtnl_unlock();
+
 	rfkill_unregister(rdev->rfkill);
 
 	/* protect the device list */

@@ -690,6 +697,7 @@ void wiphy_unregister(struct wiphy *wiphy)
 	reg_device_remove(wiphy);
 
 	cfg80211_rdev_list_generation++;
+	rdev->wiphy.registered = false;
 	device_del(&rdev->wiphy.dev);
 
 	mutex_unlock(&cfg80211_mutex);

net/wireless/sysfs.c +8 -2

index c6e4ca6..107516b 100644
@@ -93,7 +93,10 @@ static int wiphy_suspend(struct device *dev, pm_message_t state)
 
 	if (rdev->ops->suspend) {
 		rtnl_lock();
-		ret = rdev->ops->suspend(&rdev->wiphy, rdev->wowlan);
+		if (rdev->wiphy.registered)
+			ret = rdev->ops->suspend(&rdev->wiphy, rdev->wowlan);
+		else
+			ret = -ENODEV;
 		rtnl_unlock();
 	}

@@ -112,7 +115,10 @@ static int wiphy_resume(struct device *dev)
 
 	if (rdev->ops->resume) {
 		rtnl_lock();
-		ret = rdev->ops->resume(&rdev->wiphy);
+		if (rdev->wiphy.registered)
+			ret = rdev->ops->resume(&rdev->wiphy);
+		else
+			ret = -ENODEV;
 		rtnl_unlock();
 	}