[PATCH v2 -next] blk-cgroup: synchoronize blkg creation against policy deactivation

Out test report a null pointer dereference:

[  168.534653] ==================================================================
[  168.535614] Disabling lock debugging due to kernel taint
[  168.536346] BUG: kernel NULL pointer dereference, address: 0000000000000008
[  168.537274] #PF: supervisor read access in kernel mode
[  168.537964] #PF: error_code(0x0000) - not-present page
[  168.538667] PGD 0 P4D 0
[  168.539025] Oops: 0000 [#1] PREEMPT SMP KASAN
[  168.539656] CPU: 13 PID: 759 Comm: bash Tainted: G    B             5.15.0-rc2-next-202100
[  168.540954] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_0738364
[  168.542736] RIP: 0010:bfq_pd_init+0x88/0x1e0
[  168.543318] Code: 98 00 00 00 e8 c9 e4 5b ff 4c 8b 65 00 49 8d 7c 24 08 e8 bb e4 5b ff 4d0
[  168.545803] RSP: 0018:ffff88817095f9c0 EFLAGS: 00010002
[  168.546497] RAX: 0000000000000001 RBX: ffff888101a1c000 RCX: 0000000000000000
[  168.547438] RDX: 0000000000000003 RSI: 0000000000000002 RDI: ffff888106553428
[  168.548402] RBP: ffff888106553400 R08: ffffffff961bcaf4 R09: 0000000000000001
[  168.549365] R10: ffffffffa2e16c27 R11: fffffbfff45c2d84 R12: 0000000000000000
[  168.550291] R13: ffff888101a1c098 R14: ffff88810c7a08c8 R15: ffffffffa55541a0
[  168.551221] FS:  00007fac75227700(0000) GS:ffff88839ba80000(0000) knlGS:0000000000000000
[  168.552278] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  168.553040] CR2: 0000000000000008 CR3: 0000000165ce7000 CR4: 00000000000006e0
[  168.554000] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  168.554929] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  168.555888] Call Trace:
[  168.556221]  <TASK>
[  168.556510]  blkg_create+0x1c0/0x8c0
[  168.556989]  blkg_conf_prep+0x574/0x650
[  168.557502]  ? stack_trace_save+0x99/0xd0
[  168.558033]  ? blkcg_conf_open_bdev+0x1b0/0x1b0
[  168.558629]  tg_set_conf.constprop.0+0xb9/0x280
[  168.559231]  ? kasan_set_track+0x29/0x40
[  168.559758]  ? kasan_set_free_info+0x30/0x60
[  168.560344]  ? tg_set_limit+0xae0/0xae0
[  168.560853]  ? do_sys_openat2+0x33b/0x640
[  168.561383]  ? do_sys_open+0xa2/0x100
[  168.561877]  ? __x64_sys_open+0x4e/0x60
[  168.562383]  ? __kasan_check_write+0x20/0x30
[  168.562951]  ? copyin+0x48/0x70
[  168.563390]  ? _copy_from_iter+0x234/0x9e0

75 more lines

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index eb48090eefce..043f85a1e05e 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c

@@ -621,6 +621,7 @@ struct block_device *blkcg_conf_open_bdev(char **inputp)
  */
 int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 		   char *input, struct blkg_conf_ctx *ctx)
+	__acquires(&bdev->bd_disk->queue->blkg_lock)
 	__acquires(rcu) __acquires(&bdev->bd_disk->queue->queue_lock)
 {
 	struct block_device *bdev;

@@ -634,6 +635,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 
 	q = bdev->bd_disk->queue;
 
+	mutex_lock(&q->blkg_lock);
 	rcu_read_lock();
 	spin_lock_irq(&q->queue_lock);
 

@@ -713,6 +715,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
 fail_unlock:
 	spin_unlock_irq(&q->queue_lock);
 	rcu_read_unlock();
+	mutex_unlock(&q->blkg_lock);
 fail:
 	blkdev_put_no_open(bdev);
 	/*

@@ -738,9 +741,11 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep);
  */
 void blkg_conf_finish(struct blkg_conf_ctx *ctx)
 	__releases(&ctx->bdev->bd_disk->queue->queue_lock) __releases(rcu)
+	__releases(&ctx->bdev->bd_disk->queue->blkg_lock)
 {
 	spin_unlock_irq(&ctx->bdev->bd_disk->queue->queue_lock);
 	rcu_read_unlock();
+	mutex_unlock(&ctx->bdev->bd_disk->queue->blkg_lock);
 	blkdev_put_no_open(ctx->bdev);
 }
 EXPORT_SYMBOL_GPL(blkg_conf_finish);

@@ -1401,6 +1406,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
 	if (queue_is_mq(q))
 		blk_mq_freeze_queue(q);
 
+	mutex_lock(&q->blkg_lock);
 	spin_lock_irq(&q->queue_lock);
 
 	__clear_bit(pol->plid, q->blkcg_pols);

@@ -1419,6 +1425,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
 	}
 
 	spin_unlock_irq(&q->queue_lock);
+	mutex_unlock(&q->blkg_lock);
 
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q);

diff --git a/block/blk-core.c b/block/blk-core.c
index d83e56b2f64e..f37fc03b1113 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -544,6 +544,7 @@ struct request_queue *blk_alloc_queue(int node_id)
 	INIT_LIST_HEAD(&q->icq_list);
 #ifdef CONFIG_BLK_CGROUP
 	INIT_LIST_HEAD(&q->blkg_list);
+	mutex_init(&q->blkg_lock);
 #endif
 
 	kobject_init(&q->kobj, &blk_queue_ktype);

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b19172db7eef..c2d5be0f5f37 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -245,6 +245,11 @@ struct request_queue {
 	DECLARE_BITMAP		(blkcg_pols, BLKCG_MAX_POLS);
 	struct blkcg_gq		*root_blkg;
 	struct list_head	blkg_list;
+	/*
+	 * used to synchronize blkg allocation and initialization against
+	 * policy deactivation.
+	 */
+	struct mutex		blkg_lock;
 #endif
 
 	struct queue_limits	limits;

-- 
2.31.1

On Wed, Oct 13, 2021 at 08:44:56PM +0800, Yu Kuai wrote:

Given that deactivation drains q_usage_counter through
blk_mq_freeze_queue(), can't the blkg_conf_prep() just pin the usage count?

Thanks.

-- 
tejun

On 2021/10/15 7:02, Tejun Heo wrote:

On Wed, Oct 13, 2021 at 08:44:56PM +0800, Yu Kuai wrote:

quoted

@@ -1401,6 +1406,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
  	if (queue_is_mq(q))
  		blk_mq_freeze_queue(q);
  
+	mutex_lock(&q->blkg_lock);
  	spin_lock_irq(&q->queue_lock);

Given that deactivation drains q_usage_counter through
blk_mq_freeze_queue(), can't the blkg_conf_prep() just pin the usage count?

In fact, the caller of blkcg_deactivate_policy() aready freeze the
queue, either from blk_cleanup_queue() or from elevator_switch().

Grab q_usage_counter in blk_conf_prep() can prevent concurrent with
blkcg_deactivate_policy(), I'm afraid will it be too much since it
will also be synchoronized with all the other freeze/unfreeze queues?

Thanks,
Kuai