[PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[PATCH 1/2] 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

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 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

[PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[James Prestwood]

Signed-off-by: James Prestwood <prestwoj@gmail.com>
---
 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

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

Hi,

I was tempted to apply this (sans the feature advertisement part that I
don't think should be in nl80211), but:

> 
> Signed-off-by: James Prestwood <prestwoj@gmail.com>

Please add a commit log.

> +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;

Can you please document why this is there? Maybe all of the conditions,
for that matter.

I'm not even entirely sure it _is_ needed - if we've still not created
the IBSS but are scanning for it or trying to merge the MAC address
won't really matter yet? Probably?

> +		break;
> +	case NL80211_IFTYPE_STATION:
> +	case NL80211_IFTYPE_P2P_CLIENT:
> +		if (!list_empty(&sdata->local->roc_list) ||
> +					!sdata->local->scanning)
> +			return -EBUSY;

AP, mesh and other interfaces *can* scan, so that test should be pulled
out to be generic - but then in fact all of them should probably be
generic - ROC maybe can't be done on other interfaces yet, but unless
you're going to check *which* interface is actually doing the ROC, you
should just make that a generic check that applies to all interfaces.

If you do care about this being more granular then you should check
*which* interface is scanning, and then you can still switch the MAC
address for *other* interfaces - but I'd still argue it should be
independent of interface type.

And, I'm confused, but isn't the polarity of the scanning check wrong?

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[James Prestwood]

On Fri, 2019-10-04 at 13:56 +0200, Johannes Berg wrote:
> Hi,
> 
> I was tempted to apply this (sans the feature advertisement part that
> I
> don't think should be in nl80211), but:
> 
> > 
> > Signed-off-by: James Prestwood <prestwoj@gmail.com>
> 
> Please add a commit log.
> 
> > +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;
> 
> Can you please document why this is there? Maybe all of the
> conditions,
> for that matter.
> 
> I'm not even entirely sure it _is_ needed - if we've still not
> created
> the IBSS but are scanning for it or trying to merge the MAC address
> won't really matter yet? Probably?

I guess its just paranoia, rather be safe than sorry. I can take this
out, but is "Probably?" a good reason? ;)

> 
> > +		break;
> > +	case NL80211_IFTYPE_STATION:
> > +	case NL80211_IFTYPE_P2P_CLIENT:
> > +		if (!list_empty(&sdata->local->roc_list) ||
> > +					!sdata->local->scanning)
> > +			return -EBUSY;
> 
> AP, mesh and other interfaces *can* scan, so that test should be
> pulled
> out to be generic - but then in fact all of them should probably be
> generic - ROC maybe can't be done on other interfaces yet, but unless
> you're going to check *which* interface is actually doing the ROC,
> you
> should just make that a generic check that applies to all interfaces.

Ok so no switch statement, simply just check that we aren't offchannel
or scanning. I guess this would then cover the IBSS case too.

> 
> If you do care about this being more granular then you should check
> *which* interface is scanning, and then you can still switch the MAC
> address for *other* interfaces - but I'd still argue it should be
> independent of interface type.

I think maybe in the future we might want this, but for now lets not
worry about it. But just to make sure we are on the same page, your
talking about e.g. hardware with multiple radios so you could be doing
offchannel work/scanning/connecting simultaneously without having to
wait for the current operation to complete?

> 
> And, I'm confused, but isn't the polarity of the scanning check
> wrong?

Ah yeah, after you pointed that out I realized 'scanning' is a bit
field. I should be doing:

test_bit(SCAN_HW_SCANNING, &sdata->local->scanning)

Feel free the merge this, but I haven t had a chance yet to look into
adding a flag to RTNL (based on what you said in your previous email).
Without some way of telling userspace this is supported, its kinda
useless IMO.

Either way I'll send another patch with these things addressed.

Thanks,
James

> 
> johannes
> 

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[James Prestwood]

Hi Johannes,

After thinking about this more:

On Fri, 2019-10-04 at 09:25 -0700, James Prestwood wrote:
> On Fri, 2019-10-04 at 13:56 +0200, Johannes Berg wrote:
> > Hi,
> > 
> > I was tempted to apply this (sans the feature advertisement part
> > that
> > I
> > don't think should be in nl80211), but:
> > 
> > > 
> > > Signed-off-by: James Prestwood <prestwoj@gmail.com>
> > 
> > Please add a commit log.
> > 
> > > +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;
> > 
> > Can you please document why this is there? Maybe all of the
> > conditions,
> > for that matter.
> > 
> > I'm not even entirely sure it _is_ needed - if we've still not
> > created
> > the IBSS but are scanning for it or trying to merge the MAC address
> > won't really matter yet? Probably?
> 
> I guess its just paranoia, rather be safe than sorry. I can take this
> out, but is "Probably?" a good reason? ;)
> 
> > 
> > > +		break;
> > > +	case NL80211_IFTYPE_STATION:
> > > +	case NL80211_IFTYPE_P2P_CLIENT:
> > > +		if (!list_empty(&sdata->local->roc_list) ||
> > > +					!sdata->local->scanning)
> > > +			return -EBUSY;
> > 
> > AP, mesh and other interfaces *can* scan, so that test should be
> > pulled
> > out to be generic - but then in fact all of them should probably be
> > generic - ROC maybe can't be done on other interfaces yet, but
> > unless
> > you're going to check *which* interface is actually doing the ROC,
> > you
> > should just make that a generic check that applies to all
> > interfaces.
> 
> Ok so no switch statement, simply just check that we aren't
> offchannel
> or scanning. I guess this would then cover the IBSS case too.
> 
> > 
> > If you do care about this being more granular then you should check
> > *which* interface is scanning, and then you can still switch the
> > MAC
> > address for *other* interfaces - but I'd still argue it should be
> > independent of interface type.

So yes these can scan, but this should be covered by the
netif_carrier_ok check which is done first. We can just remove the
switch entirely, but the roc_list/scanning check only matters for
station/p2p_client so checking for the other interface types is kinda
pointless and redundant.

Also I am not sure what you mean by *which* interface. This function is
called on a single interface, so checking what other interfaces are
doing seems strange...

> 
> > 
> > And, I'm confused, but isn't the polarity of the scanning check
> > wrong?
> 
> Ah yeah, after you pointed that out I realized 'scanning' is a bit
> field. I should be doing:
> 
> test_bit(SCAN_HW_SCANNING, &sdata->local->scanning)
> 
> Feel free the merge this, but I haven t had a chance yet to look into
> adding a flag to RTNL (based on what you said in your previous
> email).
> Without some way of telling userspace this is supported, its kinda
> useless IMO.
> 
> Either way I'll send another patch with these things addressed.
> 
> Thanks,
> James
> 
> > 
> > johannes
> > 
> 
> 

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

Hi,

> > > If you do care about this being more granular then you should check
> > > *which* interface is scanning, and then you can still switch the
> > > MAC
> > > address for *other* interfaces - but I'd still argue it should be
> > > independent of interface type.
> 
> So yes these can scan, but this should be covered by the
> netif_carrier_ok check which is done first.

Not sure what you mean by that.

You could have two interfaces, one which is scanning right now, right?
And then theoretically you don't care about the other one - it *should*
be OK to remove/re-add (with new MAC address) the one that *isn't*
scanning, right?

But we don't have that granularity here for anything - you're just
checking "sdata->local->something", and by going from sdata to local
you've now checked the whole NIC, not just a single interface on that
NIC.

Which is fine, no complaint from me, just in that case you end up
failing when really there isn't much need to fail. In fact, in a case
like this, actually clearing IFF_UP, changing address and setting IFF_UP
would work, concurrently with another interface scanning.

>  We can just remove the
> switch entirely, but the roc_list/scanning check only matters for
> station/p2p_client so checking for the other interface types is kinda
> pointless and redundant.

But it's also completely confusing to do it this way because you go from
"sdata" to "local", and at that point the data that you're working on is
no longer specific to that one interface, it's actually for the whole
NIC.

Basically what I'm saying is this: it's confusing and makes no sense to
do something like

	if (this_is_a_certain_netdev_type)
		check_some_global_data();

> Also I am not sure what you mean by *which* interface. This function is
> called on a single interface, so checking what other interfaces are
> doing seems strange...

My point exactly - but that's what you're doing here in the code. Now I
think perhaps without even realizing?

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

On 10/7/19 4:16 PM, Johannes Berg wrote:
> Hi,
> 
>>>> If you do care about this being more granular then you should check
>>>> *which* interface is scanning, and then you can still switch the
>>>> MAC
>>>> address for *other* interfaces - but I'd still argue it should be
>>>> independent of interface type.
>>
>> So yes these can scan, but this should be covered by the
>> netif_carrier_ok check which is done first.
> 
> Not sure what you mean by that.
> 
> You could have two interfaces, one which is scanning right now, right?
> And then theoretically you don't care about the other one - it *should*
> be OK to remove/re-add (with new MAC address) the one that *isn't*
> scanning, right?

Actually, I don't think you can?  Unless I'm missing something?  All the 
scan state is stored on struct ieee80211_local, so if that struct is 
allocated per phy as you point out below, then what you suggest is 
currently not possible?

> 
> But we don't have that granularity here for anything - you're just
> checking "sdata->local->something", and by going from sdata to local
> you've now checked the whole NIC, not just a single interface on that
> NIC.

Right.  But that seems to be a limitation of mac80211 actually.  We 
can't run two scans concurrently on different interfaces.  This is 
rather unintuitive given that scan requests require an ifindex/wdev.

Can this be changed / fixed in mac80211 actually?  I would expect that 
if a card supports p2p and station simultaneously, then it can scan / go 
offchannel on two interfaces simultaneously? Or not?  What can iwlwifi 
do for example?

> 
> Which is fine, no complaint from me, just in that case you end up
> failing when really there isn't much need to fail. In fact, in a case
> like this, actually clearing IFF_UP, changing address and setting IFF_UP
> would work, concurrently with another interface scanning.
> 
>>   We can just remove the
>> switch entirely, but the roc_list/scanning check only matters for
>> station/p2p_client so checking for the other interface types is kinda
>> pointless and redundant.
> 
> But it's also completely confusing to do it this way because you go from
> "sdata" to "local", and at that point the data that you're working on is
> no longer specific to that one interface, it's actually for the whole
> NIC.

I agree its confusing, but that seems to be how mac80211 works?

> 
> Basically what I'm saying is this: it's confusing and makes no sense to
> do something like
> 
> 	if (this_is_a_certain_netdev_type)
> 		check_some_global_data();
> 
>> Also I am not sure what you mean by *which* interface. This function is
>> called on a single interface, so checking what other interfaces are
>> doing seems strange...
> 
> My point exactly - but that's what you're doing here in the code. Now I
> think perhaps without even realizing?
> 

Given the above, I'm not sure I see anything wrong?  The switch/case can 
probably be gotten rid of, but it actually makes things clear that only 
station/p2p_device and adhoc are handled specially.

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

Hi,

> > You could have two interfaces, one which is scanning right now, right?
> > And then theoretically you don't care about the other one - it *should*
> > be OK to remove/re-add (with new MAC address) the one that *isn't*
> > scanning, right?
> 
> Actually, I don't think you can?  Unless I'm missing something?  All the 
> scan state is stored on struct ieee80211_local, so if that struct is 
> allocated per phy as you point out below, then what you suggest is 
> currently not possible?

?

The scan_req struct contains a reference to which interface is scanning,
so it should very well be possible to have

phy0:
 wlan0: IFF_UP & scanning
 wlan1: IFF_UP & change MAC address all the time

just like it's possible to change the MAC address when wlan1 *isn't*
IFF_UP even if wlan0 is scanning, right?

> > But we don't have that granularity here for anything - you're just
> > checking "sdata->local->something", and by going from sdata to local
> > you've now checked the whole NIC, not just a single interface on that
> > NIC.
> 
> Right.  But that seems to be a limitation of mac80211 actually.  We 
> can't run two scans concurrently on different interfaces.  This is 
> rather unintuitive given that scan requests require an ifindex/wdev.
> 
> Can this be changed / fixed in mac80211 actually?  I would expect that 
> if a card supports p2p and station simultaneously, then it can scan / go 
> offchannel on two interfaces simultaneously? Or not?  What can iwlwifi 
> do for example?

No, this typically cannot be fixed, and it doesn't really make sense.
The NIC cannot possibly do two scans at a time since it has only a
single radio resource :-)

> > But it's also completely confusing to do it this way because you go from
> > "sdata" to "local", and at that point the data that you're working on is
> > no longer specific to that one interface, it's actually for the whole
> > NIC.
> 
> I agree its confusing, but that seems to be how mac80211 works?

See above.

> Given the above, I'm not sure I see anything wrong?  The switch/case can 
> probably be gotten rid of, but it actually makes things clear that only 
> station/p2p_device and adhoc are handled specially.

I just don't think they *should* be handled specially.

Given your code now, you can have

phy0:
 wlan0: STATION, IFF_UP & something is doing remain-on-channel
 wlan1: STATION, IFF_UP

--> cannot change wlan1 MAC address

phy0:
 wlan0: STATION, IFF_UP & something is doing remain-on-channel

wlan1: AP, IFF_UP

--> *can* change wlan1 MAC address

This doesn't really make much sense?

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

On 10/8/19 10:52 AM, Johannes Berg wrote:
> Hi,
> 
>>> You could have two interfaces, one which is scanning right now, right?
>>> And then theoretically you don't care about the other one - it *should*
>>> be OK to remove/re-add (with new MAC address) the one that *isn't*
>>> scanning, right?
>>
>> Actually, I don't think you can?  Unless I'm missing something?  All the
>> scan state is stored on struct ieee80211_local, so if that struct is
>> allocated per phy as you point out below, then what you suggest is
>> currently not possible?
> 
> ?
> 
> The scan_req struct contains a reference to which interface is scanning,
> so it should very well be possible to have
> 
> phy0:
>   wlan0: IFF_UP & scanning
>   wlan1: IFF_UP & change MAC address all the time
> 
> just like it's possible to change the MAC address when wlan1 *isn't*
> IFF_UP even if wlan0 is scanning, right?
> 

Indeed.  But that is not what you were suggesting earlier with just 
checking local->scanning.  So if scan_req contains a wdev, then yes it 
should be possible to compare the scan_req->wdev to the interface being 
changed.

>>> But we don't have that granularity here for anything - you're just
>>> checking "sdata->local->something", and by going from sdata to local
>>> you've now checked the whole NIC, not just a single interface on that
>>> NIC.
>>
>> Right.  But that seems to be a limitation of mac80211 actually.  We
>> can't run two scans concurrently on different interfaces.  This is
>> rather unintuitive given that scan requests require an ifindex/wdev.
>>
>> Can this be changed / fixed in mac80211 actually?  I would expect that
>> if a card supports p2p and station simultaneously, then it can scan / go
>> offchannel on two interfaces simultaneously? Or not?  What can iwlwifi
>> do for example?
> 
> No, this typically cannot be fixed, and it doesn't really make sense.
> The NIC cannot possibly do two scans at a time since it has only a
> single radio resource :-)

So why is the scan request not per phy then?  And should mac address 
even affect the ongoing scan?  Can we simply change it with the scan 
ongoing?

> 
>> Given the above, I'm not sure I see anything wrong?  The switch/case can
>> probably be gotten rid of, but it actually makes things clear that only
>> station/p2p_device and adhoc are handled specially.
> 
> I just don't think they *should* be handled specially.
> 

Fair enough.

> Given your code now, you can have
> 
> phy0:
>   wlan0: STATION, IFF_UP & something is doing remain-on-channel
>   wlan1: STATION, IFF_UP
> 
> --> cannot change wlan1 MAC address
> 
> phy0:
>   wlan0: STATION, IFF_UP & something is doing remain-on-channel
> 
> wlan1: AP, IFF_UP
> 
> --> *can* change wlan1 MAC address
> 
> This doesn't really make much sense?
> 

Agreed.

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

Hi,

> > The scan_req struct contains a reference to which interface is scanning,
> > so it should very well be possible to have
> > 
> > phy0:
> >   wlan0: IFF_UP & scanning
> >   wlan1: IFF_UP & change MAC address all the time
> > 
> > just like it's possible to change the MAC address when wlan1 *isn't*
> > IFF_UP even if wlan0 is scanning, right?
> > 
> 
> Indeed.  But that is not what you were suggesting earlier with just 
> checking local->scanning.  So if scan_req contains a wdev, then yes it 
> should be possible to compare the scan_req->wdev to the interface being 
> changed.

Well, yes, but only because I was incrementally going from James's
patch, which was checking that only.

Similar with the other local-> things being checked here, btw, though in
some cases it might be harder to actually determine which wdev is doing
something and which isn't.

> > No, this typically cannot be fixed, and it doesn't really make sense.
> > The NIC cannot possibly do two scans at a time since it has only a
> > single radio resource :-)
> 
> So why is the scan request not per phy then?  And should mac address 
> even affect the ongoing scan?  Can we simply change it with the scan 
> ongoing?

