[PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Zygo Blaxell]

Fix mismatch between calls to write_lock() and write_unlock() in
gen_pool_destroy by removing the write_lock().

Signed-off-by: Zygo Blaxell <zygo.blaxell@xandros.com>
---
There is a call to write_lock() in gen_pool_destroy which is not balanced
by any corresponding write_unlock().  This causes problems with preemption
because the preemption-disable counter is incremented in the write_lock()
call, but never decremented by any call to write_unlock().  This bug is
difficult to observe in the field because only two in-tree drivers call
gen_pool_destroy, and one of them is non-x86 arch-specific code.

To fix this, I have chosen removing the write_lock() over adding a
write_unlock() because the lock in question is inside a structure which
is being freed.  Any other thread that waited to acquire such a lock
while gen_pool_destroy was running would find itself holding a lock
in recently-freed or about-to-be-freed memory.  This would result in
memory corruption or a crash whether &pool->lock is held or not.

Using a pool while it is in the process of being destroyed is a bug that
must be resolved outside of the gen_pool_destroy function.

 lib/genalloc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/lib/genalloc.c b/lib/genalloc.c
index f6d276d..eed2bdb 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -85,7 +85,6 @@ void gen_pool_destroy(struct gen_pool *pool)
 	int bit, end_bit;
 
 
-	write_lock(&pool->lock);
 	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
 		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
 		list_del(&chunk->next_chunk);
-- 
1.5.6.5

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Jiri Kosina]

On Fri, 12 Jun 2009, Zygo Blaxell wrote:

> Fix mismatch between calls to write_lock() and write_unlock() in
> gen_pool_destroy by removing the write_lock().
> 
> Signed-off-by: Zygo Blaxell <zygo.blaxell@xandros.com>
> ---
> There is a call to write_lock() in gen_pool_destroy which is not balanced
> by any corresponding write_unlock().  This causes problems with preemption
> because the preemption-disable counter is incremented in the write_lock()
> call, but never decremented by any call to write_unlock().  This bug is
> difficult to observe in the field because only two in-tree drivers call
> gen_pool_destroy, and one of them is non-x86 arch-specific code.
> 
> To fix this, I have chosen removing the write_lock() over adding a
> write_unlock() because the lock in question is inside a structure which
> is being freed.  Any other thread that waited to acquire such a lock
> while gen_pool_destroy was running would find itself holding a lock
> in recently-freed or about-to-be-freed memory.  This would result in
> memory corruption or a crash whether &pool->lock is held or not.
> 
> Using a pool while it is in the process of being destroyed is a bug that
> must be resolved outside of the gen_pool_destroy function.
> 
>  lib/genalloc.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/genalloc.c b/lib/genalloc.c
> index f6d276d..eed2bdb 100644
> --- a/lib/genalloc.c
> +++ b/lib/genalloc.c
> @@ -85,7 +85,6 @@ void gen_pool_destroy(struct gen_pool *pool)
>  	int bit, end_bit;
>  
>  
> -	write_lock(&pool->lock);
>  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
>  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
>  		list_del(&chunk->next_chunk);
> -- 
> 1.5.6.5
> 

Hi Zygo,

this doesn't really qualify for trivial tree, as it introduces a 
significant code change. Adding some CCs.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Steve Wise]

Jiri Kosina wrote:
> On Fri, 12 Jun 2009, Zygo Blaxell wrote:
>
>   
>> Fix mismatch between calls to write_lock() and write_unlock() in
>> gen_pool_destroy by removing the write_lock().
>>
>> Signed-off-by: Zygo Blaxell <zygo.blaxell@xandros.com>
>> ---
>> There is a call to write_lock() in gen_pool_destroy which is not balanced
>> by any corresponding write_unlock().  This causes problems with preemption
>> because the preemption-disable counter is incremented in the write_lock()
>> call, but never decremented by any call to write_unlock().  This bug is
>> difficult to observe in the field because only two in-tree drivers call
>> gen_pool_destroy, and one of them is non-x86 arch-specific code.
>>
>> To fix this, I have chosen removing the write_lock() over adding a
>> write_unlock() because the lock in question is inside a structure which
>> is being freed.  Any other thread that waited to acquire such a lock
>> while gen_pool_destroy was running would find itself holding a lock
>> in recently-freed or about-to-be-freed memory.  This would result in
>> memory corruption or a crash whether &pool->lock is held or not.
>>
>> Using a pool while it is in the process of being destroyed is a bug that
>> must be resolved outside of the gen_pool_destroy function.
>>
>>  lib/genalloc.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/genalloc.c b/lib/genalloc.c
>> index f6d276d..eed2bdb 100644
>> --- a/lib/genalloc.c
>> +++ b/lib/genalloc.c
>> @@ -85,7 +85,6 @@ void gen_pool_destroy(struct gen_pool *pool)
>>  	int bit, end_bit;
>>  
>>  
>> -	write_lock(&pool->lock);
>>  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
>>  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
>>  		list_del(&chunk->next_chunk);
>> -- 
>> 1.5.6.5
>>
>>     
>
> Hi Zygo,
>
> this doesn't really qualify for trivial tree, as it introduces a 
> significant code change. Adding some CCs.
>
>   

