[Devel] [PATCH hci-8.1 0/2] blk-throttle: fix a host panic setting the first throttle limit on a disk under I/O

[Devel] [PATCH hci-8.1 0/2] blk-throttle: fix a host panic setting the first throttle limit on a disk under I/O

[Denis V. Lunev]

A compute node panicked in submit_bio_noacct() under a vhost-blk worker
(ASUP-3910):

  BUG: kernel NULL pointer dereference, address: 000000000000019e
  RIP: 0010:submit_bio_noacct+0x12e/0x530
   vhost_blk_req_submit+0xf7/0x1c0 [vhost_blk]
   vhost_blk_req_handle+0x3b4/0x400 [vhost_blk]
   vhost_blk_handle_guest_kick+0x1a0/0x2c6 [vhost_blk]

blk_throtl_init() publishes q->td before blkcg_activate_policy()
attaches throttle data to the blkgs. The throttle check in
submit_bio_noacct() runs before bio_queue_enter(), so the queue freeze
does not cover it, and a bio in that window gets a NULL throtl_grp. In
the vmcore the bio is the first read of a dm-qcow2 device created a
moment earlier, and its blkg holds the per-disk limit being applied.
vhost-blk submits as soon as the guest kicks, which makes the window
easy to hit on VM start.

Patch 1 is the upstream fix. Patch 2 is its upstream follow-up and
must go with it: with patch 1 alone blkg_destroy_all() clears the
policy bit before blk_throtl_exit() runs, and q->td leaks on every
removal of a throttled disk. Both apply cleanly.

Build-tested only: the race needs a throttle limit set at the moment
the first bio of a fresh disk is submitted.

https://virtuozzo.atlassian.net/browse/VSTOR-152993

Han Guangjiang (1):
  ms/blk-throttle: fix access race during throttle policy activation

Yu Kuai (1):
  ms/blk-throttle: fix throtl_data leak during disk release

 block/blk-cgroup.c   |  6 ------
 block/blk-cgroup.h   |  6 ++++++
 block/blk-throttle.c | 13 ++++++-------
 block/blk-throttle.h | 18 +++++++++++-------
 4 files changed, 23 insertions(+), 20 deletions(-)

-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 1/2] ms/blk-throttle: fix access race during throttle policy activation

[Denis V. Lunev]

From: Han Guangjiang <hanguangjiang@lixiang.com>

On repeated cold boots we occasionally hit a NULL pointer crash in
blk_should_throtl() when throttling is consulted before the throttle
policy is fully enabled for the queue. Checking only q->td != NULL is
insufficient during early initialization, so blkg_to_pd() for the
throttle policy can still return NULL and blkg_to_tg() becomes NULL,
which later gets dereferenced.

 Unable to handle kernel NULL pointer dereference
 at virtual address 0000000000000156
 ...
 pc : submit_bio_noacct+0x14c/0x4c8
 lr : submit_bio_noacct+0x48/0x4c8
 sp : ffff800087f0b690
 x29: ffff800087f0b690 x28: 0000000000005f90 x27: ffff00068af393c0
 x26: 0000000000080000 x25: 000000000002fbc0 x24: ffff000684ddcc70
 x23: 0000000000000000 x22: 0000000000000000 x21: 0000000000000000
 x20: 0000000000080000 x19: ffff000684ddcd08 x18: ffffffffffffffff
 x17: 0000000000000000 x16: ffff80008132a550 x15: 0000ffff98020fff
 x14: 0000000000000000 x13: 1fffe000d11d7021 x12: ffff000688eb810c
 x11: ffff00077ec4bb80 x10: ffff000688dcb720 x9 : ffff80008068ef60
 x8 : 00000a6fb8a86e85 x7 : 000000000000111e x6 : 0000000000000002
 x5 : 0000000000000246 x4 : 0000000000015cff x3 : 0000000000394500
 x2 : ffff000682e35e40 x1 : 0000000000364940 x0 : 000000000000001a
 Call trace:
  submit_bio_noacct+0x14c/0x4c8
  verity_map+0x178/0x2c8
  __map_bio+0x228/0x250
  dm_submit_bio+0x1c4/0x678
  __submit_bio+0x170/0x230
  submit_bio_noacct_nocheck+0x16c/0x388
  submit_bio_noacct+0x16c/0x4c8
  submit_bio+0xb4/0x210
  f2fs_submit_read_bio+0x4c/0xf0
  f2fs_mpage_readpages+0x3b0/0x5f0
  f2fs_readahead+0x90/0xe8

Tighten blk_throtl_activated() to also require that the throttle policy
bit is set on the queue:

  return q->td != NULL &&
         test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);

This prevents blk_should_throtl() from accessing throttle group state
until policy data has been attached to blkgs.

Fixes: a3166c51702b ("blk-throttle: delay initialization until configuration")
Co-developed-by: Liang Jie <liangjie@lixiang.com>
Signed-off-by: Liang Jie <liangjie@lixiang.com>
Signed-off-by: Han Guangjiang <hanguangjiang@lixiang.com>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit bd9fd5be6bc0836820500f68fff144609fbd85a9)

https://virtuozzo.atlassian.net/browse/VSTOR-152993
Feature: fix ms/block

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/blk-cgroup.c   |  6 ------
 block/blk-cgroup.h   |  6 ++++++
 block/blk-throttle.c |  6 +-----
 block/blk-throttle.h | 18 +++++++++++-------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index e4ef47299609..39821d20ba07 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -110,12 +110,6 @@ static struct cgroup_subsys_state *blkcg_css(void)
 	return task_css(current, io_cgrp_id);
 }
 
-static bool blkcg_policy_enabled(struct request_queue *q,
-				 const struct blkcg_policy *pol)
-{
-	return pol && test_bit(pol->plid, q->blkcg_pols);
-}
-
 static void blkg_free_workfn(struct work_struct *work)
 {
 	struct blkcg_gq *blkg = container_of(work, struct blkcg_gq,
diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 81868ad86330..83367086cb6a 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -459,6 +459,12 @@ static inline bool blk_cgroup_mergeable(struct request *rq, struct bio *bio)
 		bio_issue_as_root_blkg(rq->bio) == bio_issue_as_root_blkg(bio);
 }
 
+static inline bool blkcg_policy_enabled(struct request_queue *q,
+				const struct blkcg_policy *pol)
+{
+	return pol && test_bit(pol->plid, q->blkcg_pols);
+}
+
 void blk_cgroup_bio_start(struct bio *bio);
 void blkcg_add_delay(struct blkcg_gq *blkg, u64 now, u64 delta);
 #else	/* CONFIG_BLK_CGROUP */
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 397b6a410f9e..cfa1cd60d2c5 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1327,17 +1327,13 @@ static int blk_throtl_init(struct gendisk *disk)
 	INIT_WORK(&td->dispatch_work, blk_throtl_dispatch_work_fn);
 	throtl_service_queue_init(&td->service_queue);
 
-	/*
-	 * Freeze queue before activating policy, to synchronize with IO path,
-	 * which is protected by 'q_usage_counter'.
-	 */
 	memflags = blk_mq_freeze_queue(disk->queue);
 	blk_mq_quiesce_queue(disk->queue);
 
 	q->td = td;
 	td->queue = q;
 
-	/* activate policy */
+	/* activate policy, blk_throtl_activated() will return true */
 	ret = blkcg_activate_policy(disk, &blkcg_policy_throtl);
 	if (ret) {
 		q->td = NULL;
diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 3b27755bfbff..9d7a42c039a1 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -156,7 +156,13 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
 
 static inline bool blk_throtl_activated(struct request_queue *q)
 {
-	return q->td != NULL;
+	/*
+	 * q->td guarantees that the blk-throttle module is already loaded,
+	 * and the plid of blk-throttle is assigned.
+	 * blkcg_policy_enabled() guarantees that the policy is activated
+	 * in the request_queue.
+	 */
+	return q->td != NULL && blkcg_policy_enabled(q, &blkcg_policy_throtl);
 }
 
 static inline bool blk_should_throtl(struct bio *bio)
@@ -164,11 +170,6 @@ static inline bool blk_should_throtl(struct bio *bio)
 	struct throtl_grp *tg;
 	int rw = bio_data_dir(bio);
 
-	/*
-	 * This is called under bio_queue_enter(), and it's synchronized with
-	 * the activation of blk-throtl, which is protected by
-	 * blk_mq_freeze_queue().
-	 */
 	if (!blk_throtl_activated(bio->bi_bdev->bd_queue))
 		return false;
 
@@ -194,7 +195,10 @@ static inline bool blk_should_throtl(struct bio *bio)
 
 static inline bool blk_throtl_bio(struct bio *bio)
 {
-
+	/*
+	 * block throttling takes effect if the policy is activated
+	 * in the bio's request_queue.
+	 */
 	if (!blk_should_throtl(bio))
 		return false;
 
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/2] ms/blk-throttle: fix access race during throttle policy activation

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] blk-throttle: blk_throtl_exit() skips teardown and leaks td after bit cleared
--

commit b69e5d24a46730350dbae00100359769cb6cf354
Author: Han Guangjiang <hanguangjiang@lixiang.com>

ms/blk-throttle: fix access race during throttle policy activation

This backport tightens blk_throtl_activated() so that it also requires the
throttle policy bit to be set in q->blkcg_pols. The aim is to stop
blk_should_throtl() from dereferencing a NULL throtl_grp while the policy
is still being activated. It also moves blkcg_policy_enabled() into
blk-cgroup.h so blk-throttle.h can use it.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152993

> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h

[ ... ]

>  static inline bool blk_throtl_activated(struct request_queue *q)
>  {
> -	return q->td != NULL;
> +	/*
> +	 * q->td guarantees that the blk-throttle module is already loaded,
> +	 * and the plid of blk-throttle is assigned.
> +	 * blkcg_policy_enabled() guarantees that the policy is activated
> +	 * in the request_queue.
> +	 */
> +	return q->td != NULL && blkcg_policy_enabled(q, &blkcg_policy_throtl);
>  }

[Severity: Medium]
Does this stricter predicate break blk_throtl_exit(), which also uses
blk_throtl_activated()?

The disk release path is:

blkcg_exit_disk()
    blkg_destroy_all(disk);
    blk_throtl_exit(disk);

blkg_destroy_all() clears the bit for every registered policy, including
the throttle policy:

    for (i = 0; i < BLKCG_MAX_POLS; i++) {
        struct blkcg_policy *pol = blkcg_policy[i];

        if (pol)
            __clear_bit(pol->plid, q->blkcg_pols);
    }

When blk_throtl_exit() runs next, blkcg_policy_enabled() is already
false:

block/blk-throttle.c:blk_throtl_exit() {
    if (!blk_throtl_activated(q))
        return;

    timer_delete_sync(&q->td->service_queue.pending_timer);
    throtl_shutdown_wq(q);
    blkcg_deactivate_policy(disk, &blkcg_policy_throtl);
    kfree(q->td);
}

The function returns early whenever throttling was configured on the disk.
Can this leak struct throtl_data and skip timer_delete_sync() on
pending_timer and the dispatch_work shutdown in throtl_shutdown_wq()?

Before this patch the guard was only q->td != NULL, so this looks like a
regression from the changed predicate. I believe the timer and work are
normally idle on a dead queue, so the main effect is the leak of q->td.

The follow-up patch in this series, "ms/blk-throttle: fix throtl_data leak
during disk release", changes the guard in blk_throtl_exit() to
"if (!q->td) return;". That would fix this, but it leaves the intermediate
commit with the leak. Could the two be folded together, or ordered so
this one does not regress blk_throtl_exit()?

blk_throtl_cancel_bios() is not affected. It is called from del_gendisk()
before blkg_destroy_all(), so the throttle bit is still set there.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102161219.3735623-1-den@openvz.org?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/2] ms/blk-throttle: fix access race during throttle policy activation

