[PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Changes in v3:
 - add ack tag from Tejun for patch 1,2
 - as suggested by Tejun, update commit message and comments in patch 3

The problem was found in iocost orignally([1]) that ioc can be freed in
ioc_pd_free(). And later we found that there are more problem in
iocost([2]).

After some discussion, as suggested by Tejun([3]), we decide to fix the
problem that parent pd can be freed before child pd in cgroup layer
first. And the problem in [1] will be fixed later if this patchset is
applied.

[1] https://lore.kernel.org/all/20221130132156.2836184-8-linan122-hv44wF8Li93QT0dZR+AlfA@public.gmane.org/
[2] https://lore.kernel.org/all/aa924294-2f54-1b53-fc6e-e4f8fa019b14-XF6JlduFytWkHkcT6e4Xnw@public.gmane.org/
[3] https://lore.kernel.org/all/20221227125502.541931-1-yukuai1-XF6JlduFytWkHkcT6e4Xnw@public.gmane.org/

Yu Kuai (3):
  blk-cgroup: dropping parent refcount after pd_free_fn() is done
  blk-cgroup: support to track if policy is online
  blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and
    blkcg_deactivate_policy()

 block/blk-cgroup.c     | 63 ++++++++++++++++++++++++++++++++----------
 block/blk-cgroup.h     |  1 +
 include/linux/blkdev.h |  1 +
 3 files changed, 50 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH -next v3 2/3] blk-cgroup: support to track if policy is online

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

A new field 'online' is added to blkg_policy_data to fix following
2 problem:

1) In blkcg_activate_policy(), if pd_alloc_fn() with 'GFP_NOWAIT'
   failed, 'queue_lock' will be dropped and pd_alloc_fn() will try again
   without 'GFP_NOWAIT'. In the meantime, remove cgroup can race with
   it, and pd_offline_fn() will be called without pd_init_fn() and
   pd_online_fn(). This way null-ptr-deference can be triggered.

2) In order to synchronize pd_free_fn() from blkg_free_workfn() and
   blkcg_deactivate_policy(), 'list_del_init(&blkg->q_node)' will be
   delayed to blkg_free_workfn(), hence pd_offline_fn() can be called
   first in blkg_destroy(), and then blkcg_deactivate_policy() will
   call it again, we must prevent it.

The new field 'online' will be set after pd_online_fn() and will be
cleared after pd_offline_fn(), in the meantime pd_offline_fn() will only
be called if 'online' is set.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 block/blk-cgroup.c | 24 +++++++++++++++++-------
 block/blk-cgroup.h |  1 +
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index c6d7d1fce65a..75f3c4460715 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -288,6 +288,7 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct gendisk *disk,
 		blkg->pd[i] = pd;
 		pd->blkg = blkg;
 		pd->plid = i;
+		pd->online = false;
 	}
 
 	return blkg;
@@ -359,8 +360,11 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg, struct gendisk *disk,
 		for (i = 0; i < BLKCG_MAX_POLS; i++) {
 			struct blkcg_policy *pol = blkcg_policy[i];
 
-			if (blkg->pd[i] && pol->pd_online_fn)
-				pol->pd_online_fn(blkg->pd[i]);
+			if (blkg->pd[i]) {
+				if (pol->pd_online_fn)
+					pol->pd_online_fn(blkg->pd[i]);
+				blkg->pd[i]->online = true;
+			}
 		}
 	}
 	blkg->online = true;
@@ -465,8 +469,11 @@ static void blkg_destroy(struct blkcg_gq *blkg)
 	for (i = 0; i < BLKCG_MAX_POLS; i++) {
 		struct blkcg_policy *pol = blkcg_policy[i];
 
-		if (blkg->pd[i] && pol->pd_offline_fn)
-			pol->pd_offline_fn(blkg->pd[i]);
+		if (blkg->pd[i] && blkg->pd[i]->online) {
+			if (pol->pd_offline_fn)
+				pol->pd_offline_fn(blkg->pd[i]);
+			blkg->pd[i]->online = false;
+		}
 	}
 
 	blkg->online = false;
@@ -1448,6 +1455,7 @@ int blkcg_activate_policy(struct request_queue *q,
 		blkg->pd[pol->plid] = pd;
 		pd->blkg = blkg;
 		pd->plid = pol->plid;
+		pd->online = false;
 	}
 
 	/* all allocated, init in the same order */