Looks ok to me.  Its dumb to aquire the lock you're gonna free anyway.  
Maybe some BUG_ON() that sez nobody better be holding this lock?

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Andrew Morton]

On Mon, 15 Jun 2009 17:04:15 -0500
Steve Wise <swise@opengridcomputing.com> wrote:

> > this doesn't really qualify for trivial tree, as it introduces a 
> > significant code change. Adding some CCs.
> >
> >   
> 
> Looks ok to me.  Its dumb to aquire the lock you're gonna free anyway.  
> Maybe some BUG_ON() that sez nobody better be holding this lock?

CONFIG_DEBUG_OBJECTS_FREE could detect the freeing of a held rwlock. 
But probably it hasn't been wired up to handle rwlocks.

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Jiri Kosina]

On Mon, 15 Jun 2009, Andrew Morton wrote:

> > > this doesn't really qualify for trivial tree, as it introduces a 
> > > significant code change. Adding some CCs.
> > Looks ok to me.  Its dumb to aquire the lock you're gonna free anyway.  
> > Maybe some BUG_ON() that sez nobody better be holding this lock?
> CONFIG_DEBUG_OBJECTS_FREE could detect the freeing of a held rwlock. 
> But probably it hasn't been wired up to handle rwlocks.

Hmm, in fact ... am I just completely blind, or is the only user of 
CONFIG_DEBUG_OBJECTS_FREE the timer code?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Andrew Morton]

On Tue, 16 Jun 2009 10:23:08 +0200 (CEST) Jiri Kosina <jkosina@suse.cz> wrote:

> On Mon, 15 Jun 2009, Andrew Morton wrote:
> 
> > > > this doesn't really qualify for trivial tree, as it introduces a 
> > > > significant code change. Adding some CCs.
> > > Looks ok to me.  Its dumb to aquire the lock you're gonna free anyway.  
> > > Maybe some BUG_ON() that sez nobody better be holding this lock?
> > CONFIG_DEBUG_OBJECTS_FREE could detect the freeing of a held rwlock. 
> > But probably it hasn't been wired up to handle rwlocks.
> 
> Hmm, in fact ... am I just completely blind, or is the only user of 
> CONFIG_DEBUG_OBJECTS_FREE the timer code?

Could be.  That infrastructure hasn't really been followed up on yet.

There's also CONFIG_DEBUG_LOCK_ALLOC which perhaps could/should be
implemented via debugobjects.

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Andrew Morton]

On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
Jiri Kosina <trivial@kernel.org> wrote:

> > -	write_lock(&pool->lock);
> >  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
> >  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
> >  		list_del(&chunk->next_chunk);
> > -- 
> > 1.5.6.5
> > 
> 
> Hi Zygo,
> 
> this doesn't really qualify for trivial tree, as it introduces a 
> significant code change. Adding some CCs.

yep, I merged it, thanks.

I wonder why drivers/infiniband/hw/cxgb3 users never noticed this.

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Steve Wise]

Andrew Morton wrote:
> On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
> Jiri Kosina <trivial@kernel.org> wrote:
>
>   
>>> -	write_lock(&pool->lock);
>>>  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
>>>  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
>>>  		list_del(&chunk->next_chunk);
>>> -- 
>>> 1.5.6.5
>>>
>>>       
>> Hi Zygo,
>>
>> this doesn't really qualify for trivial tree, as it introduces a 
>> significant code change. Adding some CCs.
>>     
>
> yep, I merged it, thanks.
>
> I wonder why drivers/infiniband/hw/cxgb3 users never noticed this.
>   

