| Message ID | 20210301233406.3964281-1-matthewmwang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Reject auths during explicit roam requests | expand |
On 3/1/21 3:34 PM, 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. I had similar issues, in case it helps, here is my patch: https://github.com/greearb/hostap-ct/commit/50b1c17d0a782dc4d354ac7366500763611ea5af I first relaxed thresholds by default, and then allowed configuring them. When something beyond hostapd is controlling roaming, I set the thresholds so that it would never automatically roam based on scan results. Lord git only knows how much of this was built on previous patches in my tree... Thanks, Ben > > Signed-off-by: Matthew Wang <matthewmwang@chromium.org> > Series-to: hostap@lists.infradead.org > Series-cc: j@w1.fi, matthewmwang@chromium.org > Change-Id: Ibeb6cceddabd10cab6938411f54ed63f4d4428c1 > --- > 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(+) > > diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c > index 0d4c2bc8f0b..048fe72d472 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 = true; > + } > + > 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..6803558accd 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 = true; > + } > + > return NULL; > #endif /* CONFIG_NO_SCAN_PROCESSING */ > } > diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c > index 522c8297f7e..dde80863a98 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 = false; > + > 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..b6c339ab711 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 */ > + bool 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 >
Thanks Ben, the patch is a good idea, but doesn't exactly address my issue. The D-Bus/wpa_cli roam command sets the reassociate bit, and this short circuits the roam logic in wpa_supplicant_need_to_roam (it just returns true at the top of the function instead of going into need_to_roam_within_ess). On Mon, Mar 1, 2021 at 3:58 PM Ben Greear <greearb@candelatech.com> wrote: > > On 3/1/21 3:34 PM, 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. > > I had similar issues, in case it helps, here is my patch: > > https://github.com/greearb/hostap-ct/commit/50b1c17d0a782dc4d354ac7366500763611ea5af > > I first relaxed thresholds by default, and then allowed configuring them. When something > beyond hostapd is controlling roaming, I set the thresholds so that it would never automatically > roam based on scan results. > > Lord git only knows how much of this was built on previous patches in my tree... > > Thanks, > Ben > > > > > > Signed-off-by: Matthew Wang <matthewmwang@chromium.org> > > Series-to: hostap@lists.infradead.org > > Series-cc: j@w1.fi, matthewmwang@chromium.org > > Change-Id: Ibeb6cceddabd10cab6938411f54ed63f4d4428c1 > > --- > > 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(+) > > > > diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c > > index 0d4c2bc8f0b..048fe72d472 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 = true; > > + } > > + > > 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..6803558accd 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 = true; > > + } > > + > > return NULL; > > #endif /* CONFIG_NO_SCAN_PROCESSING */ > > } > > diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c > > index 522c8297f7e..dde80863a98 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 = false; > > + > > 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..b6c339ab711 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 */ > > + bool 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 > > > > > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com
On 3/1/21 5:25 PM, Matthew Wang wrote: > Thanks Ben, the patch is a good idea, but doesn't exactly address my > issue. The D-Bus/wpa_cli roam command sets the reassociate bit, and > this short circuits the roam logic in wpa_supplicant_need_to_roam (it > just returns true at the top of the function instead of going into > need_to_roam_within_ess). Yeah, I believe your application would only want to temporarily fully restrict the auto-roaming, where I wanted a way to do it for the duration of my scenario (and then would re-load a new supplicant config afterwards). Seems to me that you would want to disable roam-based-on-scan results for a bit longer though if you are explicitly trying a roam based on logic that should know better than supplicant (otherwise, why bother, just let supplicant do the job). Thanks, Ben > > On Mon, Mar 1, 2021 at 3:58 PM Ben Greear <greearb@candelatech.com> wrote: >> >> On 3/1/21 3:34 PM, 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. >> >> I had similar issues, in case it helps, here is my patch: >> >> https://github.com/greearb/hostap-ct/commit/50b1c17d0a782dc4d354ac7366500763611ea5af >> >> I first relaxed thresholds by default, and then allowed configuring them. When something >> beyond hostapd is controlling roaming, I set the thresholds so that it would never automatically >> roam based on scan results. >> >> Lord git only knows how much of this was built on previous patches in my tree... >> >> Thanks, >> Ben >> >> >>> >>> Signed-off-by: Matthew Wang <matthewmwang@chromium.org> >>> Series-to: hostap@lists.infradead.org >>> Series-cc: j@w1.fi, matthewmwang@chromium.org >>> Change-Id: Ibeb6cceddabd10cab6938411f54ed63f4d4428c1 >>> --- >>> 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(+) >>> >>> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c >>> index 0d4c2bc8f0b..048fe72d472 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 = true; >>> + } >>> + >>> 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..6803558accd 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 = true; >>> + } >>> + >>> return NULL; >>> #endif /* CONFIG_NO_SCAN_PROCESSING */ >>> } >>> diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c >>> index 522c8297f7e..dde80863a98 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 = false; >>> + >>> 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..b6c339ab711 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 */ >>> + bool 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 >>> >> >> >> -- >> Ben Greear <greearb@candelatech.com> >> Candela Technologies Inc http://www.candelatech.com >
diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c index 0d4c2bc8f0b..048fe72d472 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 = true; + } + 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..6803558accd 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 = true; + } + return NULL; #endif /* CONFIG_NO_SCAN_PROCESSING */ } diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c index 522c8297f7e..dde80863a98 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 = false; + 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..b6c339ab711 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 */ + bool 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> Series-to: hostap@lists.infradead.org Series-cc: j@w1.fi, matthewmwang@chromium.org Change-Id: Ibeb6cceddabd10cab6938411f54ed63f4d4428c1 --- 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(+)