CONFIG_KASAN_SW_TAGS=y not play well with kmemleak
diff mbox series

Message ID 59db8d6b-4224-2ec9-09de-909c4338b67a@lca.pw
State New
Headers show
Series
  • CONFIG_KASAN_SW_TAGS=y not play well with kmemleak
Related show

Commit Message

Qian Cai Feb. 8, 2019, 4:16 a.m. UTC
Kmemleak is totally busted with CONFIG_KASAN_SW_TAGS=y because most of tracking
object pointers passed to create_object() have the upper bits set by KASAN.
However, even after applied this patch [1] to fix a few things, it still has
many errors during boot.

https://git.sr.ht/~cai/linux-debug/tree/master/dmesg

What I don't understand is that even the patch did call kasan_reset_tag() in
paint_ptr(), it still complained on objects with upper bits set which indicates
that this line did not run.

return (__s64)(value << shift) >> shift;

[   42.462799] kmemleak: Trying to color unknown object at 0xffff80082df80000 as
Grey
[   42.470524] CPU: 128 PID: 1 Comm: swapper/0 Not tainted 5.0.0-rc5+ #17
[   42.477153] Call trace:
[   42.479639]  dump_backtrace+0x0/0x450
[   42.483362]  show_stack+0x20/0x2c
[   42.486733]  __dump_stack+0x20/0x28
[   42.490276]  dump_stack+0xa0/0xfc
[   42.493649]  paint_ptr+0xa8/0xf4
[   42.496934]  kmemleak_not_leak+0xa4/0x15c
[   42.501013]  init_section_page_ext+0x1bc/0x328
[   42.505528]  page_ext_init+0x4dc/0x75c
[   42.509336]  kernel_init_freeable+0x684/0x1104
[   42.513857]  kernel_init+0x18/0x2a4
[   42.517407]  ret_from_fork+0x10/0x18

[1]
        object->min_count = min_count;
