[PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Qian Cai]

The first kmemleak_scan() after boot would trigger a crash below because

kernel_init
  free_initmem
    mem_encrypt_free_decrypted_mem
      free_init_pages

unmapped some memory inside the .bss.

BUG: unable to handle kernel paging request at ffffffffbd402000
CPU: 12 PID: 325 Comm: kmemleak Not tainted 5.1.0-rc4+ #4
RIP: 0010:scan_block+0x58/0x160
Call Trace:
 scan_gray_list+0x1d9/0x280
 kmemleak_scan+0x485/0xad0
 kmemleak_scan_thread+0x9f/0xc4
 kthread+0x1d2/0x1f0
 ret_from_fork+0x35/0x40

Signed-off-by: Qian Cai <cai@lca.pw>
---
 arch/x86/mm/mem_encrypt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 385afa2b9e17..614ab156024f 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -18,6 +18,7 @@
 #include <linux/dma-direct.h>
 #include <linux/swiotlb.h>
 #include <linux/mem_encrypt.h>
+#include <linux/kmemleak.h>
 
 #include <asm/tlbflush.h>
 #include <asm/fixmap.h>
@@ -369,6 +370,11 @@ void __init mem_encrypt_free_decrypted_mem(void)
 		}
 	}
 
+	/*
+	 * Inform kmemleak about the hole in the .bss section since the
+	 * corresponding pages will be unmapped with DEBUG_PAGEALLOC=y.
+	 */
+	kmemleak_free_part((void *)vaddr, vaddr_end - vaddr);
 	free_init_pages("unused decrypted", vaddr, vaddr_end);
 }
 
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Catalin Marinas]

On Tue, Apr 09, 2019 at 12:05:02AM -0400, Qian Cai wrote:
> The first kmemleak_scan() after boot would trigger a crash below because
> 
> kernel_init
>   free_initmem
>     mem_encrypt_free_decrypted_mem
>       free_init_pages
> 
> unmapped some memory inside the .bss.
> 
> BUG: unable to handle kernel paging request at ffffffffbd402000
> CPU: 12 PID: 325 Comm: kmemleak Not tainted 5.1.0-rc4+ #4
> RIP: 0010:scan_block+0x58/0x160
> Call Trace:
>  scan_gray_list+0x1d9/0x280
>  kmemleak_scan+0x485/0xad0
>  kmemleak_scan_thread+0x9f/0xc4
>  kthread+0x1d2/0x1f0
>  ret_from_fork+0x35/0x40
> 
> Signed-off-by: Qian Cai <cai@lca.pw>

It seems that commit 298a32b13208 ("kmemleak: powerpc: skip scanning
holes in the .bss section") has other uses as well.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Borislav Petkov]

+ Brijesh.

On Tue, Apr 09, 2019 at 12:05:02AM -0400, Qian Cai wrote:
> The first kmemleak_scan() after boot would trigger a crash below because
> 
> kernel_init
>   free_initmem
>     mem_encrypt_free_decrypted_mem
>       free_init_pages
> 
> unmapped some memory inside the .bss.
> 
> BUG: unable to handle kernel paging request at ffffffffbd402000
> CPU: 12 PID: 325 Comm: kmemleak Not tainted 5.1.0-rc4+ #4
> RIP: 0010:scan_block+0x58/0x160
> Call Trace:
>  scan_gray_list+0x1d9/0x280
>  kmemleak_scan+0x485/0xad0
>  kmemleak_scan_thread+0x9f/0xc4
>  kthread+0x1d2/0x1f0
>  ret_from_fork+0x35/0x40
> 
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  arch/x86/mm/mem_encrypt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 385afa2b9e17..614ab156024f 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -18,6 +18,7 @@
>  #include <linux/dma-direct.h>
>  #include <linux/swiotlb.h>
>  #include <linux/mem_encrypt.h>
> +#include <linux/kmemleak.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/fixmap.h>
> @@ -369,6 +370,11 @@ void __init mem_encrypt_free_decrypted_mem(void)
>  		}
>  	}
>  
> +	/*
> +	 * Inform kmemleak about the hole in the .bss section since the
> +	 * corresponding pages will be unmapped with DEBUG_PAGEALLOC=y.
> +	 */
> +	kmemleak_free_part((void *)vaddr, vaddr_end - vaddr);
>  	free_init_pages("unused decrypted", vaddr, vaddr_end);

