Bug 249402 - Locking issue when requesting an ieee80211 scan
Summary: Locking issue when requesting an ieee80211 scan
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: wireless (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-wireless (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-17 18:30 UTC by martin
Modified: 2020-09-18 13:30 UTC (History)
2 users (show)

See Also:


Attachments
Patch to unlock the wifi ic before calling ieee80211_check_scan() (651 bytes, patch)
2020-09-17 18:30 UTC, martin
no flags Details | Diff
Alternative fix: add ie80211_scan_current_locked() and use that (3.82 KB, patch)
2020-09-18 13:30 UTC, martin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description martin 2020-09-17 18:30:56 UTC
Created attachment 218029 [details]
Patch to unlock the wifi ic before calling ieee80211_check_scan()

While adapting the ieee80211 stack to NetBSD I ran into a locking issue whenever scans are started.

I don't know if locking the wifi IC in FreeBSD allows for recursion - in NetBSD it doesn't and all scan starts will result in a "locking against myself" assertion with LOCKDEBUG kernels.

The call here comes in via the ic_newstate callback, which always is called with IC locked (see also the assertion a few lines above), but the ieee80211_check_scan() function (and indirectly ieee80211_check_scan_current() which calls it) will lock the ic again.

Simple patch to work around attached.
Comment 1 Adrian Chadd freebsd_committer 2020-09-17 18:42:15 UTC
Oh, I'd /totally love/ the comlock to be non-recursive in freebsd. That would force us to tidy up a whole lot of things.

Maybe we should add some _locked() versions that expect the lock held, and use those? I'd like to avoid the unlock/<thing>/relock pattern because it is racy as the state can change in that window. (The scan code is notorious for being bad at this..)
Comment 2 martin 2020-09-18 13:30:10 UTC
Created attachment 218053 [details]
Alternative fix: add ie80211_scan_current_locked() and use that

Yes, unlocking/relocking is evil, but this variant looks a bit bloated - better pass an argument telling we already hold the lock?

Martin