@@ -1455,9 +1463,11 @@ int blkcg_activate_policy(struct request_queue *q,
 		list_for_each_entry_reverse(blkg, &q->blkg_list, q_node)
 			pol->pd_init_fn(blkg->pd[pol->plid]);
 
-	if (pol->pd_online_fn)
-		list_for_each_entry_reverse(blkg, &q->blkg_list, q_node)
+	list_for_each_entry_reverse(blkg, &q->blkg_list, q_node) {
+		if (pol->pd_online_fn)
 			pol->pd_online_fn(blkg->pd[pol->plid]);
+		blkg->pd[pol->plid]->online = true;
+	}
 
 	__set_bit(pol->plid, q->blkcg_pols);
 	ret = 0;
@@ -1519,7 +1529,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
 
 		spin_lock(&blkcg->lock);
 		if (blkg->pd[pol->plid]) {
-			if (pol->pd_offline_fn)
+			if (blkg->pd[pol->plid]->online && pol->pd_offline_fn)
 				pol->pd_offline_fn(blkg->pd[pol->plid]);
 			pol->pd_free_fn(blkg->pd[pol->plid]);
 			blkg->pd[pol->plid] = NULL;
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 1e94e404eaa8..b13ee84f358e 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -135,6 +135,7 @@ struct blkg_policy_data {
 	/* the blkg and policy id this per-policy data belongs to */
 	struct blkcg_gq			*blkg;
 	int				plid;
+	bool				online;
 };
 
 /*
-- 
2.31.1

Re: [PATCH -next v3 2/3] blk-cgroup: support to track if policy is online

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

[PATCH -next v3 3/3] blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently parent pd can be freed before child pd:

t1: remove cgroup C1
blkcg_destroy_blkgs
 blkg_destroy
  list_del_init(&blkg->q_node)
  // remove blkg from queue list
  percpu_ref_kill(&blkg->refcnt)
   blkg_release
    call_rcu

t2: from t1
__blkg_release
 blkg_free
  schedule_work
			t4: deactivate policy
			blkcg_deactivate_policy
			 pd_free_fn
			 // parent of C1 is freed first
t3: from t2
 blkg_free_workfn
  pd_free_fn

If policy(for example, ioc_timer_fn() from iocost) access parent pd from
child pd after pd_offline_fn(), then UAF can be triggered.

Fix the problem by delaying 'list_del_init(&blkg->q_node)' from
blkg_destroy() to blkg_free_workfn(), and using a new disk level mutex to
synchronize blkg_free_workfn() and blkcg_deactivate_policy().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.c     | 35 +++++++++++++++++++++++++++++------
 include/linux/blkdev.h |  1 +
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 75f3c4460715..cb110fc51940 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -118,16 +118,32 @@ static void blkg_free_workfn(struct work_struct *work)
 {
 	struct blkcg_gq *blkg = container_of(work, struct blkcg_gq,
 					     free_work);
+	struct request_queue *q = blkg->q;
 	int i;
 
+	/*
+	 * pd_free_fn() can also be called from blkcg_deactivate_policy(),
+	 * in order to make sure pd_free_fn() is called in order, the deletion
+	 * of the list blkg->q_node is delayed to here from blkg_destroy(), and
+	 * blkcg_mutex is used to synchronize blkg_free_workfn() and
+	 * blkcg_deactivate_policy().
+	 */
+	if (q)
+		mutex_lock(&q->blkcg_mutex);
+
 	for (i = 0; i < BLKCG_MAX_POLS; i++)
 		if (blkg->pd[i])
 			blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
 
 	if (blkg->parent)
 		blkg_put(blkg->parent);
-	if (blkg->q)
-		blk_put_queue(blkg->q);
+
+	if (q) {
+		list_del_init(&blkg->q_node);
+		mutex_unlock(&q->blkcg_mutex);
+		blk_put_queue(q);
+	}
+
 	free_percpu(blkg->iostat_cpu);
 	percpu_ref_exit(&blkg->refcnt);
 	kfree(blkg);
@@ -462,9 +478,14 @@ static void blkg_destroy(struct blkcg_gq *blkg)
 	lockdep_assert_held(&blkg->q->queue_lock);
 	lockdep_assert_held(&blkcg->lock);
 
-	/* Something wrong if we are trying to remove same group twice */
-	WARN_ON_ONCE(list_empty(&blkg->q_node));
-	WARN_ON_ONCE(hlist_unhashed(&blkg->blkcg_node));
+	/*
+	 * blkg stays on the queue list until blkg_free_workfn(), see details in
+	 * blkg_free_workfn(), hence this function can be called from
+	 * blkcg_destroy_blkgs() first and again from blkg_destroy_all() before
+	 * blkg_free_workfn().
+	 */
+	if (hlist_unhashed(&blkg->blkcg_node))
+		return;
 
 	for (i = 0; i < BLKCG_MAX_POLS; i++) {
 		struct blkcg_policy *pol = blkcg_policy[i];
@@ -479,7 +500,6 @@ static void blkg_destroy(struct blkcg_gq *blkg)
 	blkg->online = false;
 
 	radix_tree_delete(&blkcg->blkg_tree, blkg->q->id);
-	list_del_init(&blkg->q_node);
 	hlist_del_init_rcu(&blkg->blkcg_node);
 
 	/*
@@ -1280,6 +1300,7 @@ int blkcg_init_disk(struct gendisk *disk)
 	int ret;
 
 	INIT_LIST_HEAD(&q->blkg_list);
+	mutex_init(&q->blkcg_mutex);
 
 	new_blkg = blkg_alloc(&blkcg_root, disk, GFP_KERNEL);
 	if (!new_blkg)
@@ -1520,6 +1541,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
 	if (queue_is_mq(q))
 		blk_mq_freeze_queue(q);
 
+	mutex_lock(&q->blkcg_mutex);
 	spin_lock_irq(&q->queue_lock);
 
 	__clear_bit(pol->plid, q->blkcg_pols);
@@ -1538,6 +1560,7 @@ void blkcg_deactivate_policy(struct request_queue *q,
 	}
 
 	spin_unlock_irq(&q->queue_lock);
+	mutex_unlock(&q->blkcg_mutex);
 
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b87ed829ab94..53ae0a7fe377 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -485,6 +485,7 @@ struct request_queue {
 	DECLARE_BITMAP		(blkcg_pols, BLKCG_MAX_POLS);
 	struct blkcg_gq		*root_blkg;
 	struct list_head	blkg_list;
+	struct mutex		blkcg_mutex;
 #endif
 
 	struct queue_limits	limits;
-- 
2.31.1

Re: [PATCH -next v3 3/3] blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()

[Tejun Heo]

On Thu, Jan 19, 2023 at 07:03:50PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently parent pd can be freed before child pd:
> 
> t1: remove cgroup C1
> blkcg_destroy_blkgs
>  blkg_destroy
>   list_del_init(&blkg->q_node)
>   // remove blkg from queue list
>   percpu_ref_kill(&blkg->refcnt)
>    blkg_release
>     call_rcu
> 
> t2: from t1
> __blkg_release
>  blkg_free
>   schedule_work
> 			t4: deactivate policy
> 			blkcg_deactivate_policy
> 			 pd_free_fn
> 			 // parent of C1 is freed first
> t3: from t2
>  blkg_free_workfn
>   pd_free_fn
> 
> If policy(for example, ioc_timer_fn() from iocost) access parent pd from
> child pd after pd_offline_fn(), then UAF can be triggered.
> 
> Fix the problem by delaying 'list_del_init(&blkg->q_node)' from
> blkg_destroy() to blkg_free_workfn(), and using a new disk level mutex to
> synchronize blkg_free_workfn() and blkcg_deactivate_policy().
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH -next v3 3/3] blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()

[Christoph Hellwig]

The this change itself looks good, but it will clash badly with my
"switch blk-cgroup to work on gendisk" series:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Can I get some reviews for that series once I rebase and repost it
after this series is merged?

[PATCH -next v3 1/3] blk-cgroup: dropping parent refcount after pd_free_fn() is done

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Some cgroup policies will access parent pd through child pd even
after pd_offline_fn() is done. If pd_free_fn() for parent is called
before child, then UAF can be triggered. Hence it's better to guarantee
the order of pd_free_fn().

Currently refcount of parent blkg is dropped in __blkg_release(), which
is before pd_free_fn() is called in blkg_free_work_fn() while
blkg_free_work_fn() is called asynchronously.

This patch make sure pd_free_fn() called from removing cgroup is ordered
by delaying dropping parent refcount after calling pd_free_fn() for
child.

BTW, pd_free_fn() will also be called from blkcg_deactivate_policy()
from deleting device, and following patches will guarantee the order.

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 block/blk-cgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 4c94a6560f62..c6d7d1fce65a 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -124,6 +124,8 @@ static void blkg_free_workfn(struct work_struct *work)
 		if (blkg->pd[i])
 			blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
 
+	if (blkg->parent)
+		blkg_put(blkg->parent);
 	if (blkg->q)
 		blk_put_queue(blkg->q);
 	free_percpu(blkg->iostat_cpu);
@@ -158,8 +160,6 @@ static void __blkg_release(struct rcu_head *rcu)
 
 	/* release the blkcg and parent blkg refs this blkg has been holding */
 	css_put(&blkg->blkcg->css);
-	if (blkg->parent)
-		blkg_put(blkg->parent);
 	blkg_free(blkg);
 }
 
-- 
2.31.1

Re: [PATCH -next v3 1/3] blk-cgroup: dropping parent refcount after pd_free_fn() is done

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

Re: [PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[Jens Axboe]

On 1/19/23 4:03 AM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Changes in v3:
>  - add ack tag from Tejun for patch 1,2
>  - as suggested by Tejun, update commit message and comments in patch 3
> 
> The problem was found in iocost orignally([1]) that ioc can be freed in
> ioc_pd_free(). And later we found that there are more problem in
> iocost([2]).
> 
> After some discussion, as suggested by Tejun([3]), we decide to fix the
> problem that parent pd can be freed before child pd in cgroup layer
> first. And the problem in [1] will be fixed later if this patchset is
> applied.

Doesn't apply against for-6.3/block (or linux-next or my for-next, for
that matter). Can you resend a tested one against for-6.3/block?

-- 
Jens Axboe

Re: [PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[Yu Kuai]

Hi, Jens

在 2023/01/20 2:54, Jens Axboe 写道:
> On 1/19/23 4:03 AM, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>
>> Changes in v3:
>>   - add ack tag from Tejun for patch 1,2
>>   - as suggested by Tejun, update commit message and comments in patch 3
>>
>> The problem was found in iocost orignally([1]) that ioc can be freed in
>> ioc_pd_free(). And later we found that there are more problem in
>> iocost([2]).
>>
>> After some discussion, as suggested by Tejun([3]), we decide to fix the
>> problem that parent pd can be freed before child pd in cgroup layer
>> first. And the problem in [1] will be fixed later if this patchset is
>> applied.
> 
> Doesn't apply against for-6.3/block (or linux-next or my for-next, for
> that matter). Can you resend a tested one against for-6.3/block?
> 

This is weird, I just test latest linux-next, and I can apply this
patchset on the top of following commit:

For latest for-6.3/block, this patch 2 can't be applied because
following commit is not here:

e3ff8887e7db blk-cgroup: fix missing pd_online_fn() while activating policy

But this patch is already merged into 6.2-rc5.

Thanks,
Kuai

Re: [PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[Jens Axboe]

On 1/28/23 11:06 PM, Yu Kuai wrote:
> Hi, Jens
> 
> 在 2023/01/20 2:54, Jens Axboe 写道:
>> On 1/19/23 4:03 AM, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Changes in v3:
>>>   - add ack tag from Tejun for patch 1,2
>>>   - as suggested by Tejun, update commit message and comments in patch 3
>>>
>>> The problem was found in iocost orignally([1]) that ioc can be freed in
>>> ioc_pd_free(). And later we found that there are more problem in
>>> iocost([2]).
>>>
>>> After some discussion, as suggested by Tejun([3]), we decide to fix the
>>> problem that parent pd can be freed before child pd in cgroup layer
>>> first. And the problem in [1] will be fixed later if this patchset is
>>> applied.
>>
>> Doesn't apply against for-6.3/block (or linux-next or my for-next, for
>> that matter). Can you resend a tested one against for-6.3/block?
>>
> 
> This is weird, I just test latest linux-next, and I can apply this
> patchset on the top of following commit:
> 
> For latest for-6.3/block, this patch 2 can't be applied because
> following commit is not here:
> 
> e3ff8887e7db blk-cgroup: fix missing pd_online_fn() while activating policy
> 
> But this patch is already merged into 6.2-rc5.

Since I have one more conflict, I think we'll just rebase for-6.3/block
when -rc6 is out, and then it should apply cleanly.

-- 
Jens Axboe

Re: [PATCH -next v3 0/3] blk-cgroup: make sure pd_free_fn() is called in order

[Jens Axboe]

On Thu, 19 Jan 2023 19:03:47 +0800, Yu Kuai wrote:
> Changes in v3:
>  - add ack tag from Tejun for patch 1,2
>  - as suggested by Tejun, update commit message and comments in patch 3
> 
> The problem was found in iocost orignally([1]) that ioc can be freed in
> ioc_pd_free(). And later we found that there are more problem in
> iocost([2]).
> 
> [...]

Applied, thanks!

[1/3] blk-cgroup: dropping parent refcount after pd_free_fn() is done
      commit: c7241babf0855d8a6180cd1743ff0ec34de40b4e
[2/3] blk-cgroup: support to track if policy is online
      commit: dfd6200a095440b663099d8d42f1efb0175a1ce3
[3/3] blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()
      commit: f1c006f1c6850c14040f8337753a63119bba39b9

Best regards,
-- 
Jens Axboe