linux-kernel - Re: [PATCH v3] kmemleaak: survive in a low-memory situation

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190326160536.GO10344@bombadil.infradead.org>
Date:   Tue, 26 Mar 2019 09:05:36 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Qian Cai <cai@....pw>
Cc:     akpm@...ux-foundation.org, catalin.marinas@....com,
        mhocko@...nel.org, cl@...ux.com, penberg@...nel.org,
        rientjes@...gle.com, iamjoonsoo.kim@....com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kmemleaak: survive in a low-memory situation

On Tue, Mar 26, 2019 at 11:43:38AM -0400, Qian Cai wrote:
> Unless there is a brave soul to reimplement the kmemleak to embed it's
> metadata into the tracked memory itself in a foreseeable future, this
> provides a good balance between enabling kmemleak in a low-memory
> situation and not introducing too much hackiness into the existing
> code for now.

I don't understand kmemleak.  Kirill pointed me at this a few days ago:

https://gist.github.com/kiryl/3225e235fea390aa2e49bf625bbe83ec

It's caused by the XArray allocating memory using GFP_NOWAIT | __GFP_NOWARN.
kmemleak then decides it needs to allocate memory to track this memory.
So it calls kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));

#define gfp_kmemleak_mask(gfp)  (((gfp) & (GFP_KERNEL | GFP_ATOMIC)) | \
                                 __GFP_NORETRY | __GFP_NOMEMALLOC | \
                                 __GFP_NOWARN | __GFP_NOFAIL)

then the page allocator gets to see GFP_NOFAIL | GFP_NOWAIT and gets angry.

But I don't understand why kmemleak needs to mess with the GFP flags at
all.  Just allocate using the same flags as the caller, and fail the original
allocation if the kmemleak allocation fails.  Like this:

+++ b/mm/slab.h
@@ -435,12 +435,22 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
        for (i = 0; i < size; i++) {
                p[i] = kasan_slab_alloc(s, p[i], flags);
                /* As p[i] might get tagged, call kmemleak hook after KASAN. */
-               kmemleak_alloc_recursive(p[i], s->object_size, 1,
-                                        s->flags, flags);
+               if (kmemleak_alloc_recursive(p[i], s->object_size, 1,
+                                        s->flags, flags))
+                       goto fail;
        }
 
        if (memcg_kmem_enabled())
                memcg_kmem_put_cache(s);
+       return;
+
+fail:
+       while (i > 0) {
+               kasan_blah(...);
+               kmemleak_blah();
+               i--;
+       }
+	free_blah(p);
+       *p = NULL;
 }
 
 #ifndef CONFIG_SLOB


and if we had something like this, we wouldn't need kmemleak to have this
self-disabling or must-succeed property.