[PATCH 1/1] P2P: Prevent pending_action_tx from truncating extended listen

[Jouni Malinen]

On Thu, Aug 07, 2014 at 04:33:46PM +0530, Jithu Jance wrote:
> On receiving the cancel remain on channel event, the pending_tx
> is scheduled immediately and returned. This was preventing
> the wpas_p2p_listen_start function from execution thereby resulting
> in termination of the extended listen.
> 
> Please see whether the patch is fine.

This looks a bit confusing. I think some of the confusion can be removed
by fixing small issues, but even after that, I'm not sure I understood
or agreed with all the remaining changes.

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -4943,27 +4955,35 @@ void wpas_p2p_cancel_remain_on_channel_cb(struct wpa_supplicant *wpa_s,
>  	wpas_p2p_listen_work_done(wpa_s);
>  	if (wpa_s->global->p2p_disabled || wpa_s->global->p2p == NULL)
>  		return;
> -	if (p2p_listen_end(wpa_s->global->p2p, freq) > 0)
> +	if (p2p_listen_end(wpa_s->global->p2p, freq) > 0) {
> +		wpa_printf(MSG_DEBUG, "P2P: p2p module started a new operation");
>  		return; /* P2P module started a new operation */

That is just a debug print change, but it made the diff a bit more
complex than necessary.

> -	if (offchannel_pending_action_tx(wpa_s))
> -		return;

This is the larger part, i.e., moving this call to the end of the
function. The return here is clearly trying to avoid starting the next
Listen step, so this looks suspicious. The new place of the return call
is in the end of the function, so it doesn't do anything.

>  	if (wpa_s->p2p_long_listen > 0)
>  		wpa_s->p2p_long_listen -= wpa_s->max_remain_on_chan;
> -	if (wpa_s->p2p_long_listen > 0) {
> -		wpa_printf(MSG_DEBUG, "P2P: Continuing long Listen state");
> -		wpas_p2p_listen_start(wpa_s, wpa_s->p2p_long_listen);
> -	} else {
> +	else {

This looks incorrect.. The previous version decremented
wpa_s->p2p_long_listen first and only after that compared it to >0 (and
well, <=0 in case of the else clause).

>  		/*
>  		 * When listen duration is over, stop listen & update p2p_state
>  		 * to IDLE.
>  		 */
>  		p2p_stop_listen(wpa_s->global->p2p);
>  	}

So this could potentially not happen for the first time p2p_long_listen
drops to zero (or below).

> +	if (offchannel_pending_action_tx(wpa_s)) {
> +		/*
> +		 * Allow the pending tx to happen before we
> +		 * resume the listen or SEARCH operation
> +		 */
> +		if (wpa_s->p2p_long_listen > 0)
> +			eloop_register_timeout(0, 100,
> +				wpas_p2p_continue_listen, wpa_s, NULL);
> +		return;
> +	}

And this is the place where wpas_p2p_listen_start() could be called even
if there is a pending Action TX that should really be sent instead of
starting a new Listen operation.

In addition, this open a race condition on the eloop timeout, i.e.,
there would need to the unregister call somewhere on P2P deinit path to
avoid crashes due to this timeout getting left behind. Anyway, I'm not
yet convinced that this change is fine in the current form.

Unless I'm missing something here, more appropriate way of addressing
this would be to have the completion of the pending Action frame TX
schedule a new P2P Listen if wpa_s->p2p_long_listen > 0.

-- 
Jouni Malinen                                            PGP id EFC895FA