* [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
@ 2023-03-07 7:14 Shinichiro Kawasaki
2023-03-07 8:57 ` Yu Kuai
0 siblings, 1 reply; 12+ messages in thread
From: Shinichiro Kawasaki @ 2023-03-07 7:14 UTC (permalink / raw)
To: linux-block
Cc: Gabriele Felici, Gianmarco Lusvardi, Giulio Barabino,
Emiliano Maccaferri, Paolo Valente, Damien Le Moal
I observe the KASAN BUG message with kernel v6.3-rc1 during my system boot [1].
The BUG is reliably recreated. I bisected and found that the trigger commit is
fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq"). I
reverted the commit from v6.3-rc1, and observed the BUG disappears. Action for
fix will be appreciated. I can take actions on my system if it helps.
[1]
...
[ 49.534400] NET: Registered PF_QIPCRTR protocol family
[ 51.420663] ==================================================================
[ 51.422452] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator+0x120b/0x1650
[ 51.423576] Read of size 4 at addr ffff88811a8bd600 by task NetworkManager/724
[ 51.425032] CPU: 3 PID: 724 Comm: NetworkManager Not tainted 6.3.0-rc1-kts #1
[ 51.426105] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014
[ 51.427647] Call Trace:
[ 51.428103] <TASK>
[ 51.428472] dump_stack_lvl+0x57/0x90
[ 51.429042] print_report+0xcf/0x630
[ 51.429642] ? bfq_setup_cooperator+0x120b/0x1650
[ 51.430296] kasan_report+0xbb/0xf0
[ 51.430843] ? bfq_setup_cooperator+0x120b/0x1650
[ 51.431487] bfq_setup_cooperator+0x120b/0x1650
[ 51.432175] ? __pfx_lock_release+0x10/0x10
[ 51.432769] ? __pfx_bfq_setup_cooperator+0x10/0x10
[ 51.433442] ? lock_is_held_type+0xe3/0x140
[ 51.434046] bfq_insert_requests+0xdfc/0x9360
[ 51.434622] ? __pfx___lock_acquire+0x10/0x10
[ 51.435248] ? set_operstate+0x193/0x1f0
[ 51.435778] ? __pfx_bfq_insert_requests+0x10/0x10
[ 51.436440] ? blk_mq_sched_insert_requests+0xba/0x880
[ 51.437091] ? __pfx_lock_release+0x10/0x10
[ 51.437700] blk_mq_sched_insert_requests+0x16b/0x880
[ 51.438356] blk_mq_flush_plug_list+0x341/0xdb0
[ 51.438930] ? __pfx_blk_mq_flush_plug_list+0x10/0x10
[ 51.439600] __blk_flush_plug+0x28d/0x450
[ 51.440117] ? __pfx___blk_flush_plug+0x10/0x10
[ 51.440734] blk_finish_plug+0x4b/0xa0
[ 51.441199] read_pages+0x50a/0xb90
[ 51.441627] ? __pfx_read_pages+0x10/0x10
[ 51.442147] ? free_unref_page_commit+0x243/0x500
[ 51.442698] ? _raw_spin_unlock+0x29/0x50
[ 51.443176] ? free_unref_page+0x2f2/0x400
[ 51.443687] page_cache_ra_order+0x617/0x870
[ 51.444198] filemap_fault+0xe45/0x1eb0
[ 51.444714] ? __pfx_filemap_fault+0x10/0x10
[ 51.445221] ? lock_is_held_type+0xe3/0x140
[ 51.445711] ? lock_is_held_type+0xe3/0x140
[ 51.446238] __xfs_filemap_fault+0x141/0x7d0 [xfs]
[ 51.447406] ? __pfx___xfs_filemap_fault+0x10/0x10 [xfs]
[ 51.448302] ? xfs_filemap_map_pages+0x9d/0xd0 [xfs]
[ 51.449200] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
[ 51.450073] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
[ 51.450953] __do_fault+0xef/0x5b0
[ 51.451357] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
[ 51.452236] do_fault+0x4c1/0xec0
[ 51.452619] ? __pfx_pmd_page_vaddr+0x10/0x10
[ 51.453139] __handle_mm_fault+0xc40/0x2410
[ 51.453605] ? lock_is_held_type+0xe3/0x140
[ 51.454063] ? __pfx___handle_mm_fault+0x10/0x10
[ 51.454617] ? count_memcg_events.constprop.0+0x40/0x50
[ 51.455171] handle_mm_fault+0x21f/0x7a0
[ 51.455616] do_user_addr_fault+0x344/0xed0
[ 51.456130] exc_page_fault+0x65/0x100
[ 51.456555] asm_exc_page_fault+0x22/0x30
[ 51.456999] RIP: 0033:0x562259a3da00
[ 51.457472] Code: Unable to access opcode bytes at 0x562259a3d9d6.
[ 51.458109] RSP: 002b:00007ffcd8f6c5d8 EFLAGS: 00010287
[ 51.458718] RAX: 0000562259a3da00 RBX: 0000000000000000 RCX: 0000000000000000
[ 51.459449] RDX: 00007f07c1ce9310 RSI: 000056225a0bb6f0 RDI: 000056225a07d8f0
[ 51.460224] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000001
[ 51.460919] R10: 0000000000000001 R11: 00007f07c1b58c80 R12: 000056225a07d8f0
[ 51.461669] R13: 0000000000000000 R14: 000056225a0c43d0 R15: 00007ffcd8f6c780
[ 51.462382] </TASK>
[ 51.462907] Allocated by task 723:
[ 51.463287] kasan_save_stack+0x1c/0x40
[ 51.463705] kasan_set_track+0x21/0x30
[ 51.464163] __kasan_slab_alloc+0x85/0x90
[ 51.464602] kmem_cache_alloc_node+0x16a/0x330
[ 51.465068] bfq_get_queue+0x1fc/0x1420
[ 51.465537] bfq_get_bfqq_handle_split+0x11a/0x510
[ 51.466029] bfq_insert_requests+0x731/0x9360
[ 51.466492] blk_mq_sched_insert_requests+0x16b/0x880
[ 51.467068] blk_mq_flush_plug_list+0x341/0xdb0
[ 51.513146] __blk_flush_plug+0x28d/0x450
[ 51.743436] blk_finish_plug+0x4b/0xa0
[ 51.800072] _xfs_buf_ioapply+0x68c/0xab0 [xfs]
[ 51.800884] __xfs_buf_submit+0x1e8/0x7b0 [xfs]
[ 51.801677] xfs_buf_read_map+0x301/0xad0 [xfs]
[ 51.802521] xfs_trans_read_buf_map+0x280/0x7c0 [xfs]
[ 51.803368] xfs_imap_to_bp+0xe6/0x140 [xfs]
[ 51.804164] xfs_iget+0x780/0x2a60 [xfs]
[ 51.804909] xfs_lookup+0x234/0x390 [xfs]
[ 51.805669] xfs_vn_lookup+0x108/0x150 [xfs]
[ 51.806442] lookup_open.isra.0+0x7e8/0x1280
[ 51.806965] path_openat+0x829/0x25d0
[ 51.807388] do_filp_open+0x19f/0x3b0
[ 51.807803] do_open_execat+0xa8/0x570
[ 51.808282] bprm_execve+0x3da/0x15e0
[ 51.808698] do_execveat_common.isra.0+0x4d6/0x6c0
[ 51.809213] __x64_sys_execve+0x88/0xb0
[ 51.809678] do_syscall_64+0x37/0x90
[ 51.810084] entry_SYSCALL_64_after_hwframe+0x72/0xdc
[ 51.810895] Freed by task 724:
[ 51.811256] kasan_save_stack+0x1c/0x40
[ 51.811688] kasan_set_track+0x21/0x30
[ 51.812161] kasan_save_free_info+0x2a/0x50
[ 51.812627] ____kasan_slab_free+0x169/0x1c0
[ 51.813096] slab_free_freelist_hook+0xdb/0x1b0
[ 51.813642] kmem_cache_free+0xdb/0x390
[ 51.814071] bfq_put_queue+0x439/0x950
[ 51.814497] bfq_setup_cooperator+0xa41/0x1650
[ 51.815030] bfq_insert_requests+0xdfc/0x9360
[ 51.815503] blk_mq_sched_insert_requests+0x16b/0x880
[ 51.816042] blk_mq_flush_plug_list+0x341/0xdb0
[ 51.816583] __blk_flush_plug+0x28d/0x450
[ 51.817027] blk_finish_plug+0x4b/0xa0
[ 51.817451] read_pages+0x50a/0xb90
[ 51.817897] page_cache_ra_order+0x617/0x870
[ 51.818368] filemap_fault+0xe45/0x1eb0
[ 51.818801] __xfs_filemap_fault+0x141/0x7d0 [xfs]
[ 51.819622] __do_fault+0xef/0x5b0
[ 51.820011] do_fault+0x4c1/0xec0
[ 51.820448] __handle_mm_fault+0xc40/0x2410
[ 51.820910] handle_mm_fault+0x21f/0x7a0
[ 51.821352] do_user_addr_fault+0x344/0xed0
[ 51.821864] exc_page_fault+0x65/0x100
[ 51.822289] asm_exc_page_fault+0x22/0x30
[ 51.822956] The buggy address belongs to the object at ffff88811a8bd600
which belongs to the cache bfq_queue of size 576
[ 51.824269] The buggy address is located 0 bytes inside of
freed 576-byte region [ffff88811a8bd600, ffff88811a8bd840)
[ 51.825761] The buggy address belongs to the physical page:
[ 51.826393] page:00000000e11d915c refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88811a8bc2c0 pfn:0x11a8bc
[ 51.827462] head:00000000e11d915c order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[ 51.828247] flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
[ 51.829021] raw: 0017ffffc0010200 ffff888100a95cc0 dead000000000122 0000000000000000
[ 51.829779] raw: ffff88811a8bc2c0 0000000080170011 00000001ffffffff 0000000000000000
[ 51.830581] page dumped because: kasan: bad access detected
[ 51.831358] Memory state around the buggy address:
[ 51.831907] ffff88811a8bd500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 51.832615] ffff88811a8bd580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 51.833371] >ffff88811a8bd600: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 51.834077] ^
[ 51.834496] ffff88811a8bd680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 51.835228] ffff88811a8bd700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 51.836009] ==================================================================
[ 51.836739] Disabling lock debugging due to kernel taint
[ 51.999350] e1000: ens3 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
...
--
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 7:14 [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator Shinichiro Kawasaki
@ 2023-03-07 8:57 ` Yu Kuai
2023-03-07 9:13 ` Shinichiro Kawasaki
0 siblings, 1 reply; 12+ messages in thread
From: Yu Kuai @ 2023-03-07 8:57 UTC (permalink / raw)
To: Shinichiro Kawasaki, linux-block
Cc: Gabriele Felici, Gianmarco Lusvardi, Giulio Barabino,
Emiliano Maccaferri, Paolo Valente, Damien Le Moal, yukuai (C),
Jan Kara
Hi,
在 2023/03/07 15:14, Shinichiro Kawasaki 写道:
> I observe the KASAN BUG message with kernel v6.3-rc1 during my system boot [1].
> The BUG is reliably recreated. I bisected and found that the trigger commit is
> fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq"). I
> reverted the commit from v6.3-rc1, and observed the BUG disappears. Action for
> fix will be appreciated. I can take actions on my system if it helps.
>
> [1]
>
> ...
> [ 49.534400] NET: Registered PF_QIPCRTR protocol family
> [ 51.420663] ==================================================================
> [ 51.422452] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator+0x120b/0x1650
> [ 51.423576] Read of size 4 at addr ffff88811a8bd600 by task NetworkManager/724
>
> [ 51.425032] CPU: 3 PID: 724 Comm: NetworkManager Not tainted 6.3.0-rc1-kts #1
> [ 51.426105] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014
> [ 51.427647] Call Trace:
> [ 51.428103] <TASK>
> [ 51.428472] dump_stack_lvl+0x57/0x90
> [ 51.429042] print_report+0xcf/0x630
> [ 51.429642] ? bfq_setup_cooperator+0x120b/0x1650
> [ 51.430296] kasan_report+0xbb/0xf0
> [ 51.430843] ? bfq_setup_cooperator+0x120b/0x1650
> [ 51.431487] bfq_setup_cooperator+0x120b/0x1650
> [ 51.432175] ? __pfx_lock_release+0x10/0x10
> [ 51.432769] ? __pfx_bfq_setup_cooperator+0x10/0x10
> [ 51.433442] ? lock_is_held_type+0xe3/0x140
> [ 51.434046] bfq_insert_requests+0xdfc/0x9360
> [ 51.434622] ? __pfx___lock_acquire+0x10/0x10
> [ 51.435248] ? set_operstate+0x193/0x1f0
> [ 51.435778] ? __pfx_bfq_insert_requests+0x10/0x10
> [ 51.436440] ? blk_mq_sched_insert_requests+0xba/0x880
> [ 51.437091] ? __pfx_lock_release+0x10/0x10
> [ 51.437700] blk_mq_sched_insert_requests+0x16b/0x880
> [ 51.438356] blk_mq_flush_plug_list+0x341/0xdb0
> [ 51.438930] ? __pfx_blk_mq_flush_plug_list+0x10/0x10
> [ 51.439600] __blk_flush_plug+0x28d/0x450
> [ 51.440117] ? __pfx___blk_flush_plug+0x10/0x10
> [ 51.440734] blk_finish_plug+0x4b/0xa0
> [ 51.441199] read_pages+0x50a/0xb90
> [ 51.441627] ? __pfx_read_pages+0x10/0x10
> [ 51.442147] ? free_unref_page_commit+0x243/0x500
> [ 51.442698] ? _raw_spin_unlock+0x29/0x50
> [ 51.443176] ? free_unref_page+0x2f2/0x400
> [ 51.443687] page_cache_ra_order+0x617/0x870
> [ 51.444198] filemap_fault+0xe45/0x1eb0
> [ 51.444714] ? __pfx_filemap_fault+0x10/0x10
> [ 51.445221] ? lock_is_held_type+0xe3/0x140
> [ 51.445711] ? lock_is_held_type+0xe3/0x140
> [ 51.446238] __xfs_filemap_fault+0x141/0x7d0 [xfs]
> [ 51.447406] ? __pfx___xfs_filemap_fault+0x10/0x10 [xfs]
> [ 51.448302] ? xfs_filemap_map_pages+0x9d/0xd0 [xfs]
> [ 51.449200] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
> [ 51.450073] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
> [ 51.450953] __do_fault+0xef/0x5b0
> [ 51.451357] ? __pfx_xfs_filemap_map_pages+0x10/0x10 [xfs]
> [ 51.452236] do_fault+0x4c1/0xec0
> [ 51.452619] ? __pfx_pmd_page_vaddr+0x10/0x10
> [ 51.453139] __handle_mm_fault+0xc40/0x2410
> [ 51.453605] ? lock_is_held_type+0xe3/0x140
> [ 51.454063] ? __pfx___handle_mm_fault+0x10/0x10
> [ 51.454617] ? count_memcg_events.constprop.0+0x40/0x50
> [ 51.455171] handle_mm_fault+0x21f/0x7a0
> [ 51.455616] do_user_addr_fault+0x344/0xed0
> [ 51.456130] exc_page_fault+0x65/0x100
> [ 51.456555] asm_exc_page_fault+0x22/0x30
> [ 51.456999] RIP: 0033:0x562259a3da00
> [ 51.457472] Code: Unable to access opcode bytes at 0x562259a3d9d6.
> [ 51.458109] RSP: 002b:00007ffcd8f6c5d8 EFLAGS: 00010287
> [ 51.458718] RAX: 0000562259a3da00 RBX: 0000000000000000 RCX: 0000000000000000
> [ 51.459449] RDX: 00007f07c1ce9310 RSI: 000056225a0bb6f0 RDI: 000056225a07d8f0
> [ 51.460224] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000001
> [ 51.460919] R10: 0000000000000001 R11: 00007f07c1b58c80 R12: 000056225a07d8f0
> [ 51.461669] R13: 0000000000000000 R14: 000056225a0c43d0 R15: 00007ffcd8f6c780
> [ 51.462382] </TASK>
>
> [ 51.462907] Allocated by task 723:
> [ 51.463287] kasan_save_stack+0x1c/0x40
> [ 51.463705] kasan_set_track+0x21/0x30
> [ 51.464163] __kasan_slab_alloc+0x85/0x90
> [ 51.464602] kmem_cache_alloc_node+0x16a/0x330
> [ 51.465068] bfq_get_queue+0x1fc/0x1420
> [ 51.465537] bfq_get_bfqq_handle_split+0x11a/0x510
> [ 51.466029] bfq_insert_requests+0x731/0x9360
> [ 51.466492] blk_mq_sched_insert_requests+0x16b/0x880
> [ 51.467068] blk_mq_flush_plug_list+0x341/0xdb0
> [ 51.513146] __blk_flush_plug+0x28d/0x450
> [ 51.743436] blk_finish_plug+0x4b/0xa0
> [ 51.800072] _xfs_buf_ioapply+0x68c/0xab0 [xfs]
> [ 51.800884] __xfs_buf_submit+0x1e8/0x7b0 [xfs]
> [ 51.801677] xfs_buf_read_map+0x301/0xad0 [xfs]
> [ 51.802521] xfs_trans_read_buf_map+0x280/0x7c0 [xfs]
> [ 51.803368] xfs_imap_to_bp+0xe6/0x140 [xfs]
> [ 51.804164] xfs_iget+0x780/0x2a60 [xfs]
> [ 51.804909] xfs_lookup+0x234/0x390 [xfs]
> [ 51.805669] xfs_vn_lookup+0x108/0x150 [xfs]
> [ 51.806442] lookup_open.isra.0+0x7e8/0x1280
> [ 51.806965] path_openat+0x829/0x25d0
> [ 51.807388] do_filp_open+0x19f/0x3b0
> [ 51.807803] do_open_execat+0xa8/0x570
> [ 51.808282] bprm_execve+0x3da/0x15e0
> [ 51.808698] do_execveat_common.isra.0+0x4d6/0x6c0
> [ 51.809213] __x64_sys_execve+0x88/0xb0
> [ 51.809678] do_syscall_64+0x37/0x90
> [ 51.810084] entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> [ 51.810895] Freed by task 724:
> [ 51.811256] kasan_save_stack+0x1c/0x40
> [ 51.811688] kasan_set_track+0x21/0x30
> [ 51.812161] kasan_save_free_info+0x2a/0x50
> [ 51.812627] ____kasan_slab_free+0x169/0x1c0
> [ 51.813096] slab_free_freelist_hook+0xdb/0x1b0
> [ 51.813642] kmem_cache_free+0xdb/0x390
> [ 51.814071] bfq_put_queue+0x439/0x950
> [ 51.814497] bfq_setup_cooperator+0xa41/0x1650
> [ 51.815030] bfq_insert_requests+0xdfc/0x9360
> [ 51.815503] blk_mq_sched_insert_requests+0x16b/0x880
> [ 51.816042] blk_mq_flush_plug_list+0x341/0xdb0
> [ 51.816583] __blk_flush_plug+0x28d/0x450
> [ 51.817027] blk_finish_plug+0x4b/0xa0
> [ 51.817451] read_pages+0x50a/0xb90
> [ 51.817897] page_cache_ra_order+0x617/0x870
> [ 51.818368] filemap_fault+0xe45/0x1eb0
> [ 51.818801] __xfs_filemap_fault+0x141/0x7d0 [xfs]
> [ 51.819622] __do_fault+0xef/0x5b0
> [ 51.820011] do_fault+0x4c1/0xec0
> [ 51.820448] __handle_mm_fault+0xc40/0x2410
> [ 51.820910] handle_mm_fault+0x21f/0x7a0
> [ 51.821352] do_user_addr_fault+0x344/0xed0
> [ 51.821864] exc_page_fault+0x65/0x100
> [ 51.822289] asm_exc_page_fault+0x22/0x30
>
> [ 51.822956] The buggy address belongs to the object at ffff88811a8bd600
> which belongs to the cache bfq_queue of size 576
> [ 51.824269] The buggy address is located 0 bytes inside of
> freed 576-byte region [ffff88811a8bd600, ffff88811a8bd840)
>
> [ 51.825761] The buggy address belongs to the physical page:
> [ 51.826393] page:00000000e11d915c refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88811a8bc2c0 pfn:0x11a8bc
> [ 51.827462] head:00000000e11d915c order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> [ 51.828247] flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> [ 51.829021] raw: 0017ffffc0010200 ffff888100a95cc0 dead000000000122 0000000000000000
> [ 51.829779] raw: ffff88811a8bc2c0 0000000080170011 00000001ffffffff 0000000000000000
> [ 51.830581] page dumped because: kasan: bad access detected
>
> [ 51.831358] Memory state around the buggy address:
> [ 51.831907] ffff88811a8bd500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 51.832615] ffff88811a8bd580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 51.833371] >ffff88811a8bd600: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 51.834077] ^
> [ 51.834496] ffff88811a8bd680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 51.835228] ffff88811a8bd700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 51.836009] ==================================================================
> [ 51.836739] Disabling lock debugging due to kernel taint
> [ 51.999350] e1000: ens3 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX
> ...
>
Thanks for the report, can you help to provide the result of add2line of
following?
bfq_setup_cooperator+0x120b/0x1650
bfq_setup_cooperator+0xa41/0x1650
That will help to locate the problem.
Thanks,
Kuai
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 8:57 ` Yu Kuai
@ 2023-03-07 9:13 ` Shinichiro Kawasaki
2023-03-07 9:36 ` Yu Kuai
0 siblings, 1 reply; 12+ messages in thread
From: Shinichiro Kawasaki @ 2023-03-07 9:13 UTC (permalink / raw)
To: Yu Kuai
Cc: linux-block, Gabriele Felici, Gianmarco Lusvardi,
Giulio Barabino, Emiliano Maccaferri, Paolo Valente,
Damien Le Moal, yukuai (C),
Jan Kara
On Mar 07, 2023 / 16:57, Yu Kuai wrote:
[...]
> Thanks for the report, can you help to provide the result of add2line of
> following?
>
> bfq_setup_cooperator+0x120b/0x1650
> bfq_setup_cooperator+0xa41/0x1650
>
> That will help to locate the problem.
Hi, Yu thanks for looking into this. Here are the faddr2line outputs:
$ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0x120b/0x1650
bfq_setup_cooperator+0x120b/0x1650:
bfqq_process_refs at block/bfq-iosched.c:1200
(inlined by) bfq_setup_stable_merge at block/bfq-iosched.c:2855
(inlined by) bfq_setup_cooperator at block/bfq-iosched.c:2941
$ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0xa41/0x1650
bfq_setup_cooperator+0xa41/0x1650:
bfq_setup_cooperator at block/bfq-iosched.c:2939
--
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 9:13 ` Shinichiro Kawasaki
@ 2023-03-07 9:36 ` Yu Kuai
2023-03-07 10:20 ` Jan Kara
0 siblings, 1 reply; 12+ messages in thread
From: Yu Kuai @ 2023-03-07 9:36 UTC (permalink / raw)
To: Shinichiro Kawasaki, Yu Kuai
Cc: linux-block, Gabriele Felici, Gianmarco Lusvardi,
Giulio Barabino, Emiliano Maccaferri, Paolo Valente,
Damien Le Moal, Jan Kara, yukuai (C)
Hi,
在 2023/03/07 17:13, Shinichiro Kawasaki 写道:
> On Mar 07, 2023 / 16:57, Yu Kuai wrote:
> [...]
>> Thanks for the report, can you help to provide the result of add2line of
>> following?
>>
>> bfq_setup_cooperator+0x120b/0x1650
>> bfq_setup_cooperator+0xa41/0x1650
>>
>> That will help to locate the problem.
>
> Hi, Yu thanks for looking into this. Here are the faddr2line outputs:
>
> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0x120b/0x1650
> bfq_setup_cooperator+0x120b/0x1650:
> bfqq_process_refs at block/bfq-iosched.c:1200
> (inlined by) bfq_setup_stable_merge at block/bfq-iosched.c:2855
> (inlined by) bfq_setup_cooperator at block/bfq-iosched.c:2941
>
> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0xa41/0x1650
> bfq_setup_cooperator+0xa41/0x1650:
> bfq_setup_cooperator at block/bfq-iosched.c:2939
>
So, after a quick look at the code, the difference is that fd571df0ac5b
changes the logic when bfqq_proces_refs(stable_merge_bfqq) is 0.
I think perhaps following patch can work, at least 'stable_merge_bfqq'
won't be accessed after bfq_put_stable_ref() in this case:
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 8a8d4441519c..881f74b3a556 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2856,6 +2856,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd,
struct bfq_queue *bfqq,
bfqq_process_refs(stable_merge_bfqq));
struct bfq_queue *new_bfqq;
+ /* deschedule stable merge, because done or aborted here */
+ bfq_put_stable_ref(stable_merge_bfqq);
+
+ bfqq_data->stable_merge_bfqq = NULL;
+
if (idling_boosts_thr_without_issues(bfqd, bfqq) ||
proc_ref == 0)
return NULL;
@@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd,
struct bfq_queue *bfqq,
struct bfq_queue *stable_merge_bfqq =
bfqq_data->stable_merge_bfqq;
- /* deschedule stable merge, because done or
aborted here */
- bfq_put_stable_ref(stable_merge_bfqq);
-
- bfqq_data->stable_merge_bfqq = NULL;
-
return bfq_setup_stable_merge(bfqd, bfqq,
stable_merge_bfqq,
bfqq_data);
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 9:36 ` Yu Kuai
@ 2023-03-07 10:20 ` Jan Kara
2023-03-07 10:28 ` Yu Kuai
0 siblings, 1 reply; 12+ messages in thread
From: Jan Kara @ 2023-03-07 10:20 UTC (permalink / raw)
To: Yu Kuai
Cc: Shinichiro Kawasaki, linux-block, Gabriele Felici,
Gianmarco Lusvardi, Giulio Barabino, Emiliano Maccaferri,
Paolo Valente, Damien Le Moal, Jan Kara, yukuai (C)
On Tue 07-03-23 17:36:19, Yu Kuai wrote:
> Hi,
>
> 在 2023/03/07 17:13, Shinichiro Kawasaki 写道:
> > On Mar 07, 2023 / 16:57, Yu Kuai wrote:
> > [...]
> > > Thanks for the report, can you help to provide the result of add2line of
> > > following?
> > >
> > > bfq_setup_cooperator+0x120b/0x1650
> > > bfq_setup_cooperator+0xa41/0x1650
> > >
> > > That will help to locate the problem.
> >
> > Hi, Yu thanks for looking into this. Here are the faddr2line outputs:
> >
> > $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0x120b/0x1650
> > bfq_setup_cooperator+0x120b/0x1650:
> > bfqq_process_refs at block/bfq-iosched.c:1200
> > (inlined by) bfq_setup_stable_merge at block/bfq-iosched.c:2855
> > (inlined by) bfq_setup_cooperator at block/bfq-iosched.c:2941
> >
> > $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0xa41/0x1650
> > bfq_setup_cooperator+0xa41/0x1650:
> > bfq_setup_cooperator at block/bfq-iosched.c:2939
> >
>
> So, after a quick look at the code, the difference is that fd571df0ac5b
> changes the logic when bfqq_proces_refs(stable_merge_bfqq) is 0.
>
> I think perhaps following patch can work, at least 'stable_merge_bfqq'
> won't be accessed after bfq_put_stable_ref() in this case:
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 8a8d4441519c..881f74b3a556 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2856,6 +2856,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct
> bfq_queue *bfqq,
> bfqq_process_refs(stable_merge_bfqq));
> struct bfq_queue *new_bfqq;
>
> + /* deschedule stable merge, because done or aborted here */
> + bfq_put_stable_ref(stable_merge_bfqq);
> +
> + bfqq_data->stable_merge_bfqq = NULL;
> +
I don't think this is going to help. stable_merge_bfqq is used just a few
lines below again in bfq_setup_merge(). The problem really is that the
reference from stable merge can be the last one keeping bfqq alive so in
bfq_setup_cooperator() we need to see if stable_merge_bfqq still has
process references (and cancel the merge if not) before dropping our ref.
So rather doing something like:
bfqq_data->stable_merge_bfqq = NULL;
new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
stable_merge_bfqq, bfqq_data);
bfq_put_stable_ref(stable_merge_bfqq);
return new_bfqq;
should work in bfq_setup_cooperator().
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 10:20 ` Jan Kara
@ 2023-03-07 10:28 ` Yu Kuai
2023-03-07 11:49 ` Shinichiro Kawasaki
0 siblings, 1 reply; 12+ messages in thread
From: Yu Kuai @ 2023-03-07 10:28 UTC (permalink / raw)
To: Jan Kara, Yu Kuai
Cc: Shinichiro Kawasaki, linux-block, Gabriele Felici,
Gianmarco Lusvardi, Giulio Barabino, Emiliano Maccaferri,
Paolo Valente, Damien Le Moal, yukuai (C)
Hi, Jan
在 2023/03/07 18:20, Jan Kara 写道:
> On Tue 07-03-23 17:36:19, Yu Kuai wrote:
>> Hi,
>>
>> 在 2023/03/07 17:13, Shinichiro Kawasaki 写道:
>>> On Mar 07, 2023 / 16:57, Yu Kuai wrote:
>>> [...]
>>>> Thanks for the report, can you help to provide the result of add2line of
>>>> following?
>>>>
>>>> bfq_setup_cooperator+0x120b/0x1650
>>>> bfq_setup_cooperator+0xa41/0x1650
>>>>
>>>> That will help to locate the problem.
>>>
>>> Hi, Yu thanks for looking into this. Here are the faddr2line outputs:
>>>
>>> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0x120b/0x1650
>>> bfq_setup_cooperator+0x120b/0x1650:
>>> bfqq_process_refs at block/bfq-iosched.c:1200
>>> (inlined by) bfq_setup_stable_merge at block/bfq-iosched.c:2855
>>> (inlined by) bfq_setup_cooperator at block/bfq-iosched.c:2941
>>>
>>> $ ./scripts/faddr2line vmlinux bfq_setup_cooperator+0xa41/0x1650
>>> bfq_setup_cooperator+0xa41/0x1650:
>>> bfq_setup_cooperator at block/bfq-iosched.c:2939
>>>
>>
>> So, after a quick look at the code, the difference is that fd571df0ac5b
>> changes the logic when bfqq_proces_refs(stable_merge_bfqq) is 0.
>>
>> I think perhaps following patch can work, at least 'stable_merge_bfqq'
>> won't be accessed after bfq_put_stable_ref() in this case:
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 8a8d4441519c..881f74b3a556 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -2856,6 +2856,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct
>> bfq_queue *bfqq,
>> bfqq_process_refs(stable_merge_bfqq));
>> struct bfq_queue *new_bfqq;
>>
>> + /* deschedule stable merge, because done or aborted here */
>> + bfq_put_stable_ref(stable_merge_bfqq);
>> +
>> + bfqq_data->stable_merge_bfqq = NULL;
>> +
>
> I don't think this is going to help. stable_merge_bfqq is used just a few
> lines below again in bfq_setup_merge(). The problem really is that the
> reference from stable merge can be the last one keeping bfqq alive so in
> bfq_setup_cooperator() we need to see if stable_merge_bfqq still has
> process references (and cancel the merge if not) before dropping our ref.
I was thinking that bfq_setup_merge() will only be called if 'proc_ref'
is not 0, which means that stable_merge_bfqq won't be freed.
>
> So rather doing something like:
>
> bfqq_data->stable_merge_bfqq = NULL;
> new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
> stable_merge_bfqq, bfqq_data);
> bfq_put_stable_ref(stable_merge_bfqq);
> return new_bfqq;
>
> should work in bfq_setup_cooperator().
Yes, this will work.
Thanks,
Kuai
>
> Honza
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 10:28 ` Yu Kuai
@ 2023-03-07 11:49 ` Shinichiro Kawasaki
2023-03-07 14:26 ` Jens Axboe
0 siblings, 1 reply; 12+ messages in thread
From: Shinichiro Kawasaki @ 2023-03-07 11:49 UTC (permalink / raw)
To: Yu Kuai
Cc: Jan Kara, linux-block, Gabriele Felici, Gianmarco Lusvardi,
Giulio Barabino, Emiliano Maccaferri, Paolo Valente,
Damien Le Moal, yukuai (C)
On Mar 07, 2023 / 18:28, Yu Kuai wrote:
> Hi, Jan
>
> 在 2023/03/07 18:20, Jan Kara 写道:
[...]
> > So rather doing something like:
> >
> > bfqq_data->stable_merge_bfqq = NULL;
> > new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
> > stable_merge_bfqq, bfqq_data);
> > bfq_put_stable_ref(stable_merge_bfqq);
> > return new_bfqq;
> >
> > should work in bfq_setup_cooperator().
>
> Yes, this will work.
Based on the description above, I quickly created the dirty patch below, and
confirmed it avoids the BUG. Looks good. Jan, Yu, thanks for the quick actions.
Let me wait for the formal patch.
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 8a8d4441519c..50eb435efed0 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2932,15 +2932,15 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
msecs_to_jiffies(bfq_late_stable_merging))) {
struct bfq_queue *stable_merge_bfqq =
bfqq_data->stable_merge_bfqq;
+ static struct bfq_queue *new_bfqq;
/* deschedule stable merge, because done or aborted here */
- bfq_put_stable_ref(stable_merge_bfqq);
-
bfqq_data->stable_merge_bfqq = NULL;
-
- return bfq_setup_stable_merge(bfqd, bfqq,
- stable_merge_bfqq,
- bfqq_data);
+ new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
+ stable_merge_bfqq,
+ bfqq_data);
+ bfq_put_stable_ref(stable_merge_bfqq);
+ return new_bfqq;
}
}
--
Shin'ichiro Kawasaki
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator
2023-03-07 11:49 ` Shinichiro Kawasaki
@ 2023-03-07 14:26 ` Jens Axboe
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
0 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2023-03-07 14:26 UTC (permalink / raw)
To: Shinichiro Kawasaki, Yu Kuai
Cc: Jan Kara, linux-block, Gabriele Felici, Gianmarco Lusvardi,
Giulio Barabino, Emiliano Maccaferri, Paolo Valente,
Damien Le Moal, yukuai (C)
On 3/7/23 4:49 AM, Shinichiro Kawasaki wrote:
> On Mar 07, 2023 / 18:28, Yu Kuai wrote:
>> Hi, Jan
>>
>> 在 2023/03/07 18:20, Jan Kara 写道:
>
> [...]
>
>>> So rather doing something like:
>>>
>>> bfqq_data->stable_merge_bfqq = NULL;
>>> new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
>>> stable_merge_bfqq, bfqq_data);
>>> bfq_put_stable_ref(stable_merge_bfqq);
>>> return new_bfqq;
>>>
>>> should work in bfq_setup_cooperator().
>>
>> Yes, this will work.
>
> Based on the description above, I quickly created the dirty patch below, and
> confirmed it avoids the BUG. Looks good. Jan, Yu, thanks for the quick actions.
> Let me wait for the formal patch.
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 8a8d4441519c..50eb435efed0 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2932,15 +2932,15 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> msecs_to_jiffies(bfq_late_stable_merging))) {
> struct bfq_queue *stable_merge_bfqq =
> bfqq_data->stable_merge_bfqq;
> + static struct bfq_queue *new_bfqq;
>
> /* deschedule stable merge, because done or aborted here */
> - bfq_put_stable_ref(stable_merge_bfqq);
> -
> bfqq_data->stable_merge_bfqq = NULL;
> -
> - return bfq_setup_stable_merge(bfqd, bfqq,
> - stable_merge_bfqq,
> - bfqq_data);
> + new_bfqq = bfq_setup_stable_merge(bfqd, bfqq,
> + stable_merge_bfqq,
> + bfqq_data);
> + bfq_put_stable_ref(stable_merge_bfqq);
> + return new_bfqq;
> }
> }
Can you or Jan post this as a real patch so we can get it queued
up?
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq'
2023-03-07 14:26 ` Jens Axboe
@ 2023-03-08 2:32 ` Yu Kuai
2023-03-08 5:56 ` Shinichiro Kawasaki
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Yu Kuai @ 2023-03-08 2:32 UTC (permalink / raw)
To: jack, shinichiro.kawasaki, paolo.valente, axboe, glusvardi,
damien.lemoal, felicigb, inbox
Cc: linux-block, linux-kernel, yukuai3, yukuai1, yi.zhang, yangerkun
From: Yu Kuai <yukuai3@huawei.com>
Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array
in bfq_io_cq"), process reference is read before bfq_put_stable_ref(),
and it's safe if bfq_put_stable_ref() put the last reference, because
process reference will be 0 and 'stable_merge_bfqq' won't be accessed
in this case. However, the commit changed the order and will cause
uaf for 'stable_merge_bfqq'.
In order to emphasize that bfq_put_stable_ref() can drop the last
reference, fix the problem by moving bfq_put_stable_ref() to the end of
bfq_setup_stable_merge().
Fixes: fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq")
Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Link: https://lore.kernel.org/linux-block/20230307071448.rzihxbm4jhbf5krj@shindev/
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
block/bfq-iosched.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 8a8d4441519c..d9ed3108c17a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
{
int proc_ref = min(bfqq_process_refs(bfqq),
bfqq_process_refs(stable_merge_bfqq));
- struct bfq_queue *new_bfqq;
+ struct bfq_queue *new_bfqq = NULL;
- if (idling_boosts_thr_without_issues(bfqd, bfqq) ||
- proc_ref == 0)
- return NULL;
+ bfqq_data->stable_merge_bfqq = NULL;
+ if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0)
+ goto out;
/* next function will take at least one ref */
new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq);
@@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
new_bfqq_data->stably_merged = true;
}
}
+
+out:
+ /* deschedule stable merge, because done or aborted here */
+ bfq_put_stable_ref(stable_merge_bfqq);
+
return new_bfqq;
}
@@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
struct bfq_queue *stable_merge_bfqq =
bfqq_data->stable_merge_bfqq;
- /* deschedule stable merge, because done or aborted here */
- bfq_put_stable_ref(stable_merge_bfqq);
-
- bfqq_data->stable_merge_bfqq = NULL;
-
return bfq_setup_stable_merge(bfqd, bfqq,
stable_merge_bfqq,
bfqq_data);
--
2.31.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq'
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
@ 2023-03-08 5:56 ` Shinichiro Kawasaki
2023-03-08 10:21 ` Jan Kara
2023-03-08 14:35 ` Jens Axboe
2 siblings, 0 replies; 12+ messages in thread
From: Shinichiro Kawasaki @ 2023-03-08 5:56 UTC (permalink / raw)
To: Yu Kuai
Cc: jack, paolo.valente, axboe, glusvardi, damien.lemoal, felicigb,
inbox, linux-block, linux-kernel, yukuai3, yi.zhang, yangerkun
On Mar 08, 2023 / 10:32, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array
> in bfq_io_cq"), process reference is read before bfq_put_stable_ref(),
> and it's safe if bfq_put_stable_ref() put the last reference, because
> process reference will be 0 and 'stable_merge_bfqq' won't be accessed
> in this case. However, the commit changed the order and will cause
> uaf for 'stable_merge_bfqq'.
>
> In order to emphasize that bfq_put_stable_ref() can drop the last
> reference, fix the problem by moving bfq_put_stable_ref() to the end of
> bfq_setup_stable_merge().
>
> Fixes: fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq")
> Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Link: https://lore.kernel.org/linux-block/20230307071448.rzihxbm4jhbf5krj@shindev/
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> block/bfq-iosched.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 8a8d4441519c..d9ed3108c17a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> {
> int proc_ref = min(bfqq_process_refs(bfqq),
> bfqq_process_refs(stable_merge_bfqq));
> - struct bfq_queue *new_bfqq;
> + struct bfq_queue *new_bfqq = NULL;
>
> - if (idling_boosts_thr_without_issues(bfqd, bfqq) ||
> - proc_ref == 0)
> - return NULL;
> + bfqq_data->stable_merge_bfqq = NULL;
> + if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0)
> + goto out;
>
> /* next function will take at least one ref */
> new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq);
> @@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> new_bfqq_data->stably_merged = true;
> }
> }
> +
> +out:
> + /* deschedule stable merge, because done or aborted here */
> + bfq_put_stable_ref(stable_merge_bfqq);
> +
> return new_bfqq;
> }
>
> @@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> struct bfq_queue *stable_merge_bfqq =
> bfqq_data->stable_merge_bfqq;
Nit: I suggest to remove the local variable above. The two references to it
below are removed, and the argument of bfq_setup_stable_merge() can be replaced
with bfqq_data->stable_merge_bfqq.
>
> - /* deschedule stable merge, because done or aborted here */
> - bfq_put_stable_ref(stable_merge_bfqq);
> -
> - bfqq_data->stable_merge_bfqq = NULL;
> -
> return bfq_setup_stable_merge(bfqd, bfqq,
> stable_merge_bfqq,
> bfqq_data);
> --
> 2.31.1
>
--
Shin'ichiro Kawasaki
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq'
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
2023-03-08 5:56 ` Shinichiro Kawasaki
@ 2023-03-08 10:21 ` Jan Kara
2023-03-08 14:35 ` Jens Axboe
2 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2023-03-08 10:21 UTC (permalink / raw)
To: Yu Kuai
Cc: jack, shinichiro.kawasaki, paolo.valente, axboe, glusvardi,
damien.lemoal, felicigb, inbox, linux-block, linux-kernel,
yukuai3, yi.zhang, yangerkun
On Wed 08-03-23 10:32:08, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array
> in bfq_io_cq"), process reference is read before bfq_put_stable_ref(),
> and it's safe if bfq_put_stable_ref() put the last reference, because
> process reference will be 0 and 'stable_merge_bfqq' won't be accessed
> in this case. However, the commit changed the order and will cause
> uaf for 'stable_merge_bfqq'.
>
> In order to emphasize that bfq_put_stable_ref() can drop the last
> reference, fix the problem by moving bfq_put_stable_ref() to the end of
> bfq_setup_stable_merge().
>
> Fixes: fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq")
> Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Link: https://lore.kernel.org/linux-block/20230307071448.rzihxbm4jhbf5krj@shindev/
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Looks good to me (with or without getting rid of the stable_merge_bfqq)
variable. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> block/bfq-iosched.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 8a8d4441519c..d9ed3108c17a 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> {
> int proc_ref = min(bfqq_process_refs(bfqq),
> bfqq_process_refs(stable_merge_bfqq));
> - struct bfq_queue *new_bfqq;
> + struct bfq_queue *new_bfqq = NULL;
>
> - if (idling_boosts_thr_without_issues(bfqd, bfqq) ||
> - proc_ref == 0)
> - return NULL;
> + bfqq_data->stable_merge_bfqq = NULL;
> + if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0)
> + goto out;
>
> /* next function will take at least one ref */
> new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq);
> @@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> new_bfqq_data->stably_merged = true;
> }
> }
> +
> +out:
> + /* deschedule stable merge, because done or aborted here */
> + bfq_put_stable_ref(stable_merge_bfqq);
> +
> return new_bfqq;
> }
>
> @@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
> struct bfq_queue *stable_merge_bfqq =
> bfqq_data->stable_merge_bfqq;
>
> - /* deschedule stable merge, because done or aborted here */
> - bfq_put_stable_ref(stable_merge_bfqq);
> -
> - bfqq_data->stable_merge_bfqq = NULL;
> -
> return bfq_setup_stable_merge(bfqd, bfqq,
> stable_merge_bfqq,
> bfqq_data);
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq'
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
2023-03-08 5:56 ` Shinichiro Kawasaki
2023-03-08 10:21 ` Jan Kara
@ 2023-03-08 14:35 ` Jens Axboe
2 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2023-03-08 14:35 UTC (permalink / raw)
To: jack, shinichiro.kawasaki, paolo.valente, glusvardi,
damien.lemoal, felicigb, inbox, Yu Kuai
Cc: linux-block, linux-kernel, yukuai3, yi.zhang, yangerkun
On Wed, 08 Mar 2023 10:32:08 +0800, Yu Kuai wrote:
> Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array
> in bfq_io_cq"), process reference is read before bfq_put_stable_ref(),
> and it's safe if bfq_put_stable_ref() put the last reference, because
> process reference will be 0 and 'stable_merge_bfqq' won't be accessed
> in this case. However, the commit changed the order and will cause
> uaf for 'stable_merge_bfqq'.
>
> [...]
Applied, thanks!
[1/1] block, bfq: fix uaf for 'stable_merge_bfqq'
commit: e2f2a39452c43b64ea3191642a2661cb8d03827a
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-08 14:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-07 7:14 [bug report] BUG: KASAN: slab-use-after-free in bfq_setup_cooperator Shinichiro Kawasaki
2023-03-07 8:57 ` Yu Kuai
2023-03-07 9:13 ` Shinichiro Kawasaki
2023-03-07 9:36 ` Yu Kuai
2023-03-07 10:20 ` Jan Kara
2023-03-07 10:28 ` Yu Kuai
2023-03-07 11:49 ` Shinichiro Kawasaki
2023-03-07 14:26 ` Jens Axboe
2023-03-08 2:32 ` [PATCH] block, bfq: fix uaf for 'stable_merge_bfqq' Yu Kuai
2023-03-08 5:56 ` Shinichiro Kawasaki
2023-03-08 10:21 ` Jan Kara
2023-03-08 14:35 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.