[Konstantin Khorenko]

On 11/2/26 17:59, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] blk-throttle: blk_throtl_exit() skips teardown and leaks td after bit cleared
> --
> 
> commit b69e5d24a46730350dbae00100359769cb6cf354
> Author: Han Guangjiang <hanguangjiang@lixiang.com>
> 
> ms/blk-throttle: fix access race during throttle policy activation
> 
> This backport tightens blk_throtl_activated() so that it also requires the
> throttle policy bit to be set in q->blkcg_pols. The aim is to stop
> blk_should_throtl() from dereferencing a NULL throtl_grp while the policy
> is still being activated. It also moves blkcg_policy_enabled() into
> blk-cgroup.h so blk-throttle.h can use it.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-152993
> 
>> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
>> --- a/block/blk-throttle.h
>> +++ b/block/blk-throttle.h
> 
> [ ... ]
> 
>>  static inline bool blk_throtl_activated(struct request_queue *q)
>>  {
>> -	return q->td != NULL;
>> +	/*
>> +	 * q->td guarantees that the blk-throttle module is already loaded,
>> +	 * and the plid of blk-throttle is assigned.
>> +	 * blkcg_policy_enabled() guarantees that the policy is activated
>> +	 * in the request_queue.
>> +	 */
>> +	return q->td != NULL && blkcg_policy_enabled(q, &blkcg_policy_throtl);
>>  }
> 
> [Severity: Medium]
> Does this stricter predicate break blk_throtl_exit(), which also uses
> blk_throtl_activated()?
> 
> The disk release path is:
> 
> blkcg_exit_disk()
>     blkg_destroy_all(disk);
>     blk_throtl_exit(disk);
> 
> blkg_destroy_all() clears the bit for every registered policy, including
> the throttle policy:
> 
>     for (i = 0; i < BLKCG_MAX_POLS; i++) {
>         struct blkcg_policy *pol = blkcg_policy[i];
> 
>         if (pol)
>             __clear_bit(pol->plid, q->blkcg_pols);
>     }
> 
> When blk_throtl_exit() runs next, blkcg_policy_enabled() is already
> false:
> 
> block/blk-throttle.c:blk_throtl_exit() {
>     if (!blk_throtl_activated(q))
>         return;
> 
>     timer_delete_sync(&q->td->service_queue.pending_timer);
>     throtl_shutdown_wq(q);
>     blkcg_deactivate_policy(disk, &blkcg_policy_throtl);
>     kfree(q->td);
> }
> 
> The function returns early whenever throttling was configured on the disk.
> Can this leak struct throtl_data and skip timer_delete_sync() on
> pending_timer and the dispatch_work shutdown in throtl_shutdown_wq()?
> 
> Before this patch the guard was only q->td != NULL, so this looks like a
> regression from the changed predicate. I believe the timer and work are
> normally idle on a dead queue, so the main effect is the leak of q->td.
> 
> The follow-up patch in this series, "ms/blk-throttle: fix throtl_data leak
> during disk release", changes the guard in blk_throtl_exit() to
> "if (!q->td) return;". That would fix this, but it leaves the intermediate
> commit with the leak. Could the two be folded together, or ordered so
> this one does not regress blk_throtl_exit()?
> 
> blk_throtl_cancel_bios() is not affected. It is called from del_gendisk()
> before blkg_destroy_all(), so the throttle bit is still set there.

  ▎ Correct, and that is what patch 2 fixes. The two are kept as separate commits on purpose: they mirror upstream
  ▎ bd9fd5be6bc0 and 336aec7b06be one to one, mainline has the same intermediate state, and the series is applied as a
  ▎ whole.



