[RFC 0/4] Allow live MAC address change

[RFC 0/4] Allow live MAC address change

[James Prestwood]

This set enables userspace to change the devices MAC address via RTNL
or CMD_CONNECT/CMD_AUTHENTICATE without bringing the device down then
back up. This has several benefits:

 - Simplifies user space logic. No longer requires tracking multiple
   RTNL/netlink message transactions (down, change mac, up, connect).
 - Drastically speeds up connection times when changing MAC on a
   per-connect basis.
 - Eliminates potential race conditions in userspace. At any time
   during the down/change/up sequence the device could be hot
   unplugged or something could go wrong in the kernel leaving
   userspace in an unknown state.

A new extended feature was added,
NL80211_EXT_FEATURE_LIVE_ADDRESS_CHANGE which is set if the device
supports this feature. This feature is enabled by default for mac80211
based drivers.

A new NL80211 attribute was added, NL80211_ATTR_MAC_TO_CHANGE. This
attribute can be used with CMD_CONNECT/CMD_AUTHENTICATE to change the
MAC address automatically without having to go through RTNL.

I have taken some timings for all 3 ways of changing the MAC. Powered
change via RTNL, non powered via RTNL, and changing through
CMD_CONNECT. All times were taken in microseconds and tested on an
Intel 7260 PCI wireless adapter:

Powered via RTNL:

Average: 294508.9
Min: 284523
Max: 300345

==================================
Non-Powered via RTNL:

Average: 14824.7
Min: 11037
Max: 17812

Speedup from powered change: 19.87x (average)

==================================
via CMD_CONNECT:

Average: 11848.7
Min: 9748
Max: 17152

Speedup from powered change: 24.86x (average)

==================================

The speed increases between powered and non-powered are quite
impressive. Besides the obvious speedup, the amount of code requred
for userspace to take advantage of this feature is extremely small
(via CMD_CONNECT, essentially a feature check and an extra attribute).
In addition, the changes/complexity to the kernel is minimal.

James Prestwood (4):
  nl80211: Add LIVE_ADDR_CHANGE feature
  {nl|cfg}80211: Support mac change as part of SME Connect
  mac80211: Support LIVE_ADDRESS_CHANGE feature
  {nl,cfg}nl80211: Support mac change for mlme_authenticate

 include/net/cfg80211.h       |  1 +
 include/uapi/linux/nl80211.h | 10 +++++++
 net/mac80211/iface.c         | 51 ++++++++++++++++++++++++++++++++++--
 net/mac80211/main.c          |  1 +
 net/wireless/core.h          |  4 ++-
 net/wireless/mlme.c          |  9 ++++++-
 net/wireless/nl80211.c       | 18 ++++++++++++-
 net/wireless/sme.c           |  9 ++++++-
 net/wireless/util.c          | 11 ++++++++
 9 files changed, 108 insertions(+), 6 deletions(-)

-- 
2.17.1

[RFC 1/4] nl80211: Add LIVE_ADDR_CHANGE feature

[James Prestwood]

Add a new feature bit signifying that the wireless hardware supports
changing the mac address while the underlying net_device is UP.  Note
that this is slightly different from IFF_LIVE_ADDR_CHANGE as additional
restrictions might be imposed by the hardware.  Typical restrictions
are:
	- No connection is active on this interface, e.g. carrier is off
	- No scan is in progress
	- No offchannel operation is in progress
