Re: [PATCH 0/3] bfq: Avoid use-after-free when moving processes between cgroups

[Jan Kara <jack@suse.cz>]

On Fri 24-12-21 09:30:04, yukuai (C) wrote:
> 在 2021/12/24 1:31, Jan Kara 写道:
> > Hello,
> > 
> > these three patches fix use-after-free issues in BFQ when processes with merged
> > queues get moved to different cgroups. The patches have survived some beating
> > in my test VM but so far I fail to reproduce the original KASAN reports so
> > testing from people who can reproduce them is most welcome. Thanks!
> 
> Hi,
> 
> Unfortunately, this patchset can't fix the UAF, just to mark
> split_coop in patch 3 seems not enough.

Thanks for testing! 

> Here is the result:
> 
> [  548.440184]
> ==============================================================
> [  548.441680] BUG: KASAN: use-after-free in
> __bfq_deactivate_entity+0x21/0x290
> [  548.443155] Read of size 1 at addr ffff8881723e00b0 by task rmmod/13984
> [  548.444109]
> [  548.444321] CPU: 30 PID: 13984 Comm: rmmod Tainted: G        W
> 5.16.0-rc5-next-2026
> [  548.445549] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> ?-20190727_073836-4
> [  548.447348] Call Trace:
> [  548.447682]  <TASK>
> [  548.447967]  dump_stack_lvl+0x34/0x44
> [  548.448470]  print_address_description.constprop.0.cold+0xab/0x36b
> [  548.449303]  ? __bfq_deactivate_entity+0x21/0x290
> [  548.449929]  ? __bfq_deactivate_entity+0x21/0x290
> [  548.450565]  kasan_report.cold+0x83/0xdf
> [  548.451114]  ? _raw_read_lock_bh+0x20/0x40
> [  548.451658]  ? __bfq_deactivate_entity+0x21/0x290
> [  548.452296]  __bfq_deactivate_entity+0x21/0x290
> [  548.452917]  bfq_pd_offline+0xc1/0x110

Can you pass the trace through addr2line please? I'm curious whether this
is a call in bfq_flush_idle_tree() or directly from bfq_pd_offline(). Also
whether the crash in __bfq_deactivate_entity() is indeed inside
bfq_entity_service_tree() as I expect.

> [  548.453436]  blkcg_deactivate_policy+0x14b/0x210
> [  548.454058]  bfq_exit_queue+0xe5/0x100
> [  548.454573]  blk_mq_exit_sched+0x113/0x140
> [  548.455162]  elevator_exit+0x30/0x50
> [  548.455645]  blk_release_queue+0xa8/0x160

So I'm not convinced this is the same problem as before. I'll be able to
tell more when I see the addr2line output.

> How do you think about this:
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 1ce1a99a7160..14c1d1c3811e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2626,6 +2626,11 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> bfq_queue *new_bfqq)
>         while ((__bfqq = new_bfqq->new_bfqq)) {
>                 if (__bfqq == bfqq)
>                         return NULL;
> +               if (__bfqq->entity.parent != bfqq->entity.parent) {
> +                       if (bfq_bfqq_coop(__bfqq))
> +                               bfq_mark_bfqq_split_coop(__bfqq);
> +                       return NULL;
> +               }

So why is this needed? In my patches we do check in bfq_setup_merge() that
the bfqq we merge to is in the same cgroup. If some intermediate bfqq
happens to move to a different cgroup between the detection and merge, well
that's a bad luck but practically I don't think that is a real problem and
it should not cause the use-after-free issues. Or am I missing something?

>                 new_bfqq = __bfqq;
>         }
> 
> @@ -2825,8 +2830,16 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct
> bfq_queue *bfqq,
>         if (bfq_too_late_for_merging(bfqq))
>                 return NULL;
> 
> -       if (bfqq->new_bfqq)
> -               return bfqq->new_bfqq;
> +       if (bfqq->new_bfqq) {
> +               struct bfq_queue *new_bfqq = bfqq->new_bfqq;
> +
> +               if(bfqq->entity.parent == new_bfqq->entity.parent)
> +                       return new_bfqq;
> +
> +               if(bfq_bfqq_coop(new_bfqq))
> +                       bfq_mark_bfqq_split_coop(new_bfqq);
> +               return NULL;
> +       }

So I wanted to say that this should be already handled by the change to
__bfq_bic_change_cgroup() in my series. But bfq_setup_cooperator() can also
be called from bfq_allow_bio_merge() and on that path we don't call
bfq_bic_update_cgroup() so indeed we can merge bfqq to a bfqq in a
different (or dead) cgroup through that path. I actually think we should
call bfq_bic_update_cgroup() in bfq_allow_bio_merge() so that we reparent /
split bfqq when new IO arrives for a different cgroup. 

Also I have realized that my change to __bfq_bic_change_cgroup() may not be
enough and we should also clear sync_bfqq->new_bfqq if it is set.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR