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(®_regdb_search_lock);
while (!list_empty(®_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(®dom, curdom);
...
spin_unlock(®_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?
--
js
On Wed, Mar 17, 2010 at 05:54:16AM +0530, John W. Linville wrote:
> On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> > On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > > 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.
> > > 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.
>
> Are you objecting to "can be" instead of "was"?
I am not objecting anything...
>
> John
> --
> John W. Linville ? ? ? ? ? ? ? ?Someday the world will need a hero, and you
> [email protected] ? ? ? ? ? ? ? ? ?might be all we have. ?Be ready.
On Tue, 2010-03-16 at 22:56 -0400, John W. Linville wrote:
> On Tue, Mar 16, 2010 at 05:54:50PM +0530, Senthil Balasubramanian wrote:
> > On Wed, Mar 17, 2010 at 05:54:16AM +0530, John W. Linville wrote:
> > > On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> > > > On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > > > > 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.
> > > > > 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.
> > >
> > > 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
2010/3/16 Jiri Slaby <[email protected]>:
> 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(®_regdb_search_lock);
> while (!list_empty(®_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(®dom, curdom);
> ...
> spin_unlock(®_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?
I don't think you can convert this directly to a mutex. The spin_lock
in question (®_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?
> 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
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(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index ed89c59..1ef1639 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -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(®_regdb_search_lock);
+ mutex_lock(®_regdb_search_mutex);
while (!list_empty(®_regdb_search_list)) {
request = list_first_entry(®_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(®dom, curdom);
if (r)
break;
- spin_unlock(®_regdb_search_lock);
mutex_lock(&cfg80211_mutex);
set_regdom(regdom);
mutex_unlock(&cfg80211_mutex);
- spin_lock(®_regdb_search_lock);
break;
}
}
kfree(request);
}
- spin_unlock(®_regdb_search_lock);
+ mutex_unlock(®_regdb_search_mutex);
}
static DECLARE_WORK(reg_regdb_work, reg_regdb_search);
--
1.6.2.5
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(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index ed89c59..5f623ed 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -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(®_regdb_search_lock);
+ mutex_lock(®_regdb_search_mutex);
while (!list_empty(®_regdb_search_list)) {
request = list_first_entry(®_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(®dom, curdom);
if (r)
break;
- spin_unlock(®_regdb_search_lock);
+ mutex_unlock(®_regdb_search_mutex);
mutex_lock(&cfg80211_mutex);
set_regdom(regdom);
mutex_unlock(&cfg80211_mutex);
- spin_lock(®_regdb_search_lock);
+ mutex_lock(®_regdb_search_mutex);
break;
}
}
kfree(request);
}
- spin_unlock(®_regdb_search_lock);
+ mutex_unlock(®_regdb_search_mutex);
}
static DECLARE_WORK(reg_regdb_work, reg_regdb_search);
--
1.6.2.5
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]>
---
Missed a hunk...
net/wireless/reg.c | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index ed89c59..81fcafc 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -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(®_regdb_search_lock);
+ mutex_lock(®_regdb_search_mutex);
while (!list_empty(®_regdb_search_list)) {
request = list_first_entry(®_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(®dom, curdom);
if (r)
break;
- spin_unlock(®_regdb_search_lock);
mutex_lock(&cfg80211_mutex);
set_regdom(regdom);
mutex_unlock(&cfg80211_mutex);
- spin_lock(®_regdb_search_lock);
break;
}
}
kfree(request);
}
- spin_unlock(®_regdb_search_lock);
+ mutex_unlock(®_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(®_regdb_search_lock);
+ mutex_lock(®_regdb_search_mutex);
list_add_tail(&request->list, ®_regdb_search_list);
- spin_unlock(®_regdb_search_lock);
+ mutex_unlock(®_regdb_search_mutex);
schedule_work(®_regdb_work);
}
--
1.6.2.5
On Tue, Mar 16, 2010 at 05:54:50PM +0530, Senthil Balasubramanian wrote:
> On Wed, Mar 17, 2010 at 05:54:16AM +0530, John W. Linville wrote:
> > On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> > > On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > > > 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.
> > > > 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.
> >
> > 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
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > 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.
> > 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
--
John W. Linville ? ? ? ? ? ? ? ?Someday the world will need a hero, and you
[email protected] ? ? ? ? ? ? ? ? ?might be all we have. ?Be ready.
On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> 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.
> 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(-)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index ed89c59..5f623ed 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -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(®_regdb_search_lock);
> + mutex_lock(®_regdb_search_mutex);
> while (!list_empty(®_regdb_search_list)) {
> request = list_first_entry(®_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(®dom, curdom);
> if (r)
> break;
> - spin_unlock(®_regdb_search_lock);
> + mutex_unlock(®_regdb_search_mutex);
> mutex_lock(&cfg80211_mutex);
> set_regdom(regdom);
> mutex_unlock(&cfg80211_mutex);
> - spin_lock(®_regdb_search_lock);
> + mutex_lock(®_regdb_search_mutex);
> break;
> }
> }
>
> kfree(request);
> }
> - spin_unlock(®_regdb_search_lock);
> + mutex_unlock(®_regdb_search_mutex);
> }
>
> static DECLARE_WORK(reg_regdb_work, reg_regdb_search);
> --
> 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
On Tue, Mar 16, 2010 at 11:42 AM, Jiri Slaby <[email protected]> wrote:
> On 03/16/2010 05:18 PM, Luis R. Rodriguez wrote:
>>
>> 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.
> 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
On Tue, Mar 16, 2010 at 08:24:35PM -0700, Johannes Berg wrote:
> On Tue, 2010-03-16 at 22:56 -0400, John W. Linville wrote:
> > On Tue, Mar 16, 2010 at 05:54:50PM +0530, Senthil Balasubramanian wrote:
> > > On Wed, Mar 17, 2010 at 05:54:16AM +0530, John W. Linville wrote:
> > > > On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> > > > > On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > > > > > 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.
> > > > > > 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.
> > > >
> > > > 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)
Ah, that seems like a plausible explanation for the confusion.
Bitte mein herr!
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Wed, Mar 17, 2010 at 08:55:13AM -0400, John W. Linville wrote:
> On Tue, Mar 16, 2010 at 08:24:35PM -0700, Johannes Berg wrote:
> > 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
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Wed, Mar 17, 2010 at 08:54:35AM +0530, Johannes Berg wrote:
> On Tue, 2010-03-16 at 22:56 -0400, John W. Linville wrote:
> > On Tue, Mar 16, 2010 at 05:54:50PM +0530, Senthil Balasubramanian wrote:
> > > On Wed, Mar 17, 2010 at 05:54:16AM +0530, John W. Linville wrote:
> > > > On Tue, Mar 16, 2010 at 03:33:38PM +0530, Senthil Balasubramanian wrote:
> > > > > On Wed, Mar 17, 2010 at 01:19:00AM +0530, John W. Linville wrote:
> > > > > > 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.
> > > > > > 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.
> > > >
> > > > 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.
>
> For John: "can" == "it could happen that"
> For Senthil: "can" == "may"
>
> (when really you may NOT do GFP_KERNEL allocations in atomic context)
>
> johannes
>
On 03/16/2010 05:18 PM, Luis R. Rodriguez wrote:
> 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.