---
 include/uapi/linux/nl80211.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index bf7c4222f512..0ceb945a08fb 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -5481,6 +5481,14 @@ enum nl80211_feature_flags {
  * @NL80211_EXT_FEATURE_SAE_OFFLOAD: Device wants to do SAE authentication in
  *	station mode (SAE password is passed as part of the connect command).
  *
+ * @NL80211_EXT_FEATURE_LIVE_ADDR_CHANGE: Device can perform a MAC address
+ *	change without having to bring the underlying network device down
+ *	first.  For example, in station mode this can be used to vary the
+ *	origin MAC address prior to a connection to a new AP for privacy
+ *	or other reasons.  Note that certain driver specific restrictions
+ *	might apply, e.g. no scans in progress, no offchannel operations
+ *	in progress, and no active connections.
+ *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5526,6 +5534,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_EXT_KEY_ID,
 	NL80211_EXT_FEATURE_STA_TX_PWR,
 	NL80211_EXT_FEATURE_SAE_OFFLOAD,
+	NL80211_EXT_FEATURE_LIVE_ADDRESS_CHANGE,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
-- 
2.17.1

[RFC 2/4] {nl|cfg}80211: Support mac change as part of SME Connect

[James Prestwood]

---
 include/net/cfg80211.h       |  1 +
 include/uapi/linux/nl80211.h |  1 +
 net/wireless/core.h          |  1 +
 net/wireless/nl80211.c       | 11 +++++++++++
 net/wireless/sme.c           |  7 +++++++
 net/wireless/util.c          | 11 +++++++++++
 6 files changed, 32 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5253e7f667bd..25eacbebfa29 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2553,6 +2553,7 @@ struct cfg80211_connect_params {
 	size_t fils_erp_rrk_len;
 	bool want_1x;
 	struct ieee80211_edmg edmg;
+	const u8 *mac_to_use;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0ceb945a08fb..1bb4ce58da67 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2834,6 +2834,7 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_WIPHY_EDMG_CHANNELS,
 	NL80211_ATTR_WIPHY_EDMG_BW_CONFIG,
+	NL80211_ATTR_MAC_TO_USE,
 
 	/* add attributes here, update the policy in nl80211.c */
 
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 77556c58d9ac..29e6ab2cf343 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -454,6 +454,7 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
 			  struct vif_params *params);
 void cfg80211_process_rdev_events(struct cfg80211_registered_device *rdev);
 void cfg80211_process_wdev_events(struct wireless_dev *wdev);
+int cfg80211_set_mac_to_use(struct net_device *dev, const u8 *mac);
 
 bool cfg80211_does_bw_fit_range(const struct ieee80211_freq_range *freq_range,
 				u32 center_freq_khz, u32 bw_khz);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 4565d7385884..0202a762b5c8 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -591,6 +591,8 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 					.len = SAE_PASSWORD_MAX_LEN },
 	[NL80211_ATTR_TWT_RESPONDER] = { .type = NLA_FLAG },
 	[NL80211_ATTR_HE_OBSS_PD] = NLA_POLICY_NESTED(he_obss_pd_policy),
+	[NL80211_ATTR_MAC_TO_USE] = { .type = NLA_EXACT_LEN_WARN,
+				      .len = ETH_ALEN },
 };
 
 /* policy for the key attributes */
@@ -10045,6 +10047,15 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
 		connect.flags |= CONNECT_REQ_EXTERNAL_AUTH_SUPPORT;
 	}
 
