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

2017-01-05 Thread Dan Williams 
On Thu, Jan 5, 2017 at 4:03 PM, Dan Williams  wrote:
> On Thu, Dec 1, 2016 at 12:18 AM, Jan Kara  wrote:
>> From: Rabin Vincent 
>>
>> 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 0508
>>  IP: [] blk_get_backing_dev_info+0x10/0x20
>>  PGD 9e62067 PUD 9ee8067 PMD 0
>>  Oops:  [#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: 880009f4d700 ti: 880009f5c000 task.ti: 880009f5c000
>>  RIP: 0010:[]  [] 
>> blk_get_backing_dev_info+0x10/0x20
>>  RSP: 0018:880009f5fe68  EFLAGS: 00010246
>>  RAX:  RBX: 88000ec17a38 RCX: 81a4e940
>>  RDX: 7fff RSI:  RDI: 88000ec176c0
>>  RBP: 880009f5fe68 R08:  R09: 
>>  R10: 0001 R11:  R12: 88000ec17860
>>  R13: 811b25c0 R14: 88000ec178e0 R15: 88000ec17a38
>>  FS:  7faee505d700() GS:88000fb0() knlGS:
>>  CS:  0010 DS:  ES:  CR0: 8005003b
>>  CR2: 0508 CR3: 09e8a000 CR4: 06e0
>>  Stack:
>>   880009f5feb8 8112e7f5  7fff
>>     7fff 0001
>>   88000ec178e0 88000ec17860 880009f5fec8 8112e81f
>>  Call Trace:
>>   [] __filemap_fdatawrite_range+0x85/0x90
>>   [] filemap_fdatawrite+0x1f/0x30
>>   [] fdatawrite_one_bdev+0x16/0x20
>>   [] iterate_bdevs+0xf2/0x130
>>   [] sys_sync+0x63/0x90
>>   [] 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  [] blk_get_backing_dev_info+0x10/0x20
>>   RSP 
>>  CR2: 0508
>>  ---[ 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: sta...@vger.kernel.org
>> Fixes: 5c0d6b60a0ba46d45020547eacf7199171920935
>> Reported-and-tested-by: Wei Fang 
>> Signed-off-by: Rabin Vincent 
>> Signed-off-by: Jan Kara 
>> ---
>>  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(_superblock->s_inode_list_lock);
>> list_for_each_entry(inode, _superblock->s_inodes, 
>> i_sb_list) {
>> struct address_space *mapping = inode->i_mapping;
>> +   struct block_device *bdev;
>>
>> spin_lock(>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(>bd_mutex);
>> +   if (bdev->bd_openers)
>> +   func(bdev, arg);
>> +   mutex_unlock(>bd_mutex);
>>
>> spin_lock(_superblock->s_inode_list_lock);
>> }
>> --
>
>
> Hi,
>
> I hit a bug with a similar signature back in October:
>
> http://marc.info/?l=linux-block=147769594003740=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 0568
> [..]
> [ 5750.959449] CPU: 1 PID: 8910 Comm: lt-libndctl Tainted: G
> O4.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: 88033173c600 task.stack: c90006a8
> [ 5750.964694] RIP: 0010:blk_get_backing_dev_info+0x10/0x20
> [ 5750.965774] RSP: 0018:c90006a83b00 EFLAGS: 00010246

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

2017-01-05 Thread Dan Williams 
On Thu, Dec 1, 2016 at 12:18 AM, Jan Kara  wrote:
> From: Rabin Vincent 
>
> 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 0508
>  IP: [] blk_get_backing_dev_info+0x10/0x20
>  PGD 9e62067 PUD 9ee8067 PMD 0
>  Oops:  [#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: 880009f4d700 ti: 880009f5c000 task.ti: 880009f5c000
>  RIP: 0010:[]  [] 
> blk_get_backing_dev_info+0x10/0x20
>  RSP: 0018:880009f5fe68  EFLAGS: 00010246
>  RAX:  RBX: 88000ec17a38 RCX: 81a4e940
>  RDX: 7fff RSI:  RDI: 88000ec176c0
>  RBP: 880009f5fe68 R08:  R09: 
>  R10: 0001 R11:  R12: 88000ec17860
>  R13: 811b25c0 R14: 88000ec178e0 R15: 88000ec17a38
>  FS:  7faee505d700() GS:88000fb0() knlGS:
>  CS:  0010 DS:  ES:  CR0: 8005003b
>  CR2: 0508 CR3: 09e8a000 CR4: 06e0
>  Stack:
>   880009f5feb8 8112e7f5  7fff
>     7fff 0001
>   88000ec178e0 88000ec17860 880009f5fec8 8112e81f
>  Call Trace:
>   [] __filemap_fdatawrite_range+0x85/0x90
>   [] filemap_fdatawrite+0x1f/0x30
>   [] fdatawrite_one_bdev+0x16/0x20
>   [] iterate_bdevs+0xf2/0x130
>   [] sys_sync+0x63/0x90
>   [] 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  [] blk_get_backing_dev_info+0x10/0x20
>   RSP 
>  CR2: 0508
>  ---[ 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: sta...@vger.kernel.org
> Fixes: 5c0d6b60a0ba46d45020547eacf7199171920935
> Reported-and-tested-by: Wei Fang 
> Signed-off-by: Rabin Vincent 
> Signed-off-by: Jan Kara 
> ---
>  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(_superblock->s_inode_list_lock);
> list_for_each_entry(inode, _superblock->s_inodes, i_sb_list) 
> {
> struct address_space *mapping = inode->i_mapping;
> +   struct block_device *bdev;
>
> spin_lock(>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(>bd_mutex);
> +   if (bdev->bd_openers)
> +   func(bdev, arg);
> +   mutex_unlock(>bd_mutex);
>
> spin_lock(_superblock->s_inode_list_lock);
> }
> --


Hi,

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

http://marc.info/?l=linux-block=147769594003740=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 0568
[..]
[ 5750.959449] CPU: 1 PID: 8910 Comm: lt-libndctl Tainted: G
O4.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: 88033173c600 task.stack: c90006a8
[ 5750.964694] RIP: 0010:blk_get_backing_dev_info+0x10/0x20
[ 5750.965774] RSP: 0018:c90006a83b00 EFLAGS: 00010246
[ 5750.966842] RAX:  RBX: c90006a83b60 RCX: 
[ 5750.968136] RDX: 0001 RSI:  RDI: 88031b3a8040
[ 5750.969429] RBP: c90006a83b00 R08:

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

2016-12-01 Thread Christoph Hellwig 
Looks good,

Reviewed-by: Christoph Hellwig 
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html