* [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()
@ 2022-04-21 16:15 Matthieu Baerts
2022-04-21 23:20 ` Mat Martineau
0 siblings, 1 reply; 5+ messages in thread
From: Matthieu Baerts @ 2022-04-21 16:15 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, Matthieu Baerts, Ingo Molnar, Rafael J. Wysocki,
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 [1]
- wait ~1 minute
- start a kmemleak scan
It seems kmemleak has an issue with the array allocated in
msr_build_context() and assigned to a pointer in a static structure
(saved_context.saved_msrs->array): there is no leak then.
It looks like this is a limitation from kmemleak but that's alright,
kmemleak_no_leak() can be used to avoid complaining about that.
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").
Depending on their CPUs, others have probably 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. But I
understand if someone says the origin of this issue is more on
kmemleak's side. What is unclear to me is why this issue was not seen by
other people and CIs. Maybe the kernel config [1]?
[1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
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
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
arch/x86/power/cpu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 3822666fb73d..1467c6d1a966 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -14,6 +14,7 @@
#include <linux/tboot.h>
#include <linux/dmi.h>
#include <linux/pgtable.h>
+#include <linux/kmemleak.h>
#include <asm/proto.h>
#include <asm/mtrr.h>
@@ -413,6 +414,9 @@ static int msr_build_context(const u32 *msr_id, const int num)
return -ENOMEM;
}
+ /* The pointer is going to be stored in static struct (saved_context) */
+ kmemleak_not_leak(msr_array);
+
if (saved_msrs->array) {
/*
* Multiple callbacks can invoke this function, so copy any
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()
2022-04-21 16:15 [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context() Matthieu Baerts
@ 2022-04-21 23:20 ` Mat Martineau
2022-04-22 11:51 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Mat Martineau @ 2022-04-21 23:20 UTC (permalink / raw)
To: Matthieu Baerts
Cc: Rafael J. Wysocki, Pavel Machek, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Chen Yu,
Pawan Gupta, Ingo Molnar, Rafael J. Wysocki, linux-pm,
linux-kernel
On Thu, 21 Apr 2022, 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 [1]
> - wait ~1 minute
> - start a kmemleak scan
>
> It seems kmemleak has an issue with the array allocated in
> msr_build_context() and assigned to a pointer in a static structure
> (saved_context.saved_msrs->array): there is no leak then.
>
> It looks like this is a limitation from kmemleak but that's alright,
> kmemleak_no_leak() can be used to avoid complaining about that.
>
> 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").
>
> Depending on their CPUs, others have probably 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. But I
> understand if someone says the origin of this issue is more on
> kmemleak's side. What is unclear to me is why this issue was not seen by
> other people and CIs. Maybe the kernel config [1]?
>
> [1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
>
Hi Matthieu -
It looks like the root cause here is alignment within the packed struct
saved_context (from suspend_64.h). Kmemleak only searches for pointers
that are aligned, but pahole shows that the saved_msrs struct member and
all members after it in the structure are unaligned:
(gcc 11.2.1, x86_64)
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 odd offset values for the remainder of this struct vvv */
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__));
If I move 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 and __save/__restore_processor_state() if the struct is
modified - looks like it's the members before misc_enable that must be
carefully placed.
So far I've only tried this on my local machine, I'll work on getting more
thorough validation.
Looks like struct saved_context in suspend_32.h has similar odd alignment.
- Mat
> 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
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> arch/x86/power/cpu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index 3822666fb73d..1467c6d1a966 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -14,6 +14,7 @@
> #include <linux/tboot.h>
> #include <linux/dmi.h>
> #include <linux/pgtable.h>
> +#include <linux/kmemleak.h>
>
> #include <asm/proto.h>
> #include <asm/mtrr.h>
> @@ -413,6 +414,9 @@ static int msr_build_context(const u32 *msr_id, const int num)
> return -ENOMEM;
> }
>
> + /* The pointer is going to be stored in static struct (saved_context) */
> + kmemleak_not_leak(msr_array);
> +
> if (saved_msrs->array) {
> /*
> * Multiple callbacks can invoke this function, so copy any
> --
> 2.34.1
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()
2022-04-21 23:20 ` Mat Martineau
@ 2022-04-22 11:51 ` Rafael J. Wysocki
2022-04-22 12:25 ` Matthieu Baerts
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2022-04-22 11:51 UTC (permalink / raw)
To: Mat Martineau
Cc: Matthieu Baerts, Rafael J. Wysocki, Pavel Machek,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
the arch/x86 maintainers, H. Peter Anvin, Chen Yu, Pawan Gupta,
Ingo Molnar, Rafael J. Wysocki, Linux PM,
Linux Kernel Mailing List
On Fri, Apr 22, 2022 at 1:21 AM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Thu, 21 Apr 2022, 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 [1]
> > - wait ~1 minute
> > - start a kmemleak scan
> >
> > It seems kmemleak has an issue with the array allocated in
> > msr_build_context() and assigned to a pointer in a static structure
> > (saved_context.saved_msrs->array): there is no leak then.
> >
> > It looks like this is a limitation from kmemleak but that's alright,
> > kmemleak_no_leak() can be used to avoid complaining about that.
> >
> > 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").
> >
> > Depending on their CPUs, others have probably 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. But I
> > understand if someone says the origin of this issue is more on
> > kmemleak's side. What is unclear to me is why this issue was not seen by
> > other people and CIs. Maybe the kernel config [1]?
> >
> > [1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
> >
>
> Hi Matthieu -
>
> It looks like the root cause here is alignment within the packed struct
> saved_context (from suspend_64.h). Kmemleak only searches for pointers
> that are aligned, but pahole shows that the saved_msrs struct member and
> all members after it in the structure are unaligned:
>
> (gcc 11.2.1, x86_64)
>
> 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 odd offset values for the remainder of this struct vvv */
>
> 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__));
>
> If I move 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 and __save/__restore_processor_state() if the struct is
> modified - looks like it's the members before misc_enable that must be
> carefully placed.
Yes, you can move misc_enable_saved to the end of it safely AFAICS.
> So far I've only tried this on my local machine, I'll work on getting more
> thorough validation.
>
> Looks like struct saved_context in suspend_32.h has similar odd alignment.
Right, and it can be changed too AFAICS.
> > 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
> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > ---
> > arch/x86/power/cpu.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> > index 3822666fb73d..1467c6d1a966 100644
> > --- a/arch/x86/power/cpu.c
> > +++ b/arch/x86/power/cpu.c
> > @@ -14,6 +14,7 @@
> > #include <linux/tboot.h>
> > #include <linux/dmi.h>
> > #include <linux/pgtable.h>
> > +#include <linux/kmemleak.h>
> >
> > #include <asm/proto.h>
> > #include <asm/mtrr.h>
> > @@ -413,6 +414,9 @@ static int msr_build_context(const u32 *msr_id, const int num)
> > return -ENOMEM;
> > }
> >
> > + /* The pointer is going to be stored in static struct (saved_context) */
> > + kmemleak_not_leak(msr_array);
> > +
> > if (saved_msrs->array) {
> > /*
> > * Multiple callbacks can invoke this function, so copy any
> > --
> > 2.34.1
> >
> >
>
> --
> Mat Martineau
> Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()
2022-04-22 11:51 ` Rafael J. Wysocki
@ 2022-04-22 12:25 ` Matthieu Baerts
2022-04-22 12:32 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Matthieu Baerts @ 2022-04-22 12:25 UTC (permalink / raw)
To: Rafael J. Wysocki, Mat Martineau
Cc: Pavel Machek, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, the arch/x86 maintainers, H. Peter Anvin, Chen Yu,
Pawan Gupta, Ingo Molnar, Rafael J. Wysocki, Linux PM,
Linux Kernel Mailing List
Hi Mat, Rafael,
(oops, please ignore the "mptcp-next" tag I added by reflex in the
subject: this is not related to MPTCP :) )
On 22/04/2022 13:51, Rafael J. Wysocki wrote:
> On Fri, Apr 22, 2022 at 1:21 AM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Thu, 21 Apr 2022, 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 [1]
>>> - wait ~1 minute
>>> - start a kmemleak scan
>>>
>>> It seems kmemleak has an issue with the array allocated in
>>> msr_build_context() and assigned to a pointer in a static structure
>>> (saved_context.saved_msrs->array): there is no leak then.
>>>
>>> It looks like this is a limitation from kmemleak but that's alright,
>>> kmemleak_no_leak() can be used to avoid complaining about that.
>>>
>>> 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").
>>>
>>> Depending on their CPUs, others have probably 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. But I
>>> understand if someone says the origin of this issue is more on
>>> kmemleak's side. What is unclear to me is why this issue was not seen by
>>> other people and CIs. Maybe the kernel config [1]?
>>>
>>> [1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
>>>
>>
>> Hi Matthieu -
>>
>> It looks like the root cause here is alignment within the packed struct
>> saved_context (from suspend_64.h). Kmemleak only searches for pointers
>> that are aligned, but pahole shows that the saved_msrs struct member and
>> all members after it in the structure are unaligned:
@Mat: Thank you for the analysis and finding the root cause!
>> (gcc 11.2.1, x86_64)
>>
>> 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 odd offset values for the remainder of this struct vvv */
>>
>> 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__));
>>
>> If I move 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 and __save/__restore_processor_state() if the struct is
>> modified - looks like it's the members before misc_enable that must be
>> carefully placed.
>
> Yes, you can move misc_enable_saved to the end of it safely AFAICS.
@Rafael: thank you for the reply!
Before doing that, is it still needed to keep the "packed" attribute?
This attribute was already there before the first Git commit.
Without it, I no longer have the kmemleak and pahole reports this:
struct saved_context {
(...)
bool misc_enable_saved; /* 240 1 */
/* XXX 7 bytes hole, try to pack */
struct saved_msrs saved_msrs; /* 248 16 */
(...)
/* size: 328, cachelines: 6, members: 25 */
/* sum members: 317, holes: 2, sum holes: 11 */
/* last cacheline: 8 bytes */
};
Everything is still at the same place before 'misc_enable' member.
If it is important to reduce the cachelines, it is still interesting to
move the bool to avoid a whole which costs one cacheline.
>> So far I've only tried this on my local machine, I'll work on getting more
>> thorough validation.
>>
>> Looks like struct saved_context in suspend_32.h has similar odd alignment.
>
> Right, and it can be changed too AFAICS.
Thanks!
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()
2022-04-22 12:25 ` Matthieu Baerts
@ 2022-04-22 12:32 ` Rafael J. Wysocki
0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2022-04-22 12:32 UTC (permalink / raw)
To: Matthieu Baerts
Cc: Rafael J. Wysocki, Mat Martineau, Pavel Machek, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen,
the arch/x86 maintainers, H. Peter Anvin, Chen Yu, Pawan Gupta,
Ingo Molnar, Rafael J. Wysocki, Linux PM,
Linux Kernel Mailing List
On Fri, Apr 22, 2022 at 2:25 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Mat, Rafael,
>
> (oops, please ignore the "mptcp-next" tag I added by reflex in the
> subject: this is not related to MPTCP :) )
>
> On 22/04/2022 13:51, Rafael J. Wysocki wrote:
> > On Fri, Apr 22, 2022 at 1:21 AM Mat Martineau
> > <mathew.j.martineau@linux.intel.com> wrote:
> >>
> >> On Thu, 21 Apr 2022, 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 [1]
> >>> - wait ~1 minute
> >>> - start a kmemleak scan
> >>>
> >>> It seems kmemleak has an issue with the array allocated in
> >>> msr_build_context() and assigned to a pointer in a static structure
> >>> (saved_context.saved_msrs->array): there is no leak then.
> >>>
> >>> It looks like this is a limitation from kmemleak but that's alright,
> >>> kmemleak_no_leak() can be used to avoid complaining about that.
> >>>
> >>> 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").
> >>>
> >>> Depending on their CPUs, others have probably 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. But I
> >>> understand if someone says the origin of this issue is more on
> >>> kmemleak's side. What is unclear to me is why this issue was not seen by
> >>> other people and CIs. Maybe the kernel config [1]?
> >>>
> >>> [1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
> >>>
> >>
> >> Hi Matthieu -
> >>
> >> It looks like the root cause here is alignment within the packed struct
> >> saved_context (from suspend_64.h). Kmemleak only searches for pointers
> >> that are aligned, but pahole shows that the saved_msrs struct member and
> >> all members after it in the structure are unaligned:
>
> @Mat: Thank you for the analysis and finding the root cause!
>
> >> (gcc 11.2.1, x86_64)
> >>
> >> 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 odd offset values for the remainder of this struct vvv */
> >>
> >> 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__));
> >>
> >> If I move 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 and __save/__restore_processor_state() if the struct is
> >> modified - looks like it's the members before misc_enable that must be
> >> carefully placed.
> >
> > Yes, you can move misc_enable_saved to the end of it safely AFAICS.
>
> @Rafael: thank you for the reply!
>
> Before doing that, is it still needed to keep the "packed" attribute?
> This attribute was already there before the first Git commit.
It is there because of the RAX-relative accesses in the assembly code.
I'm not sure if correct computation of the offsets in that code can be
guaranteed without it.
> Without it, I no longer have the kmemleak and pahole reports this:
>
>
> struct saved_context {
>
> (...)
> bool misc_enable_saved; /* 240 1 */
>
> /* XXX 7 bytes hole, try to pack */
>
> struct saved_msrs saved_msrs; /* 248 16 */
>
> (...)
>
> /* size: 328, cachelines: 6, members: 25 */
> /* sum members: 317, holes: 2, sum holes: 11 */
> /* last cacheline: 8 bytes */
> };
>
>
> Everything is still at the same place before 'misc_enable' member.
>
> If it is important to reduce the cachelines, it is still interesting to
> move the bool to avoid a whole which costs one cacheline.
>
>
> >> So far I've only tried this on my local machine, I'll work on getting more
> >> thorough validation.
> >>
> >> Looks like struct saved_context in suspend_32.h has similar odd alignment.
> >
> > Right, and it can be changed too AFAICS.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-22 12:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 16:15 [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context() Matthieu Baerts
2022-04-21 23:20 ` Mat Martineau
2022-04-22 11:51 ` Rafael J. Wysocki
2022-04-22 12:25 ` Matthieu Baerts
2022-04-22 12:32 ` 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).