[06/10] wpa_supplicant: Handle race condition in sched_scan stop

From: Roee Zamir <roee.zamir@intel.com>

In case that stop sched command is sent after the completion of
scheduled scan and before the processing of the
EVENT_SCHED_SCAN_STOPPED event, stopping the scheduled scan would
fail, in such a case do not set the sched_scan_stop_req flag.

Signed-off-by: Roee Zamir <roee.zamir@intel.com>
---
 wpa_supplicant/scan.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On Thu, Oct 27, 2016 at 03:18:28PM +0300, Andrei Otcheretianski wrote:
> In case that stop sched command is sent after the completion of
> scheduled scan and before the processing of the
> EVENT_SCHED_SCAN_STOPPED event, stopping the scheduled scan would
> fail, in such a case do not set the sched_scan_stop_req flag.

So this patch is for wpas_stop_pno().. What about
wpa_supplicant_cancel_sched_scan() which is also calling
wpa_supplicant_stop_sched_scan() and setting wpa_s->sched_scan_stop_req
= 1 regardless of what wpa_supplicant_stop_sched_scan() returns?

> diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
> @@ -2550,7 +2550,14 @@ int wpas_stop_pno(struct wpa_supplicant *wpa_s)
>  		return 0;
>  
>  	ret = wpa_supplicant_stop_sched_scan(wpa_s);
> -	wpa_s->sched_scan_stop_req = 1;
> +
> +	/* In case that stop sched command is sent after the completion of
> +	 * sched scan and before processing the EVENT_SCHED_SCAN_STOPPED event,
> +	 * stopping the scheduled scan would fail.
> +	 * In such a case do not set the flag
> +	 */
> +	if (!ret)
> +		wpa_s->sched_scan_stop_req = 1;
>  
>  	wpa_s->pno = 0;

So this is already protected with !wpa_s->pno above this context. I'm
not sure I fully understood the sequence in which this could be hit.
Would you happen to have a debug log showing a case where this change is
needed here?

> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Saturday, October 29, 2016 19:37
> To: Otcheretianski, Andrei <andrei.otcheretianski@intel.com>
> Cc: hostap@lists.infradead.org; Zamir, Roee <roee.zamir@intel.com>
> Subject: Re: [PATCH 06/10] wpa_supplicant: Handle race condition in
> sched_scan stop
> 
> On Thu, Oct 27, 2016 at 03:18:28PM +0300, Andrei Otcheretianski wrote:
> > In case that stop sched command is sent after the completion of
> > scheduled scan and before the processing of the
> > EVENT_SCHED_SCAN_STOPPED event, stopping the scheduled scan would
> > fail, in such a case do not set the sched_scan_stop_req flag.
> 
> So this patch is for wpas_stop_pno().. What about
> wpa_supplicant_cancel_sched_scan() which is also calling
> wpa_supplicant_stop_sched_scan() and setting wpa_s-
> >sched_scan_stop_req = 1 regardless of what
> wpa_supplicant_stop_sched_scan() returns?

The problem with wpas_stop_pno() is that it relies on the EVENT_SCHED_SCAN_STOPPED processing to clear the sched_scan_stop_req flag.
If the event doesn't arrive (or was processed before) wpas_start_pno() will not work and only set pno_sched_pending flag.
And pno_sched_pending will prevent wpa_supplicant_scan too.
This is not the case wpa_supplicant_cancel_sched_scan() - wpa_supplicant_req_sched_scan() will just clear this flag and continue.
 
> > diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c @@ -2550,7
> > +2550,14 @@ int wpas_stop_pno(struct wpa_supplicant *wpa_s)
> >  		return 0;
> >
> >  	ret = wpa_supplicant_stop_sched_scan(wpa_s);
> > -	wpa_s->sched_scan_stop_req = 1;
> > +
> > +	/* In case that stop sched command is sent after the completion of
> > +	 * sched scan and before processing the
> EVENT_SCHED_SCAN_STOPPED event,
> > +	 * stopping the scheduled scan would fail.
> > +	 * In such a case do not set the flag
> > +	 */
> > +	if (!ret)
> > +		wpa_s->sched_scan_stop_req = 1;
> >
> >  	wpa_s->pno = 0;
> 
> So this is already protected with !wpa_s->pno above this context. I'm not
> sure I fully understood the sequence in which this could be hit.

wpa_s->pno is set correctly but I don't see how it can prevent the flow that I described.

> Would you happen to have a debug log showing a case where this change is
> needed here?

We have some logs based on our internal tree - the flow there is a little bit different, so it will be confusing if I'll post it here.
Basically we hit " Schedule PNO after previous sched scan has stopped" and pno never gets restarted and the scan is stuck with "Skip scan - PNO is in progress".

> 
> --
> Jouni Malinen                                            PGP id EFC895FA