[RFC] mac80211: fix resuming when device is gone

[RFC] mac80211: fix resuming when device is gone

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

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 <sgruszka@redhat.com>
---
 net/mac80211/ieee80211_i.h |    6 ++++++
 net/mac80211/main.c        |    2 ++
 net/mac80211/util.c        |    3 +++
 3 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 400c09b..ce8201f 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -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
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..7ef2698 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -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();
 
 	/*
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index ddeb1b9..05da753 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -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;
-- 
1.7.1

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Mon, 2011-08-08 at 16:19 +0200, Stanislaw Gruszka wrote:
> 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?"
> 
> 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

Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka]

On Mon, Aug 08, 2011 at 05:58:26PM +0200, Johannes Berg wrote:
> On Mon, 2011-08-08 at 16:19 +0200, Stanislaw Gruszka wrote:
> > 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?"
> > 
> > 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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 11:23 +0200, Stanislaw Gruszka wrote:

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

> 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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 11:28 +0200, Johannes Berg wrote:

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

Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka]

On Tue, Aug 09, 2011 at 11:29:26AM +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 11:28 +0200, Johannes Berg wrote:
> 
> > > 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?
> > 
> > > 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

Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka]

On Tue, Aug 09, 2011 at 11:28:01AM +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 11:23 +0200, Stanislaw Gruszka wrote:
> 
> > > 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?

Yes.

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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 11:39 +0200, Stanislaw Gruszka wrote:

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

Yeah.

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

Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka]

On Tue, Aug 09, 2011 at 11:45:13AM +0200, Johannes Berg wrote:
> > > > 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()?

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

Stanislaw

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 13:36 +0200, Stanislaw Gruszka wrote:
> On Tue, Aug 09, 2011 at 11:45:13AM +0200, Johannes Berg wrote:
> > > > > 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()?
> 
> 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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 13:43 +0200, Johannes Berg wrote:

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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 13:45 +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 13:43 +0200, Johannes Berg wrote:
> 
> > > 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...

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

Re: [RFC] mac80211: fix resuming when device is gone

[Johannes Berg]

On Tue, 2011-08-09 at 13:46 +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 13:45 +0200, Johannes Berg wrote:
> > On Tue, 2011-08-09 at 13:43 +0200, Johannes Berg wrote:
> > 
> > > > 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...
> 
> Actually...
> 
> Doesn't that just mean we need to reorder the code in _free_hw()?

I mean ieee80211_unregister_hw().

>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC] mac80211: fix resuming when device is gone

[Stanislaw Gruszka]

On Tue, Aug 09, 2011 at 01:47:43PM +0200, Johannes Berg wrote:
> On Tue, 2011-08-09 at 13:46 +0200, Johannes Berg wrote:
> > On Tue, 2011-08-09 at 13:45 +0200, Johannes Berg wrote:
> > > On Tue, 2011-08-09 at 13:43 +0200, Johannes Berg wrote:
> > > 
> > > > > 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...
> > 
> > Actually...
> > 
> > Doesn't that just mean we need to reorder the code in _free_hw()?
> 
> I mean ieee80211_unregister_hw().
> 
> >  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):

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..acb4423 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -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);
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 645437c..453fcc6 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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);
diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c
index c6e4ca6..107516b 100644
--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -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();
 	}