[RFC] make kmemleak scan __ro_after_init section (was: Re: [PATCH 0/5] genetlink improvements)
From: Jakub Kicinski
Date: Wed Nov 02 2016 - 19:48:08 EST
On Wed, 2 Nov 2016 13:30:34 -0700, Cong Wang wrote:
> On Tue, Nov 1, 2016 at 11:56 AM, Jakub Kicinski <kubakici@xxxxx> wrote:
> > On Tue, 1 Nov 2016 11:32:52 -0700, Cong Wang wrote:
> >> On Tue, Nov 1, 2016 at 10:28 AM, Jakub Kicinski <kubakici@xxxxx> wrote:
> >> > unreferenced object 0xffff8807389cba28 (size 128):
> >> > comm "swapper/0", pid 1, jiffies 4294898463 (age 781.332s)
> >> > hex dump (first 32 bytes):
> >> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> >> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> >> > backtrace:
> >> > [<ffffffff85decad8>] kmemleak_alloc+0x28/0x50
> >> > [<ffffffff84771246>] __kmalloc+0x206/0x5a0
> >> > [<ffffffff859e1261>] genl_register_family+0x711/0x11d0
> >> > [<ffffffff888d9524>] netlbl_mgmt_genl_init+0x10/0x12
> >> > [<ffffffff888d91e8>] netlbl_netlink_init+0x9/0x26
> >> > [<ffffffff888d9254>] netlbl_init+0x4f/0x85
> >> > [<ffffffff840022b7>] do_one_initcall+0xb7/0x2a0
> >> > [<ffffffff887f9102>] kernel_init_freeable+0x597/0x636
> >> > [<ffffffff85de7793>] kernel_init+0x13/0x140
> >> > [<ffffffff85e0246a>] ret_from_fork+0x2a/0x40
> >>
> >> Looks like we are missing a kfree(family->attrbuf); on error path,
> >> but it is not related to Johannes' recent patches.
> >>
> >> Could the attached patch help?
> >
> > Still there:
> >
> > unreferenced object 0xffff88073fb204e8 (size 64):
> > comm "swapper/0", pid 1, jiffies 4294898455 (age 88.528s)
> > hex dump (first 32 bytes):
> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > backtrace:
> > [<ffffffff93decbf8>] kmemleak_alloc+0x28/0x50
> > [<ffffffff92771246>] __kmalloc+0x206/0x5a0
> > [<ffffffff939e1471>] genl_register_family+0x921/0x1270
> > [<ffffffff968d0ecf>] genl_init+0x11/0x43
> > [<ffffffff920022b7>] do_one_initcall+0xb7/0x2a0
> > [<ffffffff967f9102>] kernel_init_freeable+0x597/0x636
> > [<ffffffff93de78b3>] kernel_init+0x13/0x140
> > [<ffffffff93e0256a>] ret_from_fork+0x2a/0x40
> > [<ffffffffffffffff>] 0xffffffffffffffff
> >
> > etc.
>
> Interesting, from the size it does look like we are leaking family->attrbuf,
> but I don't see other cases could leak it except the error path I fixed.
>
> Mind doing a quick bisect?
Thanks for looking into this! Bisect led me to the following commit:
commit 56989f6d8568c21257dcec0f5e644d5570ba3281
Author: Johannes Berg <johannes.berg@xxxxxxxxx>
Date: Mon Oct 24 14:40:05 2016 +0200
genetlink: mark families as __ro_after_init
Now genl_register_family() is the only thing (other than the
users themselves, perhaps, but I didn't find any doing that)
writing to the family struct.
In all families that I found, genl_register_family() is only
called from __init functions (some indirectly, in which case
I've add __init annotations to clarifly things), so all can
actually be marked __ro_after_init.
This protects the data structure from accidental corruption.
Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx>
Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
I realized that kmemleak is not scanning the __ro_after_init section...
Following patch solves the false positives but I wonder if it's the
right/acceptable solution.
--->8----------------------------------------------------------------
diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index 000e6e91f6a0..841579932c52 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -62,9 +62,11 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
__start_ro_after_init = .;
+ VMLINUX_SYMBOL(__start_data_ro_after_init) = .;
.data..ro_after_init : {
*(.data..ro_after_init)
}
+ VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
EXCEPTION_TABLE(16)
. = ALIGN(PAGE_SIZE);
__end_ro_after_init = .;
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index af0254c09424..4df64a1fc09e 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -14,6 +14,8 @@
* [_sdata, _edata]: contains .data.* sections, may also contain .rodata.*
* and/or .init.* sections.
* [__start_rodata, __end_rodata]: contains .rodata.* sections
+ * [__start_data_ro_after_init, __end_data_ro_after_init]:
+ * contains data.ro_after_init section
* [__init_begin, __init_end]: contains .init.* sections, but .init.text.*
* may be out of this range on some architectures.
* [_sinittext, _einittext]: contains .init.text.* sections
@@ -31,6 +33,7 @@
extern char __bss_start[], __bss_stop[];
extern char __init_begin[], __init_end[];
extern char _sinittext[], _einittext[];
+extern char __start_data_ro_after_init[], __end_data_ro_after_init[];
extern char _end[];
extern char __per_cpu_load[], __per_cpu_start[], __per_cpu_end[];
extern char __kprobes_text_start[], __kprobes_text_end[];
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 30747960bc54..71c75fb64945 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -259,7 +259,10 @@
* own by defining an empty RO_AFTER_INIT_DATA.
*/
#ifndef RO_AFTER_INIT_DATA
-#define RO_AFTER_INIT_DATA *(.data..ro_after_init)
+#define RO_AFTER_INIT_DATA \
+ VMLINUX_SYMBOL(__start_data_ro_after_init) = .; \
+ *(.data..ro_after_init) \
+ VMLINUX_SYMBOL(__end_data_ro_after_init) = .;
#endif
/*
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index e5355a5b423f..d1380ed93fdf 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1414,6 +1414,7 @@ static void kmemleak_scan(void)
/* data/bss scanning */
scan_large_block(_sdata, _edata);
scan_large_block(__bss_start, __bss_stop);
+ scan_large_block(__start_data_ro_after_init, __end_data_ro_after_init);
#ifdef CONFIG_SMP
/* per-cpu sections scanning */