Re: move the bdi from the request_queue to the gendisk

[Jan Kara <jack@suse.cz>]

On Tue 10-08-21 22:02:56, Christoph Hellwig wrote:
> On Tue, Aug 10, 2021 at 03:36:39PM -0400, Qian Cai wrote:
> > 
> > 
> > On 8/9/2021 10:17 AM, Christoph Hellwig wrote:
> > > Hi Jens,
> > > 
> > > this series moves the pointer to the bdi from the request_queue
> > > to the bdi, better matching the life time rules of the different
> > > objects.
> > 
> > Reverting this series fixed an use-after-free in bdev_evict_inode().
> 
> Please try the patch below as a band-aid.  Although the proper fix is
> that non-default bdi_writeback structures grab a reference to the bdi,
> as this was a landmine that might have already caused spurious issues
> before.

Well, non-default bdi_writeback structures do hold bdi reference - see
wb_exit() which drops the reference. I think the problem rather was that a
block device's inode->i_wb was pointing to the default bdi_writeback
structure and that got freed after bdi_put() before block device inode was
shutdown through bdput()... So what I think we need is that if the inode
references the default writeback structure, it actually holds a reference
to the bdi.

								Honza
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index f8def1129501..2e4a9d187196 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1086,7 +1086,6 @@ static void disk_release(struct device *dev)
>  
>  	might_sleep();
>  
> -	bdi_put(disk->bdi);
>  	if (MAJOR(dev->devt) == BLOCK_EXT_MAJOR)
>  		blk_free_ext_minor(MINOR(dev->devt));
>  	disk_release_events(disk);
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 7c969f81327a..c6087dbae6cf 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -849,11 +849,15 @@ static void init_once(void *data)
>  
>  static void bdev_evict_inode(struct inode *inode)
>  {
> +	struct block_device *bdev = I_BDEV(inode);
> +
>  	truncate_inode_pages_final(&inode->i_data);
>  	invalidate_inode_buffers(inode); /* is it needed here? */
>  	clear_inode(inode);
>  	/* Detach inode from wb early as bdi_put() may free bdi->wb */
>  	inode_detach_wb(inode);
> +	if (!bdev_is_partition(bdev))
> +		bdi_put(bdev->bd_disk->bdi);
>  }
>  
>  static const struct super_operations bdev_sops = {
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR