| Message ID | 1457612763-23196-1-git-send-email-rabin.vincent@axis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
(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? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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); }