We currently have several kernel parameters that affect page allocator wrt
debugging and hardening, some also with config options: init_on_alloc,
init_on_free, page_poison, debug_pagealloc.
These options generally have their own static keys, but sometimes a decision
for e.g. clearing a page depends on multiple options, and the handling is not
as efficient as it could be. This series addresses that by centralizing the
decisions into a new init_mem_debugging() function that enables individual
static keys, and most paths now rely on a single static key check. Subtle
dependency on the order of parameters is also eliminated (Patch 1). The result
is more efficient and hopefully also more readable code.
Vlastimil Babka (3):
mm, page_alloc: do not rely on the order of page_poison and
init_on_alloc/free parameters
mm, page_poison: use static key more efficiently
mm, page_alloc: reduce static keys in prep_new_page()
drivers/virtio/virtio_balloon.c | 2 +-
include/linux/mm.h | 36 ++++-----
init/main.c | 2 +-
mm/page_alloc.c | 126 ++++++++++++++++++--------------
mm/page_poison.c | 40 ++--------
5 files changed, 97 insertions(+), 109 deletions(-)
--
2.29.0
Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces
a warning in dmesg that page_poison takes precendence. However, as these
warnings are printed in early_param handlers for init_on_alloc/free, they are
not printed if page_poison is enabled later on the command line (handlers are
called in the order of their parameters), or when init_on_alloc/free is always
enabled by the respective config option - before the page_poison early param
handler is called, it is not considered to be enabled. This is inconsistent.
We can remove the dependency on order by making the init_on_* parameters only
set a boolean variable, and postponing the evaluation after all early params
have been processed. Introduce a new init_mem_debugging() function for that,
and move the related debug_pagealloc processing there as well.
As a result init_mem_debugging() knows always accurately if init_on_* and/or
page_poison options were enabled. Thus we can also optimize want_init_on_alloc()
and want_init_on_free(). We don't need to check page_poisoning_enabled() there,
we can instead not enable the init_on_* tracepoint at all, if page poisoning is
enabled. This results in a simpler and more effective code.
Signed-off-by: Vlastimil Babka <[email protected]>
---
include/linux/mm.h | 20 ++--------
init/main.c | 2 +-
mm/page_alloc.c | 94 +++++++++++++++++++++++-----------------------
3 files changed, 50 insertions(+), 66 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ef360fe70aaf..c6a0adccf2fe 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2865,6 +2865,7 @@ extern int apply_to_existing_page_range(struct mm_struct *mm,
unsigned long address, unsigned long size,
pte_fn_t fn, void *data);
+extern void init_mem_debugging(void);
#ifdef CONFIG_PAGE_POISONING
extern bool page_poisoning_enabled(void);
extern void kernel_poison_pages(struct page *page, int numpages, int enable);
@@ -2874,35 +2875,20 @@ static inline void kernel_poison_pages(struct page *page, int numpages,
int enable) { }
#endif
-#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
-DECLARE_STATIC_KEY_TRUE(init_on_alloc);
-#else
DECLARE_STATIC_KEY_FALSE(init_on_alloc);
-#endif
static inline bool want_init_on_alloc(gfp_t flags)
{
- if (static_branch_unlikely(&init_on_alloc) &&
- !page_poisoning_enabled())
+ if (static_branch_unlikely(&init_on_alloc))
return true;
return flags & __GFP_ZERO;
}
-#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
-DECLARE_STATIC_KEY_TRUE(init_on_free);
-#else
DECLARE_STATIC_KEY_FALSE(init_on_free);
-#endif
static inline bool want_init_on_free(void)
{
- return static_branch_unlikely(&init_on_free) &&
- !page_poisoning_enabled();
+ return static_branch_unlikely(&init_on_free);
}
-#ifdef CONFIG_DEBUG_PAGEALLOC
-extern void init_debug_pagealloc(void);
-#else
-static inline void init_debug_pagealloc(void) {}
-#endif
extern bool _debug_pagealloc_enabled_early;
DECLARE_STATIC_KEY_FALSE(_debug_pagealloc_enabled);
diff --git a/init/main.c b/init/main.c
index 130376ec10ba..dca5b9093742 100644
--- a/init/main.c
+++ b/init/main.c
@@ -815,7 +815,7 @@ static void __init mm_init(void)
* bigger than MAX_ORDER unless SPARSEMEM.
*/
page_ext_init_flatmem();
- init_debug_pagealloc();
+ init_mem_debugging();
report_meminit();
mem_init();
kmem_cache_init();
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 23f5066bd4a5..b168c58ef337 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -165,53 +165,26 @@ unsigned long totalcma_pages __read_mostly;
int percpu_pagelist_fraction;
gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
-#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
-DEFINE_STATIC_KEY_TRUE(init_on_alloc);
-#else
-DEFINE_STATIC_KEY_FALSE(init_on_alloc);
-#endif
+DEFINE_STATIC_KEY_FALSE_RO(init_on_alloc);
EXPORT_SYMBOL(init_on_alloc);
-#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
-DEFINE_STATIC_KEY_TRUE(init_on_free);
-#else
-DEFINE_STATIC_KEY_FALSE(init_on_free);
-#endif
+DEFINE_STATIC_KEY_FALSE_RO(init_on_free);
EXPORT_SYMBOL(init_on_free);
+static bool _init_on_alloc_enabled_early __read_mostly
+ = IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON);
static int __init early_init_on_alloc(char *buf)
{
- int ret;
- bool bool_result;
- ret = kstrtobool(buf, &bool_result);
- if (ret)
- return ret;
- if (bool_result && page_poisoning_enabled())
- pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_alloc\n");
- if (bool_result)
- static_branch_enable(&init_on_alloc);
- else
- static_branch_disable(&init_on_alloc);
- return 0;
+ return kstrtobool(buf, &_init_on_alloc_enabled_early);
}
early_param("init_on_alloc", early_init_on_alloc);
+static bool _init_on_free_enabled_early __read_mostly
+ = IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON);
static int __init early_init_on_free(char *buf)
{
- int ret;
- bool bool_result;
-
- ret = kstrtobool(buf, &bool_result);
- if (ret)
- return ret;
- if (bool_result && page_poisoning_enabled())
- pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, will take precedence over init_on_free\n");
- if (bool_result)
- static_branch_enable(&init_on_free);
- else
- static_branch_disable(&init_on_free);
- return 0;
+ return kstrtobool(buf, &_init_on_free_enabled_early);
}
early_param("init_on_free", early_init_on_free);
@@ -728,19 +701,6 @@ static int __init early_debug_pagealloc(char *buf)
}
early_param("debug_pagealloc", early_debug_pagealloc);
-void init_debug_pagealloc(void)
-{
- if (!debug_pagealloc_enabled())
- return;
-
- static_branch_enable(&_debug_pagealloc_enabled);
-
- if (!debug_guardpage_minorder())
- return;
-
- static_branch_enable(&_debug_guardpage_enabled);
-}
-
static int __init debug_guardpage_minorder_setup(char *buf)
{
unsigned long res;
@@ -792,6 +752,44 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
unsigned int order, int migratetype) {}
#endif
+/*
+ * Enable static keys related to various memory debugging and hardening options.
+ * Some override others, and depend on early params that are evaluated in the
+ * order of appearance. So we need to first gather the full picture of what was
+ * enabled, and then make decisions.
+ */
+void init_mem_debugging()
+{
+ if (_init_on_alloc_enabled_early) {
+ if (page_poisoning_enabled()) {
+ pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
+ "will take precedence over init_on_alloc\n");
+ } else {
+ static_branch_enable(&init_on_alloc);
+ }
+ }
+ if (_init_on_free_enabled_early) {
+ if (page_poisoning_enabled()) {
+ pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
+ "will take precedence over init_on_free\n");
+ } else {
+ static_branch_enable(&init_on_free);
+ }
+ }
+
+#ifdef CONFIG_DEBUG_PAGEALLOC
+ if (!debug_pagealloc_enabled())
+ return;
+
+ static_branch_enable(&_debug_pagealloc_enabled);
+
+ if (!debug_guardpage_minorder())
+ return;
+
+ static_branch_enable(&_debug_guardpage_enabled);
+#endif
+}
+
static inline void set_buddy_order(struct page *page, unsigned int order)
{
set_page_private(page, order);
--
2.29.0
prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
init_on_alloc is enabled, but will also always skip zeroing if the page was
already zeroed on free by init_on_free or page poisoning.
The latter check implemented by free_pages_prezeroed() can involve two
different static keys. As prep_new_page() is really a hot path, let's introduce
a single static key free_pages_not_prezeroed for this purpose and initialize it
in init_mem_debugging().
Signed-off-by: Vlastimil Babka <[email protected]>
---
mm/page_alloc.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2a1be197649d..980780f5f242 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -171,6 +171,8 @@ EXPORT_SYMBOL(init_on_alloc);
DEFINE_STATIC_KEY_FALSE_RO(init_on_free);
EXPORT_SYMBOL(init_on_free);
+static DEFINE_STATIC_KEY_TRUE_RO(free_pages_not_prezeroed);
+
static bool _init_on_alloc_enabled_early __read_mostly
= IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON);
static int __init early_init_on_alloc(char *buf)
@@ -777,6 +779,16 @@ void init_mem_debugging()
}
}
+ /*
+ * We have a special static key that controls whether prep_new_page will
+ * never need to zero the page. This mode is enabled when page is
+ * already zeroed by init_on_free or page_poisoning zero mode.
+ */
+ if (_init_on_free_enabled_early ||
+ (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO)
+ && page_poisoning_enabled()))
+ static_branch_disable(&free_pages_not_prezeroed);
+
#ifdef CONFIG_PAGE_POISONING
/*
* Page poisoning is debug page alloc for some arches. If
@@ -2216,12 +2228,6 @@ static inline int check_new_page(struct page *page)
return 1;
}
-static inline bool free_pages_prezeroed(void)
-{
- return (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) &&
- page_poisoning_enabled_static()) || want_init_on_free();
-}
-
#ifdef CONFIG_DEBUG_VM
/*
* With DEBUG_VM enabled, order-0 pages are checked for expected state when
@@ -2291,7 +2297,8 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
{
post_alloc_hook(page, order, gfp_flags);
- if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
+ if (static_branch_likely(&free_pages_not_prezeroed)
+ && want_init_on_alloc(gfp_flags))
kernel_init_free_pages(page, 1 << order);
if (order && (gfp_flags & __GFP_COMP))
--
2.29.0
On 10/27/20 10:03 AM, David Hildenbrand wrote:
> On 26.10.20 18:33, Vlastimil Babka wrote:
>> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces
>> a warning in dmesg that page_poison takes precendence. However, as these
>> warnings are printed in early_param handlers for init_on_alloc/free, they are
>> not printed if page_poison is enabled later on the command line (handlers are
>> called in the order of their parameters), or when init_on_alloc/free is always
>> enabled by the respective config option - before the page_poison early param
>> handler is called, it is not considered to be enabled. This is inconsistent.
>>
>> We can remove the dependency on order by making the init_on_* parameters only
>> set a boolean variable, and postponing the evaluation after all early params
>> have been processed. Introduce a new init_mem_debugging() function for that,
>> and move the related debug_pagealloc processing there as well.
>
> init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or
> init_on_free=1 are rather security hardening mechanisms.
Well yeah, init_mem_debugging_and_hardening()?
> ... I wondered if this could be the place to initialize any kind of mm
> parameters in the future. Like init_mem_params() or so.
Maybe. In practice you often find out that different things have to be hooked in
different points of the init process, and a single function might not be enough.
I tried to group stuff that's really inter-related and can be initialized at the
same time.
>>
>> As a result init_mem_debugging() knows always accurately if init_on_* and/or
>> page_poison options were enabled. Thus we can also optimize want_init_on_alloc()
>> and want_init_on_free(). We don't need to check page_poisoning_enabled() there,
>> we can instead not enable the init_on_* tracepoint at all, if page poisoning is
>> enabled. This results in a simpler and more effective code.
>
> LGTM
>
> Reviewed-by: David Hildenbrand <[email protected]>
Thanks!
On 10/27/20 12:05 PM, Vlastimil Babka wrote:
> On 10/27/20 10:10 AM, David Hildenbrand wrote:
>> On 26.10.20 18:33, Vlastimil Babka wrote:
>>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
>>> init_on_alloc is enabled, but will also always skip zeroing if the page was
>>> already zeroed on free by init_on_free or page poisoning.
>>>
>>> The latter check implemented by free_pages_prezeroed() can involve two
>>> different static keys. As prep_new_page() is really a hot path, let's introduce
>>> a single static key free_pages_not_prezeroed for this purpose and initialize it
>>> in init_mem_debugging().
>>
>> Is this actually observable in practice? This smells like
>> micro-optimization to me.
>>
>> Also, I thought the whole reason for static keys is to have basically no
>> overhead at runtime, so I wonder if replacing two static key checks by a
>> single one actually makes *some* difference.
>
> You're right, the difference seems to be just a single NOP. The static key
> infrastructure seems to be working really well.
> (At least the asm inspection made me realize that kernel_poison_pages() is
> called unconditionally and the static key is checked inside, not inline so I'll
> be amending patch 2...)
>
> Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the
> code wrong. So unless others think it's a readability improvements, we can drop
> this patch.
>
> Or we can also reconsider this whole optimization. If the point is to be
> paranoid and enable both init_on_free and init_on_alloc, should we trust that
> nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex?
More thoughts...
PAGE_POISONING_NO_SANITY skips the check on "unpoisoning" whether poison was
corrupted
PAGE_POISONING_ZERO uses zero instead of 0xAA as poison pattern
the point of enabling both of these seems to be moot now that init_on_free
exists, as that zeroes pages that are being freed, without checking on alloc
that they are still zeroed.
What if only one is enabled?
- PAGE_POISONING_NO_SANITY without PAGE_POISONING_ZERO - we poison with the 0xAA
pattern but nobody checks it, so does it give us anything over init_on_free
writing zeroes? I don't think so?
- PAGE_POISONING_ZERO without PAGE_POISONING_NO_SANITY - we use zeroes (like
init_on_free) but also check that it wasn't corrupted. We save some time on
writing zeroes again on alloc, but the check is still expensive. And writing
0xAA would possibly detect more corruptions than writing zero (a stray write of
NULL is more likely to happen than of 0xAA?).
So my conclusion:
- We can remove PAGE_POISONING_NO_SANITY because it only makes sense with
PAGE_POISONING_ZERO, and we can use init_on_free instead
- We can also probably remove PAGE_POISONING_ZERO, because if we want to do the
unpoisoning sanity check, then we also most likely want the 0xAA pattern and not
zero.
Thoughts?
On 26.10.20 18:33, Vlastimil Babka wrote:
> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces
> a warning in dmesg that page_poison takes precendence. However, as these
> warnings are printed in early_param handlers for init_on_alloc/free, they are
> not printed if page_poison is enabled later on the command line (handlers are
> called in the order of their parameters), or when init_on_alloc/free is always
> enabled by the respective config option - before the page_poison early param
> handler is called, it is not considered to be enabled. This is inconsistent.
>
> We can remove the dependency on order by making the init_on_* parameters only
> set a boolean variable, and postponing the evaluation after all early params
> have been processed. Introduce a new init_mem_debugging() function for that,
> and move the related debug_pagealloc processing there as well.
init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or
init_on_free=1 are rather security hardening mechanisms.
... I wondered if this could be the place to initialize any kind of mm
parameters in the future. Like init_mem_params() or so.
>
> As a result init_mem_debugging() knows always accurately if init_on_* and/or
> page_poison options were enabled. Thus we can also optimize want_init_on_alloc()
> and want_init_on_free(). We don't need to check page_poisoning_enabled() there,
> we can instead not enable the init_on_* tracepoint at all, if page poisoning is
> enabled. This results in a simpler and more effective code.
LGTM
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 26.10.20 18:33, Vlastimil Babka wrote:
> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
> init_on_alloc is enabled, but will also always skip zeroing if the page was
> already zeroed on free by init_on_free or page poisoning.
>
> The latter check implemented by free_pages_prezeroed() can involve two
> different static keys. As prep_new_page() is really a hot path, let's introduce
> a single static key free_pages_not_prezeroed for this purpose and initialize it
> in init_mem_debugging().
Is this actually observable in practice? This smells like
micro-optimization to me.
Also, I thought the whole reason for static keys is to have basically no
overhead at runtime, so I wonder if replacing two static key checks by a
single one actually makes *some* difference.
--
Thanks,
David / dhildenb
On 27.10.20 10:58, Vlastimil Babka wrote:
> On 10/27/20 10:03 AM, David Hildenbrand wrote:
>> On 26.10.20 18:33, Vlastimil Babka wrote:
>>> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces
>>> a warning in dmesg that page_poison takes precendence. However, as these
>>> warnings are printed in early_param handlers for init_on_alloc/free, they are
>>> not printed if page_poison is enabled later on the command line (handlers are
>>> called in the order of their parameters), or when init_on_alloc/free is always
>>> enabled by the respective config option - before the page_poison early param
>>> handler is called, it is not considered to be enabled. This is inconsistent.
>>>
>>> We can remove the dependency on order by making the init_on_* parameters only
>>> set a boolean variable, and postponing the evaluation after all early params
>>> have been processed. Introduce a new init_mem_debugging() function for that,
>>> and move the related debug_pagealloc processing there as well.
>>
>> init_mem_debugging() is somewhat sub-optimal - init_on_alloc=1 or
>> init_on_free=1 are rather security hardening mechanisms.
>
> Well yeah, init_mem_debugging_and_hardening()?
Would work for me.
--
Thanks,
David / dhildenb
On 10/27/20 10:10 AM, David Hildenbrand wrote:
> On 26.10.20 18:33, Vlastimil Babka wrote:
>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
>> init_on_alloc is enabled, but will also always skip zeroing if the page was
>> already zeroed on free by init_on_free or page poisoning.
>>
>> The latter check implemented by free_pages_prezeroed() can involve two
>> different static keys. As prep_new_page() is really a hot path, let's introduce
>> a single static key free_pages_not_prezeroed for this purpose and initialize it
>> in init_mem_debugging().
>
> Is this actually observable in practice? This smells like
> micro-optimization to me.
>
> Also, I thought the whole reason for static keys is to have basically no
> overhead at runtime, so I wonder if replacing two static key checks by a
> single one actually makes *some* difference.
You're right, the difference seems to be just a single NOP. The static key
infrastructure seems to be working really well.
(At least the asm inspection made me realize that kernel_poison_pages() is
called unconditionally and the static key is checked inside, not inline so I'll
be amending patch 2...)
Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the
code wrong. So unless others think it's a readability improvements, we can drop
this patch.
Or we can also reconsider this whole optimization. If the point is to be
paranoid and enable both init_on_free and init_on_alloc, should we trust that
nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex?
On 10/27/20 2:32 PM, Vlastimil Babka wrote:
> So my conclusion:
> - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with
> PAGE_POISONING_ZERO, and we can use init_on_free instead
Note for this we first have to make sanity checking compatible with
hibernation, but that should be easy as the zeroing variants already
paved the way. The patch below will be added to the next version of
the series:
From 44474ee27c4f5248061ea2e5bbc2aeefc91bcdfc Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <[email protected]>
Date: Tue, 27 Oct 2020 18:25:17 +0100
Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity
checking
Page poisoning used to be incompatible with hibernation, as the state of
poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces
CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes
variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter
restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow
hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit
18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure
free pages are cleared after resume.
We can use the same mechanism to instead poison free pages with PAGE_POISON
after resume. This covers both zero and 0xAA patterns. Thus we can remove the
Kconfig restriction that disables page poison sanity checking when hibernation
is enabled.
Signed-off-by: Vlastimil Babka <[email protected]>
---
kernel/power/hibernate.c | 2 +-
kernel/power/power.h | 2 +-
kernel/power/snapshot.c | 14 ++++++++++----
mm/Kconfig.debug | 1 -
4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2fc7d509a34f..da0b41914177 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -326,7 +326,7 @@ static int create_image(int platform_mode)
if (!in_suspend) {
events_check_enabled = false;
- clear_free_pages();
+ clear_or_poison_free_pages();
}
platform_leave(platform_mode);
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 24f12d534515..778bf431ec02 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -106,7 +106,7 @@ extern int create_basic_memory_bitmaps(void);
extern void free_basic_memory_bitmaps(void);
extern int hibernate_preallocate_memory(void);
-extern void clear_free_pages(void);
+extern void clear_or_poison_free_pages(void);
/**
* Auxiliary structure used for reading the snapshot image data and
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 46b1804c1ddf..6b1c84afa891 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1144,7 +1144,7 @@ void free_basic_memory_bitmaps(void)
pr_debug("Basic memory bitmaps freed\n");
}
-void clear_free_pages(void)
+void clear_or_poison_free_pages(void)
{
struct memory_bitmap *bm = free_pages_map;
unsigned long pfn;
@@ -1152,12 +1152,18 @@ void clear_free_pages(void)
if (WARN_ON(!(free_pages_map)))
return;
- if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) {
+ if (page_poisoning_enabled() || want_init_on_free()) {
memory_bm_position_reset(bm);
pfn = memory_bm_next_pfn(bm);
while (pfn != BM_END_OF_MAP) {
- if (pfn_valid(pfn))
- clear_highpage(pfn_to_page(pfn));
+ if (pfn_valid(pfn)) {
+ struct page *page = pfn_to_page(pfn);
+ if (page_poisoning_enabled_static())
+ kernel_poison_pages(page, 1);
+ else if (want_init_on_free())
+ clear_highpage(page);
+
+ }
pfn = memory_bm_next_pfn(bm);
}
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 864f129f1937..c57786ad5be9 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -64,7 +64,6 @@ config PAGE_OWNER
config PAGE_POISONING
bool "Poison pages after freeing"
- select PAGE_POISONING_NO_SANITY if HIBERNATION
help
Fill the pages with poison patterns after free_pages() and verify
the patterns before alloc_pages. The filling of the memory helps
--
2.29.0
On Mon, Oct 26, 2020 at 06:33:56PM +0100, Vlastimil Babka wrote:
> Enabling page_poison=1 together with init_on_alloc=1 or init_on_free=1 produces
> a warning in dmesg that page_poison takes precendence. However, as these
^ precedence
> warnings are printed in early_param handlers for init_on_alloc/free, they are
> not printed if page_poison is enabled later on the command line (handlers are
> called in the order of their parameters), or when init_on_alloc/free is always
> enabled by the respective config option - before the page_poison early param
> handler is called, it is not considered to be enabled. This is inconsistent.
>
> We can remove the dependency on order by making the init_on_* parameters only
> set a boolean variable, and postponing the evaluation after all early params
> have been processed. Introduce a new init_mem_debugging() function for that,
> and move the related debug_pagealloc processing there as well.
>
> As a result init_mem_debugging() knows always accurately if init_on_* and/or
> page_poison options were enabled. Thus we can also optimize want_init_on_alloc()
> and want_init_on_free(). We don't need to check page_poisoning_enabled() there,
> we can instead not enable the init_on_* tracepoint at all, if page poisoning is
> enabled. This results in a simpler and more effective code.
>
> Signed-off-by: Vlastimil Babka <[email protected]>
With two more nits below fixed
Reviewed-by: Mike Rapoport <[email protected]>
> ---
> include/linux/mm.h | 20 ++--------
> init/main.c | 2 +-
> mm/page_alloc.c | 94 +++++++++++++++++++++++-----------------------
> 3 files changed, 50 insertions(+), 66 deletions(-)
>
...
> @@ -792,6 +752,44 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
> unsigned int order, int migratetype) {}
> #endif
>
> +/*
> + * Enable static keys related to various memory debugging and hardening options.
> + * Some override others, and depend on early params that are evaluated in the
> + * order of appearance. So we need to first gather the full picture of what was
> + * enabled, and then make decisions.
> + */
> +void init_mem_debugging()
Shouldn't it be init_mem_debug(void)?
Or whatever a new name would be :)
> +{
> + if (_init_on_alloc_enabled_early) {
> + if (page_poisoning_enabled()) {
> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> + "will take precedence over init_on_alloc\n");
> + } else {
> + static_branch_enable(&init_on_alloc);
> + }
> + }
> + if (_init_on_free_enabled_early) {
> + if (page_poisoning_enabled()) {
> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> + "will take precedence over init_on_free\n");
> + } else {
> + static_branch_enable(&init_on_free);
> + }
> + }
I think the braces for the inner ifs are not required.
> +
> +#ifdef CONFIG_DEBUG_PAGEALLOC
> + if (!debug_pagealloc_enabled())
> + return;
> +
> + static_branch_enable(&_debug_pagealloc_enabled);
> +
> + if (!debug_guardpage_minorder())
> + return;
> +
> + static_branch_enable(&_debug_guardpage_enabled);
> +#endif
> +}
> +
> static inline void set_buddy_order(struct page *page, unsigned int order)
> {
> set_page_private(page, order);
> --
> 2.29.0
>
>
--
Sincerely yours,
Mike.
On 27.10.20 18:41, Vlastimil Babka wrote:
> On 10/27/20 2:32 PM, Vlastimil Babka wrote:
>> So my conclusion:
>> - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with
>> PAGE_POISONING_ZERO, and we can use init_on_free instead
>
> Note for this we first have to make sanity checking compatible with
> hibernation, but that should be easy as the zeroing variants already
> paved the way. The patch below will be added to the next version of
> the series:
>
> From 44474ee27c4f5248061ea2e5bbc2aeefc91bcdfc Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <[email protected]>
> Date: Tue, 27 Oct 2020 18:25:17 +0100
> Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity
> checking
>
> Page poisoning used to be incompatible with hibernation, as the state of
> poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces
> CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes
> variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter
> restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow
> hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit
> 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure
> free pages are cleared after resume.
>
> We can use the same mechanism to instead poison free pages with PAGE_POISON
> after resume. This covers both zero and 0xAA patterns. Thus we can remove the
> Kconfig restriction that disables page poison sanity checking when hibernation
> is enabled.
I haven't fully dived into the details, but the idea it sounds sane to me.
--
Thanks,
David / dhildenb
On Tue, Oct 27, 2020 at 2:32 PM Vlastimil Babka <[email protected]> wrote:
>
> On 10/27/20 12:05 PM, Vlastimil Babka wrote:
> > On 10/27/20 10:10 AM, David Hildenbrand wrote:
> >> On 26.10.20 18:33, Vlastimil Babka wrote:
> >>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
> >>> init_on_alloc is enabled, but will also always skip zeroing if the page was
> >>> already zeroed on free by init_on_free or page poisoning.
> >>>
> >>> The latter check implemented by free_pages_prezeroed() can involve two
> >>> different static keys. As prep_new_page() is really a hot path, let's introduce
> >>> a single static key free_pages_not_prezeroed for this purpose and initialize it
> >>> in init_mem_debugging().
> >>
> >> Is this actually observable in practice? This smells like
> >> micro-optimization to me.
> >>
> >> Also, I thought the whole reason for static keys is to have basically no
> >> overhead at runtime, so I wonder if replacing two static key checks by a
> >> single one actually makes *some* difference.
> >
> > You're right, the difference seems to be just a single NOP. The static key
> > infrastructure seems to be working really well.
> > (At least the asm inspection made me realize that kernel_poison_pages() is
> > called unconditionally and the static key is checked inside, not inline so I'll
> > be amending patch 2...)
> >
> > Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the
> > code wrong. So unless others think it's a readability improvements, we can drop
> > this patch.
I agree with David that replacing two static keys with one is probably
a micro-optimization.
Also, if someone is enabling both init_on_alloc and init_on_free, they
are already paying so much that no one is going to notice an extra
static key.
> > Or we can also reconsider this whole optimization. If the point is to be
> > paranoid and enable both init_on_free and init_on_alloc, should we trust that
> > nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex?
I think we must trust the kernel to not overwrite zeroed pages.
After all, this could theoretically happen at any time, not only while
the memory chunk is freed.
> More thoughts...
>
> PAGE_POISONING_NO_SANITY skips the check on "unpoisoning" whether poison was
> corrupted
> PAGE_POISONING_ZERO uses zero instead of 0xAA as poison pattern
>
> the point of enabling both of these seems to be moot now that init_on_free
> exists, as that zeroes pages that are being freed, without checking on alloc
> that they are still zeroed.
>
> What if only one is enabled?
> - PAGE_POISONING_NO_SANITY without PAGE_POISONING_ZERO - we poison with the 0xAA
> pattern but nobody checks it, so does it give us anything over init_on_free
> writing zeroes? I don't think so?
>
> - PAGE_POISONING_ZERO without PAGE_POISONING_NO_SANITY - we use zeroes (like
> init_on_free) but also check that it wasn't corrupted. We save some time on
> writing zeroes again on alloc, but the check is still expensive. And writing
> 0xAA would possibly detect more corruptions than writing zero (a stray write of
> NULL is more likely to happen than of 0xAA?).
>
> So my conclusion:
> - We can remove PAGE_POISONING_NO_SANITY because it only makes sense with
> PAGE_POISONING_ZERO, and we can use init_on_free instead
Agreed.
> - We can also probably remove PAGE_POISONING_ZERO, because if we want to do the
> unpoisoning sanity check, then we also most likely want the 0xAA pattern and not
> zero.
Agreed.
It might also make sense to somehow merge page poisoning and
init_on_free together and have one config dimension instead of two
(providing something similar to the
INIT_STACK_NONE/INIT_STACK_ALL_ZERO/INIT_STACK_ALL_PATTERN configs)
> Thoughts?
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg