tracing: kmemleak warning in allocate_cmdlines_buffer()

regression Kalle Valo 2 participants 5 messages Feb 14, 2024

Subsystems (2)

Tracing Regressions

lore

Share & tools

lore

Kalle Valo

author

2024-02-14 12:51 UTC #0 lore raw

Hi Steven,

I upgraded our ath11k test setup to v6.8-rc4 and noticed a new kmemleak warning in the log:

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

I don't see this warning in v6.8-rc3 and also reverting commit 44dc5c41b5b1 ("tracing: Fix wasted memory in saved_cmdlines logic") makes the warning go away. Let me know if you need more info or help with testing, I see the warning every time so it's easy to reproduce.

Kalle

Steven Rostedt

maintainer

2024-02-14 14:44 UTC #1 lore raw

Quoted 4 lines · On Wed, 14 Feb 2024 14:50:56 +0200 Kalle Valo <[email protected]> wrote:

Hi Steven,
I upgraded our ath11k test setup to v6.8-rc4 and noticed a new kmemleak warning in the log:

Thanks for the report.

Quoted 18 lines

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

Quoted 5 lines

I don't see this warning in v6.8-rc3 and also reverting commit 44dc5c41b5b1 ("tracing: Fix wasted memory in saved_cmdlines logic") makes the warning go away. Let me know if you need more info or help with testing, I see the warning every time so it's easy to reproduce.

Hmm, I changed the code a bit and I wonder if this is a false positive?

Instead of allocating the structure via kmalloc() I now use it as part of a page.

That is, the old code had:

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

Where as the new code has:

	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);

Does kmemleak handle structures that are assigned to alloc_pages() allocations? I don't think it does.

I think we need to inform kmemleak about this. Does the following patch fix this for you?

-- Steve

kernel/trace/trace_sched_switch.c +3 -0

index e4fbcc3bede5..de4182224ea2 100644
@@ -9,6 +9,7 @@
 #include <linux/kallsyms.h>
 #include <linux/uaccess.h>
 #include <linux/ftrace.h>
+#include <linux/kmemleak.h>
 #include <trace/events/sched.h>
 
 #include "trace.h"

@@ -190,6 +191,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);
 }

@@ -210,6 +212,7 @@ static struct saved_cmdlines_buffer *allocate_cmdlines_buffer(unsigned int val)
 
 	s = page_address(page);
 	memset(s, 0, sizeof(*s));
+	kmemleak_alloc(s, size, 1, GFP_KERNEL);
 
 	/* Round up to actual allocation */
 	val = (size - sizeof(*s)) / TASK_COMM_LEN;

Kalle Valo

author Tested-by

2024-02-14 15:30 UTC #2 lore raw

Quoted 59 lines · Steven Rostedt

Quoted 4 lines · On Wed, 14 Feb 2024 14:50:56 +0200 Kalle Valo <[email protected]> wrote:
Hi Steven,
I upgraded our ath11k test setup to v6.8-rc4 and noticed a new kmemleak warning in the log:
Thanks for the report.
Quoted 18 lines
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
Quoted 5 lines
I don't see this warning in v6.8-rc3 and also reverting commit 44dc5c41b5b1 ("tracing: Fix wasted memory in saved_cmdlines logic") makes the warning go away. Let me know if you need more info or help with testing, I see the warning every time so it's easy to reproduce.
Hmm, I changed the code a bit and I wonder if this is a false positive?
Instead of allocating the structure via kmalloc() I now use it as part of a page.
That is, the old code had:
        s = kmalloc(sizeof(*s), GFP_KERNEL);
	s->saved_cmdlines = kmalloc_array(TASK_COMM_LEN, val, GFP_KERNEL);
Where as the new code has:
	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);

Quoted 5 lines

Does kmemleak handle structures that are assigned to alloc_pages() allocations? I don't think it does.
I think we need to inform kmemleak about this. Does the following patch fix this for you?

It does, thank you!

Tested-by Kalle Valo <[email protected]>

kernel/trace/trace_sched_switch.c

Quoted 4 lines

kernel/trace/trace_sched_switch.c +0 -0
index e4fbcc3bede5..de4182224ea2 100644

Although the patch didn't apply for me as in my tree the functions are in kernel/trace/trace.c. I don't know what happened so as a quick hack I just manually added the three lines to my version of trace.c. Let me know if there's a git tree or branch you would like me to test, I can do that easily.

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Steven Rostedt

maintainer

2024-02-14 15:41 UTC #3 lore raw

Quoted 6 lines · On Wed, 14 Feb 2024 17:30:40 +0200 Kalle Valo <[email protected]> wrote:

Although the patch didn't apply for me as in my tree the functions are in kernel/trace/trace.c. I don't know what happened so as a quick hack I just manually added the three lines to my version of trace.c. Let me know if there's a git tree or branch you would like me to test, I can do that easily.

Oops, I was in a middle of rebasing a patch series I was reviewing and forgot there was some changes to the code around it. Anyway, that should be the fix. I'll add your tested-by.

Thanks!

-- Steve

Steven Rostedt

maintainer

2024-02-14 16:20 UTC #4 lore raw

In reply to Kalle Valo

Quoted 5 lines · On Wed, 14 Feb 2024 17:30:40 +0200 Kalle Valo <[email protected]> wrote:

Although the patch didn't apply for me as in my tree the functions are in kernel/trace/trace.c. I don't know what happened so as a quick hack I just manually added the three lines to my version of trace.c. Let me know if there's a git tree or branch you would like me to test, I can do that easily.

I sent out a v2 that should apply and I added your Tested-by tag. But you may want to verify that it does indeed still work, as I decided to swap the order of adding kmemleak_alloc() with the memset(), so it's not exactly the same.

Thanks,

-- Steve