2018-04-19 04:09:25

by Jiang Biao

Subject: [PATCH 2/2] blkcg: init root blkcg_gq under lock

The initializing of q->root_blkg is currently outside of queue lock
and rcu, so the blkg may be destroied before the initializing, which
may cause dangling/null references. On the other side, the destroys
of blkg are protected by queue lock or rcu. Put the initializing
inside the queue lock and rcu to make it safer.

Signed-off-by: Jiang Biao <[email protected]>
Signed-off-by: Wen Yang <[email protected]>
CC: Tejun Heo <[email protected]>
CC: Jens Axboe <[email protected]>
---
block/blk-cgroup.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 07e3359..ec86837 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1138,18 +1138,16 @@ int blkcg_init_queue(struct request_queue *q)
rcu_read_lock();
spin_lock_irq(q->queue_lock);
blkg = blkg_create(&blkcg_root, q, new_blkg);
+ if (IS_ERR(blkg))
+ goto err_unlock;
+ q->root_blkg = blkg;
+ q->root_rl.blkg = blkg;
spin_unlock_irq(q->queue_lock);
rcu_read_unlock();

if (preloaded)
radix_tree_preload_end();

- if (IS_ERR(blkg))
- return PTR_ERR(blkg);
-
- q->root_blkg = blkg;
- q->root_rl.blkg = blkg;
-
ret = blk_throtl_init(q);
if (ret) {
spin_lock_irq(q->queue_lock);
@@ -1157,6 +1155,13 @@ int blkcg_init_queue(struct request_queue *q)
spin_unlock_irq(q->queue_lock);
}
return ret;
+
+err_unlock:
+ spin_unlock_irq(q->queue_lock);
+ rcu_read_unlock();
+ if (preloaded)
+ radix_tree_preload_end();
+ return PTR_ERR(blkg);
}

/**
--
2.7.4

2018-04-26 19:20:19

by Tejun Heo

Subject: Re: [PATCH 2/2] blkcg: init root blkcg_gq under lock

Hello,

On Thu, Apr 19, 2018 at 12:06:09PM +0800, Jiang Biao wrote:
> The initializing of q->root_blkg is currently outside of queue lock
> and rcu, so the blkg may be destroied before the initializing, which
> may cause dangling/null references. On the other side, the destroys
> of blkg are protected by queue lock or rcu. Put the initializing
> inside the queue lock and rcu to make it safer.

Hmm... I can't think of a scenario where the above could happen. The
queue is still being initialized and no one else has access to it, so
I can't see how it could be destroyed. Can you please describe the
sequence of events? Have you actually observed this happening?

Thanks.

--
tejun