Re: git: e8de31caceaa - main - net80211: Fix traffic hang on STA/AP VAPs on a multi-VAP interface

On Fri, 22 Apr 2022, Adrian Chadd wrote:

> The branch main has been updated by adrian:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=e8de31caceaa36caf5d7b4355072f148e2433b82
>
> commit e8de31caceaa36caf5d7b4355072f148e2433b82
> Author:     Adrian Chadd <adrian@FreeBSD.org>
> AuthorDate: 2022-04-12 20:20:28 +0000
> Commit:     Adrian Chadd <adrian@FreeBSD.org>
> CommitDate: 2022-04-22 05:49:01 +0000
>
>    net80211: Fix traffic hang on STA/AP VAPs on a multi-VAP interface
>
>    This took an embarrasingly long time to find.
>
>    The state changes for a radio with a STA /and/ AP VAP gets a bit messy.
>    The AP maps are marked as waiting, waiting for the STA AP to find a
>    channel to use before the AP VAPs become active.
>
>    However, the code path that clears the OACTIVE flag on a VAP only runs
>    during a successful run of ieee80211_newstate_cb().
>
>    So here is how it goes:
>
>    * the STA VAP goes down and needs to scan;
>    * the AP vap goes RUN->INIT; but it doesn't YET call ieee80211_newstate_cb();
>    * meanwhile - a send on the AP VAP causes the VAP to set the OACTIVE flag here;
>    * then the STA VAP finishes scan and goes to RUN;
>    * which will call wakeupwaiting() as part of the STA VAP transition to RUN;
>    * .. then the AP VAP goes INIT->RUN directly via a call to hostap_newstate
>      in wakeupwaiting rather than it being through the deferred path;
>    * /then/ the ieee80211_newstate_cb() is called, but it sees the state go
>      RUN->RUN;
>    * .. which results in the OACTIVE flag never being cleared.
>
>    This clears the OACTIVE flag when a VAP transitions RUN->RUN; the
>    driver layer or net80211 layer can set it if required in a subsequent
>    transmit.
>
>    Differential Revision: https://reviews.freebsd.org/D34920
>
>    Reviewed by: bz

I had not reviewed any final version of this given I was on the road
and hadn't checked anything beyond my direct inbox until minutes ago.

And I still see three large blobs of text in three places only with
minor changes of words and the same one and only line of work of this
change done twice and not removed the 2nd time, so clearly review
comments weren't fully addressed.


> ---
> sys/net80211/ieee80211_proto.c | 47 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/sys/net80211/ieee80211_proto.c b/sys/net80211/ieee80211_proto.c
> index 2228983050a2..d2bde99ce79c 100644
> --- a/sys/net80211/ieee80211_proto.c
> +++ b/sys/net80211/ieee80211_proto.c
> @@ -2469,6 +2469,29 @@ wakeupwaiting(struct ieee80211vap *vap0)
> 			vap->iv_flags_ext &= ~IEEE80211_FEXT_SCANWAIT;
> 			/* NB: sta's cannot go INIT->RUN */
> 			/* NB: iv_newstate may drop the lock */
> +
> +			/*
> +			 * This is problematic if the interface has OACTIVE
> +			 * set.  Only the deferred ieee80211_newstate_cb()
> +			 * will end up actually /clearing/ the OACTIVE
> +			 * flag on a state transition to RUN from a non-RUN
> +			 * state.
> +			 *
> +			 * But, we're not actually deferring this callback;
> +			 * and when the deferred call occurs it shows up as
> +			 * a RUN->RUN transition!  So the flag isn't/wasn't
> +			 * cleared!
> +			 *
> +			 * I'm also not sure if it's correct to actually
> +			 * do the transitions here fully through the deferred
> +			 * paths either as other things can be invoked as
> +			 * part of that state machine.
> +			 *
> +			 * So just keep this in mind when looking at what
> +			 * the markwaiting/wakeupwaiting routines are doing
> +			 * and how they invoke vap state changes.
> +			 */
> +
> 			vap->iv_newstate(vap,
> 			    vap->iv_opmode == IEEE80211_M_STA ?
> 			        IEEE80211_S_SCAN : IEEE80211_S_RUN, 0);
> @@ -2543,6 +2566,30 @@ ieee80211_newstate_cb(void *xvap, int npending)
> 		goto done;
> 	}
>
> +	/*
> +	 * Handle the case of a RUN->RUN transition occuring when STA + AP
> +	 * VAPs occur on the same radio.
> +	 *
> +	 * The mark and wakeup waiting routines call iv_newstate() directly,
> +	 * but they do not end up deferring state changes here.
> +	 * Thus, although the VAP newstate method sees a transition
> +	 * of RUN->INIT->RUN, the deferred path here only sees a RUN->RUN
> +	 * transition.  If OACTIVE is set then it is never cleared.
> +	 *
> +	 * So, if we're here and the state is RUN, just clear OACTIVE.
> +	 * At some point if the markwaiting/wakeupwaiting paths end up
> +	 * also invoking the deferred state updates then this will
> +	 * be no-op code - and also if OACTIVE is finally retired, it'll
> +	 * also be no-op code.
> +	 */
> +	if (nstate == IEEE80211_S_RUN) {
> +		/*
> +		 * Unblock the VAP queue; a RUN->RUN state can happen
> +		 * on a STA+AP setup on the AP vap.  See wakeupwaiting().
> +		 */
> +		vap->iv_ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
> +	}
> +
> 	/* No actual transition, skip post processing */
> 	if (ostate == nstate)
> 		goto done;
>

-- 
Bjoern A. Zeeb                                                     r15:7