* [PATCH v3] x86/pm: fix false positive kmemleak report in msr_build_context()
@ 2022-04-26 20:21 Matthieu Baerts
2022-04-27 16:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 2+ messages in thread
From: Matthieu Baerts @ 2022-04-26 20:21 UTC (permalink / raw)
To: Rafael J. Wysocki, Pavel Machek, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Chen Yu
Cc: Pawan Gupta, Catalin Marinas, linux-mm, Matthieu Baerts,
Mat Martineau, Rafael J. Wysocki, Ingo Molnar, linux-pm,
linux-kernel
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 fix
wakeup_64.S file and __save/__restore_processor_state() if the struct is
modified: it looks like all the accesses in wakeup_64.S are done through
offsets which are computed at build-time. This comment has been updated
accordingly.
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>
---
Notes:
v3:
- update the comment above 'saved_context' structure (Borislav)
v2:
- update 'saved_context' structure instead of using kmemleak_no_leak() (Mat)
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..0dc400fae1b2 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, check how it is used in arch/x86/kernel/acpi/wakeup_64.S
+ * and make sure that __save/__restore_processor_state(), defined in
+ * arch/x86/power/cpu.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) \
--
2.34.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] x86/pm: fix false positive kmemleak report in msr_build_context()
2022-04-26 20:21 [PATCH v3] x86/pm: fix false positive kmemleak report in msr_build_context() Matthieu Baerts
@ 2022-04-27 16:33 ` Rafael J. Wysocki
0 siblings, 0 replies; 2+ messages in thread
From: Rafael J. Wysocki @ 2022-04-27 16:33 UTC (permalink / raw)
To: Matthieu Baerts
Cc: Rafael J. Wysocki, Pavel Machek, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, the arch/x86 maintainers,
H. Peter Anvin, Chen Yu, Pawan Gupta, Catalin Marinas,
Linux Memory Management List, Mat Martineau, Rafael J. Wysocki,
Ingo Molnar, Linux PM, Linux Kernel Mailing List
On Tue, Apr 26, 2022 at 10:22 PM Matthieu Baerts
<matthieu.baerts@tessares.net> 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 fix
> wakeup_64.S file and __save/__restore_processor_state() if the struct is
> modified: it looks like all the accesses in wakeup_64.S are done through
> offsets which are computed at build-time. This comment has been updated
> accordingly.
>
> 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>
Still good IMV.
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> Notes:
> v3:
> - update the comment above 'saved_context' structure (Borislav)
> v2:
> - update 'saved_context' structure instead of using kmemleak_no_leak() (Mat)
>
> 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..0dc400fae1b2 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, check how it is used in arch/x86/kernel/acpi/wakeup_64.S
> + * and make sure that __save/__restore_processor_state(), defined in
> + * arch/x86/power/cpu.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) \
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-04-27 16:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-26 20:21 [PATCH v3] x86/pm: fix false positive kmemleak report in msr_build_context() Matthieu Baerts
2022-04-27 16:33 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).