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

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

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

								Honza

[PATCH 1/3] bfq: Avoid false marking of bic as stably merged

[Jan Kara]

bfq_setup_cooperator() can mark bic as stably merged even though it
decides to not merge its bfqqs (when bfq_setup_merge() returns NULL).
Make sure to mark bic as stably merged only if we are really going to
merge bfqqs.

CC: stable@vger.kernel.org
Fixes: 430a67f9d616 ("block, bfq: merge bursts of newly-created queues")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bfq-iosched.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index fec18118dc30..056399185c2f 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2762,9 +2762,12 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 				struct bfq_queue *new_bfqq =
 					bfq_setup_merge(bfqq, stable_merge_bfqq);
 
-				bic->stably_merged = true;
-				if (new_bfqq && new_bfqq->bic)
-					new_bfqq->bic->stably_merged = true;
+				if (new_bfqq) {
+					bic->stably_merged = true;
+					if (new_bfqq->bic)
+						new_bfqq->bic->stably_merged =
+									true;
+				}
 				return new_bfqq;
 			} else
 				return NULL;
-- 
2.26.2

[PATCH 2/3] bfq: Avoid merging queues with different parents

[Jan Kara]

It can happen that the parent of a bfqq changes between the moment we
decide two queues are worth to merge (and set bic->stable_merge_bfqq)
and the moment bfq_setup_merge() is called. This can happen e.g. because
the process submitted IO for a different cgroup and thus bfqq got
reparented. It can even happen that the bfqq we are merging with has
parent cgroup that is already offline and going to be destroyed in which
case the merge can lead to use-after-free issues such as:

BUG: KASAN: use-after-free in __bfq_deactivate_entity+0x9cb/0xa50
Read of size 8 at addr ffff88800693c0c0 by task runc:[2:INIT]/10544

CPU: 0 PID: 10544 Comm: runc:[2:INIT] Tainted: G            E     5.15.2-0.g5fb85fd-default #1 openSUSE Tumbleweed (unreleased) f1f3b891c72369aebecd2e43e4641a6358867c70
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a-rebuilt.opensuse.org 04/01/2014
Call Trace:
 <IRQ>
 dump_stack_lvl+0x46/0x5a
 print_address_description.constprop.0+0x1f/0x140
 ? __bfq_deactivate_entity+0x9cb/0xa50
 kasan_report.cold+0x7f/0x11b
 ? __bfq_deactivate_entity+0x9cb/0xa50
 __bfq_deactivate_entity+0x9cb/0xa50
 ? update_curr+0x32f/0x5d0
 bfq_deactivate_entity+0xa0/0x1d0
 bfq_del_bfqq_busy+0x28a/0x420
 ? resched_curr+0x116/0x1d0
 ? bfq_requeue_bfqq+0x70/0x70
 ? check_preempt_wakeup+0x52b/0xbc0
 __bfq_bfqq_expire+0x1a2/0x270
 bfq_bfqq_expire+0xd16/0x2160
 ? try_to_wake_up+0x4ee/0x1260
 ? bfq_end_wr_async_queues+0xe0/0xe0
 ? _raw_write_unlock_bh+0x60/0x60
 ? _raw_spin_lock_irq+0x81/0xe0
 bfq_idle_slice_timer+0x109/0x280
 ? bfq_dispatch_request+0x4870/0x4870
 __hrtimer_run_queues+0x37d/0x700
 ? enqueue_hrtimer+0x1b0/0x1b0
 ? kvm_clock_get_cycles+0xd/0x10
 ? ktime_get_update_offsets_now+0x6f/0x280
 hrtimer_interrupt+0x2c8/0x740

Fix the problem by checking that the parent of the two bfqqs we are
merging in bfq_setup_merge() is the same.

Link: https://lore.kernel.org/linux-block/20211125172809.GC19572@quack2.suse.cz/
CC: stable@vger.kernel.org
Fixes: 430a67f9d616 ("block, bfq: merge bursts of newly-created queues")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bfq-iosched.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 056399185c2f..0da47f2ca781 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2638,6 +2638,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
 	if (process_refs == 0 || new_process_refs == 0)
 		return NULL;
 
+	/*
+	 * Make sure merged queues belong to the same parent. Parents could
+	 * have changed since the time we decided the two queues are suitable
+	 * for merging.
+	 */
+	if (new_bfqq->entity.parent != bfqq->entity.parent)
+		return NULL;
+
 	bfq_log_bfqq(bfqq->bfqd, bfqq, "scheduling merge with queue %d",
 		new_bfqq->pid);
 
-- 
2.26.2

[PATCH 3/3] bfq: Split shared queues on move between cgroups

[Jan Kara]

When bfqq is shared by multiple processes it can happen that one of the
processes gets moved to a different cgroup (or just starts submitting IO
for different cgroup). In case that happens we need to split the merged
bfqq as otherwise we will have IO for multiple cgroups in one bfqq and
we will just account IO time to wrong entities etc.

CC: stable@vger.kernel.org
Fixes: e21b7a0b9887 ("block, bfq: add full hierarchical scheduling and cgroups support")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bfq-cgroup.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 24a5c5329bcd..1f5fb723bed7 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -730,8 +730,18 @@ static struct bfq_group *__bfq_bic_change_cgroup(struct bfq_data *bfqd,
 
 	if (sync_bfqq) {
 		entity = &sync_bfqq->entity;
-		if (entity->sched_data != &bfqg->sched_data)
+		if (entity->sched_data != &bfqg->sched_data) {
+			/*
+			 * Moving bfqq that is shared with another process?
+			 * Split the queues at the nearest occasion as the
+			 * processes can be in different cgroups now.
+			 */
+			if (bfq_bfqq_coop(sync_bfqq)) {
+				bic->stably_merged = false;
+				bfq_mark_bfqq_split_coop(sync_bfqq);
+			}
 			bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
+		}
 	}
 
 	return bfqg;
-- 
2.26.2

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

[yukuai (C)]

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

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
[  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
[  548.456191]  kobject_put+0xd4/0x270
[  548.456657]  disk_release+0xc5/0xf0
[  548.457140]  device_release+0x56/0xe0
[  548.457634]  kobject_put+0xd4/0x270
[  548.458113]  null_del_dev.part.0+0xe6/0x220 [null_blk]
[  548.458810]  null_exit+0x62/0xa6 [null_blk]
[  548.459404]  __x64_sys_delete_module+0x20a/0x2f0
[  548.460046]  ? __ia32_sys_delete_module+0x2f0/0x2f0
[  548.460716]  ? fpregs_assert_state_consistent+0x55/0x60
[  548.461436]  ? exit_to_user_mode_prepare+0x39/0x1e0
[  548.462116]  do_syscall_64+0x35/0x80
[  548.462603]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  548.463292] RIP: 0033:0x7fdabfb0efc7
[  548.463777] Code: 73 01 c3 48 8b 0d c1 de 2b 00 f7 d8 64 89 01 48 83 
c8 ff c3 66 2e 0f 1f 8
[  548.466251] RSP: 002b:00007ffd678e61d8 EFLAGS: 00000206 ORIG_RAX: 
00000000000000b0
[  548.467286] RAX: ffffffffffffffda RBX: 00007ffd678e6238 RCX: 
00007fdabfb0efc7
[  548.468237] RDX: 000000000000000a RSI: 0000000000000800 RDI: 
000055b176bb1258
[  548.469187] RBP: 000055b176bb11f0 R08: 00007ffd678e5151 R09: 
0000000000000000
[  548.470130] R10: 00007fdabfb81260 R11: 0000000000000206 R12: 
00007ffd678e6400
[  548.471083] R13: 00007ffd678e845b R14: 000055b176bb0010 R15: 
000055b176bb11f0
[  548.472051]  </TASK>
[  548.472351]
[  548.472569] Allocated by task 13874:
[  548.473066]  kasan_save_stack+0x1e/0x40
[  548.473579]  __kasan_kmalloc+0x81/0xa0
[  548.474082]  bfq_pd_alloc+0x72/0x110
[  548.474551]  blkg_alloc+0x252/0x2c0
[  548.475034]  blkg_create+0x38e/0x560
[  548.475508]  bio_associate_blkg_from_css+0x3bc/0x490
[  548.476169]  bio_associate_blkg+0x3b/0x90
[  548.476701]  submit_bh_wbc+0x18c/0x320
[  548.477207]  ll_rw_block+0x126/0x130
[  548.477681]  __block_write_begin_int+0x6fc/0xee0
[  548.478299]  block_write_begin+0x44/0x190
[  548.478830]  generic_perform_write+0x159/0x300
[  548.479441]  __generic_file_write_iter+0x162/0x250
[  548.480086]  blkdev_write_iter+0x21a/0x300
[  548.480628]  aio_write+0x218/0x400
[  548.481084]  io_submit_one+0x855/0x11c0
[  548.481587]  __x64_sys_io_submit+0xfa/0x250
[  548.482140]  do_syscall_64+0x35/0x80
[  548.482615]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  548.483298]
[  548.483502] Freed by task 13855:
[  548.483929]  kasan_save_stack+0x1e/0x40
[  548.484439]  kasan_set_track+0x21/0x30
[  548.484940]  kasan_set_free_info+0x20/0x30
[  548.485486]  __kasan_slab_free+0x103/0x180
[  548.486032]  kfree+0x9a/0x4b0
[  548.486425]  blkg_free.part.0+0x4a/0x90
[  548.486931]  rcu_do_batch+0x2e1/0x760
[  548.487440]  rcu_core+0x367/0x530
[  548.487879]  __do_softirq+0x119/0x3ba

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;
+               }
                 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;
+       }

Thanks,
Kuai
> 
> 								Honza
> .
> 

Re: [PATCH 2/3] bfq: Avoid merging queues with different parents

[yukuai (C)]

在 2021/12/24 1:31, Jan Kara 写道:
> It can happen that the parent of a bfqq changes between the moment we
> decide two queues are worth to merge (and set bic->stable_merge_bfqq)
> and the moment bfq_setup_merge() is called. This can happen e.g. because
> the process submitted IO for a different cgroup and thus bfqq got
> reparented. It can even happen that the bfqq we are merging with has
> parent cgroup that is already offline and going to be destroyed in which
> case the merge can lead to use-after-free issues such as:
> 
> BUG: KASAN: use-after-free in __bfq_deactivate_entity+0x9cb/0xa50
> Read of size 8 at addr ffff88800693c0c0 by task runc:[2:INIT]/10544
> 
> CPU: 0 PID: 10544 Comm: runc:[2:INIT] Tainted: G            E     5.15.2-0.g5fb85fd-default #1 openSUSE Tumbleweed (unreleased) f1f3b891c72369aebecd2e43e4641a6358867c70
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a-rebuilt.opensuse.org 04/01/2014
> Call Trace:
>   <IRQ>
>   dump_stack_lvl+0x46/0x5a
>   print_address_description.constprop.0+0x1f/0x140
>   ? __bfq_deactivate_entity+0x9cb/0xa50
>   kasan_report.cold+0x7f/0x11b
>   ? __bfq_deactivate_entity+0x9cb/0xa50
>   __bfq_deactivate_entity+0x9cb/0xa50
>   ? update_curr+0x32f/0x5d0
>   bfq_deactivate_entity+0xa0/0x1d0
>   bfq_del_bfqq_busy+0x28a/0x420
>   ? resched_curr+0x116/0x1d0
>   ? bfq_requeue_bfqq+0x70/0x70
>   ? check_preempt_wakeup+0x52b/0xbc0
>   __bfq_bfqq_expire+0x1a2/0x270
>   bfq_bfqq_expire+0xd16/0x2160
>   ? try_to_wake_up+0x4ee/0x1260
>   ? bfq_end_wr_async_queues+0xe0/0xe0
>   ? _raw_write_unlock_bh+0x60/0x60
>   ? _raw_spin_lock_irq+0x81/0xe0
>   bfq_idle_slice_timer+0x109/0x280
>   ? bfq_dispatch_request+0x4870/0x4870
>   __hrtimer_run_queues+0x37d/0x700
>   ? enqueue_hrtimer+0x1b0/0x1b0
>   ? kvm_clock_get_cycles+0xd/0x10
>   ? ktime_get_update_offsets_now+0x6f/0x280
>   hrtimer_interrupt+0x2c8/0x740
> 
> Fix the problem by checking that the parent of the two bfqqs we are
> merging in bfq_setup_merge() is the same.
> 
> Link: https://lore.kernel.org/linux-block/20211125172809.GC19572@quack2.suse.cz/
> CC: stable@vger.kernel.org
> Fixes: 430a67f9d616 ("block, bfq: merge bursts of newly-created queues")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>   block/bfq-iosched.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 056399185c2f..0da47f2ca781 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2638,6 +2638,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
>   	if (process_refs == 0 || new_process_refs == 0)
>   		return NULL;
>   
> +	/*
> +	 * Make sure merged queues belong to the same parent. Parents could
> +	 * have changed since the time we decided the two queues are suitable
> +	 * for merging.
> +	 */
> +	if (new_bfqq->entity.parent != bfqq->entity.parent)
> +		return NULL;
> +
Hi,

This seems unnecessary, the caller of bfq_setup_merge() aready make sure
bfqq and new_bfqq are under the same bfqg. Am I missing something?

Thanks,
Kuai
>   	bfq_log_bfqq(bfqq->bfqd, bfqq, "scheduling merge with queue %d",
>   		new_bfqq->pid);
>   
> 

Re: [PATCH 2/3] bfq: Avoid merging queues with different parents

[Jan Kara]

On Fri 24-12-21 09:45:02, yukuai (C) wrote:
> 在 2021/12/24 1:31, Jan Kara 写道:
> > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > index 056399185c2f..0da47f2ca781 100644
> > --- a/block/bfq-iosched.c
> > +++ b/block/bfq-iosched.c
> > @@ -2638,6 +2638,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct bfq_queue *new_bfqq)
> >   	if (process_refs == 0 || new_process_refs == 0)
> >   		return NULL;
> > +	/*
> > +	 * Make sure merged queues belong to the same parent. Parents could
> > +	 * have changed since the time we decided the two queues are suitable
> > +	 * for merging.
> > +	 */
> > +	if (new_bfqq->entity.parent != bfqq->entity.parent)
> > +		return NULL;
> > +
> Hi,
> 
> This seems unnecessary, the caller of bfq_setup_merge() aready make sure
> bfqq and new_bfqq are under the same bfqg. Am I missing something?

Not all the callers of bfq_setup_merge() check that queues belong to the
same cgroup (e.g. bfq_setup_cooperator() does not seem to check).

								Honza

> 
> Thanks,
> Kuai
> >   	bfq_log_bfqq(bfqq->bfqd, bfqq, "scheduling merge with queue %d",
> >   		new_bfqq->pid);
> > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

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

>                 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

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

[yukuai (C)]

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