Re: [PATCH] block: protect iterate_bdevs() against concurrent close
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(&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 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
Re: [PATCH] block: protect iterate_bdevs() against concurrent close
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(&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 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: 82b013d0 R09: 81ea8107 [ 5750.
Re: [PATCH] block: protect iterate_bdevs() against concurrent close
On 12/01/2016 01: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. I've added this to the 4.10 branch since it's a bit late in the cycle, and the regression is really old. -- Jens Axboe -- 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
Re: [PATCH] block: protect iterate_bdevs() against concurrent close
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