| Message ID | 20220811231338.563794-3-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Support for changing address while powered | expand |
On Thu, 2022-08-11 at 16:13 -0700, James Prestwood wrote: > @@ -217,7 +275,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) > + ret = drv_add_interface(local, sdata); > > if (ret == 0) > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > I still don't like the (lack of) error checking here. As far as I know, eth_mac_addr() can very happily fail if the passed address is invalid, so we really shouldn't overwrite the ret value by drv_add_interface(). Also, it seems like we should only add the interface again after updating sdata->vif.addr (last context line), so that the driver actually knows ... otherwise I'm not sure how this patch would have much effect (unless it updates the FW all the time like iwlwifi, which I guess is where you tested it, based on the rationale...) johannes
Hi Johannes, On Thu, 2022-08-25 at 11:02 +0200, Johannes Berg wrote: > On Thu, 2022-08-11 at 16:13 -0700, James Prestwood wrote: > > @@ -217,7 +275,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) > > + ret = drv_add_interface(local, sdata); > > > > if (ret == 0) > > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > > > > I still don't like the (lack of) error checking here. As far as I know, > eth_mac_addr() can very happily fail if the passed address is invalid, > so we really shouldn't overwrite the ret value by drv_add_interface(). Ah yes, that was an oversight. I assume we do want to add_interface even if eth_mac_addr fails though. So my only question is about the return from drv_add_interface(). Is this unlikely to fail? If so would just a WARN_ON be sufficient and return the value from eth_mac_addr()? So something like: if (live) drv_remove_interface(local, sdata); ret = eth_mac_addr(dev, sa); if (ret == 0) memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); if (live) WARN_ON(drv_add_interface(local, sdata)); return ret; > > Also, it seems like we should only add the interface again after > updating sdata->vif.addr (last context line), so that the driver > actually knows ... otherwise I'm not sure how this patch would have > much > effect (unless it updates the FW all the time like iwlwifi, which I > guess is where you tested it, based on the rationale...) Yes, this is where I tested it. But yeah makes sense, I'll move copying the address to before add_interface. Thanks, James > > johannes
Hi, > > > + if (live) > > > + drv_remove_interface(local, sdata); > > > ret = eth_mac_addr(dev, sa); > > > + if (live) > > > + ret = drv_add_interface(local, sdata); > > > > > > if (ret == 0) > > > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > > > > > > > I still don't like the (lack of) error checking here. As far as I know, > > eth_mac_addr() can very happily fail if the passed address is invalid, > > so we really shouldn't overwrite the ret value by drv_add_interface(). > > Ah yes, that was an oversight. I assume we do want to add_interface > even if eth_mac_addr fails though. Right. > So my only question is about the > return from drv_add_interface(). Is this unlikely to fail? If so would > just a WARN_ON be sufficient and return the value from eth_mac_addr()? Hm, yeah, I guess it really ought to not fail here. > So something like: > > if (live) > drv_remove_interface(local, sdata); > ret = eth_mac_addr(dev, sa); > if (ret == 0) > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > > if (live) > WARN_ON(drv_add_interface(local, sdata)); > > return ret; Seems reasonable. We could do something like err = drv_add_interface(...); if (err) { dev_close(...); ret = ret ?: err; } or something, but not sure that's worth it, it really shouldn't fail at this point. But I guess we could leave setting NL80211_EXT_FEATURE_AUTH_TX_RANDOM_TA to the driver if we think it'd be less risky that way? johannes
On Thu, 2022-08-25 at 20:42 +0200, Johannes Berg wrote: > Hi, > > > > > + if (live) > > > > + drv_remove_interface(local, sdata); > > > > ret = eth_mac_addr(dev, sa); > > > > + if (live) > > > > + ret = drv_add_interface(local, sdata); > > > > > > > > if (ret == 0) > > > > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > > > > > > > > > > I still don't like the (lack of) error checking here. As far as I > > > know, > > > eth_mac_addr() can very happily fail if the passed address is > > > invalid, > > > so we really shouldn't overwrite the ret value by > > > drv_add_interface(). > > > > Ah yes, that was an oversight. I assume we do want to add_interface > > even if eth_mac_addr fails though. > > Right. > > > So my only question is about the > > return from drv_add_interface(). Is this unlikely to fail? If so > > would > > just a WARN_ON be sufficient and return the value from > > eth_mac_addr()? > > Hm, yeah, I guess it really ought to not fail here. > > > So something like: > > > > if (live) > > drv_remove_interface(local, sdata); > > ret = eth_mac_addr(dev, sa); > > if (ret == 0) > > memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); > > > > if (live) > > WARN_ON(drv_add_interface(local, sdata)); > > > > return ret; > > Seems reasonable. We could do something like > > err = drv_add_interface(...); > if (err) { > dev_close(...); > ret = ret ?: err; > } > > or something, but not sure that's worth it, it really shouldn't fail > at > this point. > > But I guess we could leave setting > NL80211_EXT_FEATURE_AUTH_TX_RANDOM_TA > to the driver if we think it'd be less risky that way? (I assume you meant this new POWERED_ADDR_CHANGE feature) Anyways, is this something all mac80211 drivers _should_ be able to do (I thought I remember you indicating this was the case)? I'd hate to introduce (expose) a bug for some driver I couldn't test... but also the WARN_ON would be very easy to pinpoint what happened. IMO if mac80211-based drivers should be able to handle this, I think we should keep it in mac80211. Any issues that arise would be a driver bug that was exposed, and needs to be fixed anyways. Ultimately its up to you. I would like to avoid doing it per-driver but at the same time address randomization is completely broken on 6GHz so if we can only do it for iwlwifi so be it. Thanks, James > > johannes
On Thu, 2022-08-25 at 15:26 -0700, James Prestwood wrote: > > > > But I guess we could leave setting > > NL80211_EXT_FEATURE_AUTH_TX_RANDOM_TA > > to the driver if we think it'd be less risky that way? > > (I assume you meant this new POWERED_ADDR_CHANGE feature) Err, yes, copy/pasted that wrong. > Anyways, is this something all mac80211 drivers _should_ be able to do > (I thought I remember you indicating this was the case)? I'd hate to > introduce (expose) a bug for some driver I couldn't test... but also > the WARN_ON would be very easy to pinpoint what happened. Yeah, true. > IMO if mac80211-based drivers should be able to handle this, I think we > should keep it in mac80211. Any issues that arise would be a driver bug > that was exposed, and needs to be fixed anyways. > Agree. And you're setting it early in alloc_hw() so drivers that (think they) have problems with it can unset it. johannes
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 1a9ada411879..7c8772d8427a 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -199,15 +199,73 @@ static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr, return ret; } +static int ieee80211_can_powered_addr_change(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_roc_work *roc; + struct ieee80211_local *local = sdata->local; + struct ieee80211_sub_if_data *scan_sdata; + int ret = 0; + + /* To be the most flexible here we want to only limit changing the + * address if the specific interface is doing offchannel work or + * scanning. + */ + if (netif_carrier_ok(sdata->dev)) + return -EBUSY; + + mutex_lock(&local->mtx); + + /* First check no ROC work is happening on this iface */ + list_for_each_entry(roc, &local->roc_list, list) { + if (roc->sdata != sdata) + continue; + + if (roc->started) { + ret = -EBUSY; + goto unlock; + } + } + + /* And if this iface is scanning */ + if (local->scanning) { + scan_sdata = rcu_dereference_protected(local->scan_sdata, + lockdep_is_held(&local->mtx)); + if (sdata == scan_sdata) + ret = -EBUSY; + } + + switch (sdata->vif.type) { + case NL80211_IFTYPE_STATION: + case NL80211_IFTYPE_P2P_CLIENT: + /* More interface types could be added here but changing the + * address while powered makes the most sense in client modes. + */ + break; + default: + return -EOPNOTSUPP; + } + +unlock: + mutex_unlock(&local->mtx); + return ret; +} + 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_powered_addr_change(sdata); + if (ret) + return ret; + + live = true; + } if (sdata->vif.type == NL80211_IFTYPE_MONITOR && !(sdata->u.mntr.flags & MONITOR_FLAG_ACTIVE)) @@ -217,7 +275,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) + ret = drv_add_interface(local, sdata); if (ret == 0) memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN); @@ -2128,6 +2190,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; ndev->hw_features |= ndev->features & MAC80211_SUPPORTED_FEATURES_TX; diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 5a385d4146b9..3aeb5e598263 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -616,6 +616,8 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len, NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_TX_STATUS); wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_SCAN_FREQ_KHZ); + wiphy_ext_feature_set(wiphy, + NL80211_EXT_FEATURE_POWERED_ADDR_CHANGE); if (!ops->hw_scan) { wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |
Adds support in mac80211 for NL80211_EXT_FEATURE_POWERED_ADDR_CHANGE. The motivation behind this functionality is to fix limitations of address randomization on frequencies which are disallowed in world roaming. The way things work now, if a client wants to randomize their address per-connection it must power down the device, change the MAC, and power back up. Here lies a problem since powering down the device may result in frequencies being disabled (until the regdom is set). If the desired BSS is on one such frequency the client is unable to connect once the phy is powered again. For mac80211 based devices changing the MAC while powered is possible but currently disallowed (-EBUSY). This patch adds some logic to allow a MAC change while powered by removing the interface, changing the MAC, and adding it again. mac80211 will advertise support for this feature so userspace can determine the best course of action e.g. disallow address randomization on certain frequencies if not supported. There are certain limitations put on this which simplify the logic: - No active connection - No offchannel work, including scanning. Signed-off-by: James Prestwood <prestwoj@gmail.com> --- net/mac80211/iface.c | 67 ++++++++++++++++++++++++++++++++++++++++++-- net/mac80211/main.c | 2 ++ 2 files changed, 67 insertions(+), 2 deletions(-)