From: Rabin Vincent <[email protected]>
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: [email protected]
Signed-off-by: Rabin Vincent <[email protected]>
---
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
On Thu 10-03-16 13:26:03, Rabin Vincent wrote:
> From: Rabin Vincent <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Rabin Vincent <[email protected]>
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 <[email protected]>
SUSE Labs, CR
(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?
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.
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 <[email protected]>
SUSE Labs, CR