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

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

[Jan Kara]

Hello,

here is the fifth version of my patches to 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. Kuai, can you please give these patches a run in your setup? Thanks
a lot for your help with fixing this!

Changes since v4:
* Even more aggressive splitting of merged bfq queues to avoid problems with
  long merge chains.

Changes since v3:
* Changed handling of bfq group move to handle the case when target of the
  merge has moved.

Changes since v2:
* Improved handling of bfq queue splitting on move between cgroups
* Removed broken change to bfq_put_cooperator()

Changes since v1:
* Added fix for bfq_put_cooperator()
* Added fix to handle move between cgroups in bfq_merge_bio()

								Honza
Previous versions:
Link: http://lore.kernel.org/r/20211223171425.3551-1-jack@suse.cz # v1
Link: http://lore.kernel.org/r/20220105143037.20542-1-jack@suse.cz # v2
Link: http://lore.kernel.org/r/20220112113529.6355-1-jack@suse.cz # v3
Link: http://lore.kernel.org/r/20220114164215.28972-1-jack@suse.cz # v4

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

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 24a5c5329bcd..00184530c644 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -729,9 +729,39 @@ 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)
-			bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
+		if (!sync_bfqq->new_bfqq && !bfq_bfqq_coop(sync_bfqq)) {
+			/* We are the only user of this bfqq, just move it */
+			if (sync_bfqq->entity.sched_data != &bfqg->sched_data)
+				bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
+		} else {
+			struct bfq_queue *bfqq;
+
+			/*
+			 * The queue was merged to a different queue. Check
+			 * that the merge chain still belongs to the same
+			 * cgroup.
+			 */
+			for (bfqq = sync_bfqq; bfqq; bfqq = bfqq->new_bfqq)
+				if (bfqq->entity.sched_data !=
+				    &bfqg->sched_data)
+					break;
+			if (bfqq) {
+				/*
+				 * Some queue changed cgroup so the merge is
+				 * not valid anymore. We cannot easily just
+				 * cancel the merge (by clearing new_bfqq) as
+				 * there may be other processes using this
+				 * queue and holding refs to all queues below
+				 * sync_bfqq->new_bfqq. Similarly if the merge
+				 * already happened, we need to detach from
+				 * bfqq now so that we cannot merge bio to a
+				 * request from the old cgroup.
+				 */
+				bfq_put_cooperator(sync_bfqq);
+				bfq_release_process_ref(bfqd, sync_bfqq);
+				bic_set_bfqq(bic, NULL, 1);
+			}
+		}
 	}
 
 	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 0/4 v5] bfq: Avoid use-after-free when moving processes between cgroups

[yukuai (C)]

在 2022/01/21 19:42, yukuai (C) 写道:
> 在 2022/01/21 18:56, Jan Kara 写道:
>> Hello,
>>
>> here is the fifth version of my patches to 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. Kuai, can you please give these patches a run in your setup? 
>> Thanks
>> a lot for your help with fixing this!
>>
>> Changes since v4:
>> * Even more aggressive splitting of merged bfq queues to avoid 
>> problems with
>>    long merge chains.
>>
>> Changes since v3:
>> * Changed handling of bfq group move to handle the case when target of 
>> the
>>    merge has moved.
>>
>> Changes since v2:
>> * Improved handling of bfq queue splitting on move between cgroups
>> * Removed broken change to bfq_put_cooperator()
>>
>> Changes since v1:
>> * Added fix for bfq_put_cooperator()
>> * Added fix to handle move between cgroups in bfq_merge_bio()
>>
>>                                 Honza
>> Previous versions:
>> Link: http://lore.kernel.org/r/20211223171425.3551-1-jack@suse.cz # v1
>> Link: http://lore.kernel.org/r/20220105143037.20542-1-jack@suse.cz # v2
>> Link: http://lore.kernel.org/r/20220112113529.6355-1-jack@suse.cz # v3
>> Link: http://lore.kernel.org/r/20220114164215.28972-1-jack@suse.cz # v4
>> .
>>
> Hi, Jan
> 
> I add a new BUG_ON() in bfq_setup_merge() while iterating new_bfqq, and
> this time this BUG_ON() is triggered:
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index 00184530c644..4b17eb4a29bc 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -731,8 +731,12 @@ static struct bfq_group 
> *__bfq_bic_change_cgroup(struct bfq_data *bfqd,
>          if (sync_bfqq) {
>                  if (!sync_bfqq->new_bfqq && !bfq_bfqq_coop(sync_bfqq)) {
>                          /* We are the only user of this bfqq, just move 
> it */
> -                       if (sync_bfqq->entity.sched_data != 
> &bfqg->sched_data)
> +                       if (sync_bfqq->entity.sched_data != 
> &bfqg->sched_data) {
> +                               printk("%s: bfqq %px move from %px to 
> %px\n",
> +                                      __func__, sync_bfqq,
> +                                      bfqq_group(sync_bfqq), bfqg);
>                                  bfq_bfqq_move(bfqd, sync_bfqq, bfqg);
> +                       }
>                  } else {
>                          struct bfq_queue *bfqq;
> 
> @@ -741,10 +745,13 @@ static struct bfq_group 
> *__bfq_bic_change_cgroup(struct bfq_data *bfqd,
>                           * that the merge chain still belongs to the same
>                           * cgroup.
>                           */
> -                       for (bfqq = sync_bfqq; bfqq; bfqq = bfqq->new_bfqq)
> +                       for (bfqq = sync_bfqq; bfqq; bfqq = 
> bfqq->new_bfqq) {
> +                               printk("%s: bfqq %px(%px) bfqg %px\n", 
> __func__,
> +                                       bfqq, bfqq_group(bfqq), bfqg);
>                                  if (bfqq->entity.sched_data !=
>                                      &bfqg->sched_data)
>                                          break;
> +                       }
>                          if (bfqq) {
>                                  /*
>                                   * Some queue changed cgroup so the 
> merge is
> @@ -878,6 +885,8 @@ static void bfq_reparent_leaf_entity(struct bfq_data 
> *bfqd,
>          }
> 
>          bfqq = bfq_entity_to_bfqq(child_entity);
> +       printk("%s: bfqq %px move from %px to %px\n",  __func__, bfqq,
> +               bfqq_group(bfqq), bfqd->root_group);
>          bfq_bfqq_move(bfqd, bfqq, bfqd->root_group);
>   }
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 07be51bc229b..6d4e243c9a1e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2753,6 +2753,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct 
> bfq_queue *new_bfqq)
>          while ((__bfqq = new_bfqq->new_bfqq)) {
>                  if (__bfqq == bfqq)
>                          return NULL;
> +               if (new_bfqq->entity.parent != __bfqq->entity.parent &&
> +                   bfqq_group(__bfqq) != __bfqq->bfqd->root_group) {
> +                       printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n", 
> __func__,
> +                               new_bfqq, bfqq_group(new_bfqq), __bfqq,
> +                               bfqq_group(__bfqq));
> +                       BUG_ON(1);
> +               }
> +
>                  new_bfqq = __bfqq;
>          }
> 
> @@ -2797,6 +2805,8 @@ 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: set bfqq %px(%px) new_bfqq %px(%px)\n", __func__, 
> bfqq,
> +               bfqq_group(bfqq), new_bfqq, bfqq_group(new_bfqq));
>          new_bfqq->ref += process_refs;
>          return new_bfqq;
>   }
> @@ -2963,8 +2973,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)
> +       if (bfqq->new_bfqq) {
> +               if (bfqq->entity.parent != bfqq->new_bfqq->entity.parent &&
> +                   bfqq_group(bfqq->new_bfqq) != bfqd->root_group) {
> +                       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;
> @@ -6928,6 +6946,9 @@ static void __bfq_put_async_bfqq(struct bfq_data 
> *bfqd,
> 
>          bfq_log(bfqd, "put_async_bfqq: %p", bfqq);
>          if (bfqq) {
> +               printk("%s: bfqq %px move from %px to %px\n",  __func__, 
> bfqq,
> +                       bfqq_group(bfqq), bfqd->root_group);
> +
>                  bfq_bfqq_move(bfqd, bfqq, bfqd->root_group);
> 
>                  bfq_log_bfqq(bfqd, bfqq, "put_async_bfqq: putting %p, %d",
> 
Hi, Jan

It seems to me this problem is related to your orignal patch to move all
bfqqs to root during bfqg offline:

bfqg ffff8881721ee000  offline:
[   51.083018] bfq_reparent_leaf_entity: bfqq ffff8881130898c0 move from 
ffff8881721ee000 to ffff88817cb0f000
[   51.093889] bfq_reparent_leaf_entity: bfqq ffff8881222e2940 move from 
ffff8881721ee000 to ffff88817cb0f000
[   51.094756] bfq_reparent_leaf_entity: bfqq ffff88810e6a58c0 move from 
ffff8881721ee000 to ffff88817cb0f000
[   51.095626] bfq_reparent_leaf_entity: bfqq ffff888136b95600 move from 
ffff8881721ee000 to ffff88817cb0f000

however parent of ffff88817f8d0dc0 is still ffff8881721ee000 :
[   51.224771] bfq_setup_merge: bfqq ffff8881130898c0(ffff88817cb0f000) 
new_bfqq ffff88817f8d0dc0(ffff8881721ee000)

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

[Jan Kara]

On Fri 21-01-22 19:42:11, yukuai (C) wrote:
> 在 2022/01/21 18:56, Jan Kara 写道:
> > Hello,
> > 
> > here is the fifth version of my patches to 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. Kuai, can you please give these patches a run in your setup? Thanks
> > a lot for your help with fixing this!
> > 
> > Changes since v4:
> > * Even more aggressive splitting of merged bfq queues to avoid problems with
> >    long merge chains.
> > 
> > Changes since v3:
> > * Changed handling of bfq group move to handle the case when target of the
> >    merge has moved.
> > 
> > Changes since v2:
> > * Improved handling of bfq queue splitting on move between cgroups
> > * Removed broken change to bfq_put_cooperator()
> > 
> > Changes since v1:
> > * Added fix for bfq_put_cooperator()
> > * Added fix to handle move between cgroups in bfq_merge_bio()
> > 
> > 								Honza
> > Previous versions:
> > Link: http://lore.kernel.org/r/20211223171425.3551-1-jack@suse.cz # v1
> > Link: http://lore.kernel.org/r/20220105143037.20542-1-jack@suse.cz # v2
> > Link: http://lore.kernel.org/r/20220112113529.6355-1-jack@suse.cz # v3
> > Link: http://lore.kernel.org/r/20220114164215.28972-1-jack@suse.cz # v4
> > .
> > 
> Hi, Jan
> 
> I add a new BUG_ON() in bfq_setup_merge() while iterating new_bfqq, and
> this time this BUG_ON() is triggered:

Thanks for testing!

> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 07be51bc229b..6d4e243c9a1e 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2753,6 +2753,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> bfq_queue *new_bfqq)
>         while ((__bfqq = new_bfqq->new_bfqq)) {
>                 if (__bfqq == bfqq)
>                         return NULL;
> +               if (new_bfqq->entity.parent != __bfqq->entity.parent &&
> +                   bfqq_group(__bfqq) != __bfqq->bfqd->root_group) {
> +                       printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n",
> __func__,
> +                               new_bfqq, bfqq_group(new_bfqq), __bfqq,
> +                               bfqq_group(__bfqq));
> +                       BUG_ON(1);

This seems to be too early to check and BUG_ON(). Yes, we can walk through
and even end up with a bfqq with a different parent however in that case we
refuse to setup merge a few lines below and so there is no problem.

Are you still able to reproduce the use-after-free issue with this version
of my patches?

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

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

[Jan Kara]

[sending once again as I forgot to snip debug log at the end and mail got
bounced by vger mail server]

On Tue 25-01-22 16:23:28, yukuai (C) wrote:
> 在 2022/01/24 22:02, Jan Kara 写道:
> > On Fri 21-01-22 19:42:11, yukuai (C) wrote:
> > > 在 2022/01/21 18:56, Jan Kara 写道:
> > > > Hello,
> > > > 
> > > > here is the fifth version of my patches to 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. Kuai, can you please give these patches a run in your setup? Thanks
> > > > a lot for your help with fixing this!
> > > > 
> > > > Changes since v4:
> > > > * Even more aggressive splitting of merged bfq queues to avoid problems with
> > > >     long merge chains.
> > > > 
> > > > Changes since v3:
> > > > * Changed handling of bfq group move to handle the case when target of the
> > > >     merge has moved.
> > > > 
> > > > Changes since v2:
> > > > * Improved handling of bfq queue splitting on move between cgroups
> > > > * Removed broken change to bfq_put_cooperator()
> > > > 
> > > > Changes since v1:
> > > > * Added fix for bfq_put_cooperator()
> > > > * Added fix to handle move between cgroups in bfq_merge_bio()
> > > > 
> > > > 								Honza
> > > > Previous versions:
> > > > Link: http://lore.kernel.org/r/20211223171425.3551-1-jack@suse.cz # v1
> > > > Link: http://lore.kernel.org/r/20220105143037.20542-1-jack@suse.cz # v2
> > > > Link: http://lore.kernel.org/r/20220112113529.6355-1-jack@suse.cz # v3
> > > > Link: http://lore.kernel.org/r/20220114164215.28972-1-jack@suse.cz # v4
> > > > .
> > > > 
> > > Hi, Jan
> > > 
> > > I add a new BUG_ON() in bfq_setup_merge() while iterating new_bfqq, and
> > > this time this BUG_ON() is triggered:
> > 
> > Thanks for testing!
> > 
> > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > > index 07be51bc229b..6d4e243c9a1e 100644
> > > --- a/block/bfq-iosched.c
> > > +++ b/block/bfq-iosched.c
> > > @@ -2753,6 +2753,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
> > > bfq_queue *new_bfqq)
> > >          while ((__bfqq = new_bfqq->new_bfqq)) {
> > >                  if (__bfqq == bfqq)
> > >                          return NULL;
> > > +               if (new_bfqq->entity.parent != __bfqq->entity.parent &&
> > > +                   bfqq_group(__bfqq) != __bfqq->bfqd->root_group) {
> > > +                       printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n",
> > > __func__,
> > > +                               new_bfqq, bfqq_group(new_bfqq), __bfqq,
> > > +                               bfqq_group(__bfqq));
> > > +                       BUG_ON(1);
> > 
> > This seems to be too early to check and BUG_ON(). Yes, we can walk through
> > and even end up with a bfqq with a different parent however in that case we
> > refuse to setup merge a few lines below and so there is no problem.
> > 
> > Are you still able to reproduce the use-after-free issue with this version
> > of my patches?
> > 
> > 								Honza
> > 
> Hi, Jan
> 
> I add following additional debug info:
> 
> @ -926,6 +935,7 @@ static void bfq_pd_offline(struct blkg_policy_data *pd)
>         if (!entity) /* root group */
>                 goto put_async_queues;
> 
> +       printk("%s: bfqg %px offlined\n", __func__, bfqg);
>         /*
>          * Empty all service_trees belonging to this group before
>          * deactivating the group itself.
> @@ -965,6 +975,7 @@ static void bfq_pd_offline(struct blkg_policy_data *pd)
> 
>  put_async_queues:
>         bfq_put_async_queues(bfqd, bfqg);
> +       pd->plid = BLKCG_MAX_POLS;
> 
>         spin_unlock_irqrestore(&bfqd->lock, flags);
>         /*
> 
> @@ -6039,6 +6050,13 @@ static bool __bfq_insert_request(struct bfq_data
> *bfqd, struct request *rq)
>                 *new_bfqq = bfq_setup_cooperator(bfqd, bfqq, rq, true,
>                                                  RQ_BIC(rq));
>         bool waiting, idle_timer_disabled = false;
> +       if (new_bfqq) {
> +               printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n", __func__,
> +                       bfqq, bfqq_group(bfqq), new_bfqq,
> bfqq_group(new_bfqq));
> +       } else {
> +               printk("%s: bfqq %px(%px) new_bfqq null \n", __func__,
> +                       bfqq, bfqq_group(bfqq));
> +       }
> 
> @@ -1696,6 +1696,11 @@ void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct
> bfq_queue *bfqq)
> 
>         bfq_activate_bfqq(bfqd, bfqq);
> 
> +       if (bfqq->entity.parent && bfqq_group(bfqq)->pd.plid >=
> BLKCG_MAX_POLS) {
> +               printk("%s: bfqq %px(%px) with parent offlined\n", __func__,
> +                               bfqq, bfqq_group(bfqq));
> +               BUG_ON(1);
> +       }
> 
> And found that the uaf is triggered when bfq_setup_cooperator return
> NULL, that's why the BUG_ON() in bfq_setup_cooperator() is not
> triggered:
> 
> [   51.833290] __bfq_insert_request: bfqq ffff888106913700(ffff888107d67000)
> new_bfqq null
> [   51.834762] bfq_add_bfqq_busy: bfqq ffff888106913700(ffff888107d67000)
> with parent offlined
> 
> The new_bfqq chain relate to bfqq ffff888106913700:
> 
> t1: ffff8881114e9600 ------> t4: ffff888106913700 ------> t5:
> ffff88810719e3c0
>                         |
> t2: ffff888106913440 ----
>                         |
> t3: ffff8881114e98c0 ----
> 
> I'm still not sure about the root cause, hope these debuginfo
> can be helpful

Thanks for debugging! I was looking into this but I also do not understand
how what your tracing shows can happen. In particular I don't see why there
is no __bfq_bic_change_cgroup() call from bfq_insert_request() ->
bfq_init_rq() for the problematic __bfq_insert_request() into
ffff888106913700. I have two possible explanations. Either bio is submitted
to the offlined cgroup ffff888107d67000 or bic->blkcg_serial_nr is pointing
to different cgroup than bic_to_bfqq(bic, 1)->entity.parent.

So can you extented the debugging a bit like:
1) Add current->pid to all debug messages so that we can distinguish
different processes and see which already detached from the bfqq and which
not.

2) Print bic->blkcg_serial_nr and __bio_blkcg(bio)->css.serial_nr before
crashing in bfq_add_bfqq_busy().

3) Add BUG_ON to bic_set_bfqq() like:
	if (bfqq_group(bfqq)->css.serial_nr != bic->blkcg_serial_nr) {
		printk("%s: bfqq %px(%px) serial %d bic serial %d\n", bfqq,
			bfqq_group(bfqq), bfqq_group(bfqq)->css.serial_nr,
			bic->blkcg_serial_nr);
		BUG_ON(1);
	}

and perhaps this scheds more light on the problem... Thanks!

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

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

[yukuai (C)]

在 2022/02/03 5:53, Jan Kara 写道:
> [sending once again as I forgot to snip debug log at the end and mail got
> bounced by vger mail server]
> 
> On Tue 25-01-22 16:23:28, yukuai (C) wrote:
>> 在 2022/01/24 22:02, Jan Kara 写道:
>>> On Fri 21-01-22 19:42:11, yukuai (C) wrote:
>>>> 在 2022/01/21 18:56, Jan Kara 写道:
>>>>> Hello,
>>>>>
>>>>> here is the fifth version of my patches to 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. Kuai, can you please give these patches a run in your setup? Thanks
>>>>> a lot for your help with fixing this!
>>>>>
>>>>> Changes since v4:
>>>>> * Even more aggressive splitting of merged bfq queues to avoid problems with
>>>>>      long merge chains.
>>>>>
>>>>> Changes since v3:
>>>>> * Changed handling of bfq group move to handle the case when target of the
>>>>>      merge has moved.
>>>>>
>>>>> Changes since v2:
>>>>> * Improved handling of bfq queue splitting on move between cgroups
>>>>> * Removed broken change to bfq_put_cooperator()
>>>>>
>>>>> Changes since v1:
>>>>> * Added fix for bfq_put_cooperator()
>>>>> * Added fix to handle move between cgroups in bfq_merge_bio()
>>>>>
>>>>> 								Honza
>>>>> Previous versions:
>>>>> Link: http://lore.kernel.org/r/20211223171425.3551-1-jack@suse.cz # v1
>>>>> Link: http://lore.kernel.org/r/20220105143037.20542-1-jack@suse.cz # v2
>>>>> Link: http://lore.kernel.org/r/20220112113529.6355-1-jack@suse.cz # v3
>>>>> Link: http://lore.kernel.org/r/20220114164215.28972-1-jack@suse.cz # v4
>>>>> .
>>>>>
>>>> Hi, Jan
>>>>
>>>> I add a new BUG_ON() in bfq_setup_merge() while iterating new_bfqq, and
>>>> this time this BUG_ON() is triggered:
>>>
>>> Thanks for testing!
>>>
>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>>>> index 07be51bc229b..6d4e243c9a1e 100644
>>>> --- a/block/bfq-iosched.c
>>>> +++ b/block/bfq-iosched.c
>>>> @@ -2753,6 +2753,14 @@ bfq_setup_merge(struct bfq_queue *bfqq, struct
>>>> bfq_queue *new_bfqq)
>>>>           while ((__bfqq = new_bfqq->new_bfqq)) {
>>>>                   if (__bfqq == bfqq)
>>>>                           return NULL;
>>>> +               if (new_bfqq->entity.parent != __bfqq->entity.parent &&
>>>> +                   bfqq_group(__bfqq) != __bfqq->bfqd->root_group) {
>>>> +                       printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n",
>>>> __func__,
>>>> +                               new_bfqq, bfqq_group(new_bfqq), __bfqq,
>>>> +                               bfqq_group(__bfqq));
>>>> +                       BUG_ON(1);
>>>
>>> This seems to be too early to check and BUG_ON(). Yes, we can walk through
>>> and even end up with a bfqq with a different parent however in that case we
>>> refuse to setup merge a few lines below and so there is no problem.
>>>
>>> Are you still able to reproduce the use-after-free issue with this version
>>> of my patches?
>>>
>>> 								Honza
>>>
>> Hi, Jan
>>
>> I add following additional debug info:
>>
>> @ -926,6 +935,7 @@ static void bfq_pd_offline(struct blkg_policy_data *pd)
>>          if (!entity) /* root group */
>>                  goto put_async_queues;
>>
>> +       printk("%s: bfqg %px offlined\n", __func__, bfqg);
>>          /*
>>           * Empty all service_trees belonging to this group before
>>           * deactivating the group itself.
>> @@ -965,6 +975,7 @@ static void bfq_pd_offline(struct blkg_policy_data *pd)
>>
>>   put_async_queues:
>>          bfq_put_async_queues(bfqd, bfqg);
>> +       pd->plid = BLKCG_MAX_POLS;
>>
>>          spin_unlock_irqrestore(&bfqd->lock, flags);
>>          /*
>>
>> @@ -6039,6 +6050,13 @@ static bool __bfq_insert_request(struct bfq_data
>> *bfqd, struct request *rq)
>>                  *new_bfqq = bfq_setup_cooperator(bfqd, bfqq, rq, true,
>>                                                   RQ_BIC(rq));
>>          bool waiting, idle_timer_disabled = false;
>> +       if (new_bfqq) {
>> +               printk("%s: bfqq %px(%px) new_bfqq %px(%px)\n", __func__,
>> +                       bfqq, bfqq_group(bfqq), new_bfqq,
>> bfqq_group(new_bfqq));
>> +       } else {
>> +               printk("%s: bfqq %px(%px) new_bfqq null \n", __func__,
>> +                       bfqq, bfqq_group(bfqq));
>> +       }
>>
>> @@ -1696,6 +1696,11 @@ void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct
>> bfq_queue *bfqq)
>>
>>          bfq_activate_bfqq(bfqd, bfqq);
>>
>> +       if (bfqq->entity.parent && bfqq_group(bfqq)->pd.plid >=
>> BLKCG_MAX_POLS) {
>> +               printk("%s: bfqq %px(%px) with parent offlined\n", __func__,
>> +                               bfqq, bfqq_group(bfqq));
>> +               BUG_ON(1);
>> +       }
>>
>> And found that the uaf is triggered when bfq_setup_cooperator return
>> NULL, that's why the BUG_ON() in bfq_setup_cooperator() is not
>> triggered:
>>
>> [   51.833290] __bfq_insert_request: bfqq ffff888106913700(ffff888107d67000)
>> new_bfqq null
>> [   51.834762] bfq_add_bfqq_busy: bfqq ffff888106913700(ffff888107d67000)
>> with parent offlined
>>
>> The new_bfqq chain relate to bfqq ffff888106913700:
>>
>> t1: ffff8881114e9600 ------> t4: ffff888106913700 ------> t5:
>> ffff88810719e3c0
>>                          |
>> t2: ffff888106913440 ----
>>                          |
>> t3: ffff8881114e98c0 ----
>>
>> I'm still not sure about the root cause, hope these debuginfo
>> can be helpful
> 
> Thanks for debugging! I was looking into this but I also do not understand
> how what your tracing shows can happen. In particular I don't see why there
> is no __bfq_bic_change_cgroup() call from bfq_insert_request() ->
> bfq_init_rq() for the problematic __bfq_insert_request() into
> ffff888106913700. I have two possible explanations. Either bio is submitted
> to the offlined cgroup ffff888107d67000 or bic->blkcg_serial_nr is pointing
> to different cgroup than bic_to_bfqq(bic, 1)->entity.parent.
> 
> So can you extented the debugging a bit like:
> 1) Add current->pid to all debug messages so that we can distinguish
> different processes and see which already detached from the bfqq and which
> not.
> 
> 2) Print bic->blkcg_serial_nr and __bio_blkcg(bio)->css.serial_nr before
> crashing in bfq_add_bfqq_busy().
> 
> 3) Add BUG_ON to bic_set_bfqq() like:
> 	if (bfqq_group(bfqq)->css.serial_nr != bic->blkcg_serial_nr) {
> 		printk("%s: bfqq %px(%px) serial %d bic serial %d\n", bfqq,
> 			bfqq_group(bfqq), bfqq_group(bfqq)->css.serial_nr,
> 			bic->blkcg_serial_nr);
> 		BUG_ON(1);
> 	}
> 
> and perhaps this scheds more light on the problem... Thanks!
> 
> 								Honza
> 

Hi, Jan

Sorry about the delay, I'm on vacation for the Spring Festival.

I'll try the debugging soon.

Thanks,
Kuai

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

[Jan Kara]

On Tue 08-02-22 21:19:14, yukuai (C) wrote:
> 在 2022/02/03 5:53, Jan Kara 写道:
> > Thanks for debugging! I was looking into this but I also do not understand
> > how what your tracing shows can happen. In particular I don't see why there
> > is no __bfq_bic_change_cgroup() call from bfq_insert_request() ->
> > bfq_init_rq() for the problematic __bfq_insert_request() into
> > ffff888106913700. I have two possible explanations. Either bio is submitted
> > to the offlined cgroup ffff888107d67000 or bic->blkcg_serial_nr is pointing
> > to different cgroup than bic_to_bfqq(bic, 1)->entity.parent.
> > 
> > So can you extented the debugging a bit like:
> > 1) Add current->pid to all debug messages so that we can distinguish
> > different processes and see which already detached from the bfqq and which
> > not.
> > 
> > 2) Print bic->blkcg_serial_nr and __bio_blkcg(bio)->css.serial_nr before
> > crashing in bfq_add_bfqq_busy().
> > 
> > 3) Add BUG_ON to bic_set_bfqq() like:
> > 	if (bfqq_group(bfqq)->css.serial_nr != bic->blkcg_serial_nr) {
> > 		printk("%s: bfqq %px(%px) serial %d bic serial %d\n", bfqq,
> > 			bfqq_group(bfqq), bfqq_group(bfqq)->css.serial_nr,
> > 			bic->blkcg_serial_nr);
> > 		BUG_ON(1);
> > 	}
> > 
> > and perhaps this scheds more light on the problem... Thanks!
> > 
> > 								Honza
> > 
> 
> The debuging patch and log is attached.
> 
> bic_set_bfqq found serial mismatch serval times, bfqq_group(bfqq)->css
> doesn't exist, is the following debugging what you expected?
> 
> @@ -386,6 +386,13 @@ static void bfq_put_stable_ref(struct bfq_queue *bfqq);
> 
>  void bic_set_bfqq(struct bfq_io_cq *bic, struct bfq_queue *bfqq, bool
> is_sync)
>  {
> +       if (bfqq && bfqq_group(bfqq)->pd.blkg->blkcg->css.serial_nr !=
> +                   bic->blkcg_serial_nr) {
> +               bfqq_dbg(bfqq, "serial %lld bic serial %lld\n",
> +                        bfqq_group(bfqq)->pd.blkg->blkcg->css.serial_nr,
> +                        bic->blkcg_serial_nr);
> +               WARN_ON_ONCE(1);
> +       }

Yes, this is what I wanted. Thanks!

> 
> The problematic bfqq ffff8881682581f0 doesn't merge with any bfqq,
> which is weird.
> 
> The pid from __bfq_insert_request() is changed for the problematic
> bfqq, and each time the pid is changed, there is a bic_set_bfqq()
> shows that serial mismatch.

I had a look into debug data and now I think I understand both the WARN_ON
hit in bic_set_bfqq() as well as the final BUG_ON in bfq_add_bfqq_busy().

The first problem is apparently hit because __bio_blkcg() can change while
we are processing the bio. So bfq_bic_update_cgroup() sees different
__bio_blkcg() than bfq_get_queue() called from bfq_get_bfqq_handle_split().
This then causes mismatch between what bic & bfqq think about cgroup
membership which can lead to interesting inconsistencies down the road.

The second problem is hit because clearly __bio_blkcg() can be pointing to
a blkcg that has been already offlined. Previously I didn't think this was
possible but apparently there is nothing that would prevent this race. So
we need to handle this gracefully inside BFQ.

I need to think what would be best fixes for these issues since especially
the second one is tricky.

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

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

[yukuai (C)]

在 2022/02/10 1:40, Jan Kara 写道:
>
> I had a look into debug data and now I think I understand both the WARN_ON
> hit in bic_set_bfqq() as well as the final BUG_ON in bfq_add_bfqq_busy().
> 
> The first problem is apparently hit because __bio_blkcg() can change while
> we are processing the bio. So bfq_bic_update_cgroup() sees different
> __bio_blkcg() than bfq_get_queue() called from bfq_get_bfqq_handle_split().
> This then causes mismatch between what bic & bfqq think about cgroup
> membership which can lead to interesting inconsistencies down the road.
> 
> The second problem is hit because clearly __bio_blkcg() can be pointing to
> a blkcg that has been already offlined. Previously I didn't think this was
> possible but apparently there is nothing that would prevent this race. So
> we need to handle this gracefully inside BFQ.
> 
> I need to think what would be best fixes for these issues since especially
> the second one is tricky.

Hi, Jan

Just to be curiosity, do you have any ideas on how to fix these issues?

Thanks,
Kuai
> 
> 								Honza
> 

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

[Jan Kara]

On Fri 11-03-22 14:39:10, yukuai (C) wrote:
> 在 2022/02/10 1:40, Jan Kara 写道:
> > 
> > I had a look into debug data and now I think I understand both the WARN_ON
> > hit in bic_set_bfqq() as well as the final BUG_ON in bfq_add_bfqq_busy().
> > 
> > The first problem is apparently hit because __bio_blkcg() can change while
> > we are processing the bio. So bfq_bic_update_cgroup() sees different
> > __bio_blkcg() than bfq_get_queue() called from bfq_get_bfqq_handle_split().
> > This then causes mismatch between what bic & bfqq think about cgroup
> > membership which can lead to interesting inconsistencies down the road.
> > 
> > The second problem is hit because clearly __bio_blkcg() can be pointing to
> > a blkcg that has been already offlined. Previously I didn't think this was
> > possible but apparently there is nothing that would prevent this race. So
> > we need to handle this gracefully inside BFQ.
> > 
> > I need to think what would be best fixes for these issues since especially
> > the second one is tricky.
> 
> Hi, Jan
> 
> Just to be curiosity, do you have any ideas on how to fix these issues?

Sorry for the delay. I was on vacation and then busy with other stuff. I
have some version of the fixes ready but I want to clean them up a bit
before posting. It shouldn't take me long...

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