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

[no-reply@sashiko.virtuozzo.com]

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