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
