linux-kernel - Re: [PATCH] Revert "kmemleak: allow to coexist with fault injection"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1563304877.4610.10.camel@lca.pw>
Date:   Tue, 16 Jul 2019 15:21:17 -0400
From:   Qian Cai <cai@....pw>
To:     Yang Shi <yang.shi@...ux.alibaba.com>, catalin.marinas@....com,
        mhocko@...e.com, dvyukov@...gle.com, rientjes@...gle.com,
        willy@...radead.org, akpm@...ux-foundation.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "kmemleak: allow to coexist with fault injection"

On Tue, 2019-07-16 at 12:01 -0700, Yang Shi wrote:
> 
> On 7/16/19 11:23 AM, Qian Cai wrote:
> > On Wed, 2019-07-17 at 01:50 +0800, Yang Shi wrote:
> > > When running ltp's oom test with kmemleak enabled, the below warning was
> > > triggerred since kernel detects __GFP_NOFAIL & ~__GFP_DIRECT_RECLAIM is
> > > passed in:
> > > 
> > > WARNING: CPU: 105 PID: 2138 at mm/page_alloc.c:4608
> > > __alloc_pages_nodemask+0x1c31/0x1d50
> > > Modules linked in: loop dax_pmem dax_pmem_core ip_tables x_tables xfs
> > > virtio_net net_failover virtio_blk failover ata_generic virtio_pci
> > > virtio_ring
> > > virtio libata
> > > CPU: 105 PID: 2138 Comm: oom01 Not tainted 5.2.0-next-20190710+ #7
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-
> > > g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > > RIP: 0010:__alloc_pages_nodemask+0x1c31/0x1d50
> > > ...
> > >   kmemleak_alloc+0x4e/0xb0
> > >   kmem_cache_alloc+0x2a7/0x3e0
> > >   ? __kmalloc+0x1d6/0x470
> > >   ? ___might_sleep+0x9c/0x170
> > >   ? mempool_alloc+0x2b0/0x2b0
> > >   mempool_alloc_slab+0x2d/0x40
> > >   mempool_alloc+0x118/0x2b0
> > >   ? __kasan_check_read+0x11/0x20
> > >   ? mempool_resize+0x390/0x390
> > >   ? lock_downgrade+0x3c0/0x3c0
> > >   bio_alloc_bioset+0x19d/0x350
> > >   ? __swap_duplicate+0x161/0x240
> > >   ? bvec_alloc+0x1b0/0x1b0
> > >   ? do_raw_spin_unlock+0xa8/0x140
> > >   ? _raw_spin_unlock+0x27/0x40
> > >   get_swap_bio+0x80/0x230
> > >   ? __x64_sys_madvise+0x50/0x50
> > >   ? end_swap_bio_read+0x310/0x310
> > >   ? __kasan_check_read+0x11/0x20
> > >   ? check_chain_key+0x24e/0x300
> > >   ? bdev_write_page+0x55/0x130
> > >   __swap_writepage+0x5ff/0xb20
> > > 
> > > The mempool_alloc_slab() clears __GFP_DIRECT_RECLAIM, however kmemleak has
> > > __GFP_NOFAIL set all the time due to commit
> > > d9570ee3bd1d4f20ce63485f5ef05663866fe6c0 ("kmemleak: allow to coexist
> > > with fault injection").  But, it doesn't make any sense to have
> > > __GFP_NOFAIL and ~__GFP_DIRECT_RECLAIM specified at the same time.
> > > 
> > > According to the discussion on the mailing list, the commit should be
> > > reverted for short term solution.  Catalin Marinas would follow up with a
> > > better
> > > solution for longer term.
> > > 
> > > The failure rate of kmemleak metadata allocation may increase in some
> > > circumstances, but this should be expected side effect.
> > 
> > As mentioned in anther thread, the situation for kmemleak under memory
> > pressure
> > has already been unhealthy. I don't feel comfortable to make it even worse
> > by
> > reverting this commit alone. This could potentially make kmemleak kill
> > itself
> > easier and miss some more real memory leak later.
> > 
> > To make it really a short-term solution before the reverting, I think
> > someone
> > needs to follow up with the mempool solution with tunable pool size
> > mentioned
> > in,
> > 
> > https://lore.kernel.org/linux-mm/20190328145917.GC10283@arrakis.emea.arm.com
> > /
> > 
> > I personally not very confident that Catalin will find some time soon to
> > implement embedding kmemleak metadata into the slab. Even he or someone does
> > eventually, it probably need quite some time to test and edge out many of
> > corner
> > cases that kmemleak could have by its natural.
> 
> Thanks for sharing some background. I didn't notice this topic had been 
> discussed. I'm not sure if this revert would make things worse since I'm 
> supposed real memory leak would be detected sooner before oom kicks in, 
> and kmemleak is already broken with __GFP_NOFAIL.

Well, people could inject some memory pressure at the middle of a test run. OOM
does not necessarily mean kmemleak would always be disabled, as it sometimes
could survive if the memory is recovering fast enough.

Thanks to this commit, there are allocation with __GFP_DIRECT_RECLAIM that
succeeded would keep trying with __GFP_NOFAIL for kmemleak tracking object
allocations. Otherwise, one kmemleak object allocation failure would kill the
whole kmemleak.

> 
> It seems everyone agree __GFP_NPFAIL should be removed? Anyway, I would 
> like leave the decision to Catalin.
> 
> > 
> > > Suggested-by: Catalin Marinas <catalin.marinas@....com>
> > > Cc: Michal Hocko <mhocko@...e.com>
> > > Cc: Dmitry Vyukov <dvyukov@...gle.com>
> > > Cc: David Rientjes <rientjes@...gle.com>
> > > Cc: Matthew Wilcox <willy@...radead.org>
> > > Cc: Qian Cai <cai@....pw>
> > > Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> > > ---
> > >   mm/kmemleak.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > > index 9dd581d..884a5e3 100644
> > > --- a/mm/kmemleak.c
> > > +++ b/mm/kmemleak.c
> > > @@ -114,7 +114,7 @@
> > >   /* GFP bitmask for kmemleak internal allocations */
> > >   #define gfp_kmemleak_mask(gfp)	(((gfp) & (GFP_KERNEL |
> > > GFP_ATOMIC)) |
> > > \
> > >   				 __GFP_NORETRY | __GFP_NOMEMALLOC | \
> > > -				 __GFP_NOWARN | __GFP_NOFAIL)
> > > +				 __GFP_NOWARN)
> > >   
> > >   /* scanning area inside a memory block */
> > >   struct kmemleak_scan_area {
> 
>