Page MenuHomeFreeBSD

WPA: backout upstream IFF_ change and add logging
ClosedPublic

Authored by bz on Feb 27 2023, 10:30 AM.
Tags
None
Referenced Files
F96230983: D38807.id119307.diff
Tue, Sep 24, 4:05 AM
Unknown Object (File)
Mon, Sep 23, 4:17 PM
Unknown Object (File)
Sun, Sep 22, 8:09 PM
Unknown Object (File)
Sun, Sep 22, 2:09 PM
Unknown Object (File)
Sun, Sep 22, 12:29 PM
Unknown Object (File)
Wed, Sep 18, 6:44 PM
Unknown Object (File)
Wed, Sep 11, 6:55 AM
Unknown Object (File)
Sun, Sep 8, 4:18 AM
Subscribers

Details

Summary

This reverts the state to our old supplicant logic setting or clearing
IFF_UP if needed and logging if we do change the interface state.

In my testing I have never seen the log entry for `(changed)`
so I consider this purely for informational and testing purposes but
not for upstream or to be included in the tree as-is.

It might be wise to review my logic, just to make sure I didn't screw
it up by accident.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 50535
Build 47426: arc lint + arc unit

Event Timeline

bz requested review of this revision.Feb 27 2023, 10:30 AM

This is the patch I sent you and the patch I've been running here for the last approximately year.

This revision is now accepted and ready to land.Feb 27 2023, 12:51 PM

On second thought I'd like to try my alternative patch first. Upstream hasn't accepted this patch. We may want to propose my alternative patch to Roy Marples, the OpenBSD committer who submitted the regression to upstream in the first place.

cy requested changes to this revision.EditedFeb 27 2023, 2:57 PM

I'm reversing my acceptance of this patch. https://reviews.freebsd.org/D38805 works and is something we can start discussing with our upstream about. Even if they reject that change it allows us to build upon future revisions without significant merge conflicts.

This revision now requires changes to proceed.Feb 27 2023, 2:57 PM

This is not the same patch we've been discussing before. This one has logging and looking carefully at what it logs shows that the actual interface status always matches the one wpa_supplicant requests/expects at that point in my testing. I would be highly interested in whether it does for you too? Because then, if your D38805 makes a difference, it changes expectations somewhere.

But further to this we, well at least I, currently believe that the interface toggling simply exploits a race condition in the kernel. So "fixing" the problem by changes to IFF_UP doesn't fix the actual problem. We've tried around that path from rc and (possibly wpa_supplicant) for too long without a full fix.

See D38508 for a lengthy discussion of why scan results are missing and how we get into various situations.

I would also be curious if D38661 changes the state of art for any of you at all?

In D38807#883421, @bz wrote:

This is not the same patch we've been discussing before. This one has logging and looking carefully at what it logs shows that the actual interface status always matches the one wpa_supplicant requests/expects at that point in my testing. I would be highly interested in whether it does for you too? Because then, if your D38805 makes a difference, it changes expectations somewhere.

But further to this we, well at least I, currently believe that the interface toggling simply exploits a race condition in the kernel. So "fixing" the problem by changes to IFF_UP doesn't fix the actual problem. We've tried around that path from rc and (possibly wpa_supplicant) for too long without a full fix.

See D38508 for a lengthy discussion of why scan results are missing and how we get into various situations.

I would also be curious if D38661 changes the state of art for any of you at all?

Sorry. I see the differences.

I don't think bsd_commit is need to resolve the wpa_supplicant problem as it is only used by hostapd, which has no problems.

cy added inline comments.
contrib/wpa/src/drivers/driver_bsd.c
1837

This is not needed because hostapd is not affected. Though it won't hurt to keep it in.

Might be an idea to insert a DTrace probe into bsd_commit() to trace it in hostapd.

This revision is now accepted and ready to land.Feb 27 2023, 10:44 PM
contrib/wpa/src/drivers/driver_bsd.c
1837

I like the idea of a DTrace Probe. Let me look into that. Not done much of it in user space :)

This patch works well with my rtl8188eu.

But the problem is the same when the driver is wtap(4), please test the patch D36243. I think the problem is the same as @bz has mentioned in D38508 there is a race of issuing scan requests between net80211's scan request (issued when IF up) and wpa_supplicant's scan request. I think the patch may work well with almost every hardware NIC because the net80211's scan request will always lose the wpa_supplicant's scan request since the IF up takes lots of time. But wtap(4) is pure software, so IF up takes a little time, and unfortunately net80211's scan request dominates the scan process, so wpa_supplicant(8) will never receive the scan result event.

I have updated the D36243 and now it works well !

Drop bsd_commit() for now. USDT remains an unresolved mystery to me
so far (and this is contrib code) -- though I finally asked on the
dtrace list to see if we support it at all out of the box.

This revision now requires review to proceed.Mar 23 2023, 1:13 AM
bz marked an inline comment as done.Mar 23 2023, 1:13 AM

Let's make it happen then.

This revision is now accepted and ready to land.Mar 23 2023, 7:28 PM