There are things that affect the scan from the interface, e.g.
capability overrides, (extended) capabilities, the MAC address is used
unless randomization is requested, etc.

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

>> Indeed.  But that is not what you were suggesting earlier with just
>> checking local->scanning.  So if scan_req contains a wdev, then yes it
>> should be possible to compare the scan_req->wdev to the interface being
>> changed.
> 
> Well, yes, but only because I was incrementally going from James's
> patch, which was checking that only.

Well, something to improve.  Sometimes it is pretty hard to figure out 
what you mean.

> 
> Similar with the other local-> things being checked here, btw, though in
> some cases it might be harder to actually determine which wdev is doing
> something and which isn't.

Right

> 
>>> No, this typically cannot be fixed, and it doesn't really make sense.
>>> The NIC cannot possibly do two scans at a time since it has only a
>>> single radio resource :-)
>>
>> So why is the scan request not per phy then?  And should mac address
>> even affect the ongoing scan?  Can we simply change it with the scan
>> ongoing?
> 
> There are things that affect the scan from the interface, e.g.
> capability overrides, (extended) capabilities, the MAC address is used
> unless randomization is requested, etc.
> 

But they shouldn't change due a mac address change?  I wonder if we can 
further relax the requirements to allow mac change if 
NL80211_SCAN_FLAG_RANDOM_ADDR was used?

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

