[PATCH v5 1/3] blkcg: Introduce blkg_root_lookup()

[PATCH v5 1/3] blkcg: Introduce blkg_root_lookup()

[Bart Van Assche]

This new function will be used in a later patch to verify whether a
queue has been dissociated from the cgroup controller before being
released.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Alexandru Moise <00moses.alexander00@gmail.com>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Cc: <stable@vger.kernel.org>
---
 include/linux/blk-cgroup.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index f7b910768306..1361cfc9b878 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -341,6 +341,23 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg,
 	return __blkg_lookup(blkcg, q, false);
 }
 
+/**
+ * blkg_lookup - look up blkg for the specified request queue
+ * @q: request_queue of interest
+ *
+ * Lookup blkg for @q at the root level. See also blkg_lookup().
+ */
+static inline struct blkcg_gq *blkg_root_lookup(struct request_queue *q)
+{
+	struct blkcg_gq *blkg;
+
+	rcu_read_lock();
+	blkg = blkg_lookup(&blkcg_root, q);
+	rcu_read_unlock();
+
+	return blkg;
+}
+
 /**
  * blkg_to_pdata - get policy private data
  * @blkg: blkg of interest
@@ -864,6 +881,7 @@ static inline bool blk_cgroup_congested(void) { return false; }
 static inline void blkcg_schedule_throttle(struct request_queue *q, bool use_memdelay) { }
 
 static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, void *key) { return NULL; }
+static inline struct blkcg_gq *blkg_root_lookup(struct request_queue *q) { return NULL; }
 static inline int blkcg_init_queue(struct request_queue *q) { return 0; }
 static inline void blkcg_drain_queue(struct request_queue *q) { }
 static inline void blkcg_exit_queue(struct request_queue *q) { }
-- 
2.18.0

Re: [PATCH v5 1/3] blkcg: Introduce blkg_root_lookup()

[Tejun Heo]

Hello,

On Thu, Aug 09, 2018 at 07:53:36AM -0700, Bart Van Assche wrote:
> +/**
> + * blkg_lookup - look up blkg for the specified request queue
> + * @q: request_queue of interest
> + *
> + * Lookup blkg for @q at the root level. See also blkg_lookup().
> + */
> +static inline struct blkcg_gq *blkg_root_lookup(struct request_queue *q)
> +{
> +	struct blkcg_gq *blkg;
> +
> +	rcu_read_lock();
> +	blkg = blkg_lookup(&blkcg_root, q);
> +	rcu_read_unlock();
> +
> +	return blkg;
> +}

So, root blkg is always available as long as the request_queue is
alive.  Sth like the following would be simpler?

static inline struct blkcg_gq *blk_queue_root_blkg(struct request_queue *q)
{
	return q->root_blkg;
}

-- 
tejun

Re: [PATCH v5 1/3] blkcg: Introduce blkg_root_lookup()

[Bart Van Assche]

On Thu, 2018-08-09 at 12:56 -0700, Tejun Heo wrote:
> Hello,
> 
> On Thu, Aug 09, 2018 at 07:53:36AM -0700, Bart Van Assche wrote:
> > +/**
> > + * blkg_lookup - look up blkg for the specified request queue
> > + * @q: request_queue of interest
> > + *
> > + * Lookup blkg for @q at the root level. See also blkg_lookup().
> > + */
> > +static inline struct blkcg_gq *blkg_root_lookup(struct request_queue *q)
> > +{
> > +	struct blkcg_gq *blkg;
> > +
> > +	rcu_read_lock();
> > +	blkg = blkg_lookup(&blkcg_root, q);
> > +	rcu_read_unlock();
> > +
> > +	return blkg;
> > +}
> 
> So, root blkg is always available as long as the request_queue is
> alive.  Sth like the following would be simpler?
> 
> static inline struct blkcg_gq *blk_queue_root_blkg(struct request_queue *q)
> {
> 	return q->root_blkg;
> }

That would not only be simpler, it would also avoid that blk_queue_root_blkg()
returns NULL if bypass mode is enabled and the request queue is still associated
with the block cgroup controller. How do you want to proceed with this change?

Bart.

Re: [PATCH v5 1/3] blkcg: Introduce blkg_root_lookup()

[Jens Axboe]

On 8/9/18 2:17 PM, Bart Van Assche wrote:
> On Thu, 2018-08-09 at 12:56 -0700, Tejun Heo wrote:
>> Hello,
>>
>> On Thu, Aug 09, 2018 at 07:53:36AM -0700, Bart Van Assche wrote:
>>> +/**
>>> + * blkg_lookup - look up blkg for the specified request queue
>>> + * @q: request_queue of interest
>>> + *
>>> + * Lookup blkg for @q at the root level. See also blkg_lookup().
>>> + */
>>> +static inline struct blkcg_gq *blkg_root_lookup(struct request_queue *q)
>>> +{
>>> +	struct blkcg_gq *blkg;
>>> +
>>> +	rcu_read_lock();
>>> +	blkg = blkg_lookup(&blkcg_root, q);
>>> +	rcu_read_unlock();
>>> +
>>> +	return blkg;
>>> +}
>>
>> So, root blkg is always available as long as the request_queue is
>> alive.  Sth like the following would be simpler?
>>
>> static inline struct blkcg_gq *blk_queue_root_blkg(struct request_queue *q)
>> {
>> 	return q->root_blkg;
>> }
> 
> That would not only be simpler, it would also avoid that blk_queue_root_blkg()
> returns NULL if bypass mode is enabled and the request queue is still associated
> with the block cgroup controller. How do you want to proceed with this change?

