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

["yukuai (C)" <yukuai3@huawei.com>]

在 2022/01/04 4:37, Jan Kara 写道:
> 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?

Hi,

I'm sure that in my repoducer bfq_setup_merge() nerver merge two
bfqq from diferent group(I added some debuginfo before), and somehow
later bfqq and bfqq->new_bfqq end up from different bfqg.

I haven't thought of merge bfqqs from diferent group yet. My reporducer
probably can't cover this case...

This modification is just a lazy detection for the problem. It's right
that fix the problem in the first scene is better.

Thanks,
Kuai

> 
>>                  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
>