[PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Our syzkaller report a UAF problem(details in patch 1), however it can't
be reporduced. And this set are some corner cases fix that might be
related, and they are found by code review.

Yu Kuai (4):
  block, bfq: fix possible UAF for bfqq->bic with merge chain
  block, bfq: choose the last bfqq from merge chain in
    bfq_setup_cooperator()
  block, bfq: don't break merge chain in bfq_split_bfqq()
  block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()

 block/bfq-cgroup.c  |  7 +------
 block/bfq-iosched.c | 17 +++++++++++------
 block/bfq-iosched.h |  2 ++
 3 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH for-6.12 1/4] block, bfq: fix possible UAF for bfqq->bic with merge chain

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

1) initial state, three tasks:

		Process 1       Process 2	Process 3
		 (BIC1)          (BIC2)		 (BIC3)
		  |  Λ            |  Λ		  |  Λ
		  |  |            |  |		  |  |
		  V  |            V  |		  V  |
		  bfqq1           bfqq2		  bfqq3
process ref:	   1		    1		    1

2) bfqq1 merged to bfqq2:

		Process 1       Process 2	Process 3
		 (BIC1)          (BIC2)		 (BIC3)
		  |               |		  |  Λ
		  \--------------\|		  |  |
		                  V		  V  |
		  bfqq1--------->bfqq2		  bfqq3
process ref:	   0		    2		    1

3) bfqq2 merged to bfqq3:

		Process 1       Process 2	Process 3
		 (BIC1)          (BIC2)		 (BIC3)
	 here -> Λ                |		  |
		  \--------------\ \-------------\|
		                  V		  V
		  bfqq1--------->bfqq2---------->bfqq3
process ref:	   0		    1		    3

In this case, IO from Process 1 will get bfqq2 from BIC1 first, and then
get bfqq3 through merge chain, and finially handle IO by bfqq3.
Howerver, current code will think bfqq2 is owned by BIC1, like initial
state, and set bfqq2->bic to BIC1.

bfq_insert_request
-> by Process 1
 bfqq = bfq_init_rq(rq)
  bfqq = bfq_get_bfqq_handle_split
   bfqq = bic_to_bfqq
   -> get bfqq2 from BIC1
 bfqq->ref++
 rq->elv.priv[0] = bic
 rq->elv.priv[1] = bfqq
 if (bfqq_process_refs(bfqq) == 1)
  bfqq->bic = bic
  -> record BIC1 to bfqq2

  __bfq_insert_request
   new_bfqq = bfq_setup_cooperator
   -> get bfqq3 from bfqq2->new_bfqq
   bfqq_request_freed(bfqq)
   new_bfqq->ref++
   rq->elv.priv[1] = new_bfqq
   -> handle IO by bfqq3

Fix the problem by checking bfqq is from merge chain fist. And this
might fix a following problem reported by our syzkaller(unreproducible):

==================================================================
BUG: KASAN: slab-use-after-free in bfq_do_early_stable_merge block/bfq-iosched.c:5692 [inline]
BUG: KASAN: slab-use-after-free in bfq_do_or_sched_stable_merge block/bfq-iosched.c:5805 [inline]
BUG: KASAN: slab-use-after-free in bfq_get_queue+0x25b0/0x2610 block/bfq-iosched.c:5889
Write of size 1 at addr ffff888123839eb8 by task kworker/0:1H/18595

CPU: 0 PID: 18595 Comm: kworker/0:1H Tainted: G             L     6.6.0-07439-gba2303cacfda #6
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
Workqueue: kblockd blk_mq_requeue_work
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x91/0xf0 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:364 [inline]
 print_report+0x10d/0x610 mm/kasan/report.c:475
 kasan_report+0x8e/0xc0 mm/kasan/report.c:588
 bfq_do_early_stable_merge block/bfq-iosched.c:5692 [inline]
 bfq_do_or_sched_stable_merge block/bfq-iosched.c:5805 [inline]
 bfq_get_queue+0x25b0/0x2610 block/bfq-iosched.c:5889
 bfq_get_bfqq_handle_split+0x169/0x5d0 block/bfq-iosched.c:6757
 bfq_init_rq block/bfq-iosched.c:6876 [inline]
 bfq_insert_request block/bfq-iosched.c:6254 [inline]
 bfq_insert_requests+0x1112/0x5cf0 block/bfq-iosched.c:6304
 blk_mq_insert_request+0x290/0x8d0 block/blk-mq.c:2593
 blk_mq_requeue_work+0x6bc/0xa70 block/blk-mq.c:1502
 process_one_work kernel/workqueue.c:2627 [inline]
 process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
 worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
 kthread+0x33c/0x440 kernel/kthread.c:388
 ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305
 </TASK>

Allocated by task 20776:
 kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
 kasan_set_track+0x25/0x30 mm/kasan/common.c:52
 __kasan_slab_alloc+0x87/0x90 mm/kasan/common.c:328
 kasan_slab_alloc include/linux/kasan.h:188 [inline]
 slab_post_alloc_hook mm/slab.h:763 [inline]
 slab_alloc_node mm/slub.c:3458 [inline]
 kmem_cache_alloc_node+0x1a4/0x6f0 mm/slub.c:3503
 ioc_create_icq block/blk-ioc.c:370 [inline]
 ioc_find_get_icq+0x180/0xaa0 block/blk-ioc.c:436
 bfq_prepare_request+0x39/0xf0 block/bfq-iosched.c:6812
 blk_mq_rq_ctx_init.isra.7+0x6ac/0xa00 block/blk-mq.c:403
 __blk_mq_alloc_requests+0xcc0/0x1070 block/blk-mq.c:517
 blk_mq_get_new_requests block/blk-mq.c:2940 [inline]
 blk_mq_submit_bio+0x624/0x27c0 block/blk-mq.c:3042
 __submit_bio+0x331/0x6f0 block/blk-core.c:624
 __submit_bio_noacct_mq block/blk-core.c:703 [inline]
 submit_bio_noacct_nocheck+0x816/0xb40 block/blk-core.c:732
 submit_bio_noacct+0x7a6/0x1b50 block/blk-core.c:826
 xlog_write_iclog+0x7d5/0xa00 fs/xfs/xfs_log.c:1958
 xlog_state_release_iclog+0x3b8/0x720 fs/xfs/xfs_log.c:619
 xlog_cil_push_work+0x19c5/0x2270 fs/xfs/xfs_log_cil.c:1330
 process_one_work kernel/workqueue.c:2627 [inline]
 process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
 worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
 kthread+0x33c/0x440 kernel/kthread.c:388
 ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305

