[PATCH] wireless: fix regdom passing semantics

[PATCH] wireless: fix regdom passing semantics

[Johannes Berg]

The regdom struct is given to the core, so it might as well
free it in error conditions, the current documentation doesn't
note that you need to free it on errors anyway.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/nl80211.c |    5 +----
 net/wireless/reg.c     |    9 +++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

--- everything.orig/net/wireless/nl80211.c	2008-10-21 10:36:13.000000000 +0200
+++ everything/net/wireless/nl80211.c	2008-10-21 10:41:13.000000000 +0200
@@ -1756,12 +1756,9 @@ static int nl80211_set_reg(struct sk_buf
 	mutex_lock(&cfg80211_drv_mutex);
 	r = set_regdom(rd);
 	mutex_unlock(&cfg80211_drv_mutex);
-	if (r)
-		goto bad_reg;
-
 	return r;
 
-bad_reg:
+ bad_reg:
 	kfree(rd);
 	return -EINVAL;
 }
--- everything.orig/net/wireless/reg.c	2008-10-21 10:38:59.000000000 +0200
+++ everything/net/wireless/reg.c	2008-10-21 10:41:13.000000000 +0200
@@ -606,7 +606,6 @@ int __regulatory_hint(struct wiphy *wiph
 	return r;
 }
 
