From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id EEC6FC433EF for ; Wed, 27 Apr 2022 16:33:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5A9416B0071; Wed, 27 Apr 2022 12:33:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 530AA6B0074; Wed, 27 Apr 2022 12:33:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3D1E66B0075; Wed, 27 Apr 2022 12:33:35 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.a.hostedemail.com [64.99.140.24]) by kanga.kvack.org (Postfix) with ESMTP id 27E8A6B0071 for ; Wed, 27 Apr 2022 12:33:35 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id EE24128C88 for ; Wed, 27 Apr 2022 16:33:34 +0000 (UTC) X-FDA: 79403204748.29.1AA699C Received: from mail-yw1-f170.google.com (mail-yw1-f170.google.com [209.85.128.170]) by imf21.hostedemail.com (Postfix) with ESMTP id 014871C006C for ; Wed, 27 Apr 2022 16:33:30 +0000 (UTC) Received: by mail-yw1-f170.google.com with SMTP id 00721157ae682-2f7bb893309so24835117b3.12 for ; Wed, 27 Apr 2022 09:33:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=enk8juf44OQ3EV3J2i1uDWk0UGyi18nDBXGfWnijPQo=; b=rBhU7ISYIw9Znij4vegZkd3UcQcI6UunsKQs/FZ7MYJeNHskLvBtVqiql2qwAsBzy4 EFldMe9mmz65cNeOyJCElF0fRKVy5yV2ZjtU/GfJCDXzZ9YVh19mcqprPtxs1MzVxF05 Vrc/Z5O4dl8fVrpUq33vZODZovoMCSC7VICz1ybjWS5BO+fgTRR+0ROhlGeYl9LwZnCV +fX1t943XuPUGAXlfsseejhZkXvjtPiOofZpTHIJOq7+BkZslsAgsZi9dRL+vecQDM8s hectMOgkVQyRw+OZhRk77No2KQOQFz9Qw6ZW7BM3vi/3Ai/Z+zbvUS5nUzdSlC7kXDds 4cDA== X-Gm-Message-State: AOAM531oB8rtmEKrrm0tvJqZCklNxIMRuBVYfEoWt3uXKvIP8Ym04Omm U4bojKS6vtQlbtYaDtt/cOXY2zK+OAyDLCMLIt0= X-Google-Smtp-Source: ABdhPJxSWGQf7oQ9K7jf2r0K7x8DY4BWYwdUk/rne4lLqGYAc5rQfjcLKOlu23Yr9geRX/10IVam7MwAfw/j3GX/1JE= X-Received: by 2002:a0d:e645:0:b0:2f4:dbd6:261e with SMTP id p66-20020a0de645000000b002f4dbd6261emr28505533ywe.7.1651077213330; Wed, 27 Apr 2022 09:33:33 -0700 (PDT) MIME-Version: 1.0 References: <20220426202138.498310-1-matthieu.baerts@tessares.net> In-Reply-To: <20220426202138.498310-1-matthieu.baerts@tessares.net> From: "Rafael J. Wysocki" Date: Wed, 27 Apr 2022 18:33:22 +0200 Message-ID: Subject: Re: [PATCH v3] x86/pm: fix false positive kmemleak report in msr_build_context() 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 Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 014871C006C X-Stat-Signature: tudfbfei9x3tceboi6tngd5csuherokw X-Rspam-User: Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of rjwysocki@gmail.com designates 209.85.128.170 as permitted sender) smtp.mailfrom=rjwysocki@gmail.com; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=kernel.org (policy=none) X-HE-Tag: 1651077210-211611 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Apr 26, 2022 at 10:22 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 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 > Signed-off-by: Matthieu Baerts Still good IMV. Reviewed-by: Rafael J. Wysocki > --- > > 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 >