[PATCH] mac80211: fix chantype recalc warning

[PATCH] mac80211: fix chantype recalc warning

[Michal Kazior]

When a device driver is unloaded local->interfaces
list is cleared. If there was more than 1
interface running and connected (bound to a
chanctx) then chantype recalc was called and it
ended up with compat being NULL causing a call
trace warning.

Warn if compat becomes NULL as a result of
incompatible bss_conf.chandef of interfaces bound
to a given channel context only.

The call trace looked like this:

 WARNING: CPU: 2 PID: 2594 at /devel/src/linux/net/mac80211/chan.c:557 ieee80211_recalc_chanctx_chantype+0x2cd/0x2e0()
 Modules linked in: ath10k_pci(-) ath10k_core ath
 CPU: 2 PID: 2594 Comm: rmmod Tainted: G        W     3.16.0-rc1+ #150
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  0000000000000009 ffff88001ea279c0 ffffffff818dfa93 0000000000000000
  ffff88001ea279f8 ffffffff810514a8 ffff88001ce09cd0 ffff88001e03cc58
  0000000000000000 ffff88001ce08840 ffff88001ce09cd0 ffff88001ea27a08
 Call Trace:
  [<ffffffff818dfa93>] dump_stack+0x4d/0x66
  [<ffffffff810514a8>] warn_slowpath_common+0x78/0xa0
  [<ffffffff81051585>] warn_slowpath_null+0x15/0x20
  [<ffffffff818a407d>] ieee80211_recalc_chanctx_chantype+0x2cd/0x2e0
  [<ffffffff818a3dda>] ? ieee80211_recalc_chanctx_chantype+0x2a/0x2e0
  [<ffffffff818a4919>] ieee80211_assign_vif_chanctx+0x1a9/0x770
  [<ffffffff818a6220>] __ieee80211_vif_release_channel+0x70/0x130
  [<ffffffff818a6dd3>] ieee80211_vif_release_channel+0x43/0xb0
  [<ffffffff81885f4e>] ieee80211_stop_ap+0x21e/0x5a0
  [<ffffffff8184b9b5>] __cfg80211_stop_ap+0x85/0x520
  [<ffffffff8181c188>] __cfg80211_leave+0x68/0x120
  [<ffffffff8181c268>] cfg80211_leave+0x28/0x40
  [<ffffffff8181c5f3>] cfg80211_netdev_notifier_call+0x373/0x6b0
  [<ffffffff8107f965>] notifier_call_chain+0x55/0x110
  [<ffffffff8107fa41>] raw_notifier_call_chain+0x11/0x20
  [<ffffffff816a8dc0>] call_netdevice_notifiers_info+0x30/0x60
  [<ffffffff816a8eb9>] __dev_close_many+0x59/0xf0
  [<ffffffff816a9021>] dev_close_many+0x81/0x120
  [<ffffffff816aa1c5>] rollback_registered_many+0x115/0x2a0
  [<ffffffff816aa3a6>] unregister_netdevice_many+0x16/0xa0
  [<ffffffff8187d841>] ieee80211_remove_interfaces+0x121/0x1b0
  [<ffffffff8185e0e6>] ieee80211_unregister_hw+0x56/0x110
  [<ffffffffa0011ac4>] ath10k_mac_unregister+0x14/0x60 [ath10k_core]
  [<ffffffffa0014fe7>] ath10k_core_unregister+0x27/0x40 [ath10k_core]
  [<ffffffffa003b1f4>] ath10k_pci_remove+0x44/0xa0 [ath10k_pci]
  [<ffffffff81373138>] pci_device_remove+0x28/0x60
  [<ffffffff814cb534>] __device_release_driver+0x64/0xd0
  [<ffffffff814cbcc8>] driver_detach+0xb8/0xc0
  [<ffffffff814cb23a>] bus_remove_driver+0x4a/0xb0
  [<ffffffff814cc697>] driver_unregister+0x27/0x50

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/chan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f3317fa..d810b2e 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -549,12 +549,12 @@ static void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
 
 		compat = cfg80211_chandef_compatible(
 				&sdata->vif.bss_conf.chandef, compat);
-		if (!compat)
+		if (WARN_ON_ONCE(!compat))
 			break;
 	}
 	rcu_read_unlock();
 
-	if (WARN_ON_ONCE(!compat))
+	if (!compat)
 		return;
 
 	ieee80211_change_chanctx(local, ctx, compat);
-- 
1.8.5.3

Re: [PATCH] mac80211: fix chantype recalc warning

[Johannes Berg]

On Mon, 2014-07-28 at 15:16 +0200, Michal Kazior wrote:
> When a device driver is unloaded local->interfaces
> list is cleared. If there was more than 1
> interface running and connected (bound to a
> chanctx) then chantype recalc was called and it
> ended up with compat being NULL causing a call
> trace warning.
> 
> Warn if compat becomes NULL as a result of
> incompatible bss_conf.chandef of interfaces bound
> to a given channel context only.

Err, this slipped my radar - where does this need to go?