-/* If rd is not NULL and if this call fails the caller must free it */
 int regulatory_hint(struct wiphy *wiphy, const char *alpha2,
 	struct ieee80211_regdomain *rd)
 {
@@ -691,6 +690,7 @@ void print_regdomain_info(const struct i
 	print_rd_rules(rd);
 }
 
+/* Takes ownership of rd only if it doesn't fail */
 static int __set_regdom(const struct ieee80211_regdomain *rd)
 {
 	/* Some basic sanity checks first */
@@ -752,16 +752,17 @@ static int __set_regdom(const struct iee
 
 /* Use this call to set the current regulatory domain. Conflicts with
  * multiple drivers can be ironed out later. Caller must've already
- * kmalloc'd the rd structure. If this calls fails you should kfree()
- * the passed rd. Caller must hold cfg80211_drv_mutex */
+ * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */
 int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
 
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
-	if (r)
+	if (r) {
+		kfree(rd);
 		return r;
+	}
 
 	/* This would make this whole thing pointless */
 	BUG_ON(rd != cfg80211_regdomain);

Re: [PATCH] wireless: fix regdom passing semantics

[Luis R. Rodriguez]

On Tue, Oct 21, 2008 at 02:02:00AM -0700, Johannes Berg wrote:
> the current documentation doesn't
> note that you need to free it on errors anyway.

Yes it does.

>  /* Use this call to set the current regulatory domain. Conflicts with
>   * multiple drivers can be ironed out later. Caller must've already
> - * kmalloc'd the rd structure. If this calls fails you should kfree()
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
See?

> - * the passed rd. Caller must hold cfg80211_drv_mutex */
> + * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */

But this is fine too, so long as we keep the docs in sync.

  Luis

Re: [PATCH v2] wireless: fix regdom passing semantics

[Luis R. Rodriguez]

On Wed, Oct 22, 2008 at 11:49:54AM -0700, Johannes Berg wrote:
> The regdom struct is given to the core, so it might as well
> free it in error conditions

> the current documentation for
> regulatory_hint doesn't note that you need to free it on errors
> anyway.

This last part is not accurate, typo again?

  Luis

Re: [PATCH] wireless: fix regdom passing semantics

[Johannes Berg]

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

On Wed, 2008-10-22 at 04:37 -0700, Luis R. Rodriguez wrote:
> On Tue, Oct 21, 2008 at 02:02:00AM -0700, Johannes Berg wrote:
> > the current documentation doesn't
> > note that you need to free it on errors anyway.
> 
> Yes it does.
> 
> >  /* Use this call to set the current regulatory domain. Conflicts with
> >   * multiple drivers can be ironed out later. Caller must've already
> > - * kmalloc'd the rd structure. If this calls fails you should kfree()
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> See?
> 
> > - * the passed rd. Caller must hold cfg80211_drv_mutex */
> > + * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */
> 
> But this is fine too, so long as we keep the docs in sync.

Yeah, evidently I screwed up the commit message. Sorry. I think I'd
thought it didn't and then only later noticed. I'll resend.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] wireless: fix regdom passing semantics

[Johannes Berg]

The regdom struct is given to the core, so it might as well
free it in error conditions, the current documentation for
regulatory_hint doesn't note that you need to free it on errors
anyway.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/nl80211.c |    5 +----
 net/wireless/reg.c     |    9 +++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

--- everything.orig/net/wireless/nl80211.c	2008-10-21 10:36:13.000000000 +0200
+++ everything/net/wireless/nl80211.c	2008-10-21 10:41:13.000000000 +0200
@@ -1756,12 +1756,9 @@ static int nl80211_set_reg(struct sk_buf
 	mutex_lock(&cfg80211_drv_mutex);
 	r = set_regdom(rd);
 	mutex_unlock(&cfg80211_drv_mutex);
-	if (r)
-		goto bad_reg;
-
 	return r;
 
-bad_reg:
+ bad_reg:
 	kfree(rd);
 	return -EINVAL;
 }
--- everything.orig/net/wireless/reg.c	2008-10-21 10:38:59.000000000 +0200
+++ everything/net/wireless/reg.c	2008-10-21 10:41:13.000000000 +0200
@@ -606,7 +606,6 @@ int __regulatory_hint(struct wiphy *wiph
 	return r;
 }
 
-/* If rd is not NULL and if this call fails the caller must free it */
 int regulatory_hint(struct wiphy *wiphy, const char *alpha2,
 	struct ieee80211_regdomain *rd)
 {
@@ -691,6 +690,7 @@ void print_regdomain_info(const struct i
 	print_rd_rules(rd);
 }
 
+/* Takes ownership of rd only if it doesn't fail */
 static int __set_regdom(const struct ieee80211_regdomain *rd)
 {
 	/* Some basic sanity checks first */
@@ -752,16 +752,17 @@ static int __set_regdom(const struct iee
 
 /* Use this call to set the current regulatory domain. Conflicts with
  * multiple drivers can be ironed out later. Caller must've already
- * kmalloc'd the rd structure. If this calls fails you should kfree()
- * the passed rd. Caller must hold cfg80211_drv_mutex */
+ * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */
 int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
 
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
-	if (r)
+	if (r) {
+		kfree(rd);
 		return r;
+	}
 
 	/* This would make this whole thing pointless */
 	BUG_ON(rd != cfg80211_regdomain);

Re: [PATCH v2] wireless: fix regdom passing semantics

[Johannes Berg]

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

On Wed, 2008-10-22 at 05:36 -0700, Luis R. Rodriguez wrote:
> On Wed, Oct 22, 2008 at 11:49:54AM -0700, Johannes Berg wrote:
> > The regdom struct is given to the core, so it might as well
> > free it in error conditions
> 
> > the current documentation for
> > regulatory_hint doesn't note that you need to free it on errors
> > anyway.
> 
> This last part is not accurate, typo again?

Ahrg. No, thinko. I only saw

- * @rd: a complete regulatory domain provided by the driver. If passed
- *     the driver does not need to worry about freeing it.

not the fine print.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3] wireless: make regdom passing semantics simpler

[Johannes Berg]

The regdom struct is given to the core, so it might as well
free it in error conditions.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/nl80211.c |    5 +----
 net/wireless/reg.c     |    9 +++++----
 2 files changed, 6 insertions(+), 8 deletions(-)

--- everything.orig/net/wireless/nl80211.c	2008-10-21 10:36:13.000000000 +0200
+++ everything/net/wireless/nl80211.c	2008-10-21 10:41:13.000000000 +0200
@@ -1756,12 +1756,9 @@ static int nl80211_set_reg(struct sk_buf
 	mutex_lock(&cfg80211_drv_mutex);
 	r = set_regdom(rd);
 	mutex_unlock(&cfg80211_drv_mutex);
-	if (r)
-		goto bad_reg;
-
 	return r;
 
-bad_reg:
+ bad_reg:
 	kfree(rd);
 	return -EINVAL;
 }
--- everything.orig/net/wireless/reg.c	2008-10-21 10:38:59.000000000 +0200
+++ everything/net/wireless/reg.c	2008-10-21 10:41:13.000000000 +0200
@@ -606,7 +606,6 @@ int __regulatory_hint(struct wiphy *wiph
 	return r;
 }
 
-/* If rd is not NULL and if this call fails the caller must free it */
 int regulatory_hint(struct wiphy *wiphy, const char *alpha2,
 	struct ieee80211_regdomain *rd)
 {
@@ -691,6 +690,7 @@ void print_regdomain_info(const struct i
 	print_rd_rules(rd);
 }
 
+/* Takes ownership of rd only if it doesn't fail */
 static int __set_regdom(const struct ieee80211_regdomain *rd)
 {
 	/* Some basic sanity checks first */
@@ -752,16 +752,17 @@ static int __set_regdom(const struct iee
 
 /* Use this call to set the current regulatory domain. Conflicts with
  * multiple drivers can be ironed out later. Caller must've already
- * kmalloc'd the rd structure. If this calls fails you should kfree()
- * the passed rd. Caller must hold cfg80211_drv_mutex */
+ * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */
 int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
 
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
-	if (r)
+	if (r) {
+		kfree(rd);
 		return r;
+	}
 
 	/* This would make this whole thing pointless */
 	BUG_ON(rd != cfg80211_regdomain);