* Re: [PATCH] block: fix ioc leak in put_io_context
2012-03-12 14:59 [PATCH] block: fix ioc leak in put_io_context Xiaotian Feng
@ 2012-03-12 14:22 ` Vivek Goyal
2012-03-13 17:21 ` [PATCH v2] " Xiaotian Feng
0 siblings, 1 reply; 8+ messages in thread
From: Vivek Goyal @ 2012-03-12 14:22 UTC (permalink / raw)
To: Xiaotian Feng; +Cc: linux-kernel, Xiaotian Feng, Xiaotian Feng, Tejun Heo
On Mon, Mar 12, 2012 at 10:59:18AM -0400, Xiaotian Feng wrote:
> When put_io_context is called, if ioc->icq_list is empty and refcount
> is 1, kernel will not free the ioc.
>
> This is caught by following kmemleak:
>
> unreferenced object 0xffff880036349fe0 (size 216):
> comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
> backtrace:
> [<ffffffff8169f926>] kmemleak_alloc+0x26/0x50
> [<ffffffff81195a9c>] kmem_cache_alloc_node+0x1cc/0x2a0
> [<ffffffff81356b67>] create_io_context_slowpath+0x27/0x130
> [<ffffffff81356d2b>] get_task_io_context+0xbb/0xf0
> [<ffffffff81055f0e>] copy_process+0x188e/0x18b0
> [<ffffffff8105609b>] do_fork+0x11b/0x420
> [<ffffffff810247f8>] sys_clone+0x28/0x30
> [<ffffffff816d3373>] stub_clone+0x13/0x20
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> ioc->release_work() should be used even if ioc->icq_list is empty.
>
> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
> block/blk-ioc.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 8b782a6..d74a8ce 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -157,8 +157,7 @@ void put_io_context(struct io_context *ioc)
> */
> if (atomic_long_dec_and_test(&ioc->refcount)) {
> spin_lock_irqsave(&ioc->lock, flags);
> - if (!hlist_empty(&ioc->icq_list))
> - schedule_work(&ioc->release_work);
> + schedule_work(&ioc->release_work);
> spin_unlock_irqrestore(&ioc->lock, flags);
May be free the ioc right here instead of invoking release_work().
Thanks
Vivek
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] block: fix ioc leak in put_io_context
@ 2012-03-12 14:59 Xiaotian Feng
2012-03-12 14:22 ` Vivek Goyal
0 siblings, 1 reply; 8+ messages in thread
From: Xiaotian Feng @ 2012-03-12 14:59 UTC (permalink / raw)
To: linux-kernel; +Cc: Xiaotian Feng, Xiaotian Feng, Tejun Heo
When put_io_context is called, if ioc->icq_list is empty and refcount
is 1, kernel will not free the ioc.
This is caught by following kmemleak:
unreferenced object 0xffff880036349fe0 (size 216):
comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
backtrace:
[<ffffffff8169f926>] kmemleak_alloc+0x26/0x50
[<ffffffff81195a9c>] kmem_cache_alloc_node+0x1cc/0x2a0
[<ffffffff81356b67>] create_io_context_slowpath+0x27/0x130
[<ffffffff81356d2b>] get_task_io_context+0xbb/0xf0
[<ffffffff81055f0e>] copy_process+0x188e/0x18b0
[<ffffffff8105609b>] do_fork+0x11b/0x420
[<ffffffff810247f8>] sys_clone+0x28/0x30
[<ffffffff816d3373>] stub_clone+0x13/0x20
[<ffffffffffffffff>] 0xffffffffffffffff
ioc->release_work() should be used even if ioc->icq_list is empty.
Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
block/blk-ioc.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 8b782a6..d74a8ce 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -157,8 +157,7 @@ void put_io_context(struct io_context *ioc)
*/
if (atomic_long_dec_and_test(&ioc->refcount)) {
spin_lock_irqsave(&ioc->lock, flags);
- if (!hlist_empty(&ioc->icq_list))
- schedule_work(&ioc->release_work);
+ schedule_work(&ioc->release_work);
spin_unlock_irqrestore(&ioc->lock, flags);
}
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] block: fix ioc leak in put_io_context
2012-03-13 17:21 ` [PATCH v2] " Xiaotian Feng
@ 2012-03-13 14:28 ` Vivek Goyal
2012-03-13 14:40 ` Jens Axboe
2012-03-13 15:49 ` Tejun Heo
0 siblings, 2 replies; 8+ messages in thread
From: Vivek Goyal @ 2012-03-13 14:28 UTC (permalink / raw)
To: Xiaotian Feng; +Cc: axboe, tj, linux-kernel, Xiaotian Feng
On Tue, Mar 13, 2012 at 01:21:06PM -0400, Xiaotian Feng wrote:
> When put_io_context is called, if ioc->icq_list is empty and refcount
> is 1, kernel will not free the ioc.
>
> This is caught by following kmemleak:
>
> unreferenced object 0xffff880036349fe0 (size 216):
> comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
> backtrace:
> [<ffffffff8169f926>] kmemleak_alloc+0x26/0x50
> [<ffffffff81195a9c>] kmem_cache_alloc_node+0x1cc/0x2a0
> [<ffffffff81356b67>] create_io_context_slowpath+0x27/0x130
> [<ffffffff81356d2b>] get_task_io_context+0xbb/0xf0
> [<ffffffff81055f0e>] copy_process+0x188e/0x18b0
> [<ffffffff8105609b>] do_fork+0x11b/0x420
> [<ffffffff810247f8>] sys_clone+0x28/0x30
> [<ffffffff816d3373>] stub_clone+0x13/0x20
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> ioc should be freed if ioc->icq_list is empty.
> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> ---
> block/blk-ioc.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index 8b782a6..9690f27 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
> void put_io_context(struct io_context *ioc)
> {
> unsigned long flags;
> + bool free_ioc = false;
>
> if (ioc == NULL)
> return;
> @@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
> spin_lock_irqsave(&ioc->lock, flags);
> if (!hlist_empty(&ioc->icq_list))
> schedule_work(&ioc->release_work);
> + else
> + free_ioc = true;
> spin_unlock_irqrestore(&ioc->lock, flags);
> }
> +
> + if (free_ioc)
> + kmem_cache_free(iocontext_cachep, ioc);
> }
> EXPORT_SYMBOL(put_io_context);
This one looks good to me. Tejun?
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Vivek
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] block: fix ioc leak in put_io_context
2012-03-13 14:28 ` Vivek Goyal
@ 2012-03-13 14:40 ` Jens Axboe
2012-03-13 15:49 ` Tejun Heo
1 sibling, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2012-03-13 14:40 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Xiaotian Feng, tj, linux-kernel, Xiaotian Feng
On 03/13/2012 03:28 PM, Vivek Goyal wrote:
> On Tue, Mar 13, 2012 at 01:21:06PM -0400, Xiaotian Feng wrote:
>> When put_io_context is called, if ioc->icq_list is empty and refcount
>> is 1, kernel will not free the ioc.
>>
>> This is caught by following kmemleak:
>>
>> unreferenced object 0xffff880036349fe0 (size 216):
>> comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
>> backtrace:
>> [<ffffffff8169f926>] kmemleak_alloc+0x26/0x50
>> [<ffffffff81195a9c>] kmem_cache_alloc_node+0x1cc/0x2a0
>> [<ffffffff81356b67>] create_io_context_slowpath+0x27/0x130
>> [<ffffffff81356d2b>] get_task_io_context+0xbb/0xf0
>> [<ffffffff81055f0e>] copy_process+0x188e/0x18b0
>> [<ffffffff8105609b>] do_fork+0x11b/0x420
>> [<ffffffff810247f8>] sys_clone+0x28/0x30
>> [<ffffffff816d3373>] stub_clone+0x13/0x20
>> [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> ioc should be freed if ioc->icq_list is empty.
>> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
>> ---
>> block/blk-ioc.c | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>> index 8b782a6..9690f27 100644
>> --- a/block/blk-ioc.c
>> +++ b/block/blk-ioc.c
>> @@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
>> void put_io_context(struct io_context *ioc)
>> {
>> unsigned long flags;
>> + bool free_ioc = false;
>>
>> if (ioc == NULL)
>> return;
>> @@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
>> spin_lock_irqsave(&ioc->lock, flags);
>> if (!hlist_empty(&ioc->icq_list))
>> schedule_work(&ioc->release_work);
>> + else
>> + free_ioc = true;
>> spin_unlock_irqrestore(&ioc->lock, flags);
>> }
>> +
>> + if (free_ioc)
>> + kmem_cache_free(iocontext_cachep, ioc);
>> }
>> EXPORT_SYMBOL(put_io_context);
>
> This one looks good to me. Tejun?
It's definitely a leak. I have applied it.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] block: fix ioc leak in put_io_context
2012-03-13 14:28 ` Vivek Goyal
2012-03-13 14:40 ` Jens Axboe
@ 2012-03-13 15:49 ` Tejun Heo
2012-03-13 22:44 ` Xiaotian Feng
1 sibling, 1 reply; 8+ messages in thread
From: Tejun Heo @ 2012-03-13 15:49 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Xiaotian Feng, axboe, linux-kernel, Xiaotian Feng
On Tue, Mar 13, 2012 at 10:28:20AM -0400, Vivek Goyal wrote:
> On Tue, Mar 13, 2012 at 01:21:06PM -0400, Xiaotian Feng wrote:
> > diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> > index 8b782a6..9690f27 100644
> > --- a/block/blk-ioc.c
> > +++ b/block/blk-ioc.c
> > @@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
> > void put_io_context(struct io_context *ioc)
> > {
> > unsigned long flags;
> > + bool free_ioc = false;
> >
> > if (ioc == NULL)
> > return;
> > @@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
> > spin_lock_irqsave(&ioc->lock, flags);
> > if (!hlist_empty(&ioc->icq_list))
> > schedule_work(&ioc->release_work);
> > + else
> > + free_ioc = true;
Calling kmem_cache_free() here directly is probably better.
> > spin_unlock_irqrestore(&ioc->lock, flags);
> > }
> > +
> > + if (free_ioc)
> > + kmem_cache_free(iocontext_cachep, ioc);
> > }
> > EXPORT_SYMBOL(put_io_context);
>
> This one looks good to me. Tejun?
>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] block: fix ioc leak in put_io_context
2012-03-12 14:22 ` Vivek Goyal
@ 2012-03-13 17:21 ` Xiaotian Feng
2012-03-13 14:28 ` Vivek Goyal
0 siblings, 1 reply; 8+ messages in thread
From: Xiaotian Feng @ 2012-03-13 17:21 UTC (permalink / raw)
To: axboe, tj, vgoyal; +Cc: linux-kernel, Xiaotian Feng, Xiaotian Feng
When put_io_context is called, if ioc->icq_list is empty and refcount
is 1, kernel will not free the ioc.
This is caught by following kmemleak:
unreferenced object 0xffff880036349fe0 (size 216):
comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
backtrace:
[<ffffffff8169f926>] kmemleak_alloc+0x26/0x50
[<ffffffff81195a9c>] kmem_cache_alloc_node+0x1cc/0x2a0
[<ffffffff81356b67>] create_io_context_slowpath+0x27/0x130
[<ffffffff81356d2b>] get_task_io_context+0xbb/0xf0
[<ffffffff81055f0e>] copy_process+0x188e/0x18b0
[<ffffffff8105609b>] do_fork+0x11b/0x420
[<ffffffff810247f8>] sys_clone+0x28/0x30
[<ffffffff816d3373>] stub_clone+0x13/0x20
[<ffffffffffffffff>] 0xffffffffffffffff
ioc should be freed if ioc->icq_list is empty.
Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
---
block/blk-ioc.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 8b782a6..9690f27 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
void put_io_context(struct io_context *ioc)
{
unsigned long flags;
+ bool free_ioc = false;
if (ioc == NULL)
return;
@@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
spin_lock_irqsave(&ioc->lock, flags);
if (!hlist_empty(&ioc->icq_list))
schedule_work(&ioc->release_work);
+ else
+ free_ioc = true;
spin_unlock_irqrestore(&ioc->lock, flags);
}
+
+ if (free_ioc)
+ kmem_cache_free(iocontext_cachep, ioc);
}
EXPORT_SYMBOL(put_io_context);
--
1.7.5.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] block: fix ioc leak in put_io_context
2012-03-13 15:49 ` Tejun Heo
@ 2012-03-13 22:44 ` Xiaotian Feng
2012-03-13 22:47 ` Tejun Heo
0 siblings, 1 reply; 8+ messages in thread
From: Xiaotian Feng @ 2012-03-13 22:44 UTC (permalink / raw)
To: Tejun Heo; +Cc: Vivek Goyal, axboe, linux-kernel, Xiaotian Feng
On Tue, Mar 13, 2012 at 11:49 PM, Tejun Heo <tj@kernel.org> wrote:
> On Tue, Mar 13, 2012 at 10:28:20AM -0400, Vivek Goyal wrote:
>> On Tue, Mar 13, 2012 at 01:21:06PM -0400, Xiaotian Feng wrote:
>> > diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>> > index 8b782a6..9690f27 100644
>> > --- a/block/blk-ioc.c
>> > +++ b/block/blk-ioc.c
>> > @@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
>> > void put_io_context(struct io_context *ioc)
>> > {
>> > unsigned long flags;
>> > + bool free_ioc = false;
>> >
>> > if (ioc == NULL)
>> > return;
>> > @@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
>> > spin_lock_irqsave(&ioc->lock, flags);
>> > if (!hlist_empty(&ioc->icq_list))
>> > schedule_work(&ioc->release_work);
>> > + else
>> > + free_ioc = true;
>
> Calling kmem_cache_free() here directly is probably better.
I did this on my first try, but I got a kernel warning with the
following spin_unlock on ioc->lock :(
We'll hit a use after free bug then...
>
>> > spin_unlock_irqrestore(&ioc->lock, flags);
>> > }
>> > +
>> > + if (free_ioc)
>> > + kmem_cache_free(iocontext_cachep, ioc);
>> > }
>> > EXPORT_SYMBOL(put_io_context);
>>
>> This one looks good to me. Tejun?
>>
>> Acked-by: Vivek Goyal <vgoyal@redhat.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>
>
> Thanks.
>
> --
> tejun
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] block: fix ioc leak in put_io_context
2012-03-13 22:44 ` Xiaotian Feng
@ 2012-03-13 22:47 ` Tejun Heo
0 siblings, 0 replies; 8+ messages in thread
From: Tejun Heo @ 2012-03-13 22:47 UTC (permalink / raw)
To: Xiaotian Feng; +Cc: Vivek Goyal, axboe, linux-kernel, Xiaotian Feng
On Wed, Mar 14, 2012 at 06:44:22AM +0800, Xiaotian Feng wrote:
> On Tue, Mar 13, 2012 at 11:49 PM, Tejun Heo <tj@kernel.org> wrote:
> > On Tue, Mar 13, 2012 at 10:28:20AM -0400, Vivek Goyal wrote:
> >> On Tue, Mar 13, 2012 at 01:21:06PM -0400, Xiaotian Feng wrote:
> >> > diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> >> > index 8b782a6..9690f27 100644
> >> > --- a/block/blk-ioc.c
> >> > +++ b/block/blk-ioc.c
> >> > @@ -145,6 +145,7 @@ static void ioc_release_fn(struct work_struct *work)
> >> > void put_io_context(struct io_context *ioc)
> >> > {
> >> > unsigned long flags;
> >> > + bool free_ioc = false;
> >> >
> >> > if (ioc == NULL)
> >> > return;
> >> > @@ -159,8 +160,13 @@ void put_io_context(struct io_context *ioc)
> >> > spin_lock_irqsave(&ioc->lock, flags);
> >> > if (!hlist_empty(&ioc->icq_list))
> >> > schedule_work(&ioc->release_work);
> >> > + else
> >> > + free_ioc = true;
> >
> > Calling kmem_cache_free() here directly is probably better.
>
> I did this on my first try, but I got a kernel warning with the
> following spin_unlock on ioc->lock :(
> We'll hit a use after free bug then...
Ah, you're right. Thanks.
--
tejun
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-03-13 22:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-12 14:59 [PATCH] block: fix ioc leak in put_io_context Xiaotian Feng
2012-03-12 14:22 ` Vivek Goyal
2012-03-13 17:21 ` [PATCH v2] " Xiaotian Feng
2012-03-13 14:28 ` Vivek Goyal
2012-03-13 14:40 ` Jens Axboe
2012-03-13 15:49 ` Tejun Heo
2012-03-13 22:44 ` Xiaotian Feng
2012-03-13 22:47 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).