net80211: deal with lost state transitions

Since 5efea30f03 we can possibly lose a state transition which can
cause trouble further down the road.
The reproducer from 643d6dce6c can trigger these for example.
Drivers for firmware based wireless cards have worked around some of
this (and other) problems in the past.

Add an array of tasks rather than a single one as we would simply
get npending > 1 and lose order with other tasks.  Try to keep state
changes updated as queued in case we end up with more than one at a
time.  While this is not ideal either (call it a hack) it will sort
the problem for now.
We will queue in ieee80211_new_state_locked() and do checks there
and dequeue in ieee80211_newstate_cb().
If we still overrun the (currently) 8 slots we will drop the state
change rather than overwrite the last one.
When dequeing we will update iv_nstate and keep it around for historic
reasons for the moment.

The longer term we should make the callers of
ieee80211_new_state[_locked]() actually use the returned errors
and act appropriately but that will touch a lot more places and
drivers (possibly incl. changed behaviour for ioctls).

rtwn(4) and rum(4) should probably be revisted and net80211 internals
removed (for rum(4) at least the current logic still seems prone to
races).

PR:		271979, 271988, 275255, 263613, 274003
Sponsored by:	The FreeBSD Foundation (in 2023)
MFC after:	3 days
Reviewed by:	cc
Differential Revision: https://reviews.freebsd.org/D43389

sys/dev/rtwn/if_rtwn.c

@@ -614,10 +614,12 @@ rtwn_vap_delete(struct ieee80211vap *vap)
 	struct ieee80211com *ic = vap->iv_ic;
 	struct rtwn_softc *sc = ic->ic_softc;
 	struct rtwn_vap *uvp = RTWN_VAP(vap);
+	int i;
 
 	/* Put vap into INIT state + stop device if needed. */
 	ieee80211_stop(vap);
-	ieee80211_draintask(ic, &vap->iv_nstate_task);
+	for (i = 0; i < NET80211_IV_NSTATE_NUM; i++)
+		ieee80211_draintask(ic, &vap->iv_nstate_task[i]);
 	ieee80211_draintask(ic, &ic->ic_parent_task);
 
 	RTWN_LOCK(sc);

sys/dev/usb/wlan/if_rum.c

@@ -719,10 +719,12 @@ rum_vap_delete(struct ieee80211vap *vap)
 	struct rum_vap *rvp = RUM_VAP(vap);
 	struct ieee80211com *ic = vap->iv_ic;
 	struct rum_softc *sc = ic->ic_softc;
+	int i;
 
 	/* Put vap into INIT state. */
 	ieee80211_new_state(vap, IEEE80211_S_INIT, -1);
-	ieee80211_draintask(ic, &vap->iv_nstate_task);
+	for (i = 0; i < NET80211_IV_NSTATE_NUM; i++)
+		ieee80211_draintask(ic, &vap->iv_nstate_task[i]);
 
 	RUM_LOCK(sc);
 	/* Cancel any unfinished Tx. */

sys/net80211/ieee80211.c

@@ -730,6 +730,7 @@ ieee80211_vap_detach(struct ieee80211vap *vap)
 {
 	struct ieee80211com *ic = vap->iv_ic;
 	struct ifnet *ifp = vap->iv_ifp;
+	int i;
 
 	CURVNET_SET(ifp->if_vnet);
 
@@ -744,7 +745,8 @@ ieee80211_vap_detach(struct ieee80211vap *vap)
 	/*
 	 * Flush any deferred vap tasks.
 	 */
-	ieee80211_draintask(ic, &vap->iv_nstate_task);
+	for (i = 0; i < NET80211_IV_NSTATE_NUM; i++)
+		ieee80211_draintask(ic, &vap->iv_nstate_task[i]);
 	ieee80211_draintask(ic, &vap->iv_swbmiss_task);
 	ieee80211_draintask(ic, &vap->iv_wme_task);
 	ieee80211_draintask(ic, &ic->ic_parent_task);

sys/net80211/ieee80211_ddb.c

@@ -470,7 +470,8 @@ _db_show_vap(const struct ieee80211vap *vap, int showmesh, int showprocs)
 	if (vap->iv_opmode == IEEE80211_M_MBSS)
 		db_printf("(%p)", vap->iv_mesh);
 #endif
-	db_printf(" state %s", ieee80211_state_name[vap->iv_state]);
+	db_printf(" state %#x %s", vap->iv_state,
+	    ieee80211_state_name[vap->iv_state]);
 	db_printf(" ifp %p(%s)", vap->iv_ifp, if_name(vap->iv_ifp));
 	db_printf("\n");
 
@@ -482,6 +483,16 @@ _db_show_vap(const struct ieee80211vap *vap, int showmesh, int showprocs)
 	struct sysctllog	*iv_sysctl;	/* dynamic sysctl context */
 #endif
 	db_printf("\n");
+
+	db_printf("\tiv_nstate %#x %s iv_nstate_b %d iv_nstate_n %d\n",
+	    vap->iv_nstate, ieee80211_state_name[vap->iv_nstate], /* historic */
+	    vap->iv_nstate_b, vap->iv_nstate_n);
+	for (i = 0; i < NET80211_IV_NSTATE_NUM; i++) {
+		db_printf("\t [%d] iv_nstates %#x %s _task %p _args %d\n", i,
+		    vap->iv_nstates[i], ieee80211_state_name[vap->iv_nstates[i]],
+		    &vap->iv_nstate_task[i], vap->iv_nstate_args[i]);
+	}
+
 	db_printf("\tdebug=%b\n", vap->iv_debug, IEEE80211_MSG_BITS);
 
 	db_printf("\tflags=%b\n", vap->iv_flags, IEEE80211_F_BITS);

sys/net80211/ieee80211_proto.c

@@ -336,7 +336,8 @@ ieee80211_proto_vattach(struct ieee80211vap *vap)
 	vap->iv_bmiss_max = IEEE80211_BMISS_MAX;
 	callout_init_mtx(&vap->iv_swbmiss, IEEE80211_LOCK_OBJ(ic), 0);
 	callout_init(&vap->iv_mgtsend, 1);
-	TASK_INIT(&vap->iv_nstate_task, 0, ieee80211_newstate_cb, vap);
+	for (i = 0; i < NET80211_IV_NSTATE_NUM; i++)
+		TASK_INIT(&vap->iv_nstate_task[i], 0, ieee80211_newstate_cb, vap);
 	TASK_INIT(&vap->iv_swbmiss_task, 0, beacon_swmiss, vap);
 	TASK_INIT(&vap->iv_wme_task, 0, vap_update_wme, vap);
 	TASK_INIT(&vap->iv_slot_task, 0, vap_update_slot, vap);
@@ -2493,6 +2494,51 @@ wakeupwaiting(struct ieee80211vap *vap0)
 	}
 }
 
+static int
+_ieee80211_newstate_get_next_empty_slot(struct ieee80211vap *vap)
+{
+	int nstate_num;
+
+	IEEE80211_LOCK_ASSERT(vap->iv_ic);
+
+	if (vap->iv_nstate_n >= NET80211_IV_NSTATE_NUM)
+		return (-1);
+
+	nstate_num = vap->iv_nstate_b + vap->iv_nstate_n;
+	nstate_num %= NET80211_IV_NSTATE_NUM;
+	vap->iv_nstate_n++;
+
+	return (nstate_num);
+}
+
+static int
+_ieee80211_newstate_get_next_pending_slot(struct ieee80211vap *vap)
+{
+	int nstate_num;
+
+	IEEE80211_LOCK_ASSERT(vap->iv_ic);
+
+	KASSERT(vap->iv_nstate_n > 0, ("%s: vap %p iv_nstate_n %d\n",
+	    __func__, vap, vap->iv_nstate_n));
+
+	nstate_num = vap->iv_nstate_b;
+	vap->iv_nstate_b++;
+	if (vap->iv_nstate_b >= NET80211_IV_NSTATE_NUM)
+		vap->iv_nstate_b = 0;
+	vap->iv_nstate_n--;
+
+	return (nstate_num);
+}
+
+static int
+_ieee80211_newstate_get_npending(struct ieee80211vap *vap)
+{
+
+	IEEE80211_LOCK_ASSERT(vap->iv_ic);
+
+	return (vap->iv_nstate_n);
+}
+
 /*
  * Handle post state change work common to all operating modes.
  */
@@ -2502,17 +2548,25 @@ ieee80211_newstate_cb(void *xvap, int npending)
 	struct ieee80211vap *vap = xvap;
 	struct ieee80211com *ic = vap->iv_ic;
 	enum ieee80211_state nstate, ostate;
-	int arg, rc;
+	int arg, rc, nstate_num;
 
+	KASSERT(npending == 1, ("%s: vap %p with npending %d != 1\n",
+	    __func__, vap, npending));
 	IEEE80211_LOCK(ic);
-	nstate = vap->iv_nstate;
-	arg = vap->iv_nstate_arg;
+	nstate_num = _ieee80211_newstate_get_next_pending_slot(vap);
+
+	/*
+	 * Update the historic fields for now as they are used in some
+	 * drivers and reduce code changes for now.
+	 */
+	vap->iv_nstate = nstate = vap->iv_nstates[nstate_num];
+	arg = vap->iv_nstate_args[nstate_num];
 
 	IEEE80211_DPRINTF(vap, IEEE80211_MSG_STATE,
 	    "%s:%d: running state update %s -> %s (%d)\n",
 	    __func__, __LINE__,
 	    ieee80211_state_name[vap->iv_state],
-	    ieee80211_state_name[vap->iv_nstate],
+	    ieee80211_state_name[nstate],
 	    npending);
 
 	if (vap->iv_flags_ext & IEEE80211_FEXT_REINIT) {
@@ -2523,9 +2577,10 @@ ieee80211_newstate_cb(void *xvap, int npending)
 		/* Deny any state changes while we are here. */
 		vap->iv_nstate = IEEE80211_S_INIT;
 		IEEE80211_DPRINTF(vap, IEEE80211_MSG_STATE,
-		    "%s: %s -> %s arg %d\n", __func__,
+		    "%s: %s -> %s arg %d -> %s arg %d\n", __func__,
 		    ieee80211_state_name[vap->iv_state],
-		    ieee80211_state_name[vap->iv_nstate], arg);
+		    ieee80211_state_name[vap->iv_nstate], 0,
+		    ieee80211_state_name[nstate], arg);
 		vap->iv_newstate(vap, vap->iv_nstate, 0);
 		IEEE80211_LOCK_ASSERT(ic);
 		vap->iv_flags_ext &= ~(IEEE80211_FEXT_REINIT |
@@ -2666,7 +2721,7 @@ ieee80211_new_state_locked(struct ieee80211vap *vap,
 	struct ieee80211com *ic = vap->iv_ic;
 	struct ieee80211vap *vp;
 	enum ieee80211_state ostate;
-	int nrunning, nscanning;
+	int nrunning, nscanning, nstate_num;
 
 	IEEE80211_LOCK_ASSERT(ic);
 
@@ -2688,14 +2743,6 @@ ieee80211_new_state_locked(struct ieee80211vap *vap,
 			    ieee80211_state_name[nstate],
 			    ieee80211_state_name[vap->iv_nstate]);
 			return -1;
-		} else if (vap->iv_state != vap->iv_nstate) {
-			/* Warn if the previous state hasn't completed. */
-			IEEE80211_DPRINTF(vap, IEEE80211_MSG_STATE,
-			    "%s:%d: pending %s -> %s (now to %s) transition lost\n",
-			    __func__, __LINE__,
-			    ieee80211_state_name[vap->iv_state],
-			    ieee80211_state_name[vap->iv_nstate],
-			    ieee80211_state_name[nstate]);
 		}
 	}
 
@@ -2718,7 +2765,16 @@ ieee80211_new_state_locked(struct ieee80211vap *vap,
 				nscanning++;
 		}
 	}
