diff mbox series

mac80211: Send deauth to STA's upon AP stop

Message ID 20200618093609.16514-1-shay.bar@celeno.com (mailing list archive)
State Superseded
Delegated to: Johannes Berg
Headers show
Series mac80211: Send deauth to STA's upon AP stop | expand

Commit Message

Shay Bar June 18, 2020, 9:36 a.m. UTC
In current code, AP is not informing STA when its going down.
So STA keep looking the AP (Null function etc.) and can't find it (it is down).

Fix is to send deauth to all associated STA's upon AP stop.
__sta_info_flush() with a true bool is only called from ieee80211_stop_ap().
Rename "vlans" -> "ap_stop".

Signed-off-by: Shay Bar <shay.bar@celeno.com>
---
 net/mac80211/sta_info.c | 20 ++++++++++++++++----
 net/mac80211/sta_info.h |  5 +++--
 2 files changed, 19 insertions(+), 6 deletions(-)

Comments

Ben Greear June 18, 2020, 1:47 p.m. UTC | #1
On 06/18/2020 02:36 AM, Shay Bar wrote:
> In current code, AP is not informing STA when its going down.
> So STA keep looking the AP (Null function etc.) and can't find it (it is down).

Hello,

It will take a noticeable amount of time to send deauth to hundreds of stations
in cases where a busy AP goes down.  Is there any clever way to use something like
11k/v to send a single frame to let all stations know AP is going down?

Thanks,
Ben
Johannes Berg June 18, 2020, 1:48 p.m. UTC | #2
On Thu, 2020-06-18 at 12:36 +0300, Shay Bar wrote:
> In current code, AP is not informing STA when its going down.
> 
Hostapd normally handles that. Are we really so worried about it
crashing?

johannes
Shay Bar June 18, 2020, 2:14 p.m. UTC | #3
> Hostapd normally handles that. Are we really so worried about it
> crashing?
> johannes

Actually, it's not crashing but WIFI STA keeps sending unicast frames
to AP such as RTS, BAR, NULL function (no data).
This is what I want to avoid.
Its just seems much more clever to let STA know we are down.
don’t you agree?
Once it receive this deauth, STA stop sending any frames to AP.
Shay.
Johannes Berg June 18, 2020, 2:18 p.m. UTC | #4
On Thu, 2020-06-18 at 14:14 +0000, Shay Bar wrote:
> > Hostapd normally handles that. Are we really so worried about it
> > crashing?
> > johannes
> 
> Actually, it's not crashing but WIFI STA keeps sending unicast frames
> to AP such as RTS, BAR, NULL function (no data).
> This is what I want to avoid.
> Its just seems much more clever to let STA know we are down.
> don’t you agree?

We're doing that. More accurately, hostapd is doing that. So what's not
working for you?

johannes
Shay Bar June 18, 2020, 2:36 p.m. UTC | #5
> We're doing that. More accurately, hostapd is doing that. So what's not
> working for you?
> johannes

Air capture shows, hostapd doesn’t send deauth in the below 2 cases and I
actually don't see such hostapd code that handles that:
"ifconfig <if> down"
"killall hostapd"

Am I missing something?
Johannes Berg June 18, 2020, 2:38 p.m. UTC | #6
On Thu, 2020-06-18 at 14:36 +0000, Shay Bar wrote:
> > We're doing that. More accurately, hostapd is doing that. So what's not
> > working for you?
> > johannes
> 
> Air capture shows, hostapd doesn’t send deauth in the below 2 cases and I
> actually don't see such hostapd code that handles that:
> "ifconfig <if> down"
> "killall hostapd"

So... why would you ever do that? :)

johannes
Shay Bar June 18, 2020, 2:45 p.m. UTC | #7
> So... why would you ever do that? :)
> johannes

:)
Is it illegal to do "ifconfig <if> down" or kill hostapd
while STA's are still associated?
There are some vendors/users that are doing that.

Regarding Ben's proposal of using 11k/v, I couldn’t find such
"going down" single frame in the standard (although sounds trivial)

Shay.
Johannes Berg June 18, 2020, 2:48 p.m. UTC | #8
On Thu, 2020-06-18 at 14:45 +0000, Shay Bar wrote:
> > So... why would you ever do that? :)
> > johannes
> 
> :)
> Is it illegal to do "ifconfig <if> down" or kill hostapd
> while STA's are still associated?
> There are some vendors/users that are doing that.

It's not really *illegal* per se, but it would be weird if both did it
... But I do tend to think that if you're using hostapd or such to
control it, you shouldn't do another out-of-band control.

