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

[PATCH 1/4] 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.31.1

[PATCH 2/4] 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.31.1

[PATCH 3/4] 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.

Similarly if the bfqq is scheduled to merge with another bfqq but the
merge didn't happen yet, cancel the merge as it need not be valid
anymore.

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  | 25 ++++++++++++++++++++++++-
 block/bfq-iosched.c |  2 +-
 block/bfq-iosched.h |  1 +
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 24a5c5329bcd..dbc117e00783 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -730,8 +730,31 @@ 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) {
+			/*
+			 * Was the queue we use merged to a different queue?
+			 * Detach process from the queue as merge need not be
+			 * valid anymore. We cannot easily cancel the merge as
+			 * there may be other processes scheduled to this
+			 * queue.
+			 */
+			if (sync_bfqq->new_bfqq) {
+				bfq_put_cooperator(sync_bfqq);
+				bfq_release_process_ref(bfqd, sync_bfqq);
+				bic_set_bfqq(bic, NULL, 1);
+				return bfqg;
+			}
+			/*
+			 * 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;
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 0da47f2ca781..361d321b012a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5184,7 +5184,7 @@ static void bfq_put_stable_ref(struct bfq_queue *bfqq)
 	bfq_put_queue(bfqq);
 }
 
-static void bfq_put_cooperator(struct bfq_queue *bfqq)
+void bfq_put_cooperator(struct bfq_queue *bfqq)
 {
 	struct bfq_queue *__bfqq, *next;
 
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index a73488eec8a4..6e250db2138e 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -976,6 +976,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
 void bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 		     bool compensate, enum bfqq_expiration reason);
 void bfq_put_queue(struct bfq_queue *bfqq);
+void bfq_put_cooperator(struct bfq_queue *bfqq);
 void bfq_end_wr_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg);
 void bfq_release_process_ref(struct bfq_data *bfqd, struct bfq_queue *bfqq);
 void bfq_schedule_dispatch(struct bfq_data *bfqd);
-- 
2.31.1

[PATCH 4/4] bfq: Update cgroup information before merging bio

[Jan Kara]

When the process is migrated to a different cgroup (or in case of
writeback just starts submitting bios associated with a different
cgroup) bfq_merge_bio() can operate with stale cgroup information in
bic. Thus the bio can be merged to a request from a different cgroup or
it can result in merging of bfqqs for different cgroups or bfqqs of
already dead cgroups and causing possible use-after-free issues. Fix the
problem by updating cgroup information in bfq_merge_bio().

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-iosched.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 361d321b012a..8a088d77a0b6 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2337,10 +2337,17 @@ static bool bfq_bio_merge(struct request_queue *q, struct bio *bio,
 
 	spin_lock_irq(&bfqd->lock);
 
-	if (bic)
+	if (bic) {
+		/*
+		 * Make sure cgroup info is uptodate for current process before
+		 * considering the merge.
+		 */
+		bfq_bic_update_cgroup(bic, bio);
+
 		bfqd->bio_bfqq = bic_to_bfqq(bic, op_is_sync(bio->bi_opf));
-	else
+	} else {
 		bfqd->bio_bfqq = NULL;
+	}
 	bfqd->bio_bic = bic;
 
 	ret = blk_mq_sched_try_merge(q, bio, nr_segs, &free);
-- 
2.31.1

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

[yukuai (C)]

