| Message ID | 20171017194253.10212-1-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi Johannes, On 10/17/2017 10:42 PM, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > This is a code path that will never really get hit anyway, since > it's nonsense to change the beacon of an existing BSS to suddenly > include or no longer include the RSN IE. Reject this instead of > having the dead code, and get rid of accessing wdev->ssid/_len by > way of that. > This is not dead code, we reach it in several scenarios, mainly WPS tests. hostapd uses change_beacon to change the security of the AP so this needs to be supported. We do need to restart the AP in this case which will disconnect existing clients, but this cannot be helped... As a side note, hostapd can also use change_beacon to change the SSID. It does so by updating the SSID IE in the probe response frame. We have a pending patch that detects this and updates the FW but we also need to update wdev->ssid otherwise the wireless_dev will be out of date (not sure if it will cause any problems...)
Hi, > This is not dead code, we reach it in several scenarios, mainly WPS > tests. Interesting. > hostapd uses change_beacon to change the security of the AP so this > needs to be supported. I didn't think this made sense - Jouni? Does hostapd kick off all stations in this case? > We do need to restart the AP in this case which will > disconnect existing clients, but this cannot be helped... Why not restart the AP entirely then from userspace? Hmm. I wonder what would happen with mac80211 - I guess keys would have to removed etc? Does this just work by accident because mac80211 removes the keys with stations? What about GTK(s) though? > As a side note, hostapd can also use change_beacon to change the > SSID. When does that happen? > It does so by updating the SSID IE in the probe response frame. We > have a pending patch that detects this and updates the FW but we also > need to update wdev->ssid otherwise the wireless_dev will be out of > date (not sure if it will cause any problems...) Logic-wise it won't, but we do expose this to userspace and that'd be confusing, so we have to update it I guess. johannes
On 10/18/2017 1:13 PM, Johannes Berg wrote: .... >> hostapd uses change_beacon to change the security of the AP so this >> needs to be supported. > > I didn't think this made sense - Jouni? Does hostapd kick off all > stations in this case? > >> We do need to restart the AP in this case which will >> disconnect existing clients, but this cannot be helped... > > Why not restart the AP entirely then from userspace? Hmm. I wonder what > would happen with mac80211 - I guess keys would have to removed etc? > Does this just work by accident because mac80211 removes the keys with > stations? What about GTK(s) though? > Not sure what happens when the privacy stays the same (secure) but keys change, maybe Jouni can comment. >> As a side note, hostapd can also use change_beacon to change the >> SSID. > > When does that happen? By chance I worked on a WPS certification test last week which used a shell script to perform various operations. The AP started secure but the script could change its configuration to unsecure. It used the wps_config CLI command to change both the security and SSID and hostapd used change_beacon to perform this operation. We got this script from WIFI team so there is good chance it is in use by existing certification test beds.
Johannes Berg <johannes@sipsolutions.net> wrote: > This is a code path that will never really get hit anyway, since > it's nonsense to change the beacon of an existing BSS to suddenly > include or no longer include the RSN IE. Reject this instead of > having the dead code, and get rid of accessing wdev->ssid/_len by > way of that. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> What's the conclusion, should I take this patch or drop it?
On Fri, 2017-10-27 at 15:42 +0200, Kalle Valo wrote: > Johannes Berg <johannes@sipsolutions.net> wrote: > > > This is a code path that will never really get hit anyway, since > > it's nonsense to change the beacon of an existing BSS to suddenly > > include or no longer include the RSN IE. Reject this instead of > > having the dead code, and get rid of accessing wdev->ssid/_len by > > way of that. > > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> > > What's the conclusion, should I take this patch or drop it? Drop, at least for now. I need to look into this and probably do things in cfg80211. johannes
diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c index 85d5c04618eb..a81711451691 100644 --- a/drivers/net/wireless/ath/wil6210/cfg80211.c +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c @@ -1389,7 +1389,6 @@ static int wil_cfg80211_change_beacon(struct wiphy *wiphy, struct cfg80211_beacon_data *bcon) { struct wil6210_priv *wil = wiphy_to_wil(wiphy); - int rc; u32 privacy = 0; wil_dbg_misc(wil, "change_beacon\n"); @@ -1400,24 +1399,11 @@ static int wil_cfg80211_change_beacon(struct wiphy *wiphy, bcon->tail_len)) privacy = 1; - /* in case privacy has changed, need to restart the AP */ - if (wil->privacy != privacy) { - struct wireless_dev *wdev = ndev->ieee80211_ptr; - - wil_dbg_misc(wil, "privacy changed %d=>%d. Restarting AP\n", - wil->privacy, privacy); - - rc = _wil_cfg80211_start_ap(wiphy, ndev, wdev->ssid, - wdev->ssid_len, privacy, - wdev->beacon_interval, - wil->channel, bcon, - wil->hidden_ssid, - wil->pbss); - } else { - rc = _wil_cfg80211_set_ies(wiphy, bcon); - } + /* privacy (really RSN) shouldn't be changing */ + if (wil->privacy != privacy) + return -EINVAL; - return rc; + return _wil_cfg80211_set_ies(wiphy, bcon); } static int wil_cfg80211_start_ap(struct wiphy *wiphy,