Freed by task 946:
 kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
 kasan_set_track+0x25/0x30 mm/kasan/common.c:52
 kasan_save_free_info+0x2b/0x50 mm/kasan/generic.c:522
 ____kasan_slab_free mm/kasan/common.c:236 [inline]
 __kasan_slab_free+0x12c/0x1c0 mm/kasan/common.c:244
 kasan_slab_free include/linux/kasan.h:164 [inline]
 slab_free_hook mm/slub.c:1815 [inline]
 slab_free_freelist_hook mm/slub.c:1841 [inline]
 slab_free mm/slub.c:3786 [inline]
 kmem_cache_free+0x118/0x6f0 mm/slub.c:3808
 rcu_do_batch+0x35c/0xe30 kernel/rcu/tree.c:2189
 rcu_core+0x819/0xd90 kernel/rcu/tree.c:2462
 __do_softirq+0x1b0/0x7a2 kernel/softirq.c:553

Last potentially related work creation:
 kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
 __kasan_record_aux_stack+0xaf/0xc0 mm/kasan/generic.c:492
 __call_rcu_common kernel/rcu/tree.c:2712 [inline]
 call_rcu+0xce/0x1020 kernel/rcu/tree.c:2826
 ioc_destroy_icq+0x54c/0x830 block/blk-ioc.c:105
 ioc_release_fn+0xf0/0x360 block/blk-ioc.c:124
 process_one_work kernel/workqueue.c:2627 [inline]
 process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
 worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
 kthread+0x33c/0x440 kernel/kthread.c:388
 ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305

Second to last potentially related work creation:
 kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
 __kasan_record_aux_stack+0xaf/0xc0 mm/kasan/generic.c:492
 __call_rcu_common kernel/rcu/tree.c:2712 [inline]
 call_rcu+0xce/0x1020 kernel/rcu/tree.c:2826
 ioc_destroy_icq+0x54c/0x830 block/blk-ioc.c:105
 ioc_release_fn+0xf0/0x360 block/blk-ioc.c:124
 process_one_work kernel/workqueue.c:2627 [inline]
 process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
 worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
 kthread+0x33c/0x440 kernel/kthread.c:388
 ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305

The buggy address belongs to the object at ffff888123839d68
 which belongs to the cache bfq_io_cq of size 1360
The buggy address is located 336 bytes inside of
 freed 1360-byte region [ffff888123839d68, ffff88812383a2b8)

The buggy address belongs to the physical page:
page:ffffea00048e0e00 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88812383f588 pfn:0x123838
head:ffffea00048e0e00 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x17ffffc0000a40(workingset|slab|head|node=0|zone=2|lastcpupid=0x1fffff)
page_type: 0xffffffff()
raw: 0017ffffc0000a40 ffff88810588c200 ffffea00048ffa10 ffff888105889488
raw: ffff88812383f588 0000000000150006 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888123839d80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888123839e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff888123839e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                        ^
 ffff888123839f00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888123839f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 36a4998c4b37..83adac3e71db 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6934,7 +6934,8 @@ static struct bfq_queue *bfq_init_rq(struct request *rq)
 	 * addition, if the queue has also just been split, we have to
 	 * resume its state.
 	 */
-	if (likely(bfqq != &bfqd->oom_bfqq) && bfqq_process_refs(bfqq) == 1) {
+	if (likely(bfqq != &bfqd->oom_bfqq) && !bfqq->new_bfqq &&
+	    bfqq_process_refs(bfqq) == 1) {
 		bfqq->bic = bic;
 		if (split) {
 			/*
-- 
2.39.2

[PATCH for-6.12 2/4] block, bfq: choose the last bfqq from merge chain in bfq_setup_cooperator()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Consider the following merge chain:

Process 1       Process 2       Process 3	Process 4
 (BIC1)          (BIC2)          (BIC3)		 (BIC4)
  Λ                |               |               |
   \--------------\ \-------------\ \-------------\|
                   V               V		   V
  bfqq1--------->bfqq2---------->bfqq3----------->bfqq4

IO from Process 1 will get bfqf2 from BIC1 first, then
bfq_setup_cooperator() will found bfqq2 already merged to bfqq3 and then
handle this IO from bfqq3. However, the merge chain can be much deeper
and bfqq3 can be merged to other bfqq as well.

Fix this problem by iterating to the last bfqq in
bfq_setup_cooperator().

Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 83adac3e71db..ffaa0d56328a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2911,8 +2911,12 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
 
 	/* if a merge has already been setup, then proceed with that first */
-	if (bfqq->new_bfqq)
-		return bfqq->new_bfqq;
+	new_bfqq = bfqq->new_bfqq;
+	if (new_bfqq) {
+		while (new_bfqq->new_bfqq)
+			new_bfqq = new_bfqq->new_bfqq;
+		return new_bfqq;
+	}
 
 	/*
 	 * Check delayed stable merge for rotational or non-queueing
-- 
2.39.2

[PATCH for-6.12 3/4] block, bfq: don't break merge chain in bfq_split_bfqq()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Consider the following scenario:

    Process 1       Process 2       Process 3       Process 4
     (BIC1)          (BIC2)          (BIC3)          (BIC4)
      Λ               |               |                |
       \-------------\ \-------------\ \--------------\|
                      V               V                V
      bfqq1--------->bfqq2---------->bfqq3----------->bfqq4
ref    0              1               2                4

If Process 1 issue a new IO and bfqq2 is found, and then bfq_init_rq()
decide to spilt bfqq2 by bfq_split_bfqq(). Howerver, procress reference
of bfqq2 is 1 and bfq_split_bfqq() just clear the coop flag, which will
break the merge chain.

Expected result: caller will allocate a new bfqq for BIC1

    Process 1       Process 2       Process 3       Process 4
     (BIC1)          (BIC2)          (BIC3)          (BIC4)
                      |               |                |
                       \-------------\ \--------------\|
                                      V                V
      bfqq1--------->bfqq2---------->bfqq3----------->bfqq4
ref    0              0               1                3

Since the condition is only used for the last bfqq4 when the previous
bfqq2 and bfqq3 are already splited. Fix the problem by checking if
bfqq is the last one in the merge chain as well.

Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index ffaa0d56328a..ca766b7d5560 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6727,7 +6727,7 @@ bfq_split_bfqq(struct bfq_io_cq *bic, struct bfq_queue *bfqq)
 {
 	bfq_log_bfqq(bfqq->bfqd, bfqq, "splitting queue");
 
-	if (bfqq_process_refs(bfqq) == 1) {
+	if (bfqq_process_refs(bfqq) == 1 && !bfqq->new_bfqq) {
 		bfqq->pid = current->pid;
 		bfq_clear_bfqq_coop(bfqq);
 		bfq_clear_bfqq_split_coop(bfqq);
-- 
2.39.2

[PATCH for-6.12 4/4] block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Instead of open coding it, there are no functional changes.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-cgroup.c  | 7 +------
 block/bfq-iosched.c | 4 ++--
 block/bfq-iosched.h | 2 ++
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index b758693697c0..9fb9f3533150 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -679,12 +679,7 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 		bfq_put_idle_entity(bfq_entity_service_tree(entity), entity);
 	bfqg_and_blkg_put(old_parent);
 
-	if (entity->parent &&
-	    entity->parent->last_bfqq_created == bfqq)
-		entity->parent->last_bfqq_created = NULL;
-	else if (bfqd->last_bfqq_created == bfqq)
-		bfqd->last_bfqq_created = NULL;
-
+	bfq_reassign_last_bfqq(bfqq, NULL);
 	entity->parent = bfqg->my_entity;
 	entity->sched_data = &bfqg->sched_data;
 	/* pin down bfqg and its associated blkg  */
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index ca766b7d5560..d1bf2b8a3576 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3097,8 +3097,8 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 }
 
 
-static void
-bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq, struct bfq_queue *new_bfqq)
+void bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq,
+			    struct bfq_queue *new_bfqq)
 {
 	if (cur_bfqq->entity.parent &&
 	    cur_bfqq->entity.parent->last_bfqq_created == cur_bfqq)
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 08ddf2cfae5b..e16d96e2367b 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -1156,6 +1156,8 @@ void bfq_del_bfqq_busy(struct bfq_queue *bfqq, bool expiration);
 void bfq_add_bfqq_busy(struct bfq_queue *bfqq);
 void bfq_add_bfqq_in_groups_with_pending_reqs(struct bfq_queue *bfqq);
 void bfq_del_bfqq_in_groups_with_pending_reqs(struct bfq_queue *bfqq);
+void bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq,
+			    struct bfq_queue *new_bfqq);
 
 /* --------------- end of interface of B-WF2Q+ ---------------- */
 
-- 
2.39.2

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jens Axboe]

On 9/2/24 7:03 AM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Our syzkaller report a UAF problem(details in patch 1), however it can't
> be reporduced. And this set are some corner cases fix that might be
> related, and they are found by code review.
> 
> Yu Kuai (4):
>   block, bfq: fix possible UAF for bfqq->bic with merge chain
>   block, bfq: choose the last bfqq from merge chain in
>     bfq_setup_cooperator()
>   block, bfq: don't break merge chain in bfq_split_bfqq()
>   block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
> 
>  block/bfq-cgroup.c  |  7 +------
>  block/bfq-iosched.c | 17 +++++++++++------
>  block/bfq-iosched.h |  2 ++
>  3 files changed, 14 insertions(+), 12 deletions(-)

BFQ is effectively unmaintained, and has been for quite a while at
this point. I'll apply these, thanks for looking into it, but I think we
should move BFQ to an unmaintained state at this point.

-- 
Jens Axboe

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jens Axboe]

On Mon, 02 Sep 2024 21:03:25 +0800, Yu Kuai wrote:
> Our syzkaller report a UAF problem(details in patch 1), however it can't
> be reporduced. And this set are some corner cases fix that might be
> related, and they are found by code review.
> 
> Yu Kuai (4):
>   block, bfq: fix possible UAF for bfqq->bic with merge chain
>   block, bfq: choose the last bfqq from merge chain in
>     bfq_setup_cooperator()
>   block, bfq: don't break merge chain in bfq_split_bfqq()
>   block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
> 
> [...]

Applied, thanks!

[1/4] block, bfq: fix possible UAF for bfqq->bic with merge chain
      commit: 18ad4df091dd5d067d2faa8fce1180b79f7041a7
[2/4] block, bfq: choose the last bfqq from merge chain in bfq_setup_cooperator()
      commit: 0e456dba86c7f9a19792204a044835f1ca2c8dbb
[3/4] block, bfq: don't break merge chain in bfq_split_bfqq()
      commit: 42c306ed723321af4003b2a41bb73728cab54f85
[4/4] block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
      commit: f45916ae60eb60e7c9c3ac60cf07e66fe1a7faad

Best regards,
-- 
Jens Axboe

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Yu Kuai]

Hi,

在 2024/09/03 23:51, Jens Axboe 写道:
> On 9/2/24 7:03 AM, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> Our syzkaller report a UAF problem(details in patch 1), however it can't
>> be reporduced. And this set are some corner cases fix that might be
>> related, and they are found by code review.
>>
>> Yu Kuai (4):
>>    block, bfq: fix possible UAF for bfqq->bic with merge chain
>>    block, bfq: choose the last bfqq from merge chain in
>>      bfq_setup_cooperator()
>>    block, bfq: don't break merge chain in bfq_split_bfqq()
>>    block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
>>
>>   block/bfq-cgroup.c  |  7 +------
>>   block/bfq-iosched.c | 17 +++++++++++------
>>   block/bfq-iosched.h |  2 ++
>>   3 files changed, 14 insertions(+), 12 deletions(-)
> 
> BFQ is effectively unmaintained, and has been for quite a while at
> this point. I'll apply these, thanks for looking into it, but I think we
> should move BFQ to an unmaintained state at this point.

Sorry to hear that, we would be willing to take on the responsibility of
maintaining this code, please let me know if there are any specific
guidelines or processes we should follow. We do have customers are using
bfq in downstream kernels, and we are still running lots of test for
bfq.

Thanks,
Kuai


> 

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Bart Van Assche]

On 9/3/24 6:32 PM, Yu Kuai wrote:
> We do have customers are using bfq in downstream kernels, and we are
> still running lots of test for bfq.

It may take less time to add any missing functionality to another I/O
scheduler rather than to keep maintaining BFQ.

If Android device vendors would stop using BFQ, my job would become
easier.

Thanks,

Bart.

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Yu Kuai]

Hi,

在 2024/09/04 10:28, Bart Van Assche 写道:
> On 9/3/24 6:32 PM, Yu Kuai wrote:
>> We do have customers are using bfq in downstream kernels, and we are
>> still running lots of test for bfq.
> 
> It may take less time to add any missing functionality to another I/O
> scheduler rather than to keep maintaining BFQ.
> 
> If Android device vendors would stop using BFQ, my job would become
> easier.

I'm confused now, I think keep maintaining BFQ won't stop you from
adding new functionality to another scheduler, right? Is this something
that all scheduler have to support?

Thanks,
Kuai

> 
> Thanks,
> 
> Bart.
> .
> 

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Ming Lei]

On Wed, Sep 04, 2024 at 09:32:26AM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2024/09/03 23:51, Jens Axboe 写道:
> > On 9/2/24 7:03 AM, Yu Kuai wrote:
> > > From: Yu Kuai <yukuai3@huawei.com>
> > > 
> > > Our syzkaller report a UAF problem(details in patch 1), however it can't
> > > be reporduced. And this set are some corner cases fix that might be
> > > related, and they are found by code review.
> > > 
> > > Yu Kuai (4):
> > >    block, bfq: fix possible UAF for bfqq->bic with merge chain
> > >    block, bfq: choose the last bfqq from merge chain in
> > >      bfq_setup_cooperator()
> > >    block, bfq: don't break merge chain in bfq_split_bfqq()
> > >    block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
> > > 
> > >   block/bfq-cgroup.c  |  7 +------
> > >   block/bfq-iosched.c | 17 +++++++++++------
> > >   block/bfq-iosched.h |  2 ++
> > >   3 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > BFQ is effectively unmaintained, and has been for quite a while at
> > this point. I'll apply these, thanks for looking into it, but I think we
> > should move BFQ to an unmaintained state at this point.
> 
> Sorry to hear that, we would be willing to take on the responsibility of
> maintaining this code, please let me know if there are any specific
> guidelines or processes we should follow. We do have customers are using
> bfq in downstream kernels, and we are still running lots of test for
> bfq.

Hi Yukuai,
 
BTW, BFQ is default IO scheduler for SCSI/MMC in Fedora. That is great if you'd
like to volunteer to take over maintaining it.


thanks,
Ming

Re: [PATCH for-6.12 1/4] block, bfq: fix possible UAF for bfqq->bic with merge chain

[Jan Kara]

On Mon 02-09-24 21:03:26, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> 1) initial state, three tasks:
> 
> 		Process 1       Process 2	Process 3
> 		 (BIC1)          (BIC2)		 (BIC3)
> 		  |  Λ            |  Λ		  |  Λ
> 		  |  |            |  |		  |  |
> 		  V  |            V  |		  V  |
> 		  bfqq1           bfqq2		  bfqq3
> process ref:	   1		    1		    1
> 
> 2) bfqq1 merged to bfqq2:
> 
> 		Process 1       Process 2	Process 3
> 		 (BIC1)          (BIC2)		 (BIC3)
> 		  |               |		  |  Λ
> 		  \--------------\|		  |  |
> 		                  V		  V  |
> 		  bfqq1--------->bfqq2		  bfqq3
> process ref:	   0		    2		    1
> 
> 3) bfqq2 merged to bfqq3:
> 
> 		Process 1       Process 2	Process 3
> 		 (BIC1)          (BIC2)		 (BIC3)
> 	 here -> Λ                |		  |
> 		  \--------------\ \-------------\|
> 		                  V		  V
> 		  bfqq1--------->bfqq2---------->bfqq3
> process ref:	   0		    1		    3
> 
> In this case, IO from Process 1 will get bfqq2 from BIC1 first, and then
> get bfqq3 through merge chain, and finially handle IO by bfqq3.
> Howerver, current code will think bfqq2 is owned by BIC1, like initial
> state, and set bfqq2->bic to BIC1.
> 
> bfq_insert_request
> -> by Process 1
>  bfqq = bfq_init_rq(rq)
>   bfqq = bfq_get_bfqq_handle_split
>    bfqq = bic_to_bfqq
>    -> get bfqq2 from BIC1
>  bfqq->ref++
>  rq->elv.priv[0] = bic
>  rq->elv.priv[1] = bfqq
>  if (bfqq_process_refs(bfqq) == 1)
>   bfqq->bic = bic
>   -> record BIC1 to bfqq2
> 
>   __bfq_insert_request
>    new_bfqq = bfq_setup_cooperator
>    -> get bfqq3 from bfqq2->new_bfqq
>    bfqq_request_freed(bfqq)
>    new_bfqq->ref++
>    rq->elv.priv[1] = new_bfqq
>    -> handle IO by bfqq3
> 
> Fix the problem by checking bfqq is from merge chain fist. And this
> might fix a following problem reported by our syzkaller(unreproducible):
> 
> ==================================================================
> BUG: KASAN: slab-use-after-free in bfq_do_early_stable_merge block/bfq-iosched.c:5692 [inline]
> BUG: KASAN: slab-use-after-free in bfq_do_or_sched_stable_merge block/bfq-iosched.c:5805 [inline]
> BUG: KASAN: slab-use-after-free in bfq_get_queue+0x25b0/0x2610 block/bfq-iosched.c:5889
> Write of size 1 at addr ffff888123839eb8 by task kworker/0:1H/18595
> 
> CPU: 0 PID: 18595 Comm: kworker/0:1H Tainted: G             L     6.6.0-07439-gba2303cacfda #6
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> Workqueue: kblockd blk_mq_requeue_work
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x91/0xf0 lib/dump_stack.c:106
>  print_address_description mm/kasan/report.c:364 [inline]
>  print_report+0x10d/0x610 mm/kasan/report.c:475
>  kasan_report+0x8e/0xc0 mm/kasan/report.c:588
>  bfq_do_early_stable_merge block/bfq-iosched.c:5692 [inline]
>  bfq_do_or_sched_stable_merge block/bfq-iosched.c:5805 [inline]
>  bfq_get_queue+0x25b0/0x2610 block/bfq-iosched.c:5889
>  bfq_get_bfqq_handle_split+0x169/0x5d0 block/bfq-iosched.c:6757
>  bfq_init_rq block/bfq-iosched.c:6876 [inline]
>  bfq_insert_request block/bfq-iosched.c:6254 [inline]
>  bfq_insert_requests+0x1112/0x5cf0 block/bfq-iosched.c:6304
>  blk_mq_insert_request+0x290/0x8d0 block/blk-mq.c:2593
>  blk_mq_requeue_work+0x6bc/0xa70 block/blk-mq.c:1502
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
>  worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
>  kthread+0x33c/0x440 kernel/kthread.c:388
>  ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305
>  </TASK>
> 
> Allocated by task 20776:
>  kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
>  kasan_set_track+0x25/0x30 mm/kasan/common.c:52
>  __kasan_slab_alloc+0x87/0x90 mm/kasan/common.c:328
>  kasan_slab_alloc include/linux/kasan.h:188 [inline]
>  slab_post_alloc_hook mm/slab.h:763 [inline]
>  slab_alloc_node mm/slub.c:3458 [inline]
>  kmem_cache_alloc_node+0x1a4/0x6f0 mm/slub.c:3503
>  ioc_create_icq block/blk-ioc.c:370 [inline]
>  ioc_find_get_icq+0x180/0xaa0 block/blk-ioc.c:436
>  bfq_prepare_request+0x39/0xf0 block/bfq-iosched.c:6812
>  blk_mq_rq_ctx_init.isra.7+0x6ac/0xa00 block/blk-mq.c:403
>  __blk_mq_alloc_requests+0xcc0/0x1070 block/blk-mq.c:517
>  blk_mq_get_new_requests block/blk-mq.c:2940 [inline]
>  blk_mq_submit_bio+0x624/0x27c0 block/blk-mq.c:3042
>  __submit_bio+0x331/0x6f0 block/blk-core.c:624
>  __submit_bio_noacct_mq block/blk-core.c:703 [inline]
>  submit_bio_noacct_nocheck+0x816/0xb40 block/blk-core.c:732
>  submit_bio_noacct+0x7a6/0x1b50 block/blk-core.c:826
>  xlog_write_iclog+0x7d5/0xa00 fs/xfs/xfs_log.c:1958
>  xlog_state_release_iclog+0x3b8/0x720 fs/xfs/xfs_log.c:619
>  xlog_cil_push_work+0x19c5/0x2270 fs/xfs/xfs_log_cil.c:1330
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
>  worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
>  kthread+0x33c/0x440 kernel/kthread.c:388
>  ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305
> 
> Freed by task 946:
>  kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
>  kasan_set_track+0x25/0x30 mm/kasan/common.c:52
>  kasan_save_free_info+0x2b/0x50 mm/kasan/generic.c:522
>  ____kasan_slab_free mm/kasan/common.c:236 [inline]
>  __kasan_slab_free+0x12c/0x1c0 mm/kasan/common.c:244
>  kasan_slab_free include/linux/kasan.h:164 [inline]
>  slab_free_hook mm/slub.c:1815 [inline]
>  slab_free_freelist_hook mm/slub.c:1841 [inline]
>  slab_free mm/slub.c:3786 [inline]
>  kmem_cache_free+0x118/0x6f0 mm/slub.c:3808
>  rcu_do_batch+0x35c/0xe30 kernel/rcu/tree.c:2189
>  rcu_core+0x819/0xd90 kernel/rcu/tree.c:2462
>  __do_softirq+0x1b0/0x7a2 kernel/softirq.c:553
> 
> Last potentially related work creation:
>  kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
>  __kasan_record_aux_stack+0xaf/0xc0 mm/kasan/generic.c:492
>  __call_rcu_common kernel/rcu/tree.c:2712 [inline]
>  call_rcu+0xce/0x1020 kernel/rcu/tree.c:2826
>  ioc_destroy_icq+0x54c/0x830 block/blk-ioc.c:105
>  ioc_release_fn+0xf0/0x360 block/blk-ioc.c:124
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
>  worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
>  kthread+0x33c/0x440 kernel/kthread.c:388
>  ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305
> 
> Second to last potentially related work creation:
>  kasan_save_stack+0x20/0x40 mm/kasan/common.c:45
>  __kasan_record_aux_stack+0xaf/0xc0 mm/kasan/generic.c:492
>  __call_rcu_common kernel/rcu/tree.c:2712 [inline]
>  call_rcu+0xce/0x1020 kernel/rcu/tree.c:2826
>  ioc_destroy_icq+0x54c/0x830 block/blk-ioc.c:105
>  ioc_release_fn+0xf0/0x360 block/blk-ioc.c:124
>  process_one_work kernel/workqueue.c:2627 [inline]
>  process_scheduled_works+0x432/0x13f0 kernel/workqueue.c:2700
>  worker_thread+0x6f2/0x1160 kernel/workqueue.c:2781
>  kthread+0x33c/0x440 kernel/kthread.c:388
>  ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:305
> 
> The buggy address belongs to the object at ffff888123839d68
>  which belongs to the cache bfq_io_cq of size 1360
> The buggy address is located 336 bytes inside of
>  freed 1360-byte region [ffff888123839d68, ffff88812383a2b8)
> 
> The buggy address belongs to the physical page:
> page:ffffea00048e0e00 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88812383f588 pfn:0x123838
> head:ffffea00048e0e00 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> flags: 0x17ffffc0000a40(workingset|slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> page_type: 0xffffffff()
> raw: 0017ffffc0000a40 ffff88810588c200 ffffea00048ffa10 ffff888105889488
> raw: ffff88812383f588 0000000000150006 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  ffff888123839d80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff888123839e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >ffff888123839e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                         ^
>  ffff888123839f00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff888123839f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
> 
> Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 36a4998c4b37..83adac3e71db 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -6934,7 +6934,8 @@ static struct bfq_queue *bfq_init_rq(struct request *rq)
>  	 * addition, if the queue has also just been split, we have to
>  	 * resume its state.
>  	 */
> -	if (likely(bfqq != &bfqd->oom_bfqq) && bfqq_process_refs(bfqq) == 1) {
> +	if (likely(bfqq != &bfqd->oom_bfqq) && !bfqq->new_bfqq &&
> +	    bfqq_process_refs(bfqq) == 1) {
>  		bfqq->bic = bic;
>  		if (split) {
>  			/*
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH for-6.12 2/4] block, bfq: choose the last bfqq from merge chain in bfq_setup_cooperator()

[Jan Kara]

On Mon 02-09-24 21:03:27, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Consider the following merge chain:
> 
> Process 1       Process 2       Process 3	Process 4
>  (BIC1)          (BIC2)          (BIC3)		 (BIC4)
>   Λ                |               |               |
>    \--------------\ \-------------\ \-------------\|
>                    V               V		   V
>   bfqq1--------->bfqq2---------->bfqq3----------->bfqq4
> 
> IO from Process 1 will get bfqf2 from BIC1 first, then
> bfq_setup_cooperator() will found bfqq2 already merged to bfqq3 and then
> handle this IO from bfqq3. However, the merge chain can be much deeper
> and bfqq3 can be merged to other bfqq as well.
> 
> Fix this problem by iterating to the last bfqq in
> bfq_setup_cooperator().
> 
> Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Good catch. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 83adac3e71db..ffaa0d56328a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2911,8 +2911,12 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>  	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
>  
>  	/* if a merge has already been setup, then proceed with that first */
> -	if (bfqq->new_bfqq)
> -		return bfqq->new_bfqq;
> +	new_bfqq = bfqq->new_bfqq;
> +	if (new_bfqq) {
> +		while (new_bfqq->new_bfqq)
> +			new_bfqq = new_bfqq->new_bfqq;
> +		return new_bfqq;
> +	}
>  
>  	/*
>  	 * Check delayed stable merge for rotational or non-queueing
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH for-6.12 3/4] block, bfq: don't break merge chain in bfq_split_bfqq()

[Jan Kara]

On Mon 02-09-24 21:03:28, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Consider the following scenario:
> 
>     Process 1       Process 2       Process 3       Process 4
>      (BIC1)          (BIC2)          (BIC3)          (BIC4)
>       Λ               |               |                |
>        \-------------\ \-------------\ \--------------\|
>                       V               V                V
>       bfqq1--------->bfqq2---------->bfqq3----------->bfqq4
> ref    0              1               2                4
> 
> If Process 1 issue a new IO and bfqq2 is found, and then bfq_init_rq()
> decide to spilt bfqq2 by bfq_split_bfqq(). Howerver, procress reference
> of bfqq2 is 1 and bfq_split_bfqq() just clear the coop flag, which will
> break the merge chain.
> 
> Expected result: caller will allocate a new bfqq for BIC1
> 
>     Process 1       Process 2       Process 3       Process 4
>      (BIC1)          (BIC2)          (BIC3)          (BIC4)
>                       |               |                |
>                        \-------------\ \--------------\|
>                                       V                V
>       bfqq1--------->bfqq2---------->bfqq3----------->bfqq4
> ref    0              0               1                3
> 
> Since the condition is only used for the last bfqq4 when the previous
> bfqq2 and bfqq3 are already splited. Fix the problem by checking if
> bfqq is the last one in the merge chain as well.
> 
> Fixes: 36eca8948323 ("block, bfq: add Early Queue Merge (EQM)")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-iosched.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index ffaa0d56328a..ca766b7d5560 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -6727,7 +6727,7 @@ bfq_split_bfqq(struct bfq_io_cq *bic, struct bfq_queue *bfqq)
>  {
>  	bfq_log_bfqq(bfqq->bfqd, bfqq, "splitting queue");
>  
> -	if (bfqq_process_refs(bfqq) == 1) {
> +	if (bfqq_process_refs(bfqq) == 1 && !bfqq->new_bfqq) {
>  		bfqq->pid = current->pid;
>  		bfq_clear_bfqq_coop(bfqq);
>  		bfq_clear_bfqq_split_coop(bfqq);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH for-6.12 4/4] block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()

[Jan Kara]

On Mon 02-09-24 21:03:29, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Instead of open coding it, there are no functional changes.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/bfq-cgroup.c  | 7 +------
>  block/bfq-iosched.c | 4 ++--
>  block/bfq-iosched.h | 2 ++
>  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index b758693697c0..9fb9f3533150 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -679,12 +679,7 @@ void bfq_bfqq_move(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>  		bfq_put_idle_entity(bfq_entity_service_tree(entity), entity);
>  	bfqg_and_blkg_put(old_parent);
>  
> -	if (entity->parent &&
> -	    entity->parent->last_bfqq_created == bfqq)
> -		entity->parent->last_bfqq_created = NULL;
> -	else if (bfqd->last_bfqq_created == bfqq)
> -		bfqd->last_bfqq_created = NULL;
> -
> +	bfq_reassign_last_bfqq(bfqq, NULL);
>  	entity->parent = bfqg->my_entity;
>  	entity->sched_data = &bfqg->sched_data;
>  	/* pin down bfqg and its associated blkg  */
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index ca766b7d5560..d1bf2b8a3576 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -3097,8 +3097,8 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
>  }
>  
>  
> -static void
> -bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq, struct bfq_queue *new_bfqq)
> +void bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq,
> +			    struct bfq_queue *new_bfqq)
>  {
>  	if (cur_bfqq->entity.parent &&
>  	    cur_bfqq->entity.parent->last_bfqq_created == cur_bfqq)
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index 08ddf2cfae5b..e16d96e2367b 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -1156,6 +1156,8 @@ void bfq_del_bfqq_busy(struct bfq_queue *bfqq, bool expiration);
>  void bfq_add_bfqq_busy(struct bfq_queue *bfqq);
>  void bfq_add_bfqq_in_groups_with_pending_reqs(struct bfq_queue *bfqq);
>  void bfq_del_bfqq_in_groups_with_pending_reqs(struct bfq_queue *bfqq);
> +void bfq_reassign_last_bfqq(struct bfq_queue *cur_bfqq,
> +			    struct bfq_queue *new_bfqq);
>  
>  /* --------------- end of interface of B-WF2Q+ ---------------- */
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jan Kara]

On Wed 04-09-24 09:32:26, Yu Kuai wrote:
> 在 2024/09/03 23:51, Jens Axboe 写道:
> > On 9/2/24 7:03 AM, Yu Kuai wrote:
> > > From: Yu Kuai <yukuai3@huawei.com>
> > > 
> > > Our syzkaller report a UAF problem(details in patch 1), however it can't
> > > be reporduced. And this set are some corner cases fix that might be
> > > related, and they are found by code review.
> > > 
> > > Yu Kuai (4):
> > >    block, bfq: fix possible UAF for bfqq->bic with merge chain
> > >    block, bfq: choose the last bfqq from merge chain in
> > >      bfq_setup_cooperator()
> > >    block, bfq: don't break merge chain in bfq_split_bfqq()
> > >    block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
> > > 
> > >   block/bfq-cgroup.c  |  7 +------
> > >   block/bfq-iosched.c | 17 +++++++++++------
> > >   block/bfq-iosched.h |  2 ++
> > >   3 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > BFQ is effectively unmaintained, and has been for quite a while at
> > this point. I'll apply these, thanks for looking into it, but I think we
> > should move BFQ to an unmaintained state at this point.
> 
> Sorry to hear that, we would be willing to take on the responsibility of
> maintaining this code, please let me know if there are any specific
> guidelines or processes we should follow. We do have customers are using
> bfq in downstream kernels, and we are still running lots of test for
> bfq.

That would be awesome. I don't think there's much of a process to follow
given there's not much happening in BFQ. You can add yourself to
MAINTAINERS file under "BFQ I/O SCHEDULER" entry and then do your best to
keep BFQ alive by fixing bugs and responding to reports :) I'm not sure if
Jens would prefer you'd create your git tree from which he will pull or
whether merging patches is fine - he has to decide.

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

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jens Axboe]

On 9/4/24 6:29 AM, Jan Kara wrote:
> On Wed 04-09-24 09:32:26, Yu Kuai wrote:
>> ? 2024/09/03 23:51, Jens Axboe ??:
>>> On 9/2/24 7:03 AM, Yu Kuai wrote:
>>>> From: Yu Kuai <yukuai3@huawei.com>
>>>>
>>>> Our syzkaller report a UAF problem(details in patch 1), however it can't
>>>> be reporduced. And this set are some corner cases fix that might be
>>>> related, and they are found by code review.
>>>>
>>>> Yu Kuai (4):
>>>>    block, bfq: fix possible UAF for bfqq->bic with merge chain
>>>>    block, bfq: choose the last bfqq from merge chain in
>>>>      bfq_setup_cooperator()
>>>>    block, bfq: don't break merge chain in bfq_split_bfqq()
>>>>    block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
>>>>
>>>>   block/bfq-cgroup.c  |  7 +------
>>>>   block/bfq-iosched.c | 17 +++++++++++------
>>>>   block/bfq-iosched.h |  2 ++
>>>>   3 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> BFQ is effectively unmaintained, and has been for quite a while at
>>> this point. I'll apply these, thanks for looking into it, but I think we
>>> should move BFQ to an unmaintained state at this point.
>>
>> Sorry to hear that, we would be willing to take on the responsibility of
>> maintaining this code, please let me know if there are any specific
>> guidelines or processes we should follow. We do have customers are using
>> bfq in downstream kernels, and we are still running lots of test for
>> bfq.
> 
> That would be awesome. I don't think there's much of a process to follow
> given there's not much happening in BFQ. You can add yourself to
> MAINTAINERS file under "BFQ I/O SCHEDULER" entry and then do your best to
> keep BFQ alive by fixing bugs and responding to reports :) I'm not sure if
> Jens would prefer you'd create your git tree from which he will pull or
> whether merging patches is fine - he has to decide.

The usual process is that you start actually maintaining it, and after a
bit of a track record has been proven, then add the maintainers entry.
Too many times people start by adding a maintainers entry and then don't
really do anything. Not saying that'd necessarily be the case here, but
maintaining first and then adding an entry down the line seems like the
better approach.

I prefer people sending patches, as there's less risk there for messing
it up. Maintaining a git tree may seem easy, but lots of people end up
messing it up, particularly as a new maintainer.

-- 
Jens Axboe

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jens Axboe]

On 9/3/24 7:32 PM, Yu Kuai wrote:
> Hi,
> 
> 在 2024/09/03 23:51, Jens Axboe 写道:
>> On 9/2/24 7:03 AM, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> Our syzkaller report a UAF problem(details in patch 1), however it can't
>>> be reporduced. And this set are some corner cases fix that might be
>>> related, and they are found by code review.
>>>
>>> Yu Kuai (4):
>>>    block, bfq: fix possible UAF for bfqq->bic with merge chain
>>>    block, bfq: choose the last bfqq from merge chain in
>>>      bfq_setup_cooperator()
>>>    block, bfq: don't break merge chain in bfq_split_bfqq()
>>>    block, bfq: use bfq_reassign_last_bfqq() in bfq_bfqq_move()
>>>
>>>   block/bfq-cgroup.c  |  7 +------
>>>   block/bfq-iosched.c | 17 +++++++++++------
>>>   block/bfq-iosched.h |  2 ++
>>>   3 files changed, 14 insertions(+), 12 deletions(-)
>>
>> BFQ is effectively unmaintained, and has been for quite a while at
>> this point. I'll apply these, thanks for looking into it, but I think we
>> should move BFQ to an unmaintained state at this point.
> 
> Sorry to hear that, we would be willing to take on the responsibility of
> maintaining this code, please let me know if there are any specific
> guidelines or processes we should follow. We do have customers are using
> bfq in downstream kernels, and we are still running lots of test for
> bfq.

Most important is just reviewing fixes and tending to bug reports, and
then collecting those fixes and sending them out to the list+me for
inclusion. Not much more needs to happen, this series is a good example
of it.

-- 
Jens Axboe

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Jens Axboe]

On 9/3/24 8:45 PM, Yu Kuai wrote:
> Hi,
> 
> ? 2024/09/04 10:28, Bart Van Assche ??:
>> On 9/3/24 6:32 PM, Yu Kuai wrote:
>>> We do have customers are using bfq in downstream kernels, and we are
>>> still running lots of test for bfq.
>>
>> It may take less time to add any missing functionality to another I/O
>> scheduler rather than to keep maintaining BFQ.
>>
>> If Android device vendors would stop using BFQ, my job would become
>> easier.
> 
> I'm confused now, I think keep maintaining BFQ won't stop you from
> adding new functionality to another scheduler, right? Is this something
> that all scheduler have to support?

With fear of putting words into Bart's mouth, perhaps he's saying that
the BFQ is a bit of a mess and it'd be nice if we had a cleaner version
of some of the features it brings. But having someone actually maintain
it and perhaps clean it up a bit and reduce the complexity would be a
good thing. Really it's the authors choice on where to best spend his or
her time.

-- 
Jens Axboe

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Bart Van Assche]

On 9/3/24 7:45 PM, Yu Kuai wrote:
> 在 2024/09/04 10:28, Bart Van Assche 写道:
>> On 9/3/24 6:32 PM, Yu Kuai wrote:
>>> We do have customers are using bfq in downstream kernels, and we are
>>> still running lots of test for bfq.
>>
>> It may take less time to add any missing functionality to another I/O
>> scheduler rather than to keep maintaining BFQ.
>>
>> If Android device vendors would stop using BFQ, my job would become
>> easier.
> 
> I'm confused now, I think keep maintaining BFQ won't stop you from
> adding new functionality to another scheduler, right? Is this something
> that all scheduler have to support?

As long as the BFQ I/O scheduler does not get deprecated, there will be
Android device vendors that select it for their devices. BFQ bug reports
are either sent to one of my colleagues or to myself.

For Android devices that use UFS storage, we noticed that the
mq-deadline scheduler is good enough. The device boot time is shorter
and I'm not aware of any significant differences in application startup
time.

Thanks,

Bart.

Re: [PATCH for-6.12 0/4] block, bfq: fix corner cases related to bfqq merging

[Yu Kuai]

Hi,

在 2024/09/05 1:17, Bart Van Assche 写道:
> On 9/3/24 7:45 PM, Yu Kuai wrote:
>> 在 2024/09/04 10:28, Bart Van Assche 写道:
>>> On 9/3/24 6:32 PM, Yu Kuai wrote:
>>>> We do have customers are using bfq in downstream kernels, and we are
>>>> still running lots of test for bfq.
>>>
>>> It may take less time to add any missing functionality to another I/O
>>> scheduler rather than to keep maintaining BFQ.
>>>
>>> If Android device vendors would stop using BFQ, my job would become
>>> easier.
>>
>> I'm confused now, I think keep maintaining BFQ won't stop you from
>> adding new functionality to another scheduler, right? Is this something
>> that all scheduler have to support?
> 
> As long as the BFQ I/O scheduler does not get deprecated, there will be
> Android device vendors that select it for their devices. BFQ bug reports
> are either sent to one of my colleagues or to myself.

Then, you can share them to me now, I'll like to help.
> 
> For Android devices that use UFS storage, we noticed that the
> mq-deadline scheduler is good enough. The device boot time is shorter
> and I'm not aware of any significant differences in application startup
> time.

We're using bfq for HDD, performance overhead in bfq is not less,
like you said, if bfq doen't show better results in UFS storage, and you
don't want to use the io control feature, you can choose not to use it,
however, remove bfq will be too aggressive.

Thanks,
Kuai

> 
> Thanks,
> 
> Bart.
> 
> .
>