[syzbot] [kernel?] possible deadlock in scheduler_tick (2)

[syzbot] [kernel?] possible deadlock in scheduler_tick (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f1fcbaa18b28 Linux 6.4-rc2
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=1332a029280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3dc1cdd68141cdc3
dashboard link: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm64

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/f9e1748cceea/disk-f1fcbaa1.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/6dea99343621/vmlinux-f1fcbaa1.xz
kernel image: https://storage.googleapis.com/syzbot-assets/f5a93f86012d/Image-f1fcbaa1.gz.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com

======================================================
WARNING: possible circular locking dependency detected
6.4.0-rc2-syzkaller-gf1fcbaa18b28 #0 Not tainted
------------------------------------------------------
klogd/5578 is trying to acquire lock:
ffff0001fea76c40
 (&pgdat->kcompactd_wait){-...}-{2:2}, at: __wake_up_common_lock kernel/sched/wait.c:137 [inline]
 (&pgdat->kcompactd_wait){-...}-{2:2}, at: __wake_up+0xec/0x1a8 kernel/sched/wait.c:160

but task is already holding lock:
ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested kernel/sched/core.c:558 [inline]
ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: rq_lock kernel/sched/sched.h:1653 [inline]
ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: scheduler_tick+0xa4/0x52c kernel/sched/core.c:5616

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (&rq->__lock){-.-.}-{2:2}:
       _raw_spin_lock_nested+0x50/0x6c kernel/locking/spinlock.c:378
       raw_spin_rq_lock_nested+0x2c/0x44 kernel/sched/core.c:558
       raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
       rq_lock kernel/sched/sched.h:1653 [inline]
       task_fork_fair+0x7c/0x23c kernel/sched/fair.c:12095
       sched_cgroup_fork+0x38c/0x464 kernel/sched/core.c:4777
       copy_process+0x24fc/0x3514 kernel/fork.c:2618
       kernel_clone+0x1d8/0x8ac kernel/fork.c:2918
       user_mode_thread+0x110/0x178 kernel/fork.c:2996
       rest_init+0x2c/0x2f4 init/main.c:700
       start_kernel+0x0/0x55c init/main.c:834
       start_kernel+0x3f0/0x55c init/main.c:1088
       __primary_switched+0xb8/0xc0 arch/arm64/kernel/head.S:523

-> #1 (&p->pi_lock){-.-.}-{2:2}:
       __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
       _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
       try_to_wake_up+0xb0/0xd9c kernel/sched/core.c:4191
       default_wake_function+0x4c/0x60 kernel/sched/core.c:6993
       autoremove_wake_function+0x24/0xf8 kernel/sched/wait.c:419
       __wake_up_common+0x23c/0x3bc kernel/sched/wait.c:107
       __wake_up_common_lock kernel/sched/wait.c:138 [inline]
       __wake_up+0x10c/0x1a8 kernel/sched/wait.c:160
       wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
       balance_pgdat+0x1880/0x1c34 mm/vmscan.c:7540
       kswapd+0x7d0/0x10fc mm/vmscan.c:7737
       kthread+0x288/0x310 kernel/kthread.c:379
       ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:870

-> #0 (&pgdat->kcompactd_wait){-...}-{2:2}:
       check_prev_add kernel/locking/lockdep.c:3108 [inline]
       check_prevs_add kernel/locking/lockdep.c:3227 [inline]
       validate_chain kernel/locking/lockdep.c:3842 [inline]
       __lock_acquire+0x3310/0x75f0 kernel/locking/lockdep.c:5074
       lock_acquire+0x23c/0x71c kernel/locking/lockdep.c:5691
       __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
       _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
       __wake_up_common_lock kernel/sched/wait.c:137 [inline]
       __wake_up+0xec/0x1a8 kernel/sched/wait.c:160
       wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
       wakeup_kswapd+0x350/0x8c8 mm/vmscan.c:7791
       wake_all_kswapds+0x13c/0x23c mm/page_alloc.c:4028
       __alloc_pages_slowpath+0x378/0x1edc mm/page_alloc.c:4296
       __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
       alloc_pages+0x4bc/0x7c0
       __stack_depot_save+0x4ac/0x678 lib/stackdepot.c:410
       kasan_save_stack+0x54/0x6c mm/kasan/common.c:46
       __kasan_record_aux_stack+0xcc/0xe8 mm/kasan/generic.c:491
       kasan_record_aux_stack+0x14/0x20 mm/kasan/generic.c:496
       task_work_add+0x94/0x3c0 kernel/task_work.c:48
       task_tick_mm_cid kernel/sched/core.c:11940 [inline]
       scheduler_tick+0x2d0/0x52c kernel/sched/core.c:5626
       update_process_times+0x198/0x1f4 kernel/time/timer.c:2076
       tick_sched_handle kernel/time/tick-sched.c:243 [inline]
       tick_sched_timer+0x330/0x4e8 kernel/time/tick-sched.c:1481
       __run_hrtimer kernel/time/hrtimer.c:1685 [inline]
       __hrtimer_run_queues+0x458/0xca0 kernel/time/hrtimer.c:1749
       hrtimer_interrupt+0x2c0/0xb64 kernel/time/hrtimer.c:1811
       timer_handler drivers/clocksource/arm_arch_timer.c:656 [inline]
       arch_timer_handler_virt+0x74/0x88 drivers/clocksource/arm_arch_timer.c:667
       handle_percpu_devid_irq+0x2a4/0x804 kernel/irq/chip.c:930
       generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
       handle_irq_desc kernel/irq/irqdesc.c:651 [inline]
       generic_handle_domain_irq+0x7c/0xc4 kernel/irq/irqdesc.c:707
       __gic_handle_irq drivers/irqchip/irq-gic-v3.c:728 [inline]
       __gic_handle_irq_from_irqson drivers/irqchip/irq-gic-v3.c:779 [inline]
       gic_handle_irq+0x70/0x1e4 drivers/irqchip/irq-gic-v3.c:823
       call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:899
       do_interrupt_handler+0xd4/0x138 arch/arm64/kernel/entry-common.c:274
       __el1_irq arch/arm64/kernel/entry-common.c:471 [inline]
       el1_interrupt+0x34/0x68 arch/arm64/kernel/entry-common.c:486
       el1h_64_irq_handler+0x18/0x24 arch/arm64/kernel/entry-common.c:491
       el1h_64_irq+0x64/0x68 arch/arm64/kernel/entry.S:587
       __daif_local_irq_enable arch/arm64/include/asm/irqflags.h:33 [inline]
       arch_local_irq_enable arch/arm64/include/asm/irqflags.h:55 [inline]
       __raw_spin_unlock_irq include/linux/spinlock_api_smp.h:159 [inline]
       _raw_spin_unlock_irq+0x34/0x80 kernel/locking/spinlock.c:202
       spin_unlock_irq include/linux/spinlock.h:400 [inline]
       __filemap_add_folio+0x6f8/0x11b4 mm/filemap.c:914
       filemap_add_folio+0x10c/0x298 mm/filemap.c:939
       page_cache_ra_unbounded+0x1c0/0x58c mm/readahead.c:251
       do_page_cache_ra mm/readahead.c:300 [inline]
       page_cache_ra_order+0x7fc/0x994 mm/readahead.c:560
       do_sync_mmap_readahead+0x3a4/0x844
       filemap_fault+0x5bc/0x1154 mm/filemap.c:3279
       __do_fault+0x11c/0x3d8 mm/memory.c:4176
       do_read_fault mm/memory.c:4530 [inline]
       do_fault mm/memory.c:4659 [inline]
       do_pte_missing mm/memory.c:3647 [inline]
       handle_pte_fault mm/memory.c:4947 [inline]
       __handle_mm_fault mm/memory.c:5089 [inline]
       handle_mm_fault+0x32cc/0x48ec mm/memory.c:5243
       __do_page_fault arch/arm64/mm/fault.c:512 [inline]
       do_page_fault+0x81c/0xcbc arch/arm64/mm/fault.c:645
       do_translation_fault+0x94/0xc8 arch/arm64/mm/fault.c:731
       do_mem_abort+0x74/0x200 arch/arm64/mm/fault.c:867
       el0_ia+0x90/0x214 arch/arm64/kernel/entry-common.c:533
       el0t_64_sync_handler+0xb4/0xf0 arch/arm64/kernel/entry-common.c:661
       el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591

other info that might help us debug this:

Chain exists of:
  &pgdat->kcompactd_wait --> &p->pi_lock --> &rq->__lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&rq->__lock);
                               lock(&p->pi_lock);
                               lock(&rq->__lock);
  lock(&pgdat->kcompactd_wait);

 *** DEADLOCK ***

2 locks held by klogd/5578:
 #0: ffff0000c18ae9a0 (mapping.invalidate_lock){++++}-{3:3}, at: filemap_invalidate_lock_shared include/linux/fs.h:830 [inline]
 #0: ffff0000c18ae9a0 (mapping.invalidate_lock){++++}-{3:3}, at: page_cache_ra_unbounded+0xc8/0x58c mm/readahead.c:226
 #1: ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested kernel/sched/core.c:558 [inline]
 #1: ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
 #1: ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: rq_lock kernel/sched/sched.h:1653 [inline]
 #1: ffff0001b425bb18 (&rq->__lock){-.-.}-{2:2}, at: scheduler_tick+0xa4/0x52c kernel/sched/core.c:5616

