| Message ID | 1250640253-18434-6-git-send-email-kilroyd@googlemail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 2009-08-19 at 01:04 +0100, David Kilroy wrote: > @@ -791,18 +824,55 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev, > > return err; > } else { > + struct cfg80211_bss *bss; > + > wdev->sme_state = CFG80211_SME_CONNECTING; > wdev->connect_keys = connkeys; > + > + bss = cfg80211_get_bss(wdev->wiphy, NULL, connect->bssid, > + connect->ssid, connect->ssid_len, > + WLAN_CAPABILITY_ESS, > + WLAN_CAPABILITY_ESS); Hmm. What if the bssid isn't set? Then the card might select a different BSS than the one we have on the scan list. > + /* Failed to clone (or scan), so we can't > + * delay the connect. Free everything up and > + * go ahead with the connect */ > + if (wdev->conn) > + kfree(wdev->conn->ie); > + kfree(wdev->conn); > + wdev->conn = NULL; and that would then run into the warning and the problem anyway? Better to just reject with -ENOMEM I think? Also, I really don't think you should use wdev->conn anywhere in this code path, because some code looks at that to figure out whether or not the cfg80211 SME is used. > + } else { > + cfg80211_put_bss(bss); > err = rdev->ops->connect(&rdev->wiphy, dev, connect); And it's all racy too -- by the time the driver calls connect_result(), the BSS might have expired after it was found here now. I don't think this is really feasible to implement in cfg80211. Couldn't the driver do a probe to the BSS that the device selected, and report that before the connect result? johannes
Johannes Berg wrote: > On Wed, 2009-08-19 at 01:04 +0100, David Kilroy wrote: >> @@ -791,18 +824,55 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev, >> + bss = cfg80211_get_bss(wdev->wiphy, NULL, connect->bssid, >> + connect->ssid, connect->ssid_len, >> + WLAN_CAPABILITY_ESS, >> + WLAN_CAPABILITY_ESS); > > Hmm. What if the bssid isn't set? Then the card might select a different > BSS than the one we have on the scan list. That's correct. For the Agere driver that's also true when bssid is set - we can't specify which AP the firmware connects to. >> + /* Failed to clone (or scan), so we can't >> + * delay the connect. Free everything up and >> + * go ahead with the connect */ >> + if (wdev->conn) >> + kfree(wdev->conn->ie); >> + kfree(wdev->conn); >> + wdev->conn = NULL; > > and that would then run into the warning and the problem anyway? Better > to just reject with -ENOMEM I think? Also, I really don't think you > should use wdev->conn anywhere in this code path, because some code > looks at that to figure out whether or not the cfg80211 SME is used. I thought that might be the case. >> + } else { >> + cfg80211_put_bss(bss); > >> err = rdev->ops->connect(&rdev->wiphy, dev, connect); > > And it's all racy too -- by the time the driver calls connect_result(), > the BSS might have expired after it was found here now. Agreed, but with a 15s expiry period I wouldn't expect this to be a problem in practice. > I don't think this is really feasible to implement in cfg80211. cfg80211 seemed like the appropriate place because it avoids different (fullmac) drivers having to re-implement this. > Couldn't > the driver do a probe to the BSS that the device selected, and report > that before the connect result? Yes, that's possible. If we went this way it would make sense to encode this in the interface by changing the cfg80211_connect_result prototype to: void cfg80211_connect_result(struct net_device *dev, const struct bss *bss, const u8 *req_ie, size_t req_ie_len, const u8 *resp_ie, size_t resp_ie_len, u16 status, gfp_t gfp); So on connecting: * the driver has to call cfg80211_get_bss() to get the bss pointer. * if it is not available, scan/probe to get the information, call cfg80211_inform_bss(), and then use the returned pointer in the cfg80211_connect_result call. This means the driver may have to hold onto the IE info to use after the scan returns. Another alternative is for cfg80211_connect_result to trigger the scan if it doesn't have the bss, and only complete the connect when the scan returns. I think I like the sound of this best. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2009-08-19 at 20:30 +0100, Dave wrote: > > Hmm. What if the bssid isn't set? Then the card might select a different > > BSS than the one we have on the scan list. > > That's correct. For the Agere driver that's also true when bssid is set > - we can't specify which AP the firmware connects to. Ok. We may want a feature flag for the latter case so we know what's going on, and reject a BSSID setting. > >> + } else { > >> + cfg80211_put_bss(bss); > > > >> err = rdev->ops->connect(&rdev->wiphy, dev, connect); > > > > And it's all racy too -- by the time the driver calls connect_result(), > > the BSS might have expired after it was found here now. > > Agreed, but with a 15s expiry period I wouldn't expect this to be a > problem in practice. Well, the user could scan, take 12 seconds to pick out the AP manually, enter the paramters in another 2.5 seconds, and then it would already happen, I think? > > I don't think this is really feasible to implement in cfg80211. > > cfg80211 seemed like the appropriate place because it avoids different > (fullmac) drivers having to re-implement this. True. > > Couldn't > > the driver do a probe to the BSS that the device selected, and report > > that before the connect result? > > Yes, that's possible. If we went this way it would make sense to encode > this in the interface by changing the cfg80211_connect_result prototype to: > > void cfg80211_connect_result(struct net_device *dev, > const struct bss *bss, > const u8 *req_ie, size_t req_ie_len, > const u8 *resp_ie, size_t resp_ie_len, > u16 status, gfp_t gfp); > > So on connecting: > > * the driver has to call cfg80211_get_bss() to get the bss pointer. > > * if it is not available, scan/probe to get the information, call > cfg80211_inform_bss(), and then use the returned pointer in the > cfg80211_connect_result call. > > This means the driver may have to hold onto the IE info to use after the > scan returns. Indeed, that would work, although it seems somewhat pointless to pass around the BSS pointer and require the driver to do the lookup, but it nicely avoids any races that we have even now. > Another alternative is for cfg80211_connect_result to trigger the scan > if it doesn't have the bss, and only complete the connect when the scan > returns. I think I like the sound of this best. Good option too, though then it would be useful to pass the channel pointer if available to scan only on that channel. Of course, if it still can't be found things are really amiss and we should probably disconnect and send a failed event to userspace. johannes
Quoting Dave <kilroyd@googlemail.com>: > > Another alternative is for cfg80211_connect_result to trigger the scan > if it doesn't have the bss, and only complete the connect when the scan > returns. I think I like the sound of this best. > This sounds best, as with rndis, driver can get currently connected bss from device and scan isn't required. -Jussi -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Johannes Berg wrote: > On Wed, 2009-08-19 at 20:30 +0100, Dave wrote: >>> Hmm. What if the bssid isn't set? Then the card might select a different >>> BSS than the one we have on the scan list. >> That's correct. For the Agere driver that's also true when bssid is set >> - we can't specify which AP the firmware connects to. > > Ok. We may want a feature flag for the latter case so we know what's > going on, and reject a BSSID setting. I'll look into that. >>> And it's all racy too -- by the time the driver calls connect_result(), >>> the BSS might have expired after it was found here now. >> Agreed, but with a 15s expiry period I wouldn't expect this to be a >> problem in practice. > > Well, the user could scan, take 12 seconds to pick out the AP manually, > enter the paramters in another 2.5 seconds, and then it would already > happen, I think? As usual, you're right :) >> Another alternative is for cfg80211_connect_result to trigger the scan >> if it doesn't have the bss, and only complete the connect when the scan >> returns. I think I like the sound of this best. johannes said: > Good option too, though then it would be useful to pass the channel > pointer if available to scan only on that channel. Of course, if it > still can't be found things are really amiss and we should probably > disconnect and send a failed event to userspace. Jussi said: > This sounds best, as with rndis, driver can get currently connected > bss from device and scan isn't required. OK, I'll chase this option and see where it goes. Regards, Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 9ddc00e..6dc7981 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -26,11 +26,13 @@ struct cfg80211_conn { CFG80211_CONN_AUTHENTICATING, CFG80211_CONN_ASSOCIATE_NEXT, CFG80211_CONN_ASSOCIATING, + CFG80211_CONN_DELAYED_CONNECT, } state; u8 bssid[ETH_ALEN], prev_bssid[ETH_ALEN]; u8 *ie; size_t ie_len; bool auto_auth, prev_bssid_valid; + bool connect_after_scan; }; @@ -148,6 +150,19 @@ static int cfg80211_conn_do_work(struct wireless_dev *wdev) NULL, 0, WLAN_REASON_DEAUTH_LEAVING); return err; + case CFG80211_CONN_DELAYED_CONNECT: + BUG_ON(!rdev->ops->connect); + err = rdev->ops->connect(&rdev->wiphy, wdev->netdev, + &wdev->conn->params); + kfree(wdev->conn->ie); + kfree(wdev->conn); + wdev->conn = NULL; + if (err) { + wdev->connect_keys = NULL; + wdev->sme_state = CFG80211_SME_IDLE; + wdev->ssid_len = 0; + } + return err; default: return 0; } @@ -234,6 +249,12 @@ static void __cfg80211_sme_scan_done(struct net_device *dev) wdev->conn->state != CFG80211_CONN_SCAN_AGAIN) return; + if (wdev->conn->connect_after_scan) { + wdev->conn->state = CFG80211_CONN_DELAYED_CONNECT; + schedule_work(&rdev->conn_work); + return; + } + if (!cfg80211_get_conn_bss(wdev)) { /* not found */ if (wdev->conn->state == CFG80211_CONN_SCAN_AGAIN) @@ -655,6 +676,52 @@ void cfg80211_disconnected(struct net_device *dev, u16 reason, } EXPORT_SYMBOL(cfg80211_disconnected); +static int cfg80211_conn_clone(struct wireless_dev *wdev, + struct cfg80211_connect_params *connect) +{ + if (WARN_ON(wdev->conn)) + return -EINPROGRESS; + + wdev->conn = kzalloc(sizeof(*wdev->conn), GFP_KERNEL); + if (!wdev->conn) + return -ENOMEM; + + memcpy(&wdev->conn->params, connect, sizeof(*connect)); + if (connect->bssid) { + wdev->conn->params.bssid = wdev->conn->bssid; + memcpy(wdev->conn->bssid, connect->bssid, ETH_ALEN); + } + + if (connect->ie) { + wdev->conn->ie = kmemdup(connect->ie, connect->ie_len, + GFP_KERNEL); + wdev->conn->params.ie = wdev->conn->ie; + if (!wdev->conn->ie) { + kfree(wdev->conn); + wdev->conn = NULL; + return -ENOMEM; + } + } + + if (connect->auth_type == NL80211_AUTHTYPE_AUTOMATIC) { + wdev->conn->auto_auth = true; + /* start with open system ... should mostly work */ + wdev->conn->params.auth_type = + NL80211_AUTHTYPE_OPEN_SYSTEM; + } else { + wdev->conn->auto_auth = false; + } + + memcpy(wdev->ssid, connect->ssid, connect->ssid_len); + wdev->ssid_len = connect->ssid_len; + wdev->conn->params.ssid = wdev->ssid; + wdev->conn->params.ssid_len = connect->ssid_len; + + wdev->conn->connect_after_scan = false; + + return 0; +} + int __cfg80211_connect(struct cfg80211_registered_device *rdev, struct net_device *dev, struct cfg80211_connect_params *connect, @@ -710,46 +777,12 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev, if (!rdev->ops->auth || !rdev->ops->assoc) return -EOPNOTSUPP; - if (WARN_ON(wdev->conn)) - return -EINPROGRESS; - - wdev->conn = kzalloc(sizeof(*wdev->conn), GFP_KERNEL); - if (!wdev->conn) - return -ENOMEM; - /* * Copy all parameters, and treat explicitly IEs, BSSID, SSID. */ - memcpy(&wdev->conn->params, connect, sizeof(*connect)); - if (connect->bssid) { - wdev->conn->params.bssid = wdev->conn->bssid; - memcpy(wdev->conn->bssid, connect->bssid, ETH_ALEN); - } - - if (connect->ie) { - wdev->conn->ie = kmemdup(connect->ie, connect->ie_len, - GFP_KERNEL); - wdev->conn->params.ie = wdev->conn->ie; - if (!wdev->conn->ie) { - kfree(wdev->conn); - wdev->conn = NULL; - return -ENOMEM; - } - } - - if (connect->auth_type == NL80211_AUTHTYPE_AUTOMATIC) { - wdev->conn->auto_auth = true; - /* start with open system ... should mostly work */ - wdev->conn->params.auth_type = - NL80211_AUTHTYPE_OPEN_SYSTEM; - } else { - wdev->conn->auto_auth = false; - } - - memcpy(wdev->ssid, connect->ssid, connect->ssid_len); - wdev->ssid_len = connect->ssid_len; - wdev->conn->params.ssid = wdev->ssid; - wdev->conn->params.ssid_len = connect->ssid_len; + err = cfg80211_conn_clone(wdev, connect); + if (err) + return err; /* don't care about result -- but fill bssid & channel */ if (!wdev->conn->params.bssid || !wdev->conn->params.channel) @@ -791,18 +824,55 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev, return err; } else { + struct cfg80211_bss *bss; + wdev->sme_state = CFG80211_SME_CONNECTING; wdev->connect_keys = connkeys; + + bss = cfg80211_get_bss(wdev->wiphy, NULL, connect->bssid, + connect->ssid, connect->ssid_len, + WLAN_CAPABILITY_ESS, + WLAN_CAPABILITY_ESS); + if (!bss) { + /* We don't have a matching BSS. + * + * This means __connect_result will fail, + * unless the driver provides scan results + * between now and then. So do an explicit + * scan, and try connect later. + */ + err = cfg80211_conn_clone(wdev, connect); + if (!err) { + wdev->conn->connect_after_scan = true; + err = cfg80211_conn_scan(wdev); + + if (!err) + return 0; + } + + /* Failed to clone (or scan), so we can't + * delay the connect. Free everything up and + * go ahead with the connect */ + if (wdev->conn) + kfree(wdev->conn->ie); + kfree(wdev->conn); + wdev->conn = NULL; + + } else { + cfg80211_put_bss(bss); + + memcpy(wdev->ssid, connect->ssid, connect->ssid_len); + wdev->ssid_len = connect->ssid_len; + } + err = rdev->ops->connect(&rdev->wiphy, dev, connect); if (err) { wdev->connect_keys = NULL; wdev->sme_state = CFG80211_SME_IDLE; + wdev->ssid_len = 0; return err; } - memcpy(wdev->ssid, connect->ssid, connect->ssid_len); - wdev->ssid_len = connect->ssid_len; - return 0; } }
The connect_result callback relies on cfg80211 having the bss information (via beacons/probes). For a fullmac driver, this information is only likely to be present after a scan. Userspace is not guaranteed to scan before connecting (e.g. wpa_supplicant ap_scan=2 in wext mode), so before calling ->connect do an explicit ->scan if we don't have the bss in our list. Signed-off-by: David Kilroy <kilroyd@googlemail.com> --- net/wireless/sme.c | 150 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 110 insertions(+), 40 deletions(-)