| Message ID | 20210223004925.342911-1-matthewmwang@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Reject auths during explicit roam requests | expand |
On Mon, Feb 22, 2021 at 04:49:25PM -0800, Matthew Wang wrote: > The roam D-Bus and wpa_cli commands flip the reassociate bit before > calling wpa_supplicant_connect. wpa_supplicant connect eventually aborts > ongoing scans, which causes scan results to be reported. Since the > reassociate bit is set, this will trigger a connection attempt based on > the aborted scan's scan results and cancel the initial connetion > request. This often causes wpa_supplicant to reassociate to the same AP > it is currently associated to. > > Add a roam_in_progress flag to indicate that we're currently attempting > to roam via D-Bus so that we don't initate another connection attempt. > diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c > @@ -5686,6 +5686,15 @@ static int wpa_supplicant_ctrl_iface_roam(struct wpa_supplicant *wpa_s, > + /* > + * Indicate that an explicitly requested roam is in progress so scan > + * results that come in before the 'sme-connect' radio work gets > + * executed do not override the original connection attempt. > + */ > + if (radio_work_pending(wpa_s, "sme-connect")) { > + wpa_s->roam_in_progress = 1; > + } Is this limited to "sme-connect" on purpose or should "connect" be included as well? I understand the only user of wpa_s->roam_in_progress in this patch is in sme.c, but is that really the only issue that this tries to address or could the same issue with ignoring an explicit roam request apply to drivers that use internal SME instead of the wpa_supplicant/sme.c implementation with split authentication and association steps? > diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c > index c6cef5b1447..4fd0c5e104f 100644 > --- a/wpa_supplicant/sme.c > +++ b/wpa_supplicant/sme.c > @@ -941,6 +941,8 @@ static void sme_auth_start_cb(struct wpa_radio_work *work, int deinit) > struct wpa_connect_work *cwork = work->ctx; > struct wpa_supplicant *wpa_s = work->wpa_s; > > + wpa_s->roam_in_progress = 0; This is now the only place where this new variable is reset to 0. Is that sufficient? What if the roam attempt fails for some reason before being able to reach the authentication step? Could this flag be forgotten in place and could that cause issues for the future completely unrelated connection attempt? > @@ -981,6 +983,11 @@ void sme_authenticate(struct wpa_supplicant *wpa_s, > return; > } > > + if (wpa_s->roam_in_progress) { > + wpa_dbg(wpa_s, MSG_DEBUG, > + "SME: Reject sme_authenticate() in favor of explicit roam request"); > + return; > + } I.e., how would this recover from sme_auth_start_cb() not getting called after the last roam request? > diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h > + int roam_in_progress; /* roam in progress */ The preferred type for new boolean variables is to use the C99 boolean type through bool val = true/false.
Thanks for the replies Jouni. > Is this limited to "sme-connect" on purpose or should "connect" be included as well? This is limited to sme-connect. Where the two differ is that sme-connect always prefers the newest request while connect defaults to keeping the pending one. > This is now the only place where this new variable is reset to 0. Is that sufficient? What if the roam attempt fails for some reason before being able to reach the authentication step? Could this flag be forgotten in place and could that cause issues for the future completely unrelated connection attempt? As far as I can tell, the callback will always be called. The flag will only be set when we see that there is an 'sme-connect' radio work pending, and when the radio work is pending, it should always call the callback (and the flag is reset at the very top of the callback). Even cancelling the radio work should call the callback, just with deinit == 1 (this won't prevent the flag from being reset since it's done before we check for deinit). Unless there is a way to cancel the radio work without triggering the callback (I'm not aware of any), then the flag should always be reset. I'll resend the patch with bool instead of int. On Fri, Feb 26, 2021 at 1:25 PM Jouni Malinen <j@w1.fi> wrote: > > On Mon, Feb 22, 2021 at 04:49:25PM -0800, Matthew Wang wrote: > > The roam D-Bus and wpa_cli commands flip the reassociate bit before > > calling wpa_supplicant_connect. wpa_supplicant connect eventually aborts > > ongoing scans, which causes scan results to be reported. Since the > > reassociate bit is set, this will trigger a connection attempt based on > > the aborted scan's scan results and cancel the initial connetion > > request. This often causes wpa_supplicant to reassociate to the same AP > > it is currently associated to. > > > > Add a roam_in_progress flag to indicate that we're currently attempting > > to roam via D-Bus so that we don't initate another connection attempt. > > > diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c > > @@ -5686,6 +5686,15 @@ static int wpa_supplicant_ctrl_iface_roam(struct wpa_supplicant *wpa_s, > > + /* > > + * Indicate that an explicitly requested roam is in progress so scan > > + * results that come in before the 'sme-connect' radio work gets > > + * executed do not override the original connection attempt. > > + */ > > + if (radio_work_pending(wpa_s, "sme-connect")) { > > + wpa_s->roam_in_progress = 1; > > + } > > Is this limited to "sme-connect" on purpose or should "connect" be > included as well? I understand the only user of wpa_s->roam_in_progress > in this patch is in sme.c, but is that really the only issue that this > tries to address or could the same issue with ignoring an explicit roam > request apply to drivers that use internal SME instead of the > wpa_supplicant/sme.c implementation with split authentication and > association steps? > > > diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c > > index c6cef5b1447..4fd0c5e104f 100644 > > --- a/wpa_supplicant/sme.c > > +++ b/wpa_supplicant/sme.c > > @@ -941,6 +941,8 @@ static void sme_auth_start_cb(struct wpa_radio_work *work, int deinit) > > struct wpa_connect_work *cwork = work->ctx; > > struct wpa_supplicant *wpa_s = work->wpa_s; > > > > + wpa_s->roam_in_progress = 0; > > This is now the only place where this new variable is reset to 0. Is > that sufficient? What if the roam attempt fails for some reason before > being able to reach the authentication step? Could this flag be > forgotten in place and could that cause issues for the future completely > unrelated connection attempt? > > > @@ -981,6 +983,11 @@ void sme_authenticate(struct wpa_supplicant *wpa_s, > > return; > > } > > > > + if (wpa_s->roam_in_progress) { > > + wpa_dbg(wpa_s, MSG_DEBUG, > > + "SME: Reject sme_authenticate() in favor of explicit roam request"); > > + return; > > + } > > I.e., how would this recover from sme_auth_start_cb() not getting called > after the last roam request? > > > diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h > > + int roam_in_progress; /* roam in progress */ > > The preferred type for new boolean variables is to use the C99 boolean > type through bool val = true/false. > > -- > Jouni Malinen PGP id EFC895FA
diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c index 0d4c2bc8f0b..c89be5d4482 100644 --- a/wpa_supplicant/ctrl_iface.c +++ b/wpa_supplicant/ctrl_iface.c @@ -5686,6 +5686,15 @@ static int wpa_supplicant_ctrl_iface_roam(struct wpa_supplicant *wpa_s, wpa_s->reassociate = 1; wpa_supplicant_connect(wpa_s, bss, ssid); + /* + * Indicate that an explicitly requested roam is in progress so scan + * results that come in before the 'sme-connect' radio work gets + * executed do not override the original connection attempt. + */ + if (radio_work_pending(wpa_s, "sme-connect")) { + wpa_s->roam_in_progress = 1; + } + return 0; #endif /* CONFIG_NO_SCAN_PROCESSING */ } diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c index 7d20f2123a8..5ace9bd396d 100644 --- a/wpa_supplicant/dbus/dbus_new_handlers.c +++ b/wpa_supplicant/dbus/dbus_new_handlers.c @@ -2005,6 +2005,15 @@ DBusMessage * wpas_dbus_handler_roam(DBusMessage *message, wpa_s->reassociate = 1; wpa_supplicant_connect(wpa_s, bss, ssid); + /* + * Indicate that an explicitly requested roam is in progress so scan + * results that come in before the 'sme-connect' radio work gets + * executed do not override the original connection attempt. + */ + if (radio_work_pending(wpa_s, "sme-connect")) { + wpa_s->roam_in_progress = 1; + } + return NULL; #endif /* CONFIG_NO_SCAN_PROCESSING */ } diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c index c6cef5b1447..4fd0c5e104f 100644 --- a/wpa_supplicant/sme.c +++ b/wpa_supplicant/sme.c @@ -941,6 +941,8 @@ static void sme_auth_start_cb(struct wpa_radio_work *work, int deinit) struct wpa_connect_work *cwork = work->ctx; struct wpa_supplicant *wpa_s = work->wpa_s; + wpa_s->roam_in_progress = 0; + if (deinit) { if (work->started) wpa_s->connect_work = NULL; @@ -981,6 +983,11 @@ void sme_authenticate(struct wpa_supplicant *wpa_s, return; } + if (wpa_s->roam_in_progress) { + wpa_dbg(wpa_s, MSG_DEBUG, + "SME: Reject sme_authenticate() in favor of explicit roam request"); + return; + } if (radio_work_pending(wpa_s, "sme-connect")) { /* * The previous sme-connect work might no longer be valid due to diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h index 7d14b627d89..d15b5c6a4d1 100644 --- a/wpa_supplicant/wpa_supplicant_i.h +++ b/wpa_supplicant/wpa_supplicant_i.h @@ -621,6 +621,7 @@ struct wpa_supplicant { u8 pending_bssid[ETH_ALEN]; /* If wpa_state == WPA_ASSOCIATING, this * field contains the target BSSID. */ int reassociate; /* reassociation requested */ + int roam_in_progress; /* roam in progress */ unsigned int reassoc_same_bss:1; /* reassociating to the same BSS */ unsigned int reassoc_same_ess:1; /* reassociating to the same ESS */ int disconnected; /* all connections disabled; i.e., do no reassociate
The roam D-Bus and wpa_cli commands flip the reassociate bit before calling wpa_supplicant_connect. wpa_supplicant connect eventually aborts ongoing scans, which causes scan results to be reported. Since the reassociate bit is set, this will trigger a connection attempt based on the aborted scan's scan results and cancel the initial connetion request. This often causes wpa_supplicant to reassociate to the same AP it is currently associated to. Add a roam_in_progress flag to indicate that we're currently attempting to roam via D-Bus so that we don't initate another connection attempt. Signed-off-by: Matthew Wang <matthewmwang@chromium.org> --- wpa_supplicant/ctrl_iface.c | 9 +++++++++ wpa_supplicant/dbus/dbus_new_handlers.c | 9 +++++++++ wpa_supplicant/sme.c | 7 +++++++ wpa_supplicant/wpa_supplicant_i.h | 1 + 4 files changed, 26 insertions(+)