On Sat 26-11-16 10:06:22, Wei Fang wrote: > ->bd_disk is assigned to NULL in __blkdev_put() when no one is holding > the bdev. After that, ->bd_inode still can be touched in the > blockdev_superblock->s_inodes list before the final iput. So iterate_bdevs() > can still get this inode, and start writeback on mapping dirty pages. > ->bd_disk will be dereferenced in mapping_cap_writeback_dirty() in this > case, and a NULL dereference crash will be triggered: > > Unable to handle kernel NULL pointer dereference at virtual address 00000388 > ... > [<ffff8000004cb1e4>] blk_get_backing_dev_info+0x1c/0x28 > [<ffff8000001c879c>] __filemap_fdatawrite_range+0x54/0x98 > [<ffff8000001c8804>] filemap_fdatawrite+0x24/0x2c > [<ffff80000027e7a4>] fdatawrite_one_bdev+0x20/0x28 > [<ffff800000288b44>] iterate_bdevs+0xec/0x144 > [<ffff80000027eb50>] sys_sync+0x84/0xd0 > > Since mapping_cap_writeback_dirty() is always return true about > block device inodes, no need to check it if the inode is a block > device inode. > > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Wei Fang <fangwei1@xxxxxxxxxx> Good catch but I don't like sprinkling checks like this into the writeback code and furthermore we don't want to call into writeback code when block device is in the process of being destroyed which is what would happen with your patch. That is a bug waiting to happen... As I'm looking into the code, we need a serialization between bdev writeback and blkdev_put(). That should be doable if we use writeback_single_inode() for writing bdev inode instead of simple filemap_fdatawrite() and then use inode_wait_for_writeback() in blkdev_put() but it needs some careful thought. Frankly that whole idea of tearing block devices down on last close is a major headache and keeps biting us. I'm wondering whether it is still worth it these days... Honza > --- > mm/filemap.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 235021e..d607677 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -334,8 +334,9 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start, > .range_end = end, > }; > > - if (!mapping_cap_writeback_dirty(mapping)) > - return 0; > + if (!sb_is_blkdev_sb(mapping->host->i_sb)) > + if (!mapping_cap_writeback_dirty(mapping)) > + return 0; > > wbc_attach_fdatawrite_inode(&wbc, mapping->host); > ret = do_writepages(mapping, &wbc); > -- > 2.4.11 > -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR -- 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