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

["Huang, Ying" <ying.huang@intel.com>]

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