I seem to remember trying to get this removed a few years ago and the 
owner didn't want it removed...

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Andrew Morton]

On Mon, 15 Jun 2009 17:30:32 -0500
Steve Wise <swise@opengridcomputing.com> wrote:

> Andrew Morton wrote:
> > On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
> > Jiri Kosina <trivial@kernel.org> wrote:
> >
> >   
> >>> -	write_lock(&pool->lock);
> >>>  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
> >>>  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
> >>>  		list_del(&chunk->next_chunk);
> >>> -- 
> >>> 1.5.6.5
> >>>
> >>>       
> >> Hi Zygo,
> >>
> >> this doesn't really qualify for trivial tree, as it introduces a 
> >> significant code change. Adding some CCs.
> >>     
> >
> > yep, I merged it, thanks.
> >
> > I wonder why drivers/infiniband/hw/cxgb3 users never noticed this.
> >   
> 
> I seem to remember trying to get this removed a few years ago and the 
> owner didn't want it removed...
> 

void gen_pool_destroy(struct gen_pool *pool)
{
	struct list_head *_chunk, *_next_chunk;
	struct gen_pool_chunk *chunk;
	int order = pool->min_alloc_order;
	int bit, end_bit;


	write_lock(&pool->lock);
	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
		list_del(&chunk->next_chunk);

		end_bit = (chunk->end_addr - chunk->start_addr) >> order;
		bit = find_next_bit(chunk->bits, end_bit, 0);
		BUG_ON(bit < end_bit);

		kfree(chunk);
	}
	kfree(pool);
	return;
}

The write_lock is unneeded and wrong.  Because if any other thread of
control is concurrently playing with this pool, it will sometimes do a
use-after-free.

So no other thread of control should have access to this pool, so
there's no need for the write_lock().

Re: [PATCH] LIB: remove unmatched write_lock() in gen_pool_destroy

[Steve Wise]

Andrew Morton wrote:
> On Mon, 15 Jun 2009 17:30:32 -0500
> Steve Wise <swise@opengridcomputing.com> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
>>> Jiri Kosina <trivial@kernel.org> wrote:
>>>
>>>   
>>>       
>>>>> -	write_lock(&pool->lock);
>>>>>  	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
>>>>>  		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
>>>>>  		list_del(&chunk->next_chunk);
>>>>> -- 
>>>>> 1.5.6.5
>>>>>
>>>>>       
>>>>>           
>>>> Hi Zygo,
>>>>
>>>> this doesn't really qualify for trivial tree, as it introduces a 
>>>> significant code change. Adding some CCs.
>>>>     
>>>>         
>>> yep, I merged it, thanks.
>>>
>>> I wonder why drivers/infiniband/hw/cxgb3 users never noticed this.
>>>   
>>>       
>> I seem to remember trying to get this removed a few years ago and the 
>> owner didn't want it removed...
>>
>>     
>
> void gen_pool_destroy(struct gen_pool *pool)
> {
> 	struct list_head *_chunk, *_next_chunk;
> 	struct gen_pool_chunk *chunk;
> 	int order = pool->min_alloc_order;
> 	int bit, end_bit;
>
>
> 	write_lock(&pool->lock);
> 	list_for_each_safe(_chunk, _next_chunk, &pool->chunks) {
> 		chunk = list_entry(_chunk, struct gen_pool_chunk, next_chunk);
> 		list_del(&chunk->next_chunk);
>
> 		end_bit = (chunk->end_addr - chunk->start_addr) >> order;
> 		bit = find_next_bit(chunk->bits, end_bit, 0);
> 		BUG_ON(bit < end_bit);
>
> 		kfree(chunk);
> 	}
> 	kfree(pool);
> 	return;
> }
>
> The write_lock is unneeded and wrong.  Because if any other thread of
> control is concurrently playing with this pool, it will sometimes do a
> use-after-free.
>
> So no other thread of control should have access to this pool, so
> there's no need for the write_lock().
>   

Yup. 

My original patch adding gen_pool_destroy() didn't have the 
write_lock().  It was added as part of "reviewing" the patch. :)

Steve.