Re: [RFC PATCH v1 3/3] blk-cgroup: move async bio punt state to blkcg

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A race condition in `blkcg_punt_bio_submit()` allows a use-after-free on `struct blkcg` because `queue_work()` is called after dropping the lock without holding a local reference to the cgroup.
--

--- Patch [3]: [RFC PATCH v1 3/3] blk-cgroup: move async bio punt state to blkcg ---
commit 0ed7c55259ed7cd55caede491e674b21be72ca58
Author: Yu Kuai <[email protected]>

blk-cgroup: move async bio punt state to blkcg

This commit moves the async bio punt state from blkg to blkcg.
This preserves priority-inversion avoidance while preventing
blkcg_punt_bio_submit() from creating blkgs that are not needed
by any policy.

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 68a1d93e6584e..a70f92f1541b1 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c

[ ... ]

> @@ -239,15 +234,15 @@ static void blkg_async_bio_workfn(struct work_struct *work)
>   */
>  void blkcg_punt_bio_submit(struct bio *bio)
>  {
> -	struct blkcg_gq *blkg = bio_blkg(bio);
> +	struct blkcg *blkcg = bio_blkcg(bio);
>  
> -	if (blkg && blkg->parent) {
> -		spin_lock(&blkg->async_bio_lock);
> -		bio_list_add(&blkg->async_bios, bio);
> -		spin_unlock(&blkg->async_bio_lock);
> -		queue_work(blkcg_punt_bio_wq, &blkg->async_bio_work);
> +	if (blkcg && cgroup_parent(blkcg->css.cgroup)) {
> +		spin_lock(&blkcg->async_bio_lock);
> +		bio_list_add(&blkcg->async_bios, bio);
> +		spin_unlock(&blkcg->async_bio_lock);
> +		queue_work(blkcg_punt_bio_wq, &blkcg->async_bio_work);

[Severity: High]
This is a pre-existing issue, but could this race window between
spin_unlock() and queue_work() lead to a use-after-free on the blkcg structure?

If Thread A calls blkcg_punt_bio_submit(), places the bio into async_bios
under the spinlock, and is then preempted right after unlocking:

Thread A:
    spin_unlock(&blkcg->async_bio_lock);
    <preempted>

A concurrent worker thread might awaken, acquire the lock, extract the
newly added bio, and submit it. If the bio completes very quickly, it will
drop its css reference.

If this was the last reference keeping blkcg alive (e.g. if the cgroup was
being deleted), blkcg_css_free() could execute and free the blkcg:

blkcg_css_free()
    ...
    free_percpu(blkcg->lhead);
    kfree(blkcg);

When Thread A resumes, wouldn't it execute queue_work() on the freed blkcg
memory?

>  	} else {
> -		/* Never bounce if there is no non-root blkg to queue on. */
> +		/* Never bounce if there is no non-root blkcg to queue on. */
>  		submit_bio(bio);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3