| Message ID | 20220423182410.1841114-1-matthieu.baerts@tessares.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/pm: fix false positive kmemleak report in msr_build_context() | expand |
On 4/23/2022 8:24 PM, Matthieu Baerts wrote: > Since commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume"), > kmemleak reports this issue: > > unreferenced object 0xffff888009cedc00 (size 256): > comm "swapper/0", pid 1, jiffies 4294693823 (age 73.764s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 48 00 00 00 00 00 00 00 ........H....... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > msr_build_context (include/linux/slab.h:621) > pm_check_save_msr (arch/x86/power/cpu.c:520) > do_one_initcall (init/main.c:1298) > kernel_init_freeable (init/main.c:1370) > kernel_init (init/main.c:1504) > ret_from_fork (arch/x86/entry/entry_64.S:304) > > It is easy to reproduce it on my side: > > - boot the VM with a debug kernel config (see the 'Closes:' tag) > - wait ~1 minute > - start a kmemleak scan > > It seems kmemleak has an issue with the array allocated in > msr_build_context(). This array is assigned to a pointer in a static > structure (saved_context.saved_msrs->array): there is no leak then. > > A simple fix for this issue would be to use kmemleak_no_leak() but Mat > noticed that the root cause here is alignment within the packed 'struct > saved_context' (from suspend_64.h). Kmemleak only searches for pointers > that are aligned (see how pointers are scanned in kmemleak.c), but > pahole shows that the saved_msrs struct member and all members after it > in the structure are unaligned: > > struct saved_context { > struct pt_regs regs; /* 0 168 */ > /* --- cacheline 2 boundary (128 bytes) was 40 bytes ago --- */ > u16 ds; /* 168 2 */ > u16 es; /* 170 2 */ > u16 fs; /* 172 2 */ > u16 gs; /* 174 2 */ > long unsigned int kernelmode_gs_base; /* 176 8 */ > long unsigned int usermode_gs_base; /* 184 8 */ > /* --- cacheline 3 boundary (192 bytes) --- */ > long unsigned int fs_base; /* 192 8 */ > long unsigned int cr0; /* 200 8 */ > long unsigned int cr2; /* 208 8 */ > long unsigned int cr3; /* 216 8 */ > long unsigned int cr4; /* 224 8 */ > u64 misc_enable; /* 232 8 */ > bool misc_enable_saved; /* 240 1 */ > > /* Note below odd offset values for the remainder of this struct */ > > struct saved_msrs saved_msrs; /* 241 16 */ > /* --- cacheline 4 boundary (256 bytes) was 1 bytes ago --- */ > long unsigned int efer; /* 257 8 */ > u16 gdt_pad; /* 265 2 */ > struct desc_ptr gdt_desc; /* 267 10 */ > u16 idt_pad; /* 277 2 */ > struct desc_ptr idt; /* 279 10 */ > u16 ldt; /* 289 2 */ > u16 tss; /* 291 2 */ > long unsigned int tr; /* 293 8 */ > long unsigned int safety; /* 301 8 */ > long unsigned int return_address; /* 309 8 */ > > /* size: 317, cachelines: 5, members: 25 */ > /* last cacheline: 61 bytes */ > } __attribute__((__packed__)); > > By moving 'misc_enable_saved' to the end of the struct declaration, > 'saved_msrs' fits in before the cacheline 4 boundary and the kmemleak > warning goes away. > > The comment above the 'saved_context' declaration says to check > wakeup_64.S file and __save/__restore_processor_state() if the struct is > modified: it looks like it's the members before 'misc_enable' that must > be carefully placed. > > At the end, the false positive kmemleak report is due to a limitation > from kmemleak but that's always good to avoid unaligned member for > optimisation purposes. > > Please note that it looks like this issue is not new, e.g. > > https://lore.kernel.org/all/9f1bb619-c4ee-21c4-a251-870bd4db04fa@lwfinger.net/ > https://lore.kernel.org/all/94e48fcd-1dbd-ebd2-4c91-f39941735909@molgen.mpg.de/ > > But on my side, msr_build_context() is only used since: > > commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume"). > > Others probably have the same issue since: > > commit 7a9c2dd08ead ("x86/pm: Introduce quirk framework to save/restore extra MSR registers around suspend/resume"), > > Hence the 'Fixes' tag here below to help with the backports. > > Fixes: 7a9c2dd08ead ("x86/pm: Introduce quirk framework to save/restore extra MSR registers around suspend/resume") > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/268 > Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> All good AFAICS. Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > arch/x86/include/asm/suspend_32.h | 2 +- > arch/x86/include/asm/suspend_64.h | 12 ++++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h > index 7b132d0312eb..a800abb1a992 100644 > --- a/arch/x86/include/asm/suspend_32.h > +++ b/arch/x86/include/asm/suspend_32.h > @@ -19,7 +19,6 @@ struct saved_context { > u16 gs; > unsigned long cr0, cr2, cr3, cr4; > u64 misc_enable; > - bool misc_enable_saved; > struct saved_msrs saved_msrs; > struct desc_ptr gdt_desc; > struct desc_ptr idt; > @@ -28,6 +27,7 @@ struct saved_context { > unsigned long tr; > unsigned long safety; > unsigned long return_address; > + bool misc_enable_saved; > } __attribute__((packed)); > > /* routines for saving/restoring kernel state */ > diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h > index 35bb35d28733..bb7023dbf524 100644 > --- a/arch/x86/include/asm/suspend_64.h > +++ b/arch/x86/include/asm/suspend_64.h > @@ -14,9 +14,13 @@ > * Image of the saved processor state, used by the low level ACPI suspend to > * RAM code and by the low level hibernation code. > * > - * If you modify it, fix arch/x86/kernel/acpi/wakeup_64.S and make sure that > - * __save/__restore_processor_state(), defined in arch/x86/kernel/suspend_64.c, > - * still work as required. > + * If you modify it before 'misc_enable', fix arch/x86/kernel/acpi/wakeup_64.S > + * and make sure that __save/__restore_processor_state(), defined in > + * arch/x86/kernel/suspend_64.c, still work as required. > + * > + * Because the structure is packed, make sure to avoid unaligned members. For > + * optimisations purposes but also because tools like Kmemleak only search for > + * pointers that are aligned. > */ > struct saved_context { > struct pt_regs regs; > @@ -36,7 +40,6 @@ struct saved_context { > > unsigned long cr0, cr2, cr3, cr4; > u64 misc_enable; > - bool misc_enable_saved; > struct saved_msrs saved_msrs; > unsigned long efer; > u16 gdt_pad; /* Unused */ > @@ -48,6 +51,7 @@ struct saved_context { > unsigned long tr; > unsigned long safety; > unsigned long return_address; > + bool misc_enable_saved; > } __attribute__((packed)); > > #define loaddebug(thread,register) \
On Sat, Apr 23, 2022 at 08:24:10PM +0200, Matthieu Baerts wrote: > diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h > index 35bb35d28733..bb7023dbf524 100644 > --- a/arch/x86/include/asm/suspend_64.h > +++ b/arch/x86/include/asm/suspend_64.h > @@ -14,9 +14,13 @@ > * Image of the saved processor state, used by the low level ACPI suspend to > * RAM code and by the low level hibernation code. > * > - * If you modify it, fix arch/x86/kernel/acpi/wakeup_64.S and make sure that > - * __save/__restore_processor_state(), defined in arch/x86/kernel/suspend_64.c, > - * still work as required. > + * If you modify it before 'misc_enable', fix arch/x86/kernel/acpi/wakeup_64.S Why does before misc_enable matter? arch/x86/kernel/asm-offsets_64.c computes the offsets and there is a member like saved_context_gdt_desc which will get moved after your change but that's not a problem because the offset will get recomputed at build time. Hm?
On Tue, Apr 26, 2022 at 5:22 PM Borislav Petkov <bp@alien8.de> wrote: > > On Sat, Apr 23, 2022 at 08:24:10PM +0200, Matthieu Baerts wrote: > > diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h > > index 35bb35d28733..bb7023dbf524 100644 > > --- a/arch/x86/include/asm/suspend_64.h > > +++ b/arch/x86/include/asm/suspend_64.h > > @@ -14,9 +14,13 @@ > > * Image of the saved processor state, used by the low level ACPI suspend to > > * RAM code and by the low level hibernation code. > > * > > - * If you modify it, fix arch/x86/kernel/acpi/wakeup_64.S and make sure that > > - * __save/__restore_processor_state(), defined in arch/x86/kernel/suspend_64.c, > > - * still work as required. > > + * If you modify it before 'misc_enable', fix arch/x86/kernel/acpi/wakeup_64.S > > Why does before misc_enable matter? > > arch/x86/kernel/asm-offsets_64.c computes the offsets and there is a > member like saved_context_gdt_desc which will get moved after your > change but that's not a problem because the offset will get recomputed > at build time. > > Hm? So can the comment be dropped entirely?
On Tue, Apr 26, 2022 at 06:24:04PM +0200, Rafael J. Wysocki wrote:
> So can the comment be dropped entirely?
Looks like it to me. All the accesses in wakeup_64.S are done through
those offsets which are computed at build-time so they should always be
valid.
OTOH, I wouldn't mind having there some text making any future person
touching this, aware of where to look when making changes.
Some changes like removing a struct member are nicely caught, ofc,
see below. But for something else which is a lot more subtle having a
comment say "hey, have a look at where this is used in wakeup_64.S and
make sure everything is still kosher" is better than having no comment
at all. IMHO.
Thx.
In file included from arch/x86/kernel/asm-offsets.c:14:
arch/x86/kernel/asm-offsets_64.c: In function ‘main’:
./include/linux/stddef.h:16:33: error: ‘struct saved_context’ has no member named ‘gdt_desc’
16 | #define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
| ^~~~~~~~~~~~~~~~~~
./include/linux/kbuild.h:6:69: note: in definition of macro ‘DEFINE’
6 | asm volatile("\n.ascii \"->" #sym " %0 " #val "\"" : : "i" (val))
| ^~~
./include/linux/kbuild.h:11:21: note: in expansion of macro ‘offsetof’
11 | DEFINE(sym, offsetof(struct str, mem))
|
Hi Borislav, Rafael, Thank you for your reviews! On 26/04/2022 19:27, Borislav Petkov wrote: > On Tue, Apr 26, 2022 at 06:24:04PM +0200, Rafael J. Wysocki wrote: >> So can the comment be dropped entirely? > > Looks like it to me. All the accesses in wakeup_64.S are done through > those offsets which are computed at build-time so they should always be > valid. > > OTOH, I wouldn't mind having there some text making any future person > touching this, aware of where to look when making changes. > > Some changes like removing a struct member are nicely caught, ofc, > see below. But for something else which is a lot more subtle having a > comment say "hey, have a look at where this is used in wakeup_64.S and > make sure everything is still kosher" is better than having no comment > at all. IMHO. Good point, let me update the comment and the commit message in a new v3. Cheers, Matt
diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h index 7b132d0312eb..a800abb1a992 100644 --- a/arch/x86/include/asm/suspend_32.h +++ b/arch/x86/include/asm/suspend_32.h @@ -19,7 +19,6 @@ struct saved_context { u16 gs; unsigned long cr0, cr2, cr3, cr4; u64 misc_enable; - bool misc_enable_saved; struct saved_msrs saved_msrs; struct desc_ptr gdt_desc; struct desc_ptr idt; @@ -28,6 +27,7 @@ struct saved_context { unsigned long tr; unsigned long safety; unsigned long return_address; + bool misc_enable_saved; } __attribute__((packed)); /* routines for saving/restoring kernel state */ diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h index 35bb35d28733..bb7023dbf524 100644 --- a/arch/x86/include/asm/suspend_64.h +++ b/arch/x86/include/asm/suspend_64.h @@ -14,9 +14,13 @@ * Image of the saved processor state, used by the low level ACPI suspend to * RAM code and by the low level hibernation code. * - * If you modify it, fix arch/x86/kernel/acpi/wakeup_64.S and make sure that - * __save/__restore_processor_state(), defined in arch/x86/kernel/suspend_64.c, - * still work as required. + * If you modify it before 'misc_enable', fix arch/x86/kernel/acpi/wakeup_64.S + * and make sure that __save/__restore_processor_state(), defined in + * arch/x86/kernel/suspend_64.c, still work as required. + * + * Because the structure is packed, make sure to avoid unaligned members. For + * optimisations purposes but also because tools like Kmemleak only search for + * pointers that are aligned. */ struct saved_context { struct pt_regs regs; @@ -36,7 +40,6 @@ struct saved_context { unsigned long cr0, cr2, cr3, cr4; u64 misc_enable; - bool misc_enable_saved; struct saved_msrs saved_msrs; unsigned long efer; u16 gdt_pad; /* Unused */ @@ -48,6 +51,7 @@ struct saved_context { unsigned long tr; unsigned long safety; unsigned long return_address; + bool misc_enable_saved; } __attribute__((packed)); #define loaddebug(thread,register) \
Since commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume"), kmemleak reports this issue: unreferenced object 0xffff888009cedc00 (size 256): comm "swapper/0", pid 1, jiffies 4294693823 (age 73.764s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 48 00 00 00 00 00 00 00 ........H....... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: msr_build_context (include/linux/slab.h:621) pm_check_save_msr (arch/x86/power/cpu.c:520) do_one_initcall (init/main.c:1298) kernel_init_freeable (init/main.c:1370) kernel_init (init/main.c:1504) ret_from_fork (arch/x86/entry/entry_64.S:304) It is easy to reproduce it on my side: - boot the VM with a debug kernel config (see the 'Closes:' tag) - wait ~1 minute - start a kmemleak scan It seems kmemleak has an issue with the array allocated in msr_build_context(). This array is assigned to a pointer in a static structure (saved_context.saved_msrs->array): there is no leak then. A simple fix for this issue would be to use kmemleak_no_leak() but Mat noticed that the root cause here is alignment within the packed 'struct saved_context' (from suspend_64.h). Kmemleak only searches for pointers that are aligned (see how pointers are scanned in kmemleak.c), but pahole shows that the saved_msrs struct member and all members after it in the structure are unaligned: struct saved_context { struct pt_regs regs; /* 0 168 */ /* --- cacheline 2 boundary (128 bytes) was 40 bytes ago --- */ u16 ds; /* 168 2 */ u16 es; /* 170 2 */ u16 fs; /* 172 2 */ u16 gs; /* 174 2 */ long unsigned int kernelmode_gs_base; /* 176 8 */ long unsigned int usermode_gs_base; /* 184 8 */ /* --- cacheline 3 boundary (192 bytes) --- */ long unsigned int fs_base; /* 192 8 */ long unsigned int cr0; /* 200 8 */ long unsigned int cr2; /* 208 8 */ long unsigned int cr3; /* 216 8 */ long unsigned int cr4; /* 224 8 */ u64 misc_enable; /* 232 8 */ bool misc_enable_saved; /* 240 1 */ /* Note below odd offset values for the remainder of this struct */ struct saved_msrs saved_msrs; /* 241 16 */ /* --- cacheline 4 boundary (256 bytes) was 1 bytes ago --- */ long unsigned int efer; /* 257 8 */ u16 gdt_pad; /* 265 2 */ struct desc_ptr gdt_desc; /* 267 10 */ u16 idt_pad; /* 277 2 */ struct desc_ptr idt; /* 279 10 */ u16 ldt; /* 289 2 */ u16 tss; /* 291 2 */ long unsigned int tr; /* 293 8 */ long unsigned int safety; /* 301 8 */ long unsigned int return_address; /* 309 8 */ /* size: 317, cachelines: 5, members: 25 */ /* last cacheline: 61 bytes */ } __attribute__((__packed__)); By moving 'misc_enable_saved' to the end of the struct declaration, 'saved_msrs' fits in before the cacheline 4 boundary and the kmemleak warning goes away. The comment above the 'saved_context' declaration says to check wakeup_64.S file and __save/__restore_processor_state() if the struct is modified: it looks like it's the members before 'misc_enable' that must be carefully placed. At the end, the false positive kmemleak report is due to a limitation from kmemleak but that's always good to avoid unaligned member for optimisation purposes. Please note that it looks like this issue is not new, e.g. https://lore.kernel.org/all/9f1bb619-c4ee-21c4-a251-870bd4db04fa@lwfinger.net/ https://lore.kernel.org/all/94e48fcd-1dbd-ebd2-4c91-f39941735909@molgen.mpg.de/ But on my side, msr_build_context() is only used since: commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume"). Others probably have the same issue since: commit 7a9c2dd08ead ("x86/pm: Introduce quirk framework to save/restore extra MSR registers around suspend/resume"), Hence the 'Fixes' tag here below to help with the backports. Fixes: 7a9c2dd08ead ("x86/pm: Introduce quirk framework to save/restore extra MSR registers around suspend/resume") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/268 Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> --- arch/x86/include/asm/suspend_32.h | 2 +- arch/x86/include/asm/suspend_64.h | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)