From de9c1173bfeaddf7453b5a592a644cb1cbf052c7 Mon Sep 17 00:00:00 2001 From: stsp Date: Fri, 3 Dec 2021 12:41:36 +0000 Subject: [PATCH] Introduce an optional driver-specific bgscan_done() handler which allows the driver to take control of the roaming teardown sequence. This handler allows drivers to ensure that race conditions between firmware state and net80211 state are avoided, and will be used by the iwm(4) and iwx(4) drivers soon. Split the existing roaming teardown sequence into two steps, one step for tearing down Tx block ack sessions which sends a DELBA frame, and a second step for flushing Tx rings followed by sending a DEAUTH frame. We used to queue both frames, expecting to switch APs once both were sent. Now we effectively expect everything to be sent before we queue a final DEAUTH frame, and wait for just this frame to be sent before switching. This already made issues on iwm/iwx less frequent but by itself this was not enough to close all races for those drivers. It should however help when adding background scan support to a non-firmware device driver. Tested, with driver patches: iwm 8265: Aaron Poffenberger, stsp iwm 9260: florian iwm 9560: sthen iwx ax200: jmc, stsp --- sys/net80211/ieee80211_node.c | 156 ++++++++++++++++++++++++--------- sys/net80211/ieee80211_node.h | 4 +- sys/net80211/ieee80211_proto.c | 9 +- sys/net80211/ieee80211_var.h | 7 +- 4 files changed, 127 insertions(+), 49 deletions(-) diff --git a/sys/net80211/ieee80211_node.c b/sys/net80211/ieee80211_node.c index 116d71f94d6..752d139b056 100644 --- a/sys/net80211/ieee80211_node.c +++ b/sys/net80211/ieee80211_node.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ieee80211_node.c,v 1.188 2021/11/03 11:52:59 krw Exp $ */ +/* $OpenBSD: ieee80211_node.c,v 1.189 2021/12/03 12:41:36 stsp Exp $ */ /* $NetBSD: ieee80211_node.c,v 1.14 2004/05/09 09:18:47 dyoung Exp $ */ /*- @@ -73,6 +73,8 @@ void ieee80211_node_set_timeouts(struct ieee80211_node *); void ieee80211_setup_node(struct ieee80211com *, struct ieee80211_node *, const u_int8_t *); struct ieee80211_node *ieee80211_alloc_node_helper(struct ieee80211com *); +void ieee80211_node_free_unref_cb(struct ieee80211_node *); +void ieee80211_node_tx_flushed(struct ieee80211com *, struct ieee80211_node *); void ieee80211_node_switch_bss(struct ieee80211com *, struct ieee80211_node *); void ieee80211_node_addba_request(struct ieee80211_node *, int); void ieee80211_node_addba_request_ac_be_to(void *); @@ -1165,6 +1167,75 @@ struct ieee80211_node_switch_bss_arg { u_int8_t sel_macaddr[IEEE80211_ADDR_LEN]; }; +void +ieee80211_node_free_unref_cb(struct ieee80211_node *ni) +{ + free(ni->ni_unref_arg, M_DEVBUF, ni->ni_unref_arg_size); + + /* Guard against accidental reuse. */ + ni->ni_unref_cb = NULL; + ni->ni_unref_arg = NULL; + ni->ni_unref_arg_size = 0; +} + +/* Implements ni->ni_unref_cb(). */ +void +ieee80211_node_tx_stopped(struct ieee80211com *ic, + struct ieee80211_node *ni) +{ + splassert(IPL_NET); + + if ((ic->ic_flags & IEEE80211_F_BGSCAN) == 0) + return; + + /* + * Install a callback which will switch us to the new AP once + * the de-auth frame has been processed by hardware. + * Pass on the existing ni->ni_unref_arg argument. + */ + ic->ic_bss->ni_unref_cb = ieee80211_node_switch_bss; + + /* + * All data frames queued to hardware have been flushed and + * A-MPDU Tx has been stopped. We are now going to switch APs. + * Queue a de-auth frame addressed at our current AP. + */ + if (IEEE80211_SEND_MGMT(ic, ic->ic_bss, + IEEE80211_FC0_SUBTYPE_DEAUTH, + IEEE80211_REASON_AUTH_LEAVE) != 0) { + ic->ic_flags &= ~IEEE80211_F_BGSCAN; + ieee80211_node_free_unref_cb(ni); + ieee80211_new_state(ic, IEEE80211_S_SCAN, -1); + return; + } + + /* F_BGSCAN flag gets cleared in ieee80211_node_join_bss(). */ +} + +/* Implements ni->ni_unref_cb(). */ +void +ieee80211_node_tx_flushed(struct ieee80211com *ic, struct ieee80211_node *ni) +{ + splassert(IPL_NET); + + if ((ic->ic_flags & IEEE80211_F_BGSCAN) == 0) + return; + + /* All data frames queued to hardware have been flushed. */ + if (ic->ic_caps & IEEE80211_C_TX_AMPDU) { + /* + * Install a callback which will switch us to the + * new AP once Tx agg sessions have been stopped, + * which involves sending a DELBA frame. + * Pass on the existing ni->ni_unref_arg argument. + */ + ic->ic_bss->ni_unref_cb = ieee80211_node_tx_stopped; + ieee80211_stop_ampdu_tx(ic, ic->ic_bss, + IEEE80211_FC0_SUBTYPE_DEAUTH); + } else + ieee80211_node_tx_stopped(ic, ni); +} + /* Implements ni->ni_unref_cb(). */ void ieee80211_node_switch_bss(struct ieee80211com *ic, struct ieee80211_node *ni) @@ -1175,16 +1246,14 @@ ieee80211_node_switch_bss(struct ieee80211com *ic, struct ieee80211_node *ni) splassert(IPL_NET); - if ((ic->ic_flags & IEEE80211_F_BGSCAN) == 0) { - free(sba, M_DEVBUF, sizeof(*sba)); + if ((ic->ic_flags & IEEE80211_F_BGSCAN) == 0) return; - } ic->ic_xflags &= ~IEEE80211_F_TX_MGMT_ONLY; selbs = ieee80211_find_node(ic, sba->sel_macaddr); if (selbs == NULL) { - free(sba, M_DEVBUF, sizeof(*sba)); + ieee80211_node_free_unref_cb(ni); ic->ic_flags &= ~IEEE80211_F_BGSCAN; ieee80211_new_state(ic, IEEE80211_S_SCAN, -1); return; @@ -1192,7 +1261,7 @@ ieee80211_node_switch_bss(struct ieee80211com *ic, struct ieee80211_node *ni) curbs = ieee80211_find_node(ic, sba->cur_macaddr); if (curbs == NULL) { - free(sba, M_DEVBUF, sizeof(*sba)); + ieee80211_node_free_unref_cb(ni); ic->ic_flags &= ~IEEE80211_F_BGSCAN; ieee80211_new_state(ic, IEEE80211_S_SCAN, -1); return; @@ -1206,7 +1275,11 @@ ieee80211_node_switch_bss(struct ieee80211com *ic, struct ieee80211_node *ni) ieee80211_chan2ieee(ic, selbs->ni_chan)); } ieee80211_node_newstate(curbs, IEEE80211_STA_CACHE); - ieee80211_node_join_bss(ic, selbs); /* frees arg and ic->ic_bss */ + /* + * ieee80211_node_join_bss() frees arg and ic->ic_bss via + * ic->ic_node_copy() in ieee80211_node_cleanup(). + */ + ieee80211_node_join_bss(ic, selbs); } void @@ -1487,32 +1560,32 @@ ieee80211_end_scan(struct ifnet *ifp) ic->ic_bgscan_fail = 0; - /* - * We are going to switch APs. Stop A-MPDU Tx and - * queue a de-auth frame addressed to our current AP. - */ - ieee80211_stop_ampdu_tx(ic, ic->ic_bss, - IEEE80211_FC0_SUBTYPE_DEAUTH); - if (IEEE80211_SEND_MGMT(ic, ic->ic_bss, - IEEE80211_FC0_SUBTYPE_DEAUTH, - IEEE80211_REASON_AUTH_LEAVE) != 0) { - ic->ic_flags &= ~IEEE80211_F_BGSCAN; - free(arg, M_DEVBUF, sizeof(*arg)); - return; - } - /* Prevent dispatch of additional data frames to hardware. */ ic->ic_xflags |= IEEE80211_F_TX_MGMT_ONLY; + IEEE80211_ADDR_COPY(arg->cur_macaddr, curbs->ni_macaddr); + IEEE80211_ADDR_COPY(arg->sel_macaddr, selbs->ni_macaddr); + + if (ic->ic_bgscan_done) { + /* + * The driver will flush its queues and allow roaming + * to proceed once queues have been flushed. + * On failure the driver will move back to SCAN state. + */ + ic->ic_bgscan_done(ic, arg, sizeof(*arg)); + return; + } + /* * Install a callback which will switch us to the new AP once * all dispatched frames have been processed by hardware. */ - IEEE80211_ADDR_COPY(arg->cur_macaddr, curbs->ni_macaddr); - IEEE80211_ADDR_COPY(arg->sel_macaddr, selbs->ni_macaddr); ic->ic_bss->ni_unref_arg = arg; ic->ic_bss->ni_unref_arg_size = sizeof(*arg); - ic->ic_bss->ni_unref_cb = ieee80211_node_switch_bss; + if (ic->ic_bss->ni_refcnt > 0) + ic->ic_bss->ni_unref_cb = ieee80211_node_tx_flushed; + else + ieee80211_node_tx_flushed(ic, ni); /* F_BGSCAN flag gets cleared in ieee80211_node_join_bss(). */ return; } else if (selbs == NULL) @@ -1611,14 +1684,10 @@ ieee80211_node_cleanup(struct ieee80211com *ic, struct ieee80211_node *ni) ni->ni_rsnie = NULL; } ieee80211_ba_del(ni); - ni->ni_unref_cb = NULL; - free(ni->ni_unref_arg, M_DEVBUF, ni->ni_unref_arg_size); - ni->ni_unref_arg = NULL; - ni->ni_unref_arg_size = 0; - #ifndef IEEE80211_STA_ONLY mq_purge(&ni->ni_savedq); #endif + ieee80211_node_free_unref_cb(ni); } void @@ -1978,6 +2047,18 @@ ieee80211_find_node_for_beacon(struct ieee80211com *ic, return (keep); } +void +ieee80211_node_tx_ba_clear(struct ieee80211_node *ni, int tid) +{ + struct ieee80211_tx_ba *ba = &ni->ni_tx_ba[tid]; + + if (ba->ba_state != IEEE80211_BA_INIT) { + if (timeout_pending(&ba->ba_to)) + timeout_del(&ba->ba_to); + ba->ba_state = IEEE80211_BA_INIT; + } +} + void ieee80211_ba_del(struct ieee80211_node *ni) { @@ -1994,14 +2075,8 @@ ieee80211_ba_del(struct ieee80211_node *ni) } } - for (tid = 0; tid < nitems(ni->ni_tx_ba); tid++) { - struct ieee80211_tx_ba *ba = &ni->ni_tx_ba[tid]; - if (ba->ba_state != IEEE80211_BA_INIT) { - if (timeout_pending(&ba->ba_to)) - timeout_del(&ba->ba_to); - ba->ba_state = IEEE80211_BA_INIT; - } - } + for (tid = 0; tid < nitems(ni->ni_tx_ba); tid++) + ieee80211_node_tx_ba_clear(ni, tid); timeout_del(&ni->ni_addba_req_to[EDCA_AC_BE]); timeout_del(&ni->ni_addba_req_to[EDCA_AC_BK]); @@ -2040,17 +2115,18 @@ void ieee80211_release_node(struct ieee80211com *ic, struct ieee80211_node *ni) { int s; + void (*ni_unref_cb)(struct ieee80211com *, struct ieee80211_node *); DPRINTF(("%s refcnt %u\n", ether_sprintf(ni->ni_macaddr), ni->ni_refcnt)); s = splnet(); if (ieee80211_node_decref(ni) == 0) { if (ni->ni_unref_cb) { - (*ni->ni_unref_cb)(ic, ni); + /* The callback may set ni->ni_unref_cb again. */ + ni_unref_cb = ni->ni_unref_cb; ni->ni_unref_cb = NULL; /* Freed by callback if necessary: */ - ni->ni_unref_arg = NULL; - ni->ni_unref_arg_size = 0; + (*ni_unref_cb)(ic, ni); } if (ni->ni_state == IEEE80211_STA_COLLECT) ieee80211_free_node(ic, ni); diff --git a/sys/net80211/ieee80211_node.h b/sys/net80211/ieee80211_node.h index d8fce4ca1f5..ca7f9cd59bb 100644 --- a/sys/net80211/ieee80211_node.h +++ b/sys/net80211/ieee80211_node.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ieee80211_node.h,v 1.89 2021/10/11 09:01:06 stsp Exp $ */ +/* $OpenBSD: ieee80211_node.h,v 1.90 2021/12/03 12:41:36 stsp Exp $ */ /* $NetBSD: ieee80211_node.h,v 1.9 2004/04/30 22:57:32 dyoung Exp $ */ /*- @@ -518,6 +518,7 @@ struct ieee80211_node *ieee80211_dup_bss(struct ieee80211com *, const u_int8_t *); struct ieee80211_node *ieee80211_find_node(struct ieee80211com *, const u_int8_t *); +void ieee80211_node_tx_ba_clear(struct ieee80211_node *, int); void ieee80211_ba_del(struct ieee80211_node *); struct ieee80211_node *ieee80211_find_rxnode(struct ieee80211com *, const struct ieee80211_frame *); @@ -553,6 +554,7 @@ void ieee80211_node_join(struct ieee80211com *, void ieee80211_node_leave(struct ieee80211com *, struct ieee80211_node *); int ieee80211_match_bss(struct ieee80211com *, struct ieee80211_node *, int); +void ieee80211_node_tx_stopped(struct ieee80211com *, struct ieee80211_node *); struct ieee80211_node *ieee80211_node_choose_bss(struct ieee80211com *, int, struct ieee80211_node **); void ieee80211_node_join_bss(struct ieee80211com *, struct ieee80211_node *); diff --git a/sys/net80211/ieee80211_proto.c b/sys/net80211/ieee80211_proto.c index 41d846d591d..447a2676bfb 100644 --- a/sys/net80211/ieee80211_proto.c +++ b/sys/net80211/ieee80211_proto.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ieee80211_proto.c,v 1.105 2021/10/11 09:01:06 stsp Exp $ */ +/* $OpenBSD: ieee80211_proto.c,v 1.106 2021/12/03 12:41:36 stsp Exp $ */ /* $NetBSD: ieee80211_proto.c,v 1.8 2004/04/30 23:58:20 dyoung Exp $ */ /*- @@ -733,14 +733,9 @@ ieee80211_delba_request(struct ieee80211com *ic, struct ieee80211_node *ni, } if (dir) { /* MLME-DELBA.confirm(Originator) */ - struct ieee80211_tx_ba *ba = &ni->ni_tx_ba[tid]; - if (ic->ic_ampdu_tx_stop != NULL) ic->ic_ampdu_tx_stop(ic, ni, tid); - - ba->ba_state = IEEE80211_BA_INIT; - /* stop Block Ack inactivity timer */ - timeout_del(&ba->ba_to); + ieee80211_node_tx_ba_clear(ni, tid); } else { /* MLME-DELBA.confirm(Recipient) */ struct ieee80211_rx_ba *ba = &ni->ni_rx_ba[tid]; diff --git a/sys/net80211/ieee80211_var.h b/sys/net80211/ieee80211_var.h index 174da137a22..dd17ed76031 100644 --- a/sys/net80211/ieee80211_var.h +++ b/sys/net80211/ieee80211_var.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ieee80211_var.h,v 1.107 2021/10/11 09:02:01 stsp Exp $ */ +/* $OpenBSD: ieee80211_var.h,v 1.108 2021/12/03 12:41:36 stsp Exp $ */ /* $NetBSD: ieee80211_var.h,v 1.7 2004/05/06 03:07:10 dyoung Exp $ */ /*- @@ -212,6 +212,8 @@ struct ieee80211_defrag { #define IEEE80211_GROUP_NKID 6 +struct ieee80211_node_switch_bss_arg; + struct ieee80211com { struct arpcom ic_ac; LIST_ENTRY(ieee80211com) ic_list; /* chain of all ieee80211com */ @@ -248,6 +250,9 @@ struct ieee80211com { void (*ic_updateprot)(struct ieee80211com *); void (*ic_updatechan)(struct ieee80211com *); int (*ic_bgscan_start)(struct ieee80211com *); + void (*ic_bgscan_done)(struct ieee80211com *, + struct ieee80211_node_switch_bss_arg *, + size_t); struct timeout ic_bgscan_timeout; uint32_t ic_bgscan_fail; u_int8_t ic_myaddr[IEEE80211_ADDR_LEN]; -- 2.20.1