[PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[gj.han]

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.

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")
Cc: stable@vger.kernel.org

Co-developed-by: Liang Jie <liangjie@lixiang.com>
Signed-off-by: Liang Jie <liangjie@lixiang.com>
Signed-off-by: Han Guangjiang <hanguangjiang@lixiang.com>
---
 block/blk-throttle.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 3b27755bfbff..9ca43dc56eda 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -156,7 +156,7 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
 
 static inline bool blk_throtl_activated(struct request_queue *q)
 {
-	return q->td != NULL;
+	return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
 }
 
 static inline bool blk_should_throtl(struct bio *bio)
-- 
2.25.1

[PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[gj.han]

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>
---
 block/blk-throttle.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 3b27755bfbff..9ca43dc56eda 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -156,7 +156,7 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
 
 static inline bool blk_throtl_activated(struct request_queue *q)
 {
-	return q->td != NULL;
+	return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
 }
 
 static inline bool blk_should_throtl(struct bio *bio)
-- 
2.25.1

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai]

Hi,

在 2025/9/2 20:39, gj.han@foxmail.com 写道:
> 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>
> ---
>   block/blk-throttle.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 3b27755bfbff..9ca43dc56eda 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -156,7 +156,7 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
>   
>   static inline bool blk_throtl_activated(struct request_queue *q)
>   {
> -	return q->td != NULL;
> +	return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
>   }

Instead of add checking from hot path, do you consider delaying setting q->td
until policy is activated from the slow path? I think this is better solution.

Thanks,
Kuai

>   
>   static inline bool blk_should_throtl(struct bio *bio)

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Han Guangjiang]

Hi Kuai,

> Instead of add checking from hot path, do you consider delaying setting q->td
> until policy is activated from the slow path? I think this is better solution.

Thank you for your review. You're absolutely right that performance 
considerations in the hot path are important.

We actually considered delaying the setting of q->td until after policy 
activation, but we found that q->td is needed by blkcg_activate_policy() 
during its execution, so it has to be set before calling 
blkcg_activate_policy().

We explored several alternative approaches:

1) Adding a dedicated flag like 'throttle_ready' to struct request_queue: 
   - Set this flag at the end of blk_throtl_init()
   - Check this flag in blk_throtl_activated() to determine if policy 
     loading is complete
   - However, this requires adding a new bool variable to the struct

2) Reusing the q->td pointer with low-order bit flags:
   - Use pointer low-order bits to mark initialization completion status
   - This would avoid adding new fields but requires careful handling 
     and additional processing

Given these constraints, we chose the current approach of checking the 
policy bit in blk_throtl_activated() as it:
- Doesn't require struct changes
- Provides a clean, atomic check
- Aligns with the existing policy activation mechanism

We would appreciate your suggestions on how to better handle this 
initialization race condition.

Thanks,
Han Guangjiang

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai]

Hi,

在 2025/09/03 10:55, Han Guangjiang 写道:
> Hi Kuai,
> 
>> Instead of add checking from hot path, do you consider delaying setting q->td
>> until policy is activated from the slow path? I think this is better solution.
> 
> Thank you for your review. You're absolutely right that performance
> considerations in the hot path are important.
> 
> We actually considered delaying the setting of q->td until after policy
> activation, but we found that q->td is needed by blkcg_activate_policy()
> during its execution, so it has to be set before calling
> blkcg_activate_policy().

That's not hard to bypass, q->td is used to initialze tg->td in
throtl_pd_init(), actually you can just remove it, and add a helper
tg_to_td() to replace it;

struct throtl_data *tg_to_td(struct throtl_grp *tg)
{
	return tg_to_blkg(tg)->q->td;
}

Meanwhile, please remove the comment about freeze queue, turns out it
can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
yet while issuing bio.

Thanks,
Kuai

> 
> We explored several alternative approaches:
> 
> 1) Adding a dedicated flag like 'throttle_ready' to struct request_queue:
>     - Set this flag at the end of blk_throtl_init()
>     - Check this flag in blk_throtl_activated() to determine if policy
>       loading is complete
>     - However, this requires adding a new bool variable to the struct
> 
> 2) Reusing the q->td pointer with low-order bit flags:
>     - Use pointer low-order bits to mark initialization completion status
>     - This would avoid adding new fields but requires careful handling
>       and additional processing
> 
> Given these constraints, we chose the current approach of checking the
> policy bit in blk_throtl_activated() as it:
> - Doesn't require struct changes
> - Provides a clean, atomic check
> - Aligns with the existing policy activation mechanism
> 
> We would appreciate your suggestions on how to better handle this
> initialization race condition.
> 
> Thanks,
> Han Guangjiang
> 
> 
> 
> .
> 

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Liang Jie]

On Wed, 3 Sep 2025 14:03:37 +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote:

>在 2025/09/03 10:55, Han Guangjiang 写道:
>> Hi Kuai,
>> 
>>> Instead of add checking from hot path, do you consider delaying setting q->td
>>> until policy is activated from the slow path? I think this is better solution.
>> 
>> Thank you for your review. You're absolutely right that performance
>> considerations in the hot path are important.
>> 
>> We actually considered delaying the setting of q->td until after policy
>> activation, but we found that q->td is needed by blkcg_activate_policy()
>> during its execution, so it has to be set before calling
>> blkcg_activate_policy().
>
>That's not hard to bypass, q->td is used to initialze tg->td in
>throtl_pd_init(), actually you can just remove it, and add a helper
>tg_to_td() to replace it;
>
>struct throtl_data *tg_to_td(struct throtl_grp *tg)
>{
>	return tg_to_blkg(tg)->q->td;
>}

Hi Kuai,

Thanks for the suggestion. Just a quick note: in throtl_pd_init(), q->td is not
only used to init tg->td, it’s also needed for sq->parent_sq:

 - sq->parent_sq = &td->service_queue;

So if we remove tg->td and delay q->td, throtl_pd_init() won’t have a valid td
to set parent_sq.

>
>Meanwhile, please remove the comment about freeze queue, turns out it
>can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
>yet while issuing bio.

You’re right. We’ll remove that comment in patch v2.

Thanks,
Liang Jie

>
>Thanks,
>Kuai
>
>> 
>> We explored several alternative approaches:
>> 
>> 1) Adding a dedicated flag like 'throttle_ready' to struct request_queue:
>>     - Set this flag at the end of blk_throtl_init()
>>     - Check this flag in blk_throtl_activated() to determine if policy
>>       loading is complete
>>     - However, this requires adding a new bool variable to the struct
>> 
>> 2) Reusing the q->td pointer with low-order bit flags:
>>     - Use pointer low-order bits to mark initialization completion status
>>     - This would avoid adding new fields but requires careful handling
>>       and additional processing
>> 
>> Given these constraints, we chose the current approach of checking the
>> policy bit in blk_throtl_activated() as it:
>> - Doesn't require struct changes
>> - Provides a clean, atomic check
>> - Aligns with the existing policy activation mechanism
>> 
>> We would appreciate your suggestions on how to better handle this
>> initialization race condition.
>> 
>> Thanks,
>> Han Guangjiang
>> 

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai]

Hi,

在 2025/09/03 15:21, Liang Jie 写道:
> On Wed, 3 Sep 2025 14:03:37 +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> 在 2025/09/03 10:55, Han Guangjiang 写道:
>>> Hi Kuai,
>>>
>>>> Instead of add checking from hot path, do you consider delaying setting q->td
>>>> until policy is activated from the slow path? I think this is better solution.
>>>
>>> Thank you for your review. You're absolutely right that performance
>>> considerations in the hot path are important.
>>>
>>> We actually considered delaying the setting of q->td until after policy
>>> activation, but we found that q->td is needed by blkcg_activate_policy()
>>> during its execution, so it has to be set before calling
>>> blkcg_activate_policy().
>>
>> That's not hard to bypass, q->td is used to initialze tg->td in
>> throtl_pd_init(), actually you can just remove it, and add a helper
>> tg_to_td() to replace it;
>>
>> struct throtl_data *tg_to_td(struct throtl_grp *tg)
>> {
>> 	return tg_to_blkg(tg)->q->td;
>> }
> 
> Hi Kuai,
> 
> Thanks for the suggestion. Just a quick note: in throtl_pd_init(), q->td is not
> only used to init tg->td, it’s also needed for sq->parent_sq:
> 
>   - sq->parent_sq = &td->service_queue;
> 
> So if we remove tg->td and delay q->td, throtl_pd_init() won’t have a valid td
> to set parent_sq.

Yes, however, this can be fixed very similar:

Set sq->parent_sq to NULL here, and add a helper parent_sq(q, sq):

if (sq->parent_sq)
	return sq->parent_sq;

td_sq = &q->td->service_queue;
return sq == td_sq ? NULL : td_sq;

And sq_to_tg() need to be changed as well. So far, I'm not sure how many
code changes are required this way. We of course want a simple fix for
stable backport, but we definitely still want this kind of fix in future
release.

Thanks,
Kuai

> 
>>
>> Meanwhile, please remove the comment about freeze queue, turns out it
>> can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
>> yet while issuing bio.
> 
> You’re right. We’ll remove that comment in patch v2.
> 
> Thanks,
> Liang Jie
> 
>>
>> Thanks,
>> Kuai
>>
>>>
>>> We explored several alternative approaches:
>>>
>>> 1) Adding a dedicated flag like 'throttle_ready' to struct request_queue:
>>>      - Set this flag at the end of blk_throtl_init()
>>>      - Check this flag in blk_throtl_activated() to determine if policy
>>>        loading is complete
>>>      - However, this requires adding a new bool variable to the struct
>>>
>>> 2) Reusing the q->td pointer with low-order bit flags:
>>>      - Use pointer low-order bits to mark initialization completion status
>>>      - This would avoid adding new fields but requires careful handling
>>>        and additional processing
>>>
>>> Given these constraints, we chose the current approach of checking the
>>> policy bit in blk_throtl_activated() as it:
>>> - Doesn't require struct changes
>>> - Provides a clean, atomic check
>>> - Aligns with the existing policy activation mechanism
>>>
>>> We would appreciate your suggestions on how to better handle this
>>> initialization race condition.
>>>
>>> Thanks,
>>> Han Guangjiang
>>>
> 
> .
> 

[PATCH v2] blk-throttle: fix access race during throttle policy activation

[Han Guangjiang]

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>
---
v2:
 - remove the comment about freeze queue in blk_should_throtl()
 - Retitle: "blk-throttle: fix access race during throttle policy activation"
---
 block/blk-throttle.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/blk-throttle.h b/block/blk-throttle.h
index 3b27755bfbff..fbf97c531c48 100644
--- a/block/blk-throttle.h
+++ b/block/blk-throttle.h
@@ -156,7 +156,7 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
 
 static inline bool blk_throtl_activated(struct request_queue *q)
 {
-	return q->td != NULL;
+	return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
 }
 
 static inline bool blk_should_throtl(struct bio *bio)
@@ -164,11 +164,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;
 
-- 
2.25.1

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Han Guangjiang]

Hi,

> Yes, however, this can be fixed very similar:
> 
> Set sq->parent_sq to NULL here, and add a helper parent_sq(q, sq):
> 
> if (sq->parent_sq)
>         return sq->parent_sq;
> 
> td_sq = &q->td->service_queue;
> return sq == td_sq ? NULL : td_sq;
> 
> And sq_to_tg() need to be changed as well. So far, I'm not sure how many
> code changes are required this way. We of course want a simple fix for
> stable backport, but we definitely still want this kind of fix in future
> release.

We preliminarily tried implementing this approach. But the changes are 
scattered and backport might be complex. So we provide a simple fix first.

> Meanwhile, please remove the comment about freeze queue, turns out it
> can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
> yet while issuing bio.

As discussed before, we also removed some outdated comments in 
blk_should_throtl().

We just submitted v2 which includes the changes.

We look forward to your feedback and suggestions on the v2 patch.

Thanks,
Han Guangjiang

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Han Guangjiang]

Hi,

> Yes, however, this can be fixed very similar:
> 
> Set sq->parent_sq to NULL here, and add a helper parent_sq(q, sq):
> 
> if (sq->parent_sq)
>         return sq->parent_sq;
> 
> td_sq = &q->td->service_queue;
> return sq == td_sq ? NULL : td_sq;
> 
> And sq_to_tg() need to be changed as well. So far, I'm not sure how many
> code changes are required this way. We of course want a simple fix for
> stable backport, but we definitely still want this kind of fix in future
> release.

We preliminarily tried implementing this approach. But the changes are 
scattered and backport might be complex. So we provide a simple fix first.

> Meanwhile, please remove the comment about freeze queue, turns out it
> can't protect blk_throtl_bio() becasue q_usage_coutner is not grabbed
> yet while issuing bio.

As discussed before, we also removed some outdated comments in 
blk_should_throtl().

We just submitted v2 which includes the changes.

We look forward to your feedback and suggestions on the v2 patch.

Thanks,
Han Guangjiang

Re: [PATCH v2] blk-throttle: fix access race during throttle policy activation

[Yu Kuai]

Hi,

在 2025/09/04 16:17, Han Guangjiang 写道:
> 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>
> ---
> v2:
>   - remove the comment about freeze queue in blk_should_throtl()
>   - Retitle: "blk-throttle: fix access race during throttle policy activation"
> ---
>   block/blk-throttle.h | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 3b27755bfbff..fbf97c531c48 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -156,7 +156,7 @@ void blk_throtl_cancel_bios(struct gendisk *disk);
>   
>   static inline bool blk_throtl_activated(struct request_queue *q)
>   {
> -	return q->td != NULL;
> +	return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
>   }

You can just remove the fist checking, p->td must be set if policy is
enabled. And please make blkcg_policy_enabled() inline function in
blk-cgroup.h and use it here.

>   
>   static inline bool blk_should_throtl(struct bio *bio)
> @@ -164,11 +164,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;
>   
> 

Please also remove the comment in blk_throtl_init() about freeze_queue,
and add comments in both blk_throtl_init() and blk_throtl_bio() about
synchronization.

Thanks,
Kuai

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Han Guangjiang]

Hi,

>>   static inline bool blk_throtl_activated(struct request_queue *q)
>>   {
>> -        return q->td != NULL;
>> +        return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
>>   }
> 
> You can just remove the fist checking, p->td must be set if policy is
> enabled. And please make blkcg_policy_enabled() inline function in
> blk-cgroup.h and use it here.

We intentionally kept the q->td != NULL check because we cannot guarantee 
that the policy module is fully loaded when this function is called. 
If the policy module is not loaded yet, blkcg_policy_throtl.plid might not be 
assigned, which could cause the test_bit() check to be incorrect.

By keeping this check, we ensure that we have at least reached the cgroup 
configuration flow, indicating that the policy loading is complete.

I'm wondering if there are any risks here and whether we should remove 
the q->td != NULL check?

Thanks,
Han Guangjiang

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai]

Hi,

在 2025/9/4 18:05, Han Guangjiang 写道:
> Hi,
>
>>>    static inline bool blk_throtl_activated(struct request_queue *q)
>>>    {
>>> -        return q->td != NULL;
>>> +        return q->td != NULL && test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
>>>    }
>> You can just remove the fist checking, p->td must be set if policy is
>> enabled. And please make blkcg_policy_enabled() inline function in
>> blk-cgroup.h and use it here.
> We intentionally kept the q->td != NULL check because we cannot guarantee
> that the policy module is fully loaded when this function is called.
> If the policy module is not loaded yet, blkcg_policy_throtl.plid might not be
> assigned, which could cause the test_bit() check to be incorrect.
>
> By keeping this check, we ensure that we have at least reached the cgroup
> configuration flow, indicating that the policy loading is complete.
>
> I'm wondering if there are any risks here and whether we should remove
> the q->td != NULL check?

I think there is none. blk-throttle can't be build as module, if config is n,
blk_throtl_bio() is a non-function, if config is y, policy is registered during
init. And throtl_init() failure will panic, noted blkcg_policy_register() will
never fail for blk-throttle. BTW, policy pid is not a dynamic value at runtime.

Perhaps remove the checking and add some comments?

Thanks,
Kuai

>
> Thanks,
> Han Guangjiang
>
>

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Han Guangjiang]

Hi,

> >>>    static inline bool blk_throtl_activated(struct request_queue *q)
> >>>    {
> >>> -        return q->td != NULL;
> >>> +        return q->td != NULL && 
> >>> +               test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
> >>>    }
> >> You can just remove the fist checking, p->td must be set if policy is
> >> enabled. And please make blkcg_policy_enabled() inline function in
> >> blk-cgroup.h and use it here.
> > We intentionally kept the q->td != NULL check because we cannot guarantee
> > that the policy module is fully loaded when this function is called.
> > If the policy module is not loaded yet, blkcg_policy_throtl.plid might not be
> > assigned, which could cause the test_bit() check to be incorrect.
> >
> > By keeping this check, we ensure that we have at least reached the cgroup
> > configuration flow, indicating that the policy loading is complete.
> >
> > I'm wondering if there are any risks here and whether we should remove
> > the q->td != NULL check?
> 
> I think there is none. blk-throttle can't be build as module, if config is n,
> blk_throtl_bio() is a non-function, if config is y, policy is registered during
> init. And throtl_init() failure will panic, noted blkcg_policy_register() will
> never fail for blk-throttle. BTW, policy pid is not a dynamic value at runtime.

I understand your point, but I think there's still a potential race 
condition during kernel initialization. While blk-throttle is built as a 
built-in module, block devices can also be built as built-in modules, and 
at the same initcall level, the loading order may be unpredictable. 
Additionally, the policy plid is allocated during blk-throttle module 
initialization, so we need to verify this timing issue.

I conducted an experiment on the qemu platform by adding a BUG() statement 
in the blk_throtl_activated() function:

------------[ cut here ]------------
kernel BUG at block/blk-throttle.h:157!
Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT_RT SMP
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.17-g69f6c99f1c48 #2
Hardware name: linux,dummy-virt (DT)
pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : submit_bio_noacct+0xec/0x388
lr : submit_bio_noacct+0x40/0x388
sp : ffff80008135b530
x29: ffff80008135b530 x28: ffff00000180f000 x27: fffffdffc00876c0
x26: 0000000000000040 x25: ffff800080a405f0 x24: 0000000000000004
x23: ffff800080393128 x22: ffff000002720000 x21: ffff0000018c0700
x20: 0000000000000000 x19: ffff00000207b180 x18: 0000000000000020
x17: 0000000000000002 x16: 0000000000000002 x15: ffffffffffffffff
x14: 0000000000000000 x13: ffff800080dff350 x12: ffffffffffffffff
x11: 0000000000000000 x10: 0000000000000020 x9 : ffff8000804cd088
x8 : ffff00000180f088 x7 : 0000000000000000 x6 : ffff00000207b1f8
x5 : 0000000000000008 x4 : ffff0000018c0700 x3 : ffff7fffdee71000
x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
 submit_bio_noacct+0xec/0x388
 submit_bio+0xb4/0x150
 submit_bh_wbc+0x14c/0x1d0
 block_read_full_folio+0x25c/0x398
 blkdev_read_folio+0x24/0x38
 filemap_read_folio+0x34/0xf0
 do_read_cache_folio+0x150/0x290
 read_cache_folio+0x1c/0x30
 read_part_sector+0x48/0xd0
 read_lba+0x9c/0x180
 efi_partition+0xa0/0x780
 bdev_disk_changed+0x238/0x608
 blkdev_get_whole+0xac/0x150
 bdev_open+0x28c/0x418
 bdev_file_open_by_dev+0xe0/0x150
 disk_scan_partitions+0x74/0x160
 device_add_disk+0x3f4/0x448
 virtblk_probe+0x74c/0x920
 virtio_dev_probe+0x1a4/0x250
 really_probe+0xb4/0x2b0
 __driver_probe_device+0x80/0x140
 driver_probe_device+0xdc/0x178
 __driver_attach+0x9c/0x1b8
 bus_for_each_dev+0x7c/0xe8
 driver_attach+0x2c/0x40
 bus_add_driver+0xec/0x218
 driver_register+0x68/0x138
 __register_virtio_driver+0x2c/0x50
 virtio_blk_init+0x74/0xd0
 do_one_initcall+0x64/0x290
 kernel_init_freeable+0x214/0x408
 kernel_init+0x2c/0x1f0
 ret_from_fork+0x10/0x20

From the experimental results, we can see that virtio block device is 
executing blk_throtl_activated() during initialization in do_one_initcall() 
when loading virio-blk module_init(). 

Combined with the information that blk-throttle is also loaded as a 
built-in module, there exists a scenario where the block device probes 
first, at which point the plid has not been allocated yet, and 
blk_throtl_activated() is executed, followed by the loading of the 
blk-throttle built-in module.

Given this experimental evidence, I'm wondering if we should consider 
keeping the q->td != NULL check as a safeguard against this initialization 
race condition. I'd appreciate your thoughts on this matter.

Thanks,
Han Guangjiang

Re: [PATCH] blk-throttle: check policy bit in blk_throtl_activated()

[Yu Kuai]

Hi,

