[PATCH] block: protect iterate_bdevs() against concurrent close

[PATCH] block: protect iterate_bdevs() against concurrent close

[Rabin Vincent]

From: Rabin Vincent <rabinv@axis.com>

If a block device is closed while iterate_bdevs() is handling it, the
following NULL pointer dereference occurs because bdev->b_disk is NULL
in bdev_get_queue(), which is called from blk_get_backing_dev_info() (in
turn called by the mapping_cap_writeback_dirty() call in
__filemap_fdatawrite_range()):

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000508
 IP: [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
 PGD 9e62067 PUD 9ee8067 PMD 0
 Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
 Modules linked in:
 CPU: 1 PID: 2422 Comm: sync Not tainted 4.5.0-rc7+ #400
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 task: ffff880009f4d700 ti: ffff880009f5c000 task.ti: ffff880009f5c000
 RIP: 0010:[<ffffffff81314790>]  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
 RSP: 0018:ffff880009f5fe68  EFLAGS: 00010246
 RAX: 0000000000000000 RBX: ffff88000ec17a38 RCX: ffffffff81a4e940
 RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: ffff88000ec176c0
 RBP: ffff880009f5fe68 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000001 R11: 0000000000000000 R12: ffff88000ec17860
 R13: ffffffff811b25c0 R14: ffff88000ec178e0 R15: ffff88000ec17a38
 FS:  00007faee505d700(0000) GS:ffff88000fb00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 CR2: 0000000000000508 CR3: 0000000009e8a000 CR4: 00000000000006e0
 Stack:
  ffff880009f5feb8 ffffffff8112e7f5 0000000000000000 7fffffffffffffff
  0000000000000000 0000000000000000 7fffffffffffffff 0000000000000001
  ffff88000ec178e0 ffff88000ec17860 ffff880009f5fec8 ffffffff8112e81f
 Call Trace:
  [<ffffffff8112e7f5>] __filemap_fdatawrite_range+0x85/0x90
  [<ffffffff8112e81f>] filemap_fdatawrite+0x1f/0x30
  [<ffffffff811b25d6>] fdatawrite_one_bdev+0x16/0x20
  [<ffffffff811bc402>] iterate_bdevs+0xf2/0x130
  [<ffffffff811b2763>] sys_sync+0x63/0x90
  [<ffffffff815d4272>] entry_SYSCALL_64_fastpath+0x12/0x76
 Code: 0f 1f 44 00 00 48 8b 87 f0 00 00 00 55 48 89 e5 <48> 8b 80 08 05 00 00 5d
 RIP  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
  RSP <ffff880009f5fe68>
 CR2: 0000000000000508
 ---[ end trace 2487336ceb3de62d ]---

The crash is easily reproducible by running the following command, if an
msleep(100) is inserted before the call to func() in iterate_devs():

 while :; do head -c1 /dev/nullb0; done > /dev/null & while :; do sync; done

Fix it by holding the bd_mutex across the func() call and only calling
func() if the bdev is opened.

Cc: stable@vger.kernel.org
Signed-off-by: Rabin Vincent <rabinv@axis.com>
---
 fs/block_dev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 826b164..78c9f2a 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1884,6 +1884,7 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg)
 	spin_lock(&blockdev_superblock->s_inode_list_lock);
 	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
 		struct address_space *mapping = inode->i_mapping;
+		struct block_device *bdev;
 
 		spin_lock(&inode->i_lock);
 		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
@@ -1904,8 +1905,12 @@ void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg)
 		 */
 		iput(old_inode);
 		old_inode = inode;
+		bdev = I_BDEV(inode);
 
-		func(I_BDEV(inode), arg);
+		mutex_lock(&bdev->bd_mutex);
+		if (bdev->bd_openers)
+			func(bdev, arg);
+		mutex_unlock(&bdev->bd_mutex);
 
 		spin_lock(&blockdev_superblock->s_inode_list_lock);
 	}
-- 
2.7.0

Re: [PATCH] block: protect iterate_bdevs() against concurrent close

[Jan Kara]

On Thu 10-03-16 13:26:03, Rabin Vincent wrote:
> From: Rabin Vincent <rabinv@axis.com>
> 
> If a block device is closed while iterate_bdevs() is handling it, the
> following NULL pointer dereference occurs because bdev->b_disk is NULL
> in bdev_get_queue(), which is called from blk_get_backing_dev_info() (in
> turn called by the mapping_cap_writeback_dirty() call in
> __filemap_fdatawrite_range()):
> 
>  BUG: unable to handle kernel NULL pointer dereference at 0000000000000508
>  IP: [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>  PGD 9e62067 PUD 9ee8067 PMD 0
>  Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>  Modules linked in:
>  CPU: 1 PID: 2422 Comm: sync Not tainted 4.5.0-rc7+ #400
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>  task: ffff880009f4d700 ti: ffff880009f5c000 task.ti: ffff880009f5c000
>  RIP: 0010:[<ffffffff81314790>]  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>  RSP: 0018:ffff880009f5fe68  EFLAGS: 00010246
>  RAX: 0000000000000000 RBX: ffff88000ec17a38 RCX: ffffffff81a4e940
>  RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: ffff88000ec176c0
>  RBP: ffff880009f5fe68 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000001 R11: 0000000000000000 R12: ffff88000ec17860
>  R13: ffffffff811b25c0 R14: ffff88000ec178e0 R15: ffff88000ec17a38
>  FS:  00007faee505d700(0000) GS:ffff88000fb00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>  CR2: 0000000000000508 CR3: 0000000009e8a000 CR4: 00000000000006e0
>  Stack:
>   ffff880009f5feb8 ffffffff8112e7f5 0000000000000000 7fffffffffffffff
>   0000000000000000 0000000000000000 7fffffffffffffff 0000000000000001
>   ffff88000ec178e0 ffff88000ec17860 ffff880009f5fec8 ffffffff8112e81f
>  Call Trace:
>   [<ffffffff8112e7f5>] __filemap_fdatawrite_range+0x85/0x90
>   [<ffffffff8112e81f>] filemap_fdatawrite+0x1f/0x30
>   [<ffffffff811b25d6>] fdatawrite_one_bdev+0x16/0x20
>   [<ffffffff811bc402>] iterate_bdevs+0xf2/0x130
>   [<ffffffff811b2763>] sys_sync+0x63/0x90
>   [<ffffffff815d4272>] entry_SYSCALL_64_fastpath+0x12/0x76
>  Code: 0f 1f 44 00 00 48 8b 87 f0 00 00 00 55 48 89 e5 <48> 8b 80 08 05 00 00 5d
>  RIP  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>   RSP <ffff880009f5fe68>
>  CR2: 0000000000000508
>  ---[ end trace 2487336ceb3de62d ]---
> 
> The crash is easily reproducible by running the following command, if an
> msleep(100) is inserted before the call to func() in iterate_devs():
> 
>  while :; do head -c1 /dev/nullb0; done > /dev/null & while :; do sync; done
> 
> Fix it by holding the bd_mutex across the func() call and only calling
> func() if the bdev is opened.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rabin Vincent <rabinv@axis.com>

Thanks for spotting the problem. The patch will fix the problem you found.
But what prevents e.g. flusher thread from trying to writeback the block
device inode while that gets invalidated at the same moment? I.e., the
problem doesn't seem to be specific to iterate_bdevs() but rather hits
anything which writes back block device inodes, right?

Frankly it seems like a catch that mapping_cap_foo() requires block device
to be open (if mapping belongs to a block device) to be safe. The trouble
is we'd need to make inode_to_bdi() result stable while we work with the
block device inode. Christoph, do you have any idea how to cleanly achieve
that? All I'm able to come up with are ugly hacks...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] block: protect iterate_bdevs() against concurrent close

[Rabin Vincent]

(fixed Jens' address)

On Thu, Mar 10, 2016 at 06:37:27PM +0100, Jan Kara wrote:
> On Thu 10-03-16 13:26:03, Rabin Vincent wrote:
> > If a block device is closed while iterate_bdevs() is handling it, the
> > following NULL pointer dereference occurs because bdev->b_disk is NULL
> > in bdev_get_queue(), which is called from blk_get_backing_dev_info() (in
> > turn called by the mapping_cap_writeback_dirty() call in
> > __filemap_fdatawrite_range()):
> 
> Thanks for spotting the problem. The patch will fix the problem you
> found.  But what prevents e.g. flusher thread from trying to writeback
> the block device inode while that gets invalidated at the same moment?

Don't the sync_block_dev() / bdev_write_inode() calls in __blkdev_put()
prevent this?

Re: [PATCH] block: protect iterate_bdevs() against concurrent close

[Christoph Hellwig]

On Thu, Mar 10, 2016 at 06:37:27PM +0100, Jan Kara wrote:
> Thanks for spotting the problem. The patch will fix the problem you found.
> But what prevents e.g. flusher thread from trying to writeback the block
> device inode while that gets invalidated at the same moment? I.e., the
> problem doesn't seem to be specific to iterate_bdevs() but rather hits
> anything which writes back block device inodes, right?
> 
> Frankly it seems like a catch that mapping_cap_foo() requires block device
> to be open (if mapping belongs to a block device) to be safe. The trouble
> is we'd need to make inode_to_bdi() result stable while we work with the
> block device inode. Christoph, do you have any idea how to cleanly achieve
> that? All I'm able to come up with are ugly hacks...

I think the whole idea of clearing a queue from a block device
just because it's not open is the root of the problem.  Or, to dive
deeper into the issue our whole object model and life time rules
between struct block_device, struct gendisk, struct request_queue and
struct backing_dev_info are rather broken.  I'd done a few attempt at
sorting this out, but it's a huge task and I haven't gotten far.

I think for now the patch from Rabin should be fine.

Re: [PATCH] block: protect iterate_bdevs() against concurrent close

[Jan Kara]

On Mon 14-03-16 18:42:58, Rabin Vincent wrote:
> (fixed Jens' address)
> 
> On Thu, Mar 10, 2016 at 06:37:27PM +0100, Jan Kara wrote:
> > On Thu 10-03-16 13:26:03, Rabin Vincent wrote:
> > > If a block device is closed while iterate_bdevs() is handling it, the
> > > following NULL pointer dereference occurs because bdev->b_disk is NULL
> > > in bdev_get_queue(), which is called from blk_get_backing_dev_info() (in
> > > turn called by the mapping_cap_writeback_dirty() call in
> > > __filemap_fdatawrite_range()):
> > 
> > Thanks for spotting the problem. The patch will fix the problem you
> > found.  But what prevents e.g. flusher thread from trying to writeback
> > the block device inode while that gets invalidated at the same moment?
> 
> Don't the sync_block_dev() / bdev_write_inode() calls in __blkdev_put()
> prevent this?

They don't. They make sure all dirty data is written but flusher thread can
still be in the middle of iteration over block device pages. So flusher
thread may still hold reference to the block device inode while
__blkdev_put() is done with it. Although flusher has nothing to write
anymore, it can still call some function where using inode_to_bdi() will
result in NULL pointer dereference...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR