diff mbox series

[v2] tracing: Inform kmemleak of saved_cmdlines allocation

Message ID 20240214112046.09a322d6@gandalf.local.home
State New
Headers
Series [v2] tracing: Inform kmemleak of saved_cmdlines allocation |

Commit Message

Steven Rostedt Feb. 14, 2024, 4:20 p.m. UTC 
  From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The allocation of the struct saved_cmdlines_buffer structure changed from:

        s = kmalloc(sizeof(*s), GFP_KERNEL);
	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);

to:

	orig_size = sizeof(*s) + val * TASK_COMM_LEN;
	order = get_order(orig_size);
	size = 1 << (order + PAGE_SHIFT);
	page = alloc_pages(GFP_KERNEL, order);
	if (!page)
		return NULL;

	s = page_address(page);
	memset(s, 0, sizeof(*s));

	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);

Where that s->saved_cmdlines allocation looks to be a dangling allocation
to kmemleak. That's because kmemleak only keeps track of kmalloc()
allocations. For allocations that use page_alloc() directly, the kmemleak
needs to be explicitly informed about it.

Add kmemleak_alloc() and kmemleak_free() around the page allocation so
that it doesn't give the following false positive:

unreferenced object 0xffff8881010c8000 (size 32760):
  comm "swapper", pid 0, jiffies 4294667296
  hex dump (first 32 bytes):
    ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
    ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
  backtrace (crc ae6ec1b9):
    [<ffffffff86722405>] kmemleak_alloc+0x45/0x80
    [<ffffffff8414028d>] __kmalloc_large_node+0x10d/0x190
    [<ffffffff84146ab1>] __kmalloc+0x3b1/0x4c0
    [<ffffffff83ed7103>] allocate_cmdlines_buffer+0x113/0x230
    [<ffffffff88649c34>] tracer_alloc_buffers.isra.0+0x124/0x460
    [<ffffffff8864a174>] early_trace_init+0x14/0xa0
    [<ffffffff885dd5ae>] start_kernel+0x12e/0x3c0
    [<ffffffff885f5758>] x86_64_start_reservations+0x18/0x30
    [<ffffffff885f582b>] x86_64_start_kernel+0x7b/0x80
    [<ffffffff83a001c3>] secondary_startup_64_no_verify+0x15e/0x16b

Link: https://lore.kernel.org/linux-trace-kernel/87r0hfnr9r.fsf@kernel.org/

Fixes: 44dc5c41b5b1 ("tracing: Fix wasted memory in saved_cmdlines logic")
Reported-by: Kalle Valo <kvalo@kernel.org>
Tested-by: Kalle Valo <kvalo@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
Changes since v1: https://lore.kernel.org/linux-trace-kernel/20240214102614.1a1405be@gandalf.local.home/

- Rebased on my urgent branch that still has the code in trace.c and not
  my for-next local branch that moved it to trace_sched_switch.c

 kernel/trace/trace.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kalle Valo Feb. 14, 2024, 6:20 p.m. UTC | #1 
Steven Rostedt <rostedt@goodmis.org> writes:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
>
> The allocation of the struct saved_cmdlines_buffer structure changed from:
>
>         s = kmalloc(sizeof(*s), GFP_KERNEL);
> 	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);
>
> to:
>
> 	orig_size = sizeof(*s) + val * TASK_COMM_LEN;
> 	order = get_order(orig_size);
> 	size = 1 << (order + PAGE_SHIFT);
> 	page = alloc_pages(GFP_KERNEL, order);
> 	if (!page)
> 		return NULL;
>
> 	s = page_address(page);
> 	memset(s, 0, sizeof(*s));
>
> 	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);
>
> Where that s->saved_cmdlines allocation looks to be a dangling allocation
> to kmemleak. That's because kmemleak only keeps track of kmalloc()
> allocations. For allocations that use page_alloc() directly, the kmemleak
> needs to be explicitly informed about it.
>
> Add kmemleak_alloc() and kmemleak_free() around the page allocation so
> that it doesn't give the following false positive:
>
> unreferenced object 0xffff8881010c8000 (size 32760):
>   comm "swapper", pid 0, jiffies 4294667296
>   hex dump (first 32 bytes):
>     ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
>     ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
>   backtrace (crc ae6ec1b9):
>     [<ffffffff86722405>] kmemleak_alloc+0x45/0x80
>     [<ffffffff8414028d>] __kmalloc_large_node+0x10d/0x190
>     [<ffffffff84146ab1>] __kmalloc+0x3b1/0x4c0
>     [<ffffffff83ed7103>] allocate_cmdlines_buffer+0x113/0x230
>     [<ffffffff88649c34>] tracer_alloc_buffers.isra.0+0x124/0x460
>     [<ffffffff8864a174>] early_trace_init+0x14/0xa0
>     [<ffffffff885dd5ae>] start_kernel+0x12e/0x3c0
>     [<ffffffff885f5758>] x86_64_start_reservations+0x18/0x30
>     [<ffffffff885f582b>] x86_64_start_kernel+0x7b/0x80
>     [<ffffffff83a001c3>] secondary_startup_64_no_verify+0x15e/0x16b
>
> Link: https://lore.kernel.org/linux-trace-kernel/87r0hfnr9r.fsf@kernel.org/
>
> Fixes: 44dc5c41b5b1 ("tracing: Fix wasted memory in saved_cmdlines logic")
> Reported-by: Kalle Valo <kvalo@kernel.org>
> Tested-by: Kalle Valo <kvalo@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Applies cleanly to v6.8-rc4 and I don't see the leak anymore, thank you
for fixing it so quickly!

Tested-by: Kalle Valo <kvalo@kernel.org>
diff mbox series

Patch

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index aa54810e8b56..8198bfc54b58 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -39,6 +39,7 @@ 
 #include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/panic_notifier.h>
+#include <linux/kmemleak.h>
 #include <linux/poll.h>
 #include <linux/nmi.h>
 #include <linux/fs.h>
@@ -2339,6 +2340,7 @@  static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
 	int order = get_order(sizeof(*s) + s->cmdline_num * TASK_COMM_LEN);
 
 	kfree(s->map_cmdline_to_pid);
+	kmemleak_free(s);
 	free_pages((unsigned long)s, order);
 }
 
@@ -2358,6 +2360,7 @@  static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
 		return NULL;
 
 	s = page_address(page);
+	kmemleak_alloc(s, size, 1, GFP_KERNEL);
 	memset(s, 0, sizeof(*s));
 
 	/* Round up to actual allocation */