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
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.
>
> 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.
>
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
diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
index e4fbcc3bede5..de4182224ea2 100644
--- a/kernel/trace/trace_sched_switch.c
+++ b/kernel/trace/trace_sched_switch.c
@@ -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;
Steven Rostedt <[email protected]> writes:
> 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.
>
>>
>> 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.
>>
>
> 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?
It does, thank you!
Tested-by: Kalle Valo <[email protected]>
> diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
> index e4fbcc3bede5..de4182224ea2 100644
> --- a/kernel/trace/trace_sched_switch.c
> +++ b/kernel/trace/trace_sched_switch.c
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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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
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