Re: [PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Jan Kara]

On Tue 08-09-15 12:20:22, Tejun Heo wrote:
> bdi's are initialized in two steps, bdi_init() and bdi_register(), but
> destroyed in a single step by bdi_destroy() which, for a bdi embedded
> in a request_queue, is called during blk_cleanup_queue() which makes
> the queue invisible and starts the draining of remaining usages.
> 
> A request_queue's user can access the congestion state of the embedded
> bdi as long as it holds a reference to the queue.  As such, it may
> access the congested state of a queue which finished
> blk_cleanup_queue() but hasn't reached blk_release_queue() yet.
> Because the congested state was embedded in backing_dev_info which in
> turn is embedded in request_queue, accessing the congested state after
> bdi_destroy() was called was fine.  The bdi was destroyed but the
> memory region for the congested state remained accessible till the
> queue got released.
> 
> a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in
> bdi_writeback") changed the situation.  Now, the root congested state
> which is expected to be pinned while request_queue remains accessible
> is separately reference counted and the base ref is put during
> bdi_destroy().  This means that the root congested state may go away
> prematurely while the queue is between bdi_dstroy() and
> blk_cleanup_queue(), which was detected by Andrey's KASAN tests.
> 
> The root cause of this problem is that bdi doesn't distinguish the two
> steps of destruction, unregistration and release, and now the root
> congested state actually requires a separate release step.  To fix the
> issue, this patch separates out bdi_unregister() and bdi_exit() from
> bdi_destroy().  bdi_unregister() is called from blk_cleanup_queue()
> and bdi_exit() from blk_release_queue().  bdi_destroy() is now just a
> simple wrapper calling the two steps back-to-back.
> 
> While at it, the prototype of bdi_destroy() is moved right below
> bdi_setup_and_register() so that the counterpart operations are
> located together.
> 
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
> Cc: stable@xxxxxxxxxxxxxxx # v4.2+
> Reported-and-tested-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@xxxxxxxxxxxxxx

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@xxxxxxxx>

								Honza

> ---
>  block/blk-core.c            |    2 +-
>  block/blk-sysfs.c           |    1 +
>  include/linux/backing-dev.h |    6 +++++-
>  mm/backing-dev.c            |   12 +++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..ae49240 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
>  		q->queue_lock = &q->__queue_lock;
>  	spin_unlock_irq(lock);
>  
> -	bdi_destroy(&q->backing_dev_info);
> +	bdi_unregister(&q->backing_dev_info);
>  
>  	/* @q is and will stay empty, shutdown and put */
>  	blk_put_queue(q);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 3e44a9d..07b42f5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
>  	struct request_queue *q =
>  		container_of(kobj, struct request_queue, kobj);
>  
> +	bdi_exit(&q->backing_dev_info);
>  	blkcg_exit_queue(q);
>  
>  	if (q->elevator) {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 0fe9df9..fe0ab98 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -18,13 +18,17 @@
>  #include <linux/slab.h>
>  
>  int __must_check bdi_init(struct backing_dev_info *bdi);
> -void bdi_destroy(struct backing_dev_info *bdi);
> +void bdi_exit(struct backing_dev_info *bdi);
>  
>  __printf(3, 4)
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  		const char *fmt, ...);
>  int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> +void bdi_unregister(struct backing_dev_info *bdi);
> +
>  int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> +void bdi_destroy(struct backing_dev_info *bdi);
> +
>  void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
>  			bool range_cyclic, enum wb_reason reason);
>  void wb_start_background_writeback(struct bdi_writeback *wb);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..dc07d88 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
>  	synchronize_rcu_expedited();
>  }
>  
> -void bdi_destroy(struct backing_dev_info *bdi)
> +void bdi_unregister(struct backing_dev_info *bdi)
>  {
>  	/* make sure nobody finds us on the bdi_list anymore */
>  	bdi_remove_from_list(bdi);
> @@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
> +}
>  
> +void bdi_exit(struct backing_dev_info *bdi)
> +{
> +	WARN_ON_ONCE(bdi->dev);
>  	wb_exit(&bdi->wb);
>  }
> +
> +void bdi_destroy(struct backing_dev_info *bdi)
> +{
> +	bdi_unregister(bdi);
> +	bdi_exit(bdi);
> +}
>  EXPORT_SYMBOL(bdi_destroy);
>  
>  /*
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/