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

[Alexander Potapenko <glider@google.com>]

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