Re: [PATCH 2/2] wifi: mwifiex: Stop rejecting connection attempts while connected

[Brian Norris <briannorris@chromium.org>]

On Mon, Aug 07, 2023 at 04:35:49PM -0600, Kevin Lund wrote:
> Hey Brian, thanks for the response. I'm revising the patch now to
> better illustrate the context and purpose for the patch, but I'll also
> respond to your comments here so there is a clear chain.

Yes, good idea :)

> > But, did you attempt any background work on this, to determine why it
> > exists, or what other mitigations are in place?
> 
> Yes, I'll share some of my background checks here:
> 
> The check was initially added in the following patch:
> 
> https://github.com/torvalds/linux/commit/71954f24c93fd569314985e9a7319b68e0b918e6

Ah, good context. I didn't notice that part, and instead assumed that
the quirk (like so many quirks in this driver) existed since its
introduction.

Definitely note this in your followup patch, and while you're at it,
maybe avoid linking github and instead use the preferred commit format.
I think think checkpatch.pl would tell you that's something like:
commit 71954f24c93f ("mwifiex: do not re-associate when already
connected")

And you'll want to explain how you account for the original problem
being solved, or else explain that the problem doesn't apply.

> Before this patch, the driver would explicitly deauthenticate
> from its current AP whenever a new connection was requested.
> Apparently this was racy, so the deauthentication was removed.
> The commit message states that they want to  "Avoid
> re-association while the device is already associated to a
> network." My assertion is that this is invalid, since
> re-associating while connect is a valid action, and it happens
> during BSS-transitions.

Actually, I believe that commit was primarily addressing a particular
WARN()/WARN_ON() issue seen when doing this kind of reassociation,
because of how mwifiex was doing an internal deassociation, and losing
the context of the SSID that it was going to report back to cfg80211
upon (re)association.

So you'd need to show that you don't hit that warning again. (I doubt
you will, because the aforementioned commit also dropped the internal
deauthentication call.)

But I think you'd also need to explain why (or even better,
explain+test) the internal deauthentication was present in the first
place. The previous commit removed the internal deauthentication, and
instead just rejected the request. The former is OK to do, but only(?)
because we did the latter.

So, you'd have to help me remove that question mark: why is it OK for
mwifiex to run its connect (HostCmd_CMD_802_11_ASSOCIATE) flow without
an intervening disconnect? It's not clear to me that the firmware
protocol actually supports this, or that it's been vetted very much, given
how the original mwifiex used to behave (with a full disconnect and
reconnect).

Maybe David Lin @ NXP (CC'd) would be able to help here, as this starts
to ask questions we can only answer by either inspecting the firmware
(i.e., ask NXP) or by testing.

Regarding testing: what kind of testing has been done? On your multi-BSS
setup, have you ensured we're really sending appropriate 802.11 level
frames with this patch? e.g., do mwifiex clients end up sending a proper
REASSOCIATION REQUEST frame, or just ASSOCIATION REQUEST? It doesn't
look like mwifiex actually pushes the required "Current AP Address" down
into the association command, but it's always possible the firmware just
memorizes this and rewrites it...
...or alternatively, maybe mwifiex doesn't actually do Reassociation at
all here, and it just kinda happens to work OK when sending a regular
Association. I'm not sure if that makes this a good patch, or if we'll
end up with interop problems where cfg80211 thinks we're reassociating,
but the AP thinks we're associating, and eventually things break down.

Sorry if that's just a bunch of "unknowns" here, but that's life when
trying to retrofit things into an old full-MAC driver with no support
from the owners of the proprietary firmware. Maybe 802.11 protocol dumps
would make us happy enough though.

> I cross checked this behavior with the userspace SME side
> and looked into a driver with userspace SME, and I could
> not find any indication that the driver rejects association
> attempts while connected.

I don't think comparison with other drivers gives much evidence here.
It's a question of whether the firmware is properly tracking
"reassociation" (to the same or different BSS), or whether it really
needs a DEAUTH in between.

> > For example, I see that
> > sme.c's cfg80211_connect() makes a similar check, but only rejects
> > things if the SSID is different. So with that understanding, it's a
> > reasonable guess to say that mwifiex would be OK with just relying on
> > the existing cfg80211 checks instead.
> 
> > In other words, I think this patch may be OK, but probably could use a
> > bit more explanation.
> 
[...]
> The second condition says that if we are currently
> connected, then
> 
> 1. We must have a prev_bssid value. This is the BSSID
> that we were connected to previously, and it's presence
> is the indication that we are requesting a re-association
> rather than a normal association. Basically, if we're
> connected, then we must be re-associating, not
> normal associating.
> 
> 2. prev_bssid must be the same as our current bssid.
> All this is saying is that the previous BSSID which was
> supplied by the caller must match the BSSID which the
> driver thinks it is connected to. Basically, "the caller is
> saying we should move from A to B, let's make sure
> we're actually connected to A".

I misread what cfg80211_connect() was looking for -- thanks for the
additional explanation.

> So, based on these checks it is abundantly clear that
> cfg80211 absolutely intends it to be a normal flow to
> request a connection while currently connected, and
> it makes deliberate checks to ensure we're in a good
> state when that happens.

Right, thanks, I misunderstood cfg80211 for a bit. So I agree on the
cfg80211 expectation, but that still doesn't tell us how the mwifiex
firmware really behaves. I guess I retain some confusion (per above) of
why mwifiex would have forcibly DEAUTH'd on each reassociation request
in the past, if it wasn't necessary.

Brian