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@xxxxxxxx>
SUSE Labs, CR