Do a followup patch?

-- 
Jens Axboe

[PATCH v5 2/3] block: Introduce blk_exit_queue()

[Bart Van Assche]

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Alexandru Moise <00moses.alexander00@gmail.com>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Cc: <stable@vger.kernel.org>
---
 block/blk-core.c | 54 +++++++++++++++++++++++++++---------------------
 block/blk.h      |  1 +
 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2e054b65de42..55bc22ef2934 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -719,6 +719,35 @@ void blk_set_queue_dying(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_set_queue_dying);
 
+/* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
+void blk_exit_queue(struct request_queue *q)
+{
+	/*
+	 * Since the I/O scheduler exit code may access cgroup information,
+	 * perform I/O scheduler exit before disassociating from the block
+	 * cgroup controller.
+	 */
+	if (q->elevator) {
+		ioc_clear_queue(q);
+		elevator_exit(q, q->elevator);
+		q->elevator = NULL;
+	}
+
+	/*
+	 * Remove all references to @q from the block cgroup controller before
+	 * restoring @q->queue_lock to avoid that restoring this pointer causes
+	 * e.g. blkcg_print_blkgs() to crash.
+	 */
+	blkcg_exit_queue(q);
+
+	/*
+	 * Since the cgroup code may dereference the @q->backing_dev_info
+	 * pointer, only decrease its reference count after having removed the
+	 * association with the block cgroup controller.
+	 */
+	bdi_put(q->backing_dev_info);
+}
+
 /**
  * blk_cleanup_queue - shutdown a request queue
  * @q: request queue to shutdown
@@ -788,30 +817,7 @@ void blk_cleanup_queue(struct request_queue *q)
 	 */
 	WARN_ON_ONCE(q->kobj.state_in_sysfs);
 
-	/*
-	 * Since the I/O scheduler exit code may access cgroup information,
-	 * perform I/O scheduler exit before disassociating from the block
-	 * cgroup controller.
-	 */
-	if (q->elevator) {
-		ioc_clear_queue(q);
-		elevator_exit(q, q->elevator);
-		q->elevator = NULL;
-	}
-
-	/*
-	 * Remove all references to @q from the block cgroup controller before
-	 * restoring @q->queue_lock to avoid that restoring this pointer causes
-	 * e.g. blkcg_print_blkgs() to crash.
-	 */
-	blkcg_exit_queue(q);
-
-	/*
-	 * Since the cgroup code may dereference the @q->backing_dev_info
-	 * pointer, only decrease its reference count after having removed the
-	 * association with the block cgroup controller.
-	 */
-	bdi_put(q->backing_dev_info);
+	blk_exit_queue(q);
 
 	if (q->mq_ops)
 		blk_mq_free_queue(q);
diff --git a/block/blk.h b/block/blk.h
index 6adae8f94279..aeb8026f4d7b 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -130,6 +130,7 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 int blk_init_rl(struct request_list *rl, struct request_queue *q,
 		gfp_t gfp_mask);
 void blk_exit_rl(struct request_queue *q, struct request_list *rl);
+void blk_exit_queue(struct request_queue *q);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 			struct bio *bio);
 void blk_queue_bypass_start(struct request_queue *q);
-- 
2.18.0

[PATCH v5 3/3] block: Ensure that a request queue is dissociated from the cgroup controller

[Bart Van Assche]

Several block drivers call alloc_disk() followed by put_disk() if
something fails before device_add_disk() is called without calling
blk_cleanup_queue(). Make sure that also for this scenario a request
queue is dissociated from the cgroup controller. This patch avoids
that loading the parport_pc, paride and pf drivers triggers the
following kernel crash:

BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
Read of size 4 at addr 0000000000000008 by task modprobe/744
Call Trace:
dump_stack+0x9a/0xeb
kasan_report+0x139/0x350
pi_init+0x42e/0x580 [paride]
pf_init+0x2bb/0x1000 [pf]
do_one_initcall+0x8e/0x405
do_init_module+0xd9/0x2f2
load_module+0x3ab4/0x4700
SYSC_finit_module+0x176/0x1a0
do_syscall_64+0xee/0x2b0
entry_SYSCALL_64_after_hwframe+0x42/0xb7

Reported-by: Alexandru Moise <00moses.alexander00@gmail.com>
Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the block cgroup controller") # v4.17
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Tested-by: Alexandru Moise <00moses.alexander00@gmail.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Alexandru Moise <00moses.alexander00@gmail.com>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Cc: <stable@vger.kernel.org>
---
 block/blk-sysfs.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index ca1984ecbdeb..fcadea471779 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -802,6 +802,21 @@ static void __blk_release_queue(struct work_struct *work)
 		blk_stat_remove_callback(q, q->poll_cb);
 	blk_stat_free_callback(q->poll_cb);
 
+	if (!blk_queue_dead(q)) {
+		/*
+		 * Last reference was dropped without having called
+		 * blk_cleanup_queue().
+		 */
+		WARN_ONCE(blk_queue_init_done(q),
+			  "request queue %p has been registered but blk_cleanup_queue() has not been called for that queue\n",
+			  q);
+		blk_exit_queue(q);
+	}
+
+	WARN(blkg_root_lookup(q),
+	     "request queue %p is being released but it has not yet been removed from the blkcg controller\n",
+	     q);
+
 	blk_free_queue_stats(q->stats);
 
 	blk_exit_rl(q, &q->root_rl);
-- 
2.18.0