Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> writes: > 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. 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. Scattering low-level gfp flags > like 0 or __GFP_HIGH should be avoided in order to replace GFP_NOWAIT or > GFP_ATOMIC. > > 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. TBH, I don't like to remove __GFP_KSWAPD_RECLAIM flag unnecessarily. But this is only my personal opinion. > While we are at it, let's make __stack_depot_save() accept __GFP_NORETRY > and __GFP_RETRY_MAYFAIL flags, based on the following reason. > > 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. > > 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. > > 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... Per my understanding, this isn't locking issue reported by syzbot? If so, I suggest to put this in a separate patch. > Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@xxxxxxxxxxxxxxxxxxxxxxxxx> > Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1 > Suggested-by: Alexander Potapenko <glider@xxxxxxxxxx> > Cc: Huang, Ying <ying.huang@xxxxxxxxx> > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ). > 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@xxxxxxxxxxxxxx ). > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ). > > 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; Why not just alloc_flags &= ~__GFP_KSWAPD_RECLAIM; ? > + else > + alloc_flags &= ~__GFP_NOFAIL; > alloc_flags |= __GFP_NOWARN; > page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER); > if (page) Best Regards, Huang, Ying
