[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.

 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
>>>
> 
> .
>