> Regarding Ben's proposal of using 11k/v, I couldn’t find such
> "going down" single frame in the standard (although sounds trivial)

Broadcast deauth :)

johannes
Shay Bar June 18, 2020, 3:11 p.m. UTC | #9
>> Is it illegal to do "ifconfig <if> down" or kill hostapd
>> while STA's are still associated?
>> There are some vendors/users that are doing that.

> It's not really *illegal* per se, but it would be weird if both did it
> ... But I do tend to think that if you're using hostapd or such to
> control it, you shouldn't do another out-of-band control.

Maybe I was misunderstood, using _any_ one of the above methods
to down the interface, have the same result -
hostapd doesn’t send any deauth and STA's stay unaware of AP down.

Shay.
Ben Greear June 18, 2020, 3:26 p.m. UTC | #10
On 06/18/2020 07:48 AM, Johannes Berg wrote:
> On Thu, 2020-06-18 at 14:45 +0000, Shay Bar wrote:
>>> So... why would you ever do that? :)
>>> johannes
>>
>> :)
>> Is it illegal to do "ifconfig <if> down" or kill hostapd
>> while STA's are still associated?
>> There are some vendors/users that are doing that.
>
> It's not really *illegal* per se, but it would be weird if both did it
> ... But I do tend to think that if you're using hostapd or such to
> control it, you shouldn't do another out-of-band control.
>
>> Regarding Ben's proposal of using 11k/v, I couldn’t find such
>> "going down" single frame in the standard (although sounds trivial)
>
> Broadcast deauth :)

Or use k/v to tell STA that AP is going down immediately and here is (null, likely)
list of APs to associate with instead?

Another thing I notice:  In order to keep things from blocking for long periods (3+ seconds)
with rtnl held, I ended up doing a force flush in ath10k-ct, so at least when using that
driver, sending frames and then immediately doing flush will likely not put much on air.

I have hoped to one day add tx-completion call-backs handling to mac80211 instead of just calling a flush
for these types of station-down communication where flush is to be called shortly.

Thanks,
Ben
Shay Bar June 21, 2020, 10:12 a.m. UTC | #11
Hi Johannes and Ben,

To conclude this thread, hostapd doesn’t send any deauth/disassoc
upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
AP interface).
This is causing a situation where stations keep sending unicast frames
to a down AP interface as it doesn’t know it's gone down.
I tried your suggestion and sent 1 deauth/disassoc as broadcast
(instead of unicast to each STA), but some stations (e.g. Samsung S8)
Ignore this broadcast (while it will not ignore unicast deauth/disassoc).
Although not indicated in the standard, I think it's better to let STA
Know AP gone down by sending this unicast deauth to each STA
(as this patch does).

Please let me know your decision.
Shay.
Johannes Berg June 25, 2020, 9:51 a.m. UTC | #12
On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
> Hi Johannes and Ben,
> 
> To conclude this thread, hostapd doesn’t send any deauth/disassoc
> upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
> AP interface).

Right. I'm sort of suggesting you just shouldn't be doing this, and it
doesn't seem like most people actually do, otherwise we'd have seen this
issue before?

> This is causing a situation where stations keep sending unicast frames
> to a down AP interface as it doesn’t know it's gone down.
> I tried your suggestion and sent 1 deauth/disassoc as broadcast
> (instead of unicast to each STA), but some stations (e.g. Samsung S8)
> Ignore this broadcast (while it will not ignore unicast deauth/disassoc).
> Although not indicated in the standard, I think it's better to let STA
> Know AP gone down by sending this unicast deauth to each STA
> (as this patch does).

I'm not really sure. That's a _lot_ of frames and potentially quite a
long time. In the patch, as written, I'm not even convinced you can be
sure that they will all make it out to the air?

johannes
Shay Bar June 25, 2020, 10:29 a.m. UTC | #13
On 25/06/2020 12:51, Johannes Berg wrote:
> External Email
>
>
> On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
>> Hi Johannes and Ben,
>>
>> To conclude this thread, hostapd doesn’t send any deauth/disassoc
>> upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
>> AP interface).
>
> Right. I'm sort of suggesting you just shouldn't be doing this, and it
> doesn't seem like most people actually do, otherwise we'd have seen this
> issue before?
>
I shouldn't kill hostapd? Isn't this a very basic action?
What is the alternative for stopping the AP?

>> This is causing a situation where stations keep sending unicast frames
>> to a down AP interface as it doesn’t know it's gone down.
>> I tried your suggestion and sent 1 deauth/disassoc as broadcast
>> (instead of unicast to each STA), but some stations (e.g. Samsung S8)
>> Ignore this broadcast (while it will not ignore unicast deauth/disassoc).
>> Although not indicated in the standard, I think it's better to let STA
>> Know AP gone down by sending this unicast deauth to each STA
>> (as this patch does).
>
> I'm not really sure. That's a _lot_ of frames and potentially quite a
> long time. In the patch, as written, I'm not even convinced you can be
> sure that they will all make it out to the air?
I agree, but what's the alternative?

Thanks,
Shay
Johannes Berg July 30, 2020, 2 p.m. UTC | #14
On Thu, 2020-06-25 at 13:29 +0300, Shay Bar wrote:
> On 25/06/2020 12:51, Johannes Berg wrote:
> > External Email
> > 
> > 
> > On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
> > > Hi Johannes and Ben,
> > > 
> > > To conclude this thread, hostapd doesn’t send any deauth/disassoc
> > > upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
> > > AP interface).
> > 
> > Right. I'm sort of suggesting you just shouldn't be doing this, and it
> > doesn't seem like most people actually do, otherwise we'd have seen this
> > issue before?
> > 
> I shouldn't kill hostapd? Isn't this a very basic action?
> What is the alternative for stopping the AP?

I guess I would say to use hostapd_cli first to "disable" the
interfaces?

johannes
Shay Bar July 30, 2020, 2:23 p.m. UTC | #15
On 30/07/2020 17:00, Johannes Berg wrote:
> External Email
>
>
> On Thu, 2020-06-25 at 13:29 +0300, Shay Bar wrote:
>> On 25/06/2020 12:51, Johannes Berg wrote:
>>> External Email
>>>
>>>
>>> On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
>>>> Hi Johannes and Ben,
>>>>
>>>> To conclude this thread, hostapd doesn’t send any deauth/disassoc
>>>> upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
>>>> AP interface).
>>> Right. I'm sort of suggesting you just shouldn't be doing this, and it
>>> doesn't seem like most people actually do, otherwise we'd have seen this
>>> issue before?
>>>
>> I shouldn't kill hostapd? Isn't this a very basic action?
>> What is the alternative for stopping the AP?
> I guess I would say to use hostapd_cli first to "disable" the
> interfaces?

Indeed, using hostapd_cli "DISABLE" will call hostapd_flush_old_stations() that will send broadcast deauth.
The problem is (mentioned before) that some stations (e.g. Samsung S8) ignore this broadcast (while it will not ignore unicast deauth/disassoc).
Johannes Berg July 30, 2020, 2:28 p.m. UTC | #16
On Thu, 2020-07-30 at 14:23 +0000, Shay Bar wrote:
> On 30/07/2020 17:00, Johannes Berg wrote:
> > External Email
> > 
> > 
> > On Thu, 2020-06-25 at 13:29 +0300, Shay Bar wrote:
> > > On 25/06/2020 12:51, Johannes Berg wrote:
> > > > External Email
> > > > 
> > > > 
> > > > On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
> > > > > Hi Johannes and Ben,
> > > > > 
> > > > > To conclude this thread, hostapd doesn’t send any deauth/disassoc
> > > > > upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
> > > > > AP interface).
> > > > Right. I'm sort of suggesting you just shouldn't be doing this, and it
> > > > doesn't seem like most people actually do, otherwise we'd have seen this
> > > > issue before?
> > > > 
> > > I shouldn't kill hostapd? Isn't this a very basic action?
> > > What is the alternative for stopping the AP?
> > I guess I would say to use hostapd_cli first to "disable" the
> > interfaces?
> 
> Indeed, using hostapd_cli "DISABLE" will call
> hostapd_flush_old_stations() that will send broadcast deauth.

Ah.

> The problem is (mentioned before) that some stations (e.g. Samsung S8)
> ignore this broadcast (while it will not ignore unicast
> deauth/disassoc).

Right, you mentioned that. I guess you could work around by disabling
all stations separately, but ... not really a great solution either.

Perhaps we should fix that at the hostapd level?

Not really sure how much trouble we should go to for bad stations
though. I mean, it's not like they should be ignoring that.

Maybe they just miss it due to super aggressive powersave, and we should
send it a few times?