-	ostate = vap->iv_state;
+	/*
+	 * Look ahead for the "old state" at that point when the last queued
+	 * state transition is run.
+	 */
+	if (vap->iv_nstate_n == 0) {
+		ostate = vap->iv_state;
+	} else {
+		nstate_num = (vap->iv_nstate_b + vap->iv_nstate_n - 1) % NET80211_IV_NSTATE_NUM;
+		ostate = vap->iv_nstates[nstate_num];
+	}
 	IEEE80211_DPRINTF(vap, IEEE80211_MSG_STATE,
 	    "%s: %s -> %s (arg %d) (nrunning %d nscanning %d)\n", __func__,
 	    ieee80211_state_name[ostate], ieee80211_state_name[nstate], arg,
@@ -2812,11 +2868,37 @@ ieee80211_new_state_locked(struct ieee80211vap *vap,
 	default:
 		break;
 	}
-	/* defer the state change to a thread */
-	vap->iv_nstate = nstate;
-	vap->iv_nstate_arg = arg;
+	/*
+	 * Defer the state change to a thread.
+	 * We support up-to NET80211_IV_NSTATE_NUM pending state changes
+	 * using a separate task for each. Otherwise, if we enqueue
+	 * more than one state change they will be folded together,
+	 * npedning will be > 1 and we may run then out of sequence with
+	 * other events.
+	 * This is kind-of a hack after 10 years but we know how to provoke
+	 * these cases now (and seen them in the wild).
+	 */
+	nstate_num = _ieee80211_newstate_get_next_empty_slot(vap);
+	if (nstate_num == -1) {
+		/*
+		 * This is really bad and we should just go kaboom.
+		 * Instead drop it.  No one checks the return code anyway.
+		 */
+		ic_printf(ic, "%s:%d: pending %s -> %s (now to %s) "
+		    "transition lost. %d/%d pending state changes:\n",
+		    __func__, __LINE__,
+		    ieee80211_state_name[vap->iv_state],
+		    ieee80211_state_name[vap->iv_nstate],
+		    ieee80211_state_name[nstate],
+		    _ieee80211_newstate_get_npending(vap),
+		    NET80211_IV_NSTATE_NUM);
+
+		return (EAGAIN);
+	}
+	vap->iv_nstates[nstate_num] = nstate;
+	vap->iv_nstate_args[nstate_num] = arg;
 	vap->iv_flags_ext |= IEEE80211_FEXT_STATEWAIT;
-	ieee80211_runtask(ic, &vap->iv_nstate_task);
+	ieee80211_runtask(ic, &vap->iv_nstate_task[nstate_num]);
 	return EINPROGRESS;
 }
 

sys/net80211/ieee80211_var.h

@@ -410,9 +410,16 @@ struct ieee80211vap {
 	uint32_t		iv_com_state;	/* com usage / detached flag */
 	enum ieee80211_opmode	iv_opmode;	/* operation mode */
 	enum ieee80211_state	iv_state;	/* state machine state */
-	enum ieee80211_state	iv_nstate;	/* pending state */
-	int			iv_nstate_arg;	/* pending state arg */
-	struct task		iv_nstate_task;	/* deferred state processing */
+
+	/* Deferred state processing. */
+	enum ieee80211_state	iv_nstate;		/* next pending state (historic) */
+#define	NET80211_IV_NSTATE_NUM	8
+	int			iv_nstate_b;		/* First filled slot. */
+	int			iv_nstate_n;		/* # of filled slots. */
+	enum ieee80211_state	iv_nstates[NET80211_IV_NSTATE_NUM];	/* queued pending state(s) */
+	int			iv_nstate_args[NET80211_IV_NSTATE_NUM];	/* queued pending state(s) arg */
+	struct task		iv_nstate_task[NET80211_IV_NSTATE_NUM];
+
 	struct task		iv_swbmiss_task;/* deferred iv_bmiss call */
 	struct callout		iv_mgtsend;	/* mgmt frame response timer */
 						/* inactivity timer settings */