I don't understand what the logic here is: we have a couple of other
free_init_pages() calls but they don't have kmemleak_free_part() in
front.

Now, if kmemleak needs to be told that memory is getting freed, why
isn't kmemleak_free_part() called in free_init_pages() ?

This needs more explanation.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Qian Cai]

On 4/16/19 1:11 PM, Borislav Petkov wrote:
>> +	/*
>> +	 * Inform kmemleak about the hole in the .bss section since the
>> +	 * corresponding pages will be unmapped with DEBUG_PAGEALLOC=y.
>> +	 */
>> +	kmemleak_free_part((void *)vaddr, vaddr_end - vaddr);
>>  	free_init_pages("unused decrypted", vaddr, vaddr_end);
> 
> I don't understand what the logic here is: we have a couple of other
> free_init_pages() calls but they don't have kmemleak_free_part() in
> front.
> 
> Now, if kmemleak needs to be told that memory is getting freed, why
> isn't kmemleak_free_part() called in free_init_pages() ?
> 
> This needs more explanation.

kmemleak_init() will register the data/bss sections (only register
.data..ro_after_init if not within .data) and then kmemleak_scan() will scan
those address and dereference them looking for pointer referencing. If
free_init_pages() free and unmap pages in those sections, kmemleak_scan() will
trigger a crash if referencing one of those addresses.

I checked other x86 free_init_pages() call sites and don't see anything obvious
where another place to free an address in those sections.

__smp_locks[]: .smp_locks
__initramfs_start[]: .init
__init_begin: .init
from text_end to rodata_start: contains .notes, __ex_table
from rodata_end to _sdata: .pci_fixup, __ksymtab, __ksymtab_gpl etc

So, I don't think it need to add kmemleak_free_part() in every free_init_pages()
calls.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Borislav Petkov]

On Tue, Apr 16, 2019 at 08:38:30PM -0400, Qian Cai wrote:
> kmemleak_init() will register the data/bss sections (only register
> .data..ro_after_init if not within .data) and then kmemleak_scan() will scan
> those address and dereference them looking for pointer referencing. If
> free_init_pages() free and unmap pages in those sections, kmemleak_scan() will
> trigger a crash if referencing one of those addresses.
>
> I checked other x86 free_init_pages() call sites and don't see anything obvious
> where another place to free an address in those sections.

And why is .bss/.data special and why does it need that special handling
by kmemleak?

There must be some rule or a heuristic why kmemleak does that. Is that
documented somewhere?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Catalin Marinas]

On Thu, Apr 18, 2019 at 09:45:10AM +0200, Borislav Petkov wrote:
> On Tue, Apr 16, 2019 at 08:38:30PM -0400, Qian Cai wrote:
> > kmemleak_init() will register the data/bss sections (only register
> > .data..ro_after_init if not within .data) and then kmemleak_scan() will scan
> > those address and dereference them looking for pointer referencing. If
> > free_init_pages() free and unmap pages in those sections, kmemleak_scan() will
> > trigger a crash if referencing one of those addresses.
> >
> > I checked other x86 free_init_pages() call sites and don't see anything obvious
> > where another place to free an address in those sections.
> 
> And why is .bss/.data special and why does it need that special handling
> by kmemleak?

Kmemleak is basically a tri-colour marking tracing garbage collector [1]
but without automatic freeing. It relies on a graph of references
(pointers) between various objects and the root of such graph is the
.bss/.data.

free_init_pages() is called on memory regions outside .bss/.data like
smp_locks, initrd, kernel init text which kmemleak does not track
anyway. That said, kmemleak_free_part() is tolerant to unknown pointers,
so we could call it directly from free_init_pages().

> There must be some rule or a heuristic why kmemleak does that. Is that
> documented somewhere?

There is Documentation/dev-tools/kmemleak.rst briefly mentioning the
tracing garbage collector (although the Wikipedia link is no longer
valid, it should be replaced with [1]). It doesn't, however, state why
.bss/.data are special.

-- 
Catalin

[1] https://en.wikipedia.org/wiki/Tracing_garbage_collection#Tri-color_marking

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Borislav Petkov]

Hi Catalin,

sorry for the delay.

On Thu, Apr 18, 2019 at 10:50:15AM +0100, Catalin Marinas wrote:
> Kmemleak is basically a tri-colour marking tracing garbage collector [1]

Thanks for that - interesting read.

> but without automatic freeing. It relies on a graph of references
> (pointers) between various objects and the root of such graph is the
> .bss/.data.
> 
> free_init_pages() is called on memory regions outside .bss/.data like
> smp_locks, initrd, kernel init text which kmemleak does not track
> anyway. That said, kmemleak_free_part() is tolerant to unknown pointers,
> so we could call it directly from free_init_pages().

Ok, lemme think out loud for a bit here: kmemleak_scan() goes over
an object list and I guess in our particular case, the memory which
got freed in mem_encrypt_free_decrypted_mem() *was* in that list too,
leading to the crash.

Looking at the splat, it is in scan_gray_list() which would mean that
the object we freed was reachable from the root(s) in .bss.

Now, the docs say:

"The memory allocations via :c:func:`kmalloc`, :c:func:`vmalloc`,
:c:func:`kmem_cache_alloc` and
friends are traced and the pointers, together with additional
information like size and stack trace, are stored in a rbtree."

So I guess free_init_pages() should be somehow telling kmemleak, "hey,
just freed that object, pls adjust your tracking lists" no?

Because, otherwise, if we start sprinkling those kmemleak_free_part()
calls everywhere, that'll quickly turn into a game of whack-a-mole. And
we don't need that especially if kmemleak can easily be taught to handle
such cases.

IM *very* HO, of course. :-)

Here's where you tell me whether that even makes sense at all.

> There is Documentation/dev-tools/kmemleak.rst briefly mentioning the
> tracing garbage collector (although the Wikipedia link is no longer
> valid, it should be replaced with [1]). It doesn't, however, state why
> .bss/.data are special.

The fact that they're special is important info, I'd say.

Also,

> [1] https://en.wikipedia.org/wiki/Tracing_garbage_collection#Tri-color_marking

is nice. While reading, it made me think how our discussion would go if
we didn't have wikipedia. You'd probably say, go to the library and read
this and that section in this and that book on tri-color marking. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Catalin Marinas]

Hi Boris,

On Tue, Apr 23, 2019 at 03:25:18PM +0200, Borislav Petkov wrote:
> On Thu, Apr 18, 2019 at 10:50:15AM +0100, Catalin Marinas wrote:
> > Kmemleak is basically a tri-colour marking tracing garbage collector [1]
> 
> Thanks for that - interesting read.
> 
> > but without automatic freeing. It relies on a graph of references
> > (pointers) between various objects and the root of such graph is the
> > .bss/.data.

Sorry for the misleading information here, the root of the graph was
changed recently (see below).

> > free_init_pages() is called on memory regions outside .bss/.data like
> > smp_locks, initrd, kernel init text which kmemleak does not track
> > anyway. That said, kmemleak_free_part() is tolerant to unknown pointers,
> > so we could call it directly from free_init_pages().
> 
> Ok, lemme think out loud for a bit here: kmemleak_scan() goes over
> an object list and I guess in our particular case, the memory which
> got freed in mem_encrypt_free_decrypted_mem() *was* in that list too,
> leading to the crash.

Yes.

> Looking at the splat, it is in scan_gray_list() which would mean that
> the object we freed was reachable from the root(s) in .bss.

The .bss/.data used to be root until recently when commit 298a32b13208
("kmemleak: powerpc: skip scanning holes in the .bss section") changed
this to accommodate a similar problem on powerpc. With this commit,
.bss/.data are traced objects but painted "grey" by default so that they
will be always scanned, pretty much like the root (and they can't
"leak").

In Qian's splat, the unmapped area was actually in the .bss which is now
a traced object (no longer a root one). In his previous report on
powerpc [1], the splat was in scan_large_block().

