regd: sleeping in atomic

PATCH Superseded Jiri Slaby 5 participants 15 messages Mar 16, 2010 – Mar 18, 2010 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

Jiri Slaby

author

2010-03-16 13:26 UTC #0 lore raw

Hi,

Stanse found an atomic error in reg_copy_regd:

static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd,
const struct ieee80211_regdomain *src_regd)

{
        struct ieee80211_regdomain *regd;
        int size_of_regd = 0;
        unsigned int i;

        size_of_regd = sizeof(struct ieee80211_regdomain) +
          ((src_regd->n_reg_rules + 1) * sizeof(struct

ieee80211_reg_rule));

regd = kzalloc(size_of_regd, GFP_KERNEL); <---- here

Called from:

static void reg_regdb_search(struct work_struct *work)

{
        spin_lock(&reg_regdb_search_lock);
        while (!list_empty(&reg_regdb_search_list)) {
...
                for (i=0; i<reg_regdb_size; i++) {
                        curdom = reg_regdb[i];

                        if (!memcmp(request->alpha2, curdom->alpha2, 2)) {
                                r = reg_copy_regd(&regdom, curdom);
...
        spin_unlock(&reg_regdb_search_lock);
}

Whole error temporarily available at:
http://decibel.fi.muni.cz/~xslaby/stanse/error.cgi?db=34-rc&id=578

It is introduced by 3b377ea9d4efc94dc52fe41b4dfdb463635ab298.

Do you plan to extend it somehow or may the spinlock be converted to mutex? If not how much may size_of_regd be -- can we safely switch to GFP_ATOMIC?

Signature · 1 line

js

Luis R. Rodriguez

2010-03-16 16:18 UTC #1 lore raw

2010/3/16 Jiri Slaby <[email protected]>:

Quoted 32 lines

Hi,
Stanse found an atomic error in reg_copy_regd:
static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd, const struct ieee80211_regdomain *src_regd) {
struct ieee80211_regdomain *regd;
int size_of_regd = 0;
unsigned int i;
size_of_regd = sizeof(struct ieee80211_regdomain) + ((src_regd->n_reg_rules + 1) * sizeof(struct ieee80211_reg_rule));
regd = kzalloc(size_of_regd, GFP_KERNEL); <---- here
Called from:
static void reg_regdb_search(struct work_struct *work) {
spin_lock(&reg_regdb_search_lock); while (!list_empty(&reg_regdb_search_list)) {
...
for (i=0; i<reg_regdb_size; i++) { curdom = reg_regdb[i];
if (!memcmp(request->alpha2, curdom->alpha2, 2)) { r = reg_copy_regd(&regdom, curdom);
...
spin_unlock(&reg_regdb_search_lock);
}

Quoted 6 lines

Whole error temporarily available at:
http://decibel.fi.muni.cz/~xslaby/stanse/error.cgi?db=34-rc&id=578
It is introduced by 3b377ea9d4efc94dc52fe41b4dfdb463635ab298.
Do you plan to extend it somehow or may the spinlock be converted to mutex?

I don't think you can convert this directly to a mutex. The spin_lock in question (&reg_regdb_search_lock) gets also used by reg_regdb_query() which in turn gets called by call_crda(). There is one iteration of call_crda() which happens during module initialization and from what I gather I don't think the kernel is happy when you mutex_lock on load routines, please correct my foggy memory if I am mistaken. So during module load we directly end up hitting the spin_lock in question, prior to the workqueue using it.

Not sure yet how to address this, any ideas John?

Quoted 1 line

If not how much may size_of_regd be -- can we safely switch to GFP_ATOMIC?

The count is up to 117 right now. It should be around the number of countries of the world today, tomorrow even different countries could have different regulatory domains. This happened with Japan a while back when they required manufacturers to apply certification rules depending on the year the product went out.. and this changed many times in that country. But another reason why this number may also increase in any given country without regards to silly legislation is for experimenters who want to enhance the use of the spectrum in certain areas and would use sha1sums instead of alpha2s for the regdomains. But we're light years away from that happening.

Luis

Jiri Slaby

author

2010-03-16 18:42 UTC #2 lore raw

Quoted 4 lines · Luis R. Rodriguez

one iteration of call_crda() which happens during module initialization and from what I gather I don't think the kernel is happy when you mutex_lock on load routines, please correct my foggy memory if I am mistaken.

No, using mutex in init/exit module routines is fine. Where do you have the information from? There might be problems only with request_module, flush_scheduled_work or similar.

Luis R. Rodriguez

2010-03-16 20:37 UTC #3 lore raw

Quoted 9 lines · Jiri Slaby

Quoted 5 lines · Luis R. Rodriguez
one iteration of call_crda() which happens during module initialization and from what I gather I don't think the kernel is happy when you mutex_lock on load routines, please correct my foggy memory if I am mistaken.
No, using mutex in init/exit module routines is fine. Where do you have the information from?

A snag I hit when trying this a while ago on reg.c. I suppose the real issue could have been something else.

Quoted 2 lines

There might be problems only with request_module,
flush_scheduled_work or similar.

OK -- then in that case I think using a mutex would work here, unless John spots any other issues.

Luis

John W. Linville

2010-03-16 20:00 UTC #4 lore raw

PATCH 1/1 wireless: convert reg_regdb_search_lock to mutex

In reply to Jiri Slaby

Stanse discovered that kmalloc can be called with GFP_KERNEL while holding this spinlock. It can be a mutex instead.

Reported-by Jiri Slaby <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
Hmmm...I think I can get rid of the unlock of reg_regdb_search_mutex before calling set_regdom now as well?

 net/wireless/reg.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

net/wireless/reg.c +5 -5

index ed89c59..5f623ed 100644
@@ -324,7 +324,7 @@ struct reg_regdb_search_request {
 };
 
 static LIST_HEAD(reg_regdb_search_list);
-static DEFINE_SPINLOCK(reg_regdb_search_lock);
+static DEFINE_MUTEX(reg_regdb_search_mutex);
 
 static void reg_regdb_search(struct work_struct *work)
 {

@@ -332,7 +332,7 @@ static void reg_regdb_search(struct work_struct *work)
 	const struct ieee80211_regdomain *curdom, *regdom;
 	int i, r;
 
-	spin_lock(&reg_regdb_search_lock);
+	mutex_lock(&reg_regdb_search_mutex);
 	while (!list_empty(&reg_regdb_search_list)) {
 		request = list_first_entry(&reg_regdb_search_list,
 					   struct reg_regdb_search_request,

@@ -346,18 +346,18 @@ static void reg_regdb_search(struct work_struct *work)
 				r = reg_copy_regd(&regdom, curdom);
 				if (r)
 					break;
-				spin_unlock(&reg_regdb_search_lock);
+				mutex_unlock(&reg_regdb_search_mutex);
 				mutex_lock(&cfg80211_mutex);
 				set_regdom(regdom);
 				mutex_unlock(&cfg80211_mutex);
-				spin_lock(&reg_regdb_search_lock);
+				mutex_lock(&reg_regdb_search_mutex);
 				break;
 			}
 		}
 
 		kfree(request);
 	}
-	spin_unlock(&reg_regdb_search_lock);
+	mutex_unlock(&reg_regdb_search_mutex);
 }
 
 static DECLARE_WORK(reg_regdb_work, reg_regdb_search);

Signature · 1 line

1.6.2.5

Senthil Balasubramanian

2010-03-16 22:34 UTC #5 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 1 line · John W. Linville

Stanse discovered that kmalloc can be called with GFP_KERNEL while

This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.

Quoted 62 lines

holding this spinlock. It can be a mutex instead.

Reported-by Jiri Slaby <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
Hmmm...I think I can get rid of the unlock of reg_regdb_search_mutex before calling set_regdom now as well?

 net/wireless/reg.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

net/wireless/reg.c +5 -5

index ed89c59..5f623ed 100644
@@ -324,7 +324,7 @@ struct reg_regdb_search_request {
 };
 
 static LIST_HEAD(reg_regdb_search_list);
-static DEFINE_SPINLOCK(reg_regdb_search_lock);
+static DEFINE_MUTEX(reg_regdb_search_mutex);
 
 static void reg_regdb_search(struct work_struct *work)
 {

@@ -332,7 +332,7 @@ static void reg_regdb_search(struct work_struct *work)
 	const struct ieee80211_regdomain *curdom, *regdom;
 	int i, r;
 
-	spin_lock(&reg_regdb_search_lock);
+	mutex_lock(&reg_regdb_search_mutex);
 	while (!list_empty(&reg_regdb_search_list)) {
 		request = list_first_entry(&reg_regdb_search_list,
 					   struct reg_regdb_search_request,

@@ -346,18 +346,18 @@ static void reg_regdb_search(struct work_struct *work)
 				r = reg_copy_regd(&regdom, curdom);
 				if (r)
 					break;
-				spin_unlock(&reg_regdb_search_lock);
+				mutex_unlock(&reg_regdb_search_mutex);
 				mutex_lock(&cfg80211_mutex);
 				set_regdom(regdom);
 				mutex_unlock(&cfg80211_mutex);
-				spin_lock(&reg_regdb_search_lock);
+				mutex_lock(&reg_regdb_search_mutex);
 				break;
 			}
 		}
 
 		kfree(request);
 	}
-	spin_unlock(&reg_regdb_search_lock);
+	mutex_unlock(&reg_regdb_search_mutex);
 }
 
 static DECLARE_WORK(reg_regdb_work, reg_regdb_search);

Signature · 6 lines

1.6.2.5

--
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

John W. Linville

2010-03-17 00:26 UTC #6 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 6 lines · Senthil Balasubramanian

Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.
Quoted 1 line
holding this spinlock. It can be a mutex instead.

Not half so confusing as your criticism... :-)

Are you objecting to "can be" instead of "was"?

John

Signature · 2 lines

John W. Linville                Someday the world will need a hero, and you
[email protected]                  might be all we have.  Be ready.

Senthil Balasubramanian

2010-03-17 00:56 UTC #7 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 9 lines · John W. Linville

Quoted 6 lines · Senthil Balasubramanian
Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.
Quoted 1 line
holding this spinlock. It can be a mutex instead.
Not half so confusing as your criticism... :-)

sorry! if my mail wasn't proper. I didn't meant to blame/criticize. I was confused when I read the commit log and so I replied.

Quoted 2 lines

Are you objecting to "can be" instead of "was"?

I am not objecting anything...

Quoted 5 lines

John

Signature · 2 lines

John W. Linville                Someday the world will need a hero, and you
[email protected]                  might be all we have.  Be ready.

John W. Linville

2010-03-17 03:15 UTC #8 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 7 lines · Senthil Balasubramanian

Quoted 6 lines · John W. Linville
Quoted 5 lines · Senthil Balasubramanian
Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
holding this spinlock. It can be a mutex instead.
Not half so confusing as your criticism... :-)
sorry! if my mail wasn't proper. I didn't meant to blame/criticize. I was confused when I read the commit log and so I replied.
Quoted 2 lines
Are you objecting to "can be" instead of "was"?
I am not objecting anything...

Criticism is fine -- I just don't understand what you were saying or what you think I should have said in the commit log.

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Johannes Berg

maintainer

2010-03-17 03:24 UTC #9 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 11 lines · John W. Linville

Quoted 10 lines · Senthil Balasubramanian
Quoted 9 lines · John W. Linville
Quoted 6 lines · Senthil Balasubramanian
Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.
Quoted 1 line
holding this spinlock. It can be a mutex instead.
Not half so confusing as your criticism... :-)

Quoted 8 lines

Quoted 5 lines
sorry! if my mail wasn't proper. I didn't meant to blame/criticize. I was confused when I read the commit log and so I replied.
Quoted 2 lines
Are you objecting to "can be" instead of "was"?
I am not objecting anything...
Criticism is fine -- I just don't understand what you were saying or what you think I should have said in the commit log.

English at work ... let me guess:

For John: "can" == "it could happen that"
For Senthil: "can" == "may"

(when really you may NOT do GFP_KERNEL allocations in atomic context)

johannes

John W. Linville

2010-03-17 13:00 UTC #10 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 21 lines · Johannes Berg

Quoted 11 lines · John W. Linville
Quoted 10 lines · Senthil Balasubramanian
Quoted 9 lines · John W. Linville
Quoted 6 lines · Senthil Balasubramanian
Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.
Quoted 1 line
holding this spinlock. It can be a mutex instead.
Not half so confusing as your criticism... :-)
Quoted 8 lines
Quoted 5 lines
sorry! if my mail wasn't proper. I didn't meant to blame/criticize. I was confused when I read the commit log and so I replied.
Quoted 2 lines
Are you objecting to "can be" instead of "was"?
I am not objecting anything...
Criticism is fine -- I just don't understand what you were saying or what you think I should have said in the commit log.

Quoted 6 lines

English at work ... let me guess:
For John: "can" == "it could happen that"
For Senthil: "can" == "may"
(when really you may NOT do GFP_KERNEL allocations in atomic context)

Ah, that seems like a plausible explanation for the confusion. Bitte mein herr!

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

John W. Linville

2010-03-17 13:02 UTC #11 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

Quoted 11 lines · John W. Linville

Quoted 6 lines · Johannes Berg
English at work ... let me guess:
For John: "can" == "it could happen that"
For Senthil: "can" == "may"
(when really you may NOT do GFP_KERNEL allocations in atomic context)
Ah, that seems like a plausible explanation for the confusion. Bitte mein herr!

Err...danke mein herr! :-)

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Senthil Balasubramanian

2010-03-17 13:57 UTC #12 lore raw

Re: [PATCH] wireless: convert reg_regdb_search_lock to mutex

In reply to Johannes Berg

Quoted 15 lines · Johannes Berg

Quoted 14 lines · John W. Linville
Quoted 13 lines · Senthil Balasubramanian
Quoted 9 lines · John W. Linville
Quoted 6 lines · Senthil Balasubramanian
Quoted 1 line · John W. Linville
Stanse discovered that kmalloc can be called with GFP_KERNEL while
This commit log is confusing. It Should be "Stanse discovered kmalloc was called with GFP_KERNEL". Obviously kmalloc with GFP_KERNEL shouldn't be used while holding a spinlock.
Quoted 1 line
holding this spinlock. It can be a mutex instead.
Not half so confusing as your criticism... :-)
sorry! if my mail wasn't proper. I didn't meant to blame/criticize. I was confused when I read the commit log and so I replied.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Quoted 1 line
Are you objecting to "can be" instead of "was"?
I am not objecting anything...
Criticism is fine -- I just don't understand what you were saying or what you think I should have said in the commit log.
English at work ... let me guess:

Yes. I basically misread the commit log and understood like GFP_KENREL is allowed while holding a spinlock. So I replied.

Sorry! for the unnecessary noise.. here... Please ignore my comment.

Quoted 8 lines

For John: "can" == "it could happen that"
For Senthil: "can" == "may"
(when really you may NOT do GFP_KERNEL allocations in atomic context)
johannes

John W. Linville

2010-03-17 19:45 UTC #13 lore raw

PATCH v2 1/1 wireless: convert reg_regdb_search_lock to mutex

In reply to John W. Linville

Stanse discovered that kmalloc is being called with GFP_KERNEL while holding this spinlock. The spinlock can be a mutex instead, which also enables the removal of the unlock/lock around the lock/unlock of cfg80211_mutex and the call to set_regdom.

Reported-by Jiri Slaby <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
 net/wireless/reg.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

net/wireless/reg.c +3 -5

index ed89c59..1ef1639 100644
@@ -324,7 +324,7 @@ struct reg_regdb_search_request {
 };
 
 static LIST_HEAD(reg_regdb_search_list);
-static DEFINE_SPINLOCK(reg_regdb_search_lock);
+static DEFINE_MUTEX(reg_regdb_search_mutex);
 
 static void reg_regdb_search(struct work_struct *work)
 {

@@ -332,7 +332,7 @@ static void reg_regdb_search(struct work_struct *work)
 	const struct ieee80211_regdomain *curdom, *regdom;
 	int i, r;
 
-	spin_lock(&reg_regdb_search_lock);
+	mutex_lock(&reg_regdb_search_mutex);
 	while (!list_empty(&reg_regdb_search_list)) {
 		request = list_first_entry(&reg_regdb_search_list,
 					   struct reg_regdb_search_request,

@@ -346,18 +346,16 @@ static void reg_regdb_search(struct work_struct *work)
 				r = reg_copy_regd(&regdom, curdom);
 				if (r)
 					break;
-				spin_unlock(&reg_regdb_search_lock);
 				mutex_lock(&cfg80211_mutex);
 				set_regdom(regdom);
 				mutex_unlock(&cfg80211_mutex);
-				spin_lock(&reg_regdb_search_lock);
 				break;
 			}
 		}
 
 		kfree(request);
 	}
