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)?
