[RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

__GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
But it's a wrong combination. As __GFP_NOFAIL is blockable, but
__GFP_NORETY is not blockable, make it self-contradiction.

__GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
it's not the real intention, as kmemleak allow alloc failure happen in
memory pressure, in that case kmemleak just disables itself.

commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
slowpath") documented that what user wants here should use GFP_NOWAIT, and
the WARN in __alloc_pages_slowpath caught this weird usage.

 <snip>
 WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
 CPU: 3 PID: 64 Comm: kswapd1 Not tainted 4.17.0-rc1.syzcaller+ #12
 Hardware name: Red Hat KVM, BIOS 0.0.0 02/06/2015
 RIP: 0010:__alloc_pages_slowpath+0x1cc3/0x2780
 RSP: 0000:ffff88002fa5e6c8 EFLAGS: 00010046
 RAX: 0000000000000000 RBX: 0000000000010000 RCX: 1ffff10005f4bcb6
 RDX: 1ffff10007da3f46 RSI: 0000000000000000 RDI: ffff88003ed1fa38
 RBP: 0000000001000000 R08: 0000000000000040 R09: 0000000000000030
 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
 kmemleak: Kernel memory leak detector disabled
 R13: ffff88002fa5ea68 R14: dffffc0000000000 R15: ffff88002fa5ea68
 FS:  0000000000000000(0000) GS:ffff88003e900000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f5db91c6640 CR3: 0000000004014000 CR4: 00000000001406e0
 Call Trace:
  __alloc_pages_nodemask+0x5ce/0x7c0
  alloc_pages_current+0xb6/0x230
  new_slab+0x29d/0x9f0
  ___slab_alloc+0x4e5/0xa90
  __slab_alloc.isra.37+0x92/0x120
  kmem_cache_alloc+0x35f/0x580
  create_object+0xa6/0xaf0
  kmem_cache_alloc+0x20a/0x580
  mempool_alloc+0x13a/0x350
  bio_alloc_bioset+0x3ef/0x6e0
  get_swap_bio+0x125/0x490
  __swap_writepage+0x7be/0x11f0
  swap_writepage+0x46/0xb0
  pageout.isra.33+0x435/0xe70
  ? trace_event_raw_event_mm_shrink_slab_start+0x4d0/0x4d0
  ? page_mapped+0x165/0x3f0
  shrink_page_list+0x1ded/0x3960
  shrink_inactive_list+0x737/0x14b0
  shrink_node_memcg+0xa9f/0x1ef0
  shrink_node+0x376/0x15f0
  balance_pgdat+0x2c9/0x970
  kswapd+0x537/0xfe0
  kthread+0x387/0x510
  <snip>

Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
is no blockable and no reclaim, making kmemleak less disruptive to user
processes in pressure.

Signed-off-by: Chunyu Hu <chuhu@redhat.com>
CC: Michal Hocko <mhocko@suse.com>
---
 mm/kmemleak.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 9a085d5..4ea07e4 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -126,7 +126,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 | GFP_NOWAIT)
 
 /* scanning area inside a memory block */
 struct kmemleak_scan_area {
-- 
1.8.3.1

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Catalin Marinas]

On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
> __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
> But it's a wrong combination. As __GFP_NOFAIL is blockable, but
> __GFP_NORETY is not blockable, make it self-contradiction.
> 
> __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
> it's not the real intention, as kmemleak allow alloc failure happen in
> memory pressure, in that case kmemleak just disables itself.

Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
("kmemleak: allow to coexist with fault injection") to keep kmemleak
usable under fault injection.

> commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
> slowpath") documented that what user wants here should use GFP_NOWAIT, and
> the WARN in __alloc_pages_slowpath caught this weird usage.
> 
>  <snip>
>  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
[...]
> Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
> and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
> is no blockable and no reclaim, making kmemleak less disruptive to user
> processes in pressure.

It doesn't solve the fault injection problem for kmemleak (unless we
change __should_failslab() somehow, not sure yet). An option would be to
replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
is enabled.

BTW, does the combination of NOWAIT and NORETRY make kmemleak
allocations more likely to fail?

Cc'ing Dmitry as well.

> Signed-off-by: Chunyu Hu <chuhu@redhat.com>
> CC: Michal Hocko <mhocko@suse.com>
> ---
>  mm/kmemleak.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 9a085d5..4ea07e4 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -126,7 +126,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 | GFP_NOWAIT)
>  
>  /* scanning area inside a memory block */
>  struct kmemleak_scan_area {
> -- 
> 1.8.3.1

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Dmitry Vyukov]

On Fri, Apr 20, 2018 at 7:50 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
>> __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
>> But it's a wrong combination. As __GFP_NOFAIL is blockable, but
>> __GFP_NORETY is not blockable, make it self-contradiction.
>>
>> __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
>> it's not the real intention, as kmemleak allow alloc failure happen in
>> memory pressure, in that case kmemleak just disables itself.
>
> Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
> ("kmemleak: allow to coexist with fault injection") to keep kmemleak
> usable under fault injection.
>
>> commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
>> slowpath") documented that what user wants here should use GFP_NOWAIT, and
>> the WARN in __alloc_pages_slowpath caught this weird usage.
>>
>>  <snip>
>>  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
> [...]
>> Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
>> and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
>> is no blockable and no reclaim, making kmemleak less disruptive to user
>> processes in pressure.
>
> It doesn't solve the fault injection problem for kmemleak (unless we
> change __should_failslab() somehow, not sure yet). An option would be to
> replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
> is enabled.
>
> BTW, does the combination of NOWAIT and NORETRY make kmemleak
> allocations more likely to fail?
>
> Cc'ing Dmitry as well.

Yes, it would be bad if there allocations fail due to fault injection.
These are both debugging tools and ideally should not interfere.

>> Signed-off-by: Chunyu Hu <chuhu@redhat.com>
>> CC: Michal Hocko <mhocko@suse.com>
>> ---
>>  mm/kmemleak.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 9a085d5..4ea07e4 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -126,7 +126,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 | GFP_NOWAIT)
>>
>>  /* scanning area inside a memory block */
>>  struct kmemleak_scan_area {
>> --
>> 1.8.3.1

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Michal Hocko]

On Fri 20-04-18 18:50:24, Catalin Marinas wrote:
> On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
> > __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
> > But it's a wrong combination. As __GFP_NOFAIL is blockable, but
> > __GFP_NORETY is not blockable, make it self-contradiction.
> > 
> > __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
> > it's not the real intention, as kmemleak allow alloc failure happen in
> > memory pressure, in that case kmemleak just disables itself.
> 
> Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
> ("kmemleak: allow to coexist with fault injection") to keep kmemleak
> usable under fault injection.
> 
> > commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
> > slowpath") documented that what user wants here should use GFP_NOWAIT, and
> > the WARN in __alloc_pages_slowpath caught this weird usage.
> > 
> >  <snip>
> >  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
> [...]
> > Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
> > and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
> > is no blockable and no reclaim, making kmemleak less disruptive to user
> > processes in pressure.
> 
> It doesn't solve the fault injection problem for kmemleak (unless we
> change __should_failslab() somehow, not sure yet). An option would be to
> replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
> is enabled.

Cannot we simply have a disable_fault_injection knob around the
allocation rather than playing this dirty tricks with gfp flags which do
not make any sense?

> BTW, does the combination of NOWAIT and NORETRY make kmemleak
> allocations more likely to fail?

NOWAIT + NORETRY simply doesn't make much sesne. It is equivalent to
NOWAIT.

-- 
Michal Hocko
SUSE Labs

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Dmitry Vyukov]

On Sun, Apr 22, 2018 at 2:51 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 20-04-18 18:50:24, Catalin Marinas wrote:
>> On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
>> > __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
>> > But it's a wrong combination. As __GFP_NOFAIL is blockable, but
>> > __GFP_NORETY is not blockable, make it self-contradiction.
>> >
>> > __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
>> > it's not the real intention, as kmemleak allow alloc failure happen in
>> > memory pressure, in that case kmemleak just disables itself.
>>
>> Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
>> ("kmemleak: allow to coexist with fault injection") to keep kmemleak
>> usable under fault injection.
>>
>> > commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
>> > slowpath") documented that what user wants here should use GFP_NOWAIT, and
>> > the WARN in __alloc_pages_slowpath caught this weird usage.
>> >
>> >  <snip>
>> >  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
>> [...]
>> > Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
>> > and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
>> > is no blockable and no reclaim, making kmemleak less disruptive to user
>> > processes in pressure.
>>
>> It doesn't solve the fault injection problem for kmemleak (unless we
>> change __should_failslab() somehow, not sure yet). An option would be to
>> replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
>> is enabled.
>
> Cannot we simply have a disable_fault_injection knob around the
> allocation rather than playing this dirty tricks with gfp flags which do
> not make any sense?
>
>> BTW, does the combination of NOWAIT and NORETRY make kmemleak
>> allocations more likely to fail?
>
> NOWAIT + NORETRY simply doesn't make much sesne. It is equivalent to
> NOWAIT.

Specifying a flag that says "don't do fault injection for this
allocation" looks like a reasonable solution. Fewer lines of code and
no need to switch on interrupts. __GFP_NOFAIL seems to mean more than
that, so perhaps we need a separate flag that affects only fault
injection and should be used only in debugging code (no-op without
fault injection anyway).

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

On 22 April 2018 at 23:00, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Sun, Apr 22, 2018 at 2:51 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> On Fri 20-04-18 18:50:24, Catalin Marinas wrote:
>>> On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
>>> > __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
>>> > But it's a wrong combination. As __GFP_NOFAIL is blockable, but
>>> > __GFP_NORETY is not blockable, make it self-contradiction.
>>> >
>>> > __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
>>> > it's not the real intention, as kmemleak allow alloc failure happen in
>>> > memory pressure, in that case kmemleak just disables itself.
>>>
>>> Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
>>> ("kmemleak: allow to coexist with fault injection") to keep kmemleak
>>> usable under fault injection.
>>>
>>> > commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
>>> > slowpath") documented that what user wants here should use GFP_NOWAIT, and
>>> > the WARN in __alloc_pages_slowpath caught this weird usage.
>>> >
>>> >  <snip>
>>> >  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
>>> [...]
>>> > Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
>>> > and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
>>> > is no blockable and no reclaim, making kmemleak less disruptive to user
>>> > processes in pressure.
>>>
>>> It doesn't solve the fault injection problem for kmemleak (unless we
>>> change __should_failslab() somehow, not sure yet). An option would be to
>>> replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
>>> is enabled.
>>
>> Cannot we simply have a disable_fault_injection knob around the
>> allocation rather than playing this dirty tricks with gfp flags which do
>> not make any sense?

To this way, looks like we need to change the attrs. but what we have stored in
attr is also gfp flags, even we define a new member, ignore_flags, we still
need a new flag, or did I miss something?  But looks like a new flag is simple.

For slab, it supports cache_filter, so it's possible to add a
filer_out slab flag,
and skip it.  But for page, it's just has gfp flag and size info for
filter, no other info.

#ifdef CONFIG_FAIL_PAGE_ALLOC

static struct {
    struct fault_attr attr;

    bool ignore_gfp_highmem;
    bool ignore_gfp_reclaim;
    u32 min_order;
} fail_page_alloc = {
    .attr = FAULT_ATTR_INITIALIZER,
    .ignore_gfp_reclaim = true,
    .ignore_gfp_highmem = true,
    .min_order = 1,
};


static struct {
    struct fault_attr attr;
    bool ignore_gfp_reclaim;
    bool cache_filter;
} failslab = {
    .attr = FAULT_ATTR_INITIALIZER,
    .ignore_gfp_reclaim = true,
    .cache_filter = false,
};

>>
>>> BTW, does the combination of NOWAIT and NORETRY make kmemleak
>>> allocations more likely to fail?
>>
>> NOWAIT + NORETRY simply doesn't make much sesne. It is equivalent to
>> NOWAIT.
>
> Specifying a flag that says "don't do fault injection for this
> allocation" looks like a reasonable solution. Fewer lines of code and
> no need to switch on interrupts. __GFP_NOFAIL seems to mean more than
> that, so perhaps we need a separate flag that affects only fault
> injection and should be used only in debugging code (no-op without
> fault injection anyway).


Got the  two places for skipping fault injection, both they check the gfp flags
as part of the work.  If we have the new no fault inject flag, and
define the wrapper
as below, then it will look like.
#define _GFP_NOFAULTINJECT (__GFP_NOFAIL|___GFP_NOFAULTINJECT)

bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags)
{
    /* No fault-injection for bootstrap cache */
    if (unlikely(s == kmem_cache))
        return false;

    if (gfpflags &  _GFP_NOFAULTINJECTL)
        return false;

    if (failslab.ignore_gfp_reclaim && (gfpflags & __GFP_RECLAIM))
        return false;

    if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
        return false;

    return should_fail(&failslab.attr, s->object_size);
}


static bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
{
    if (order < fail_page_alloc.min_order)
        return false;
    if (gfp_mask &  _GFP_NOFAULTINJECT)
        return false;
    if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
        return false;
    if (fail_page_alloc.ignore_gfp_reclaim &&
            (gfp_mask & __GFP_DIRECT_RECLAIM))
        return false;

    return should_fail(&fail_page_alloc.attr, 1 << order);
}

the gfp flags defined in linux/gfp.h. we have now 24 flags already, and we have
an precedent ___GFP_NOLOCKDEP for skipping lockdep.

/* Plain integer GFP bitmasks. Do not use this directly. */
#define ___GFP_DMA      0x01u
#define ___GFP_HIGHMEM      0x02u
#define ___GFP_DMA32        0x04u
#define ___GFP_MOVABLE      0x08u
#define ___GFP_RECLAIMABLE  0x10u
#define ___GFP_HIGH     0x20u
#define ___GFP_IO       0x40u
#define ___GFP_FS       0x80u
#define ___GFP_NOWARN       0x200u
#define ___GFP_RETRY_MAYFAIL    0x400u
#define ___GFP_NOFAIL       0x800u
#define ___GFP_NORETRY      0x1000u
#define ___GFP_MEMALLOC     0x2000u
#define ___GFP_COMP     0x4000u
#define ___GFP_ZERO     0x8000u
#define ___GFP_NOMEMALLOC   0x10000u
#define ___GFP_HARDWALL     0x20000u
#define ___GFP_THISNODE     0x40000u
#define ___GFP_ATOMIC       0x80000u
#define ___GFP_ACCOUNT      0x100000u
#define ___GFP_DIRECT_RECLAIM   0x400000u
#define ___GFP_WRITE        0x800000u
#define ___GFP_KSWAPD_RECLAIM   0x1000000u
#ifdef CONFIG_LOCKDEP
#define ___GFP_NOLOCKDEP    0x2000000u
#else
#define ___GFP_NOLOCKDEP    0
#endif

So if there is a new flag, it would be the 25th bits.

#ifdef CONFIG_KMEMLEAK
#define ___GFP_NOFAULTINJECT    0x4000000u
#else
#define ___GFP_NOFAULT_INJECT    0
#endif

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Michal Hocko]

On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
[...]
> So if there is a new flag, it would be the 25th bits.

No new flags please. Can you simply store a simple bool into fail_page_alloc
and have save/restore api for that?

-- 
Michal Hocko
SUSE Labs

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Catalin Marinas]

On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> [...]
> > So if there is a new flag, it would be the 25th bits.
> 
> No new flags please. Can you simply store a simple bool into fail_page_alloc
> and have save/restore api for that?

For kmemleak, we probably first hit failslab. Something like below may
do the trick:

diff --git a/mm/failslab.c b/mm/failslab.c
index 1f2f248e3601..63f13da5cb47 100644
--- a/mm/failslab.c
+++ b/mm/failslab.c
@@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags)
 	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
 		return false;
 
+	if (s->flags & SLAB_NOLEAKTRACE)
+		return false;
+
 	return should_fail(&failslab.attr, s->object_size);
 }
 

Can we get a second should_fail() via should_fail_alloc_page() if a new
slab page is allocated?

-- 
Catalin

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Catalin Marinas" <catalin.marinas@arm.com>
> To: "Michal Hocko" <mhocko@kernel.org>
> Cc: "Chunyu Hu" <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>, "Chunyu Hu" <chuhu@redhat.com>, "LKML"
> <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Tuesday, April 24, 2018 9:41:48 PM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > [...]
> > > So if there is a new flag, it would be the 25th bits.
> > 
> > No new flags please. Can you simply store a simple bool into
> > fail_page_alloc
> > and have save/restore api for that?
> 
> For kmemleak, we probably first hit failslab. Something like below may
> do the trick:
> 
> diff --git a/mm/failslab.c b/mm/failslab.c
> index 1f2f248e3601..63f13da5cb47 100644
> --- a/mm/failslab.c
> +++ b/mm/failslab.c
> @@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
> gfpflags)
>  	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
>  		return false;
>  
> +	if (s->flags & SLAB_NOLEAKTRACE)
> +		return false;
> +
>  	return should_fail(&failslab.attr, s->object_size);
>  }

This maybe is the easy enough way for skipping fault injection for kmemleak slab object. 
 
>  
> 
> Can we get a second should_fail() via should_fail_alloc_page() if a new
> slab page is allocated?

looking at code path blow, what do you mean by getting a second should_fail() via
fail_alloc_page?  Seems we need to insert the flag between alloc_slab_page and 
alloc_pages()? Without GFP flag, it's difficult to pass info to should_fail_alloc_page
and keep simple at same time. 

Or as Michal suggested, completely disabling page alloc fail injection when kmemleak
enabled. And enable it again when kmemleak off. 

 alloc_slab_page   
          <========= flag to change the behavior of should_fail_alloc_page
     alloc_pages
         alloc_pages_current
             __alloc_pages_nodemask
                 prepare_alloc_pages
                     should_fail_alloc_page

> 
> --
> Catalin
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Catalin Marinas]

On Wed, Apr 25, 2018 at 05:50:41AM -0400, Chunyu Hu wrote:
> ----- Original Message -----
> > From: "Catalin Marinas" <catalin.marinas@arm.com>
> > On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> > > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > > [...]
> > > > So if there is a new flag, it would be the 25th bits.
> > > 
> > > No new flags please. Can you simply store a simple bool into
> > > fail_page_alloc
> > > and have save/restore api for that?
> > 
> > For kmemleak, we probably first hit failslab. Something like below may
> > do the trick:
> > 
> > diff --git a/mm/failslab.c b/mm/failslab.c
> > index 1f2f248e3601..63f13da5cb47 100644
> > --- a/mm/failslab.c
> > +++ b/mm/failslab.c
> > @@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
> > gfpflags)
> >  	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
> >  		return false;
> >  
> > +	if (s->flags & SLAB_NOLEAKTRACE)
> > +		return false;
> > +
> >  	return should_fail(&failslab.attr, s->object_size);
> >  }
> 
> This maybe is the easy enough way for skipping fault injection for
> kmemleak slab object. 

This was added to avoid kmemleak tracing itself, so could be used for
other kmemleak-related cases.

> > Can we get a second should_fail() via should_fail_alloc_page() if a new
> > slab page is allocated?
> 
> looking at code path below, what do you mean by getting a second
> should_fail() via fail_alloc_page?

Kmemleak calls kmem_cache_alloc() on a cache with SLAB_LEAKNOTRACE, so the
first point of failure injection is __should_failslab() which we can
handle with the slab flag. The slab allocator itself ends up calling
alloc_pages() to allocate a slab page (and __GFP_NOFAIL is explicitly
cleared). Here we have the second potential failure injection via
fail_alloc_page(). That's unless the order < fail_page_alloc.min_order
which I think is the default case (min_order = 1 while the slab page
allocation for kmemleak would need an order of 0. It's not ideal but we
may get away with it.

> Seems we need to insert the flag between alloc_slab_page and
> alloc_pages()? Without GFP flag, it's difficult to pass info to
> should_fail_alloc_page and keep simple at same time. 

Indeed.

> Or as Michal suggested, completely disabling page alloc fail injection
> when kmemleak enabled. And enable it again when kmemleak off. 

Dmitry's point was that kmemleak is still useful to detect leaks on the
error path where errors are actually introduced by the fault injection.
Kmemleak cannot cope with allocation failures as it needs a pretty
precise tracking of the allocated objects.

An alternative could be to not free the early_log buffer in kmemleak and
use that memory in an emergency when allocation fails (though I don't
particularly like this).

Yet another option is to use NOFAIL and remove NORETRY in kmemleak when
fault injection is enabled.

-- 
Catalin

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Catalin Marinas" <catalin.marinas@arm.com>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: "Michal Hocko" <mhocko@kernel.org>, "Chunyu Hu" <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>,
> "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Wednesday, April 25, 2018 8:51:55 PM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> On Wed, Apr 25, 2018 at 05:50:41AM -0400, Chunyu Hu wrote:
> > ----- Original Message -----
> > > From: "Catalin Marinas" <catalin.marinas@arm.com>
> > > On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> > > > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > > > [...]
> > > > > So if there is a new flag, it would be the 25th bits.
> > > > 
> > > > No new flags please. Can you simply store a simple bool into
> > > > fail_page_alloc
> > > > and have save/restore api for that?
> > > 
> > > For kmemleak, we probably first hit failslab. Something like below may
> > > do the trick:
> > > 
> > > diff --git a/mm/failslab.c b/mm/failslab.c
> > > index 1f2f248e3601..63f13da5cb47 100644
> > > --- a/mm/failslab.c
> > > +++ b/mm/failslab.c
> > > @@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
> > > gfpflags)
> > >  	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
> > >  		return false;
> > >  
> > > +	if (s->flags & SLAB_NOLEAKTRACE)
> > > +		return false;
> > > +
> > >  	return should_fail(&failslab.attr, s->object_size);
> > >  }
> > 
> > This maybe is the easy enough way for skipping fault injection for
> > kmemleak slab object.
> 
> This was added to avoid kmemleak tracing itself, so could be used for
> other kmemleak-related cases.
> 
> > > Can we get a second should_fail() via should_fail_alloc_page() if a new
> > > slab page is allocated?
> > 
> > looking at code path below, what do you mean by getting a second
> > should_fail() via fail_alloc_page?
> 
> Kmemleak calls kmem_cache_alloc() on a cache with SLAB_LEAKNOTRACE, so the
> first point of failure injection is __should_failslab() which we can
> handle with the slab flag. The slab allocator itself ends up calling
> alloc_pages() to allocate a slab page (and __GFP_NOFAIL is explicitly
> cleared). Here we have the second potential failure injection via

Indeed.

> fail_alloc_page(). That's unless the order < fail_page_alloc.min_order
> which I think is the default case (min_order = 1 while the slab page
> allocation for kmemleak would need an order of 0. It's not ideal but we
> may get away with it.

In my workstation, I checked the value shown is order=2

[mm]# cat /sys/kernel/slab/kmemleak_object/order 
2
[mm]# uname -r
4.17.0-rc1.syzcaller+


If order is 2, then not into the branch, no false is returned, so not skipped..
static bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
{
    if (order < fail_page_alloc.min_order)
        return false;


> 
> > Seems we need to insert the flag between alloc_slab_page and
> > alloc_pages()? Without GFP flag, it's difficult to pass info to
> > should_fail_alloc_page and keep simple at same time.
> 
> Indeed.
> 
> > Or as Michal suggested, completely disabling page alloc fail injection
> > when kmemleak enabled. And enable it again when kmemleak off.
> 
> Dmitry's point was that kmemleak is still useful to detect leaks on the
> error path where errors are actually introduced by the fault injection.
> Kmemleak cannot cope with allocation failures as it needs a pretty
> precise tracking of the allocated objects.

understand.

> 
> An alternative could be to not free the early_log buffer in kmemleak and
> use that memory in an emergency when allocation fails (though I don't
> particularly like this).
> 
> Yet another option is to use NOFAIL and remove NORETRY in kmemleak when
> fault injection is enabled.

I'm going to have a try this way to see if any warning can be seen when running.
This should be the best if it works fine. 

> 
> --
> Catalin
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Chunyu Hu" <chuhu@redhat.com>
> To: "Catalin Marinas" <catalin.marinas@arm.com>
> Cc: "Michal Hocko" <mhocko@kernel.org>, "Chunyu Hu" <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>,
> "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Wednesday, April 25, 2018 10:33:49 PM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> 
> 
> ----- Original Message -----
> > From: "Catalin Marinas" <catalin.marinas@arm.com>
> > To: "Chunyu Hu" <chuhu@redhat.com>
> > Cc: "Michal Hocko" <mhocko@kernel.org>, "Chunyu Hu"
> > <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>,
> > "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> > Sent: Wednesday, April 25, 2018 8:51:55 PM
> > Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in
> > gfp_kmemleak_mask
> > 
> > On Wed, Apr 25, 2018 at 05:50:41AM -0400, Chunyu Hu wrote:
> > > ----- Original Message -----
> > > > From: "Catalin Marinas" <catalin.marinas@arm.com>
> > > > On Tue, Apr 24, 2018 at 07:20:57AM -0600, Michal Hocko wrote:
> > > > > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > > > > [...]
> > > > > > So if there is a new flag, it would be the 25th bits.
> > > > > 
> > > > > No new flags please. Can you simply store a simple bool into
> > > > > fail_page_alloc
> > > > > and have save/restore api for that?
> > > > 
> > > > For kmemleak, we probably first hit failslab. Something like below may
> > > > do the trick:
> > > > 
> > > > diff --git a/mm/failslab.c b/mm/failslab.c
> > > > index 1f2f248e3601..63f13da5cb47 100644
> > > > --- a/mm/failslab.c
> > > > +++ b/mm/failslab.c
> > > > @@ -29,6 +29,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
> > > > gfpflags)
> > > >  	if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
> > > >  		return false;
> > > >  
> > > > +	if (s->flags & SLAB_NOLEAKTRACE)
> > > > +		return false;
> > > > +
> > > >  	return should_fail(&failslab.attr, s->object_size);
> > > >  }

Looks like if just for this slab fault inject issue, and when fail page
alloc is not enabled, this should be enough to make the warning go away.

And for page allocate fail part,  per task handling is an option way, without
introducing GFP new flag for fault injection. 

> > > 
> > > This maybe is the easy enough way for skipping fault injection for
> > > kmemleak slab object.
> > 
> > This was added to avoid kmemleak tracing itself, so could be used for
> > other kmemleak-related cases.
> > 
> > > > Can we get a second should_fail() via should_fail_alloc_page() if a new
> > > > slab page is allocated?
> > > 
> > > looking at code path below, what do you mean by getting a second
> > > should_fail() via fail_alloc_page?
> > 
> > Kmemleak calls kmem_cache_alloc() on a cache with SLAB_LEAKNOTRACE, so the
> > first point of failure injection is __should_failslab() which we can
> > handle with the slab flag. The slab allocator itself ends up calling
> > alloc_pages() to allocate a slab page (and __GFP_NOFAIL is explicitly
> > cleared). Here we have the second potential failure injection via
> 
> Indeed.
> 
> > fail_alloc_page(). That's unless the order < fail_page_alloc.min_order
> > which I think is the default case (min_order = 1 while the slab page
> > allocation for kmemleak would need an order of 0. It's not ideal but we
> > may get away with it.
> 
> In my workstation, I checked the value shown is order=2
> 
> [mm]# cat /sys/kernel/slab/kmemleak_object/order
> 2
> [mm]# uname -r
> 4.17.0-rc1.syzcaller+
> 
> 
> If order is 2, then not into the branch, no false is returned, so not
> skipped..
> static bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
> {
>     if (order < fail_page_alloc.min_order)
>         return false;
> 
> 
> > 
> > > Seems we need to insert the flag between alloc_slab_page and
> > > alloc_pages()? Without GFP flag, it's difficult to pass info to
> > > should_fail_alloc_page and keep simple at same time.
> > 
> > Indeed.
> > 
> > > Or as Michal suggested, completely disabling page alloc fail injection
> > > when kmemleak enabled. And enable it again when kmemleak off.
> > 
> > Dmitry's point was that kmemleak is still useful to detect leaks on the
> > error path where errors are actually introduced by the fault injection.
> > Kmemleak cannot cope with allocation failures as it needs a pretty
> > precise tracking of the allocated objects.
> 
> understand.
> 
> > 
> > An alternative could be to not free the early_log buffer in kmemleak and
> > use that memory in an emergency when allocation fails (though I don't
> > particularly like this).

This is still an option. 

> > 
> > Yet another option is to use NOFAIL and remove NORETRY in kmemleak when
> > fault injection is enabled.
> 
> I'm going to have a try this way to see if any warning can be seen when
> running.
> This should be the best if it works fine.

NOFAIL has a strict requirement that it must direct_reclaimable, otherwise, the
warning still will be seen, though 'use NOFAIL and remove NORETRY' as you
suggested.  so this is not an option.

mm/page_alloc.c
4256     if (gfp_mask & __GFP_NOFAIL) {                                                                                                                                      
4257         /*
4258          * All existing users of the __GFP_NOFAIL are blockable, so warn
4259          * of any new users that actually require GFP_NOWAIT
4260          */
4261         if (WARN_ON_ONCE(!can_direct_reclaim))
4262             goto fail;

So I also tried to add DIRECT_RECLAIM and NOFAIL together, and no  doubt, it will
sleep in irq, so don't work.

[  168.802049] BUG: sleeping function called from invalid context at mm/slab.h:421
[  168.802937] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/2
[  168.803701] INFO: lockdep is turned off.
[  168.804162] Preemption disabled at:
[  168.804171] [<ffffffff8111ee71>] start_secondary+0x141/0x5f0
[  168.805259] CPU: 2 PID: 0 Comm: swapper/2 Tainted: G        W         4.17.0-rc2.syzcaller+ #18
[  168.806267] Hardware name: Red Hat KVM, BIOS 0.0.0 02/06/2015
[  168.806928] Call Trace:
[  168.807211]  <IRQ>
[  168.807456]  dump_stack+0x11b/0x1be
[  168.807854]  ? show_regs_print_info+0x12/0x12
[  168.808347]  ? start_secondary+0x141/0x5f0
[  168.808845]  ? create_object+0xa6/0xaf0
[  168.809284]  ___might_sleep+0x3a6/0x5d0
[  168.809732]  kmem_cache_alloc+0x2d0/0x580
[  168.810186]  ? update_sd_lb_stats+0x3080/0x3080
[  168.810727]  ? __netif_receive_skb_core+0x15a3/0x3400
[  168.811293]  ? __build_skb+0x86/0x3b0
[  168.811698]  create_object+0xa6/0xaf0



> 
> > 
> > --
> > Catalin
> > 
> 
> --
> Regards,
> Chunyu Hu
> 
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Michal Hocko" <mhocko@kernel.org>
> To: "Chunyu Hu" <chuhu.ncepu@gmail.com>
> Cc: "Dmitry Vyukov" <dvyukov@google.com>, "Catalin Marinas" <catalin.marinas@arm.com>, "Chunyu Hu"
> <chuhu@redhat.com>, "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Tuesday, April 24, 2018 9:20:57 PM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> [...]
> > So if there is a new flag, it would be the 25th bits.
> 
> No new flags please. Can you simply store a simple bool into fail_page_alloc
> and have save/restore api for that?

Hi Michal,

I still don't get your point. The original NOFAIL added in kmemleak was 
for skipping fault injection in page/slab  allocation for kmemleak object, 
since kmemleak will disable itself until next reboot, whenever it hit an 
allocation failure, in that case, it will lose effect to check kmemleak 
in errer path rose by fault injection. But NOFAULT's effect is more than 
skipping fault injection, it's also for hard allocation. So a dedicated flag
for skipping fault injection in specified slab/page allocation was mentioned.
 
d9570ee3bd1d ("kmemleak: allow to coexist with fault injection") 
  
Do you mean something like below, with the save/store api? But looks like
to make it possible to skip a specified allocation, not global disabling,
a bool is not enough, and a gfp_flag is also needed. Maybe I missed something?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 905db9d..ca6f609 100644                                                                                                                     
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3053,14 +3053,28 @@ struct page *rmqueue(struct zone *preferred_zone,
 
    bool ignore_gfp_highmem;
    bool ignore_gfp_reclaim;
+   bool ignore_kmemleak_fault;
    u32 min_order;
 } fail_page_alloc = { 
    .attr = FAULT_ATTR_INITIALIZER,
    .ignore_gfp_reclaim = true,
    .ignore_gfp_highmem = true,
+   .ignore_kmemleak_fault = true,
    .min_order = 1,
 };
 
+bool saved_fail_page_alloc_ignore;
+void fail_page_alloc_ignore_save(void)
+{
+   saved_fail_page_alloc_ignore = fail_page_alloc.ignore_kmemleak_fault;
+   fail_page_alloc.ignore_kmemleak_fault = true;
+}
+
+void fail_page_alloc_ignore_restore(void)
+{
+   fail_page_alloc.ignore_kmemleak_fault = saved_fail_page_alloc_ignore;
+}
+
 static int __init setup_fail_page_alloc(char *str)
 {
    return setup_fault_attr(&fail_page_alloc.attr, str);
@@ -3075,6 +3089,9 @@ static bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
        return false;
    if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
        return false;
+   /* looks like here we still need a new GFP_KMEMLKEAK flag ... */
+   if (fail_page_alloc.ignore_kmemleak_fault && (gfp_mask & __GFP_KMEMLEAK))
+       return false;
    if (fail_page_alloc.ignore_gfp_reclaim &&
            (gfp_mask & __GFP_DIRECT_RECLAIM))
        return false;   

> 
> --
> Michal Hocko
> SUSE Labs
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Michal Hocko]

On Tue 24-04-18 12:48:50, Chunyu Hu wrote:
> 
> 
> ----- Original Message -----
> > From: "Michal Hocko" <mhocko@kernel.org>
> > To: "Chunyu Hu" <chuhu.ncepu@gmail.com>
> > Cc: "Dmitry Vyukov" <dvyukov@google.com>, "Catalin Marinas" <catalin.marinas@arm.com>, "Chunyu Hu"
> > <chuhu@redhat.com>, "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> > Sent: Tuesday, April 24, 2018 9:20:57 PM
> > Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> > 
> > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > [...]
> > > So if there is a new flag, it would be the 25th bits.
> > 
> > No new flags please. Can you simply store a simple bool into fail_page_alloc
> > and have save/restore api for that?
> 
> Hi Michal,
> 
> I still don't get your point. The original NOFAIL added in kmemleak was 
> for skipping fault injection in page/slab  allocation for kmemleak object, 
> since kmemleak will disable itself until next reboot, whenever it hit an 
> allocation failure, in that case, it will lose effect to check kmemleak 
> in errer path rose by fault injection. But NOFAULT's effect is more than 
> skipping fault injection, it's also for hard allocation. So a dedicated flag
> for skipping fault injection in specified slab/page allocation was mentioned.

I am not familiar with the kmemleak all that much, but fiddling with the
gfp_mask is a wrong way to achieve kmemleak specific action. I might be
easilly wrong but I do not see any code that would restore the original
gfp_mask down the kmem_cache_alloc path.

> d9570ee3bd1d ("kmemleak: allow to coexist with fault injection") 
>   
> Do you mean something like below, with the save/store api? But looks like
> to make it possible to skip a specified allocation, not global disabling,
> a bool is not enough, and a gfp_flag is also needed. Maybe I missed something?

Yes, this is essentially what I meant. It is still a global thing which
is not all that great and if it matters then you can make it per
task_struct. That really depends on the code flow here.

-- 
Michal Hocko
SUSE Labs

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Dmitry Vyukov]

On Tue, Apr 24, 2018 at 7:02 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 24-04-18 12:48:50, Chunyu Hu wrote:
>>
>>
>> ----- Original Message -----
>> > From: "Michal Hocko" <mhocko@kernel.org>
>> > To: "Chunyu Hu" <chuhu.ncepu@gmail.com>
>> > Cc: "Dmitry Vyukov" <dvyukov@google.com>, "Catalin Marinas" <catalin.marinas@arm.com>, "Chunyu Hu"
>> > <chuhu@redhat.com>, "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
>> > Sent: Tuesday, April 24, 2018 9:20:57 PM
>> > Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
>> >
>> > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
>> > [...]
>> > > So if there is a new flag, it would be the 25th bits.
>> >
>> > No new flags please. Can you simply store a simple bool into fail_page_alloc
>> > and have save/restore api for that?
>>
>> Hi Michal,
>>
>> I still don't get your point. The original NOFAIL added in kmemleak was
>> for skipping fault injection in page/slab  allocation for kmemleak object,
>> since kmemleak will disable itself until next reboot, whenever it hit an
>> allocation failure, in that case, it will lose effect to check kmemleak
>> in errer path rose by fault injection. But NOFAULT's effect is more than
>> skipping fault injection, it's also for hard allocation. So a dedicated flag
>> for skipping fault injection in specified slab/page allocation was mentioned.
>
> I am not familiar with the kmemleak all that much, but fiddling with the
> gfp_mask is a wrong way to achieve kmemleak specific action. I might be

I would say this is more like slab fault injection-specific action. It
can be used in other debugging facilities. Slab fault injection is a
part of slab. Slab behavior is generally controlled with gfp_mask.

> easilly wrong but I do not see any code that would restore the original
> gfp_mask down the kmem_cache_alloc path.
>
>> d9570ee3bd1d ("kmemleak: allow to coexist with fault injection")
>>
>> Do you mean something like below, with the save/store api? But looks like
>> to make it possible to skip a specified allocation, not global disabling,
>> a bool is not enough, and a gfp_flag is also needed. Maybe I missed something?
>
> Yes, this is essentially what I meant. It is still a global thing which
> is not all that great and if it matters then you can make it per
> task_struct. That really depends on the code flow here.

If we go this route, it definitely needs to be per task and also needs
to work with interrupts: switch on interrupts and not corrupt on
interrupts. A gfp flag is free of these problems.

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Michal Hocko" <mhocko@kernel.org>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: "Chunyu Hu" <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>, "Catalin Marinas"
> <catalin.marinas@arm.com>, "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Wednesday, April 25, 2018 1:02:39 AM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> On Tue 24-04-18 12:48:50, Chunyu Hu wrote:
> > 
> > 
> > ----- Original Message -----
> > > From: "Michal Hocko" <mhocko@kernel.org>
> > > To: "Chunyu Hu" <chuhu.ncepu@gmail.com>
> > > Cc: "Dmitry Vyukov" <dvyukov@google.com>, "Catalin Marinas"
> > > <catalin.marinas@arm.com>, "Chunyu Hu"
> > > <chuhu@redhat.com>, "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM"
> > > <linux-mm@kvack.org>
> > > Sent: Tuesday, April 24, 2018 9:20:57 PM
> > > Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in
> > > gfp_kmemleak_mask
> > > 
> > > On Mon 23-04-18 12:17:32, Chunyu Hu wrote:
> > > [...]
> > > > So if there is a new flag, it would be the 25th bits.
> > > 
> > > No new flags please. Can you simply store a simple bool into
> > > fail_page_alloc
> > > and have save/restore api for that?
> > 
> > Hi Michal,
> > 
> > I still don't get your point. The original NOFAIL added in kmemleak was
> > for skipping fault injection in page/slab  allocation for kmemleak object,
> > since kmemleak will disable itself until next reboot, whenever it hit an
> > allocation failure, in that case, it will lose effect to check kmemleak
> > in errer path rose by fault injection. But NOFAULT's effect is more than
> > skipping fault injection, it's also for hard allocation. So a dedicated
> > flag
> > for skipping fault injection in specified slab/page allocation was
> > mentioned.
> 
> I am not familiar with the kmemleak all that much, but fiddling with the

kmemleak is using kmem_cache to record every pointers returned from kernel mem 
allocation activities such as kmem_cache_alloc(). every time an object from
slab allocator is returned, a following new kmemleak object is allocated.  

And when a slab object is freed, then the kmemleak object which contains
the ptr will also be freed. 

and kmemleak scan thread will run in period to scan the kernel data, stack, 
and per cpu areas to check that every pointers recorded by kmemleak has at least
one reference in those areas beside the one recorded by kmemleak. If there
is no place in the memory acreas recording the ptr, then it's possible a leak.

so once a kmemleak object allocation failed, it has to disable itself, otherwise
it would lose track of some object pointers, and become less meaningful to 
continue record and scan the kernel memory for the pointers. So disable
it forever. so this is why kmemleak can't tolerate a slab alloc fail (from fault injection)

@Catalin,

Is this right? If something not so correct or precise, please correct me.
I'm thinking about, is it possible that make kmemleak don't disable itself
when fail_page_alloc is enabled?  I can't think clearly what would happen
if several memory allocation missed by kmelkeak trace, what's the bad result? 


> gfp_mask is a wrong way to achieve kmemleak specific action. I might be

As Dmirty explained, this is in fact for fault injection providing a method
to make some debug feature be able to not be injected fault by the 'fault injection'.

Some other features beside kmemleak I can think out is ftrace will also 
hard disable itself when page allocation failed (not sure about slab)

> easilly wrong but I do not see any code that would restore the original
> gfp_mask down the kmem_cache_alloc path.

looks like currently flag __GFP_NOFAIL can go very deep to where before new slab 
needed to be allocated. So what's a pity is when CONFIG_FAIL_PAGE_ALLOC is 
defined, when new slab is created, this flag will be filtered out. So
GFP_NOFAIL currently is used by two meanings, first is allocation 
can't fail, second is meaning not fault injection, but page allocator
don't accept the second meaning. 

So there is a real need for the 'avoid-fault' meaning, though I agree, 
current there is only limited user/market, kmemleak, it just don't have api
to avoid being fault injected. But I think there will be other debug
feature who can be used during fault injection works.

> 
> > d9570ee3bd1d ("kmemleak: allow to coexist with fault injection")

even use GFP_NOFAIL, when new slab needs to be allocated, and when
CONFIG_FAIL_PAGE_ALLOC yes, it still can't avoid hurting by fault injection. 
as GFP_NOFAIL is filtered out in first fast path of allocate_slab.  

> >   
> > Do you mean something like below, with the save/store api? But looks like
> > to make it possible to skip a specified allocation, not global disabling,
> > a bool is not enough, and a gfp_flag is also needed. Maybe I missed
> > something?
> 
> Yes, this is essentially what I meant. It is still a global thing which
> is not all that great and if it matters then you can make it per
> task_struct. That really depends on the code flow here.

Thank you Michal for this suggestion. What I can imagine this  seems would be
very complicated. You know memory allocation is very frequent, and some memory 
allocation can sleep, and some not. Also there are complicated kernel threads.  

If I got your point correctly, a member added in task_struct, then every mem
allocation, needs to touch the member, and needs lock to protect the member.
Also it needs to interact with kmemleak structure. I'm still not very clear and
still thinking about it. @Dmitry know more about this? 


> 
> --
> Michal Hocko
> SUSE Labs
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Catalin Marinas]

On Thu, Apr 26, 2018 at 08:23:19AM -0400, Chunyu Hu wrote:
> kmemleak is using kmem_cache to record every pointers returned from kernel mem 
> allocation activities such as kmem_cache_alloc(). every time an object from
> slab allocator is returned, a following new kmemleak object is allocated.  
> 
> And when a slab object is freed, then the kmemleak object which contains
> the ptr will also be freed. 
> 
> and kmemleak scan thread will run in period to scan the kernel data, stack, 
> and per cpu areas to check that every pointers recorded by kmemleak has at least
> one reference in those areas beside the one recorded by kmemleak. If there
> is no place in the memory acreas recording the ptr, then it's possible a leak.
> 
> so once a kmemleak object allocation failed, it has to disable itself, otherwise
> it would lose track of some object pointers, and become less meaningful to 
> continue record and scan the kernel memory for the pointers. So disable
> it forever. so this is why kmemleak can't tolerate a slab alloc fail (from fault injection)
> 
> @Catalin,
> 
> Is this right? If something not so correct or precise, please correct me.

That's a good description, thanks.

> I'm thinking about, is it possible that make kmemleak don't disable itself
> when fail_page_alloc is enabled?  I can't think clearly what would happen
> if several memory allocation missed by kmelkeak trace, what's the bad result? 

Take for example a long linked list. If kmemleak doesn't track an object
in such list (because the metadata allocation failed), such list_head is
never scanned and the subsequent objects in the list (pointed at by
'next') will be reported as leaks. Kmemleak pretty much becomes unusable
with a high number of false positives.

-- 
Catalin

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

----- Original Message -----
> From: "Catalin Marinas" <catalin.marinas@arm.com>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: "Michal Hocko" <mhocko@kernel.org>, "Chunyu Hu" <chuhu.ncepu@gmail.com>, "Dmitry Vyukov" <dvyukov@google.com>,
> "LKML" <linux-kernel@vger.kernel.org>, "Linux-MM" <linux-mm@kvack.org>
> Sent: Thursday, April 26, 2018 8:56:35 PM
> Subject: Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask
> 
> On Thu, Apr 26, 2018 at 08:23:19AM -0400, Chunyu Hu wrote:
> > kmemleak is using kmem_cache to record every pointers returned from kernel
> > mem
> > allocation activities such as kmem_cache_alloc(). every time an object from
> > slab allocator is returned, a following new kmemleak object is allocated.
> > 
> > And when a slab object is freed, then the kmemleak object which contains
> > the ptr will also be freed.
> > 
> > and kmemleak scan thread will run in period to scan the kernel data, stack,
> > and per cpu areas to check that every pointers recorded by kmemleak has at
> > least
> > one reference in those areas beside the one recorded by kmemleak. If there
> > is no place in the memory acreas recording the ptr, then it's possible a
> > leak.
> > 
> > so once a kmemleak object allocation failed, it has to disable itself,
> > otherwise
> > it would lose track of some object pointers, and become less meaningful to
> > continue record and scan the kernel memory for the pointers. So disable
> > it forever. so this is why kmemleak can't tolerate a slab alloc fail (from
> > fault injection)
> > 
> > @Catalin,
> > 
> > Is this right? If something not so correct or precise, please correct me.
> 
> That's a good description, thanks.
> 
> > I'm thinking about, is it possible that make kmemleak don't disable itself
> > when fail_page_alloc is enabled?  I can't think clearly what would happen
> > if several memory allocation missed by kmelkeak trace, what's the bad
> > result?
> 
> Take for example a long linked list. If kmemleak doesn't track an object
> in such list (because the metadata allocation failed), such list_head is
> never scanned and the subsequent objects in the list (pointed at by
> 'next') will be reported as leaks. Kmemleak pretty much becomes unusable
> with a high number of false positives.

Thanks for the example, one object may contain many pointers, so loose one,
means many false reports. I'm clear now. 

> 
> --
> Catalin
> 

-- 
Regards,
Chunyu Hu

Re: [RFC] mm: kmemleak: replace __GFP_NOFAIL to GFP_NOWAIT in gfp_kmemleak_mask

[Chunyu Hu]

On 21 April 2018 at 01:50, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Sat, Apr 21, 2018 at 12:58:33AM +0800, Chunyu Hu wrote:
>> __GFP_NORETRY and  __GFP_NOFAIL are combined in gfp_kmemleak_mask now.
>> But it's a wrong combination. As __GFP_NOFAIL is blockable, but
>> __GFP_NORETY is not blockable, make it self-contradiction.
>>
>> __GFP_NOFAIL means 'The VM implementation _must_ retry infinitely'. But
>> it's not the real intention, as kmemleak allow alloc failure happen in
>> memory pressure, in that case kmemleak just disables itself.
>
> Good point. The __GFP_NOFAIL flag was added by commit d9570ee3bd1d
> ("kmemleak: allow to coexist with fault injection") to keep kmemleak
> usable under fault injection.

oops. I did not notice this.  so this is for skipping fault injection.
>
>> commit 9a67f6488eca ("mm: consolidate GFP_NOFAIL checks in the allocator
>> slowpath") documented that what user wants here should use GFP_NOWAIT, and
>> the WARN in __alloc_pages_slowpath caught this weird usage.

>>
>>  <snip>
>>  WARNING: CPU: 3 PID: 64 at mm/page_alloc.c:4261 __alloc_pages_slowpath+0x1cc3/0x2780
> [...]
>> Replace the __GFP_NOFAIL with GFP_NOWAIT in gfp_kmemleak_mask, __GFP_NORETRY
>> and GFP_NOWAIT are in the gfp_kmemleak_mask. So kmemleak object allocaion
>> is no blockable and no reclaim, making kmemleak less disruptive to user
>> processes in pressure.
>
> It doesn't solve the fault injection problem for kmemleak (unless we
> change __should_failslab() somehow, not sure yet). An option would be to
> replace __GFP_NORETRY with __GFP_NOFAIL in kmemleak when fault injection
> is enabled.
>
> BTW, does the combination of NOWAIT and NORETRY make kmemleak
> allocations more likely to fail?

this NOWAIT added here seems won't make it more likely to fail, as it its
defined as, which make kswapd woke in pressure.  But this replacement
indeed will make fault injection in kmemleak got back.

#define GFP_NOWAIT  (__GFP_KSWAPD_RECLAIM)

>
> Cc'ing Dmitry as well.
>
>> Signed-off-by: Chunyu Hu <chuhu@redhat.com>
>> CC: Michal Hocko <mhocko@suse.com>
>> ---
>>  mm/kmemleak.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index 9a085d5..4ea07e4 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -126,7 +126,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 | GFP_NOWAIT)
>>
>>  /* scanning area inside a memory block */
>>  struct kmemleak_scan_area {
>> --
>> 1.8.3.1