+	if (info->attrs[NL80211_ATTR_MAC_TO_USE]) {
+		if (!wiphy_ext_feature_isset(wiphy,
+				NL80211_EXT_FEATURE_LIVE_ADDRESS_CHANGE))
+			return -EOPNOTSUPP;
+
+		connect.mac_to_use =
+			nla_data(info->attrs[NL80211_ATTR_MAC_TO_USE]);
+	}
+
 	wdev_lock(dev->ieee80211_ptr);
 
 	err = cfg80211_connect(rdev, dev, &connect, connkeys,
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 7a6c38ddc65a..f164af33655f 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1242,11 +1242,18 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
 	wdev->conn_bss_type = connect->pbss ? IEEE80211_BSS_TYPE_PBSS :
 					      IEEE80211_BSS_TYPE_ESS;
 
+	if (connect->mac_to_use) {
+		err = cfg80211_set_mac_to_use(dev, connect->mac_to_use);
+		if (err)
+			goto fail;
+	}
+
 	if (!rdev->ops->connect)
 		err = cfg80211_sme_connect(wdev, connect, prev_bssid);
 	else
 		err = rdev_connect(rdev, dev, connect);
 
+fail:
 	if (err) {
 		wdev->connect_keys = NULL;
 		/*
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 92cb2cbb179b..06700431cba0 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2148,3 +2148,14 @@ bool cfg80211_iftype_allowed(struct wiphy *wiphy, enum nl80211_iftype iftype,
 	return false;
 }
 EXPORT_SYMBOL(cfg80211_iftype_allowed);
+
+int cfg80211_set_mac_to_use(struct net_device *dev, const u8 *mac)
+{
+	struct sockaddr sa;
+
+	sa.sa_family = dev->type;
+	memcpy(sa.sa_data, mac, ETH_ALEN);
+
+	return dev_set_mac_address(dev, &sa, NULL);
+}
+EXPORT_SYMBOL(cfg80211_set_mac_to_use);
-- 
2.17.1

[RFC 3/4] mac80211: Support LIVE_ADDRESS_CHANGE feature

[James Prestwood]

---
 net/mac80211/iface.c | 51 ++++++++++++++++++++++++++++++++++++++++++--
 net/mac80211/main.c  |  1 +
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 8dc6580e1787..16ef6b83e7ea 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -198,15 +198,57 @@ static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr,
 	return ret;
 }
 
+static int ieee80211_can_live_addr_change(struct ieee80211_sub_if_data *sdata)
+{
+	if (netif_carrier_ok(sdata->dev))
+		return -EBUSY;
+
+	switch (sdata->vif.type) {
+	case NL80211_IFTYPE_AP:
+	case NL80211_IFTYPE_P2P_GO:
+	case NL80211_IFTYPE_AP_VLAN:
+	case NL80211_IFTYPE_WDS:
+	case NL80211_IFTYPE_MESH_POINT:
+	case NL80211_IFTYPE_MONITOR:
+	case NL80211_IFTYPE_OCB:
+		/* No further checking required, when started or UP these
+		 * interface types set carrier
+		 */
+		break;
+	case NL80211_IFTYPE_ADHOC:
+		if (sdata->u.ibss.ssid_len != 0)
+			return -EBUSY;
+		break;
+	case NL80211_IFTYPE_STATION:
+	case NL80211_IFTYPE_P2P_CLIENT:
+		if (!list_empty(&sdata->local->roc_list) ||
+					!sdata->local->scanning)
+			return -EBUSY;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+
 static int ieee80211_change_mac(struct net_device *dev, void *addr)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	struct ieee80211_local *local = sdata->local;
 	struct sockaddr *sa = addr;
 	bool check_dup = true;
+	bool live = false;
 	int ret;
 
-	if (ieee80211_sdata_running(sdata))
-		return -EBUSY;
+	if (ieee80211_sdata_running(sdata)) {
+		ret = ieee80211_can_live_addr_change(sdata);
+		if (ret)
+			return ret;
+
+		live = true;
+	}
 
 	if (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
 	    !(sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE))
@@ -216,7 +258,11 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
 	if (ret)
 		return ret;
 
+	if (live)
+		drv_remove_interface(local, sdata);
 	ret = eth_mac_addr(dev, sa);
+	if (live)
+		drv_add_interface(local, sdata);
 
 	if (ret == 0)
 		memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
@@ -1871,6 +1917,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 			sdata->u.mgd.use_4addr = params->use_4addr;
 
 		ndev->features |= local->hw.netdev_features;
+		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 
 		netdev_set_default_ethtool_ops(ndev, &ieee80211_ethtool_ops);
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 29b9d57df1a3..0aea583e5e69 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -596,6 +596,7 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 		wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_TXQS);
 
 	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_RRM);
+	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_LIVE_ADDRESS_CHANGE);
 
 	wiphy->bss_priv_size = sizeof(struct ieee80211_bss);
 
-- 
2.17.1

[RFC 4/4] {nl,cfg}nl80211: Support mac change for mlme_authenticate

[James Prestwood]

---
 net/wireless/core.h    | 3 ++-
 net/wireless/mlme.c    | 9 ++++++++-
 net/wireless/nl80211.c | 7 ++++++-
 net/wireless/sme.c     | 2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index 29e6ab2cf343..869ab983692d 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -366,7 +366,8 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device *rdev,
 		       const u8 *ssid, int ssid_len,
 		       const u8 *ie, int ie_len,
 		       const u8 *key, int key_len, int key_idx,
