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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 1, 2016 at 12:18 AM, Jan Kara <jack@xxxxxxx> wrote:
> From: Rabin Vincent <rabinv@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Fixes: 5c0d6b60a0ba46d45020547eacf7199171920935
> Reported-and-tested-by: Wei Fang <fangwei1@xxxxxxxxxx>
> Signed-off-by: Rabin Vincent <rabinv@xxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
>  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 05b553368bb4..899fa8ccc347 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1950,6 +1950,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) ||
> @@ -1970,8 +1971,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);
>         }
> --


Hi,

I hit a bug with a similar signature back in October:

    http://marc.info/?l=linux-block&m=147769594003740&w=2

...and still see it in 4.10-rc2.

My reproducer is not very reliable. I'm thinking this fix doesn't work
because it assumes the only race is close vs sync, but my case is
device-shutdown vs sync. In fact iterate_bdevs() is not in my
backtrace:

[ 5750.941063] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000568
[..]
[ 5750.959449] CPU: 1 PID: 8910 Comm: lt-libndctl Tainted: G
O    4.10.0-rc2+ #672
[ 5750.961283] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
[ 5750.963538] task: ffff88033173c600 task.stack: ffffc90006a80000
[ 5750.964694] RIP: 0010:blk_get_backing_dev_info+0x10/0x20
[ 5750.965774] RSP: 0018:ffffc90006a83b00 EFLAGS: 00010246
[ 5750.966842] RAX: 0000000000000000 RBX: ffffc90006a83b60 RCX: 0000000000000000
[ 5750.968136] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88031b3a8040
[ 5750.969429] RBP: ffffc90006a83b00 R08: ffffffff82b013d0 R09: ffffffff81ea8107
[ 5750.970732] R10: 0000000000000001 R11: ffffffff82acd5c0 R12: ffff88031b3a83d0
[ 5750.972046] R13: ffff88031b3a81d0 R14: ffff8801efa934a0 R15: ffff88031b3a81d0
[ 5750.973344] FS:  00007f2cc4ed5d80(0000) GS:ffff88033ed00000(0000)
knlGS:0000000000000000
[ 5750.975171] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5750.976295] CR2: 0000000000000568 CR3: 0000000333fcf000 CR4: 00000000000406e0
[ 5750.977608] Call Trace:
[ 5750.978345]  __inode_attach_wb+0x3a7/0x5d0
[ 5750.979281]  __filemap_fdatawrite_range+0xf8/0x100
[ 5750.980303]  filemap_write_and_wait+0x40/0x90
[ 5750.981270]  fsync_bdev+0x54/0x60
[ 5750.982110]  ? bdget_disk+0x30/0x40
[ 5750.982967]  invalidate_partition+0x24/0x50
[ 5750.983910]  del_gendisk+0xfa/0x230
[ 5750.984768]  pmem_release_disk+0x12/0x20 [nd_pmem]
[ 5750.985787]  devm_action_release+0xf/0x20
[ 5750.986697]  release_nodes+0x16d/0x2b0
[ 5750.987595]  devres_release_all+0x3c/0x60
[ 5750.988510]  device_release_driver_internal+0x16d/0x210
[ 5750.989586]  device_release_driver+0x12/0x20
[ 5750.990537]  unbind_store+0x10f/0x160
[ 5750.991425]  drv_attr_store+0x25/0x30
[ 5750.992301]  sysfs_kf_write+0x45/0x60
[ 5750.993176]  kernfs_fop_write+0x13c/0x1c0
[ 5750.994099]  __vfs_write+0x37/0x160
[ 5750.994967]  ? rcu_read_lock_sched_held+0x5d/0x70
[ 5750.995975]  ? rcu_sync_lockdep_assert+0x2f/0x60
[ 5750.996975]  ? __sb_start_write+0xce/0x1d0
[ 5750.997913]  ? vfs_write+0x17d/0x1a0
[ 5750.998790]  vfs_write+0xb5/0x1a0
[ 5750.999626]  SyS_write+0x58/0xc0
[ 5751.000458]  entry_SYSCALL_64_fastpath+0x1f/0xc2

...so I'm going to take a look at having blk_get_backing_dev_info()
take its own blkdev_get() reference.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]