_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 2/2] ms/blk-throttle: fix throtl_data leak during disk release

[Denis V. Lunev]

From: Yu Kuai <yukuai3@huawei.com>

Tightening the throttle activation check in blk_throtl_activated() to
require both q->td presence and policy bit set introduced a memory leak
during disk release:

blkg_destroy_all() clears the policy bit first during queue deactivation,
causing subsequent blk_throtl_exit() to skip throtl_data cleanup when
blk_throtl_activated() fails policy check.

Idealy we should avoid modifying blk_throtl_exit() activation check because
it's intuitive that blk-throtl start from blk_throtl_init() and end in
blk_throtl_exit(). However, call blk_throtl_exit() before
blkg_destroy_all() will make a long term deadlock problem easier to
trigger[1], hence fix this problem by checking if q->td is NULL from
blk_throtl_exit(), and remove policy deactivation as well since it's
useless.

[1] https://lore.kernel.org/all/CAHj4cs9p9H5yx+ywsb3CMUdbqGPhM+8tuBvhW=9ADiCjAqza9w@mail.gmail.com/#t

Fixes: bd9fd5be6bc0 ("blk-throttle: fix access race during throttle policy activation")
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Closes: https://lore.kernel.org/all/CAHj4cs-p-ZwBEKigBj7T6hQCOo-H68-kVwCrV6ZvRovrr9Z+HA@mail.gmail.com/
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit 336aec7b06be860477be80a4299263a2e9355789)

