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