> Now, the docs say:
> 
> "The memory allocations via :c:func:`kmalloc`, :c:func:`vmalloc`,
> :c:func:`kmem_cache_alloc` and
> friends are traced and the pointers, together with additional
> information like size and stack trace, are stored in a rbtree."
> 
> So I guess free_init_pages() should be somehow telling kmemleak, "hey,
> just freed that object, pls adjust your tracking lists" no?
> 
> Because, otherwise, if we start sprinkling those kmemleak_free_part()
> calls everywhere, that'll quickly turn into a game of whack-a-mole. And
> we don't need that especially if kmemleak can easily be taught to handle
> such cases.

Object freeing is tracked in general via the corresponding kfree(),
vfree() etc. and don't need special handling. The .bss doesn't have this
alloc/free symmetry and not freeing it all either, hence this
workaround to register it as a traced object and allow partial freeing.

Anyway, I agree with you. As I mentioned in the previous email,
kmemleak_free_part() is tolerant to unknown objects (not tracked by
kmemleak), so I'm fine with calling it from free_init_pages() even if
not all address ranges passed to this function are known to kmemleak.

> > There is Documentation/dev-tools/kmemleak.rst briefly mentioning the
> > tracing garbage collector (although the Wikipedia link is no longer
> > valid, it should be replaced with [1]). It doesn't, however, state why
> > .bss/.data are special.
> 
> The fact that they're special is important info, I'd say.

I took a note to improve this when I get some time.

> > [1] https://en.wikipedia.org/wiki/Tracing_garbage_collection#Tri-color_marking
> 
> is nice. While reading, it made me think how our discussion would go if
> we didn't have wikipedia. You'd probably say, go to the library and read
> this and that section in this and that book on tri-color marking. :-)

There are probably some academic papers published somewhere ;). But
wikipedia makes things much easier (and free).

-- 
Catalin

[1] http://lkml.kernel.org/r/20190312191412.28656-1-cai@lca.pw

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Qian Cai]

On Tue, 2019-04-23 at 15:25 +0200, Borislav Petkov wrote:
> So I guess free_init_pages() should be somehow telling kmemleak, "hey,
> just freed that object, pls adjust your tracking lists" no?
> 
> Because, otherwise, if we start sprinkling those kmemleak_free_part()
> calls everywhere, that'll quickly turn into a game of whack-a-mole. And
> we don't need that especially if kmemleak can easily be taught to handle
> such cases.

In reality, this is only the second place that needs this kmemleak_free_part()
call for all those years since kmemleak was born.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Borislav Petkov]

On Tue, Apr 23, 2019 at 03:02:56PM +0100, Catalin Marinas wrote:
> The .bss/.data used to be root until recently when commit 298a32b13208
> ("kmemleak: powerpc: skip scanning holes in the .bss section") changed
> this to accommodate a similar problem on powerpc. With this commit,
> .bss/.data are traced objects but painted "grey" by default so that they
> will be always scanned, pretty much like the root (and they can't
> "leak").

I see.

> In Qian's splat, the unmapped area was actually in the .bss which is now
> a traced object (no longer a root one).

Right, makes sense.

> Object freeing is tracked in general via the corresponding kfree(),
> vfree() etc. and don't need special handling. The .bss doesn't have this
> alloc/free symmetry and not freeing it all either, hence this
> workaround to register it as a traced object and allow partial freeing.
> 
> Anyway, I agree with you. As I mentioned in the previous email,
> kmemleak_free_part() is tolerant to unknown objects (not tracked by
> kmemleak), so I'm fine with calling it from free_init_pages() even if
> not all address ranges passed to this function are known to kmemleak.

Cool.

> I took a note to improve this when I get some time.

Thanks. That would be helpful, I think, for people like me who'd like to
get a short intro on kmemleak in order to understand what is being fixed
when a patch lands in their mbox. :)

> There are probably some academic papers published somewhere ;). But
> wikipedia makes things much easier (and free).

Yap :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

[Borislav Petkov]

On Tue, Apr 23, 2019 at 10:16:46AM -0400, Qian Cai wrote:
> In reality, this is only the second place that needs this kmemleak_free_part()
> call for all those years since kmemleak was born.

And? Which place warrants doing it right? The third? Fourth?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.