Re: [RFC PATCH v1 2/3] blk-cgroup: store blkcg in bio instead of blkg

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Dropping a bio's pinned `blkg` reference fails if the cgroup is offlined while IO is in flight, leaking the originally pinned `blkg` and causing a reference underflow on the root `blkg`.
- [High] `bio_blkg()` returns NULL when blkg creation fails and it falls back to an ancestor, leading to an immediate kernel panic in blk-throttle.
- [Medium] Standard IO statistics (io.stat) are silently dropped because the block cgroup association no longer creates missing blkgs by default.
--

--- Patch [2]: [RFC PATCH v1 2/3] blk-cgroup: store blkcg in bio instead of blkg ---
commit be98a58b9b6c0f35f61427866bf52e9085c5a028
Author: Yu Kuai <[email protected]>
Subject: blk-cgroup: store blkcg in bio instead of blkg

A bio currently stores a queue-local blkg reference. This forces bio
association and remap paths to look up or create a blkg even when the bio
will never enter a blkcg policy. Store the blkcg css association in the
bio instead, and derive the blkg from the bio's blkcg and current bdev
when a policy needs it.

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 52e7faaff0e5b..68a1d93e6584e 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
[ ... ]
> +static struct blkcg_gq *bio_pinned_blkg(struct bio *bio)
> +{
> +	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> +	struct blkcg_gq *blkg;
> +
> +	rcu_read_lock();
> +	blkg = blkg_lookup(bio_blkcg(bio), q);
> +	rcu_read_unlock();
> +
> +	if (WARN_ON_ONCE(!blkg))
> +		return q->root_blkg;
> +	return blkg;
> +}

[Severity: Critical]
How does this handle a cgroup being offlined while IO is in flight?
If an unprivileged user issues async IO and then immediately removes the
cgroup via rmdir, blkg_destroy() will remove the blkg from the radix tree.
When the bio completes, blkg_lookup() here will fail to find the child blkg
and fall back to returning q->root_blkg.

[ ... ]
> +void bio_put_blkg_ref(struct bio *bio)
> +{
> +	if (bio_flagged(bio, BIO_BLKG_REF)) {
> +		struct blkcg_gq *blkg = bio_pinned_blkg(bio);
> +
> +		blkg_put(blkg);
> +		bio_clear_flag(bio, BIO_BLKG_REF);
> +	}
> +}

[Severity: Critical]
Because bio_pinned_blkg() falls back to q->root_blkg when the child blkg is
no longer in the radix tree, blkg_put() here will drop the reference on the
root blkg instead of the actually pinned child blkg. Will this leak the
child blkg and eventually cause a use-after-free panic on the root blkg
due to the reference underflow?

[ ... ]
> +struct blkcg_gq *bio_blkg(struct bio *bio)
> +{
> +	struct blkcg *blkcg = bio_blkcg(bio);
> +	struct gendisk *disk;
> +	struct request_queue *q;
> +	struct blkcg_gq *blkg;
> +
> +	if (!blkcg || !bio->bi_bdev)
> +		return NULL;
> +
> +	if (bio_flagged(bio, BIO_BLKG_REF))
> +		return bio_pinned_blkg(bio);
> +
> +	disk = bio->bi_bdev->bd_disk;
> +	q = disk->queue;
>  
>  	spin_lock_irq(&q->queue_lock);
> -	blkg = blkg_lookup_create(blkcg, bio->bi_bdev->bd_disk);
> -	if (blkg)
> -		blkg = blkg_lookup_tryget(blkg);
> +	blkg = blkg_lookup_create(blkcg, disk);
> +	if (blkg && blkg->blkcg == blkcg && blkg_tryget(blkg))
> +		bio_set_flag(bio, BIO_BLKG_REF);
> +	else
> +		blkg = NULL;
>  	spin_unlock_irq(&q->queue_lock);
>  
>  	return blkg;
>  }

[Severity: High]
Is it intentional to return NULL when blkg_lookup_create() falls back
to an ancestor? If blkg creation fails due to ENOMEM, it correctly
returns the closest alive ancestor. But this strict blkg->blkcg == blkcg
check fails, explicitly returning NULL. Since blk-throttle unconditionally
dereferences the returned blkg (for example in __blk_throtl_bio() via
blkg_to_tg(blkg)->td), won't this cause a kernel panic under memory pressure
instead of gracefully falling back?

[ ... ]
>  void blk_cgroup_bio_start(struct bio *bio)
>  {
> -	struct blkcg_gq *blkg = bio_blkg(bio);
>  	struct blkcg *blkcg = bio_blkcg(bio);
> +	struct blkcg_gq *blkg;
>  	int rwd = blk_cgroup_io_type(bio), cpu;
>  	struct blkg_iostat_set *bis;
>  	unsigned long flags;
>  
>  	if (!cgroup_subsys_on_dfl(io_cgrp_subsys))
>  		return;
> +	if (!blkcg)
> +		return;
>  
>  	/* Root-level stats are sourced from system-wide IO stats */
>  	if (!cgroup_parent(blkcg->css.cgroup))
>  		return;
>  
> +	blkg = bio_blkg_lookup(bio);
> +	if (!blkg)
> +		return;

[Severity: Medium]
Can this silently drop standard IO statistics (io.stat)? Before this
patch, bio_associate_blkg() proactively created the blkg. Now, missing
blkgs are not created by default. If no other active IO policy like
throttle or BFQ processes the bio and explicitly calls bio_blkg() to
create the hierarchy, bio_blkg_lookup() returns NULL and
blk_cgroup_bio_start() aborts. Does this completely lose cgroup IO accounting
on disks without active block IO policies?

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