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

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

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