Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> 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
