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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

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