Patchwork [1/2] P2P: improve deinit for go neg non-zero status

login
register
mail settings
Submitter Eduardo Abinader
Date May 11, 2014, 4:54 p.m.
Message ID <1399827284-4893-1-git-send-email-eduardo.abinader@openbossa.org>
Download mbox | patch
Permalink /patch/347800/
State New
Headers show

Comments

Eduardo Abinader - May 11, 2014, 4:54 p.m.
Added cancel timeout and issuing formation failed, in order
to improve detection for group formation failure, upon
non-zero status.

Signed-off-by: Eduardo Abinader <eduardo.abinader@openbossa.org>
---
 wpa_supplicant/p2p_supplicant.c | 2 ++
 1 file changed, 2 insertions(+)
Jouni Malinen - May 12, 2014, 3:18 p.m.
On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote:
> Added cancel timeout and issuing formation failed, in order
> to improve detection for group formation failure, upon
> non-zero status.

> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res)
>  			       res->status);
>  		wpas_notify_p2p_go_neg_completed(wpa_s, res);
>  		wpas_p2p_remove_pending_group_interface(wpa_s);
> +		eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL);
> +		wpas_p2p_group_formation_failed(wpa_s);
>  		return;
>  	}

I can understand the wpas_p2p_group_formation_failed() call, but what is
the purpose of canceling wpas_p2p_long_listen_timeout here? Actually,
that same question would really apply for the earlier commit
5eae87a7d61b053fb315c667f97954d004c195a5 ('P2P: Fix GO failed interface
init') as well.. That was the only place where that
eloop_cancel_timeout() was introduced without clearing
wpa_s->p2p_long_listen to zero.

The p2p_long_listen and its timeout mechanism are used to stop long
listen operation so that it does not trigger during group formation
steps. Doing that in failure cases looks a bit odd. I think I'd rather
remove that previously added eloop_cancel_timeout() and this one here as
well, unless there is a good reason for these that I cannot currently
think of. These changes would also make patch 2/2 less useful (and well,
if it should be added, I'd say it should also do wpa_s->p2p_long_listen
= 0).
Eduardo Abinader - May 12, 2014, 3:42 p.m.
On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote:
>
> On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote:
> > Added cancel timeout and issuing formation failed, in order
> > to improve detection for group formation failure, upon
> > non-zero status.
>
> > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res)
> >                              res->status);
> >               wpas_notify_p2p_go_neg_completed(wpa_s, res);
> >               wpas_p2p_remove_pending_group_interface(wpa_s);
> > +             eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL);
> > +             wpas_p2p_group_formation_failed(wpa_s);
> >               return;
> >       }
>
> I can understand the wpas_p2p_group_formation_failed() call, but what is
> the purpose of canceling wpas_p2p_long_listen_timeout here? Actually,
> that same question would really apply for the earlier commit
> 5eae87a7d61b053fb315c667f97954d004c195a5 ('P2P: Fix GO failed interface
> init') as well.. That was the only place where that
> eloop_cancel_timeout() was introduced without clearing
> wpa_s->p2p_long_listen to zero.

You're right. We must ensure p2p_long_listen=0.

I think we could keep this cancel_timeout, in order to earlier free
the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just
moving after the formation_failed). This would comply with
p2p_stop_find_oper and p2p_listen, that do the same.

Either way, keeping the timeout everything would be fine also.


>
> The p2p_long_listen and its timeout mechanism are used to stop long
> listen operation so that it does not trigger during group formation
> steps. Doing that in failure cases looks a bit odd. I think I'd rather
> remove that previously added eloop_cancel_timeout() and this one here as
> well, unless there is a good reason for these that I cannot currently
> think of. These changes would also make patch 2/2 less useful (and well,
> if it should be added, I'd say it should also do wpa_s->p2p_long_listen
> = 0).
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
Jouni Malinen - May 12, 2014, 4:29 p.m.
On Mon, May 12, 2014 at 11:42:55AM -0400, Eduardo Abinader wrote:
> On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote:
> >
> > On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote:
> > > Added cancel timeout and issuing formation failed, in order
> > > to improve detection for group formation failure, upon
> > > non-zero status.

Actually, where does this improve detection?

> > > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res)
> > >                              res->status);
> > >               wpas_notify_p2p_go_neg_completed(wpa_s, res);
> > >               wpas_p2p_remove_pending_group_interface(wpa_s);
> > > +             eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL);
> > > +             wpas_p2p_group_formation_failed(wpa_s);
> > >               return;
> > >       }

I was trying to go through the wpas_p2p_group_formation_failed() path
here and it ends up doing number of things that do not have any
relevance for this specific non-zero status case:
- eloop_cancel_timeout for wpas_p2p_group_formation_timeout (which has
  not even been started yet)
- p2p_group_formation_failed (which does not do anything new since
  p2p_go_neg_failed() is already taking care of that before calling this
  callback)
- wpas_group_formation_completed (which does number of items related to
  clearing group interface or network block which have not even been
  added yet).

In other words, I'm clearly missing something else about this.. What
does not detect that group formation failed based on the already
existing notification on GO Negotiation failure?

> You're right. We must ensure p2p_long_listen=0.
> 
> I think we could keep this cancel_timeout, in order to earlier free
> the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just
> moving after the formation_failed). This would comply with
> p2p_stop_find_oper and p2p_listen, that do the same.
> 
> Either way, keeping the timeout everything would be fine also.

I was thinking of going ahead with p2p_long_listen = 0 added here, but
then got to the questions above.. Clearly this would do much more than
what could potentially be needed here and as such, I don't think it is
necessary to call wpas_p2p_group_formation_failed(). Instead, a subset
of that functionality could be added here for GO Negotiation failure
case if such subset can be identified.
Eduardo Abinader - May 12, 2014, 5:21 p.m.
On Mon, May 12, 2014 at 12:29 PM, Jouni Malinen <j@w1.fi> wrote:
> On Mon, May 12, 2014 at 11:42:55AM -0400, Eduardo Abinader wrote:
>> On Mon, May 12, 2014 at 11:18 AM, Jouni Malinen <j@w1.fi> wrote:
>> >
>> > On Sun, May 11, 2014 at 12:54:43PM -0400, Eduardo Abinader wrote:
>> > > Added cancel timeout and issuing formation failed, in order
>> > > to improve detection for group formation failure, upon
>> > > non-zero status.
>
> Actually, where does this improve detection?
>
>> > > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
>> > > @@ -1590,6 +1590,8 @@ static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res)
>> > >                              res->status);
>> > >               wpas_notify_p2p_go_neg_completed(wpa_s, res);
>> > >               wpas_p2p_remove_pending_group_interface(wpa_s);
>> > > +             eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL);
>> > > +             wpas_p2p_group_formation_failed(wpa_s);
>> > >               return;
>> > >       }
>
> I was trying to go through the wpas_p2p_group_formation_failed() path
> here and it ends up doing number of things that do not have any
> relevance for this specific non-zero status case:
> - eloop_cancel_timeout for wpas_p2p_group_formation_timeout (which has
>   not even been started yet)

Actually, it depends on the scenario. I have faced issues with wps
pbc, where timeout is being set.

> - p2p_group_formation_failed (which does not do anything new since
>   p2p_go_neg_failed() is already taking care of that before calling this
>   callback)

p2p_group_formation_failed for this particular case, 4-way handshake
is not completed, and status is non-zero, it speeds up the deauth. It
acutally does: I got 4 to 5 seconds faster.

> - wpas_group_formation_completed (which does number of items related to
>   clearing group interface or network block which have not even been
>   added yet).
>
> In other words, I'm clearly missing something else about this.. What
> does not detect that group formation failed based on the already
> existing notification on GO Negotiation failure?
>

As I said, the 4-way handshake may not complete, falling under
non-zero status situation. Thus the GO-neg failure may not apply.

>> You're right. We must ensure p2p_long_listen=0.
>>
>> I think we could keep this cancel_timeout, in order to earlier free
>> the timeout queue and set this p2p_long_listen=0 in patch 2/2 (just
>> moving after the formation_failed). This would comply with
>> p2p_stop_find_oper and p2p_listen, that do the same.
>>
>> Either way, keeping the timeout everything would be fine also.
>
> I was thinking of going ahead with p2p_long_listen = 0 added here, but
> then got to the questions above.. Clearly this would do much more than
> what could potentially be needed here and as such, I don't think it is
> necessary to call wpas_p2p_group_formation_failed(). Instead, a subset
> of that functionality could be added here for GO Negotiation failure
> case if such subset can be identified.
>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap

Patch

diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index 522d277..62e628d 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -1590,6 +1590,8 @@  static void wpas_go_neg_completed(void *ctx, struct p2p_go_neg_results *res)
 			       res->status);
 		wpas_notify_p2p_go_neg_completed(wpa_s, res);
 		wpas_p2p_remove_pending_group_interface(wpa_s);
+		eloop_cancel_timeout(wpas_p2p_long_listen_timeout, wpa_s, NULL);
+		wpas_p2p_group_formation_failed(wpa_s);
 		return;
 	}