@@ -748,11 +748,12 @@ static void paint_it(struct kmemleak_object *object, int
color)
 static void paint_ptr(unsigned long ptr, int color)
 {
        struct kmemleak_object *object;
+       unsigned long addr = (unsigned long)kasan_reset_tag((void *)ptr);

-       object = find_and_get_object(ptr, 0);
+       object = find_and_get_object(addr, 0);
        if (!object) {
                kmemleak_warn("Trying to color unknown object at 0x%08lx as %s\n",
-                             ptr,
+                             addr,
                              (color == KMEMLEAK_GREY) ? "Grey" :
                              (color == KMEMLEAK_BLACK) ? "Black" : "Unknown");
                return;

Comments

Andrey Konovalov Feb. 8, 2019, 5:15 p.m. UTC | #1
On Fri, Feb 8, 2019 at 5:16 AM Qian Cai <cai@lca.pw> wrote:
>
> Kmemleak is totally busted with CONFIG_KASAN_SW_TAGS=y because most of tracking
> object pointers passed to create_object() have the upper bits set by KASAN.

Hi Qian,

Yeah, the issue is that kmemleak performs a bunch of pointer
comparisons that break when pointers are tagged. Try the attached
patch, it should fix the issue. I don't like the way this patch does
it though, I'll see if I can come up with something better.

Thanks for the report!

> However, even after applied this patch [1] to fix a few things, it still has
> many errors during boot.
>
> https://git.sr.ht/~cai/linux-debug/tree/master/dmesg
>
> What I don't understand is that even the patch did call kasan_reset_tag() in
> paint_ptr(), it still complained on objects with upper bits set which indicates
> that this line did not run.
>
> return (__s64)(value << shift) >> shift;
>
> [   42.462799] kmemleak: Trying to color unknown object at 0xffff80082df80000 as
> Grey
> [   42.470524] CPU: 128 PID: 1 Comm: swapper/0 Not tainted 5.0.0-rc5+ #17
> [   42.477153] Call trace:
> [   42.479639]  dump_backtrace+0x0/0x450
> [   42.483362]  show_stack+0x20/0x2c
> [   42.486733]  __dump_stack+0x20/0x28
> [   42.490276]  dump_stack+0xa0/0xfc
> [   42.493649]  paint_ptr+0xa8/0xf4
> [   42.496934]  kmemleak_not_leak+0xa4/0x15c
> [   42.501013]  init_section_page_ext+0x1bc/0x328
> [   42.505528]  page_ext_init+0x4dc/0x75c
> [   42.509336]  kernel_init_freeable+0x684/0x1104
> [   42.513857]  kernel_init+0x18/0x2a4
> [   42.517407]  ret_from_fork+0x10/0x18
>
> [1]
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index f9d9dc250428..70343d887f34 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -588,7 +588,7 @@ static struct kmemleak_object *create_object(unsigned long
> ptr, size_t size,
>         spin_lock_init(&object->lock);
>         atomic_set(&object->use_count, 1);
>         object->flags = OBJECT_ALLOCATED;
> -       object->pointer = ptr;
> +       object->pointer = (unsigned long)kasan_reset_tag((void *)ptr);
>         object->size = size;
>         object->excess_ref = 0;
>         object->min_count = min_count;
> @@ -748,11 +748,12 @@ static void paint_it(struct kmemleak_object *object, int
> color)
>  static void paint_ptr(unsigned long ptr, int color)
>  {
>         struct kmemleak_object *object;
> +       unsigned long addr = (unsigned long)kasan_reset_tag((void *)ptr);
>
> -       object = find_and_get_object(ptr, 0);
> +       object = find_and_get_object(addr, 0);
>         if (!object) {
>                 kmemleak_warn("Trying to color unknown object at 0x%08lx as %s\n",
> -                             ptr,
> +                             addr,
>                               (color == KMEMLEAK_GREY) ? "Grey" :
>                               (color == KMEMLEAK_BLACK) ? "Black" : "Unknown");
>                 return;
>
>
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index f9d9dc250428..5354e74f0d19 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -437,6 +437,8 @@ static struct kmemleak_object *lookup_object(unsigned long ptr, int alias)
 {
 	struct rb_node *rb = object_tree_root.rb_node;
 
+	ptr = (unsigned long)kasan_reset_tag((void *)ptr);
+
 	while (rb) {
 		struct kmemleak_object *object =
 			rb_entry(rb, struct kmemleak_object, rb_node);
@@ -575,6 +577,8 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	struct kmemleak_object *object, *parent;
 	struct rb_node **link, *rb_parent;
 
+	ptr = (unsigned long)kasan_reset_tag((void *)ptr);
+
 	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {
 		pr_warn("Cannot allocate a kmemleak_object structure\n");
@@ -701,6 +705,8 @@ static void delete_object_part(unsigned long ptr, size_t size)
 	struct kmemleak_object *object;
 	unsigned long start, end;
 
+	ptr = (unsigned long)kasan_reset_tag((void *)ptr);
+
 	object = find_and_remove_object(ptr, 1);
 	if (!object) {
 #ifdef DEBUG
@@ -789,6 +795,8 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 	struct kmemleak_object *object;
 	struct kmemleak_scan_area *area;
 
+	ptr = (unsigned long)kasan_reset_tag((void *)ptr);
+
 	object = find_and_get_object(ptr, 1);
 	if (!object) {
 		kmemleak_warn("Adding scan area to unknown object at 0x%08lx\n",
@@ -1334,6 +1342,9 @@ static void scan_block(void *_start, void *_end,
 	unsigned long *end = _end - (BYTES_PER_POINTER - 1);
 	unsigned long flags;
 
+	start = (unsigned long *)kasan_reset_tag((void *)start);
+	end = (unsigned long *)kasan_reset_tag((void *)end);
+
 	read_lock_irqsave(&kmemleak_lock, flags);
 	for (ptr = start; ptr < end; ptr++) {
 		struct kmemleak_object *object;
@@ -1344,7 +1355,7 @@ static void scan_block(void *_start, void *_end,
 			break;
 
 		kasan_disable_current();
-		pointer = *ptr;
+		pointer = (unsigned long)kasan_reset_tag((void *)*ptr);
 		kasan_enable_current();
 
 		if (pointer < min_addr || pointer >= max_addr)
Qian Cai Feb. 9, 2019, 2:17 a.m. UTC | #2
On 2/8/19 12:15 PM, Andrey Konovalov wrote:
> Yeah, the issue is that kmemleak performs a bunch of pointer
> comparisons that break when pointers are tagged. Try the attached
> patch, it should fix the issue. I don't like the way this patch does
> it though, I'll see if I can come up with something better.

The patch works great!
Catalin Marinas Feb. 11, 2019, 12:15 p.m. UTC | #3
On Fri, Feb 08, 2019 at 06:15:02PM +0100, Andrey Konovalov wrote:
> On Fri, Feb 8, 2019 at 5:16 AM Qian Cai <cai@lca.pw> wrote:
> > Kmemleak is totally busted with CONFIG_KASAN_SW_TAGS=y because most of tracking
> > object pointers passed to create_object() have the upper bits set by KASAN.
> 
> Yeah, the issue is that kmemleak performs a bunch of pointer
> comparisons that break when pointers are tagged.

Does it mean that the kmemleak API receives pointer aliases (i.e. same
object tagged with different values or tagged/untagged)?
Andrey Konovalov Feb. 11, 2019, 6:52 p.m. UTC | #4
On Mon, Feb 11, 2019 at 1:16 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Feb 08, 2019 at 06:15:02PM +0100, Andrey Konovalov wrote:
> > On Fri, Feb 8, 2019 at 5:16 AM Qian Cai <cai@lca.pw> wrote:
> > > Kmemleak is totally busted with CONFIG_KASAN_SW_TAGS=y because most of tracking
> > > object pointers passed to create_object() have the upper bits set by KASAN.
> >
> > Yeah, the issue is that kmemleak performs a bunch of pointer
> > comparisons that break when pointers are tagged.
>
> Does it mean that the kmemleak API receives pointer aliases (i.e. same
> object tagged with different values or tagged/untagged)?

Please disregard that comment. This is a bug in KASAN, will send a fix soon.

Patch
diff mbox series

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index f9d9dc250428..70343d887f34 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -588,7 +588,7 @@  static struct kmemleak_object *create_object(unsigned long
ptr, size_t size,
        spin_lock_init(&object->lock);
        atomic_set(&object->use_count, 1);
        object->flags = OBJECT_ALLOCATED;
-       object->pointer = ptr;
+       object->pointer = (unsigned long)kasan_reset_tag((void *)ptr);
        object->size = size;
        object->excess_ref = 0;