stack backtrace:
CPU: 1 PID: 5578 Comm: klogd Not tainted 6.4.0-rc2-syzkaller-gf1fcbaa18b28 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023
Call trace:
 dump_backtrace+0x1b8/0x1e4 arch/arm64/kernel/stacktrace.c:233
 show_stack+0x2c/0x44 arch/arm64/kernel/stacktrace.c:240
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xd0/0x124 lib/dump_stack.c:106
 dump_stack+0x1c/0x28 lib/dump_stack.c:113
 print_circular_bug+0x150/0x1b8 kernel/locking/lockdep.c:2066
 check_noncircular+0x2cc/0x378 kernel/locking/lockdep.c:2188
 check_prev_add kernel/locking/lockdep.c:3108 [inline]
 check_prevs_add kernel/locking/lockdep.c:3227 [inline]
 validate_chain kernel/locking/lockdep.c:3842 [inline]
 __lock_acquire+0x3310/0x75f0 kernel/locking/lockdep.c:5074
 lock_acquire+0x23c/0x71c kernel/locking/lockdep.c:5691
 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
 _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
 __wake_up_common_lock kernel/sched/wait.c:137 [inline]
 __wake_up+0xec/0x1a8 kernel/sched/wait.c:160
 wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
 wakeup_kswapd+0x350/0x8c8 mm/vmscan.c:7791
 wake_all_kswapds+0x13c/0x23c mm/page_alloc.c:4028
 __alloc_pages_slowpath+0x378/0x1edc mm/page_alloc.c:4296
 __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
 alloc_pages+0x4bc/0x7c0
 __stack_depot_save+0x4ac/0x678 lib/stackdepot.c:410
 kasan_save_stack+0x54/0x6c mm/kasan/common.c:46
 __kasan_record_aux_stack+0xcc/0xe8 mm/kasan/generic.c:491
 kasan_record_aux_stack+0x14/0x20 mm/kasan/generic.c:496
 task_work_add+0x94/0x3c0 kernel/task_work.c:48
 task_tick_mm_cid kernel/sched/core.c:11940 [inline]
 scheduler_tick+0x2d0/0x52c kernel/sched/core.c:5626
 update_process_times+0x198/0x1f4 kernel/time/timer.c:2076
 tick_sched_handle kernel/time/tick-sched.c:243 [inline]
 tick_sched_timer+0x330/0x4e8 kernel/time/tick-sched.c:1481
 __run_hrtimer kernel/time/hrtimer.c:1685 [inline]
 __hrtimer_run_queues+0x458/0xca0 kernel/time/hrtimer.c:1749
 hrtimer_interrupt+0x2c0/0xb64 kernel/time/hrtimer.c:1811
 timer_handler drivers/clocksource/arm_arch_timer.c:656 [inline]
 arch_timer_handler_virt+0x74/0x88 drivers/clocksource/arm_arch_timer.c:667
 handle_percpu_devid_irq+0x2a4/0x804 kernel/irq/chip.c:930
 generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
 handle_irq_desc kernel/irq/irqdesc.c:651 [inline]
 generic_handle_domain_irq+0x7c/0xc4 kernel/irq/irqdesc.c:707
 __gic_handle_irq drivers/irqchip/irq-gic-v3.c:728 [inline]
 __gic_handle_irq_from_irqson drivers/irqchip/irq-gic-v3.c:779 [inline]
 gic_handle_irq+0x70/0x1e4 drivers/irqchip/irq-gic-v3.c:823
 call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:899
 do_interrupt_handler+0xd4/0x138 arch/arm64/kernel/entry-common.c:274
 __el1_irq arch/arm64/kernel/entry-common.c:471 [inline]
 el1_interrupt+0x34/0x68 arch/arm64/kernel/entry-common.c:486
 el1h_64_irq_handler+0x18/0x24 arch/arm64/kernel/entry-common.c:491
 el1h_64_irq+0x64/0x68 arch/arm64/kernel/entry.S:587
 __daif_local_irq_enable arch/arm64/include/asm/irqflags.h:33 [inline]
 arch_local_irq_enable arch/arm64/include/asm/irqflags.h:55 [inline]
 __raw_spin_unlock_irq include/linux/spinlock_api_smp.h:159 [inline]
 _raw_spin_unlock_irq+0x34/0x80 kernel/locking/spinlock.c:202
 spin_unlock_irq include/linux/spinlock.h:400 [inline]
 __filemap_add_folio+0x6f8/0x11b4 mm/filemap.c:914
 filemap_add_folio+0x10c/0x298 mm/filemap.c:939
 page_cache_ra_unbounded+0x1c0/0x58c mm/readahead.c:251
 do_page_cache_ra mm/readahead.c:300 [inline]
 page_cache_ra_order+0x7fc/0x994 mm/readahead.c:560
 do_sync_mmap_readahead+0x3a4/0x844
 filemap_fault+0x5bc/0x1154 mm/filemap.c:3279
 __do_fault+0x11c/0x3d8 mm/memory.c:4176
 do_read_fault mm/memory.c:4530 [inline]
 do_fault mm/memory.c:4659 [inline]
 do_pte_missing mm/memory.c:3647 [inline]
 handle_pte_fault mm/memory.c:4947 [inline]
 __handle_mm_fault mm/memory.c:5089 [inline]
 handle_mm_fault+0x32cc/0x48ec mm/memory.c:5243
 __do_page_fault arch/arm64/mm/fault.c:512 [inline]
 do_page_fault+0x81c/0xcbc arch/arm64/mm/fault.c:645
 do_translation_fault+0x94/0xc8 arch/arm64/mm/fault.c:731
 do_mem_abort+0x74/0x200 arch/arm64/mm/fault.c:867
 el0_ia+0x90/0x214 arch/arm64/kernel/entry-common.c:533
 el0t_64_sync_handler+0xb4/0xf0 arch/arm64/kernel/entry-common.c:661
 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [kernel?] possible deadlock in scheduler_tick (2)

[Tetsuo Handa]

This looks same pattern with https://lkml.kernel.org/r/6577e1fa-b6ee-f2be-2414-a2b51b1c5e30@I-love.SAKURA.ne.jp .
I think stackdepot needs to drop __GFP_KSWAPD_RECLAIM as well as debugobject did.

Like I wrote at https://lore.kernel.org/linux-mm/d642e597-cf7d-b410-16ce-22dff483fd8e@I-love.SAKURA.ne.jp/ ,
I wish that GFP_ATOMIC / GFP_NOWAIT do not imply __GFP_KSWAPD_RECLAIM...

On 2023/05/20 17:26, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    f1fcbaa18b28 Linux 6.4-rc2
> git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
> console output: https://syzkaller.appspot.com/x/log.txt?x=1332a029280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=3dc1cdd68141cdc3
> dashboard link: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
> userspace arch: arm64
> 
> Unfortunately, I don't have any reproducer for this issue yet.
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/f9e1748cceea/disk-f1fcbaa1.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/6dea99343621/vmlinux-f1fcbaa1.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/f5a93f86012d/Image-f1fcbaa1.gz.xz
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com

[PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

syzbot is reporting lockdep warning in __stack_depot_save(), for
wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
calls wakeup_kcompactd() when __GFP_KSWAPD_RECLAIM is set and
__GFP_DIRECT_RECLAIM is not set (i.e. GFP_ATOMIC).

Since __stack_depot_save() might be called with arbitrary locks held,
__stack_depot_save() should not wake kswapd which in turn wakes kcompactd.

Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: cd11016e5f52 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB")
---
 lib/stackdepot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 2f5aa851834e..5c331a80b87a 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
 		 * contexts and I/O.
 		 */
 		alloc_flags &= ~GFP_ZONEMASK;
-		alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
+		if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
+			alloc_flags &= __GFP_HIGH;
+		else
+			alloc_flags &= GFP_KERNEL;
 		alloc_flags |= __GFP_NOWARN;
 		page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
 		if (page)
-- 
2.18.4

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

On 2023/05/20 20:33, Tetsuo Handa wrote:
> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>  		 * contexts and I/O.
>  		 */
>  		alloc_flags &= ~GFP_ZONEMASK;
> -		alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
> +		if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
> +			alloc_flags &= __GFP_HIGH;
> +		else
> +			alloc_flags &= GFP_KERNEL;
>  		alloc_flags |= __GFP_NOWARN;

Well, comparing with a report which reached __stack_depot_save() via fill_pool()
( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that
above lines might be bogus.

Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because
fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context.
Then, these lines could be simplified like below.

	if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
		alloc_flags = __GFP_HIGH | __GFP_NOWARN;
	else
		alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN;

How is the importance of memory allocation in __stack_depot_save() ?
If allocation failure is welcome, maybe we should not trigger OOM killer
by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ...

>  		page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
>  		if (page)

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

On 2023/05/20 22:14, Tetsuo Handa wrote:
> On 2023/05/20 20:33, Tetsuo Handa wrote:
>> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>>  		 * contexts and I/O.
>>  		 */
>>  		alloc_flags &= ~GFP_ZONEMASK;
>> -		alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
>> +		if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
>> +			alloc_flags &= __GFP_HIGH;
>> +		else
>> +			alloc_flags &= GFP_KERNEL;
>>  		alloc_flags |= __GFP_NOWARN;
> 
> Well, comparing with a report which reached __stack_depot_save() via fill_pool()
> ( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that
> above lines might be bogus.
> 
> Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because
> fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context.
> Then, these lines could be simplified like below.
> 
> 	if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
> 		alloc_flags = __GFP_HIGH | __GFP_NOWARN;
> 	else
> 		alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN;
> 
> How is the importance of memory allocation in __stack_depot_save() ?
> If allocation failure is welcome, maybe we should not trigger OOM killer
> by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ...
> 
>>  		page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
>>  		if (page)
> 

Well, since stackdepot itself simply use GFP flags supplied by kasan,
this should be considered as a kasan's problem?

__kasan_record_aux_stack() {
	alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc); // May deadlock due to including __GFP_KSWAPD_RECLAIM bit.
}

Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
Where do we want to drop this bit (in the caller side, or in the callee side)?

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2023/05/20 22:14, Tetsuo Handa wrote:
>> On 2023/05/20 20:33, Tetsuo Handa wrote:
>>> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>>>  		 * contexts and I/O.
>>>  		 */
>>>  		alloc_flags &= ~GFP_ZONEMASK;
>>> -		alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
>>> +		if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
>>> +			alloc_flags &= __GFP_HIGH;
>>> +		else
>>> +			alloc_flags &= GFP_KERNEL;
>>>  		alloc_flags |= __GFP_NOWARN;
>> 
>> Well, comparing with a report which reached __stack_depot_save() via fill_pool()
>> ( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that
>> above lines might be bogus.
>> 
>> Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because
>> fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context.
>> Then, these lines could be simplified like below.
>> 
>> 	if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
>> 		alloc_flags = __GFP_HIGH | __GFP_NOWARN;
>> 	else
>> 		alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN;
>> 
>> How is the importance of memory allocation in __stack_depot_save() ?
>> If allocation failure is welcome, maybe we should not trigger OOM killer
>> by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ...
>> 
>>>  		page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
>>>  		if (page)
>> 
>
> Well, since stackdepot itself simply use GFP flags supplied by kasan,
> this should be considered as a kasan's problem?
>
> __kasan_record_aux_stack() {
> 	alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc); // May deadlock due to including __GFP_KSWAPD_RECLAIM bit.
> }
>
> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
> Where do we want to drop this bit (in the caller side, or in the callee side)?

Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
(instead of GFP_ATOMIC) for debug code?  The debug code may be called at
almost arbitrary places, and wakeup_kswap() isn't safe to be called in
some situations.

BTW: I still think that it's better to show the circular lock order in
the patch description.  I know the information is in syzkaller report.
It will make reader's life easier if the patch description is
self-contained.

Best Regards,
Huang, Ying

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

On 2023/05/22 11:13, Huang, Ying wrote:
>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
>> Where do we want to drop this bit (in the caller side, or in the callee side)?
> 
> Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
> (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
> almost arbitrary places, and wakeup_kswap() isn't safe to be called in
> some situations.

What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?
Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
reduces latency of atomic allocations (which is important when called from "atomic" context).
I consider that memory allocations which do not do direct reclaim should be geared towards
less locking dependency.

In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages.
It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM
right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd
upon GFP_ATOMIC or GFP_NOWAIT allocations.

If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add
__GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or
GFP_NOWAIT is not recommended from the beginning...

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2023/05/22 11:13, Huang, Ying wrote:
>>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
>>> Where do we want to drop this bit (in the caller side, or in the callee side)?
>> 
>> Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
>> (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
>> almost arbitrary places, and wakeup_kswap() isn't safe to be called in
>> some situations.
>
> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?
> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
> reduces latency of atomic allocations (which is important when called from "atomic" context).
> I consider that memory allocations which do not do direct reclaim should be geared towards
> less locking dependency.

Except debug code, where do you find locking issues for waking up kswapd?

> In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages.
> It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM
> right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd
> upon GFP_ATOMIC or GFP_NOWAIT allocations.
>
> If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add
> __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or
> GFP_NOWAIT is not recommended from the beginning...

From performance perspective, it's better to wake up kswapd as early as
possible.  Because it can reduce the possibility of the direct
reclaiming, which may case very long latency.

Best Regards,
Huang, Ying

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

On 2023/05/22 12:07, Huang, Ying wrote:
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> 
>> On 2023/05/22 11:13, Huang, Ying wrote:
>>>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
>>>> Where do we want to drop this bit (in the caller side, or in the callee side)?
>>>
>>> Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
>>> (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
>>> almost arbitrary places, and wakeup_kswap() isn't safe to be called in
>>> some situations.
>>
>> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?
>> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
>> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
>> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
>> reduces latency of atomic allocations (which is important when called from "atomic" context).
>> I consider that memory allocations which do not do direct reclaim should be geared towards
>> less locking dependency.
> 
> Except debug code, where do you find locking issues for waking up kswapd?

I'm not aware of lockdep reports except debug code.

But due to too many locking dependency, lockdep gives up tracking all dependency (e.g.

  https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2
  https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f
  https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7
  https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9

). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock
and zonelist_update_seq are candidates which need not to be held from interrupt context.

> 
>> In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages.
>> It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM
>> right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd
>> upon GFP_ATOMIC or GFP_NOWAIT allocations.
>>
>> If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add
>> __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or
>> GFP_NOWAIT is not recommended from the beginning...
> 
>>From performance perspective, it's better to wake up kswapd as early as
> possible.  Because it can reduce the possibility of the direct
> reclaiming, which may case very long latency.

My expectation is that a __GFP_DIRECT_RECLAIM allocation request which happened
after a !__GFP_KSWAPD_RECLAIM allocation request wakes kswapd before future
__GFP_DIRECT_RECLAIM allocation requests have to perform the direct reclaiming.

> 
> Best Regards,
> Huang, Ying
> 

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2023/05/22 12:07, Huang, Ying wrote:
>> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
>> 
>>> On 2023/05/22 11:13, Huang, Ying wrote:
>>>>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
>>>>> Where do we want to drop this bit (in the caller side, or in the callee side)?
>>>>
>>>> Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
>>>> (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
>>>> almost arbitrary places, and wakeup_kswap() isn't safe to be called in
>>>> some situations.
>>>
>>> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?
>>> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
>>> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
>>> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
>>> reduces latency of atomic allocations (which is important when called from "atomic" context).
>>> I consider that memory allocations which do not do direct reclaim should be geared towards
>>> less locking dependency.
>> 
>> Except debug code, where do you find locking issues for waking up kswapd?
>
> I'm not aware of lockdep reports except debug code.
>
> But due to too many locking dependency, lockdep gives up tracking all dependency (e.g.
>
>   https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2
>   https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f
>   https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7
>   https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9
>
> ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock
> and zonelist_update_seq are candidates which need not to be held from interrupt context.

Why is it not safe to wake up kswapd/kcompactd from interrupt context?

Best Regards,
Huang, Ying

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Tetsuo Handa]

On 2023/05/23 9:07, Huang, Ying wrote:
>>> Except debug code, where do you find locking issues for waking up kswapd?
>>
>> I'm not aware of lockdep reports except debug code.
>>
>> But due to too many locking dependency, lockdep gives up tracking all dependency (e.g.
>>
>>   https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2
>>   https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f
>>   https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7
>>   https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9
>>
>> ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock
>> and zonelist_update_seq are candidates which need not to be held from interrupt context.
> 
> Why is it not safe to wake up kswapd/kcompactd from interrupt context?

I'm not saying it is not safe to wake up kswapd/kcompactd from interrupt context.
Please notice that I'm using "need not" than "must not".

Since total amount of RAM a Linux kernel can use had been increased over years,
watermark gap between "kswapd should start background reclaim" and "current thread
must start foreground reclaim" also increased. Then, randomly allocating small
amount of pages from interrupt context (or atomic context) without waking up
will not needlessly increase possibility of reaching "current thread must start
foreground reclaim" watermark. Then, reducing locking dependency by not waking up
becomes a gain.





KASAN developers, I'm waiting for your response on

  How is the importance of memory allocation in __stack_depot_save() ?
  If allocation failure is welcome, maybe we should not trigger OOM killer
  by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ...

part.

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2023/05/23 9:07, Huang, Ying wrote:
>>>> Except debug code, where do you find locking issues for waking up kswapd?
>>>
>>> I'm not aware of lockdep reports except debug code.
>>>
>>> But due to too many locking dependency, lockdep gives up tracking all dependency (e.g.
>>>
>>>   https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2
>>>   https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f
>>>   https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7
>>>   https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9
>>>
>>> ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock
>>> and zonelist_update_seq are candidates which need not to be held from interrupt context.
>> 
>> Why is it not safe to wake up kswapd/kcompactd from interrupt context?
>
> I'm not saying it is not safe to wake up kswapd/kcompactd from interrupt context.
> Please notice that I'm using "need not" than "must not".

Got it.

> Since total amount of RAM a Linux kernel can use had been increased over years,
> watermark gap between "kswapd should start background reclaim" and "current thread
> must start foreground reclaim" also increased. Then, randomly allocating small
> amount of pages from interrupt context (or atomic context) without waking up
> will not needlessly increase possibility of reaching "current thread must start
> foreground reclaim" watermark. Then, reducing locking dependency by not waking up
> becomes a gain.

Personally, I prefer to wake up kswapd ASAP.  And fix the deadlock if
possible.

Best Regards,
Huang, Ying

Re: [PATCH] lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context

[Michal Hocko]

On Mon 22-05-23 11:47:25, Tetsuo Handa wrote:
> On 2023/05/22 11:13, Huang, Ying wrote:
> >> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit.
> >> Where do we want to drop this bit (in the caller side, or in the callee side)?
> > 
> > Yes.  I think we should fix the KASAN.  Maybe define a new GFP_XXX
> > (instead of GFP_ATOMIC) for debug code?  The debug code may be called at
> > almost arbitrary places, and wakeup_kswap() isn't safe to be called in
> > some situations.
> 
> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT?

Not a good idea IMO. It is really hard to achieve real locklessness in the
page allocator. If we ever need something like that it should be pretty
obviously requested by a dedicated gfp flag rather than overriding a
long term established semantic. While GFP_ATOMIC is a bit of a misnomer
it has many users who really only require non-sleeping behavior.

> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe
> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks.
> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and
> reduces latency of atomic allocations (which is important when called from "atomic" context).

I would really like to see any numbers to believe this is the case
actually. Waking up kswapd should be pretty non-visible.

> I consider that memory allocations which do not do direct reclaim should be geared towards
> less locking dependency.
> 
> In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages.

This hugely depend on the workload. I do not think we can make any
generic statements like that.

> It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM
> right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd
> upon GFP_ATOMIC or GFP_NOWAIT allocations.

The thing is that you do not know this is a the case. You might have a
IRQ heavy prossing making a lot of memory allocations (e.g. networking)
while the rest of the processing doesn't require any additional memory.
 
> If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add
> __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or
> GFP_NOWAIT is not recommended from the beginning...

As much as I do not really like the long term GFP_ATOMIC semantic I do
not think we should be changing it to what you are proposing for reasons
mentioned above. GFP_NOWAIT change is even more questionable. Many users
simply use GFP_NOWAIT as a way of an optimistic allocation with a more
expensinsive fallback. We do not want to allow those consumers to
consume watermark gap memory to force others to hit the direct reclaim
wall.

Really there is very likely only a handfull of users who cannot even
wake kswapd or perform other non-sleeping locking and those should
currently drop __GFP_KSWAPD_RECLAIM. Maybe we should consider an alias
for them to not bother with the low level flag. Maybe we will need
GFP_LOCKLESS or something similar.
-- 
Michal Hocko
SUSE Labs

[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan

[Tetsuo Handa]

syzbot is reporting lockdep warning in __stack_depot_save(), for
the caller of __stack_depot_save() (i.e. __kasan_record_aux_stack() in
this report) is responsible for masking __GFP_KSWAPD_RECLAIM flag in
order not to wake kswapd which in turn wakes kcompactd.

Since kasan/kmsan functions might be called with arbitrary locks held,
mask __GFP_KSWAPD_RECLAIM flag from all GFP_NOWAIT/GFP_ATOMIC allocations
in kasan/kmsan.

Note that kmsan_save_stack_with_flags() is changed to mask both
__GFP_DIRECT_RECLAIM flag and __GFP_KSWAPD_RECLAIM flag, for
wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
calls wakeup_kcompactd() if __GFP_KSWAPD_RECLAIM flag is set and
__GFP_DIRECT_RECLAIM flag is not set.

Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/kasan/generic.c         | 4 ++--
 mm/kasan/tags.c            | 2 +-
 mm/kmsan/core.c            | 6 +++---
 mm/kmsan/instrumentation.c | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index e5eef670735e..2c94f4943240 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -488,7 +488,7 @@ static void __kasan_record_aux_stack(void *addr, bool can_alloc)
 		return;
 
 	alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0];
-	alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc);
+	alloc_meta->aux_stack[0] = kasan_save_stack(0, can_alloc);
 }
 
 void kasan_record_aux_stack(void *addr)
@@ -518,7 +518,7 @@ void kasan_save_free_info(struct kmem_cache *cache, void *object)
 	if (!free_meta)
 		return;
 
-	kasan_set_track(&free_meta->free_track, GFP_NOWAIT);
+	kasan_set_track(&free_meta->free_track, 0);
 	/* The object was freed and has free track set. */
 	*(u8 *)kasan_mem_to_shadow(object) = KASAN_SLAB_FREETRACK;
 }
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 67a222586846..7dcfe341d48e 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -140,5 +140,5 @@ void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags)
 
 void kasan_save_free_info(struct kmem_cache *cache, void *object)
 {
-	save_stack_info(cache, object, GFP_NOWAIT, true);
+	save_stack_info(cache, object, 0, true);
 }
diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
index 7d1e4aa30bae..3adb4c1d3b19 100644
--- a/mm/kmsan/core.c
+++ b/mm/kmsan/core.c
@@ -74,7 +74,7 @@ depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
 	nr_entries = stack_trace_save(entries, KMSAN_STACK_DEPTH, 0);
 
 	/* Don't sleep. */
-	flags &= ~__GFP_DIRECT_RECLAIM;
+	flags &= ~(__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM);
 
 	handle = __stack_depot_save(entries, nr_entries, flags, true);
 	return stack_depot_set_extra_bits(handle, extra);
@@ -245,7 +245,7 @@ depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
 	extra_bits = kmsan_extra_bits(depth, uaf);
 
 	entries[0] = KMSAN_CHAIN_MAGIC_ORIGIN;
-	entries[1] = kmsan_save_stack_with_flags(GFP_ATOMIC, 0);
+	entries[1] = kmsan_save_stack_with_flags(__GFP_HIGH, 0);
 	entries[2] = id;
 	/*
 	 * @entries is a local var in non-instrumented code, so KMSAN does not
@@ -253,7 +253,7 @@ depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
 	 * positives when __stack_depot_save() passes it to instrumented code.
 	 */
 	kmsan_internal_unpoison_memory(entries, sizeof(entries), false);
-	handle = __stack_depot_save(entries, ARRAY_SIZE(entries), GFP_ATOMIC,
+	handle = __stack_depot_save(entries, ARRAY_SIZE(entries), __GFP_HIGH,
 				    true);
 	return stack_depot_set_extra_bits(handle, extra_bits);
 }
diff --git a/mm/kmsan/instrumentation.c b/mm/kmsan/instrumentation.c
index cf12e9616b24..cc3907a9c33a 100644
--- a/mm/kmsan/instrumentation.c
+++ b/mm/kmsan/instrumentation.c
@@ -282,7 +282,7 @@ void __msan_poison_alloca(void *address, uintptr_t size, char *descr)
 
 	/* stack_depot_save() may allocate memory. */
 	kmsan_enter_runtime();
-	handle = stack_depot_save(entries, ARRAY_SIZE(entries), GFP_ATOMIC);
+	handle = stack_depot_save(entries, ARRAY_SIZE(entries), __GFP_HIGH);
 	kmsan_leave_runtime();
 
 	kmsan_internal_set_shadow_origin(address, size, -1, handle,
-- 
2.34.1

Re: [syzbot] [ntfs3?] possible deadlock in scheduler_tick (2)

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit:    eb0f1697d729 Merge branch 'for-next/core', remote-tracking..
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci
console output: https://syzkaller.appspot.com/x/log.txt?x=17733b4d280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=8860074b9a9d6c45
dashboard link: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
compiler:       Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm64
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15b4e536280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=10224d6d280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/034232da7cff/disk-eb0f1697.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/b11411bec33e/vmlinux-eb0f1697.xz
kernel image: https://storage.googleapis.com/syzbot-assets/a53c52e170dd/Image-eb0f1697.gz.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/2ce9b06770e0/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com

======================================================
WARNING: possible circular locking dependency detected
6.4.0-rc3-syzkaller-geb0f1697d729 #0 Not tainted
------------------------------------------------------
klogd/5578 is trying to acquire lock:
ffff0001fea76c40 (&pgdat->kcompactd_wait){-...}-{2:2}, at: __wake_up_common_lock kernel/sched/wait.c:137 [inline]
ffff0001fea76c40 (&pgdat->kcompactd_wait){-...}-{2:2}, at: __wake_up+0xec/0x1a8 kernel/sched/wait.c:160

but task is already holding lock:
ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested kernel/sched/core.c:558 [inline]
ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: rq_lock kernel/sched/sched.h:1653 [inline]
ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: scheduler_tick+0xa4/0x52c kernel/sched/core.c:5616

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (&rq->__lock){-.-.}-{2:2}:
       _raw_spin_lock_nested+0x50/0x6c kernel/locking/spinlock.c:378
       raw_spin_rq_lock_nested+0x2c/0x44 kernel/sched/core.c:558
       raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
       rq_lock kernel/sched/sched.h:1653 [inline]
       task_fork_fair+0x7c/0x23c kernel/sched/fair.c:12095
       sched_cgroup_fork+0x38c/0x464 kernel/sched/core.c:4777
       copy_process+0x24fc/0x3514 kernel/fork.c:2618
       kernel_clone+0x1d8/0x8ac kernel/fork.c:2918
       user_mode_thread+0x110/0x178 kernel/fork.c:2996
       rest_init+0x2c/0x2f4 init/main.c:700
       start_kernel+0x0/0x55c init/main.c:834
       start_kernel+0x3f0/0x55c init/main.c:1088
       __primary_switched+0xb8/0xc0 arch/arm64/kernel/head.S:523

-> #1 (&p->pi_lock){-.-.}-{2:2}:
       __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
       _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
       try_to_wake_up+0xb0/0xd9c kernel/sched/core.c:4191
       default_wake_function+0x4c/0x60 kernel/sched/core.c:6993
       autoremove_wake_function+0x24/0xf8 kernel/sched/wait.c:419
       __wake_up_common+0x23c/0x3bc kernel/sched/wait.c:107
       __wake_up_common_lock kernel/sched/wait.c:138 [inline]
       __wake_up+0x10c/0x1a8 kernel/sched/wait.c:160
       wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
       balance_pgdat+0x1880/0x1c34 mm/vmscan.c:7541
       kswapd+0x7d0/0x10fc mm/vmscan.c:7738
       kthread+0x288/0x310 kernel/kthread.c:379
       ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:853

-> #0 (&pgdat->kcompactd_wait){-...}-{2:2}:
       check_prev_add kernel/locking/lockdep.c:3108 [inline]
       check_prevs_add kernel/locking/lockdep.c:3227 [inline]
       validate_chain kernel/locking/lockdep.c:3842 [inline]
       __lock_acquire+0x3310/0x75f0 kernel/locking/lockdep.c:5074
       lock_acquire+0x23c/0x71c kernel/locking/lockdep.c:5691
       __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
       _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
       __wake_up_common_lock kernel/sched/wait.c:137 [inline]
       __wake_up+0xec/0x1a8 kernel/sched/wait.c:160
       wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
       wakeup_kswapd+0x350/0x8c8 mm/vmscan.c:7792
       wake_all_kswapds+0x13c/0x23c mm/page_alloc.c:4028
       __alloc_pages_slowpath+0x378/0x1edc mm/page_alloc.c:4296
       __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
       alloc_pages+0x4bc/0x7c0
       __stack_depot_save+0x4ac/0x678 lib/stackdepot.c:410
       kasan_save_stack+0x54/0x6c mm/kasan/common.c:46
       __kasan_record_aux_stack+0xcc/0xe8 mm/kasan/generic.c:491
       kasan_record_aux_stack+0x14/0x20 mm/kasan/generic.c:496
       task_work_add+0x94/0x3c0 kernel/task_work.c:48
       task_tick_mm_cid kernel/sched/core.c:11940 [inline]
       scheduler_tick+0x2d0/0x52c kernel/sched/core.c:5626
       update_process_times+0x198/0x1f4 kernel/time/timer.c:2076
       tick_sched_handle kernel/time/tick-sched.c:243 [inline]
       tick_sched_timer+0x330/0x4e8 kernel/time/tick-sched.c:1481
       __run_hrtimer kernel/time/hrtimer.c:1685 [inline]
       __hrtimer_run_queues+0x458/0xca0 kernel/time/hrtimer.c:1749
       hrtimer_interrupt+0x2c0/0xb64 kernel/time/hrtimer.c:1811
       timer_handler drivers/clocksource/arm_arch_timer.c:656 [inline]
       arch_timer_handler_virt+0x74/0x88 drivers/clocksource/arm_arch_timer.c:667
       handle_percpu_devid_irq+0x2a4/0x804 kernel/irq/chip.c:930
       generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
       handle_irq_desc kernel/irq/irqdesc.c:651 [inline]
       generic_handle_domain_irq+0x7c/0xc4 kernel/irq/irqdesc.c:707
       __gic_handle_irq drivers/irqchip/irq-gic-v3.c:728 [inline]
       __gic_handle_irq_from_irqson drivers/irqchip/irq-gic-v3.c:779 [inline]
       gic_handle_irq+0x70/0x1e4 drivers/irqchip/irq-gic-v3.c:823
       call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:882
       do_interrupt_handler+0xd4/0x138 arch/arm64/kernel/entry-common.c:274
       __el1_irq arch/arm64/kernel/entry-common.c:471 [inline]
       el1_interrupt+0x34/0x68 arch/arm64/kernel/entry-common.c:486
       el1h_64_irq_handler+0x18/0x24 arch/arm64/kernel/entry-common.c:491
       el1h_64_irq+0x64/0x68 arch/arm64/kernel/entry.S:587
       __daif_local_irq_restore arch/arm64/include/asm/irqflags.h:182 [inline]
       arch_local_irq_restore arch/arm64/include/asm/irqflags.h:202 [inline]
       dump_stack_lvl+0x104/0x124 lib/dump_stack.c:107
       dump_stack+0x1c/0x28 lib/dump_stack.c:113
       dump_header+0xb4/0x954 mm/oom_kill.c:460
       oom_kill_process+0x10c/0x6ec mm/oom_kill.c:1036
       out_of_memory+0xe24/0x103c mm/oom_kill.c:1156
       __alloc_pages_may_oom mm/page_alloc.c:3669 [inline]
       __alloc_pages_slowpath+0x1714/0x1edc mm/page_alloc.c:4444
       __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
       alloc_pages+0x4bc/0x7c0
       alloc_slab_page+0xa0/0x164 mm/slub.c:1851
       allocate_slab mm/slub.c:2006 [inline]
       new_slab+0x210/0x2f4 mm/slub.c:2051
       ___slab_alloc+0x80c/0xdf4 mm/slub.c:3192
       __slab_alloc mm/slub.c:3291 [inline]
       __slab_alloc_node mm/slub.c:3344 [inline]
       slab_alloc_node mm/slub.c:3441 [inline]
       kmem_cache_alloc_node+0x318/0x46c mm/slub.c:3496
       __alloc_skb+0x19c/0x3d8 net/core/skbuff.c:644
       alloc_skb include/linux/skbuff.h:1288 [inline]
       alloc_skb_with_frags+0xb4/0x590 net/core/skbuff.c:6378
       sock_alloc_send_pskb+0x76c/0x884 net/core/sock.c:2729
       unix_dgram_sendmsg+0x480/0x16c0 net/unix/af_unix.c:1944
       sock_sendmsg_nosec net/socket.c:724 [inline]
       sock_sendmsg net/socket.c:747 [inline]
       __sys_sendto+0x3b4/0x538 net/socket.c:2144
       __do_sys_sendto net/socket.c:2156 [inline]
       __se_sys_sendto net/socket.c:2152 [inline]
       __arm64_sys_sendto+0xd8/0xf8 net/socket.c:2152
       __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline]
       invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52
       el0_svc_common+0x138/0x258 arch/arm64/kernel/syscall.c:142
       do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:193
       el0_svc+0x4c/0x15c arch/arm64/kernel/entry-common.c:637
       el0t_64_sync_handler+0x84/0xf0 arch/arm64/kernel/entry-common.c:655
       el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591

other info that might help us debug this:

Chain exists of:
  &pgdat->kcompactd_wait --> &p->pi_lock --> &rq->__lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&rq->__lock);
                               lock(&p->pi_lock);
                               lock(&rq->__lock);
  lock(&pgdat->kcompactd_wait);

 *** DEADLOCK ***

2 locks held by klogd/5578:
 #0: ffff8000161245e8 (oom_lock){+.+.}-{3:3}, at: __alloc_pages_may_oom mm/page_alloc.c:3618 [inline]
 #0: ffff8000161245e8 (oom_lock){+.+.}-{3:3}, at: __alloc_pages_slowpath+0x1694/0x1edc mm/page_alloc.c:4444
 #1: ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock_nested kernel/sched/core.c:558 [inline]
 #1: ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: raw_spin_rq_lock kernel/sched/sched.h:1366 [inline]
 #1: ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: rq_lock kernel/sched/sched.h:1653 [inline]
 #1: ffff0001b4259b18 (&rq->__lock){-.-.}-{2:2}, at: scheduler_tick+0xa4/0x52c kernel/sched/core.c:5616

stack backtrace:
CPU: 1 PID: 5578 Comm: klogd Not tainted 6.4.0-rc3-syzkaller-geb0f1697d729 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
Call trace:
 dump_backtrace+0x1b8/0x1e4 arch/arm64/kernel/stacktrace.c:233
 show_stack+0x2c/0x44 arch/arm64/kernel/stacktrace.c:240
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xd0/0x124 lib/dump_stack.c:106
 dump_stack+0x1c/0x28 lib/dump_stack.c:113
 print_circular_bug+0x150/0x1b8 kernel/locking/lockdep.c:2066
 check_noncircular+0x2cc/0x378 kernel/locking/lockdep.c:2188
 check_prev_add kernel/locking/lockdep.c:3108 [inline]
 check_prevs_add kernel/locking/lockdep.c:3227 [inline]
 validate_chain kernel/locking/lockdep.c:3842 [inline]
 __lock_acquire+0x3310/0x75f0 kernel/locking/lockdep.c:5074
 lock_acquire+0x23c/0x71c kernel/locking/lockdep.c:5691
 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
 _raw_spin_lock_irqsave+0x5c/0x7c kernel/locking/spinlock.c:162
 __wake_up_common_lock kernel/sched/wait.c:137 [inline]
 __wake_up+0xec/0x1a8 kernel/sched/wait.c:160
 wakeup_kcompactd+0x254/0x310 mm/compaction.c:2942
 wakeup_kswapd+0x350/0x8c8 mm/vmscan.c:7792
 wake_all_kswapds+0x13c/0x23c mm/page_alloc.c:4028
 __alloc_pages_slowpath+0x378/0x1edc mm/page_alloc.c:4296
 __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
 alloc_pages+0x4bc/0x7c0
 __stack_depot_save+0x4ac/0x678 lib/stackdepot.c:410
 kasan_save_stack+0x54/0x6c mm/kasan/common.c:46
 __kasan_record_aux_stack+0xcc/0xe8 mm/kasan/generic.c:491
 kasan_record_aux_stack+0x14/0x20 mm/kasan/generic.c:496
 task_work_add+0x94/0x3c0 kernel/task_work.c:48
 task_tick_mm_cid kernel/sched/core.c:11940 [inline]
 scheduler_tick+0x2d0/0x52c kernel/sched/core.c:5626
 update_process_times+0x198/0x1f4 kernel/time/timer.c:2076
 tick_sched_handle kernel/time/tick-sched.c:243 [inline]
 tick_sched_timer+0x330/0x4e8 kernel/time/tick-sched.c:1481
 __run_hrtimer kernel/time/hrtimer.c:1685 [inline]
 __hrtimer_run_queues+0x458/0xca0 kernel/time/hrtimer.c:1749
 hrtimer_interrupt+0x2c0/0xb64 kernel/time/hrtimer.c:1811
 timer_handler drivers/clocksource/arm_arch_timer.c:656 [inline]
 arch_timer_handler_virt+0x74/0x88 drivers/clocksource/arm_arch_timer.c:667
 handle_percpu_devid_irq+0x2a4/0x804 kernel/irq/chip.c:930
 generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
 handle_irq_desc kernel/irq/irqdesc.c:651 [inline]
 generic_handle_domain_irq+0x7c/0xc4 kernel/irq/irqdesc.c:707
 __gic_handle_irq drivers/irqchip/irq-gic-v3.c:728 [inline]
 __gic_handle_irq_from_irqson drivers/irqchip/irq-gic-v3.c:779 [inline]
 gic_handle_irq+0x70/0x1e4 drivers/irqchip/irq-gic-v3.c:823
 call_on_irq_stack+0x24/0x4c arch/arm64/kernel/entry.S:882
 do_interrupt_handler+0xd4/0x138 arch/arm64/kernel/entry-common.c:274
 __el1_irq arch/arm64/kernel/entry-common.c:471 [inline]
 el1_interrupt+0x34/0x68 arch/arm64/kernel/entry-common.c:486
 el1h_64_irq_handler+0x18/0x24 arch/arm64/kernel/entry-common.c:491
 el1h_64_irq+0x64/0x68 arch/arm64/kernel/entry.S:587
 __daif_local_irq_restore arch/arm64/include/asm/irqflags.h:182 [inline]
 arch_local_irq_restore arch/arm64/include/asm/irqflags.h:202 [inline]
 dump_stack_lvl+0x104/0x124 lib/dump_stack.c:107
 dump_stack+0x1c/0x28 lib/dump_stack.c:113
 dump_header+0xb4/0x954 mm/oom_kill.c:460
 oom_kill_process+0x10c/0x6ec mm/oom_kill.c:1036
 out_of_memory+0xe24/0x103c mm/oom_kill.c:1156
 __alloc_pages_may_oom mm/page_alloc.c:3669 [inline]
 __alloc_pages_slowpath+0x1714/0x1edc mm/page_alloc.c:4444
 __alloc_pages+0x3bc/0x698 mm/page_alloc.c:4781
 alloc_pages+0x4bc/0x7c0
 alloc_slab_page+0xa0/0x164 mm/slub.c:1851
 allocate_slab mm/slub.c:2006 [inline]
 new_slab+0x210/0x2f4 mm/slub.c:2051
 ___slab_alloc+0x80c/0xdf4 mm/slub.c:3192
 __slab_alloc mm/slub.c:3291 [inline]
 __slab_alloc_node mm/slub.c:3344 [inline]
 slab_alloc_node mm/slub.c:3441 [inline]
 kmem_cache_alloc_node+0x318/0x46c mm/slub.c:3496
 __alloc_skb+0x19c/0x3d8 net/core/skbuff.c:644
 alloc_skb include/linux/skbuff.h:1288 [inline]
 alloc_skb_with_frags+0xb4/0x590 net/core/skbuff.c:6378
 sock_alloc_send_pskb+0x76c/0x884 net/core/sock.c:2729
 unix_dgram_sendmsg+0x480/0x16c0 net/unix/af_unix.c:1944
 sock_sendmsg_nosec net/socket.c:724 [inline]
 sock_sendmsg net/socket.c:747 [inline]
 __sys_sendto+0x3b4/0x538 net/socket.c:2144
 __do_sys_sendto net/socket.c:2156 [inline]
 __se_sys_sendto net/socket.c:2152 [inline]
 __arm64_sys_sendto+0xd8/0xf8 net/socket.c:2152
 __invoke_syscall arch/arm64/kernel/syscall.c:38 [inline]
 invoke_syscall+0x98/0x2c0 arch/arm64/kernel/syscall.c:52
 el0_svc_common+0x138/0x258 arch/arm64/kernel/syscall.c:142
 do_el0_svc+0x64/0x198 arch/arm64/kernel/syscall.c:193
 el0_svc+0x4c/0x15c arch/arm64/kernel/entry-common.c:637
 el0t_64_sync_handler+0x84/0xf0 arch/arm64/kernel/entry-common.c:655
 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591


---
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

Re: [PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> syzbot is reporting lockdep warning in __stack_depot_save(), for
> the caller of __stack_depot_save() (i.e. __kasan_record_aux_stack() in
> this report) is responsible for masking __GFP_KSWAPD_RECLAIM flag in
> order not to wake kswapd which in turn wakes kcompactd.
>
> Since kasan/kmsan functions might be called with arbitrary locks held,
> mask __GFP_KSWAPD_RECLAIM flag from all GFP_NOWAIT/GFP_ATOMIC allocations
> in kasan/kmsan.
>
> Note that kmsan_save_stack_with_flags() is changed to mask both
> __GFP_DIRECT_RECLAIM flag and __GFP_KSWAPD_RECLAIM flag, for
> wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
> calls wakeup_kcompactd() if __GFP_KSWAPD_RECLAIM flag is set and
> __GFP_DIRECT_RECLAIM flag is not set.
>
> Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

This looks good to me.  Thanks!

Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

> ---
>  mm/kasan/generic.c         | 4 ++--
>  mm/kasan/tags.c            | 2 +-
>  mm/kmsan/core.c            | 6 +++---
>  mm/kmsan/instrumentation.c | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index e5eef670735e..2c94f4943240 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -488,7 +488,7 @@ static void __kasan_record_aux_stack(void *addr, bool can_alloc)
>  		return;
>  
>  	alloc_meta->aux_stack[1] = alloc_meta->aux_stack[0];
> -	alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc);
> +	alloc_meta->aux_stack[0] = kasan_save_stack(0, can_alloc);
>  }
>  
>  void kasan_record_aux_stack(void *addr)
> @@ -518,7 +518,7 @@ void kasan_save_free_info(struct kmem_cache *cache, void *object)
>  	if (!free_meta)
>  		return;
>  
> -	kasan_set_track(&free_meta->free_track, GFP_NOWAIT);
> +	kasan_set_track(&free_meta->free_track, 0);
>  	/* The object was freed and has free track set. */
>  	*(u8 *)kasan_mem_to_shadow(object) = KASAN_SLAB_FREETRACK;
>  }
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index 67a222586846..7dcfe341d48e 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -140,5 +140,5 @@ void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags)
>  
>  void kasan_save_free_info(struct kmem_cache *cache, void *object)
>  {
> -	save_stack_info(cache, object, GFP_NOWAIT, true);
> +	save_stack_info(cache, object, 0, true);
>  }
> diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
> index 7d1e4aa30bae..3adb4c1d3b19 100644
> --- a/mm/kmsan/core.c
> +++ b/mm/kmsan/core.c
> @@ -74,7 +74,7 @@ depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
>  	nr_entries = stack_trace_save(entries, KMSAN_STACK_DEPTH, 0);
>  
>  	/* Don't sleep. */
> -	flags &= ~__GFP_DIRECT_RECLAIM;
> +	flags &= ~(__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM);
>  
>  	handle = __stack_depot_save(entries, nr_entries, flags, true);
>  	return stack_depot_set_extra_bits(handle, extra);
> @@ -245,7 +245,7 @@ depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
>  	extra_bits = kmsan_extra_bits(depth, uaf);
>  
>  	entries[0] = KMSAN_CHAIN_MAGIC_ORIGIN;
> -	entries[1] = kmsan_save_stack_with_flags(GFP_ATOMIC, 0);
> +	entries[1] = kmsan_save_stack_with_flags(__GFP_HIGH, 0);
>  	entries[2] = id;
>  	/*
>  	 * @entries is a local var in non-instrumented code, so KMSAN does not
> @@ -253,7 +253,7 @@ depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
>  	 * positives when __stack_depot_save() passes it to instrumented code.
>  	 */
>  	kmsan_internal_unpoison_memory(entries, sizeof(entries), false);
> -	handle = __stack_depot_save(entries, ARRAY_SIZE(entries), GFP_ATOMIC,
> +	handle = __stack_depot_save(entries, ARRAY_SIZE(entries), __GFP_HIGH,
>  				    true);
>  	return stack_depot_set_extra_bits(handle, extra_bits);
>  }
> diff --git a/mm/kmsan/instrumentation.c b/mm/kmsan/instrumentation.c
> index cf12e9616b24..cc3907a9c33a 100644
> --- a/mm/kmsan/instrumentation.c
> +++ b/mm/kmsan/instrumentation.c
> @@ -282,7 +282,7 @@ void __msan_poison_alloca(void *address, uintptr_t size, char *descr)
>  
>  	/* stack_depot_save() may allocate memory. */
>  	kmsan_enter_runtime();
> -	handle = stack_depot_save(entries, ARRAY_SIZE(entries), GFP_ATOMIC);
> +	handle = stack_depot_save(entries, ARRAY_SIZE(entries), __GFP_HIGH);
>  	kmsan_leave_runtime();
>  
>  	kmsan_internal_set_shadow_origin(address, size, -1, handle,

Re: [PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan

[Alexander Potapenko]

On Mon, May 29, 2023 at 3:08 AM Huang, Ying <ying.huang@intel.com> wrote:
>
> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
>
> > syzbot is reporting lockdep warning in __stack_depot_save(), for
> > the caller of __stack_depot_save() (i.e. __kasan_record_aux_stack() in
> > this report) is responsible for masking __GFP_KSWAPD_RECLAIM flag in
> > order not to wake kswapd which in turn wakes kcompactd.
> >
> > Since kasan/kmsan functions might be called with arbitrary locks held,
> > mask __GFP_KSWAPD_RECLAIM flag from all GFP_NOWAIT/GFP_ATOMIC allocations
> > in kasan/kmsan.
> >
> > Note that kmsan_save_stack_with_flags() is changed to mask both
> > __GFP_DIRECT_RECLAIM flag and __GFP_KSWAPD_RECLAIM flag, for
> > wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
> > calls wakeup_kcompactd() if __GFP_KSWAPD_RECLAIM flag is set and
> > __GFP_DIRECT_RECLAIM flag is not set.
> >
> > Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> > Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> This looks good to me.  Thanks!
>
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

Sorry for the late reply, but maybe it would be better to mask this
flag in __stack_depot_save() (lib/stackdepot.c) instead?
We are already masking out a number of flags there, and the problem
seems quite generic.

Re: [PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan

[Andrew Morton]

On Wed, 31 May 2023 15:31:53 +0200 Alexander Potapenko <glider@google.com> wrote:

> On Mon, May 29, 2023 at 3:08 AM Huang, Ying <ying.huang@intel.com> wrote:
> >
> > ? Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> >
> > > syzbot is reporting lockdep warning in __stack_depot_save(), for
> > > the caller of __stack_depot_save() (i.e. __kasan_record_aux_stack() in
> > > this report) is responsible for masking __GFP_KSWAPD_RECLAIM flag in
> > > order not to wake kswapd which in turn wakes kcompactd.
> > >
> > > Since kasan/kmsan functions might be called with arbitrary locks held,
> > > mask __GFP_KSWAPD_RECLAIM flag from all GFP_NOWAIT/GFP_ATOMIC allocations
> > > in kasan/kmsan.
> > >
> > > Note that kmsan_save_stack_with_flags() is changed to mask both
> > > __GFP_DIRECT_RECLAIM flag and __GFP_KSWAPD_RECLAIM flag, for
> > > wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
> > > calls wakeup_kcompactd() if __GFP_KSWAPD_RECLAIM flag is set and
> > > __GFP_DIRECT_RECLAIM flag is not set.
> > >
> > > Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> > > Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >
> > This looks good to me.  Thanks!
> >
> > Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
> 
> Sorry for the late reply, but maybe it would be better to mask this
> flag in __stack_depot_save() (lib/stackdepot.c) instead?
> We are already masking out a number of flags there, and the problem
> seems quite generic.


Tetsuo?

Re: [PATCH v3] lib/stackdepot: fix gfp flags manipulation in __stack_depot_save()

[Huang, Ying]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> syzbot is reporting lockdep warning in __stack_depot_save(), for
> __kasan_record_aux_stack() is passing GFP_NOWAIT which will result in
> calling wakeup_kcompactd() from wakeup_kswapd() from wake_all_kswapds()
>  from __alloc_pages_slowpath().
>
> Strictly speaking, __kasan_record_aux_stack() is responsible for removing
> __GFP_KSWAPD_RECLAIM flag in order not to wake kswapd which in turn wakes
> kcompactd. But since KASAN and KMSAN functions might be called with
> arbitrary locks held, we should consider removing __GFP_KSWAPD_RECLAIM
> flag from KASAN and KMSAN. And this patch goes one step further; let's
> remove __GFP_KSWAPD_RECLAIM flag in the __stack_depot_save() side, based
> on the following reasons.
>
> Reason 1:
>
>   Currently, __stack_depot_save() has "alloc_flags &= ~GFP_ZONEMASK;" line
>   which is pointless because "alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);"
>   line will also zero out zone modifiers. But why is __stack_depot_save()
>   trying to mask gfp flags supplied by the caller?
>
>   I guess that __stack_depot_save() tried to be as robust as possible. But
>   __stack_depot_save() is a debugging function where all callers have to
>   be able to survive allocation failures. Scattering low-level gfp flags
>   like 0 or __GFP_HIGH should be avoided in order to replace GFP_NOWAIT or
>   GFP_ATOMIC.
>
> Reason 2:
>
>   __stack_depot_save() from stack_depot_save() is also called by
>   ref_tracker_alloc() from __netns_tracker_alloc() from
>   netns_tracker_alloc() from get_net_track(), and some of get_net_track()
>   users are passing GFP_ATOMIC because waking kswapd/kcompactd is safe.
>   But even if we mask __GFP_KSWAPD_RECLAIM flag at __stack_depot_save(),
>   it is very likely that allocations with __GFP_KSWAPD_RECLAIM flag happen
>   somewhere else by the moment __stack_depot_save() is called for the next
>   time.
>
>   Therefore, not waking kswapd/kcompactd when doing allocation for
>   __stack_depot_save() will be acceptable from the memory reclaim latency
>   perspective.

TBH, I don't like to remove __GFP_KSWAPD_RECLAIM flag unnecessarily.
But this is only my personal opinion.

> While we are at it, let's make __stack_depot_save() accept __GFP_NORETRY
> and __GFP_RETRY_MAYFAIL flags, based on the following reason.
>
> Reason 3:
>
>   Since DEPOT_POOL_ORDER is defined as 2, we must mask __GFP_NOFAIL flag
>   in order not to complain rmqueue(). But masking __GFP_NORETRY flag and
>   __GFP_RETRY_MAYFAIL flag might be overkill.
>
>   The OOM killer might be needlessly invoked due to order-2 allocation if
>   GFP_KERNEL is supplied by the caller, despite the caller might have
>   passed GFP_KERNEL for doing order-0 allocation.
>
>   Allocation for order-2 might stall if GFP_NOFS or GFP_NOIO is supplied
>   by the caller, despite the caller might have passed GFP_NOFS or GFP_NOIO
>   for doing order-0 allocation.
>
>   Generally speaking, I feel that doing order-2 allocation from
>   __stack_depot_save() with gfp flags supplied by the caller is an
>   unexpected behavior for the callers. We might want to use only order-0
>   allocation, and/or stop using gfp flags supplied by the caller...

Per my understanding, this isn't locking issue reported by syzbot?  If
so, I suggest to put this in a separate patch.

> Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> Suggested-by: Alexander Potapenko <glider@google.com>
> Cc: Huang, Ying <ying.huang@intel.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Changes in v3:
>   Huang, Ying thinks that masking __GFP_KSWAPD_RECLAIM flag in the callers
>   side is preferable
>   ( https://lkml.kernel.org/r/87fs7nyhs3.fsf@yhuang6-desk2.ccr.corp.intel.com ).
>   But Alexander Potapenko thinks that masking __GFP_KSWAPD_RECLAIM flag
>   in the callee side would be the better
>   ( https://lkml.kernel.org/r/CAG_fn=UTTbkGeOX0teGcNOeobtgV=mfGOefZpV-NTN4Ouus7xA@mail.gmail.com ).
>   I took Alexander's suggestion, and added reasoning for masking
>   __GFP_KSWAPD_RECLAIM flag in the callee side.
>
> Changes in v2:
>   Mask __GFP_KSWAPD_RECLAIM flag in the callers, suggested by Huang, Ying
>   ( https://lkml.kernel.org/r/87edn92jvz.fsf@yhuang6-desk2.ccr.corp.intel.com ).
>
>  lib/stackdepot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 2f5aa851834e..33ebefaa7074 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>  		 * contexts and I/O.
>  		 */
>  		alloc_flags &= ~GFP_ZONEMASK;
> -		alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
> +		if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
> +			alloc_flags &= __GFP_HIGH;

Why not just

                        alloc_flags &= ~__GFP_KSWAPD_RECLAIM;

?

> +		else
> +			alloc_flags &= ~__GFP_NOFAIL;
>  		alloc_flags |= __GFP_NOWARN;
>  		page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
>  		if (page)

Best Regards,
Huang, Ying

Re: [PATCH v3] lib/stackdepot: fix gfp flags manipulation in __stack_depot_save()

[Alexander Potapenko]

On Sat, Jun 10, 2023 at 1:40 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> syzbot is reporting lockdep warning in __stack_depot_save(), for
> __kasan_record_aux_stack() is passing GFP_NOWAIT which will result in
> calling wakeup_kcompactd() from wakeup_kswapd() from wake_all_kswapds()
>  from __alloc_pages_slowpath().
>
> Strictly speaking, __kasan_record_aux_stack() is responsible for removing
> __GFP_KSWAPD_RECLAIM flag in order not to wake kswapd which in turn wakes
> kcompactd. But since KASAN and KMSAN functions might be called with
> arbitrary locks held, we should consider removing __GFP_KSWAPD_RECLAIM
> flag from KASAN and KMSAN. And this patch goes one step further; let's
> remove __GFP_KSWAPD_RECLAIM flag in the __stack_depot_save() side, based
> on the following reasons.
>
> Reason 1:
>
>   Currently, __stack_depot_save() has "alloc_flags &= ~GFP_ZONEMASK;" line
>   which is pointless because "alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);"
>   line will also zero out zone modifiers.

Good catch, we indeed do not need the GFP_ZONEMASK line now.
But looks like you'll need it at least in the __GFP_NOFAIL branch?

> But why is __stack_depot_save()
>   trying to mask gfp flags supplied by the caller?
>
>   I guess that __stack_depot_save() tried to be as robust as possible. But
>   __stack_depot_save() is a debugging function where all callers have to
>   be able to survive allocation failures.

This, but also the allocation should not deadlock.
E.g. KMSAN can call __stack_depot_save() from almost any function in
the kernel, so we'd better avoid heavyweight memory reclaiming,
because that in turn may call __stack_depot_save() again.

>
> Reason 2:
>
>   __stack_depot_save() from stack_depot_save() is also called by
>   ref_tracker_alloc() from __netns_tracker_alloc() from
>   netns_tracker_alloc() from get_net_track(), and some of get_net_track()
>   users are passing GFP_ATOMIC because waking kswapd/kcompactd is safe.
>   But even if we mask __GFP_KSWAPD_RECLAIM flag at __stack_depot_save(),
>   it is very likely that allocations with __GFP_KSWAPD_RECLAIM flag happen
>   somewhere else by the moment __stack_depot_save() is called for the next
>   time.
>
>   Therefore, not waking kswapd/kcompactd when doing allocation for
>   __stack_depot_save() will be acceptable from the memory reclaim latency
>   perspective.

Ack.

> While we are at it, let's make __stack_depot_save() accept __GFP_NORETRY
> and __GFP_RETRY_MAYFAIL flags, based on the following reason.

Looks like you're accepting a whole bunch of flags in addition to
__GFP_NORETRY and __GFP_RETRY_MAYFAIL - maybe list the two explicitly?

> Reason 3:
>
>   Since DEPOT_POOL_ORDER is defined as 2, we must mask __GFP_NOFAIL flag
>   in order not to complain rmqueue(). But masking __GFP_NORETRY flag and
>   __GFP_RETRY_MAYFAIL flag might be overkill.
>
>   The OOM killer might be needlessly invoked due to order-2 allocation if
>   GFP_KERNEL is supplied by the caller, despite the caller might have
>   passed GFP_KERNEL for doing order-0 allocation.

As you noted above, stackdepot is a debug feature anyway, so invoking
OOM killer because there is no memory for an order-2 allocation might
be an acceptable behavior?

>   Allocation for order-2 might stall if GFP_NOFS or GFP_NOIO is supplied
>   by the caller, despite the caller might have passed GFP_NOFS or GFP_NOIO
>   for doing order-0 allocation.

What if the caller passed GFP_NOFS to avoid calling back into FS, and
discarding that flag would result in a recursion?
Same for GFP_NOIO.

>   Generally speaking, I feel that doing order-2 allocation from
>   __stack_depot_save() with gfp flags supplied by the caller is an
>   unexpected behavior for the callers. We might want to use only order-0
>   allocation, and/or stop using gfp flags supplied by the caller...

Right now stackdepot allows the following list of flags: __GFP_HIGH,
__GFP_KSWAPD_RECLAIM, __GFP_DIRECT_RECLAIM, __GFP_IO, __GFP_FS.
We could restrict it further to __GFP_HIGH | __GFP_DIRECT_RECLAIM to
be on the safe side - plus allow __GFP_NORETRY and
__GFP_RETRY_MAYFAIL.



> Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> Suggested-by: Alexander Potapenko <glider@google.com>
> Cc: Huang, Ying <ying.huang@intel.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Changes in v3:
>   Huang, Ying thinks that masking __GFP_KSWAPD_RECLAIM flag in the callers
>   side is preferable
>   ( https://lkml.kernel.org/r/87fs7nyhs3.fsf@yhuang6-desk2.ccr.corp.intel.com ).
>   But Alexander Potapenko thinks that masking __GFP_KSWAPD_RECLAIM flag
>   in the callee side would be the better
>   ( https://lkml.kernel.org/r/CAG_fn=UTTbkGeOX0teGcNOeobtgV=mfGOefZpV-NTN4Ouus7xA@mail.gmail.com ).
>   I took Alexander's suggestion, and added reasoning for masking
>   __GFP_KSWAPD_RECLAIM flag in the callee side.
>
> Changes in v2:
>   Mask __GFP_KSWAPD_RECLAIM flag in the callers, suggested by Huang, Ying
>   ( https://lkml.kernel.org/r/87edn92jvz.fsf@yhuang6-desk2.ccr.corp.intel.com ).
>
>  lib/stackdepot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 2f5aa851834e..33ebefaa7074 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries,
>                  * contexts and I/O.
>                  */
>                 alloc_flags &= ~GFP_ZONEMASK;
> -               alloc_flags &= (GFP_ATOMIC | GFP_KERNEL);
> +               if (!(alloc_flags & __GFP_DIRECT_RECLAIM))
> +                       alloc_flags &= __GFP_HIGH;
> +               else
> +                       alloc_flags &= ~__GFP_NOFAIL;
>                 alloc_flags |= __GFP_NOWARN;
>                 page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER);
>                 if (page)
> --
> 2.18.4
>
>

Re: [PATCH v3] lib/stackdepot: fix gfp flags manipulation in __stack_depot_save()

[Tetsuo Handa]

On 2023/06/21 21:56, Alexander Potapenko wrote:
>> But why is __stack_depot_save()
>>   trying to mask gfp flags supplied by the caller?
>>
>>   I guess that __stack_depot_save() tried to be as robust as possible. But
>>   __stack_depot_save() is a debugging function where all callers have to
>>   be able to survive allocation failures.
> 
> This, but also the allocation should not deadlock.
> E.g. KMSAN can call __stack_depot_save() from almost any function in
> the kernel, so we'd better avoid heavyweight memory reclaiming,
> because that in turn may call __stack_depot_save() again.

Then, isn't "[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from
kasan/kmsan" the better fix?



>>   Allocation for order-2 might stall if GFP_NOFS or GFP_NOIO is supplied
>>   by the caller, despite the caller might have passed GFP_NOFS or GFP_NOIO
>>   for doing order-0 allocation.
> 
> What if the caller passed GFP_NOFS to avoid calling back into FS, and
> discarding that flag would result in a recursion?
> Same for GFP_NOIO.

Excuse me, but "alloc_flags &= ~__GFP_NOFAIL;" will not discard flags in
GFP_NOFS / GFP_NOIO ?



>>   Generally speaking, I feel that doing order-2 allocation from
>>   __stack_depot_save() with gfp flags supplied by the caller is an
>>   unexpected behavior for the callers. We might want to use only order-0
>>   allocation, and/or stop using gfp flags supplied by the caller...
> 
> Right now stackdepot allows the following list of flags: __GFP_HIGH,
> __GFP_KSWAPD_RECLAIM, __GFP_DIRECT_RECLAIM, __GFP_IO, __GFP_FS.
> We could restrict it further to __GFP_HIGH | __GFP_DIRECT_RECLAIM to
> be on the safe side - plus allow __GFP_NORETRY and
> __GFP_RETRY_MAYFAIL.

I feel that making such change is killing more than needed; there is
no need to discard __GFP_KSWAPD_RECLAIM when GFP_KERNEL is given.

"[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan"
looks the better.

Re: [PATCH v3] lib/stackdepot: fix gfp flags manipulation in __stack_depot_save()

[Alexander Potapenko]

On Wed, Jun 21, 2023 at 4:07 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2023/06/21 21:56, Alexander Potapenko wrote:
> >> But why is __stack_depot_save()
> >>   trying to mask gfp flags supplied by the caller?
> >>
> >>   I guess that __stack_depot_save() tried to be as robust as possible. But
> >>   __stack_depot_save() is a debugging function where all callers have to
> >>   be able to survive allocation failures.
> >
> > This, but also the allocation should not deadlock.
> > E.g. KMSAN can call __stack_depot_save() from almost any function in
> > the kernel, so we'd better avoid heavyweight memory reclaiming,
> > because that in turn may call __stack_depot_save() again.
>
> Then, isn't "[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from
> kasan/kmsan" the better fix?

Perhaps you are right and I shouldn't have insisted on pushing this
flag down to stackdepot.
If other users (e.g. page_owner) can afford invoking kswapd, then we
are good to go, and new compiler-based tools can use the same flags
KASAN and KMSAN do.


>
> >>   Allocation for order-2 might stall if GFP_NOFS or GFP_NOIO is supplied
> >>   by the caller, despite the caller might have passed GFP_NOFS or GFP_NOIO
> >>   for doing order-0 allocation.
> >
> > What if the caller passed GFP_NOFS to avoid calling back into FS, and
> > discarding that flag would result in a recursion?
> > Same for GFP_NOIO.
>
> Excuse me, but "alloc_flags &= ~__GFP_NOFAIL;" will not discard flags in
> GFP_NOFS / GFP_NOIO ?

But not for the other if-clause?
Anyway, I actually confused GFP_NOIO (which is technically
__GFP_RECLAIM) and GFP_NOFS with __GFP_IO/__GFP_FS, thinking that
there's a separate pair of GFP flags opposite to __GFP_IO and
__GFP_FS.
Please disregard.

>
>
> >>   Generally speaking, I feel that doing order-2 allocation from
> >>   __stack_depot_save() with gfp flags supplied by the caller is an
> >>   unexpected behavior for the callers. We might want to use only order-0
> >>   allocation, and/or stop using gfp flags supplied by the caller...
> >
> > Right now stackdepot allows the following list of flags: __GFP_HIGH,
> > __GFP_KSWAPD_RECLAIM, __GFP_DIRECT_RECLAIM, __GFP_IO, __GFP_FS.
> > We could restrict it further to __GFP_HIGH | __GFP_DIRECT_RECLAIM to
> > be on the safe side - plus allow __GFP_NORETRY and
> > __GFP_RETRY_MAYFAIL.
>
> I feel that making such change is killing more than needed; there is
> no need to discard __GFP_KSWAPD_RECLAIM when GFP_KERNEL is given.
>
> "[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan"
> looks the better.
>

I agree, let's go for it.
Sorry for the trouble.

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH v3] lib/stackdepot: fix gfp flags manipulation in __stack_depot_save()

[Tetsuo Handa]

On 2023/06/21 23:42, Alexander Potapenko wrote:
>> "[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan"
>> looks the better.
>>
> 
> I agree, let's go for it.
> Sorry for the trouble.
> 

No problem. :-)

Andrew, please take "[PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan"
at https://lkml.kernel.org/r/656cb4f5-998b-c8d7-3c61-c2d37aa90f9a@I-love.SAKURA.ne.jp
with "Reviewed-by: "Huang, Ying" <ying.huang@intel.com>" added.

Re: [PATCH] kasan,kmsan: remove __GFP_KSWAPD_RECLAIM usage from kasan/kmsan

[Alexander Potapenko]

On Sat, Jun 10, 2023 at 12:31 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 31 May 2023 15:31:53 +0200 Alexander Potapenko <glider@google.com> wrote:
>
> > On Mon, May 29, 2023 at 3:08 AM Huang, Ying <ying.huang@intel.com> wrote:
> > >
> > > ? Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
> > >
> > > > syzbot is reporting lockdep warning in __stack_depot_save(), for
> > > > the caller of __stack_depot_save() (i.e. __kasan_record_aux_stack() in
> > > > this report) is responsible for masking __GFP_KSWAPD_RECLAIM flag in
> > > > order not to wake kswapd which in turn wakes kcompactd.
> > > >
> > > > Since kasan/kmsan functions might be called with arbitrary locks held,
> > > > mask __GFP_KSWAPD_RECLAIM flag from all GFP_NOWAIT/GFP_ATOMIC allocations
> > > > in kasan/kmsan.
> > > >
> > > > Note that kmsan_save_stack_with_flags() is changed to mask both
> > > > __GFP_DIRECT_RECLAIM flag and __GFP_KSWAPD_RECLAIM flag, for
> > > > wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath()
> > > > calls wakeup_kcompactd() if __GFP_KSWAPD_RECLAIM flag is set and
> > > > __GFP_DIRECT_RECLAIM flag is not set.
> > > >
> > > > Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com>
> > > > Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1
> > > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > >
> > > This looks good to me.  Thanks!
> > >
> > > Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
> >
> > Sorry for the late reply, but maybe it would be better to mask this
> > flag in __stack_depot_save() (lib/stackdepot.c) instead?
> > We are already masking out a number of flags there, and the problem
> > seems quite generic.
>
>
> Tetsuo?

Reviewed-by: Alexander Potapenko <glider@google.com>

Andrew, please accept this patch. As noted in the other thread, no
changes to stackdepot are needed.

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg