[PATCH] P2P: Race condition in GO-NEG process if both peers enter p2p_connect at the same time

[Jouni Malinen]

On Mon, May 21, 2012 at 06:28:55AM +0000, Neeraj Kumar Garg wrote:
> We hit a case where both the peers assumed that other peer will be GO. Let us assume that p2p_connect command was given on both the peers using a script at the same time. Also assume that P1 has higher mac address than P2. 

Thanks for bringing this up! I'm pretty sure I've seen similar things
reported in the past and getting this address will hopefully make GO
Negotiation more robust.

> 1. P1 will send a GO-NEG-REQ and P2 will also send a GO-NEG-REQ.
> 2. Before P2 could get a callback p2p_go_neg_req_cb to update the variable go_neg_req_sent, P2 receives a GO-NEG request of P1 in the dwell time of its own request.
> 3. So P2 prepares the GO-NEG-RSP and send it even though its mac address is lower than P1 because go_neg_req_sent variable is NOT yet incremented.

It was a bit difficult to reproduce this, but I managed to build a
hacked version of wpa_supplicant to force this to happen more easily.. I
think that the race condition would actually be possible even with the
proposed change even though this makes it less likely to happen. As
such, we better be prepared for the unexpected GO Negotiation Response
frame regardless.

> 4. Now P1 will get P2's GO-NEG-REQ and will reply it since it has higher mac address.
> 5. Both peers end up in sending GO-CONF frame.

This part is the main issue that needs to be prevented since it can
result in getting different results from the GO negotiation. I think
I've now fixed this with the following commit:
http://w1.fi/gitweb/gitweb.cgi?p=hostap.git;a=commitdiff;h=198f82a1e3f78fd478e879185c43965c25840e0d

> To resolve this race, we propose that we increment go_neg_req_sent as soon as p2p_send_action is called for GO-NEG-REQ. And then decrement go_neg_req_sent if because of some reason the success is not reported in the callback p2p_go_neg_req_cb.

This sounds fine as an additional cleanup even though the other commit
should prevent the issue of getting conflicting negotiation results. I
applied this with some more robust design to avoid incrementing the
counter if sending of the action frame fails.

-- 
Jouni Malinen                                            PGP id EFC895FA