johannes
Shay Bar July 30, 2020, 2:45 p.m. UTC | #17
On 30/07/2020 17:28, Johannes Berg wrote:
> External Email
>
>
> On Thu, 2020-07-30 at 14:23 +0000, Shay Bar wrote:
>> On 30/07/2020 17:00, Johannes Berg wrote:
>>> External Email
>>>
>>>
>>> On Thu, 2020-06-25 at 13:29 +0300, Shay Bar wrote:
>>>> On 25/06/2020 12:51, Johannes Berg wrote:
>>>>> External Email
>>>>>
>>>>>
>>>>> On Sun, 2020-06-21 at 10:12 +0000, Shay Bar wrote:
>>>>>> Hi Johannes and Ben,
>>>>>>
>>>>>> To conclude this thread, hostapd doesn’t send any deauth/disassoc
>>>>>> upon AP stop (when hostapd is killed _or_ when "ifconfig down" the
>>>>>> AP interface).
>>>>> Right. I'm sort of suggesting you just shouldn't be doing this, and it
>>>>> doesn't seem like most people actually do, otherwise we'd have seen this
>>>>> issue before?
>>>>>
>>>> I shouldn't kill hostapd? Isn't this a very basic action?
>>>> What is the alternative for stopping the AP?
>>> I guess I would say to use hostapd_cli first to "disable" the
>>> interfaces?
>> Indeed, using hostapd_cli "DISABLE" will call
>> hostapd_flush_old_stations() that will send broadcast deauth.
> Ah.
>
>> The problem is (mentioned before) that some stations (e.g. Samsung S8)
>> ignore this broadcast (while it will not ignore unicast
>> deauth/disassoc).
> Right, you mentioned that. I guess you could work around by disabling
> all stations separately, but ... not really a great solution either.
>
> Perhaps we should fix that at the hostapd level?
>
> Not really sure how much trouble we should go to for bad stations
> though. I mean, it's not like they should be ignoring that.
>
> Maybe they just miss it due to super aggressive powersave, and we should
> send it a few times?

Power save is possible.
I will try sending it 3 times (instead of only 1) and see if it helps.
Anyway, I agree we should not put extra effort for STA's that ignore this bcast,
Lets discard this patch.

Thanks.
diff mbox series

Patch

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index cd8487bc6fc2..46b26e66430a 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1179,7 +1179,7 @@  void sta_info_stop(struct ieee80211_local *local)
 }
 
 
-int __sta_info_flush(struct ieee80211_sub_if_data *sdata, bool vlans)
+int __sta_info_flush(struct ieee80211_sub_if_data *sdata, bool ap_stop)
 {
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta, *tmp;
@@ -1188,13 +1188,25 @@  int __sta_info_flush(struct ieee80211_sub_if_data *sdata, bool vlans)
 
 	might_sleep();
 
-	WARN_ON(vlans && sdata->vif.type != NL80211_IFTYPE_AP);
-	WARN_ON(vlans && !sdata->bss);
+	WARN_ON(ap_stop && sdata->vif.type != NL80211_IFTYPE_AP);
+	WARN_ON(ap_stop && !sdata->bss);
 
 	mutex_lock(&local->sta_mtx);
 	list_for_each_entry_safe(sta, tmp, &local->sta_list, list) {
 		if (sdata == sta->sdata ||
-		    (vlans && sdata->bss == sta->sdata->bss)) {
+		    (ap_stop && sdata->bss == sta->sdata->bss)) {
+			if (ap_stop) {
+				u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
+				u16 stype = IEEE80211_STYPE_DEAUTH;
+				u16 reason = WLAN_REASON_DEAUTH_LEAVING;
+				ieee80211_send_deauth_disassoc(sdata,
+							       sta->sta.addr,
+							       sdata->vif.addr,
+							       stype,
+							       reason,
+							       true,
+							       frame_buf);
+			}
 			if (!WARN_ON(__sta_info_destroy_part1(sta)))
 				list_add(&sta->free_list, &free_list);
 			ret++;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 49728047dfad..f0aedfa221c2 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -789,9 +789,10 @@  void sta_info_stop(struct ieee80211_local *local);
  * Returns the number of removed STA entries.
  *
  * @sdata: sdata to remove all stations from
- * @vlans: if the given interface is an AP interface, also flush VLANs
+ * @ap_stop: if the given interface is an AP being stopped, we should send
+ * deauth to STA's and flush VLANs
  */
-int __sta_info_flush(struct ieee80211_sub_if_data *sdata, bool vlans);
+int __sta_info_flush(struct ieee80211_sub_if_data *sdata, bool ap_stop);
 
 static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
 {