block: fix ioc leak in put_io_context

PATCH Superseded Xiaotian Feng 4 participants 8 messages Mar 12, 2012 – Mar 13, 2012 Block layer

lore

Share & tools

lore

Xiaotian Feng

author

2012-03-12 06:56 UTC #0 lore raw

PATCH 1/1 block: fix ioc leak in put_io_context

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 <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-ioc.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

block/blk-ioc.c +1 -2

index 8b782a6..d74a8ce 100644
@@ -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);
 	}
 }

Signature · 1 line

1.7.5.4

Vivek Goyal

2012-03-12 15:19 UTC #1 lore raw

Quoted 35 lines · 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->release_work() should be used even if ioc->icq_list is empty.
Signed-off-by Xiaotian Feng <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
---
 block/blk-ioc.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
block/blk-ioc.c +0 -0
index 8b782a6..d74a8ce 100644

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 161

Quoted 7 lines

block/blk-ioc.c +1 -2

@@ -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

Xiaotian Feng

author

2012-03-13 09:18 UTC #2 lore raw

PATCH v2 1/1 block: fix ioc leak in put_io_context

    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 <[email protected]>

---
 block/blk-ioc.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

block/blk-ioc.c +6 -0

index 8b782a6..9690f27 100644
@@ -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);

Signature · 1 line

1.7.5.4

Vivek Goyal

Acked-by on v2 1/1

2012-03-13 14:28 UTC #3 lore raw

Quoted 42 lines · 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 <[email protected]>

---
 block/blk-ioc.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

block/blk-ioc.c +1 -0

index 8b782a6..9690f27 100644
@@ -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))

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 171

Quoted 10 lines

block/blk-ioc.c +5 -0

 			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 <[email protected]>

Vivek

Jens Axboe

maintainer

2012-03-13 14:41 UTC #4 lore raw

Quoted 47 lines · Vivek Goyal

Quoted 46 lines · 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 <[email protected]>

---
 block/blk-ioc.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

block/blk-ioc.c +3 -0

index 8b782a6..9690f27 100644
@@ -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);

Quoted 8 lines

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 171
Quoted 6 lines
block/blk-ioc.c +3 -0
 	}
+
+	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.

Signature · 1 line

Jens Axboe

Tejun Heo

Acked-by

2012-03-13 15:49 UTC #5 lore raw

In reply to Vivek Goyal

Quoted 11 lines · Vivek Goyal

Quoted 10 lines · Xiaotian Feng

block/blk-ioc.c +1 -0

index 8b782a6..9690f27 100644
@@ -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;

Quoted 8 lines

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 164

Quoted 8 lines

block/blk-ioc.c +2 -0

 	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.

Quoted 11 lines

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 171
Quoted 7 lines
block/blk-ioc.c +3 -0
 		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 <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Xiaotian Feng

author

2012-03-13 22:44 UTC #6 lore raw

Quoted 12 lines · Tejun Heo

Quoted 11 lines · Vivek Goyal

Quoted 10 lines · Xiaotian Feng

block/blk-ioc.c +1 -0

index 8b782a6..9690f27 100644
@@ -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;

Quoted 10 lines

Quoted 8 lines

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 164

Quoted 8 lines

block/blk-ioc.c +2 -0

     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...

Quoted 19 lines

Quoted 9 lines
block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 171
Quoted 8 lines
block/blk-ioc.c +3 -0
             spin_unlock_irqrestore(&ioc->lock, flags);
     }
+
+   if (free_ioc)
+           kmem_cache_free(iocontext_cachep, ioc);
  }
  EXPORT_SYMBOL(put_io_context);
Quoted 3 lines
This one looks good to me. Tejun?
Acked-by Vivek Goyal <[email protected]>
Acked-by Tejun Heo <[email protected]>
Thanks.
--
tejun

Tejun Heo

2012-03-13 22:47 UTC #7 lore raw

Quoted 24 lines · Xiaotian Feng

Quoted 12 lines · Tejun Heo

Quoted 11 lines · Vivek Goyal

Quoted 10 lines · Xiaotian Feng

block/blk-ioc.c +1 -0

index 8b782a6..9690f27 100644
@@ -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;

Quoted 10 lines

Quoted 8 lines

block/blk-ioc.c void put_io_context(struct io_context *ioc) · line 164

Quoted 8 lines

block/blk-ioc.c +2 -0

     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.

Quoted 3 lines

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.

Signature · 1 line

tejun