This introduces the pending config (not used yet) and by doing so
fixes the memory leak in the wext compat code.
Signed-off-by: Johannes Berg <[email protected]>
---
include/net/cfg80211.h | 4 ++-
include/net/wireless.h | 3 ++
net/wireless/core.h | 5 ----
net/wireless/nl80211.c | 2 -
net/wireless/wext-compat.c | 46 +++++----------------------------------------
5 files changed, 13 insertions(+), 47 deletions(-)
--- wireless-dev.orig/include/net/wireless.h 2007-02-15 13:23:41.937940064 +0100
+++ wireless-dev/include/net/wireless.h 2007-02-15 13:23:43.707940064 +0100
@@ -45,6 +45,9 @@ struct wiphy {
*/
struct wireless_dev {
struct wiphy *wiphy;
+
+ /* private to the generic wireless code */
+ struct cfg80211_config pending_config;
};
/**
--- wireless-dev.orig/include/net/cfg80211.h 2007-02-15 13:23:41.967940064 +0100
+++ wireless-dev/include/net/cfg80211.h 2007-02-15 13:23:43.707940064 +0100
@@ -11,6 +11,8 @@
* Copyright 2006 Johannes Berg <[email protected]>
*/
+#define SSID_MAX_LEN 32
+
/**
* struct cfg80211_config - description of a configuration (request)
*/
@@ -19,7 +21,7 @@ struct cfg80211_config {
u32 valid;
s8 ssid_len;
- u8 *ssid;
+ u8 ssid[SSID_MAX_LEN];
u16 network_id;
s32 rx_sensitivity;
--- wireless-dev.orig/net/wireless/core.h 2007-02-15 13:23:42.037940064 +0100
+++ wireless-dev/net/wireless/core.h 2007-02-15 13:23:43.707940064 +0100
@@ -24,11 +24,6 @@ struct cfg80211_registered_device {
/* wiphy index, internal only */
int idx;
-#ifdef CONFIG_CFG80211_WEXT_COMPAT
- /* wext compat */
- struct cfg80211_config *wext_pending_config;
-#endif
-
/* must be last because of the way we do wiphy_priv(),
* and it should at least be aligned to NETDEV_ALIGN */
struct wiphy wiphy __attribute__((__aligned__(NETDEV_ALIGN)));
--- wireless-dev.orig/net/wireless/nl80211.c 2007-02-15 13:23:42.137940064 +0100
+++ wireless-dev/net/wireless/nl80211.c 2007-02-15 13:23:43.717940064 +0100
@@ -389,8 +389,8 @@ static int nl80211_configure(struct sk_b
attr = info->attrs[NL80211_ATTR_SSID];
if (attr) {
- config.ssid = nla_data(attr);
config.ssid_len = nla_len(attr);
+ memcpy(config.ssid, nla_data(attr), config.ssid_len);
}
attr = info->attrs[NL80211_ATTR_NETWORK_ID];
--- wireless-dev.orig/net/wireless/wext-compat.c 2007-02-15 13:23:42.207940064 +0100
+++ wireless-dev/net/wireless/wext-compat.c 2007-02-15 13:23:43.717940064 +0100
@@ -10,26 +10,16 @@
*
* Theory of operation, so to speak:
*
- * To implement compatibility, I added a new field to struct net_device
- * that contains the pending configuration structure. This is dynamically
- * allocated when needed and freed when committed.
- *
* Commit is done some time after the last parameter was changed
* (with each parameter change simply (re-)schedule a timer) or
* if explicitly asked for. This is probably not what most people
* would expect, but perfectly fine in the WE API.
*
- * NB: we leak memory if the user
- * - changes some settings
- * - quickly rmmod's the module so that the net device is destroyed
- * Since only root can do it and I don't see a way to hook into
- * the net device's destruction... tough.
- *
- * NB2: Note that each of the wrappers should check if the cfg80211
+ * NB: Note that each of the wrappers should check if the cfg80211
* user provides the command, and for configure() it must also check
* if that parameter can be set or not via get_config_valid()
*
- * NB3: It's really bad that we can't report an error from the timer-
+ * NB2: It's really bad that we can't report an error from the timer-
* based commit... Hopefully get_config_valid() can catch everything?
*
* see set_essid for an example
@@ -67,17 +57,9 @@ static void cfg80211_wx_start_commit_tim
* as well as taking the rtnl lock (due to wext)! */
}
-static struct cfg80211_config *cfg80211_ensure_pending_cfg(
- struct cfg80211_registered_device *drv)
+static struct cfg80211_config *get_pending_cfg(struct net_device *dev)
{
- struct cfg80211_config *cfg = drv->wext_pending_config;
- if (!cfg)
- cfg = kmalloc(sizeof(*cfg)+32, GFP_KERNEL);
- if (cfg) {
- cfg->ssid = (char*)cfg + sizeof(*cfg);
- drv->wext_pending_config = cfg;
- }
- return cfg;
+ return &dev->ieee80211_ptr->pending_config;
}
static int cfg80211_wx_set_commit(struct cfg80211_registered_device *drv,
@@ -86,20 +68,8 @@ static int cfg80211_wx_set_commit(struct
union iwreq_data *data,
char *extra)
{
- int err;
-
- if (!drv->wext_pending_config) {
- err = 0;
- goto out;
- }
-
- err = drv->ops->configure(&drv->wiphy, net_dev,
- drv->wext_pending_config);
-
- kfree(drv->wext_pending_config);
- drv->wext_pending_config = NULL;
+ int err = -EOPNOTSUPP;
- out:
return err;
}
@@ -317,11 +287,7 @@ static int cfg80211_wx_set_essid(struct
& CFG80211_CFG_VALID_SSID))
goto out;
- cfg = cfg80211_ensure_pending_cfg(drv);
- if (!cfg) {
- err = -ENOMEM;
- goto out;
- }
+ cfg = get_pending_cfg(net_dev);
memcpy(cfg->ssid, extra, data->essid.length);
cfg->ssid_len = data->essid.length;
--
On Friday 16 February 2007 14:10, Dan Williams wrote:
> > --- wireless-dev.orig/include/net/cfg80211.h 2007-02-15
> > 13:23:41.967940064 +0100 +++
> > wireless-dev/include/net/cfg80211.h 2007-02-15 13:23:43.707940064 +0100
> > @@ -11,6 +11,8 @@
> > * Copyright 2006 Johannes Berg <[email protected]>
> > */
> >
> > +#define SSID_MAX_LEN 32
> > +
>
> This has _got_ to be defined somewhere else in the stack? Can we use
> that define, or is it being moved to here? Seems a shame to have it
> more than one place.
>
> On a side note, many many drivers need something like this, and many
> many drivers redefine it internally. Is there some header that all
> drivers (even fullmac like airo, orinoco, atmel, and libertas) can get
> this from?
>
That's what include/linux/ieee80211.h is (or will be) for.
-Michael Wu
On Thu, 2007-02-15 at 15:42 +0100, Johannes Berg wrote:
> plain text document attachment (009-pending-config.patch)
> This introduces the pending config (not used yet) and by doing so
> fixes the memory leak in the wext compat code.
>
> Signed-off-by: Johannes Berg <[email protected]>
>
> ---
> include/net/cfg80211.h | 4 ++-
> include/net/wireless.h | 3 ++
> net/wireless/core.h | 5 ----
> net/wireless/nl80211.c | 2 -
> net/wireless/wext-compat.c | 46 +++++----------------------------------------
> 5 files changed, 13 insertions(+), 47 deletions(-)
>
> --- wireless-dev.orig/include/net/wireless.h 2007-02-15 13:23:41.937940064 +0100
> +++ wireless-dev/include/net/wireless.h 2007-02-15 13:23:43.707940064 +0100
> @@ -45,6 +45,9 @@ struct wiphy {
> */
> struct wireless_dev {
> struct wiphy *wiphy;
> +
> + /* private to the generic wireless code */
> + struct cfg80211_config pending_config;
> };
>
> /**
> --- wireless-dev.orig/include/net/cfg80211.h 2007-02-15 13:23:41.967940064 +0100
> +++ wireless-dev/include/net/cfg80211.h 2007-02-15 13:23:43.707940064 +0100
> @@ -11,6 +11,8 @@
> * Copyright 2006 Johannes Berg <[email protected]>
> */
>
> +#define SSID_MAX_LEN 32
> +
This has _got_ to be defined somewhere else in the stack? Can we use
that define, or is it being moved to here? Seems a shame to have it
more than one place.
On a side note, many many drivers need something like this, and many
many drivers redefine it internally. Is there some header that all
drivers (even fullmac like airo, orinoco, atmel, and libertas) can get
this from?
> /**
> * struct cfg80211_config - description of a configuration (request)
> */
> @@ -19,7 +21,7 @@ struct cfg80211_config {
> u32 valid;
>
> s8 ssid_len;
> - u8 *ssid;
> + u8 ssid[SSID_MAX_LEN];
>
> u16 network_id;
> s32 rx_sensitivity;
> --- wireless-dev.orig/net/wireless/core.h 2007-02-15 13:23:42.037940064 +0100
> +++ wireless-dev/net/wireless/core.h 2007-02-15 13:23:43.707940064 +0100
> @@ -24,11 +24,6 @@ struct cfg80211_registered_device {
> /* wiphy index, internal only */
> int idx;
>
> -#ifdef CONFIG_CFG80211_WEXT_COMPAT
> - /* wext compat */
> - struct cfg80211_config *wext_pending_config;
> -#endif
> -
> /* must be last because of the way we do wiphy_priv(),
> * and it should at least be aligned to NETDEV_ALIGN */
> struct wiphy wiphy __attribute__((__aligned__(NETDEV_ALIGN)));
> --- wireless-dev.orig/net/wireless/nl80211.c 2007-02-15 13:23:42.137940064 +0100
> +++ wireless-dev/net/wireless/nl80211.c 2007-02-15 13:23:43.717940064 +0100
> @@ -389,8 +389,8 @@ static int nl80211_configure(struct sk_b
>
> attr = info->attrs[NL80211_ATTR_SSID];
> if (attr) {
> - config.ssid = nla_data(attr);
> config.ssid_len = nla_len(attr);
> + memcpy(config.ssid, nla_data(attr), config.ssid_len);
> }
>
> attr = info->attrs[NL80211_ATTR_NETWORK_ID];
> --- wireless-dev.orig/net/wireless/wext-compat.c 2007-02-15 13:23:42.207940064 +0100
> +++ wireless-dev/net/wireless/wext-compat.c 2007-02-15 13:23:43.717940064 +0100
> @@ -10,26 +10,16 @@
> *
> * Theory of operation, so to speak:
> *
> - * To implement compatibility, I added a new field to struct net_device
> - * that contains the pending configuration structure. This is dynamically
> - * allocated when needed and freed when committed.
> - *
> * Commit is done some time after the last parameter was changed
> * (with each parameter change simply (re-)schedule a timer) or
> * if explicitly asked for. This is probably not what most people
> * would expect, but perfectly fine in the WE API.
> *
> - * NB: we leak memory if the user
> - * - changes some settings
> - * - quickly rmmod's the module so that the net device is destroyed
> - * Since only root can do it and I don't see a way to hook into
> - * the net device's destruction... tough.
> - *
> - * NB2: Note that each of the wrappers should check if the cfg80211
> + * NB: Note that each of the wrappers should check if the cfg80211
> * user provides the command, and for configure() it must also check
> * if that parameter can be set or not via get_config_valid()
> *
> - * NB3: It's really bad that we can't report an error from the timer-
> + * NB2: It's really bad that we can't report an error from the timer-
> * based commit... Hopefully get_config_valid() can catch everything?
Well, we'd have two choices; first, a netlink notification that the
config was unsuccessful, or second a WEXT SIOCSIWAP event of
00:00:00:xxx for WE compat. Why wouldn't one of those work?
Dan
> *
> * see set_essid for an example
> @@ -67,17 +57,9 @@ static void cfg80211_wx_start_commit_tim
> * as well as taking the rtnl lock (due to wext)! */
> }
>
> -static struct cfg80211_config *cfg80211_ensure_pending_cfg(
> - struct cfg80211_registered_device *drv)
> +static struct cfg80211_config *get_pending_cfg(struct net_device *dev)
> {
> - struct cfg80211_config *cfg = drv->wext_pending_config;
> - if (!cfg)
> - cfg = kmalloc(sizeof(*cfg)+32, GFP_KERNEL);
> - if (cfg) {
> - cfg->ssid = (char*)cfg + sizeof(*cfg);
> - drv->wext_pending_config = cfg;
> - }
> - return cfg;
> + return &dev->ieee80211_ptr->pending_config;
> }
>
> static int cfg80211_wx_set_commit(struct cfg80211_registered_device *drv,
> @@ -86,20 +68,8 @@ static int cfg80211_wx_set_commit(struct
> union iwreq_data *data,
> char *extra)
> {
> - int err;
> -
> - if (!drv->wext_pending_config) {
> - err = 0;
> - goto out;
> - }
> -
> - err = drv->ops->configure(&drv->wiphy, net_dev,
> - drv->wext_pending_config);
> -
> - kfree(drv->wext_pending_config);
> - drv->wext_pending_config = NULL;
> + int err = -EOPNOTSUPP;
>
> - out:
> return err;
> }
>
> @@ -317,11 +287,7 @@ static int cfg80211_wx_set_essid(struct
> & CFG80211_CFG_VALID_SSID))
> goto out;
>
> - cfg = cfg80211_ensure_pending_cfg(drv);
> - if (!cfg) {
> - err = -ENOMEM;
> - goto out;
> - }
> + cfg = get_pending_cfg(net_dev);
>
> memcpy(cfg->ssid, extra, data->essid.length);
> cfg->ssid_len = data->essid.length;
>
> --
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2007-02-16 at 14:10 -0500, Dan Williams wrote:
> This has _got_ to be defined somewhere else in the stack? Can we use
> that define, or is it being moved to here? Seems a shame to have it
> more than one place.
It probably is...
> On a side note, many many drivers need something like this, and many
> many drivers redefine it internally. Is there some header that all
> drivers (even fullmac like airo, orinoco, atmel, and libertas) can get
> this from?
Yeah, we should have some sort of stack/driver agnosting IEEE 802.11
defines header w/o functions in it. Then I'd use that here too.
> > - * NB3: It's really bad that we can't report an error from the timer-
> > + * NB2: It's really bad that we can't report an error from the timer-
> > * based commit... Hopefully get_config_valid() can catch everything?
>
> Well, we'd have two choices; first, a netlink notification that the
> config was unsuccessful, or second a WEXT SIOCSIWAP event of
> 00:00:00:xxx for WE compat. Why wouldn't one of those work?
Yes, we can give a notification that it was unsuccessful. For wext, we
don't actually need all this I think because we do it with commit
anyway.
johannes