| Message ID | 1426776103-3973-2-git-send-email-eliad@wizery.com |
|---|---|
| State | Deferred |
| Headers | show |
On Thu, Mar 19, 2015 at 04:41:40PM +0200, Eliad Peller wrote: > Update the dialog token even when GO negotiation > failed, in order prevent the same dialog token > from being used again. Could you please clarify what this solves and which frame would be the target for preventing the same dialog token from being used again? The change here is in p2p_process_go_neg_req() and that function does not verify dialog_token matching (only GO Neg Resp and Conf has such code as far as GO Negotiation is concerned).
hi Jouni, On Fri, Mar 20, 2015 at 3:18 PM, Jouni Malinen <j@w1.fi> wrote: > On Thu, Mar 19, 2015 at 04:41:40PM +0200, Eliad Peller wrote: >> Update the dialog token even when GO negotiation >> failed, in order prevent the same dialog token >> from being used again. > > Could you please clarify what this solves and which frame would be the > target for preventing the same dialog token from being used again? The > change here is in p2p_process_go_neg_req() and that function does not > verify dialog_token matching (only GO Neg Resp and Conf has such code as > far as GO Negotiation is concerned). > this doesn't solve any specific scenario, but just seems like the right thing to do in order to prevent the same dialog token from being reused (e.g. when sending a new provision discovery). Eliad.
On Sun, Mar 22, 2015 at 11:58:26AM +0200, Eliad Peller wrote: > On Fri, Mar 20, 2015 at 3:18 PM, Jouni Malinen <j@w1.fi> wrote: > > Could you please clarify what this solves and which frame would be the > > target for preventing the same dialog token from being used again? The > > change here is in p2p_process_go_neg_req() and that function does not > > verify dialog_token matching (only GO Neg Resp and Conf has such code as > > far as GO Negotiation is concerned). > > > this doesn't solve any specific scenario, but just seems like the > right thing to do in order to prevent the same dialog token from being > reused (e.g. when sending a new provision discovery). Provision Discovery? Why would take make any difference? The dialog token is specific to each request/response(/confirm) transaction for a specific type of frames; it does not have any meaning between GO Negotiation frames and Provision Discovery frames. If fetching a dialog token from a GO Negotiation frame and then using that dialog token in a Provision Discovery frame is the intent here, I don't see why that would be of any benefit.
On Sun, Mar 22, 2015 at 7:11 PM, Jouni Malinen <j@w1.fi> wrote: > On Sun, Mar 22, 2015 at 11:58:26AM +0200, Eliad Peller wrote: >> On Fri, Mar 20, 2015 at 3:18 PM, Jouni Malinen <j@w1.fi> wrote: >> > Could you please clarify what this solves and which frame would be the >> > target for preventing the same dialog token from being used again? The >> > change here is in p2p_process_go_neg_req() and that function does not >> > verify dialog_token matching (only GO Neg Resp and Conf has such code as >> > far as GO Negotiation is concerned). >> > >> this doesn't solve any specific scenario, but just seems like the >> right thing to do in order to prevent the same dialog token from being >> reused (e.g. when sending a new provision discovery). > > Provision Discovery? Why would take make any difference? The dialog > token is specific to each request/response(/confirm) transaction for a > specific type of frames; it does not have any meaning between GO > Negotiation frames and Provision Discovery frames. If fetching a dialog > token from a GO Negotiation frame and then using that dialog token in a > Provision Discovery frame is the intent here, I don't see why that would > be of any benefit. > i thought it makes sense to always make the dialog token unique (per-device) and incrementing. i guess it's also relevant for the P2P_SC_FAIL_INFO_CURRENTLY_UNAVAILABLE case, where a new GO Negotiation might be attempted, while reusing the the dialog token. Eliad.
On Mon, Mar 23, 2015 at 10:19:41AM +0200, Eliad Peller wrote: > i thought it makes sense to always make the dialog token unique > (per-device) and incrementing. Why? > i guess it's also relevant for the > P2P_SC_FAIL_INFO_CURRENTLY_UNAVAILABLE case, where a new GO > Negotiation might be attempted, while reusing the the dialog token. Please provide more details on this and ideally a debug log showing the benefit. This is an area that has risk for interoperability issues and I don't really want to change anything in the externally visible behavior without a good justification on why such a change would be needed.
On Mon, Mar 23, 2015 at 10:28 AM, Jouni Malinen <j@w1.fi> wrote: > On Mon, Mar 23, 2015 at 10:19:41AM +0200, Eliad Peller wrote: >> i thought it makes sense to always make the dialog token unique >> (per-device) and incrementing. > > Why? > >> i guess it's also relevant for the >> P2P_SC_FAIL_INFO_CURRENTLY_UNAVAILABLE case, where a new GO >> Negotiation might be attempted, while reusing the the dialog token. > > Please provide more details on this and ideally a debug log showing the > benefit. > > This is an area that has risk for interoperability issues and I don't > really want to change anything in the externally visible behavior > without a good justification on why such a change would be needed. > ok. so please drop it for now. i'm not sure there is an actual error here. i'll re-send it (with logs) in case i'll encounter any actual issue. Eliad.
diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c index 98abf9d..f9e9581 100644 --- a/src/p2p/p2p_go_neg.c +++ b/src/p2p/p2p_go_neg.c @@ -807,7 +807,6 @@ void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa, p2p_stop_find_for_freq(p2p, rx_freq); p2p_set_state(p2p, P2P_GO_NEG); p2p_clear_timeout(p2p); - dev->dialog_token = msg.dialog_token; os_memcpy(dev->intended_addr, msg.intended_addr, ETH_ALEN); p2p->go_neg_peer = dev; eloop_cancel_timeout(p2p_go_neg_wait_timeout, p2p, NULL); @@ -815,8 +814,10 @@ void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa, } fail: - if (dev) + if (dev) { dev->status = status; + dev->dialog_token = msg.dialog_token; + } resp = p2p_build_go_neg_resp(p2p, dev, msg.dialog_token, status, !tie_breaker); p2p_parse_free(&msg);
Update the dialog token even when GO negotiation failed, in order prevent the same dialog token from being used again. Signed-off-by: Eliad Peller <eliad@wizery.com> --- src/p2p/p2p_go_neg.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)