Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Wednesday, July 14th, 2021 at 5:10 AM, Kees Cook [off-list ref] wrote:

On Mon, Jul 12, 2021 at 10:58:16PM +0100, Sergei Trofimovich wrote:

quoted

To reproduce the failure we need the following system:

quoted

-   kernel command: page_poison=1 init_on_free=0 init_on_alloc=0
    

quoted

-   kernel config:
    

quoted

    -   CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y
    -   CONFIG_INIT_ON_FREE_DEFAULT_ON=y
    -   CONFIG_PAGE_POISONING=y
    

quoted

    0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
    

quoted

    0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
    

quoted

    00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
    

quoted

    CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1
    

quoted

    Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021
    

quoted

    Call Trace:
    

quoted

    dump_stack+0x64/0x7c
    

quoted

    __kernel_unpoison_pages.cold+0x48/0x84
    

quoted

    post_alloc_hook+0x60/0xa0
    

quoted

    get_page_from_freelist+0xdb8/0x1000
    

quoted

    __alloc_pages+0x163/0x2b0
    

quoted

    __get_free_pages+0xc/0x30
    

quoted

    pgd_alloc+0x2e/0x1a0
    

quoted

    ? dup_mm+0x37/0x4f0
    

quoted

    mm_init+0x185/0x270
    

quoted

    dup_mm+0x6b/0x4f0
    

quoted

    ? __lock_task_sighand+0x35/0x70
    

quoted

    copy_process+0x190d/0x1b10
    

quoted

    kernel_clone+0xba/0x3b0
    

quoted

    __do_sys_clone+0x8f/0xb0
    

quoted

    do_syscall_64+0x68/0x80
    

quoted

    ? do_syscall_64+0x11/0x80
    

quoted

    entry_SYSCALL_64_after_hwframe+0x44/0xae
    

quoted

quoted

Before the 51cba1eb ("init_on_alloc: Optimize static branches")

quoted

init_on_alloc never enabled static branch by default. It could

quoted

only be enabed explicitly by init_mem_debugging_and_hardening().

But init_mem_debugging_and_hardening() is always called (by mm_init()).

quoted

But after the 51cba1eb static branch could already be enabled

quoted

by default. There was no code to ever disable it. That caused

quoted

page_poison=1 / init_on_free=1 conflict.

quoted

This change extends init_mem_debugging_and_hardening() to also

quoted

disable static branch disabling.

quoted

CC: Andrew Morton akpm@linux-foundation.org

quoted

CC: Kees Cook keescook@chromium.org

quoted

CC: Alexander Potapenko glider@google.com

quoted

CC: Thomas Gleixner tglx@linutronix.de

quoted

CC: Vlastimil Babka vbabka@suse.cz

quoted

CC: linux-mm@kvack.org

quoted

Reported-by: bowsingbetee@pm.me

quoted

Reported-by: Mikhail Morfikov

quoted

Fixes: 51cba1eb ("init_on_alloc: Optimize static branches")

quoted

Signed-off-by: Sergei Trofimovich slyfox@gentoo.org
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

quoted

mm/page_alloc.c | 16 ++++++++++------

quoted

1 file changed, 10 insertions(+), 6 deletions(-)

quoted

diff --git a/mm/page_alloc.c b/mm/page_alloc.c

quoted

index 3b97e17806be..46cb4a9c2b50 100644

quoted

--- a/mm/page_alloc.c

quoted

+++ b/mm/page_alloc.c

quoted

@@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void)

quoted

}

quoted

#endif

quoted

-   if (_init_on_alloc_enabled_early) {
-       if (page_poisoning_requested)
        

quoted

quoted

quoted

-   if (_init_on_alloc_enabled_early ||
-       IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) {
        

quoted

This doesn't look right. _init_on_alloc_enabled_early already has the

same value:

static bool _init_on_alloc_enabled_early __read_mostly

= IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON);

So checking this is just a side-effect of how static_branch_maybe()

happens to be behaving.

quoted

-       if (page_poisoning_requested) {
        	pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
        		"will take precedence over init_on_alloc\\n");
        

quoted

quoted

quoted

-       else
        

quoted

quoted

quoted

-       	static_branch_disable(&init_on_alloc);
        

quoted

quoted

-       } else
        	static_branch_enable(&init_on_alloc);
        

quoted

quoted

    }

quoted

-   if (_init_on_free_enabled_early) {
-       if (page_poisoning_requested)
        

quoted

quoted

quoted

-   if (_init_on_free_enabled_early ||
-       IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) {
        

quoted

quoted

-       if (page_poisoning_requested) {
        	pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
        		"will take precedence over init_on_free\\n");
        

quoted

quoted

quoted

-       else
        

quoted

quoted

quoted

-       	static_branch_disable(&init_on_free);
        

quoted

quoted

-       } else
        	static_branch_enable(&init_on_free);
        

quoted

quoted

    }

I think it would be better to clean this up without additional

confusion involving the CONFIGs:

index 3b97e17806be..1f19365bc158 100644

}

#endif

-   if (_init_on_alloc_enabled_early) {
-         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_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);
        

-   if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
-         page_poisoning_requested) {
        

-         pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
        

-         	"will take precedence over init_on_alloc and init_on_free\\n");
        

B & C variants show this message and A does not, which is what I would expect to happen.

Tested variants:

A. "page_poison=1 init_on_free=0 init_on_alloc=0 slub_debug=P"
B. "page_poison=1 init_on_free=0 init_on_alloc=0 slub_debug=P init_on_free=1"
C. "page_poison=1 slub_debug=P"

in common:
CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y
CONFIG_INIT_ON_FREE_DEFAULT_ON=y
CONFIG_PAGE_POISONING=y
CONFIG_SLUB_DEBUG=y
CONFIG_SLUB=y

(the slub parts don't matter, ignore them)

-         _init_on_alloc_enabled_early = false;
        

-         _init_on_free_enabled_early = false;
        

    }
-   if (_init_on_alloc_enabled_early)
-         static_branch_enable(&init_on_alloc);
        

-   else
-         static_branch_disable(&init_on_alloc);
        

-   if (_init_on_free_enabled_early)
-         static_branch_enable(&init_on_free);
        

-   else
-         static_branch_disable(&init_on_free);
        

#ifdef CONFIG_DEBUG_PAGEALLOC

if (!debug_pagealloc_enabled())

return;

------------------------------------------------------------------------

Kees Cook

While both patches(Sergei's, and yours) work for me, I'll be using this one for now.

Thank you for your work!
Cheers!