Re: [PATCH 14/17] mm: Make hibernate handle unmapped pages

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "pavel@ucw.cz" <pavel@ucw.cz>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"linux-integrity@vger.kernel.org"
	<linux-integrity@vger.kernel.org>,
	"ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"nadav.amit@gmail.com" <nadav.amit@gmail.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"Dock, Deneen T" <deneen.t.dock@intel.com>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"linux_dti@icloud.com" <linux_dti@icloud.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"will.deacon@arm.com" <will.deacon@arm.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>
Subject: Re: [PATCH 14/17] mm: Make hibernate handle unmapped pages
Date: Thu, 17 Jan 2019 22:16:47 +0000	[thread overview]
Message-ID: <b224d88d91a5c45c44e176ea06dea558a8939ccf.camel@intel.com> (raw)
In-Reply-To: <20190117093950.GA17930@amd>

On Thu, 2019-01-17 at 10:39 +0100, Pavel Machek wrote:
> Hi!
> 
> > For architectures with CONFIG_ARCH_HAS_SET_ALIAS, pages can be unmapped
> > briefly on the directmap, even when CONFIG_DEBUG_PAGEALLOC is not
> > configured.
> > So this changes kernel_map_pages and kernel_page_present to be defined when
> > CONFIG_ARCH_HAS_SET_ALIAS is defined as well. It also changes places
> > (page_alloc.c) where those functions are assumed to only be implemented when
> > CONFIG_DEBUG_PAGEALLOC is defined.
> 
> Which architectures are that?
> 
> Should this be merged to the patch where HAS_SET_ALIAS is introduced? We
> don't want broken hibernation in between....
Thanks for taking a look. It was added for x86 for patch 13 in this patchset and
there was interest expressed for adding for arm64. If you didn't get the whole
set and want to see let me know and I can send it.

> 
> > -#ifdef CONFIG_DEBUG_PAGEALLOC
> >  extern bool _debug_pagealloc_enabled;
> > -extern void __kernel_map_pages(struct page *page, int numpages, int
> > enable);
> >  
> >  static inline bool debug_pagealloc_enabled(void)
> >  {
> > -	return _debug_pagealloc_enabled;
> > +	return IS_ENABLED(CONFIG_DEBUG_PAGEALLOC) && _debug_pagealloc_enabled;
> >  }
> 
> This will break build AFAICT. _debug_pagealloc_enabled variable does
> not exist in !CONFIG_DEBUG_PAGEALLOC case.
> 
> 									Pavel
After adding in the CONFIG_ARCH_HAS_SET_ALIAS condition to the ifdefs in this
area it looked a little hard to read to me, so I moved debug_pagealloc_enabled
and extern bool _debug_pagealloc_enabled outside to make it easier. I think you
are right, the actual non-extern variable can not be there, but the reference
here gets optimized out in that case.

Just double checked and it builds for both CONFIG_DEBUG_PAGEALLOC=n and
CONFIG_DEBUG_PAGEALLOC=y for me.

Thanks,

Rick