Fix mismatch between calls to write_lock() and write_unlock() in
gen_pool_destroy by removing the write_lock().
Signed-off-by: Zygo Blaxell <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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?
On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
Jiri Kosina <[email protected]> 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.
Andrew Morton wrote:
> On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
> Jiri Kosina <[email protected]> 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...
On Mon, 15 Jun 2009 17:04:15 -0500
Steve Wise <[email protected]> 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.
On Mon, 15 Jun 2009 17:30:32 -0500
Steve Wise <[email protected]> wrote:
> Andrew Morton wrote:
> > On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
> > Jiri Kosina <[email protected]> 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().
Andrew Morton wrote:
> On Mon, 15 Jun 2009 17:30:32 -0500
> Steve Wise <[email protected]> wrote:
>
>
>> Andrew Morton wrote:
>>
>>> On Mon, 15 Jun 2009 23:35:31 +0200 (CEST)
>>> Jiri Kosina <[email protected]> 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.
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
On Tue, 16 Jun 2009 10:23:08 +0200 (CEST) Jiri Kosina <[email protected]> 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.