From: Johannes Berg <[email protected]>
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 <[email protected]>
---
drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
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,
--
2.14.2
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
Hi Johannes,
On 10/17/2017 10:42 PM, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> 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...)
Johannes Berg <[email protected]> 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 <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
What's the conclusion, should I take this patch or drop it?
--
https://patchwork.kernel.org/patch/10012789/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Fri, 2017-10-27 at 15:42 +0200, Kalle Valo wrote:
> Johannes Berg <[email protected]> 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 <[email protected]>
> > Signed-off-by: Kalle Valo <[email protected]>
>
> 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
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.