在 2022/01/12 19:39, 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.
> 
> Similarly if the bfqq is scheduled to merge with another bfqq but the
> merge didn't happen yet, cancel the merge as it need not be valid
> anymore.
> 
> 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  | 25 ++++++++++++++++++++++++-
>   block/bfq-iosched.c |  2 +-
>   block/bfq-iosched.h |  1 +
>   3 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index 24a5c5329bcd..dbc117e00783 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -730,8 +730,31 @@ 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) {
> +			/*
> +			 * Was the queue we use merged to a different queue?
> +			 * Detach process from the queue as merge need not be
> +			 * valid anymore. We cannot easily cancel the merge as
> +			 * there may be other processes scheduled to this
> +			 * queue.
> +			 */
> +			if (sync_bfqq->new_bfqq) {
> +				bfq_put_cooperator(sync_bfqq);
Hi,

The patch " bfq: Simplify bfq_put_cooperator()" in last version is not
in this patch set, thus bfq_put_cooperator() won't set
sync_bfqq->new_bfqq to NULL. So I guess the problem still exist?

Thanks,
Kuai
> +				bfq_release_process_ref(bfqd, sync_bfqq);
> +				bic_set_bfqq(bic, NULL, 1);
> +				return bfqg;
> +			}
> +			/*
> +			 * 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;
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 0da47f2ca781..361d321b012a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5184,7 +5184,7 @@ static void bfq_put_stable_ref(struct bfq_queue *bfqq)
>   	bfq_put_queue(bfqq);
>   }
>   
> -static void bfq_put_cooperator(struct bfq_queue *bfqq)
> +void bfq_put_cooperator(struct bfq_queue *bfqq)
>   {
>   	struct bfq_queue *__bfqq, *next;
>   
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index a73488eec8a4..6e250db2138e 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -976,6 +976,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>   void bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>   		     bool compensate, enum bfqq_expiration reason);
>   void bfq_put_queue(struct bfq_queue *bfqq);
> +void bfq_put_cooperator(struct bfq_queue *bfqq);
>   void bfq_end_wr_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg);
>   void bfq_release_process_ref(struct bfq_data *bfqd, struct bfq_queue *bfqq);
>   void bfq_schedule_dispatch(struct bfq_data *bfqd);
> 

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

[yukuai (C)]

在 2022/01/13 11:57, yukuai (C) 写道:
> 在 2022/01/12 19:39, 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.
>>
>> Similarly if the bfqq is scheduled to merge with another bfqq but the
>> merge didn't happen yet, cancel the merge as it need not be valid
>> anymore.
>>
>> 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  | 25 ++++++++++++++++++++++++-
>>   block/bfq-iosched.c |  2 +-
>>   block/bfq-iosched.h |  1 +
>>   3 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
>> index 24a5c5329bcd..dbc117e00783 100644
>> --- a/block/bfq-cgroup.c
>> +++ b/block/bfq-cgroup.c
>> @@ -730,8 +730,31 @@ 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) {
>> +            /*
>> +             * Was the queue we use merged to a different queue?
>> +             * Detach process from the queue as merge need not be
>> +             * valid anymore. We cannot easily cancel the merge as
>> +             * there may be other processes scheduled to this
>> +             * queue.
>> +             */
>> +            if (sync_bfqq->new_bfqq) {
>> +                bfq_put_cooperator(sync_bfqq);
> Hi,
> 
> The patch " bfq: Simplify bfq_put_cooperator()" in last version is not
> in this patch set, thus bfq_put_cooperator() won't set
> sync_bfqq->new_bfqq to NULL. So I guess the problem still exist?
> 
> Thanks,
> Kuai
>> +                bfq_release_process_ref(bfqd, sync_bfqq);
>> +                bic_set_bfqq(bic, NULL, 1);
Hi,

I understand now that you set NULL here, however I still can repoduce
the problem with this patch set applied.

Here I add some debug message:

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index dbc117e00783..2d3da8e73ec0 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -754,6 +754,7 @@ static struct bfq_group 
*__bfq_bic_change_cgroup(struct bfq_data *bfqd,
                                 bfq_mark_bfqq_split_coop(sync_bfqq);
                         }
                         bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
+                       printk("%s: bfqq %px move to %px\n", __func__, 
sync_bfqq, bfqg);
                 }
         }

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 07be51bc229b..74d5b575626f 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2797,6 +2797,7 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct 
bfq_queue *new_bfqq)
          * are likely to increase the throughput.
          */
         bfqq->new_bfqq = new_bfqq;
+       printk("%s: bfqq %px new_bfqq %px bfqg %px\n", __func__, bfqq, 
new_bfqq, bfqq_group(bfqq));
         new_bfqq->ref += process_refs;
         return new_bfqq;
  }
@@ -2963,8 +2964,14 @@ bfq_setup_cooperator(struct bfq_data *bfqd, 
struct bfq_queue *bfqq,
         if (bfq_too_late_for_merging(bfqq))
                 return NULL;

-       if (bfqq->new_bfqq)
+       if (bfqq->new_bfqq) {
+               if (bfqq->entity.parent != bfqq->new_bfqq->entity.parent) {
+                       printk("%s: bfqq %px (%px), new_bfqq %px 
(%px)\n", __func__,
+                                       bfqq, bfqq_group(bfqq), 
bfqq->new_bfqq, bfqq_group(bfqq->new_bfqq));
+                       BUG_ON(1);
+               }
                 return bfqq->new_bfqq;
+       }

         if (!io_struct || unlikely(bfqq == &bfqd->oom_bfqq))
                 return NULL;

And here is the messages when BUG_ON is triggered（much easier than the
uaf problem):

[  157.237821] bfq_setup_merge: bfqq ffff888140654dc0 new_bfqq 
ffff888174122100 bfqg ffff88817abc6000
[  157.238739] __bfq_bic_change_cgroup: bfqq ffff888174122100 move to 
ffff888104b7c000
[  157.239675] bfq_setup_cooperator: bfqq ffff888140654dc0 
(ffff88817abc6000), new_bfqq ffff888174122100 (ffff888104b7c000)
[  157.240696] ------------[ cut here ]------------
[  157.241113] kernel BUG at block/bfq-iosched.c:2971!
[  157.241565] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[  157.242042] CPU: 4 PID: 3089 Comm: fio Tainted: G        W 
5.16.0-next-20220111+ #361
[  157.242810] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 
04/01/2014
[  157.243985] RIP: 0010:bfq_setup_cooperator+0x413/0x7e0
[  157.244470] Code: 8b 65 30 48 89 ef e8 9c ab 00 00 49 89 d9 48 89 ea 
48 c7 c6 00 d9 d9 82 48 89 c1 4d 89 e0 48 c7 c7 80 d4 d9 82 e8 88 3f 91 
00 <0f> 0b 48 8d bd f0 01 00 00 e8 0f 77 84 c
[  157.246132] RSP: 0018:ffff888171ecf1e0 EFLAGS: 00010082
[  157.246609] RAX: 000000000000006c RBX: ffff888104b7c000 RCX: 
0000000000000000
[  157.247243] RDX: 0000000000000002 RSI: ffffffff82da9fc0 RDI: 
ffffed102e3d9e2f
[  157.247889] RBP: ffff888140654dc0 R08: 000000000000006c R09: 
ffffed106cd06751
[  157.248529] R10: ffff888366833a87 R11: ffffed106cd06750 R12: 
ffff888174122100
[  157.249160] R13: 0000000000000000 R14: ffff88817abc5000 R15: 
ffff888174122100
[  157.249801] FS:  00007f21fe8d5700(0000) GS:ffff888366800000(0000) 
knlGS:0000000000000000
[  157.250524] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  157.251042] CR2: 000055f2bb593770 CR3: 0000000105045000 CR4: 
00000000000006e0
[  157.251688] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[  157.252327] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 
0000000000000400
[  157.252970] Call Trace:
[  157.253207]  <TASK>
[  157.253418]  ? bic_set_bfqq+0x80/0x80
[  157.253757]  ? memcpy+0x39/0x60
[  157.254052]  ? blk_integrity_merge_bio+0x2e/0x150
[  157.254490]  bfq_allow_bio_merge+0x9a/0x130
[  157.254874]  elv_merge+0x76/0x1b0
[  157.255185]  blk_mq_sched_try_merge+0x7a/0x2c0
[  157.255603]  ? preempt_count_sub+0x14/0xc0
[  157.255985]  ? bio_attempt_front_merge+0x8b0/0x8b0
[  157.256427]  ? kernfs_path_from_node+0x4c/0x60
[  157.256839]  ? bfq_bic_update_cgroup+0x211/0x2c0
[  157.257269]  bfq_bio_merge+0x141/0x1d0
[  157.257629]  ? bfq_request_merge+0xe0/0xe0
[  157.258001]  ? blk_mq_sched_bio_merge+0x43/0x170
[  157.258426]  blk_mq_submit_bio+0x7a6/0xba0
[  157.258800]  ? submit_bio_checks+0x4ed/0x9f0
[  157.259191]  ? blk_mq_try_issue_list_directly+0x1d0/0x1d0
[  157.259685]  ? mempool_alloc+0x250/0x250
[  157.260040]  ? mempool_destroy+0x20/0x20
[  157.260401]  ? __submit_bio+0x61/0x290
[  157.260751]  submit_bio_noacct+0x441/0x480
[  157.261125]  ? __submit_bio+0x290/0x290
[  157.261486]  ? bio_add_page+0xbc/0x100
[  157.261831]  submit_bh_wbc+0x2e7/0x320
[  157.262179]  __block_write_full_page+0x3f3/0x870
[  157.262609]  ? blkdev_write_end+0x150/0x150
[  157.262994]  ? end_buffer_write_sync+0xa0/0xa0
[  157.263401]  __writepage+0x39/0xb0
[  157.263718]  write_cache_pages+0x27e/0x840
[  157.264091]  ? tag_pages_for_writeback+0x250/0x250
[  157.264538]  ? wb_writeout_inc+0x150/0x150
[  157.264919]  ? _raw_spin_lock_irqsave+0xe0/0xe0
[  157.265339]  ? xas_start+0x7b/0x170
[  157.265673]  generic_writepages+0xb9/0x110
[  157.266042]  ? write_cache_pages+0x840/0x840
[  157.266432]  ? xas_set_mark+0xd0/0xd0
[  157.266765]  do_writepages+0x104/0x320
[  157.267106]  ? __rcu_read_unlock+0x4e/0x250
[  157.267492]  ? page_writeback_cpu_online+0x10/0x10
[  157.267919]  ? _raw_spin_lock+0x87/0xe0
[  157.268264]  ? _raw_spin_lock_irqsave+0xe0/0xe0
[  157.268676]  ? inode_to_bdi+0x79/0x90
[  157.269012]  filemap_fdatawrite_wbc+0x91/0xc0
[  157.269412]  filemap_write_and_wait_range+0xbb/0x140
[  157.269851]  ? filemap_fdatawait_keep_errors+0x30/0x30
[  157.270309]  ? generic_file_direct_write+0xcb/0x250
[  157.270748]  __generic_file_write_iter+0x1fc/0x250
[  157.271175]  blkdev_write_iter+0x21a/0x300
[  157.271547]  ? blkdev_llseek+0x80/0x80
[  157.271884]  ? preempt_count_sub+0x14/0xc0
[  157.272247]  ? __update_load_avg_cfs_rq+0x51/0x530
[  157.272678]  ? __update_load_avg_cfs_rq+0x51/0x530
[  157.273111]  new_sync_write+0x24c/0x360
[  157.273464]  ? new_sync_read+0x360/0x360
[  157.273819]  ? set_next_entity+0xb1/0x280
[  157.274178]  ? preempt_count_sub+0x14/0xc0
[  157.274551]  ? __rcu_read_unlock+0x4e/0x250
[  157.274926]  vfs_write+0x291/0x3d0
[  157.275238]  ksys_pwrite64+0xe7/0x110
[  157.275580]  ? __ia32_sys_pread64+0x60/0x60
[  157.275954]  ? switch_fpu_return+0x9d/0x140
[  157.276329]  do_syscall_64+0x35/0x80
[  157.276659]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  157.277110] RIP: 0033:0x7f220f1f0b23

It seems that when q1 and q2 are merged(q1->new_bfqq = q2), and q2
is moved to another bfqg through __bfq_bic_change_cgroug(), the q2
is not split before the bfq_setup_cooperator() is called.

Thanks,
Kuai

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

[Jan Kara]

On Thu 13-01-22 15:08:50, yukuai (C) wrote:
> 在 2022/01/13 11:57, yukuai (C) 写道:
> > 在 2022/01/12 19:39, 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.
> > > 
> > > Similarly if the bfqq is scheduled to merge with another bfqq but the
> > > merge didn't happen yet, cancel the merge as it need not be valid
> > > anymore.
> > > 
> > > 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  | 25 ++++++++++++++++++++++++-
> > >   block/bfq-iosched.c |  2 +-
> > >   block/bfq-iosched.h |  1 +
> > >   3 files changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> > > index 24a5c5329bcd..dbc117e00783 100644
> > > --- a/block/bfq-cgroup.c
> > > +++ b/block/bfq-cgroup.c
> > > @@ -730,8 +730,31 @@ 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) {
> > > +            /*
> > > +             * Was the queue we use merged to a different queue?
> > > +             * Detach process from the queue as merge need not be
> > > +             * valid anymore. We cannot easily cancel the merge as
> > > +             * there may be other processes scheduled to this
> > > +             * queue.
> > > +             */
> > > +            if (sync_bfqq->new_bfqq) {
> > > +                bfq_put_cooperator(sync_bfqq);
> > Hi,
> > 
> > The patch " bfq: Simplify bfq_put_cooperator()" in last version is not
> > in this patch set, thus bfq_put_cooperator() won't set
> > sync_bfqq->new_bfqq to NULL. So I guess the problem still exist?
> > 
> > Thanks,
> > Kuai
> > > +                bfq_release_process_ref(bfqd, sync_bfqq);
> > > +                bic_set_bfqq(bic, NULL, 1);
> Hi,
> 
> I understand now that you set NULL here,

Yes.

> however I still can repoduce
> the problem with this patch set applied.

OK.

> Here I add some debug message:
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index dbc117e00783..2d3da8e73ec0 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -754,6 +754,7 @@ static struct bfq_group *__bfq_bic_change_cgroup(struct
> bfq_data *bfqd,
>                                 bfq_mark_bfqq_split_coop(sync_bfqq);
>                         }
>                         bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
> +                       printk("%s: bfqq %px move to %px\n", __func__,
> sync_bfqq, bfqg);
>                 }
>         }
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 07be51bc229b..74d5b575626f 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2797,6 +2797,7 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> bfq_queue *new_bfqq)
>          * are likely to increase the throughput.
>          */
>         bfqq->new_bfqq = new_bfqq;
> +       printk("%s: bfqq %px new_bfqq %px bfqg %px\n", __func__, bfqq,
> new_bfqq, bfqq_group(bfqq));
>         new_bfqq->ref += process_refs;
>         return new_bfqq;
>  }
> @@ -2963,8 +2964,14 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct
> bfq_queue *bfqq,
>         if (bfq_too_late_for_merging(bfqq))
>                 return NULL;
> 
> -       if (bfqq->new_bfqq)
> +       if (bfqq->new_bfqq) {
> +               if (bfqq->entity.parent != bfqq->new_bfqq->entity.parent) {
> +                       printk("%s: bfqq %px (%px), new_bfqq %px (%px)\n",
> __func__,
> +                                       bfqq, bfqq_group(bfqq),
> bfqq->new_bfqq, bfqq_group(bfqq->new_bfqq));
> +                       BUG_ON(1);
> +               }
>                 return bfqq->new_bfqq;
> +       }
> 
>         if (!io_struct || unlikely(bfqq == &bfqd->oom_bfqq))
>                 return NULL;
> 
> And here is the messages when BUG_ON is triggered（much easier than the
> uaf problem):
> 
> [  157.237821] bfq_setup_merge: bfqq ffff888140654dc0 new_bfqq
> ffff888174122100 bfqg ffff88817abc6000
> [  157.238739] __bfq_bic_change_cgroup: bfqq ffff888174122100 move to
> ffff888104b7c000
> [  157.239675] bfq_setup_cooperator: bfqq ffff888140654dc0
> (ffff88817abc6000), new_bfqq ffff888174122100 (ffff888104b7c000)

Thanks for the debugging! So this shows that it is not 'bfqq' that changed
parent but rather bfqq->new_bfqq that changed parent. And presumably it can
even happen that the bfqq->new_bfqq parent gets offlined and UAF eventually
triggered. Indeed, I didn't think about that possibility. I have to think
how to handle this in the best way...

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

[PATCH 1/4] 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.31.1

[PATCH 1/4] 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.31.1