* [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction [not found] <20210712005732.4f9bfa78@zn3> @ 2021-07-12 21:58 ` Sergei Trofimovich 2021-07-14 2:00 ` Andrew Morton 2021-07-14 3:10 ` Kees Cook 0 siblings, 2 replies; 6+ messages in thread From: Sergei Trofimovich @ 2021-07-12 21:58 UTC (permalink / raw) To: linux-mm Cc: linux-kernel, Sergei Trofimovich, Andrew Morton, Kees Cook, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka, bowsingbetee To reproduce the failure we need the following system: - kernel command: page_poison=1 init_on_free=0 init_on_alloc=0 - kernel config: * CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y * CONFIG_INIT_ON_FREE_DEFAULT_ON=y * CONFIG_PAGE_POISONING=y 0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1 Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021 Call Trace: dump_stack+0x64/0x7c __kernel_unpoison_pages.cold+0x48/0x84 post_alloc_hook+0x60/0xa0 get_page_from_freelist+0xdb8/0x1000 __alloc_pages+0x163/0x2b0 __get_free_pages+0xc/0x30 pgd_alloc+0x2e/0x1a0 ? dup_mm+0x37/0x4f0 mm_init+0x185/0x270 dup_mm+0x6b/0x4f0 ? __lock_task_sighand+0x35/0x70 copy_process+0x190d/0x1b10 kernel_clone+0xba/0x3b0 __do_sys_clone+0x8f/0xb0 do_syscall_64+0x68/0x80 ? do_syscall_64+0x11/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae Before the 51cba1eb ("init_on_alloc: Optimize static branches") init_on_alloc never enabled static branch by default. It could only be enabed explicitly by init_mem_debugging_and_hardening(). But after the 51cba1eb static branch could already be enabled by default. There was no code to ever disable it. That caused page_poison=1 / init_on_free=1 conflict. This change extends init_mem_debugging_and_hardening() to also disable static branch disabling. CC: Andrew Morton <akpm@linux-foundation.org> CC: Kees Cook <keescook@chromium.org> CC: Alexander Potapenko <glider@google.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Vlastimil Babka <vbabka@suse.cz> CC: linux-mm@kvack.org Reported-by: bowsingbetee@pm.me Reported-by: Mikhail Morfikov Fixes: 51cba1eb ("init_on_alloc: Optimize static branches") Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- mm/page_alloc.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 3b97e17806be..46cb4a9c2b50 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void) } #endif - if (_init_on_alloc_enabled_early) { - if (page_poisoning_requested) + if (_init_on_alloc_enabled_early || + IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) { + 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_disable(&init_on_alloc); + } else static_branch_enable(&init_on_alloc); } - if (_init_on_free_enabled_early) { - if (page_poisoning_requested) + if (_init_on_free_enabled_early || + IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) { + 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_disable(&init_on_free); + } else static_branch_enable(&init_on_free); } -- 2.32.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction 2021-07-12 21:58 ` [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction Sergei Trofimovich @ 2021-07-14 2:00 ` Andrew Morton 2021-07-17 18:18 ` Sergei Trofimovich 2021-07-14 3:10 ` Kees Cook 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2021-07-14 2:00 UTC (permalink / raw) To: Sergei Trofimovich Cc: linux-mm, linux-kernel, Kees Cook, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka, bowsingbetee On Mon, 12 Jul 2021 22:58:16 +0100 Sergei Trofimovich <slyfox@gentoo.org> wrote: > To reproduce the failure we need the following system: > - kernel command: page_poison=1 init_on_free=0 init_on_alloc=0 > - kernel config: > * CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y > * CONFIG_INIT_ON_FREE_DEFAULT_ON=y > * CONFIG_PAGE_POISONING=y > > 0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1 > Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021 > Call Trace: > dump_stack+0x64/0x7c > __kernel_unpoison_pages.cold+0x48/0x84 > post_alloc_hook+0x60/0xa0 > get_page_from_freelist+0xdb8/0x1000 > __alloc_pages+0x163/0x2b0 > __get_free_pages+0xc/0x30 > pgd_alloc+0x2e/0x1a0 > ? dup_mm+0x37/0x4f0 > mm_init+0x185/0x270 > dup_mm+0x6b/0x4f0 > ? __lock_task_sighand+0x35/0x70 > copy_process+0x190d/0x1b10 > kernel_clone+0xba/0x3b0 > __do_sys_clone+0x8f/0xb0 > do_syscall_64+0x68/0x80 > ? do_syscall_64+0x11/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > Before the 51cba1eb ("init_on_alloc: Optimize static branches") > init_on_alloc never enabled static branch by default. It could > only be enabed explicitly by init_mem_debugging_and_hardening(). > > But after the 51cba1eb static branch could already be enabled > by default. There was no code to ever disable it. That caused > page_poison=1 / init_on_free=1 conflict. > > This change extends init_mem_debugging_and_hardening() to also > disable static branch disabling. > > ... > > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void) > } > #endif > > - if (_init_on_alloc_enabled_early) { > - if (page_poisoning_requested) > + if (_init_on_alloc_enabled_early || > + IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) { > + 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_disable(&init_on_alloc); > + } else > static_branch_enable(&init_on_alloc); > } > - if (_init_on_free_enabled_early) { > - if (page_poisoning_requested) > + if (_init_on_free_enabled_early || > + IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) { > + 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_disable(&init_on_free); > + } else > static_branch_enable(&init_on_free); > } > I'm thinking this is sufficiently serious and sufficiently reported to warrant a cc:stable backport. Agree? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction 2021-07-14 2:00 ` Andrew Morton @ 2021-07-17 18:18 ` Sergei Trofimovich 0 siblings, 0 replies; 6+ messages in thread From: Sergei Trofimovich @ 2021-07-17 18:18 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, linux-kernel, Kees Cook, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka, bowsingbetee On Tue, 13 Jul 2021 19:00:51 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 12 Jul 2021 22:58:16 +0100 Sergei Trofimovich <slyfox@gentoo.org> wrote: > > > To reproduce the failure we need the following system: > > - kernel command: page_poison=1 init_on_free=0 init_on_alloc=0 > > - kernel config: > > * CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y > > * CONFIG_INIT_ON_FREE_DEFAULT_ON=y > > * CONFIG_PAGE_POISONING=y > > > > 0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > 0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > 00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1 > > Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021 > > Call Trace: > > dump_stack+0x64/0x7c > > __kernel_unpoison_pages.cold+0x48/0x84 > > post_alloc_hook+0x60/0xa0 > > get_page_from_freelist+0xdb8/0x1000 > > __alloc_pages+0x163/0x2b0 > > __get_free_pages+0xc/0x30 > > pgd_alloc+0x2e/0x1a0 > > ? dup_mm+0x37/0x4f0 > > mm_init+0x185/0x270 > > dup_mm+0x6b/0x4f0 > > ? __lock_task_sighand+0x35/0x70 > > copy_process+0x190d/0x1b10 > > kernel_clone+0xba/0x3b0 > > __do_sys_clone+0x8f/0xb0 > > do_syscall_64+0x68/0x80 > > ? do_syscall_64+0x11/0x80 > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > Before the 51cba1eb ("init_on_alloc: Optimize static branches") > > init_on_alloc never enabled static branch by default. It could > > only be enabed explicitly by init_mem_debugging_and_hardening(). > > > > But after the 51cba1eb static branch could already be enabled > > by default. There was no code to ever disable it. That caused > > page_poison=1 / init_on_free=1 conflict. > > > > This change extends init_mem_debugging_and_hardening() to also > > disable static branch disabling. > > > > ... > > > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void) > > } > > #endif > > > > - if (_init_on_alloc_enabled_early) { > > - if (page_poisoning_requested) > > + if (_init_on_alloc_enabled_early || > > + IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) { > > + 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_disable(&init_on_alloc); > > + } else > > static_branch_enable(&init_on_alloc); > > } > > - if (_init_on_free_enabled_early) { > > - if (page_poisoning_requested) > > + if (_init_on_free_enabled_early || > > + IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) { > > + 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_disable(&init_on_free); > > + } else > > static_branch_enable(&init_on_free); > > } > > > > I'm thinking this is sufficiently serious and sufficiently reported to > warrant a cc:stable backport. Agree? I agree. The patch might be tricky to apply as is too far back. But current release should be fine. -- Sergei ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction 2021-07-12 21:58 ` [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction Sergei Trofimovich 2021-07-14 2:00 ` Andrew Morton @ 2021-07-14 3:10 ` Kees Cook 2021-07-14 11:11 ` bowsingbetee 1 sibling, 1 reply; 6+ messages in thread From: Kees Cook @ 2021-07-14 3:10 UTC (permalink / raw) To: Sergei Trofimovich Cc: linux-mm, linux-kernel, Andrew Morton, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka, bowsingbetee On Mon, Jul 12, 2021 at 10:58:16PM +0100, Sergei Trofimovich wrote: > To reproduce the failure we need the following system: > - kernel command: page_poison=1 init_on_free=0 init_on_alloc=0 > - kernel config: > * CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y > * CONFIG_INIT_ON_FREE_DEFAULT_ON=y > * CONFIG_PAGE_POISONING=y > > 0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1 > Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021 > Call Trace: > dump_stack+0x64/0x7c > __kernel_unpoison_pages.cold+0x48/0x84 > post_alloc_hook+0x60/0xa0 > get_page_from_freelist+0xdb8/0x1000 > __alloc_pages+0x163/0x2b0 > __get_free_pages+0xc/0x30 > pgd_alloc+0x2e/0x1a0 > ? dup_mm+0x37/0x4f0 > mm_init+0x185/0x270 > dup_mm+0x6b/0x4f0 > ? __lock_task_sighand+0x35/0x70 > copy_process+0x190d/0x1b10 > kernel_clone+0xba/0x3b0 > __do_sys_clone+0x8f/0xb0 > do_syscall_64+0x68/0x80 > ? do_syscall_64+0x11/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > Before the 51cba1eb ("init_on_alloc: Optimize static branches") > init_on_alloc never enabled static branch by default. It could > only be enabed explicitly by init_mem_debugging_and_hardening(). But init_mem_debugging_and_hardening() is always called (by mm_init()). > But after the 51cba1eb static branch could already be enabled > by default. There was no code to ever disable it. That caused > page_poison=1 / init_on_free=1 conflict. > > This change extends init_mem_debugging_and_hardening() to also > disable static branch disabling. > > CC: Andrew Morton <akpm@linux-foundation.org> > CC: Kees Cook <keescook@chromium.org> > CC: Alexander Potapenko <glider@google.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Vlastimil Babka <vbabka@suse.cz> > CC: linux-mm@kvack.org > Reported-by: bowsingbetee@pm.me > Reported-by: Mikhail Morfikov > Fixes: 51cba1eb ("init_on_alloc: Optimize static branches") > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > mm/page_alloc.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 3b97e17806be..46cb4a9c2b50 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void) > } > #endif > > - if (_init_on_alloc_enabled_early) { > - if (page_poisoning_requested) > + if (_init_on_alloc_enabled_early || > + IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) { 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. > + 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_disable(&init_on_alloc); > + } else > static_branch_enable(&init_on_alloc); > } > - if (_init_on_free_enabled_early) { > - if (page_poisoning_requested) > + if (_init_on_free_enabled_early || > + IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) { > + 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_disable(&init_on_free); > + } else > static_branch_enable(&init_on_free); > } I think it would be better to clean this up without additional confusion involving the CONFIGs: diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 3b97e17806be..1f19365bc158 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -840,21 +840,24 @@ void init_mem_debugging_and_hardening(void) } #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"); + _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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction 2021-07-14 3:10 ` Kees Cook @ 2021-07-14 11:11 ` bowsingbetee 2021-07-14 21:14 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: bowsingbetee @ 2021-07-14 11:11 UTC (permalink / raw) To: Kees Cook Cc: Sergei Trofimovich, linux-mm, linux-kernel, Andrew Morton, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka [-- Attachment #1.1: Type: text/plain, Size: 8604 bytes --] ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Wednesday, July 14th, 2021 at 5:10 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Jul 12, 2021 at 10:58:16PM +0100, Sergei Trofimovich wrote: > > > To reproduce the failure we need the following system: > > > > - kernel command: page_poison=1 init_on_free=0 init_on_alloc=0 > > > > - kernel config: > > > > - CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y > > - CONFIG_INIT_ON_FREE_DEFAULT_ON=y > > - CONFIG_PAGE_POISONING=y > > > > 0000000085629bdd: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > > 0000000022861832: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > > 00000000c597f5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > > CPU: 11 PID: 15195 Comm: bash Kdump: loaded Tainted: G U O 5.13.1-gentoo-x86_64 #1 > > > > Hardware name: System manufacturer System Product Name/PRIME Z370-A, BIOS 2801 01/13/2021 > > > > Call Trace: > > > > dump_stack+0x64/0x7c > > > > __kernel_unpoison_pages.cold+0x48/0x84 > > > > post_alloc_hook+0x60/0xa0 > > > > get_page_from_freelist+0xdb8/0x1000 > > > > __alloc_pages+0x163/0x2b0 > > > > __get_free_pages+0xc/0x30 > > > > pgd_alloc+0x2e/0x1a0 > > > > ? dup_mm+0x37/0x4f0 > > > > mm_init+0x185/0x270 > > > > dup_mm+0x6b/0x4f0 > > > > ? __lock_task_sighand+0x35/0x70 > > > > copy_process+0x190d/0x1b10 > > > > kernel_clone+0xba/0x3b0 > > > > __do_sys_clone+0x8f/0xb0 > > > > do_syscall_64+0x68/0x80 > > > > ? do_syscall_64+0x11/0x80 > > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > > > Before the 51cba1eb ("init_on_alloc: Optimize static branches") > > > > init_on_alloc never enabled static branch by default. It could > > > > only be enabed explicitly by init_mem_debugging_and_hardening(). > > But init_mem_debugging_and_hardening() is always called (by mm_init()). > > > But after the 51cba1eb static branch could already be enabled > > > > by default. There was no code to ever disable it. That caused > > > > page_poison=1 / init_on_free=1 conflict. > > > > This change extends init_mem_debugging_and_hardening() to also > > > > disable static branch disabling. > > > > CC: Andrew Morton akpm@linux-foundation.org > > > > CC: Kees Cook keescook@chromium.org > > > > CC: Alexander Potapenko glider@google.com > > > > CC: Thomas Gleixner tglx@linutronix.de > > > > CC: Vlastimil Babka vbabka@suse.cz > > > > CC: linux-mm@kvack.org > > > > Reported-by: bowsingbetee@pm.me > > > > Reported-by: Mikhail Morfikov > > > > Fixes: 51cba1eb ("init_on_alloc: Optimize static branches") > > > > Signed-off-by: Sergei Trofimovich slyfox@gentoo.org > > ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > > > mm/page_alloc.c | 16 ++++++++++------ > > > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > > index 3b97e17806be..46cb4a9c2b50 100644 > > > > --- a/mm/page_alloc.c > > > > +++ b/mm/page_alloc.c > > > > @@ -840,18 +840,22 @@ void init_mem_debugging_and_hardening(void) > > > > } > > > > #endif > > > > - if (_init_on_alloc_enabled_early) { > > - if (page_poisoning_requested) > > > > > > > > - if (_init_on_alloc_enabled_early || > > - IS_ENABLED(CONFIG_INIT_ON_ALLOC_DEFAULT_ON)) { > > > > > > 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. > > > - 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_disable(&init_on_alloc); > > > > > > - } else > > static_branch_enable(&init_on_alloc); > > > > > > } > > > > - if (_init_on_free_enabled_early) { > > - if (page_poisoning_requested) > > > > > > > > - if (_init_on_free_enabled_early || > > - IS_ENABLED(CONFIG_INIT_ON_FREE_DEFAULT_ON)) { > > > > > > - 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_disable(&init_on_free); > > > > > > - } else > > static_branch_enable(&init_on_free); > > > > > > } > > I think it would be better to clean this up without additional > > confusion involving the CONFIGs: > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 3b97e17806be..1f19365bc158 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -840,21 +840,24 @@ void init_mem_debugging_and_hardening(void) > > } > > #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! [-- Attachment #1.2: publickey - bowsingbetee@protonmail.com - 0x65EDD6AE.asc --] [-- Type: application/pgp-keys, Size: 1873 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 509 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction 2021-07-14 11:11 ` bowsingbetee @ 2021-07-14 21:14 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2021-07-14 21:14 UTC (permalink / raw) To: bowsingbetee Cc: Sergei Trofimovich, linux-mm, linux-kernel, Andrew Morton, Alexander Potapenko, Thomas Gleixner, Vlastimil Babka On Wed, Jul 14, 2021 at 11:11:18AM +0000, bowsingbetee wrote: > 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) > > [...] > > While both patches(Sergei's, and yours) work for me, I'll be using this one for now. > > Thank you for your work! > Cheers! Awesome; thanks for testing it! -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-17 18:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210712005732.4f9bfa78@zn3>
2021-07-12 21:58 ` [PATCH] mm: page_alloc: fix page_poison=1 / INIT_ON_ALLOC_DEFAULT_ON interaction Sergei Trofimovich
2021-07-14 2:00 ` Andrew Morton
2021-07-17 18:18 ` Sergei Trofimovich
2021-07-14 3:10 ` Kees Cook
2021-07-14 11:11 ` bowsingbetee
2021-07-14 21:14 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).