Received-SPF: pass (google.com: domain of linux-kernel+bounces-111266-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Fri, 22 Mar 2024 11:03:00 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Anton Altaparmakov <anton@tuxera.com>
Cc: Dave Hansen <dave.hansen@intel.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Pavel Machek <pavel@ucw.cz>, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"H . Peter Anvin" <hpa@zytor.com>, Chen Yu <yu.c.chen@intel.com>,
	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Linux Memory Management <linux-mm@kvack.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH] x86/pm: Fix false positive kmemleak report in
 msr_build_context().
Message-ID: <Zf1XVPicqdJ3VBcl@gmail.com>
References: <20240314142656.17699-1-anton@tuxera.com>
 <70261e2a-b87e-462e-964e-95a51ecde978@intel.com>
 <653BCAC0-8A79-400F-B496-23A2FA169786@tuxera.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <653BCAC0-8A79-400F-B496-23A2FA169786@tuxera.com>


* Anton Altaparmakov <anton@tuxera.com> wrote:

> Hi Dave,
> 
> > On 14 Mar 2024, at 15:05, Dave Hansen <dave.hansen@intel.com> wrote:
> > 
> > On 3/14/24 07:26, Anton Altaparmakov wrote:
> >> /* image of the saved processor state */
> >> struct saved_context {
> >> - /*
> >> - * On x86_32, all segment registers except gs are saved at kernel
> >> - * entry in pt_regs.
> >> - */
> >> - u16 gs;
> >> unsigned long cr0, cr2, cr3, cr4;
> >> u64 misc_enable;
> >> struct saved_msrs saved_msrs;
> >> @@ -27,6 +22,11 @@ struct saved_context {
> >> unsigned long tr;
> >> unsigned long safety;
> >> unsigned long return_address;
> >> + /*
> >> + * On x86_32, all segment registers except gs are saved at kernel
> >> + * entry in pt_regs.
> >> + */
> >> + u16 gs;
> >> bool misc_enable_saved;
> >> } __attribute__((packed));
> > 
> > Isn't this just kinda poking at the symptoms?  This seems to be
> > basically the exact same bug as b0b592cf08, just with a different source
> > of unaligned structure members.
> 
> Yes, that is exactly the same bug.  That's how we figured out the solution in fact - it is totally the same problem with another struct member...
> 
> > There's nothing to keep folks from reintroducing these kinds of issues
> > and evidently no way to detect when they happen without lengthy reproducers.
> 
> Correct.  But short of adding asserts / documentation that pointers must be aligned or kmemleak won't work or fixing kmemleak (which I expect is not tractical as it would become a lot slower if nothing else) not sure what else can be done.
> 
> Given I cannot see any alternative to fixing the kmemleak failures I think it is worth applying this fix.
> 
> Unless you have better ideas how to fix this issue?
> 
> What I can say is that we run a lot of tests with our CI and applying 
> this fix we do not see any kmemleak issues any more whilst without it we 
> see hundreds of the above - from a single, simple test run consisting of 
> 416 individual test cases on kernel 5.10 x86 with kmemleak enabled we got 
> 20 failures due to this which is quite a lot.  With this fix applied we 
> get zero kmemleak related failures.

I turned this tidbit into the following paragraph in the commit:

    Testing:
    
    We run a lot of tests with our CI, and after applying this fix we do not
    see any kmemleak issues any more whilst without it we see hundreds of
    the above report. From a single, simple test run consisting of 416 individual test
    cases on kernel 5.10 x86 with kmemleak enabled we got 20 failures due to this,
    which is quite a lot. With this fix applied we get zero kmemleak related failures.

Describing the impact of a fix in a changelog is always helpful.

Thanks,

	Ingo