> > > > No, this typically cannot be fixed, and it doesn't really make sense.
> > > > The NIC cannot possibly do two scans at a time since it has only a
> > > > single radio resource :-)
> > > 
> > > So why is the scan request not per phy then?  And should mac address
> > > even affect the ongoing scan?  Can we simply change it with the scan
> > > ongoing?
> > 
> > There are things that affect the scan from the interface, e.g.
> > capability overrides, (extended) capabilities, the MAC address is used
> > unless randomization is requested, etc.
> > 
> 
> But they shouldn't change due a mac address change?  I wonder if we can 
> further relax the requirements to allow mac change if 
> NL80211_SCAN_FLAG_RANDOM_ADDR was used?

No, at least with HW scan that would completely confuse the driver -
since from the driver's POV we'd remove the interface it's currently
managing the scan for.

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

>> But they shouldn't change due a mac address change?  I wonder if we can
>> further relax the requirements to allow mac change if
>> NL80211_SCAN_FLAG_RANDOM_ADDR was used?
> 
> No, at least with HW scan that would completely confuse the driver -
> since from the driver's POV we'd remove the interface it's currently
> managing the scan for.

So help me understand this better.  Just by virtue of copying the new 
mac into sdata->vif.addr we'd be confusing the driver such that it can't 
associate the ongoing scan request to the wdev it was started on?  Even 
when it has all this info?  I mean you have a single scan request on a 
phy, how hard can this be? :)

Note that some apps perform poor-man's scan address randomization by 
varying the mac (I assume prior to each nth scan).  So being able to 
change the mac while scanning might be a boon to them.  I personally 
don't think changing mac via rtnl to accomplish this is a great idea, 
but just tossing it out for you.

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

On Tue, 2019-10-08 at 13:50 -0500, Denis Kenzior wrote:
> Hi Johannes,
> 
> > > But they shouldn't change due a mac address change?  I wonder if we can
> > > further relax the requirements to allow mac change if
> > > NL80211_SCAN_FLAG_RANDOM_ADDR was used?
> > 
> > No, at least with HW scan that would completely confuse the driver -
> > since from the driver's POV we'd remove the interface it's currently
> > managing the scan for.
> 
> So help me understand this better.

include/net/mac80211.h:

       int (*hw_scan)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
                      struct ieee80211_scan_request *req);

How is it difficult to understand that with an API like that, it might
not be a good idea to remove the vif from the driver while it's
scanning?


>   Just by virtue of copying the new 
> mac into sdata->vif.addr 

That's not what happens?

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

On 10/8/19 3:16 PM, Johannes Berg wrote:
> On Tue, 2019-10-08 at 13:50 -0500, Denis Kenzior wrote:
>> Hi Johannes,
>>
>>>> But they shouldn't change due a mac address change?  I wonder if we can
>>>> further relax the requirements to allow mac change if
>>>> NL80211_SCAN_FLAG_RANDOM_ADDR was used?
>>>
>>> No, at least with HW scan that would completely confuse the driver -
>>> since from the driver's POV we'd remove the interface it's currently
>>> managing the scan for.
>>
>> So help me understand this better.
> 
> include/net/mac80211.h:
> 
>         int (*hw_scan)(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                        struct ieee80211_scan_request *req);
> 
> How is it difficult to understand that with an API like that, it might
> not be a good idea to remove the vif from the driver while it's
> scanning?

