[PATCH v2 0/3] blk-cgroup: cleanup and bugfixs in blk-cgroup

[PATCH v2 0/3] blk-cgroup: cleanup and bugfixs in blk-cgroup

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

Changes in v2:
 - Move the cleanup patch to the end of the series and added Reviewed-by
   tags
 - Add blkcg_mutex protection for blkg_list traversal in
   blkcg_activate_policy() in patch 1
 - Add Reviewed-by tags to patch 2

v1:
https://lore.kernel.org/all/20260108014416.3656493-1-zhengqixing@huaweicloud.com/

Zheng Qixing (3):
  blk-cgroup: fix race between policy activation and blkg destruction
  blk-cgroup: skip dying blkg in blkcg_activate_policy()
  blk-cgroup: factor policy pd teardown loop into helper

 block/blk-cgroup.c | 64 +++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

When switching an IO scheduler on a block device, blkcg_activate_policy()
allocates blkg_policy_data (pd) for all blkgs attached to the queue.
However, blkcg_activate_policy() may race with concurrent blkcg deletion,
leading to use-after-free and memory leak issues.

The use-after-free occurs in the following race:

T1 (blkcg_activate_policy):
  - Successfully allocates pd for blkg1 (loop0->queue, blkcgA)
  - Fails to allocate pd for blkg2 (loop0->queue, blkcgB)
  - Enters the enomem rollback path to release blkg1 resources

T2 (blkcg deletion):
  - blkcgA is deleted concurrently
  - blkg1 is freed via blkg_free_workfn()
  - blkg1->pd is freed

T1 (continued):
  - Rollback path accesses blkg1->pd->online after pd is freed
  - Triggers use-after-free

In addition, blkg_free_workfn() frees pd before removing the blkg from
q->blkg_list. This allows blkcg_activate_policy() to allocate a new pd
for a blkg that is being destroyed, leaving the newly allocated pd
unreachable when the blkg is finally freed.

Fix these races by extending blkcg_mutex coverage to serialize
blkcg_activate_policy() rollback and blkg destruction, ensuring pd
lifecycle is synchronized with blkg list visibility.

Link: https://lore.kernel.org/all/20260108014416.3656493-3-zhengqixing@huaweicloud.com/
Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
---
 block/blk-cgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 3cffb68ba5d8..600f8c5843ea 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1596,6 +1596,8 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 
 	if (queue_is_mq(q))
 		memflags = blk_mq_freeze_queue(q);
+
+	mutex_lock(&q->blkcg_mutex);
 retry:
 	spin_lock_irq(&q->queue_lock);
 
@@ -1658,6 +1660,7 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 
 	spin_unlock_irq(&q->queue_lock);
 out:
+	mutex_unlock(&q->blkcg_mutex);
 	if (queue_is_mq(q))
 		blk_mq_unfreeze_queue(q, memflags);
 	if (pinned_blkg)
-- 
2.39.2

[PATCH v2 2/3] blk-cgroup: skip dying blkg in blkcg_activate_policy()

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

When switching IO schedulers on a block device, blkcg_activate_policy()
can race with concurrent blkcg deletion, leading to a use-after-free in
rcu_accelerate_cbs.

T1:                               T2:
		                  blkg_destroy
                 		  kill(&blkg->refcnt) // blkg->refcnt=1->0
				  blkg_release // call_rcu(__blkg_release)
                                  ...
				  blkg_free_workfn
                                  ->pd_free_fn(pd)
elv_iosched_store
elevator_switch
...
iterate blkg list
blkg_get(blkg) // blkg->refcnt=0->1
                                  list_del_init(&blkg->q_node)
blkg_put(pinned_blkg) // blkg->refcnt=1->0
blkg_release // call_rcu again
rcu_accelerate_cbs // uaf

Fix this by replacing blkg_get() with blkg_tryget(), which fails if
the blkg's refcount has already reached zero. If blkg_tryget() fails,
skip processing this blkg since it's already being destroyed.

Link: https://lore.kernel.org/all/20260108014416.3656493-4-zhengqixing@huaweicloud.com/
Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 600f8c5843ea..5dbc107eec53 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1622,9 +1622,10 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 			 * GFP_NOWAIT failed.  Free the existing one and
 			 * prealloc for @blkg w/ GFP_KERNEL.
 			 */
+			if (!blkg_tryget(blkg))
+				continue;
 			if (pinned_blkg)
 				blkg_put(pinned_blkg);
-			blkg_get(blkg);
 			pinned_blkg = blkg;
 
 			spin_unlock_irq(&q->queue_lock);
-- 
2.39.2

[PATCH v2 3/3] blk-cgroup: factor policy pd teardown loop into helper

[Zheng Qixing]

From: Zheng Qixing <zhengqixing@huawei.com>

Move the teardown sequence which offlines and frees per-policy
blkg_policy_data (pd) into a helper for readability.

No functional change intended.

Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Yu Kuai <yukuai@fnnas.com>
---
 block/blk-cgroup.c | 58 +++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 5dbc107eec53..78227ab0c1d7 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1559,6 +1559,31 @@ struct cgroup_subsys io_cgrp_subsys = {
 };
 EXPORT_SYMBOL_GPL(io_cgrp_subsys);
 
+/*
+ * Tear down per-blkg policy data for @pol on @q.
+ */
+static void blkcg_policy_teardown_pds(struct request_queue *q,
+				      const struct blkcg_policy *pol)
+{
+	struct blkcg_gq *blkg;
+
+	list_for_each_entry(blkg, &q->blkg_list, q_node) {
+		struct blkcg *blkcg = blkg->blkcg;
+		struct blkg_policy_data *pd;
+
+		spin_lock(&blkcg->lock);
+		pd = blkg->pd[pol->plid];
+		if (pd) {
+			if (pd->online && pol->pd_offline_fn)
+				pol->pd_offline_fn(pd);
+			pd->online = false;
+			pol->pd_free_fn(pd);
+			blkg->pd[pol->plid] = NULL;
+		}
+		spin_unlock(&blkcg->lock);
+	}
+}
+
 /**
  * blkcg_activate_policy - activate a blkcg policy on a gendisk
  * @disk: gendisk of interest
@@ -1673,21 +1698,7 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
 enomem:
 	/* alloc failed, take down everything */
 	spin_lock_irq(&q->queue_lock);
-	list_for_each_entry(blkg, &q->blkg_list, q_node) {
-		struct blkcg *blkcg = blkg->blkcg;
-		struct blkg_policy_data *pd;
-
-		spin_lock(&blkcg->lock);
-		pd = blkg->pd[pol->plid];
-		if (pd) {
-			if (pd->online && pol->pd_offline_fn)
-				pol->pd_offline_fn(pd);
-			pd->online = false;
-			pol->pd_free_fn(pd);
-			blkg->pd[pol->plid] = NULL;
-		}
-		spin_unlock(&blkcg->lock);
-	}
+	blkcg_policy_teardown_pds(q, pol);
 	spin_unlock_irq(&q->queue_lock);
 	ret = -ENOMEM;
 	goto out;
@@ -1706,7 +1717,6 @@ void blkcg_deactivate_policy(struct gendisk *disk,
 			     const struct blkcg_policy *pol)
 {
 	struct request_queue *q = disk->queue;
-	struct blkcg_gq *blkg;
 	unsigned int memflags;
 
 	if (!blkcg_policy_enabled(q, pol))
@@ -1717,22 +1727,8 @@ void blkcg_deactivate_policy(struct gendisk *disk,
 
 	mutex_lock(&q->blkcg_mutex);
 	spin_lock_irq(&q->queue_lock);
-
 	__clear_bit(pol->plid, q->blkcg_pols);
-
-	list_for_each_entry(blkg, &q->blkg_list, q_node) {
-		struct blkcg *blkcg = blkg->blkcg;
-
-		spin_lock(&blkcg->lock);
-		if (blkg->pd[pol->plid]) {
-			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;
-		}
-		spin_unlock(&blkcg->lock);
-	}
-
+	blkcg_policy_teardown_pds(q, pol);
 	spin_unlock_irq(&q->queue_lock);
 	mutex_unlock(&q->blkcg_mutex);
 
-- 
2.39.2

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

On Tue, Jan 13, 2026 at 02:10:33PM +0800, Zheng Qixing <zhengqixing@huaweicloud.com> wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> When switching an IO scheduler on a block device, blkcg_activate_policy()
> allocates blkg_policy_data (pd) for all blkgs attached to the queue.
> However, blkcg_activate_policy() may race with concurrent blkcg deletion,
> leading to use-after-free and memory leak issues.
> 
> The use-after-free occurs in the following race:
> 
> T1 (blkcg_activate_policy):
>   - Successfully allocates pd for blkg1 (loop0->queue, blkcgA)
>   - Fails to allocate pd for blkg2 (loop0->queue, blkcgB)
>   - Enters the enomem rollback path to release blkg1 resources
> 
> T2 (blkcg deletion):
>   - blkcgA is deleted concurrently
>   - blkg1 is freed via blkg_free_workfn()
>   - blkg1->pd is freed
> 
> T1 (continued):
>   - Rollback path accesses blkg1->pd->online after pd is freed

The rollback path is under q->queue_lock same like the list removal in
blkg_free_workfn().
Why is queue_lock not enough for synchronization in this case?

(BTW have you observed this case "naturally" or have you injected the
memory allocation failure?)


>   - Triggers use-after-free
> 
> In addition, blkg_free_workfn() frees pd before removing the blkg from
> q->blkg_list.

Yeah, this looks weirdly reversed.

> This allows blkcg_activate_policy() to allocate a new pd
> for a blkg that is being destroyed, leaving the newly allocated pd
> unreachable when the blkg is finally freed.
> 
> Fix these races by extending blkcg_mutex coverage to serialize
> blkcg_activate_policy() rollback and blkg destruction, ensuring pd
> lifecycle is synchronized with blkg list visibility.
> 
> Link: https://lore.kernel.org/all/20260108014416.3656493-3-zhengqixing@huaweicloud.com/
> Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH v2 2/3] blk-cgroup: skip dying blkg in blkcg_activate_policy()

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On Tue, Jan 13, 2026 at 02:10:34PM +0800, Zheng Qixing <zhengqixing@huaweicloud.com> wrote:
> From: Zheng Qixing <zhengqixing@huawei.com>
> 
> When switching IO schedulers on a block device, blkcg_activate_policy()
> can race with concurrent blkcg deletion, leading to a use-after-free in
> rcu_accelerate_cbs.
> 
> T1:                               T2:
> 		                  blkg_destroy
>                  		  kill(&blkg->refcnt) // blkg->refcnt=1->0
> 				  blkg_release // call_rcu(__blkg_release)
>                                   ...
> 				  blkg_free_workfn
>                                   ->pd_free_fn(pd)
> elv_iosched_store
> elevator_switch
> ...
> iterate blkg list
> blkg_get(blkg) // blkg->refcnt=0->1
>                                   list_del_init(&blkg->q_node)
> blkg_put(pinned_blkg) // blkg->refcnt=1->0
> blkg_release // call_rcu again
> rcu_accelerate_cbs // uaf
> 
> Fix this by replacing blkg_get() with blkg_tryget(), which fails if
> the blkg's refcount has already reached zero. If blkg_tryget() fails,
> skip processing this blkg since it's already being destroyed.
> 
> Link: https://lore.kernel.org/all/20260108014416.3656493-4-zhengqixing@huaweicloud.com/
> Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-cgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Michal Koutný <mkoutny@suse.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Zheng Qixing]

在 2026/1/14 18:40, Michal Koutný 写道:
> On Tue, Jan 13, 2026 at 02:10:33PM +0800, Zheng Qixing <zhengqixing@huaweicloud.com> wrote:
>> From: Zheng Qixing <zhengqixing@huawei.com>
>>
>> When switching an IO scheduler on a block device, blkcg_activate_policy()
>> allocates blkg_policy_data (pd) for all blkgs attached to the queue.
>> However, blkcg_activate_policy() may race with concurrent blkcg deletion,
>> leading to use-after-free and memory leak issues.
>>
>> The use-after-free occurs in the following race:
>>
>> T1 (blkcg_activate_policy):
>>    - Successfully allocates pd for blkg1 (loop0->queue, blkcgA)
>>    - Fails to allocate pd for blkg2 (loop0->queue, blkcgB)
>>    - Enters the enomem rollback path to release blkg1 resources
>>
>> T2 (blkcg deletion):
>>    - blkcgA is deleted concurrently
>>    - blkg1 is freed via blkg_free_workfn()
>>    - blkg1->pd is freed
>>
>> T1 (continued):
>>    - Rollback path accesses blkg1->pd->online after pd is freed
> The rollback path is under q->queue_lock same like the list removal in
> blkg_free_workfn().
> Why is queue_lock not enough for synchronization in this case?
>
> (BTW have you observed this case "naturally" or have you injected the
> memory allocation failure?)
>
Yes, this issue was discovered by injecting memory allocation failure at
->pd_alloc_fn(..., GFP_KERNEL) in blkcg_activate_policy().

In blkg_free_workfn(), q->queue_lock only protects the
list_del_init(&blkg->q_node). However, ->pd_free_fn() is called before
list_del_init(), meaning the pd is already freed before the blkg is removed
from the queue's list.

     blkcg_activate_policy()                  blkg_free_workfn()
     -------------------                          ------------------
     spin_lock(&q->queue_lock)
     ...
     if (!pd) {
         spin_unlock(&q->queue_lock)
         ...
         goto enomem
     }
     enomem:
         spin_lock(&q->queue_lock)
         if (pd) {
->pd_free_fn()  // pd freed
            pd->online // uaf
         ...
         }
spin_lock(&q->queue_lock)
list_del_init(&blkg->q_node)
spin_unlock(&q->queue_lock)
>>    - Triggers use-after-free
>>
>> In addition, blkg_free_workfn() frees pd before removing the blkg from
>> q->blkg_list.
> Yeah, this looks weirdly reversed.

Commit f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from 
blkg_free_workfn() and blkcg_deactivate_policy()") delays 
list_del_init(&blkg->q_node) until after pd_free_fn() in 
blkg_free_workfn(). This keeps blkgs visible in the queue list during 
policy deactivation, preventing parent policy data from being freed 
before child policy data and avoiding use-after-free.

Kind Regards,
Qixing

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Yu Kuai]

Hi,

You are sending to my invalid huawei email address, so I didn't see this patch.

在 2026/1/13 14:10, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
>
> When switching an IO scheduler on a block device, blkcg_activate_policy()
> allocates blkg_policy_data (pd) for all blkgs attached to the queue.
> However, blkcg_activate_policy() may race with concurrent blkcg deletion,
> leading to use-after-free and memory leak issues.
>
> The use-after-free occurs in the following race:
>
> T1 (blkcg_activate_policy):
>    - Successfully allocates pd for blkg1 (loop0->queue, blkcgA)
>    - Fails to allocate pd for blkg2 (loop0->queue, blkcgB)
>    - Enters the enomem rollback path to release blkg1 resources
>
> T2 (blkcg deletion):
>    - blkcgA is deleted concurrently
>    - blkg1 is freed via blkg_free_workfn()
>    - blkg1->pd is freed
>
> T1 (continued):
>    - Rollback path accesses blkg1->pd->online after pd is freed
>    - Triggers use-after-free
>
> In addition, blkg_free_workfn() frees pd before removing the blkg from
> q->blkg_list. This allows blkcg_activate_policy() to allocate a new pd
> for a blkg that is being destroyed, leaving the newly allocated pd
> unreachable when the blkg is finally freed.
>
> Fix these races by extending blkcg_mutex coverage to serialize
> blkcg_activate_policy() rollback and blkg destruction, ensuring pd
> lifecycle is synchronized with blkg list visibility.
>
> Link: https://lore.kernel.org/all/20260108014416.3656493-3-zhengqixing@huaweicloud.com/
> Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> ---
>   block/blk-cgroup.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 3cffb68ba5d8..600f8c5843ea 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1596,6 +1596,8 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>   
>   	if (queue_is_mq(q))
>   		memflags = blk_mq_freeze_queue(q);
> +
> +	mutex_lock(&q->blkcg_mutex);
>   retry:
>   	spin_lock_irq(&q->queue_lock);
>   
> @@ -1658,6 +1660,7 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>   
>   	spin_unlock_irq(&q->queue_lock);
>   out:
> +	mutex_unlock(&q->blkcg_mutex);
>   	if (queue_is_mq(q))
>   		blk_mq_unfreeze_queue(q, memflags);
>   	if (pinned_blkg)

Can you also protect blkg_destroy_all() will blkcg_mutex as well? Then all access for q->blkg_list will
be protected.

-- 
Thansk,
Kuai

Re: [PATCH v2 2/3] blk-cgroup: skip dying blkg in blkcg_activate_policy()

[Yu Kuai]

Hi,

在 2026/1/13 14:10, Zheng Qixing 写道:
> From: Zheng Qixing <zhengqixing@huawei.com>
>
> When switching IO schedulers on a block device, blkcg_activate_policy()
> can race with concurrent blkcg deletion, leading to a use-after-free in
> rcu_accelerate_cbs.
>
> T1:                               T2:
> 		                  blkg_destroy
>                   		  kill(&blkg->refcnt) // blkg->refcnt=1->0
> 				  blkg_release // call_rcu(__blkg_release)
>                                    ...
> 				  blkg_free_workfn
>                                    ->pd_free_fn(pd)
> elv_iosched_store
> elevator_switch
> ...
> iterate blkg list
> blkg_get(blkg) // blkg->refcnt=0->1
>                                    list_del_init(&blkg->q_node)
> blkg_put(pinned_blkg) // blkg->refcnt=1->0
> blkg_release // call_rcu again
> rcu_accelerate_cbs // uaf
>
> Fix this by replacing blkg_get() with blkg_tryget(), which fails if
> the blkg's refcount has already reached zero. If blkg_tryget() fails,
> skip processing this blkg since it's already being destroyed.
>
> Link: https://lore.kernel.org/all/20260108014416.3656493-4-zhengqixing@huaweicloud.com/
> Fixes: f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from blkg_free_workfn() and blkcg_deactivate_policy()")
> Signed-off-by: Zheng Qixing <zhengqixing@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   block/blk-cgroup.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 600f8c5843ea..5dbc107eec53 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -1622,9 +1622,10 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>   			 * GFP_NOWAIT failed.  Free the existing one and
>   			 * prealloc for @blkg w/ GFP_KERNEL.
>   			 */
> +			if (!blkg_tryget(blkg))
> +				continue;

So, why this check is still before the pd_alloc_fn()?

See blkg_destroy(), can you replace this by the same checking:

list_for_each_entry_reverse()
	if (hlist_unhashed(&blkg->blkcg_node))
		continue;
	if (blkg->pd[pol->plid])
		continue;

>   			if (pinned_blkg)
>   				blkg_put(pinned_blkg);
> -			blkg_get(blkg);
>   			pinned_blkg = blkg;
>   
>   			spin_unlock_irq(&q->queue_lock);

-- 
Thansk,
Kuai

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Zheng Qixing]

Resend...

blkcg_activate_policy()                blkg_free_workfn()
-------------------                    ------------------
spin_lock(&q->queue_lock)
...
if (!pd) {
     spin_unlock(&q->queue_lock)
     ...
     goto enomem
     }
enomem:
     spin_lock(&q->queue_lock)
     if (pd) {

                        ->pd_free_fn()  // pd freed

        pd->online // uaf
        ...
     }

                        spin_lock(&q->queue_lock)

                        list_del_init(&blkg->q_node)

                        spin_unlock(&q->queue_lock) 

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Zheng Qixing]

>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 3cffb68ba5d8..600f8c5843ea 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1596,6 +1596,8 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>>    
>>    	if (queue_is_mq(q))
>>    		memflags = blk_mq_freeze_queue(q);
>> +
>> +	mutex_lock(&q->blkcg_mutex);
>>    retry:
>>    	spin_lock_irq(&q->queue_lock);
>>    
>> @@ -1658,6 +1660,7 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>>    
>>    	spin_unlock_irq(&q->queue_lock);
>>    out:
>> +	mutex_unlock(&q->blkcg_mutex);
>>    	if (queue_is_mq(q))
>>    		blk_mq_unfreeze_queue(q, memflags);
>>    	if (pinned_blkg)
> Can you also protect blkg_destroy_all() will blkcg_mutex as well? Then all access for q->blkg_list will
> be protected.
Why does blkg_destroy_all() also need blkcg_mutex?

After finishing ->pd_offline_fn() for blkgs and scheduling 
blkg_free_workfn() in blkg_destroy(),
blkg_destroy_all() clears the corresponding policy bit in q->blkcg_pols 
to avoid duplicate policy
teardown in blkcg_deactivate_policy().

Re: [PATCH v2 2/3] blk-cgroup: skip dying blkg in blkcg_activate_policy()

[Zheng Qixing]

>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 600f8c5843ea..5dbc107eec53 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1622,9 +1622,10 @@ int blkcg_activate_policy(struct gendisk *disk, const struct blkcg_policy *pol)
>>    			 * GFP_NOWAIT failed.  Free the existing one and
>>    			 * prealloc for @blkg w/ GFP_KERNEL.
>>    			 */
>> +			if (!blkg_tryget(blkg))
>> +				continue;
> So, why this check is still before the pd_alloc_fn()?
You mean 'after'?
> See blkg_destroy(), can you replace this by the same checking:
>
> list_for_each_entry_reverse()
> 	if (hlist_unhashed(&blkg->blkcg_node))
> 		continue;
> 	if (blkg->pd[pol->plid])
> 		continue;

This change makes sense.

This issue can be resolved by either doing tryget(blkg) before
pd_alloc_fn() or by accessing blkg->blkcg_node.

To keep the behavior consistent with blkg_destroy() and blkg_destroy_all(), I will revise this in v3.

Thank,

Qixing

Re: [PATCH v2 1/3] blk-cgroup: fix race between policy activation and blkg destruction

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On Thu, Jan 15, 2026 at 11:27:47AM +0800, Zheng Qixing <zhengqixing@huaweicloud.com> wrote:
> Yes, this issue was discovered by injecting memory allocation failure at
> ->pd_alloc_fn(..., GFP_KERNEL) in blkcg_activate_policy().

Fair enough.

> Commit f1c006f1c685 ("blk-cgroup: synchronize pd_free_fn() from
> blkg_free_workfn() and blkcg_deactivate_policy()") delays
> list_del_init(&blkg->q_node) until after pd_free_fn() in blkg_free_workfn().

IIUC, the point was to delay it from blkg_destroy until blkg_free_workfn
but then inside blkg_free_workfn it may have gone too far where it calls
pd_free_fn's before actual list removal.

(I'm Cc'ing the correct Kuai's address now.)
IOW, I'm wondering whether mere swap of these two actions (pd_free_fn
and list removal) wouldn't be a sufficient fix for the discovered issue
(instead of expanding lock coverage).

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]