-	spin_unlock(&reg_regdb_search_lock);
+	mutex_unlock(&reg_regdb_search_mutex);
 }
 
 static DECLARE_WORK(reg_regdb_work, reg_regdb_search);

Signature · 1 line

1.6.2.5

John W. Linville

2010-03-18 21:00 UTC #14 lore raw

PATCH v3 1/1 wireless: convert reg_regdb_search_lock to mutex

Reported-by Jiri Slaby <[email protected]>

Signed-off-by John W. Linville <[email protected]>

---
Missed a hunk...

 net/wireless/reg.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

net/wireless/reg.c +5 -7

index ed89c59..81fcafc 100644
@@ -324,7 +324,7 @@ struct reg_regdb_search_request {
 };
 
 static LIST_HEAD(reg_regdb_search_list);
-static DEFINE_SPINLOCK(reg_regdb_search_lock);
+static DEFINE_MUTEX(reg_regdb_search_mutex);
 
 static void reg_regdb_search(struct work_struct *work)
 {

@@ -332,7 +332,7 @@ static void reg_regdb_search(struct work_struct *work)
 	const struct ieee80211_regdomain *curdom, *regdom;
 	int i, r;
 
-	spin_lock(&reg_regdb_search_lock);
+	mutex_lock(&reg_regdb_search_mutex);
 	while (!list_empty(&reg_regdb_search_list)) {
 		request = list_first_entry(&reg_regdb_search_list,
 					   struct reg_regdb_search_request,

@@ -346,18 +346,16 @@ static void reg_regdb_search(struct work_struct *work)
 				r = reg_copy_regd(&regdom, curdom);
 				if (r)
 					break;
-				spin_unlock(&reg_regdb_search_lock);
 				mutex_lock(&cfg80211_mutex);
 				set_regdom(regdom);
 				mutex_unlock(&cfg80211_mutex);
-				spin_lock(&reg_regdb_search_lock);
 				break;
 			}
 		}
 
 		kfree(request);
 	}
-	spin_unlock(&reg_regdb_search_lock);
+	mutex_unlock(&reg_regdb_search_mutex);
 }
 
 static DECLARE_WORK(reg_regdb_work, reg_regdb_search);

@@ -375,9 +373,9 @@ static void reg_regdb_query(const char *alpha2)
 
 	memcpy(request->alpha2, alpha2, 2);
 
-	spin_lock(&reg_regdb_search_lock);
+	mutex_lock(&reg_regdb_search_mutex);
 	list_add_tail(&request->list, &reg_regdb_search_list);
-	spin_unlock(&reg_regdb_search_lock);
+	mutex_unlock(&reg_regdb_search_mutex);
 
 	schedule_work(&reg_regdb_work);
 }

Signature · 1 line

1.6.2.5