-		       const u8 *auth_data, int auth_data_len);
+		       const u8 *auth_data, int auth_data_len,
+		       const u8 *mac_to_use);
 int cfg80211_mlme_assoc(struct cfg80211_registered_device *rdev,
 			struct net_device *dev,
 			struct ieee80211_channel *chan,
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index f9462010575f..adf411905e64 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -226,7 +226,8 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device *rdev,
 		       const u8 *ssid, int ssid_len,
 		       const u8 *ie, int ie_len,
 		       const u8 *key, int key_len, int key_idx,
-		       const u8 *auth_data, int auth_data_len)
+		       const u8 *auth_data, int auth_data_len,
+		       const u8 *mac_to_use)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_auth_request req = {
@@ -257,6 +258,12 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device *rdev,
 	if (!req.bss)
 		return -ENOENT;
 
+	if (mac_to_use) {
+		err = cfg80211_set_mac_to_use(dev, mac_to_use);
+		if (err)
+			return err;
+	}
+
 	err = rdev_auth(rdev, dev, &req);
 
 	cfg80211_put_bss(&rdev->wiphy, req.bss);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0202a762b5c8..db32eaaaa50e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -8898,6 +8898,7 @@ static int nl80211_authenticate(struct sk_buff *skb, struct genl_info *info)
 	enum nl80211_auth_type auth_type;
 	struct key_parse key;
 	bool local_state_change;
+	const u8 *mac_to_change = NULL;
 
 	if (!info->attrs[NL80211_ATTR_MAC])
 		return -EINVAL;
@@ -8993,6 +8994,10 @@ static int nl80211_authenticate(struct sk_buff *skb, struct genl_info *info)
 
 	local_state_change = !!info->attrs[NL80211_ATTR_LOCAL_STATE_CHANGE];
 
+	if (info->attrs[NL80211_ATTR_MAC_TO_USE])
+		mac_to_change = nla_data(
+				info->attrs[NL80211_ATTR_MAC_TO_USE]);
+
 	/*
 	 * Since we no longer track auth state, ignore
 	 * requests to only change local state.
@@ -9004,7 +9009,7 @@ static int nl80211_authenticate(struct sk_buff *skb, struct genl_info *info)
 	err = cfg80211_mlme_auth(rdev, dev, chan, auth_type, bssid,
 				 ssid, ssid_len, ie, ie_len,
 				 key.p.key, key.p.key_len, key.idx,
-				 auth_data, auth_data_len);
+				 auth_data, auth_data_len, mac_to_change);
 	wdev_unlock(dev->ieee80211_ptr);
 	return err;
 }
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index f164af33655f..f66ea48b73df 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -174,7 +174,7 @@ static int cfg80211_conn_do_work(struct wireless_dev *wdev,
 					  params->ssid, params->ssid_len,
 					  NULL, 0,
 					  params->key, params->key_len,
-					  params->key_idx, NULL, 0);
+					  params->key_idx, NULL, 0, NULL);
 	case CFG80211_CONN_AUTH_FAILED_TIMEOUT:
 		*treason = NL80211_TIMEOUT_AUTH;
 		return -ENOTCONN;
-- 
2.17.1

Re: [RFC 0/4] Allow live MAC address change

[Johannes Berg]

Hi James,

TBH, I'm still not really convinced.

> I have taken some timings for all 3 ways of changing the MAC. Powered
> change via RTNL, non powered via RTNL, and changing through
> CMD_CONNECT. All times were taken in microseconds and tested on an
> Intel 7260 PCI wireless adapter:

From where to where did you measure? I mean, clearly you cannot have
counted all the way to the connection?

> Powered via RTNL:
> 
> Average: 294508.9
> Min: 284523
> Max: 300345
> 
> ==================================
> Non-Powered via RTNL:
> 
> Average: 14824.7
> Min: 11037
> Max: 17812
> 
> Speedup from powered change: 19.87x (average)

I'm assuming that this version is the IFF_LIVE_ADDR_CHANGE + setting the
MAC address via RTNL?

If so, yeah, obviously not powering off the firmware will be much faster
than powering it off. That's an easy win really.

> ==================================
> via CMD_CONNECT:
> 
> Average: 11848.7
> Min: 9748
> Max: 17152
> 
> Speedup from powered change: 24.86x (average)

And this really only gives you a gain of 3ms.

That'd be nice, but like I said before, it's not the only thing we/you
should be thinking about.

One fundamental issue I have with this is that you're now combining
together temporary with persistent state changes. After a disconnection
(or connection failure), the interface usually goes back to its previous
state. With this change, you're keeping the MAC address modified though.

Sure, you don't care (because you're probably going use a new random
address later anyway), but these are still things we should consider in
an API.

I'll happily take the subset of the patches that implements the
IFF_LIVE_ADDR_CHANGE in mac80211, but I don't think the 3ms win there
wins over having a well-defined API.

johannes

Re: [RFC 0/4] Allow live MAC address change

[James Prestwood]

On Wed, 2019-09-11 at 11:09 +0200, Johannes Berg wrote:
> Hi James,
> 
> TBH, I'm still not really convinced.
> 
> > I have taken some timings for all 3 ways of changing the MAC.
> > Powered
> > change via RTNL, non powered via RTNL, and changing through
> > CMD_CONNECT. All times were taken in microseconds and tested on an
> > Intel 7260 PCI wireless adapter:
> 
> From where to where did you measure? I mean, clearly you cannot have
> counted all the way to the connection?

I could have made this a bit more clear. I initially did measure the
time to a full connection, including EAPoL, but the more I timed the
more chance there was for scheduling delays that really threw off the
results. Not that these results weren't valid, I just would have needed
to time many many more runs to get a decent averaged time. The method
of timings I took just isolated things a bit better.

For the three methods below I measured the time from the connection
initiation (either powering down via RTNL, changing MAC via RTNL, or
sending CMD_CONNECT) until we got a success callback from CMD_CONNECT,
including changing the MAC via RTNL in those cases.

> 
> > Powered via RTNL:
> > 
> > Average: 294508.9
> > Min: 284523
> > Max: 300345
> > 
> > ==================================
> > Non-Powered via RTNL:
> > 
> > Average: 14824.7
> > Min: 11037
> > Max: 17812
> > 
> > Speedup from powered change: 19.87x (average)
> 
> I'm assuming that this version is the IFF_LIVE_ADDR_CHANGE + setting
> the
> MAC address via RTNL?
> 
> If so, yeah, obviously not powering off the firmware will be much
> faster
> than powering it off. That's an easy win really.

Yep exactly.

> 
> > ==================================
> > via CMD_CONNECT:
> > 
> > Average: 11848.7
> > Min: 9748
> > Max: 17152
> > 
> > Speedup from powered change: 24.86x (average)
> 
> And this really only gives you a gain of 3ms.
> 
> That'd be nice, but like I said before, it's not the only thing
> we/you
> should be thinking about.
> 
> One fundamental issue I have with this is that you're now combining
> together temporary with persistent state changes. After a
> disconnection
> (or connection failure), the interface usually goes back to its
> previous
> state. With this change, you're keeping the MAC address modified
> though.
> 
> Sure, you don't care (because you're probably going use a new random
> address later anyway), but these are still things we should consider
> in
> an API.

Yeah, in IWD's case if this feature is supported we would be doing the
MAC change on every connection (unless already changed previously) for
privacy reasons.

Out of curiosity how this behavior is different than the power down +
RTNL MAC change (the current way of doing things)? If you power down
the device, change the MAC, then power up does that MAC get reset after
a disconnection/failure?

> 
> I'll happily take the subset of the patches that implements the
> IFF_LIVE_ADDR_CHANGE in mac80211, but I don't think the 3ms win there
> wins over having a well-defined API.

Sure, that would be great. That is definitely still a improvement to
the existing way of doing things.

Thanks,
James

> 
> johannes
> 

Re: [RFC 0/4] Allow live MAC address change

[Johannes Berg]

On Wed, 2019-09-11 at 08:54 -0700, James Prestwood wrote:
> 
> I could have made this a bit more clear. I initially did measure the
> time to a full connection, including EAPoL, but the more I timed the
> more chance there was for scheduling delays that really threw off the
> results. Not that these results weren't valid, I just would have needed
> to time many many more runs to get a decent averaged time. The method
> of timings I took just isolated things a bit better.

Sure, makes sense, and I didn't think you were doing that, I was just
wondering what exactly you did measure.

> For the three methods below I measured the time from the connection
> initiation (either powering down via RTNL, changing MAC via RTNL, or
> sending CMD_CONNECT) until we got a success callback from CMD_CONNECT,
> including changing the MAC via RTNL in those cases.

Ah, ok.

> Out of curiosity how this behavior is different than the power down +
> RTNL MAC change (the current way of doing things)? If you power down
> the device, change the MAC, then power up does that MAC get reset after
> a disconnection/failure?

No, of course not? But then you're explicitly issuing a command ("change
the MAC address") that is supposed to affect state indefinitely, vs.
issuing a command ("please connect") that isn't really meant to.

If there was one thing that we learned from wext, IMHO it was that
keeping all the state in the kernel is bad for you, and it's much better
to handle things if the state gets reset when you disconnect etc. In
most places that's what we do now and I think it has served us well, so
I'm very reluctant to mix things that need state in the kernel with
those that don't.

(You might not remember wext, but you'd have to issue a bunch of
commands in the right order, and it would keep all the state inbetween;
if you forgot to clear the BSSID after setting it, it'd be remembered
and you couldn't connect to a new AP unless you reset it, etc.)

Thanks,
johannes

Re: [RFC 0/4] Allow live MAC address change

[James Prestwood]

Hi Johannes,

> > Out of curiosity how this behavior is different than the power down
> > +
> > RTNL MAC change (the current way of doing things)? If you power
> > down
> > the device, change the MAC, then power up does that MAC get reset
> > after
> > a disconnection/failure?
> 
> No, of course not? But then you're explicitly issuing a command
> ("change
> the MAC address") that is supposed to affect state indefinitely, vs.
> issuing a command ("please connect") that isn't really meant to.

I see what your saying, but theses kind of state changes are all over
the place in other APIs, and undocumented: One example is
SCAN_FLAG_FLUSH clearing out the scanning state for all other
processes. I'm sure I could find more. If we documented this attribute
and behavior I don't see an issue.

This is also no different than changing the MAC via SET_INTERFACE and
having CMD_CONNECT fail; the MAC still persists but instead we payed an
extra 3ms for the same result.

I know 3ms doesn't seems like a lot but everything counts and from my
testing this is even a further 20% improvement to doing so with RTNL.
Plus the added simplicity to the userspace code/API. We have taken a
lot of time to optimize IWD's connection times, and everything counts.
The connection times are fast already, but when there is room for
improvement we will push for it, especially in situations like this
when the change is quite minimal and does not introduce much
complexity.

> 
> If there was one thing that we learned from wext, IMHO it was that
> keeping all the state in the kernel is bad for you, and it's much
> better
> to handle things if the state gets reset when you disconnect etc. In
> most places that's what we do now and I think it has served us well,
> so
> I'm very reluctant to mix things that need state in the kernel with
> those that don't.

I don't really agree that this change puts any more or less state in
the kernel. Even the current way of doing things userspace still needs
to maintain what it changed the MAC to, here is no different. And
again, documenting this attribute should leave no question about what
is happening.

> 
> (You might not remember wext, but you'd have to issue a bunch of
> commands in the right order, and it would keep all the state
> inbetween;

I was not around for that, but yes that sounds bad. The difference
though is we are not issuing a bunch of state changing commands in a
row to achieve a single goal. We are issuing one single command,
CMD_CONNECT or CMD_AUTHENTICATE.

Thanks,
James

> if you forgot to clear the BSSID after setting it, it'd be remembered
> and you couldn't connect to a new AP unless you reset it, etc.)
> 
> Thanks,
> johannes
> 

Re: [RFC 0/4] Allow live MAC address change

[Kalle Valo]

James Prestwood <prestwoj@gmail.com> writes:

> I know 3ms doesn't seems like a lot but everything counts and from my
> testing this is even a further 20% improvement to doing so with RTNL.
> Plus the added simplicity to the userspace code/API. We have taken a
> lot of time to optimize IWD's connection times, and everything counts.
> The connection times are fast already, but when there is room for
> improvement we will push for it, especially in situations like this
> when the change is quite minimal and does not introduce much
> complexity.

So what kind of _total_ connection times you get now?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC 0/4] Allow live MAC address change

[James Prestwood]

Hi Kalle,

On Fri, 2019-09-13 at 13:24 +0300, Kalle Valo wrote:
> James Prestwood <prestwoj@gmail.com> writes:
> 
> > I know 3ms doesn't seems like a lot but everything counts and from
> > my
> > testing this is even a further 20% improvement to doing so with
> > RTNL.
> > Plus the added simplicity to the userspace code/API. We have taken
> > a
> > lot of time to optimize IWD's connection times, and everything
> > counts.
> > The connection times are fast already, but when there is room for
> > improvement we will push for it, especially in situations like this
> > when the change is quite minimal and does not introduce much
> > complexity.
> 
> So what kind of _total_ connection times you get now?
> 

This really depends. Most of the optimizations I was referencing are
due to scanning optimizations and moving DHCP into IWD itself, but both
of these are kinda irrelevant in this case so I wont consider them.

With this change, looking at the time from CMD_CONNECT until EAPoL/key
setting has finished I calculated 111.4ms on average. This is about a
3.5x speed up from the current method (Power down + RTNL) which I
calculated to be 391.8ms average. Note, this is rough (averaged only 5
runs just now).

So the savings are still significant even if you look at the full
connection times. The difference between doing the MAC change with RTNL
vs CMD_CONNECT are not as drastic, but from my perspective I would say
what's the harm? Your gaining further speed ups with really no added
complexity.

Thanks,
James

Re: [RFC 0/4] Allow live MAC address change

[Kalle Valo]

James Prestwood <prestwoj@gmail.com> writes:

> On Fri, 2019-09-13 at 13:24 +0300, Kalle Valo wrote:
>> James Prestwood <prestwoj@gmail.com> writes:
>> 
>> > I know 3ms doesn't seems like a lot but everything counts and from
>> > my
>> > testing this is even a further 20% improvement to doing so with
>> > RTNL.
>> > Plus the added simplicity to the userspace code/API. We have taken
>> > a
>> > lot of time to optimize IWD's connection times, and everything
>> > counts.
>> > The connection times are fast already, but when there is room for
>> > improvement we will push for it, especially in situations like this
>> > when the change is quite minimal and does not introduce much
>> > complexity.
>> 
>> So what kind of _total_ connection times you get now?
>> 
>
> This really depends. Most of the optimizations I was referencing are
> due to scanning optimizations and moving DHCP into IWD itself, but both
> of these are kinda irrelevant in this case so I wont consider them.

For user experience scanning and DHCP are also important, what kind of
numbers you get when those are included? No need to have anything
precise, I would like just to get an understanding where we are
nowadays.

> With this change, looking at the time from CMD_CONNECT until EAPoL/key
> setting has finished I calculated 111.4ms on average. This is about a
> 3.5x speed up from the current method (Power down + RTNL) which I
> calculated to be 391.8ms average. Note, this is rough (averaged only 5
> runs just now).

Ok, thanks.

> So the savings are still significant even if you look at the full
> connection times. The difference between doing the MAC change with RTNL
> vs CMD_CONNECT are not as drastic, but from my perspective I would say
> what's the harm? Your gaining further speed ups with really no added
> complexity.

As you only provided one number it's clear that you are only working
with one driver. But for us it's not that simple, we have to support a
myriad of different types of hardware and there can be complications and
additions later on, even for simple features. Like the dynamic power
save support I submitted to mac80211 over 10 years which was supposed to
be simple, and still we talk almost every year how do we get it out of
mac80211 as it makes maintenance difficult.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC 0/4] Allow live MAC address change

[Denis Kenzior]

Hi Kalle,

> For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> 
precise, I would like just to get an understanding where we are> nowadays.

Scanning heavily depends on the RF environment and the hardware.  In our 
testing ath9k takes stupid long to scan for example.

But in a sort of best case scenario, using limited scan and no mac 
change, iwd connects in ~300ms.  People have reported that they have not 
finished opening their laptop screen and they're connected, so at that 
level of latency, every millisecond is important and totally worth 
fighting for.  Randomizing the MAC would penalize our connection times 
by 2X (300 ms at least).  And Android folks have reported the penalty to 
be as high as 3 seconds.  So this needs to be fixed.  And do note that 
this is a feature every modern OS implements by default.

> 
> As you only provided one number it's clear that you are only working
> with one driver. But for us it's not that simple, we have to support a

Please don't jump to conclusions like you seem to be doing here.  James 
gave you one number that is pretty typical.  If you want us to provide 
numbers for other drivers under given conditions, just ask.  We have a 
framework for timing these.

> myriad of different types of hardware and there can be complications and
> additions later on, even for simple features. Like the dynamic power
> save support I submitted to mac80211 over 10 years which was supposed to
> be simple, and still we talk almost every year how do we get it out of
> mac80211 as it makes maintenance difficult.
> 

I'm not sure what point you're trying to make here?

Regards,
-Denis

Re: [RFC 0/4] Allow live MAC address change

[Bob Marcan]

On Tue, 17 Sep 2019 10:40:49 -0500
Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Kalle,
> 
> > For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> precise, I would like just to get an understanding where we are> nowadays.  
> 
> Scanning heavily depends on the RF environment and the hardware.  In our testing ath9k takes stupid long to scan for example.
> 
> But in a sort of best case scenario, using limited scan and no mac change, iwd connects in ~300ms.  People have reported that they have not finished opening their laptop screen and they're connected, so at that level of latency, every millisecond is important and totally worth fighting for.  Randomizing the MAC would penalize our connection times by 2X (300 ms at least).  And Android folks have reported the penalty to be as high as 3 seconds.  So this needs to be fixed.  And do note that this is a feature every modern OS implements by default.

Randomizing the MAC is a stupid decision.
Do you realy expect that this will add something to the security?

B. Marčan

Re: [RFC 0/4] Allow live MAC address change

[Ben Greear]

On 9/17/19 11:44 AM, Bob Marcan wrote:
> On Tue, 17 Sep 2019 10:40:49 -0500
> Denis Kenzior <denkenz@gmail.com> wrote:
> 
>> Hi Kalle,
>>
>>> For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> precise, I would like just to get an understanding where we are> nowadays.
>>
>> Scanning heavily depends on the RF environment and the hardware.  In our testing ath9k takes stupid long to scan for example.
>>
>> But in a sort of best case scenario, using limited scan and no mac change, iwd connects in ~300ms.  People have reported that they have not finished opening their laptop screen and they're connected, so at that level of latency, every millisecond is important and totally worth fighting for.  Randomizing the MAC would penalize our connection times by 2X (300 ms at least).  And Android folks have reported the penalty to be as high as 3 seconds.  So this needs to be fixed.  And do note that this is a feature every modern OS implements by default.
> 
> Randomizing the MAC is a stupid decision.
> Do you realy expect that this will add something to the security?

It allows one to be more anonymous.

Thanks,
Ben

> 
> B. Marčan
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 0/4] Allow live MAC address change

[Marcel Holtmann]

Hi Bob,

>>> For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> precise, I would like just to get an understanding where we are> nowadays.  
>> 
>> Scanning heavily depends on the RF environment and the hardware.  In our testing ath9k takes stupid long to scan for example.
>> 
>> But in a sort of best case scenario, using limited scan and no mac change, iwd connects in ~300ms.  People have reported that they have not finished opening their laptop screen and they're connected, so at that level of latency, every millisecond is important and totally worth fighting for.  Randomizing the MAC would penalize our connection times by 2X (300 ms at least).  And Android folks have reported the penalty to be as high as 3 seconds.  So this needs to be fixed.  And do note that this is a feature every modern OS implements by default.
> 
> Randomizing the MAC is a stupid decision.
> Do you realy expect that this will add something to the security?

the mac randomization is about privacy.

Regards

Marcel

Re: [RFC 0/4] Allow live MAC address change

[Steve deRosier]

On Tue, Sep 17, 2019 at 11:44 AM Bob Marcan <bob.marcan@gmail.com> wrote:
>
> On Tue, 17 Sep 2019 10:40:49 -0500
> Denis Kenzior <denkenz@gmail.com> wrote:
>
> > Hi Kalle,
> >
> > > For user experience scanning and DHCP are also important, what kind of > numbers you get when those are included? No need to have anything> precise, I would like just to get an understanding where we are> nowadays.
> >
> > Scanning heavily depends on the RF environment and the hardware.  In our testing ath9k takes stupid long to scan for example.
> >
> > But in a sort of best case scenario, using limited scan and no mac change, iwd connects in ~300ms.  People have reported that they have not finished opening their laptop screen and they're connected, so at that level of latency, every millisecond is important and totally worth fighting for.  Randomizing the MAC would penalize our connection times by 2X (300 ms at least).  And Android folks have reported the penalty to be as high as 3 seconds.  So this needs to be fixed.  And do note that this is a feature every modern OS implements by default.
>
> Randomizing the MAC is a stupid decision.
> Do you realy expect that this will add something to the security?
>

Hi Bob,

Thank you for your post, but this is an opinion and adds nothing to
the technical discussion underway. Let's discuss the patches please.

Thanks,
- Steve

Re: [RFC 0/4] Allow live MAC address change

[James Prestwood]

Hi Kalle,

> 
> As you only provided one number it's clear that you are only working
> with one driver. But for us it's not that simple, we have to support
> a
> myriad of different types of hardware and there can be complications
> and
> additions later on, even for simple features. Like the dynamic power
> save support I submitted to mac80211 over 10 years which was supposed
> to
> be simple, and still we talk almost every year how do we get it out
> of
> mac80211 as it makes maintenance difficult.

As Denis said we are happy to test other drivers, granted I do not have
*that* many cards on hand to test. I understand the kernel needs to
support all kinds of hardware, yes, but what is your concern with this
patch specifically? What kinds of issues do you foresee this causing?
Or is it that you simply don't know any you'd like to find out?

Thanks,
James

Re: [RFC 0/4] Allow live MAC address change

[Johannes Berg]

On Wed, 2019-09-11 at 12:20 -0700, James Prestwood wrote:

> I see what your saying, but theses kind of state changes are all over
> the place in other APIs, and undocumented: One example is
> SCAN_FLAG_FLUSH clearing out the scanning state for all other
> processes.

Scanning always changes scan list state?

> I'm sure I could find more. If we documented this attribute
> and behavior I don't see an issue.

But I'm sure you could actually find an example :-)

That doesn't really mean it's the *right* thing to do though, IMHO.

Also, who says that this is the only thing? Next up, somebody wants to
randomize the MTU? Ok, probably not, but you could pick a random other
rtnetlink attribute and have nl80211 set it. Where do we stop?

Thinking this to the extreme - why not add an rtnetlink message
interpreter into this code? ;-)

Sure, none of that is really seriously likely to happen, but I'm really
not convinced we (more or less arbitrarily) need many ways of doing the
same thing in the kernel.

Either way, regardless of that discussion, I think it'd be good if you
could repost the patches for just the "quick win" that we can all agree
on, and then we can get those reviewed and into the tree before we need
to continue this discussion; after all, while we're discussing saving
about 3 milliseconds, you're still wasting around 280 :-)

(and the easy one can be done without affecting the other, just need to
reorder the patches and split them a bit differently)

johannes