Page MenuHomeFreeBSD

LinuxKPI: 802.11: close race lkpi_sta_scan_to_auth()/(*iv_update_bss)()
Needs RevisionPublic

Authored by bz on Feb 18 2024, 10:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 1, 4:47 PM
Unknown Object (File)
Wed, May 1, 4:44 AM
Unknown Object (File)
Tue, Apr 30, 8:03 AM
Unknown Object (File)
Tue, Apr 30, 8:02 AM
Unknown Object (File)
Tue, Apr 30, 2:04 AM
Unknown Object (File)
Sun, Apr 28, 1:27 PM
Unknown Object (File)
Fri, Apr 26, 12:52 PM
Unknown Object (File)
Fri, Apr 26, 5:53 AM

Details

Reviewers
cc
emaste
Summary

We have to unlock the net80211 ic lock in order to be able to call
sleepable downcalls to the driver/firmware a 2nd thread may go through
join1() and (*iv_update_bss)() after we checked and unlocked.
Re-check status at the end of the function under the ic lock so that we
do not accidentally set lvif_bss_synched to true again despite it no
longer being true.

This should fix a race where we lost the (*iv_update_bss)() state
during startup where one SCAN->AUTH is followed by a (then) AUTH->AUTH
and lkpi_sta_a_to_a() did the wrong thing.

Once we re-considered net80211 state and allowing a second join
on a different node or iv_bss update without previously tearing down
the older node we can likely undo a lot of these extra checks and
workarounds.

PR: 274382
MFC after: 3 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 57281
Build 54169: arc lint + arc unit

Event Timeline

bz requested review of this revision.Feb 18 2024, 10:09 PM
cc requested changes to this revision.Feb 29 2024, 4:31 PM
cc added inline comments.
sys/compat/linuxkpi/common/src/linux_80211.c
1141–1149

I don't think it's necessary to move these two lines here.

This revision now requires changes to proceed.Feb 29 2024, 4:31 PM

@emaste have you been running with this change the last months?

@bz I put this in my wipbsd tree on Feb 18, and I have been switching between that tree and main since then. I haven't observed any issue attributable to this change. I still do see one Invalid TXQ sometime in early boot.

@bz I put this in my wipbsd tree on Feb 18, and I have been switching between that tree and main since then. I haven't observed any issue attributable to this change. I still do see one Invalid TXQ sometime in early boot.

Thanks. The Invalid TXQ is understood and as said https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=274382#c49 needs to be solved elsewhere. Stuff that's been around for 10-15 years and no one fixed but everyone just worked around and ignored. Nothing I want to touch before 14.1 is branched at least. I'll see to update this one and get it in as it should at least help to get better visibility.

Move the KASSERT/lsta assignment back to its original place;
I think the idea was to catch that state earlier on before more changes happen which could mask that situation but this change is really about the 2nd half so focus on that.

bz marked an inline comment as done.Mon, Apr 22, 8:09 PM
cc requested changes to this revision.Tue, Apr 23, 7:17 PM
cc added inline comments.
sys/compat/linuxkpi/common/src/linux_80211.c
1337

How to understand this sentence? "essentially the lsta version"?

1343

I am confused. ieee80211_ref_node(lsta->ni) or ieee80211_ref_node(ni)?

1369

"keop" => "keep"

Although not harm, appreciate it that if this typo can be corrected.

This revision now requires changes to proceed.Tue, Apr 23, 7:17 PM