Also - is this really the right thing? Why does this happen in this
situation? Shouldn't they all be not connected or not have a chanctx or
be compatible?

johannes

Re: [PATCH] mac80211: fix chantype recalc warning

[Michal Kazior]

On 11 August 2014 17:05, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-07-28 at 15:16 +0200, Michal Kazior wrote:
>> When a device driver is unloaded local->interfaces
>> list is cleared. If there was more than 1
>> interface running and connected (bound to a
>> chanctx) then chantype recalc was called and it
>> ended up with compat being NULL causing a call
>> trace warning.
>>
>> Warn if compat becomes NULL as a result of
>> incompatible bss_conf.chandef of interfaces bound
>> to a given channel context only.
>
> Err, this slipped my radar - where does this need to go?

No big deal. The warning is just a small nuisance with multi-vif. You
won't see this on a typical system.


>
> Also - is this really the right thing? Why does this happen in this
> situation? Shouldn't they all be not connected or not have a chanctx or
> be compatible?

If you eject, e.g. plug a usb wifi device out, its driver will call
ieee80211_unregister_hw() which calls ieee80211_remove_interfaces().
If there are still running interfaces that have a netdev (probably to
avoid a deadlock on iflist_mtx?) each interface is *moved* to a
temporary list for a later call to unregister_netdevice_many(). While
unregister_netdevice_many() executes the local->interfaces is empty.
ieee80211_recalc_chanctx_chantype() is called whenever a chanctx is
unassigned from a vif as long as there are still references to that
chanctx. If there is just 1 interface when device is ejected, then
ieee80211_recalc_chanctx_chantype() is not called. If there is more
than 1 running interfaces then the removal of each but the last one
calls to ieee80211_recalc_chanctx_chantype(). Since local->interfaces
is empty compat pointer stays NULL and this triggers WARN_ON_ONCE (but
only the first one effectively dumps a trace).

Another way to fix this would probably be to change
ieee80211_remove_interfaces() to remove all interfaces one-by-one
(instead of in bulk). It seems iflist_mtx can be temporarily released
while iterating over interfaces for the sake of calling
unregister_netdevice() for each one because RTNL is held all the time
guaranteeing no new interfaces are added in the meantime. I'm not sure
if it's perfectly safe to replace unregister_netdevice_many() just
like that though. I can look more into it.


Michał

Re: [PATCH] mac80211: fix chantype recalc warning

[Johannes Berg]

On Mon, 2014-08-18 at 15:07 +0200, Michal Kazior wrote:

> If you eject, e.g. plug a usb wifi device out, its driver will call
> ieee80211_unregister_hw() which calls ieee80211_remove_interfaces().
> If there are still running interfaces that have a netdev (probably to
> avoid a deadlock on iflist_mtx?) each interface is *moved* to a
> temporary list for a later call to unregister_netdevice_many(). While
> unregister_netdevice_many() executes the local->interfaces is empty.
> ieee80211_recalc_chanctx_chantype() is called whenever a chanctx is
> unassigned from a vif as long as there are still references to that
> chanctx. If there is just 1 interface when device is ejected, then
> ieee80211_recalc_chanctx_chantype() is not called. If there is more
> than 1 running interfaces then the removal of each but the last one
> calls to ieee80211_recalc_chanctx_chantype(). Since local->interfaces
> is empty compat pointer stays NULL and this triggers WARN_ON_ONCE (but
> only the first one effectively dumps a trace).

Ah ok.

> Another way to fix this would probably be to change
> ieee80211_remove_interfaces() to remove all interfaces one-by-one
> (instead of in bulk). It seems iflist_mtx can be temporarily released
> while iterating over interfaces for the sake of calling
> unregister_netdevice() for each one because RTNL is held all the time
> guaranteeing no new interfaces are added in the meantime. I'm not sure
> if it's perfectly safe to replace unregister_netdevice_many() just
> like that though. I can look more into it.

Well, it's safe, but it's more efficient to call the _many() because
otherwise you require rcu_synchronize() for each one.

I'll apply this, to -next for now, later we can decide if we want to
send it to stable or so?

johannes

Re: [PATCH] mac80211: fix chantype recalc warning

[Michal Kazior]

On 29 August 2014 13:05, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-08-18 at 15:07 +0200, Michal Kazior wrote:
[...]
>> Another way to fix this would probably be to change
>> ieee80211_remove_interfaces() to remove all interfaces one-by-one
>> (instead of in bulk). It seems iflist_mtx can be temporarily released
>> while iterating over interfaces for the sake of calling
>> unregister_netdevice() for each one because RTNL is held all the time
>> guaranteeing no new interfaces are added in the meantime. I'm not sure
>> if it's perfectly safe to replace unregister_netdevice_many() just
>> like that though. I can look more into it.
>
> Well, it's safe, but it's more efficient to call the _many() because
> otherwise you require rcu_synchronize() for each one.
>
> I'll apply this, to -next for now, later we can decide if we want to
> send it to stable or so?

Thanks. As far as I'm aware the splat was harmless so I'm unsure if it
qualifies for stable.


Michał