Right, so you're talking in the context of this implementation which 
performs a remove/add interface.  You're right about that.

But I was asking more in general terms.  If all we're doing is scanning, 
can we just change the mac?  Anyway, not important.  Maybe I bring this 
up once this set is accepted.

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

On Tue, 2019-10-08 at 15:55 -0500, Denis Kenzior wrote:

> Right, so you're talking in the context of this implementation which 
> performs a remove/add interface.  You're right about that.
> 
> But I was asking more in general terms.  If all we're doing is scanning, 
> can we just change the mac?  Anyway, not important.  Maybe I bring this 
> up once this set is accepted.

Maybe, but honestly, I'm not convinced the complexity would be worth it.
You'd have to push this all the way through to the driver, so it knows
to do it, or defer it until the scan is done, or something? Not
something you'd want to do on all hardware while a non-randomized scan
is running, for example (iwlwifi might actually be OK).

Or you could perhaps cache the MAC address change in mac80211 and apply
it at the next possible point in time - but then again you have to be
really careful to actually apply it and block all further operations,
even if a bunch of remain-on-channel's are active and you request a new
scan, that has to be blocked until the remain-on-channel is done *and*
the MAC address change is applied?

Seems rather complex for very little value.

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

On Fri, 2019-10-04 at 09:25 -0700, James Prestwood wrote:
> 
> > I'm not even entirely sure it _is_ needed - if we've still not
> > created
> > the IBSS but are scanning for it or trying to merge the MAC address
> > won't really matter yet? Probably?
> 
> I guess its just paranoia, rather be safe than sorry. I can take this
> out, but is "Probably?" a good reason? ;)

Fair enough, nobody really cares about IBSS anyway ;-)

I guess I was mostly wondering if you had noticed anything that would
actually be a problem.

> Ok so no switch statement, simply just check that we aren't offchannel
> or scanning. I guess this would then cover the IBSS case too.

Don't think it covers IBSS - that one is really specially accessing some
IBSS data.

> > If you do care about this being more granular then you should check
> > *which* interface is scanning, and then you can still switch the MAC
> > address for *other* interfaces - but I'd still argue it should be
> > independent of interface type.
> 
> I think maybe in the future we might want this, but for now lets not
> worry about it. But just to make sure we are on the same page, your
> talking about e.g. hardware with multiple radios so you could be doing
> offchannel work/scanning/connecting simultaneously without having to
> wait for the current operation to complete?

Not really multiple radios, who cares? Just multiple interfaces would be
sufficient. You're just removing/adding some interface (as far as the
driver is concerned) - doesn't matter if you actually are scanning or
something on another interface right?

> > And, I'm confused, but isn't the polarity of the scanning check
> > wrong?
> 
> Ah yeah, after you pointed that out I realized 'scanning' is a bit
> field. I should be doing:
> 
> test_bit(SCAN_HW_SCANNING, &sdata->local->scanning)

I think checking for all the bits is fine (and necessary, just HW scan
is unlikely to be enough, changing the MAC address would also disrupt a
software scan) - just need to invert the polarity?


> Either way I'll send another patch with these things addressed.

I'll wait, thanks.

johannes

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Denis Kenzior]

Hi Johannes,

>>> And, I'm confused, but isn't the polarity of the scanning check
>>> wrong?
>>
>> Ah yeah, after you pointed that out I realized 'scanning' is a bit
>> field. I should be doing:
>>
>> test_bit(SCAN_HW_SCANNING, &sdata->local->scanning)
> 
> I think checking for all the bits is fine (and necessary, just HW scan
> is unlikely to be enough, changing the MAC address would also disrupt a
> software scan) - just need to invert the polarity?

I concur that scanning should be checked as
if (sdata->local->scanning).  So Johannes you're right that the polarity 
is reversed.  However, __ieee80211_start_scan seems to check for 
local->scan_req instead to take deferred scans into account.  So I 
wonder if that is a better approach.

Regards,
-Denis

Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature

[Johannes Berg]

Hi,

> I concur that scanning should be checked as
> if (sdata->local->scanning).  So Johannes you're right that the polarity 
> is reversed.  However, __ieee80211_start_scan seems to check for 
> local->scan_req instead to take deferred scans into account.  So I 
> wonder if that is a better approach.

Hmm. I don't think it's necessary.

We basically only get to that kind of state if ieee80211_can_scan()
returned false - then we assign local->scan_req and defer until
ieee80211_run_deferred_scan() is called.

But in the meantime, nothing in the scan requests references the MAC
address.

It does mean that we should grab local->mtx though for these checks, and
then all around the interface change, so that we can be sure we don't
actually start scanning in the middle of the changes here.

johannes

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[Johannes Berg]

On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote:
> 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

Hmm, what do you need this patch for?

IFF_LIVE_ADDR_CHANGE should be sufficient to discover it?

johannes

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[James Prestwood]

Hi,

On Fri, 2019-09-13 at 22:48 +0200, Johannes Berg wrote:
> On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote:
> > 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
> 
> Hmm, what do you need this patch for?
> 
> IFF_LIVE_ADDR_CHANGE should be sufficient to discover it?

Because userspace needs to know if this is supported?
IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no way of
obtaining this.

Thanks,
James

> 
> johannes
> 

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[Johannes Berg]

On Fri, 2019-09-13 at 13:56 -0700, James Prestwood wrote:
> Hi,
> 
> On Fri, 2019-09-13 at 22:48 +0200, Johannes Berg wrote:
> > On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote:
> > > 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
> > 
> > Hmm, what do you need this patch for?
> > 
> > IFF_LIVE_ADDR_CHANGE should be sufficient to discover it?
> 
> Because userspace needs to know if this is supported?
> IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no way of
> obtaining this.

Oh, annoying.

But that doesn't really mean that nl80211 is an appropriate place to
advertise it, IMHO?

And in nl80211 you'd need the flag for if you actually have the "change
MAC address during connect" attribute.

johannes

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[James Prestwood]

On Fri, 2019-09-13 at 23:01 +0200, Johannes Berg wrote:
> On Fri, 2019-09-13 at 13:56 -0700, James Prestwood wrote:
> > Hi,
> > 
> > On Fri, 2019-09-13 at 22:48 +0200, Johannes Berg wrote:
> > > On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote:
> > > > 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
> > > 
> > > Hmm, what do you need this patch for?
> > > 
> > > IFF_LIVE_ADDR_CHANGE should be sufficient to discover it?
> > 
> > Because userspace needs to know if this is supported?
> > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no
> > way of
> > obtaining this.
> 
> Oh, annoying.
> 
> But that doesn't really mean that nl80211 is an appropriate place to
> advertise it, IMHO?

The intention of the flag was not soley related to
CMD_CONNECT/CMD_AUTHENTICATE. Its an indication that the
hardware/driver supports a live address change. If you don't want it
here could you suggest a better location for it?

> 
> And in nl80211 you'd need the flag for if you actually have the
> "change
> MAC address during connect" attribute.
> 
> johannes
> 

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[James Prestwood]