https://virtuozzo.atlassian.net/browse/VSTOR-152993
Feature: fix ms/block

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/blk-throttle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index cfa1cd60d2c5..95bf175c2a13 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1842,12 +1842,15 @@ void blk_throtl_exit(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
 
-	if (!blk_throtl_activated(q))
+	/*
+	 * blkg_destroy_all() already deactivate throtl policy, just check and
+	 * free throtl data.
+	 */
+	if (!q->td)
 		return;
 
 	timer_delete_sync(&q->td->service_queue.pending_timer);
 	throtl_shutdown_wq(q);
-	blkcg_deactivate_policy(disk, &blkcg_policy_throtl);
 	kfree(q->td);
 }
 
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 2/2] ms/blk-throttle: fix throtl_data leak during disk release

[no-reply]

> From: Yu Kuai <yukuai3@huawei.com>
> 
> Tightening the throttle activation check in blk_throtl_activated() to
> require both q->td presence and policy bit set introduced a memory leak
> during disk release:
> 
> blkg_destroy_all() clears the policy bit first during queue deactivation,
> causing subsequent blk_throtl_exit() to skip throtl_data cleanup when
> blk_throtl_activated() fails policy check.
> 
> Idealy we should avoid modifying blk_throtl_exit() activation check because
> it's intuitive that blk-throtl start from blk_throtl_init() and end in
> blk_throtl_exit(). However, call blk_throtl_exit() before
> blkg_destroy_all() will make a long term deadlock problem easier to
> trigger[1], hence fix this problem by checking if q->td is NULL from
> [ ... ]
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-152993
> Feature: fix ms/block
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102161219.3735623-1-den@openvz.org?part=2


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 0/2] blk-throttle: fix a host panic setting the first throttle limit on a disk under I/O

[Konstantin Khorenko]

On Mon, 02 Nov 2026 17:12:15 +0100, Denis V. Lunev wrote:
> blk-throttle: fix a host panic setting the first throttle limit on a disk under I/O
> 
> A compute node panicked in submit_bio_noacct() under a vhost-blk worker
> (ASUP-3910):
> 
>   BUG: kernel NULL pointer dereference, address: 000000000000019e
>   RIP: 0010:submit_bio_noacct+0x12e/0x530
>    vhost_blk_req_submit+0xf7/0x1c0 [vhost_blk]
>    vhost_blk_req_handle+0x3b4/0x400 [vhost_blk]
>    vhost_blk_handle_guest_kick+0x1a0/0x2c6 [vhost_blk]
> 
> [...]

Applied to branch-rh10-6.12.0-211.52.1.22.x.vz10-ovz, will be released in rh10-6.12.0-211.52.1.22.16.vz10, thanks!

[1/2] ms/blk-throttle: fix access race during throttle policy activation
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/5252718bd1d0
[2/2] ms/blk-throttle: fix throtl_data leak during disk release
      https://bitbucket.org/virtuozzocore/vzkernel.vzs/commits/fc434d3705a0

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel