Re: [RFC PATCH v1 1/3] blk-cgroup: add helpers for bio cgroup state

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

Pre-existing issues:
- [High] Potential Use-After-Free of `cgroup_subsys_state` (css) when re-associating a bio's block cgroup.
--

--- Patch [1]: [RFC PATCH v1 1/3] blk-cgroup: add helpers for bio cgroup state ---
commit 00d4f5448146c9d8e060d7ebf96b61cd64d0f752
Author: Yu Kuai <[email protected]>

blk-cgroup: add helpers for bio cgroup state

This commit introduces helpers like bio_blkg() and bio_blkcg() to replace
open-coded cgroup state accesses throughout the block layer. The helpers
are applied to read-side and release paths to prepare for future changes
to bio internal cgroup state storage.

> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index d9676126c5b5d..52e7faaff0e5b 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -2113,8 +2113,8 @@ static inline struct blkcg_gq *blkg_tryget_closest(struct bio *bio,
>  void bio_associate_blkg_from_css(struct bio *bio,
>  				 struct cgroup_subsys_state *css)
>  {
> -	if (bio->bi_blkg)
> -		blkg_put(bio->bi_blkg);
> +	if (bio_blkg(bio))
> +		blkg_put(bio_blkg(bio));
>  
>  	if (css && css->parent) {
>  		bio->bi_blkg = blkg_tryget_closest(bio, css);
> @@ -2141,7 +2141,7 @@ void bio_associate_blkg(struct bio *bio)
>  	if (blk_op_is_passthrough(bio->bi_opf))
>  		return;
>  
> -	if (bio->bi_blkg) {
> +	if (bio_blkg(bio)) {
>  		css = bio_blkcg_css(bio);
>  		bio_associate_blkg_from_css(bio, css);

[Severity: High]
This is a pre-existing issue, but could this sequence lead to a use-after-free
of the cgroup_subsys_state (css)?

In bio_associate_blkg(), we extract the css pointer from the bio's blkg
without acquiring a new reference or holding an RCU read lock:

bio_associate_blkg()
    css = bio_blkcg_css(bio);
    bio_associate_blkg_from_css(bio, css);

Then in bio_associate_blkg_from_css(), we immediately drop the bio's blkg
reference:

bio_associate_blkg_from_css()
    if (bio_blkg(bio))
        blkg_put(bio_blkg(bio));
        
    if (css && css->parent) {  <-- potential UAF

If this was the final reference to the blkg (for example, if the cgroup was
removed while I/O was in-flight and a stacked block driver remaps the bio),
the blkg and its css are queued for destruction. 

If the thread is preempted right after blkg_put(), could RCU grace periods
pass and the css memory be freed via workqueues before it is dereferenced
at css->parent?

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