Johannes,

On Fri, 2019-09-13 at 14:14 -0700, James Prestwood wrote:
> On Fri, 2019-09-13 at 23:01 +0200, Johannes Berg wrote:
> > On Fri, 2019-09-13 at 13:56 -0700, James Prestwood wrote:
> > > Hi,
> > > 
> > > On Fri, 2019-09-13 at 22:48 +0200, Johannes Berg wrote:
> > > > On Fri, 2019-09-13 at 12:59 -0700, James Prestwood wrote:
> > > > > 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
> > > > 
> > > > Hmm, what do you need this patch for?
> > > > 
> > > > IFF_LIVE_ADDR_CHANGE should be sufficient to discover it?
> > > 
> > > Because userspace needs to know if this is supported?
> > > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no
> > > way of
> > > obtaining this.
> > 
> > Oh, annoying.
> > 
> > But that doesn't really mean that nl80211 is an appropriate place
> > to
> > advertise it, IMHO?
> 
> The intention of the flag was not soley related to
> CMD_CONNECT/CMD_AUTHENTICATE. Its an indication that the
> hardware/driver supports a live address change. If you don't want it
> here could you suggest a better location for it?

Could we continue this discussion? Is there some other way you can
think of to let userspace know that this feature is supported rather
than an ext feature?

Thanks,
James
> 
> > 
> > And in nl80211 you'd need the flag for if you actually have the
> > "change
> > MAC address during connect" attribute.
> > 
> > johannes
> > 
> 
> 

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[Johannes Berg]

Hi,

> > > Because userspace needs to know if this is supported?
> > > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has no
> > > way of
> > > obtaining this.
> > 
> > Oh, annoying.
> > 
> > But that doesn't really mean that nl80211 is an appropriate place to
> > advertise it, IMHO?
> 
> The intention of the flag was not soley related to
> CMD_CONNECT/CMD_AUTHENTICATE. Its an indication that the
> hardware/driver supports a live address change. If you don't want it
> here could you suggest a better location for it?

I guess RTNL would be the right place? This can hardly be specific to
wireless, the flag comes from elsewhere.

johannes

Re: [PATCH 1/2] nl80211: Add LIVE_ADDR_CHANGE feature

[James Prestwood]

Hi Johannes,

On Tue, 2019-10-01 at 11:14 +0200, Johannes Berg wrote:
> Hi,
> 
> > > > Because userspace needs to know if this is supported?
> > > > IFF_LIVE_ADDR_CHANGE is a private flag... AFAIK userspace has
> > > > no
> > > > way of
> > > > obtaining this.
> > > 
> > > Oh, annoying.
> > > 
> > > But that doesn't really mean that nl80211 is an appropriate place
> > > to
> > > advertise it, IMHO?
> > 
> > The intention of the flag was not soley related to
> > CMD_CONNECT/CMD_AUTHENTICATE. Its an indication that the
> > hardware/driver supports a live address change. If you don't want
> > it
> > here could you suggest a better location for it?
> 
> I guess RTNL would be the right place? This can hardly be specific to
> wireless, the flag comes from elsewhere.

I do see where your coming from, and maybe the answer is both RTNL and
NL80211?

In this context a NL80211 flag would mean something different than only
putting a flag into RTNL.

The NL80211 flag means this device supports changing the MAC while
running and not offchannel/scanning. Which is what -EBUSY would mean in
this context. Managing the offchannel/scanning work is only possible
with a NL80211 application.

An RTNL only application has no idea about scanning/offchannel work, so
it has no way of knowing what -EBUSY means in the wireless device
context, or a way to prevent this from happening (stopping scanning or
offchannel before changing the MAC).

Now, another example would be an RTNL application that only cares about
non-wireless devices (like ethernet), and in this case it could benefit
from some type of RTNL specific flag and not care about a NL80211 flag.

Since there is no such application that cares about this RTNL flag
(since it doesn't exist) I would say that adding NL80211 flag is the
way to go. If someone wants to add an RTNL flag I would say that is
appropriate too. But in terms of this feature, a NL80211 flag is really
what fits best since the changes are wireless specific.

Thanks,
James

> 
> johannes
>