Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 22 Apr 2022 14:32:50 +0200 | Subject | Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context() |
| |
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.
| |