[PATCH 12/18] bgscan_simple: Verify if notified signal is above or below threshold

[Otcheretianski, Andrei]

> -----Original Message-----
> From: Jouni Malinen [mailto:j at w1.fi]
> Sent: Saturday, October 01, 2016 11:22
> To: Otcheretianski, Andrei <andrei.otcheretianski at intel.com>
> Cc: hostap at lists.infradead.org; Stern, Avraham <avraham.stern at intel.com>
> Subject: Re: [PATCH 12/18] bgscan_simple: Verify if notified signal is above or
> below threshold
> 
> On Mon, Sep 05, 2016 at 05:33:05PM +0300, andrei.otcheretianski at intel.com
> wrote:
> > When receiving signal change notification check if the reported signal
> > is above or below the signal threshold.
> > Although the event data includes this information, the threshold
> > configured to the driver might have changed.
> 
> What would have changed the threshold in the driver? If there is some other
> components changing this, I don't see how exactly the bgscan mechanism in
> wpa_supplicant is going to be compatible with such changes.

This is a rare corner case when the previous configuration exists in the driver (it can be a leftover from previous bgscan configuration, or was configured directly through control interface).
So in this case wpa_s will update the new threshold value only when it moves to COMPLETED state, however the driver (at least mac80211) will apply the old value when it moves to assoc state.
It's harmless when such notifications are processed before bgscan_init() is called, since it will be just ignored, however it's possible to process an old notifications when wpa_s is already in COMPLETED state.

> 
> > diff --git a/wpa_supplicant/bgscan_simple.c
> > b/wpa_supplicant/bgscan_simple.c @@ -213,6 +213,14 @@ static void
> > bgscan_simple_notify_signal_change(void *priv, int above,
> > +	if ((current_signal > data->signal_threshold && !above) ||
> > +	    (current_signal < data->signal_threshold && above)) {
> > +		wpa_printf(MSG_DEBUG,
> > +			   "bgscan simple: incorrect above value: above=%d
> current_signal=%d threshold=%d",
> > +			   above, current_signal, data->signal_threshold);
> > +		above = current_signal > data->signal_threshold;
> > +	}
> 
> So this would be specific to bgscan_simple.c. What about other bgscan
> implementations? Wouldn't bgscan_learn.c has the exact same issue?

Yes. Bgscan_learn has the same issue too.
 
> bgscan_notify_signal_change() in bgscan.c could be a better place to do this.

bgscan_notify_signal_change() is not aware of thresholds... I'm not sure that this is a right place to do it.

> Or maybe even better would be to "hide" this within
> src/drivers/driver_nl80211{,_event}.c since this sounds like a driver interface
> specific unexpected behavior to me and not something that a bgscam
> module should need to be aware of.

Yep.. This sounds more correct to me too. So please drop this patch meanwhile and I'll submit a new patch to driver_nl80211 if we will keep seeing such races.

Thanks
Andrei
> 
> --
> Jouni Malinen                                            PGP id EFC895FA