[PATCH] wireless: wl12xx, fix lock imbalance

[PATCH] wireless: wl12xx, fix lock imbalance

[Jiri Slaby]

Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
wl12xx_chip_wakeup fails).

Not sure if the device should be powered off?

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
 drivers/net/wireless/wl12xx/main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 603d611..d241e4a 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -336,7 +336,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
 
 	ret = wl12xx_chip_wakeup(wl);
 	if (ret < 0)
-		return ret;
+		goto unlock;
 
 	ret = wl->chip.op_boot(wl);
 	if (ret < 0)
@@ -357,7 +357,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
 out:
 	if (ret < 0)
 		wl12xx_power_off(wl);
-
+unlock:
 	mutex_unlock(&wl->mutex);
 
 	return ret;
-- 
1.6.3.2

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Johannes Berg]

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

On Mon, 2009-07-13 at 23:24 +0200, Jiri Slaby wrote:
> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
> wl12xx_chip_wakeup fails).

By the way, are you using some tool to find these? I've had local hacks
many times to make sparse aware of mutexes, is there a reason they are
not annotated with __acquire(s)/__release(s) like spinlocks etc.?

johannes

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

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Jiri Slaby]

On 07/13/2009 11:40 PM, Johannes Berg wrote:
> On Mon, 2009-07-13 at 23:24 +0200, Jiri Slaby wrote:
>> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
>> wl12xx_chip_wakeup fails).
> 
> By the way, are you using some tool to find these?

Yup, it's called stanse[1], but we still work on that to make it stable.

> I've had local hacks
> many times to make sparse aware of mutexes, is there a reason they are
> not annotated with __acquire(s)/__release(s) like spinlocks etc.?

Mutexes are often locked/unlocked interprocedural which I think sparse
can't do much about.

[1] http://iti.fi.muni.cz/stanse/

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Jiri Slaby]

On 07/13/2009 11:44 PM, Jiri Slaby wrote:
>> I've had local hacks
>> many times to make sparse aware of mutexes, is there a reason they are
>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> 
> Mutexes are often locked/unlocked interprocedural which I think sparse
> can't do much about.

(which means it has high false positive rate in those cases)

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Johannes Berg]

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

On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:

> > I've had local hacks
> > many times to make sparse aware of mutexes, is there a reason they are
> > not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> 
> Mutexes are often locked/unlocked interprocedural which I think sparse
> can't do much about.

Well, you annotate those functions too, of course.

johannes

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

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Jiri Slaby]

On 07/13/2009 11:49 PM, Johannes Berg wrote:
> On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> 
>>> I've had local hacks
>>> many times to make sparse aware of mutexes, is there a reason they are
>>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
>>
>> Mutexes are often locked/unlocked interprocedural which I think sparse
>> can't do much about.
> 
> Well, you annotate those functions too, of course.

Sorry, I don't understand. What functions I annotate?

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Johannes Berg]

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

On Mon, 2009-07-13 at 23:51 +0200, Jiri Slaby wrote:
> On 07/13/2009 11:49 PM, Johannes Berg wrote:
> > On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> > 
> >>> I've had local hacks
> >>> many times to make sparse aware of mutexes, is there a reason they are
> >>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> >>
> >> Mutexes are often locked/unlocked interprocedural which I think sparse
> >> can't do much about.
> > 
> > Well, you annotate those functions too, of course.
> 
> Sorry, I don't understand. What functions I annotate?

Well those that take the mutex, e.g.

void acquire_foo(struct foo *f)
{
	mutex_lock(&f->mtx);
}


turns to

void acquire_foo(struct foo *f)
	__acquires(f->mtx)
{
	mutex_lock(&f->mtx);
}

johannes

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

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Ingo Molnar]

* Johannes Berg <johannes@sipsolutions.net> wrote:

> On Mon, 2009-07-13 at 23:51 +0200, Jiri Slaby wrote:
> > On 07/13/2009 11:49 PM, Johannes Berg wrote:
> > > On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> > > 
> > >>> I've had local hacks
> > >>> many times to make sparse aware of mutexes, is there a reason they are
> > >>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> > >>
> > >> Mutexes are often locked/unlocked interprocedural which I think sparse
> > >> can't do much about.
> > > 
> > > Well, you annotate those functions too, of course.
> > 
> > Sorry, I don't understand. What functions I annotate?
> 
> Well those that take the mutex, e.g.
> 
> void acquire_foo(struct foo *f)
> {
> 	mutex_lock(&f->mtx);
> }
> 
> 
> turns to
> 
> void acquire_foo(struct foo *f)
> 	__acquires(f->mtx)
> {
> 	mutex_lock(&f->mtx);
> }
> 
> johannes

