[PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[Kevin Lund]

Within cfg80211_connect, reject the (re-)association request if we are
already connected to the exact BSSID which is being requested. This
prevents an unnecessary attempt to connect which in the best case
leaves us back where we started.

There is precedent for behaving this way over on the userspace SME side
of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
several basic checks to ensure the connection attempt is reasonable, so
this fits in that context.

Signed-off-by: Kevin Lund <kglund@google.com>
---
 net/wireless/sme.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 7bdeb8eea92dc..8f88e66bc85fc 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1442,7 +1442,8 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
 
 	/*
 	 * If connected, reject (re-)association unless prev_bssid
-	 * matches the current BSSID.
+	 * matches the current BSSID. Also reject if the current BSSID matches
+	 * the desired BSSID.
 	 */
 	if (wdev->connected) {
 		if (!prev_bssid)
@@ -1450,6 +1451,9 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
 		if (!ether_addr_equal(prev_bssid,
 				      wdev->u.client.connected_addr))
 			return -ENOTCONN;
+		if (ether_addr_equal(wdev->current_bss->pub.bssid,
+				     connect->bssid))
+			return -EALREADY;
 	}
 
 	/*
-- 
2.39.2

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

[Kevin Lund]

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@google.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index bcd564dc3554a..84d650c9dceb0 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -2414,12 +2414,6 @@ mwifiex_cfg80211_connect(struct wiphy *wiphy, struct net_device *dev,
 		return -EINVAL;
 	}
 
-	if (priv->wdev.connected) {
-		mwifiex_dbg(adapter, ERROR,
-			    "%s: already connected\n", dev->name);
-		return -EALREADY;
-	}
-
 	if (priv->scan_block)
 		priv->scan_block = false;
 
-- 
2.39.2

Re: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[kernel test robot]

Hi Kevin,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20230602225751.164525-1-kglund%40google.com
patch subject: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID
config: arm-randconfig-r046-20230602 (https://download.01.org/0day-ci/archive/20230603/202306031152.pDkq23ib-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
        git checkout 09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash net/wireless/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031152.pDkq23ib-lkp@intel.com/

All errors (new ones prefixed by >>):

   net/wireless/sme.c: In function 'cfg80211_connect':
>> net/wireless/sme.c:1454:42: error: 'struct wireless_dev' has no member named 'current_bss'
    1454 |                 if (ether_addr_equal(wdev->current_bss->pub.bssid,
         |                                          ^~


vim +1454 net/wireless/sme.c

  1418	
  1419	/*
  1420	 * API calls for nl80211/wext compatibility code
  1421	 */
  1422	int cfg80211_connect(struct cfg80211_registered_device *rdev,
  1423			     struct net_device *dev,
  1424			     struct cfg80211_connect_params *connect,
  1425			     struct cfg80211_cached_keys *connkeys,
  1426			     const u8 *prev_bssid)
  1427	{
  1428		struct wireless_dev *wdev = dev->ieee80211_ptr;
  1429		int err;
  1430	
  1431		ASSERT_WDEV_LOCK(wdev);
  1432	
  1433		/*
  1434		 * If we have an ssid_len, we're trying to connect or are
  1435		 * already connected, so reject a new SSID unless it's the
  1436		 * same (which is the case for re-association.)
  1437		 */
  1438		if (wdev->u.client.ssid_len &&
  1439		    (wdev->u.client.ssid_len != connect->ssid_len ||
  1440		     memcmp(wdev->u.client.ssid, connect->ssid, wdev->u.client.ssid_len)))
  1441			return -EALREADY;
  1442	
  1443		/*
  1444		 * If connected, reject (re-)association unless prev_bssid
  1445		 * matches the current BSSID. Also reject if the current BSSID matches
  1446		 * the desired BSSID.
  1447		 */
  1448		if (wdev->connected) {
  1449			if (!prev_bssid)
  1450				return -EALREADY;
  1451			if (!ether_addr_equal(prev_bssid,
  1452					      wdev->u.client.connected_addr))
  1453				return -ENOTCONN;
> 1454			if (ether_addr_equal(wdev->current_bss->pub.bssid,
  1455					     connect->bssid))
  1456				return -EALREADY;
  1457		}
  1458	
  1459		/*
  1460		 * Reject if we're in the process of connecting with WEP,
  1461		 * this case isn't very interesting and trying to handle
  1462		 * it would make the code much more complex.
  1463		 */
  1464		if (wdev->connect_keys)
  1465			return -EINPROGRESS;
  1466	
  1467		cfg80211_oper_and_ht_capa(&connect->ht_capa_mask,
  1468					  rdev->wiphy.ht_capa_mod_mask);
  1469		cfg80211_oper_and_vht_capa(&connect->vht_capa_mask,
  1470					   rdev->wiphy.vht_capa_mod_mask);
  1471	
  1472		if (connkeys && connkeys->def >= 0) {
  1473			int idx;
  1474			u32 cipher;
  1475	
  1476			idx = connkeys->def;
  1477			cipher = connkeys->params[idx].cipher;
  1478			/* If given a WEP key we may need it for shared key auth */
  1479			if (cipher == WLAN_CIPHER_SUITE_WEP40 ||
  1480			    cipher == WLAN_CIPHER_SUITE_WEP104) {
  1481				connect->key_idx = idx;
  1482				connect->key = connkeys->params[idx].key;
  1483				connect->key_len = connkeys->params[idx].key_len;
  1484	
  1485				/*
  1486				 * If ciphers are not set (e.g. when going through
  1487				 * iwconfig), we have to set them appropriately here.
  1488				 */
  1489				if (connect->crypto.cipher_group == 0)
  1490					connect->crypto.cipher_group = cipher;
  1491	
  1492				if (connect->crypto.n_ciphers_pairwise == 0) {
  1493					connect->crypto.n_ciphers_pairwise = 1;
  1494					connect->crypto.ciphers_pairwise[0] = cipher;
  1495				}
  1496			}
  1497		} else {
  1498			if (WARN_ON(connkeys))
  1499				return -EINVAL;
  1500	
  1501			/* connect can point to wdev->wext.connect which
  1502			 * can hold key data from a previous connection
  1503			 */
  1504			connect->key = NULL;
  1505			connect->key_len = 0;
  1506			connect->key_idx = 0;
  1507		}
  1508	
  1509		wdev->connect_keys = connkeys;
  1510		memcpy(wdev->u.client.ssid, connect->ssid, connect->ssid_len);
  1511		wdev->u.client.ssid_len = connect->ssid_len;
  1512	
  1513		wdev->conn_bss_type = connect->pbss ? IEEE80211_BSS_TYPE_PBSS :
  1514						      IEEE80211_BSS_TYPE_ESS;
  1515	
  1516		if (!rdev->ops->connect)
  1517			err = cfg80211_sme_connect(wdev, connect, prev_bssid);
  1518		else
  1519			err = rdev_connect(rdev, dev, connect);
  1520	
  1521		if (err) {
  1522			wdev->connect_keys = NULL;
  1523			/*
  1524			 * This could be reassoc getting refused, don't clear
  1525			 * ssid_len in that case.
  1526			 */
  1527			if (!wdev->connected)
  1528				wdev->u.client.ssid_len = 0;
  1529			return err;
  1530		}
  1531	
  1532		return 0;
  1533	}
  1534	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[kernel test robot]

Hi Kevin,

kernel test robot noticed the following build errors:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on wireless/main linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20230602225751.164525-1-kglund%40google.com
patch subject: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID
config: mips-ath79_defconfig (https://download.01.org/0day-ci/archive/20230603/202306031354.S6fuhpkH-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kevin-Lund/wifi-mwifiex-Stop-rejecting-connection-attempts-while-connected/20230603-065907
        git checkout 09f9fe87fe3588d03dafcaf05b36b3e931f8c8eb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash net/wireless/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306031354.S6fuhpkH-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/wireless/sme.c:1454:30: error: no member named 'current_bss' in 'struct wireless_dev'
                   if (ether_addr_equal(wdev->current_bss->pub.bssid,
                                        ~~~~  ^
   1 error generated.


vim +1454 net/wireless/sme.c

  1418	
  1419	/*
  1420	 * API calls for nl80211/wext compatibility code
  1421	 */
  1422	int cfg80211_connect(struct cfg80211_registered_device *rdev,
  1423			     struct net_device *dev,
  1424			     struct cfg80211_connect_params *connect,
  1425			     struct cfg80211_cached_keys *connkeys,
  1426			     const u8 *prev_bssid)
  1427	{
  1428		struct wireless_dev *wdev = dev->ieee80211_ptr;
  1429		int err;
  1430	
  1431		ASSERT_WDEV_LOCK(wdev);
  1432	
  1433		/*
  1434		 * If we have an ssid_len, we're trying to connect or are
  1435		 * already connected, so reject a new SSID unless it's the
  1436		 * same (which is the case for re-association.)
  1437		 */
  1438		if (wdev->u.client.ssid_len &&
  1439		    (wdev->u.client.ssid_len != connect->ssid_len ||
  1440		     memcmp(wdev->u.client.ssid, connect->ssid, wdev->u.client.ssid_len)))
  1441			return -EALREADY;
  1442	
  1443		/*
  1444		 * If connected, reject (re-)association unless prev_bssid
  1445		 * matches the current BSSID. Also reject if the current BSSID matches
  1446		 * the desired BSSID.
  1447		 */
  1448		if (wdev->connected) {
  1449			if (!prev_bssid)
  1450				return -EALREADY;
  1451			if (!ether_addr_equal(prev_bssid,
  1452					      wdev->u.client.connected_addr))
  1453				return -ENOTCONN;
> 1454			if (ether_addr_equal(wdev->current_bss->pub.bssid,
  1455					     connect->bssid))
  1456				return -EALREADY;
  1457		}
  1458	
  1459		/*
  1460		 * Reject if we're in the process of connecting with WEP,
  1461		 * this case isn't very interesting and trying to handle
  1462		 * it would make the code much more complex.
  1463		 */
  1464		if (wdev->connect_keys)
  1465			return -EINPROGRESS;
  1466	
  1467		cfg80211_oper_and_ht_capa(&connect->ht_capa_mask,
  1468					  rdev->wiphy.ht_capa_mod_mask);
  1469		cfg80211_oper_and_vht_capa(&connect->vht_capa_mask,
  1470					   rdev->wiphy.vht_capa_mod_mask);
  1471	
  1472		if (connkeys && connkeys->def >= 0) {
  1473			int idx;
  1474			u32 cipher;
  1475	
  1476			idx = connkeys->def;
  1477			cipher = connkeys->params[idx].cipher;
  1478			/* If given a WEP key we may need it for shared key auth */
  1479			if (cipher == WLAN_CIPHER_SUITE_WEP40 ||
  1480			    cipher == WLAN_CIPHER_SUITE_WEP104) {
  1481				connect->key_idx = idx;
  1482				connect->key = connkeys->params[idx].key;
  1483				connect->key_len = connkeys->params[idx].key_len;
  1484	
  1485				/*
  1486				 * If ciphers are not set (e.g. when going through
  1487				 * iwconfig), we have to set them appropriately here.
  1488				 */
  1489				if (connect->crypto.cipher_group == 0)
  1490					connect->crypto.cipher_group = cipher;
  1491	
  1492				if (connect->crypto.n_ciphers_pairwise == 0) {
  1493					connect->crypto.n_ciphers_pairwise = 1;
  1494					connect->crypto.ciphers_pairwise[0] = cipher;
  1495				}
  1496			}
  1497		} else {
  1498			if (WARN_ON(connkeys))
  1499				return -EINVAL;
  1500	
  1501			/* connect can point to wdev->wext.connect which
  1502			 * can hold key data from a previous connection
  1503			 */
  1504			connect->key = NULL;
  1505			connect->key_len = 0;
  1506			connect->key_idx = 0;
  1507		}
  1508	
  1509		wdev->connect_keys = connkeys;
  1510		memcpy(wdev->u.client.ssid, connect->ssid, connect->ssid_len);
  1511		wdev->u.client.ssid_len = connect->ssid_len;
  1512	
  1513		wdev->conn_bss_type = connect->pbss ? IEEE80211_BSS_TYPE_PBSS :
  1514						      IEEE80211_BSS_TYPE_ESS;
  1515	
  1516		if (!rdev->ops->connect)
  1517			err = cfg80211_sme_connect(wdev, connect, prev_bssid);
  1518		else
  1519			err = rdev_connect(rdev, dev, connect);
  1520	
  1521		if (err) {
  1522			wdev->connect_keys = NULL;
  1523			/*
  1524			 * This could be reassoc getting refused, don't clear
  1525			 * ssid_len in that case.
  1526			 */
  1527			if (!wdev->connected)
  1528				wdev->u.client.ssid_len = 0;
  1529			return err;
  1530		}
  1531	
  1532		return 0;
  1533	}
  1534	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[Johannes Berg]

On Fri, 2023-06-02 at 16:57 -0600, Kevin Lund wrote:
> Within cfg80211_connect, reject the (re-)association request if we are
> already connected to the exact BSSID which is being requested. This
> prevents an unnecessary attempt to connect which in the best case
> leaves us back where we started.
> 
> There is precedent for behaving this way over on the userspace SME side
> of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
> several basic checks to ensure the connection attempt is reasonable, so
> this fits in that context.
> 

I don't think this is right - we should be able to reassoc back to the
same AP in some cases, for example in 11be this comes up when you want
to change the negotiated links.

johannes

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

[Brian Norris]

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@google.com>
> ---
>  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@google.com/

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

Re: [PATCH 1/2] wifi: cfg80211: Reject (re-)association to the same BSSID

[Kevin Lund]

Thanks for the response. I'm still a little bit confused because
AFAICT, over on the userspace SME side of things there are two
similar checks that reject both authentication [1] and association
[2] to a BSSID which we are already connected to. Wouldn't that
break the 11be flow which you're referring to?

In any case I'm ok with abandoning this patch. I'll resubmit the
other patch in this series [3] on its own.

[1] https://github.com/torvalds/linux/blob/master/net/wireless/mlme.c#L281
[2] https://github.com/torvalds/linux/blob/master/net/wireless/mlme.c#L342
[3] https://patchwork.kernel.org/project/linux-wireless/patch/20230602225751.164525-2-kglund@google.com/

Thanks, and apologies for my late reply,
Kevin

On Mon, Jun 5, 2023 at 10:42 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2023-06-02 at 16:57 -0600, Kevin Lund wrote:
> > Within cfg80211_connect, reject the (re-)association request if we are
> > already connected to the exact BSSID which is being requested. This
> > prevents an unnecessary attempt to connect which in the best case
> > leaves us back where we started.
> >
> > There is precedent for behaving this way over on the userspace SME side
> > of things in cfg80211_mlme_auth. Further, cfg80211_connect already makes
> > several basic checks to ensure the connection attempt is reasonable, so
> > this fits in that context.
> >
>
> I don't think this is right - we should be able to reassoc back to the
> same AP in some cases, for example in 11be this comes up when you want
> to change the negotiated links.
>
> johannes

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

[Kevin Lund]

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@chromium.org> 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@google.com>
> > ---
> >  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@google.com/
>
> 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

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

[Brian Norris]

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