2021-03-26 11:29:13

by Sergei Trofimovich

[permalink] [raw]
Subject: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

init_on_free=1 does not guarantee that free pages contain only zero bytes.

Some examples:
1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1
2. free_pages_prepare() always poisons pages:

if (want_init_on_free())
kernel_init_free_pages(page, 1 << order);
kernel_poison_pages(page, 1 << order

I observed use of poisoned pages as the crash on ia64 booted with
init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
There pmd page contained 0xaaaaaaaa poison pages and led to early crash.

The change drops the assumption that init_on_free=1 guarantees free
pages to contain zeros.

Alternative would be to make interaction between runtime poisoning and
sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
more coherent. I took the simpler path.

Tested the fix on rx3600.

CC: Andrew Morton <[email protected]>
CC: [email protected]
Signed-off-by: Sergei Trofimovich <[email protected]>
---
mm/page_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cfc72873961d..d57d9b4f7089 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2301,7 +2301,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
kernel_unpoison_pages(page, 1 << order);
set_page_owner(page, order, gfp_flags);

- if (!want_init_on_free() && want_init_on_alloc(gfp_flags))
+ if (want_init_on_alloc(gfp_flags))
kernel_init_free_pages(page, 1 << order);
}

--
2.31.0


2021-03-26 13:53:47

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On 26.03.21 12:26, Sergei Trofimovich wrote:
> init_on_free=1 does not guarantee that free pages contain only zero bytes.
>
> Some examples:
> 1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1

s/ini_on_free/init_on_free/

> 2. free_pages_prepare() always poisons pages:
>
> if (want_init_on_free())
> kernel_init_free_pages(page, 1 << order);
> kernel_poison_pages(page, 1 << order

In next/master, it's the other way around already.

commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82
Author: Andrey Konovalov <[email protected]>
Date: Thu Mar 18 17:01:40 2021 +1100

kasan, mm: integrate page_alloc init with HW_TAGS


>
> I observed use of poisoned pages as the crash on ia64 booted with
> init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
>
> The change drops the assumption that init_on_free=1 guarantees free
> pages to contain zeros.
>
> Alternative would be to make interaction between runtime poisoning and
> sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> more coherent. I took the simpler path.
>

I thought latest work be Vlastimil tried to tackle that. To me, it feels
like page_poison=on and init_on_free=1 should bail out and disable one
of both things. Having both at the same time doesn't sound helpful.

> Tested the fix on rx3600.

Fixes: ?


--
Thanks,

David / dhildenb

2021-03-26 14:20:36

by Vlastimil Babka

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On 3/26/21 12:26 PM, Sergei Trofimovich wrote:
> init_on_free=1 does not guarantee that free pages contain only zero bytes.
>
> Some examples:
> 1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1

Yes, and it spits out a message that you enabled both and poisoning takes
precedence. It was that way even before my changes IIRC, but not consistent.

> 2. free_pages_prepare() always poisons pages:
>
> if (want_init_on_free())
> kernel_init_free_pages(page, 1 << order);
> kernel_poison_pages(page, 1 << order

kernel_poison_pages() includes a test if poisoning is enabled. And in that case
want_init_on_free() shouldn't be. see init_mem_debugging_and_hardening()

>
> I observed use of poisoned pages as the crash on ia64 booted with
> init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> There pmd page contained 0xaaaaaaaa poison pages and led to early crash.

Hm but that looks lika a sign that ia64 pmd allocation should use __GFP_ZERO and
doesn't. It shouldn't rely on init_on_alloc or init_on_free being enabled.

> The change drops the assumption that init_on_free=1 guarantees free
> pages to contain zeros.

The change assumes that page_poison=on also leaves want_init_on_free() enabled,
but it doesn't.

> Alternative would be to make interaction between runtime poisoning and
> sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> more coherent. I took the simpler path.

So that was done in 5.11 and the decisions can be seen in
init_mem_debugging_and_hardening(). There might be of course a bug, or later
changes broke something. Which was the version that you observed a bug?

> Tested the fix on rx3600.
>
> CC: Andrew Morton <[email protected]>
> CC: [email protected]
> Signed-off-by: Sergei Trofimovich <[email protected]>
> ---
> mm/page_alloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index cfc72873961d..d57d9b4f7089 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2301,7 +2301,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> kernel_unpoison_pages(page, 1 << order);
> set_page_owner(page, order, gfp_flags);
>
> - if (!want_init_on_free() && want_init_on_alloc(gfp_flags))
> + if (want_init_on_alloc(gfp_flags))
> kernel_init_free_pages(page, 1 << order);
> }
>
>

2021-03-26 15:02:36

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Fri, Mar 26, 2021 at 2:49 PM David Hildenbrand <[email protected]> wrote:
>
> > I observed use of poisoned pages as the crash on ia64 booted with
> > init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> > There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
> >
> > The change drops the assumption that init_on_free=1 guarantees free
> > pages to contain zeros.
> >
> > Alternative would be to make interaction between runtime poisoning and
> > sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> > more coherent. I took the simpler path.
> >
>
> I thought latest work be Vlastimil tried to tackle that. To me, it feels
> like page_poison=on and init_on_free=1 should bail out and disable one
> of both things. Having both at the same time doesn't sound helpful.

This is exactly how it works, see init_mem_debugging_and_hardening().

Sergei, could you elaborate more on what kind of crash this patch is
trying to fix? Where does it happen and why?

2021-03-26 16:43:58

by Sergei Trofimovich

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Fri, 26 Mar 2021 16:00:34 +0100
Andrey Konovalov <[email protected]> wrote:

> On Fri, Mar 26, 2021 at 2:49 PM David Hildenbrand <[email protected]> wrote:
> >
> > > I observed use of poisoned pages as the crash on ia64 booted with
> > > init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> > > There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
> > >
> > > The change drops the assumption that init_on_free=1 guarantees free
> > > pages to contain zeros.
> > >
> > > Alternative would be to make interaction between runtime poisoning and
> > > sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> > > more coherent. I took the simpler path.
> > >
> >
> > I thought latest work be Vlastimil tried to tackle that. To me, it feels
> > like page_poison=on and init_on_free=1 should bail out and disable one
> > of both things. Having both at the same time doesn't sound helpful.
>
> This is exactly how it works, see init_mem_debugging_and_hardening().
>
> Sergei, could you elaborate more on what kind of crash this patch is
> trying to fix? Where does it happen and why?

Yeah, I see I misinterpreted page_poison=on handling and misled you all.
Something else poisons a page when it should have not. I'll answer in more
detail to Vlastimil's email upthread and will provide more detail of the
unexpected poisoning I see.

--

Sergei

2021-03-26 17:27:20

by Sergei Trofimovich

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Fri, 26 Mar 2021 15:17:00 +0100
Vlastimil Babka <[email protected]> wrote:

> On 3/26/21 12:26 PM, Sergei Trofimovich wrote:
> > init_on_free=1 does not guarantee that free pages contain only zero bytes.
> >
> > Some examples:
> > 1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1
>
> Yes, and it spits out a message that you enabled both and poisoning takes
> precedence. It was that way even before my changes IIRC, but not consistent.

Yeah. I probably should not have included this case as page_poison=on actually
made my machine boot just fine. My main focus was to understand why I an seeing
the crash on kernel with init_on_alloc=1 init_on_free=1 and most debugging options
on.

My apologies! I'll try to find where this extra poisoning comes from.

Making a step back and explaining my setup:

Initially it's an ia64 box that manages to consistently corrupt memory
on socket free; https://lkml.org/lkml/2021/2/23/653

To get better understanding where corruption comes from I enabled
A Lot of VM, pagealloc and slab debugging options. Full config:

https://dev.gentoo.org/~slyfox/configs/guppy-config-5.12.0-rc4-00016-g427684abc9fd-dirty

I boot machine as:

[ 0.000000] Kernel command line: BOOT_IMAGE=/boot/vmlinuz-5.12.0-rc4-00016-g427684abc9fd-dirty root=/dev/sda3 ro slab_nomerge memblock=debug debug_pagealloc=1 hardened_usercopy=1 page_owner=on page_poison=0 init_on_alloc=1 init_on_free=1 debug_guardpage_minorder=0

My boot log:

https://dev.gentoo.org/~slyfox/bugs/ia64-boot-bug/2021-03-26-init_on_alloc-fail

Caveats in reading boot log:
- kernel crashes too early: stack unwinder does not have working kmalloc() yet
- kernel crashes in MCE handler: normally it should not. It's an unrelated bug
that makes backtrace useless. I'll try to fix it later, but it will not be fast.
- I added a bunch of printk()s around the crash.

The important pernel boot failure part is:
[ 0.000000] put_kernel_page: pmd=e000000100000000
[ 0.000000] pmd:(____ptrval____): aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa ................................

Note 1: I do not really enable page_poison at runtime and was misleading you
in previous emails. (I initially assumed kernel_poison_pages() poisons pages
unconditionally but you all explained it does not). Something else manages to
poison my pmd(s?).

Note 2: I have many other debugging options enabled that might trigger
poisoning.

> > 2. free_pages_prepare() always poisons pages:
> >
> > if (want_init_on_free())
> > kernel_init_free_pages(page, 1 << order);
> > kernel_poison_pages(page, 1 << order
>
> kernel_poison_pages() includes a test if poisoning is enabled. And in that case
> want_init_on_free() shouldn't be. see init_mem_debugging_and_hardening()

I completely missed that! Thank you! Will try to trace real cause of poisoning.

> > I observed use of poisoned pages as the crash on ia64 booted with
> > init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> > There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
>
> Hm but that looks lika a sign that ia64 pmd allocation should use __GFP_ZERO and
> doesn't. It shouldn't rely on init_on_alloc or init_on_free being enabled.

ia64 does use __GFP_ZERO (I even tried to add it manually to pmd_alloc_one()
before I realized all _PGTABLEs imply __GFP_ZERO).

I'll provide the call chain I arrived at for completeness:
- [ia64 boots]
- mem_init() (defined at arch/ia64/mm/init.c)
-> setup_gate() (defined at arch/ia64/mm/init.c)
-> put_kernel_page() (defined at arch/ia64/mm/init.c)
-> [NOTE: from now on it's all generic code, not ia64-speficic]
-> pmd_alloc() (defined at include/linux/mm.h)
-> __pmd_alloc() (defined at mm/memory.c)
-> [under #ifndef __PAGETABLE_PMD_FOLDED] pmd_alloc_one() (defined at include/asm-generic/pgalloc.h)
-> pmd_alloc_one() [defined at include/asm-generic/pgalloc.h):

static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
{
struct page *page;
gfp_t gfp = GFP_PGTABLE_USER;

if (mm == &init_mm)
gfp = GFP_PGTABLE_KERNEL;
page = alloc_pages(gfp, 0);
if (!page)
return NULL;
if (!pgtable_pmd_page_ctor(page)) {
__free_pages(page, 0);
return NULL;
}
return (pmd_t *)page_address(page);
}

In our case it is a GFP_PGTABLE_KERNEL with __GFP_ZERO and result is
poisoned page instead of zeroed page.

If I interpret the above correctly it means that something (probably
memalloc_free_pages() ?) puts initial free pages as poisoned and later
alloc_pages() assumes they are memset()-zero. But I don't see why.

> > The change drops the assumption that init_on_free=1 guarantees free
> > pages to contain zeros.
>
> The change assumes that page_poison=on also leaves want_init_on_free() enabled,
> but it doesn't.
>
> > Alternative would be to make interaction between runtime poisoning and
> > sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> > more coherent. I took the simpler path.
>
> So that was done in 5.11 and the decisions can be seen in
> init_mem_debugging_and_hardening(). There might be of course a bug, or later
> changes broke something. Which was the version that you observed a bug?
>
> > Tested the fix on rx3600.
> >
> > CC: Andrew Morton <[email protected]>
> > CC: [email protected]
> > Signed-off-by: Sergei Trofimovich <[email protected]>
> > ---
> > mm/page_alloc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index cfc72873961d..d57d9b4f7089 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2301,7 +2301,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> > kernel_unpoison_pages(page, 1 << order);
> > set_page_owner(page, order, gfp_flags);
> >
> > - if (!want_init_on_free() && want_init_on_alloc(gfp_flags))
> > + if (want_init_on_alloc(gfp_flags))
> > kernel_init_free_pages(page, 1 << order);
> > }
> >
> >
>

--

Sergei

2021-03-27 18:07:50

by Sergei Trofimovich

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Fri, 26 Mar 2021 17:25:22 +0000
Sergei Trofimovich <[email protected]> wrote:

> On Fri, 26 Mar 2021 15:17:00 +0100
> Vlastimil Babka <[email protected]> wrote:
>
> > On 3/26/21 12:26 PM, Sergei Trofimovich wrote:
> > > init_on_free=1 does not guarantee that free pages contain only zero bytes.
> > >
> > > Some examples:
> > > 1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1
> >
> > Yes, and it spits out a message that you enabled both and poisoning takes
> > precedence. It was that way even before my changes IIRC, but not consistent.
>
> Yeah. I probably should not have included this case as page_poison=on actually
> made my machine boot just fine. My main focus was to understand why I an seeing
> the crash on kernel with init_on_alloc=1 init_on_free=1 and most debugging options
> on.
>
> My apologies! I'll try to find where this extra poisoning comes from.
>
> Making a step back and explaining my setup:
>
> Initially it's an ia64 box that manages to consistently corrupt memory
> on socket free; https://lkml.org/lkml/2021/2/23/653
>
> To get better understanding where corruption comes from I enabled
> A Lot of VM, pagealloc and slab debugging options. Full config:
>
> https://dev.gentoo.org/~slyfox/configs/guppy-config-5.12.0-rc4-00016-g427684abc9fd-dirty
>
> I boot machine as:
>
> [ 0.000000] Kernel command line: BOOT_IMAGE=/boot/vmlinuz-5.12.0-rc4-00016-g427684abc9fd-dirty root=/dev/sda3 ro slab_nomerge memblock=debug debug_pagealloc=1 hardened_usercopy=1 page_owner=on page_poison=0 init_on_alloc=1 init_on_free=1 debug_guardpage_minorder=0
>
> My boot log:
>
> https://dev.gentoo.org/~slyfox/bugs/ia64-boot-bug/2021-03-26-init_on_alloc-fail
>
> Caveats in reading boot log:
> - kernel crashes too early: stack unwinder does not have working kmalloc() yet
> - kernel crashes in MCE handler: normally it should not. It's an unrelated bug
> that makes backtrace useless. I'll try to fix it later, but it will not be fast.
> - I added a bunch of printk()s around the crash.
>
> The important pernel boot failure part is:
> [ 0.000000] put_kernel_page: pmd=e000000100000000
> [ 0.000000] pmd:(____ptrval____): aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa ................................

I added WARN_ON_ONCE(1) to __kernel_poison_pages() to get the idea where
poisoning comes from and got it at:

[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at mm/page_poison.c:40 __kernel_poison_pages+0x1a0/0x1c0
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.12.0-rc4-00016-g427684abc9fd-dirty #196
Call Trace:
[ 0.000000] [<a0000001000151b0>] show_stack+0x90/0xc0
[ 0.000000] [<a000000101162490>] dump_stack+0x150/0x1c0
[ 0.000000] [<a00000010115a7b0>] __warn+0x180/0x220
[ 0.000000] [<a00000010115a910>] warn_slowpath_fmt+0xc0/0x100
[ 0.000000] [<a0000001003f02e0>] __kernel_poison_pages+0x1a0/0x1c0
[ 0.000000] [<a0000001003ba0a0>] __free_pages_ok+0x2a0/0x10c0
[ 0.000000] [<a0000001003bb9d0>] __free_pages_core+0x2d0/0x480
[ 0.000000] [<a0000001014b7050>] memblock_free_pages+0x30/0x50
[ 0.000000] [<a0000001014bb430>] memblock_free_all+0x280/0x3c0
[ 0.000000] [<a00000010149f540>] mem_init+0x70/0x2d0
[ 0.000000] [<a000000101491550>] start_kernel+0x670/0xc20
[ 0.000000] [<a00000010116e920>] start_ap+0x760/0x780
[ 0.000000] ---[ end trace 0000000000000000 ]---

I think I found where page_poison=on get enabled at init_mem_debugging_and_hardening():

void init_mem_debugging_and_hardening(void)
{
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_PAGE_POISONING
/*
* Page poisoning is debug page alloc for some arches. If
* either of those options are enabled, enable poisoning.
*/
if (page_poisoning_enabled() ||
(!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
debug_pagealloc_enabled()))
static_branch_enable(&_page_poisoning_enabled); // <- HERE
#endif
...
}

If I follow the code correctly to trigger the problem one needs to:
- have PAGE_POISONING=y
- have page_poison=off set (or just unset)
- have arch without ARCH_SUPPORTS_DEBUG_PAGEALLOC (ia64 is one of
such arches)
- have init_on_free=1
- have debug_pagealloc=1

That way we get both executed:
- static_branch_enable(&init_on_free);
- static_branch_enable(&_page_poisoning_enabled);

Sounds plausible? I'll send another version of the patch that also
fixes corruption for me.

> Note 1: I do not really enable page_poison at runtime and was misleading you
> in previous emails. (I initially assumed kernel_poison_pages() poisons pages
> unconditionally but you all explained it does not). Something else manages to
> poison my pmd(s?).
>
> Note 2: I have many other debugging options enabled that might trigger
> poisoning.
>
> > > 2. free_pages_prepare() always poisons pages:
> > >
> > > if (want_init_on_free())
> > > kernel_init_free_pages(page, 1 << order);
> > > kernel_poison_pages(page, 1 << order
> >
> > kernel_poison_pages() includes a test if poisoning is enabled. And in that case
> > want_init_on_free() shouldn't be. see init_mem_debugging_and_hardening()
>
> I completely missed that! Thank you! Will try to trace real cause of poisoning.
>
> > > I observed use of poisoned pages as the crash on ia64 booted with
> > > init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
> > > There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
> >
> > Hm but that looks lika a sign that ia64 pmd allocation should use __GFP_ZERO and
> > doesn't. It shouldn't rely on init_on_alloc or init_on_free being enabled.
>
> ia64 does use __GFP_ZERO (I even tried to add it manually to pmd_alloc_one()
> before I realized all _PGTABLEs imply __GFP_ZERO).
>
> I'll provide the call chain I arrived at for completeness:
> - [ia64 boots]
> - mem_init() (defined at arch/ia64/mm/init.c)
> -> setup_gate() (defined at arch/ia64/mm/init.c)
> -> put_kernel_page() (defined at arch/ia64/mm/init.c)
> -> [NOTE: from now on it's all generic code, not ia64-speficic]
> -> pmd_alloc() (defined at include/linux/mm.h)
> -> __pmd_alloc() (defined at mm/memory.c)
> -> [under #ifndef __PAGETABLE_PMD_FOLDED] pmd_alloc_one() (defined at include/asm-generic/pgalloc.h)
> -> pmd_alloc_one() [defined at include/asm-generic/pgalloc.h):
>
> static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
> {
> struct page *page;
> gfp_t gfp = GFP_PGTABLE_USER;
>
> if (mm == &init_mm)
> gfp = GFP_PGTABLE_KERNEL;
> page = alloc_pages(gfp, 0);
> if (!page)
> return NULL;
> if (!pgtable_pmd_page_ctor(page)) {
> __free_pages(page, 0);
> return NULL;
> }
> return (pmd_t *)page_address(page);
> }
>
> In our case it is a GFP_PGTABLE_KERNEL with __GFP_ZERO and result is
> poisoned page instead of zeroed page.
>
> If I interpret the above correctly it means that something (probably
> memalloc_free_pages() ?) puts initial free pages as poisoned and later
> alloc_pages() assumes they are memset()-zero. But I don't see why.
>
> > > The change drops the assumption that init_on_free=1 guarantees free
> > > pages to contain zeros.
> >
> > The change assumes that page_poison=on also leaves want_init_on_free() enabled,
> > but it doesn't.
> >
> > > Alternative would be to make interaction between runtime poisoning and
> > > sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
> > > more coherent. I took the simpler path.
> >
> > So that was done in 5.11 and the decisions can be seen in
> > init_mem_debugging_and_hardening(). There might be of course a bug, or later
> > changes broke something. Which was the version that you observed a bug?
> >
> > > Tested the fix on rx3600.
> > >
> > > CC: Andrew Morton <[email protected]>
> > > CC: [email protected]
> > > Signed-off-by: Sergei Trofimovich <[email protected]>
> > > ---
> > > mm/page_alloc.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index cfc72873961d..d57d9b4f7089 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -2301,7 +2301,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> > > kernel_unpoison_pages(page, 1 << order);
> > > set_page_owner(page, order, gfp_flags);
> > >
> > > - if (!want_init_on_free() && want_init_on_alloc(gfp_flags))
> > > + if (want_init_on_alloc(gfp_flags))
> > > kernel_init_free_pages(page, 1 << order);
> > > }
> > >
> > >
> >
>
> --
>
> Sergei


--

Sergei

2021-03-27 18:25:26

by Sergei Trofimovich

[permalink] [raw]
Subject: [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1

On !ARCH_SUPPORTS_DEBUG_PAGEALLOC (like ia64) debug_pagealloc=1
implies page_poison=on:

if (page_poisoning_enabled() ||
(!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
debug_pagealloc_enabled()))
static_branch_enable(&_page_poisoning_enabled);

page_poison=on needs to init_on_free=1.

Before the change id happened too late for the following case:
- have PAGE_POISONING=y
- have page_poison unset
- have !ARCH_SUPPORTS_DEBUG_PAGEALLOC arch (like ia64)
- have init_on_free=1
- have debug_pagealloc=1

That way we get both keys enabled:
- static_branch_enable(&init_on_free);
- static_branch_enable(&_page_poisoning_enabled);

which leads to poisoned pages returned for __GFP_ZERO pages.

After the change we execute only:
- static_branch_enable(&_page_poisoning_enabled);
and ignore init_on_free=1.

CC: Vlastimil Babka <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
CC: David Hildenbrand <[email protected]>
CC: Andrey Konovalov <[email protected]>
Link: https://lkml.org/lkml/2021/3/26/443
Signed-off-by: Sergei Trofimovich <[email protected]>
---
mm/page_alloc.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d57d9b4f7089..10a8a1d28c11 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -764,32 +764,36 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
*/
void init_mem_debugging_and_hardening(void)
{
+ bool page_poison_requested = page_poisoning_enabled();
+
+#ifdef CONFIG_PAGE_POISONING
+ /*
+ * Page poisoning is debug page alloc for some arches. If
+ * either of those options are enabled, enable poisoning.
+ */
+ if (page_poisoning_enabled() ||
+ (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
+ debug_pagealloc_enabled())) {
+ static_branch_enable(&_page_poisoning_enabled);
+ page_poison_requested = true;
+ }
+#endif
+
if (_init_on_alloc_enabled_early) {
- if (page_poisoning_enabled())
+ if (page_poison_requested)
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())
+ if (page_poison_requested)
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_PAGE_POISONING
- /*
- * Page poisoning is debug page alloc for some arches. If
- * either of those options are enabled, enable poisoning.
- */
- if (page_poisoning_enabled() ||
- (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
- debug_pagealloc_enabled()))
- static_branch_enable(&_page_poisoning_enabled);
-#endif
-
#ifdef CONFIG_DEBUG_PAGEALLOC
if (!debug_pagealloc_enabled())
return;
--
2.31.0

2021-03-29 09:26:09

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1

On 27.03.21 19:21, Sergei Trofimovich wrote:
> On !ARCH_SUPPORTS_DEBUG_PAGEALLOC (like ia64) debug_pagealloc=1
> implies page_poison=on:
>
> if (page_poisoning_enabled() ||
> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> debug_pagealloc_enabled()))
> static_branch_enable(&_page_poisoning_enabled);
>
> page_poison=on needs to init_on_free=1.
>
> Before the change id happened too late for the following case:
> - have PAGE_POISONING=y
> - have page_poison unset
> - have !ARCH_SUPPORTS_DEBUG_PAGEALLOC arch (like ia64)
> - have init_on_free=1
> - have debug_pagealloc=1
>
> That way we get both keys enabled:
> - static_branch_enable(&init_on_free);
> - static_branch_enable(&_page_poisoning_enabled);
>
> which leads to poisoned pages returned for __GFP_ZERO pages.
>
> After the change we execute only:
> - static_branch_enable(&_page_poisoning_enabled);
> and ignore init_on_free=1.
>
> CC: Vlastimil Babka <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: [email protected]
> CC: David Hildenbrand <[email protected]>
> CC: Andrey Konovalov <[email protected]>
> Link: https://lkml.org/lkml/2021/3/26/443

Again, Fixes: tag? IOW, which commit initially broke it.

> Signed-off-by: Sergei Trofimovich <[email protected]>
> ---
> mm/page_alloc.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d57d9b4f7089..10a8a1d28c11 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -764,32 +764,36 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
> */
> void init_mem_debugging_and_hardening(void)
> {
> + bool page_poison_requested = page_poisoning_enabled();

s/page_poison_requested/page_poisoning_requested/

And I wonder if you should just initialize to "false" here.

Without CONFIG_PAGE_POISONING, page_poisoning_enabled() will always
return false, so it seems unnecessary.

> +
> +#ifdef CONFIG_PAGE_POISONING
> + /*
> + * Page poisoning is debug page alloc for some arches. If
> + * either of those options are enabled, enable poisoning.
> + */
> + if (page_poisoning_enabled() ||
> + (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> + debug_pagealloc_enabled())) {
> + static_branch_enable(&_page_poisoning_enabled);
> + page_poison_requested = true;
> + }
> +#endif

Apart from that, looks good.


--
Thanks,

David / dhildenb

2021-03-29 12:00:30

by Vlastimil Babka

[permalink] [raw]
Subject: Re: [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1

On 3/27/21 7:21 PM, Sergei Trofimovich wrote:
> On !ARCH_SUPPORTS_DEBUG_PAGEALLOC (like ia64) debug_pagealloc=1
> implies page_poison=on:
>
> if (page_poisoning_enabled() ||
> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> debug_pagealloc_enabled()))
> static_branch_enable(&_page_poisoning_enabled);
>
> page_poison=on needs to init_on_free=1.
>
> Before the change id happened too late for the following case:
> - have PAGE_POISONING=y
> - have page_poison unset
> - have !ARCH_SUPPORTS_DEBUG_PAGEALLOC arch (like ia64)
> - have init_on_free=1
> - have debug_pagealloc=1
>
> That way we get both keys enabled:
> - static_branch_enable(&init_on_free);
> - static_branch_enable(&_page_poisoning_enabled);
>
> which leads to poisoned pages returned for __GFP_ZERO pages.

Good catch, thanks for finding the root cause!

> After the change we execute only:
> - static_branch_enable(&_page_poisoning_enabled);
> and ignore init_on_free=1.
> CC: Vlastimil Babka <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: [email protected]
> CC: David Hildenbrand <[email protected]>
> CC: Andrey Konovalov <[email protected]>
> Link: https://lkml.org/lkml/2021/3/26/443
> Signed-off-by: Sergei Trofimovich <[email protected]>

Acked-by: Vlastimil Babka <[email protected]>
8db26a3d4735 ("mm, page_poison: use static key more efficiently")
Cc: <[email protected]>

> ---
> mm/page_alloc.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d57d9b4f7089..10a8a1d28c11 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -764,32 +764,36 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
> */
> void init_mem_debugging_and_hardening(void)
> {
> + bool page_poison_requested = page_poisoning_enabled();
> +
> +#ifdef CONFIG_PAGE_POISONING
> + /*
> + * Page poisoning is debug page alloc for some arches. If
> + * either of those options are enabled, enable poisoning.
> + */
> + if (page_poisoning_enabled() ||
> + (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> + debug_pagealloc_enabled())) {
> + static_branch_enable(&_page_poisoning_enabled);
> + page_poison_requested = true;
> + }
> +#endif
> +
> if (_init_on_alloc_enabled_early) {
> - if (page_poisoning_enabled())
> + if (page_poison_requested)
> 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())
> + if (page_poison_requested)
> 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_PAGE_POISONING
> - /*
> - * Page poisoning is debug page alloc for some arches. If
> - * either of those options are enabled, enable poisoning.
> - */
> - if (page_poisoning_enabled() ||
> - (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> - debug_pagealloc_enabled()))
> - static_branch_enable(&_page_poisoning_enabled);
> -#endif
> -
> #ifdef CONFIG_DEBUG_PAGEALLOC
> if (!debug_pagealloc_enabled())
> return;
>

2021-03-29 12:12:19

by Vlastimil Babka

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On 3/26/21 2:48 PM, David Hildenbrand wrote:
> On 26.03.21 12:26, Sergei Trofimovich wrote:
>> init_on_free=1 does not guarantee that free pages contain only zero bytes.
>>
>> Some examples:
>> 1. page_poison=on takes presedence over init_on_alloc=1 / ini_on_free=1
>
> s/ini_on_free/init_on_free/
>
>> 2. free_pages_prepare() always poisons pages:
>>
>>         if (want_init_on_free())
>>             kernel_init_free_pages(page, 1 << order);
>>         kernel_poison_pages(page, 1 << order
>
> In next/master, it's the other way around already.

And that should be OK as the order should not matter, as long as they are indeed
exclusive. They should be after Sergei's v2 fix.
As long as kasan_free_nondeferred_pages() which follows doesn't do anything
unexpected to poisoned pages (I haven't check).


> commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82
> Author: Andrey Konovalov <[email protected]>
> Date:   Thu Mar 18 17:01:40 2021 +1100
>
>     kasan, mm: integrate page_alloc init with HW_TAGS

But the mmotm patch/-next commit also changes post_alloc_hook()

Before the patch it was:
kernel_unpoison_pages(page, 1 << order);
...
kernel_init_free_pages(page, 1 << order);

Now it is (for !kasan_has_integrated_init()):

kernel_init_free_pages(page, 1 << order);

kernel_unpoison_pages(page, 1 << order);

That has to be wrong, because we init the page with zeroes and then call
kernel_unpoison_pages() which checks for the 0xaa pattern. Andrey?

>>
>> I observed use of poisoned pages as the crash on ia64 booted with
>> init_on_free=1 init_on_alloc=1 (CONFIG_PAGE_POISONING=y config).
>> There pmd page contained 0xaaaaaaaa poison pages and led to early crash.
>>
>> The change drops the assumption that init_on_free=1 guarantees free
>> pages to contain zeros.
>>
>> Alternative would be to make interaction between runtime poisoning and
>> sanitizing options and build-time debug flags like CONFIG_PAGE_POISONING
>> more coherent. I took the simpler path.
>>
>
> I thought latest work be Vlastimil tried to tackle that. To me, it feels like
> page_poison=on  and init_on_free=1 should bail out and disable one of both
> things. Having both at the same time doesn't sound helpful.
>
>> Tested the fix on rx3600.
>
> Fixes: ?
>
>

2021-03-29 22:02:35

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <[email protected]> wrote:
>
> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82
> > Author: Andrey Konovalov <[email protected]>
> > Date: Thu Mar 18 17:01:40 2021 +1100
> >
> > kasan, mm: integrate page_alloc init with HW_TAGS
>
> But the mmotm patch/-next commit also changes post_alloc_hook()
>
> Before the patch it was:
> kernel_unpoison_pages(page, 1 << order);
> ...
> kernel_init_free_pages(page, 1 << order);
>
> Now it is (for !kasan_has_integrated_init()):
>
> kernel_init_free_pages(page, 1 << order);
>
> kernel_unpoison_pages(page, 1 << order);
>
> That has to be wrong, because we init the page with zeroes and then call
> kernel_unpoison_pages() which checks for the 0xaa pattern. Andrey?

It's similar to free_pages_prepare(): kernel_unpoison_pages() and
want_init_on_alloc() are exclusive, so the order shouldn't matter. Am
I missing something?

2021-03-29 22:08:36

by Vlastimil Babka

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On 3/30/21 12:00 AM, Andrey Konovalov wrote:
> On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <[email protected]> wrote:
>>
>> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82
>> > Author: Andrey Konovalov <[email protected]>
>> > Date: Thu Mar 18 17:01:40 2021 +1100
>> >
>> > kasan, mm: integrate page_alloc init with HW_TAGS
>>
>> But the mmotm patch/-next commit also changes post_alloc_hook()
>>
>> Before the patch it was:
>> kernel_unpoison_pages(page, 1 << order);
>> ...
>> kernel_init_free_pages(page, 1 << order);
>>
>> Now it is (for !kasan_has_integrated_init()):
>>
>> kernel_init_free_pages(page, 1 << order);
>>
>> kernel_unpoison_pages(page, 1 << order);
>>
>> That has to be wrong, because we init the page with zeroes and then call
>> kernel_unpoison_pages() which checks for the 0xaa pattern. Andrey?
>
> It's similar to free_pages_prepare(): kernel_unpoison_pages() and
> want_init_on_alloc() are exclusive, so the order shouldn't matter. Am
> I missing something?

Yeah, when the allocation has __GFP_ZERO, want_init_on_alloc() will return true
even with the static branches disabled.

2021-03-29 22:31:56

by Sergei Trofimovich

[permalink] [raw]
Subject: [PATCH v3] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1

On !ARCH_SUPPORTS_DEBUG_PAGEALLOC (like ia64) debug_pagealloc=1
implies page_poison=on:

if (page_poisoning_enabled() ||
(!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
debug_pagealloc_enabled()))
static_branch_enable(&_page_poisoning_enabled);

page_poison=on needs to override init_on_free=1.

Before the change it did not work as expected for the following case:
- have PAGE_POISONING=y
- have page_poison unset
- have !ARCH_SUPPORTS_DEBUG_PAGEALLOC arch (like ia64)
- have init_on_free=1
- have debug_pagealloc=1

That way we get both keys enabled:
- static_branch_enable(&init_on_free);
- static_branch_enable(&_page_poisoning_enabled);

which leads to poisoned pages returned for __GFP_ZERO pages.

After the change we execute only:
- static_branch_enable(&_page_poisoning_enabled);
and ignore init_on_free=1.

Acked-by: Vlastimil Babka <[email protected]>
Fixes: 8db26a3d4735 ("mm, page_poison: use static key more efficiently")
Cc: <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
CC: David Hildenbrand <[email protected]>
CC: Andrey Konovalov <[email protected]>
Link: https://lkml.org/lkml/2021/3/26/443

Signed-off-by: Sergei Trofimovich <[email protected]>
---
Change since v2:
- Added 'Fixes:' and 'CC: stable@' suggested by Vlastimil and David
- Renamed local variable to 'page_poisoning_requested' for
consistency suggested by David
- Simplified initialization of page_poisoning_requested suggested
by David
- Added 'Acked-by: Vlastimil'

mm/page_alloc.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cfc72873961d..4bb3cdfc47f8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -764,32 +764,36 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
*/
void init_mem_debugging_and_hardening(void)
{
+ bool page_poisoning_requested = false;
+
+#ifdef CONFIG_PAGE_POISONING
+ /*
+ * Page poisoning is debug page alloc for some arches. If
+ * either of those options are enabled, enable poisoning.
+ */
+ if (page_poisoning_enabled() ||
+ (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
+ debug_pagealloc_enabled())) {
+ static_branch_enable(&_page_poisoning_enabled);
+ page_poisoning_requested = true;
+ }
+#endif
+
if (_init_on_alloc_enabled_early) {
- if (page_poisoning_enabled())
+ if (page_poisoning_requested)
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())
+ if (page_poisoning_requested)
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_PAGE_POISONING
- /*
- * Page poisoning is debug page alloc for some arches. If
- * either of those options are enabled, enable poisoning.
- */
- if (page_poisoning_enabled() ||
- (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
- debug_pagealloc_enabled()))
- static_branch_enable(&_page_poisoning_enabled);
-#endif
-
#ifdef CONFIG_DEBUG_PAGEALLOC
if (!debug_pagealloc_enabled())
return;
--
2.31.1

2021-03-30 08:41:39

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH v3] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1

On 30.03.21 00:25, Sergei Trofimovich wrote:
> On !ARCH_SUPPORTS_DEBUG_PAGEALLOC (like ia64) debug_pagealloc=1
> implies page_poison=on:
>
> if (page_poisoning_enabled() ||
> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> debug_pagealloc_enabled()))
> static_branch_enable(&_page_poisoning_enabled);
>
> page_poison=on needs to override init_on_free=1.
>
> Before the change it did not work as expected for the following case:
> - have PAGE_POISONING=y
> - have page_poison unset
> - have !ARCH_SUPPORTS_DEBUG_PAGEALLOC arch (like ia64)
> - have init_on_free=1
> - have debug_pagealloc=1
>
> That way we get both keys enabled:
> - static_branch_enable(&init_on_free);
> - static_branch_enable(&_page_poisoning_enabled);
>
> which leads to poisoned pages returned for __GFP_ZERO pages.
>
> After the change we execute only:
> - static_branch_enable(&_page_poisoning_enabled);
> and ignore init_on_free=1.
>
> Acked-by: Vlastimil Babka <[email protected]>
> Fixes: 8db26a3d4735 ("mm, page_poison: use static key more efficiently")
> Cc: <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: [email protected]
> CC: David Hildenbrand <[email protected]>
> CC: Andrey Konovalov <[email protected]>
> Link: https://lkml.org/lkml/2021/3/26/443
>
> Signed-off-by: Sergei Trofimovich <[email protected]>
> ---
> Change since v2:
> - Added 'Fixes:' and 'CC: stable@' suggested by Vlastimil and David
> - Renamed local variable to 'page_poisoning_requested' for
> consistency suggested by David
> - Simplified initialization of page_poisoning_requested suggested
> by David
> - Added 'Acked-by: Vlastimil'
>
> mm/page_alloc.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index cfc72873961d..4bb3cdfc47f8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -764,32 +764,36 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
> */
> void init_mem_debugging_and_hardening(void)
> {
> + bool page_poisoning_requested = false;
> +
> +#ifdef CONFIG_PAGE_POISONING
> + /*
> + * Page poisoning is debug page alloc for some arches. If
> + * either of those options are enabled, enable poisoning.
> + */
> + if (page_poisoning_enabled() ||
> + (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> + debug_pagealloc_enabled())) {
> + static_branch_enable(&_page_poisoning_enabled);
> + page_poisoning_requested = true;
> + }
> +#endif
> +
> if (_init_on_alloc_enabled_early) {
> - if (page_poisoning_enabled())
> + if (page_poisoning_requested)
> 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())
> + if (page_poisoning_requested)
> 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_PAGE_POISONING
> - /*
> - * Page poisoning is debug page alloc for some arches. If
> - * either of those options are enabled, enable poisoning.
> - */
> - if (page_poisoning_enabled() ||
> - (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> - debug_pagealloc_enabled()))
> - static_branch_enable(&_page_poisoning_enabled);
> -#endif
> -
> #ifdef CONFIG_DEBUG_PAGEALLOC
> if (!debug_pagealloc_enabled())
> return;
>

Thanks!

Reviewed-by: David Hildenbrand <[email protected]>

--
Thanks,

David / dhildenb

2021-03-30 14:51:54

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc

On Tue, Mar 30, 2021 at 12:07 AM Vlastimil Babka <[email protected]> wrote:
>
> On 3/30/21 12:00 AM, Andrey Konovalov wrote:
> > On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <[email protected]> wrote:
> >>
> >> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82
> >> > Author: Andrey Konovalov <[email protected]>
> >> > Date: Thu Mar 18 17:01:40 2021 +1100
> >> >
> >> > kasan, mm: integrate page_alloc init with HW_TAGS
> >>
> >> But the mmotm patch/-next commit also changes post_alloc_hook()
> >>
> >> Before the patch it was:
> >> kernel_unpoison_pages(page, 1 << order);
> >> ...
> >> kernel_init_free_pages(page, 1 << order);
> >>
> >> Now it is (for !kasan_has_integrated_init()):
> >>
> >> kernel_init_free_pages(page, 1 << order);
> >>
> >> kernel_unpoison_pages(page, 1 << order);
> >>
> >> That has to be wrong, because we init the page with zeroes and then call
> >> kernel_unpoison_pages() which checks for the 0xaa pattern. Andrey?
> >
> > It's similar to free_pages_prepare(): kernel_unpoison_pages() and
> > want_init_on_alloc() are exclusive, so the order shouldn't matter. Am
> > I missing something?
>
> Yeah, when the allocation has __GFP_ZERO, want_init_on_alloc() will return true
> even with the static branches disabled.

Ah, I see. I'll post a fix soon.

Thank you!