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. > 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 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. 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. > 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. Yes, sme.c's cfg80211_connect() is a very useful point of reference, and I do believe that it makes sufficient checks such that the Marvell driver can rely on them, and is a part of what drove me to make this patch in the first place. The function makes two checks that basically carve out the exact re-association functionality that I'm enabling in the Marvell driver. These checks are commented as such: /* * If we have an ssid_len, we're trying to connect or are * already connected, so reject a new SSID unless it's the * same (which is the case for re-association.) */ and /* * If connected, reject (re-)association unless prev_bssid * matches the current BSSID. */ The first condition asserts that if we're connected, then the only network we should be trying to connect to should be the network which we're currently connected to. Basically, re-association is only valid within the same ESS. 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". 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. > Also, how does "BSS Transitioning" (in your description) fit in here? > IIUC, cfg80211_connect() doesn't support that, as it only allows > reassociation to the same BSSID. This is covered above, but cfg80211_connect() doesn't actually assert that we're re-associating to the same BSSID. It only asserts that the BSSID the caller thinks we're transitioning from is the same BSSID that the driver thinks we are currently connected to. Thanks, Kevin On Thu, Aug 3, 2023 at 7:21 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > > On Fri, Jun 02, 2023 at 04:57:51PM -0600, Kevin Lund wrote: > > Currently, the Marvell WiFi driver rejects any connection attmept while > > we are currently connected. This is poor logic, since there are several > > legitimate use-cases for initiating a connection attempt while > > connected, including re-association and BSS Transitioning. This logic > > means that it's impossible for userspace to request the driver to > > connect to a different BSS on the same ESS without explicitly requesting > > a disconnect first. > > > > Remove the check from the driver so that we can complete BSS transitions > > on the first attempt. > > > > Testing on Chrome OS has shown that this change resolves some issues > > with failed BSS transitions. > > > > Signed-off-by: Kevin Lund <kglund@xxxxxxxxxx> > > --- > > drivers/net/wireless/marvell/mwifiex/cfg80211.c | 6 ------ > > 1 file changed, 6 deletions(-) > > I've been told this one might need an extra look, but first off, it's > marked Rejected, likely due to feedback on patch 1, so probably needs a > resubmit if it needs consideration: > > https://patchwork.kernel.org/project/linux-wireless/patch/20230602225751.164525-2-kglund@xxxxxxxxxx/ > > But, did you attempt any background work on this, to determine why it > exists, or what other mitigations are in place? 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. > > Also, how does "BSS Transitioning" (in your description) fit in here? > IIUC, cfg80211_connect() doesn't support that, as it only allows > reassociation to the same BSSID. > (Or, I might be confused here.) > > Brian