在 2025/09/05 14:53, Han Guangjiang 写道:
> Hi,
> 
>>>>>     static inline bool blk_throtl_activated(struct request_queue *q)
>>>>>     {
>>>>> -        return q->td != NULL;
>>>>> +        return q->td != NULL &&
>>>>> +               test_bit(blkcg_policy_throtl.plid, q->blkcg_pols);
>>>>>     }
>>>> You can just remove the fist checking, p->td must be set if policy is
>>>> enabled. And please make blkcg_policy_enabled() inline function in
>>>> blk-cgroup.h and use it here.
>>> We intentionally kept the q->td != NULL check because we cannot guarantee
>>> that the policy module is fully loaded when this function is called.
>>> If the policy module is not loaded yet, blkcg_policy_throtl.plid might not be
>>> assigned, which could cause the test_bit() check to be incorrect.
>>>
>>> By keeping this check, we ensure that we have at least reached the cgroup
>>> configuration flow, indicating that the policy loading is complete.
>>>
>>> I'm wondering if there are any risks here and whether we should remove
>>> the q->td != NULL check?
>>
>> I think there is none. blk-throttle can't be build as module, if config is n,
>> blk_throtl_bio() is a non-function, if config is y, policy is registered during
>> init. And throtl_init() failure will panic, noted blkcg_policy_register() will
>> never fail for blk-throttle. BTW, policy pid is not a dynamic value at runtime.
> 
> I understand your point, but I think there's still a potential race
> condition during kernel initialization. While blk-throttle is built as a
> built-in module, block devices can also be built as built-in modules, and
> at the same initcall level, the loading order may be unpredictable.
> Additionally, the policy plid is allocated during blk-throttle module
> initialization, so we need to verify this timing issue.
> 
> I conducted an experiment on the qemu platform by adding a BUG() statement
> in the blk_throtl_activated() function:
> 
> ------------[ cut here ]------------
> kernel BUG at block/blk-throttle.h:157!
> Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT_RT SMP
> Modules linked in:
> CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.17-g69f6c99f1c48 #2
> Hardware name: linux,dummy-virt (DT)
> pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : submit_bio_noacct+0xec/0x388
> lr : submit_bio_noacct+0x40/0x388
> sp : ffff80008135b530
> x29: ffff80008135b530 x28: ffff00000180f000 x27: fffffdffc00876c0
> x26: 0000000000000040 x25: ffff800080a405f0 x24: 0000000000000004
> x23: ffff800080393128 x22: ffff000002720000 x21: ffff0000018c0700
> x20: 0000000000000000 x19: ffff00000207b180 x18: 0000000000000020
> x17: 0000000000000002 x16: 0000000000000002 x15: ffffffffffffffff
> x14: 0000000000000000 x13: ffff800080dff350 x12: ffffffffffffffff
> x11: 0000000000000000 x10: 0000000000000020 x9 : ffff8000804cd088
> x8 : ffff00000180f088 x7 : 0000000000000000 x6 : ffff00000207b1f8
> x5 : 0000000000000008 x4 : ffff0000018c0700 x3 : ffff7fffdee71000
> x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
> Call trace:
>   submit_bio_noacct+0xec/0x388
>   submit_bio+0xb4/0x150
>   submit_bh_wbc+0x14c/0x1d0
>   block_read_full_folio+0x25c/0x398
>   blkdev_read_folio+0x24/0x38
>   filemap_read_folio+0x34/0xf0
>   do_read_cache_folio+0x150/0x290
>   read_cache_folio+0x1c/0x30
>   read_part_sector+0x48/0xd0
>   read_lba+0x9c/0x180
>   efi_partition+0xa0/0x780
>   bdev_disk_changed+0x238/0x608
>   blkdev_get_whole+0xac/0x150
>   bdev_open+0x28c/0x418
>   bdev_file_open_by_dev+0xe0/0x150
>   disk_scan_partitions+0x74/0x160
>   device_add_disk+0x3f4/0x448
>   virtblk_probe+0x74c/0x920
>   virtio_dev_probe+0x1a4/0x250
>   really_probe+0xb4/0x2b0
>   __driver_probe_device+0x80/0x140
>   driver_probe_device+0xdc/0x178
>   __driver_attach+0x9c/0x1b8
>   bus_for_each_dev+0x7c/0xe8
>   driver_attach+0x2c/0x40
>   bus_add_driver+0xec/0x218
>   driver_register+0x68/0x138
>   __register_virtio_driver+0x2c/0x50
>   virtio_blk_init+0x74/0xd0
>   do_one_initcall+0x64/0x290
>   kernel_init_freeable+0x214/0x408
>   kernel_init+0x2c/0x1f0
>   ret_from_fork+0x10/0x20
> 
>>From the experimental results, we can see that virtio block device is 
> executing blk_throtl_activated() during initialization in do_one_initcall()
> when loading virio-blk module_init().
> 
> Combined with the information that blk-throttle is also loaded as a
> built-in module, there exists a scenario where the block device probes
> first, at which point the plid has not been allocated yet, and
> blk_throtl_activated() is executed, followed by the loading of the
> blk-throttle built-in module.
> 
> Given this experimental evidence, I'm wondering if we should consider
> keeping the q->td != NULL check as a safeguard against this initialization
> race condition. I'd appreciate your thoughts on this matter.
> 

Yes, this make sense, thanks for the explanation. Please add comments
about this as well.

Thanks,
Kuai

> Thanks,
> Han Guangjiang
> 
> .
>