Page MenuHomeFreeBSD

LinuxKPI: 802.11: close race lkpi_sta_scan_to_auth()/(*iv_update_bss)()
ClosedPublic

Authored by bz on Feb 18 2024, 10:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 16, 10:45 AM
Unknown Object (File)
Fri, Jun 7, 9:41 PM
Unknown Object (File)
Wed, Jun 5, 11:31 PM
Unknown Object (File)
Sun, May 26, 5:54 AM
Unknown Object (File)
Sat, May 18, 11:55 PM
Unknown Object (File)
May 17 2024, 6:29 AM
Unknown Object (File)
May 11 2024, 1:23 AM
Unknown Object (File)
May 9 2024, 7:29 PM

Details

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 56097
Build 52986: 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
1138–1146

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.Apr 22 2024, 8:09 PM
cc requested changes to this revision.Apr 23 2024, 7:17 PM
cc added inline comments.
sys/compat/linuxkpi/common/src/linux_80211.c
1332

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

1338

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

1364

"keop" => "keep"

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

This revision now requires changes to proceed.Apr 23 2024, 7:17 PM
bz marked 2 inline comments as done.

Update comments as suggested by @cc

bz marked an inline comment as done.Fri, May 24, 12:05 AM
bz added inline comments.
sys/compat/linuxkpi/common/src/linux_80211.c
1332

I'll update that in a minute.

1338

From looking again a while later it is the same; given we cache the lsta and work on the lsta here I just use the lsta for consistency and to not confuse it with other code where ni is supposed to be == vap->iv_bss. I'll add a comment.

1364

Unrelated bit also fixed :) Thank you.

bz marked an inline comment as done.Thu, May 30, 9:17 PM

@cc any final comments before this goes in?

Looks good to me. :)

This revision is now accepted and ready to land.Tue, Jun 4, 2:21 PM