Re: [PATCH 1/2 RFC] cfg80211: connect/disconnect API

Johannes Berg
2009-06-22T19:27:26Z

[-- Attachment #1: Type: text/plain, Size: 8480 bytes --]

On Mon, 2009-06-22 at 19:04 +0200, Samuel Ortiz wrote:

> +/**
> + * cfg80211_connected - notify cfg80211 that connection was dropped
> + *
> + * @dev: network device
> + * @gfp: allocation flags
> + * @reason: reason code for the disconnection, set it to 0 when invalid.
> + *
> + */
> +void cfg80211_disconnected(struct net_device *dev, gfp_t gfp, u16 reason);

Shouldn't this also be called, with reason == 0, when a ->disconnect()
call has succeeded? Not sure why it can ever fail, but we have to give
userspace an event somehow.

> + /*
> + * Here we cache the assoc request. It will be used
> + * when the cfg80211 user will call cfg80211_authenticated()
> + * letting us know that we can move forward and call the
> + * assoc hook.
> + */
> + memset(&wdev->assoc_req, 0, sizeof(wdev->assoc_req));
> + if (connect->channel)
> + wdev->assoc_req.chan = kmemdup(connect->channel,
> + sizeof(struct ieee80211_channel),
> + GFP_ATOMIC);

??
->channel always points into the registered channels, that can't go
stale until the wiphy is killed.

> + if (connect->bssid)
> + wdev->assoc_req.peer_addr = kmemdup(connect->bssid,
> + ETH_ALEN,
> + GFP_ATOMIC);

just point it to wdev->bssid?

> + wdev->assoc_req.ssid = kmemdup(connect->ssid,
> + connect->ssid_len,
> + GFP_ATOMIC);
> + wdev->assoc_req.ssid_len = connect->ssid_len;

wdev->ssid?

> + if (connect->ie)
> + wdev->assoc_req.ie = kmemdup(connect->ie,
> + connect->ie_len,
> + GFP_ATOMIC);

Yes, this needs to be kmemdup'ed :) I don't think it needs GFP_ATOMIC
though? This should only be called from nl80211 or wext, i.e. in process
context.

> + wdev->current_bss = NULL;
> + wdev->ssid_len = 0;
> + memset(wdev->bssid, 0, ETH_ALEN);
> + kfree(wdev->assoc_req.chan);
> + kfree(wdev->assoc_req.peer_addr);
> + kfree(wdev->assoc_req.ssid);
> + kfree(wdev->assoc_req.ie);

Better set = NULL too, since you don't set = NULL in _connect() if the
parameter isn't present.

> void cfg80211_send_rx_assoc(struct net_device *dev, const u8 *buf, size_t len)
> {
> - struct wiphy *wiphy = dev->ieee80211_ptr->wiphy;
> + u16 status_code;
> + struct ieee80211_mgmt *mgmt;
> + struct wireless_dev *wdev = dev->ieee80211_ptr;
> + struct wiphy *wiphy = wdev->wiphy;
> struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> +
> + mgmt = (struct ieee80211_mgmt *)buf;
> + status_code = le16_to_cpu(mgmt->u.assoc_resp.status_code);
> +
> nl80211_send_rx_assoc(rdev, dev, buf, len);
> +
> + if (status_code != WLAN_STATUS_SUCCESS)
> + wdev->sme_state = CFG80211_SME_AUTH;
> + else {
> + u8 *ie = mgmt->u.assoc_resp.variable;

I wonder if we should give up, if non-successful auth? Wouldn't this
retry again and again? Well I guess you don't queue the work, but if
another interface causes it to be queued... so I guess we need to retry,
or something, but only a limited number of times? Not sure really.

> void cfg80211_send_deauth(struct net_device *dev, const u8 *buf, size_t len)
> {
> - struct wiphy *wiphy = dev->ieee80211_ptr->wiphy;
> + struct wireless_dev *wdev = dev->ieee80211_ptr;
> + struct wiphy *wiphy = wdev->wiphy;
> struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> +
> nl80211_send_deauth(rdev, dev, buf, len);
> +
> + if (wdev->sme_state == CFG80211_SME_CONNECTED ||
> + wdev->sme_state == CFG80211_SME_ASSOC) {
> + u16 reason_code;
> + struct ieee80211_mgmt *mgmt;
> +
> + mgmt = (struct ieee80211_mgmt *)buf;
> + reason_code = le16_to_cpu(mgmt->u.deauth.reason_code);
> +
> + cfg80211_disconnected(dev, GFP_ATOMIC, reason_code);
> + }

This might be interesting -- you could get a disconnected event without
ever asking for connected. But that indicates we need yet another event:
"I've given up on connect"?

> EXPORT_SYMBOL(cfg80211_send_deauth);
>
> void cfg80211_send_disassoc(struct net_device *dev, const u8 *buf, size_t len)
> {
> - struct wiphy *wiphy = dev->ieee80211_ptr->wiphy;
> + struct wireless_dev *wdev = dev->ieee80211_ptr;
> + struct wiphy *wiphy = wdev->wiphy;
> struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> +
> nl80211_send_disassoc(rdev, dev, buf, len);
> +
> + if (wdev->sme_state == CFG80211_SME_CONNECTED) {
> + u16 reason_code;
> + struct ieee80211_mgmt *mgmt;
> +
> + mgmt = (struct ieee80211_mgmt *)buf;
> + reason_code = le16_to_cpu(mgmt->u.disassoc.reason_code);
> +
> + cfg80211_disconnected(dev, GFP_ATOMIC, reason_code);
> + }

I suspect it needs to be valid to call this without a frame, if you just
get that indication from firmware somehow. However, then there's no
reason code to pass. Maybe we just need a second exported symbol in that
case and not worry about it now. Doesn't need to be perfect, after
all :)

> EXPORT_SYMBOL(cfg80211_send_disassoc);
>
> Index: iwm-2.6/net/wireless/core.c
> ===================================================================
> --- iwm-2.6.orig/net/wireless/core.c 2009-06-22 18:46:24.000000000 +0200
> +++ iwm-2.6/net/wireless/core.c 2009-06-22 18:46:30.000000000 +0200
> @@ -321,6 +321,7 @@ struct wiphy *wiphy_new(const struct cfg
> }
>
> INIT_WORK(&drv->rfkill_sync, cfg80211_rfkill_sync_work);
> + INIT_WORK(&drv->sme_sm, cfg80211_sme_work);
>
> /*
> * Initialize wiphy parameters to IEEE 802.11 MIB default values.
> @@ -481,6 +482,8 @@ void wiphy_unregister(struct wiphy *wiph
> /* unlock again before freeing */
> mutex_unlock(&drv->mtx);
>
> + cancel_work_sync(&drv->sme_sm);
> +
> cfg80211_debugfs_drv_del(drv);
>
> /* If this device got a regulatory hint tell core its
> @@ -526,54 +529,77 @@ static int cfg80211_netdev_notifier_call
> void *ndev)
> {
> struct net_device *dev = ndev;
> + struct wireless_dev *wdev = dev->ieee80211_ptr;
> struct cfg80211_registered_device *rdev;
>
> - if (!dev->ieee80211_ptr)
> + if (!wdev)
> return NOTIFY_DONE;
>
> - rdev = wiphy_to_dev(dev->ieee80211_ptr->wiphy);
> + rdev = wiphy_to_dev(wdev->wiphy);
>
> - WARN_ON(dev->ieee80211_ptr->iftype == NL80211_IFTYPE_UNSPECIFIED);
> + WARN_ON(wdev->iftype == NL80211_IFTYPE_UNSPECIFIED);
>
> switch (state) {
> case NETDEV_REGISTER:
> mutex_lock(&rdev->devlist_mtx);
> - list_add(&dev->ieee80211_ptr->list, &rdev->netdev_list);
> + list_add(&wdev->list, &rdev->netdev_list);
> if (sysfs_create_link(&dev->dev.kobj, &rdev->wiphy.dev.kobj,
> "phy80211")) {
> printk(KERN_ERR "wireless: failed to add phy80211 "
> "symlink to netdev!\n");
> }
> - dev->ieee80211_ptr->netdev = dev;
> + wdev->netdev = dev;
> + wdev->sme_state = CFG80211_SME_IDLE;
> +
> #ifdef CONFIG_WIRELESS_EXT
> - dev->ieee80211_ptr->wext.default_key = -1;
> - dev->ieee80211_ptr->wext.default_mgmt_key = -1;
> + wdev->wext.default_key = -1;
> + wdev->wext.default_mgmt_key = -1;
> #endif
> mutex_unlock(&rdev->devlist_mtx);
> break;
> case NETDEV_GOING_DOWN:
> - if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_ADHOC)
> + if (!wdev->ssid_len)
> + break;
> +
> + switch (wdev->iftype) {
> + case NL80211_IFTYPE_ADHOC:
> + cfg80211_leave_ibss(rdev, dev, true);
> + break;
> + case NL80211_IFTYPE_STATION:
> + cfg80211_disconnect(rdev, dev,
> + WLAN_REASON_DEAUTH_LEAVING);
> break;
> - if (!dev->ieee80211_ptr->ssid_len)
> + default:
> break;
> - cfg80211_leave_ibss(rdev, dev, true);
> + }
> +
> break;
> case NETDEV_UP:
> #ifdef CONFIG_WIRELESS_EXT
> - if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_ADHOC)
> + switch (wdev->iftype) {
> + case NL80211_IFTYPE_ADHOC:
> + if (wdev->wext.ibss.ssid_len &&
> + cfg80211_join_ibss(rdev, dev, &wdev->wext.ibss))
> + return NOTIFY_STOP;
> break;
> - if (!dev->ieee80211_ptr->wext.ibss.ssid_len)
> + case NL80211_IFTYPE_STATION:
> + if (wdev->wext.connect.ssid_len &&
> + cfg80211_connect(rdev, dev, &wdev->wext.connect))
> + return NOTIFY_STOP;
> break;
> - cfg80211_join_ibss(rdev, dev, &dev->ieee80211_ptr->wext.ibss);
> - break;
> + default:
> + break;
> + }
> #endif
> + break;

Wow, that fixes a bad bug if you don't have wext configured. Good thing
that's currently pretty much impossible.

Thanks!