Yes. And in fact 'nice' code wants to be either annotated explicitly 
as 'I am taking locks', or should be balanced.

I was thinking about also using lockdep plus the function-graph 
tracer for that (in the dynamic lock debugging department).

It would work like this: __acquires()/__releases() would also emit 
section markers like __lockfunc, and lockdep would warn about 
functions that return with unbalanced locks, irqs or preempt counts 
and do not declare themselves as locking related functions.

This would help catch imbalances at their source.

Plus static tools like Jiri is working on are very useful as well. I 
think Coverty does that too and it's a pity we dont have free tools 
for that. In fact Covery will sweep clean the kernel of such bugs, 
giving OSS tools like 'stanse' the false impression that there are 
no such bugs. There are such bugs - there's a constant influx of 
them. So please work on this, it looks very useful.

	Ingo

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Johannes Berg]

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

On Sat, 2009-07-18 at 13:19 +0200, Ingo Molnar wrote:

> Yes. And in fact 'nice' code wants to be either annotated explicitly 
> as 'I am taking locks', or should be balanced.

I agree.

> I was thinking about also using lockdep plus the function-graph 
> tracer for that (in the dynamic lock debugging department).

Yeah, but that's dynamic again -- all the error paths are never caught.

> It would work like this: __acquires()/__releases() would also emit 
> section markers like __lockfunc, and lockdep would warn about 
> functions that return with unbalanced locks, irqs or preempt counts 
> and do not declare themselves as locking related functions.
> 
> This would help catch imbalances at their source.

I don't see a need to do it dynamically since sparse warns about things
like this. It's quirky in some ways and I've tried to fix it up before
(and failed) but it's not something that can't be fixed, it just needs
more than a night of hacking.

> Plus static tools like Jiri is working on are very useful as well. I 
> think Coverty does that too and it's a pity we dont have free tools 
> for that. In fact Covery will sweep clean the kernel of such bugs, 
> giving OSS tools like 'stanse' the false impression that there are 
> no such bugs. There are such bugs - there's a constant influx of 
> them. So please work on this, it looks very useful.

What's "this" in this context?

johannes

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

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Ingo Molnar]

* Johannes Berg <johannes@sipsolutions.net> wrote:

> > It would work like this: __acquires()/__releases() would also 
> > emit section markers like __lockfunc, and lockdep would warn 
> > about functions that return with unbalanced locks, irqs or 
> > preempt counts and do not declare themselves as locking related 
> > functions.
> > 
> > This would help catch imbalances at their source.
> 
> I don't see a need to do it dynamically since sparse warns about 
> things like this. It's quirky in some ways and I've tried to fix 
> it up before (and failed) but it's not something that can't be 
> fixed, it just needs more than a night of hacking.

Yeah - but Sparse warns about this if it can analyze the code path. 
If it cannot see through it then it cannot warn. Static analysis 
will go only that far - dynamic analysis will catch the cases that 
do happen.

So it's best to have both: static analysis is good at finding 
imbalances even if they have a very low likelyhood of occuring in 
practice, while dynamic analysis will catch everything that does 
trigger in practice, regardless of code flow complexity.

( The only white area on the map is rarely executed code that has a
  complex code flow. Such code is being frowned upon in general at 
  the review stage. )

> > Plus static tools like Jiri is working on are very useful as 
> > well. I think Coverty does that too and it's a pity we dont have 
> > free tools for that. In fact Covery will sweep clean the kernel 
> > of such bugs, giving OSS tools like 'stanse' the false 
> > impression that there are no such bugs. There are such bugs - 
> > there's a constant influx of them. So please work on this, it 
> > looks very useful.
> 
> What's "this" in this context?

this == stanse, the static code analyzing thing Jiri mentioned he is 
working on. The webpage says it will be under the GPL - that's good. 
Jiri, any release date for the source code?

	Ingo

stanse [was: wireless: wl12xx, fix lock imbalance]

[Jiri Slaby]

On 07/18/2009 06:10 PM, Ingo Molnar wrote:
> this == stanse, the static code analyzing thing Jiri mentioned he is 
> working on. The webpage says it will be under the GPL - that's good. 
> Jiri, any release date for the source code?

Sorry for the delay. We are currently rewriting the core: we added
interprocedural analysis and some false-positives killers. We also
merged thread-checker (it can catch (not only) circular lock deps like
in [1]). We are finishing, but it needs some testing, so I would say in
about a month.

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0904.1/03221.html

Stanse 1.0.0 released [was: wireless: wl12xx, fix lock imbalance]

[Jiri Slaby]

On 07/18/2009 06:10 PM, Ingo Molnar wrote:
>>> Plus static tools like Jiri is working on are very useful as 
>>> well. I think Coverty does that too and it's a pity we dont have 
>>> free tools for that. In fact Covery will sweep clean the kernel 
>>> of such bugs, giving OSS tools like 'stanse' the false 
>>> impression that there are no such bugs. There are such bugs - 
>>> there's a constant influx of them. So please work on this, it 
>>> looks very useful.
>>
>> What's "this" in this context?
> 
> this == stanse, the static code analyzing thing Jiri mentioned he is 
> working on. The webpage says it will be under the GPL - that's good. 
> Jiri, any release date for the source code?

Hi all. I'm pleased to announce the first official Stanse release.

For those who are interested, the tool (with other information and
documentation) is available at
http://stanse.fi.muni.cz/

There are also prebuilt rpm packages for openSUSE and Fedora 11. (Some
of them still building, will be available soon. Sorry for that, build
service is slow as hell these days.)

Also I pre-ran the stanse on 2.6.32-rc3 with results available online.
The webpage concerning this is at:
http://decibel.fi.muni.cz/~xslaby/stanse/?db=32-rc

So for those who are not interested in the toolkit itself but only in
results from more-or-less latest kernel may try it that way.

There are many janitor-like bugs. E.g. not testing return values from
k*alloc might be seamlessly taken care of by janitors, I think.

As it's only static analysis not based on symbolic execution (we will
play with that later), there are some/many (depending on a checker) many
false positives. There is a mechanism for marking them. I ask everybody
who finds a FP or error to mark the entry as such.

I'm still trying to lower FP rate. If somebody finds out another method
to do so, please share with us.

Any input appreciated. Hope it helps.

Thanks to those who support us,
--js

Re: Stanse 1.0.0 released [was: wireless: wl12xx, fix lock imbalance]

[Ingo Molnar]

* Jiri Slaby <jirislaby@gmail.com> wrote:

> On 07/18/2009 06:10 PM, Ingo Molnar wrote:
> >>> Plus static tools like Jiri is working on are very useful as 
> >>> well. I think Coverty does that too and it's a pity we dont have 
> >>> free tools for that. In fact Covery will sweep clean the kernel 
> >>> of such bugs, giving OSS tools like 'stanse' the false 
> >>> impression that there are no such bugs. There are such bugs - 
> >>> there's a constant influx of them. So please work on this, it 
> >>> looks very useful.
> >>
> >> What's "this" in this context?
> > 
> > this == stanse, the static code analyzing thing Jiri mentioned he is 
> > working on. The webpage says it will be under the GPL - that's good. 
> > Jiri, any release date for the source code?
> 
> Hi all. I'm pleased to announce the first official Stanse release.
> 
> For those who are interested, the tool (with other information and
> documentation) is available at
> http://stanse.fi.muni.cz/

the list of bugs found:

  http://stanse.fi.muni.cz/bugs.html

is already impressive!

	Ingo

Re: [PATCH] wireless: wl12xx, fix lock imbalance

[Luciano Coelho]

ext Jiri Slaby wrote:
> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
> wl12xx_chip_wakeup fails).
>   

Cool, very nice catch.  We actually just fixed this bug in our wl1271 
code (which I will hopefully send upstream this week), but we hadn't 
fixed it in the wl1251-specific code yet.

> Not sure if the device should be powered off?
>   

You should.  If the chip cannot be booted, why should it remain powered 
on? In some rare cases, the chip might fail to initialize, but can 
recover if powered off and on again, so turning it off at this point is 
the right thing to do.

> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> ---
>   
>  drivers/net/wireless/wl12xx/main.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 603d611..d241e4a 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
>   
> @@ -336,7 +336,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
>  
>  	ret = wl12xx_chip_wakeup(wl);
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>   

Here you can just "goto out;" so that the chip is powered off before we 
return.

>  
>  	ret = wl->chip.op_boot(wl);
>  	if (ret < 0)
> @@ -357,7 +357,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
>  out:
>  	if (ret < 0)
>  		wl12xx_power_off(wl);
> -
> +unlock:
>  	mutex_unlock(&wl->mutex);
>  
>  	return ret;
>   

Thanks a lot for your patch!

-- 
Cheers,
Luca.