* [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc
@ 2021-03-26 11:26 Sergei Trofimovich
2021-03-26 13:48 ` David Hildenbrand
2021-03-26 14:17 ` Vlastimil Babka
0 siblings, 2 replies; 19+ messages in thread
From: Sergei Trofimovich @ 2021-03-26 11:26 UTC (permalink / raw)
To: Andrew Morton, linux-mm; +Cc: linux-kernel, Sergei Trofimovich
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 <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 11:26 [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc Sergei Trofimovich @ 2021-03-26 13:48 ` David Hildenbrand 2021-03-26 15:00 ` Andrey Konovalov 2021-03-29 12:10 ` Vlastimil Babka 2021-03-26 14:17 ` Vlastimil Babka 1 sibling, 2 replies; 19+ messages in thread From: David Hildenbrand @ 2021-03-26 13:48 UTC (permalink / raw) To: Sergei Trofimovich, Andrew Morton, linux-mm Cc: linux-kernel, Vlastimil Babka, Andrey Konovalov 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 <andreyknvl@gmail.com> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 13:48 ` David Hildenbrand @ 2021-03-26 15:00 ` Andrey Konovalov 2021-03-29 12:10 ` Vlastimil Babka 1 sibling, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-26 15:00 UTC (permalink / raw) To: David Hildenbrand, Sergei Trofimovich Cc: Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka On Fri, Mar 26, 2021 at 2:49 PM David Hildenbrand <david@redhat.com> 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc @ 2021-03-26 15:00 ` Andrey Konovalov 0 siblings, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-26 15:00 UTC (permalink / raw) To: David Hildenbrand, Sergei Trofimovich Cc: Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka On Fri, Mar 26, 2021 at 2:49 PM David Hildenbrand <david@redhat.com> 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 15:00 ` Andrey Konovalov (?) @ 2021-03-26 16:42 ` Sergei Trofimovich -1 siblings, 0 replies; 19+ messages in thread From: Sergei Trofimovich @ 2021-03-26 16:42 UTC (permalink / raw) To: Andrey Konovalov Cc: David Hildenbrand, Andrew Morton, Linux Memory Management List, LKML, Vlastimil Babka On Fri, 26 Mar 2021 16:00:34 +0100 Andrey Konovalov <andreyknvl@gmail.com> wrote: > On Fri, Mar 26, 2021 at 2:49 PM David Hildenbrand <david@redhat.com> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 13:48 ` David Hildenbrand 2021-03-26 15:00 ` Andrey Konovalov @ 2021-03-29 12:10 ` Vlastimil Babka 2021-03-29 22:00 ` Andrey Konovalov 1 sibling, 1 reply; 19+ messages in thread From: Vlastimil Babka @ 2021-03-29 12:10 UTC (permalink / raw) To: David Hildenbrand, Sergei Trofimovich, Andrew Morton, linux-mm Cc: linux-kernel, Andrey Konovalov 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 <andreyknvl@gmail.com> > 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: ? > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-29 12:10 ` Vlastimil Babka @ 2021-03-29 22:00 ` Andrey Konovalov 0 siblings, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-29 22:00 UTC (permalink / raw) To: Vlastimil Babka Cc: David Hildenbrand, Sergei Trofimovich, Andrew Morton, Linux Memory Management List, LKML On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82 > > Author: Andrey Konovalov <andreyknvl@gmail.com> > > 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc @ 2021-03-29 22:00 ` Andrey Konovalov 0 siblings, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-29 22:00 UTC (permalink / raw) To: Vlastimil Babka Cc: David Hildenbrand, Sergei Trofimovich, Andrew Morton, Linux Memory Management List, LKML On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82 > > Author: Andrey Konovalov <andreyknvl@gmail.com> > > 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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-29 22:00 ` Andrey Konovalov (?) @ 2021-03-29 22:07 ` Vlastimil Babka 2021-03-30 14:48 ` Andrey Konovalov -1 siblings, 1 reply; 19+ messages in thread From: Vlastimil Babka @ 2021-03-29 22:07 UTC (permalink / raw) To: Andrey Konovalov Cc: David Hildenbrand, Sergei Trofimovich, Andrew Morton, Linux Memory Management List, LKML On 3/30/21 12:00 AM, Andrey Konovalov wrote: > On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82 >> > Author: Andrey Konovalov <andreyknvl@gmail.com> >> > 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-29 22:07 ` Vlastimil Babka @ 2021-03-30 14:48 ` Andrey Konovalov 0 siblings, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-30 14:48 UTC (permalink / raw) To: Vlastimil Babka Cc: David Hildenbrand, Sergei Trofimovich, Andrew Morton, Linux Memory Management List, LKML On Tue, Mar 30, 2021 at 12:07 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/30/21 12:00 AM, Andrey Konovalov wrote: > > On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82 > >> > Author: Andrey Konovalov <andreyknvl@gmail.com> > >> > 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! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc @ 2021-03-30 14:48 ` Andrey Konovalov 0 siblings, 0 replies; 19+ messages in thread From: Andrey Konovalov @ 2021-03-30 14:48 UTC (permalink / raw) To: Vlastimil Babka Cc: David Hildenbrand, Sergei Trofimovich, Andrew Morton, Linux Memory Management List, LKML On Tue, Mar 30, 2021 at 12:07 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/30/21 12:00 AM, Andrey Konovalov wrote: > > On Mon, Mar 29, 2021 at 2:10 PM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> > commit 855a9c4018f3219db8be7e4b9a65ab22aebfde82 > >> > Author: Andrey Konovalov <andreyknvl@gmail.com> > >> > 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! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 11:26 [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc Sergei Trofimovich 2021-03-26 13:48 ` David Hildenbrand @ 2021-03-26 14:17 ` Vlastimil Babka 2021-03-26 17:25 ` Sergei Trofimovich 1 sibling, 1 reply; 19+ messages in thread From: Vlastimil Babka @ 2021-03-26 14:17 UTC (permalink / raw) To: Sergei Trofimovich, Andrew Morton, linux-mm Cc: linux-kernel, David Hildenbrand 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 <akpm@linux-foundation.org> > CC: linux-mm@kvack.org > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > 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); > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 14:17 ` Vlastimil Babka @ 2021-03-26 17:25 ` Sergei Trofimovich 2021-03-27 18:03 ` Sergei Trofimovich 0 siblings, 1 reply; 19+ messages in thread From: Sergei Trofimovich @ 2021-03-26 17:25 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, linux-mm, linux-kernel, David Hildenbrand, Andrey Konovalov On Fri, 26 Mar 2021 15:17:00 +0100 Vlastimil Babka <vbabka@suse.cz> 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 <akpm@linux-foundation.org> > > CC: linux-mm@kvack.org > > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > > --- > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc 2021-03-26 17:25 ` Sergei Trofimovich @ 2021-03-27 18:03 ` Sergei Trofimovich 2021-03-27 18:21 ` [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 Sergei Trofimovich 0 siblings, 1 reply; 19+ messages in thread From: Sergei Trofimovich @ 2021-03-27 18:03 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, linux-mm, linux-kernel, David Hildenbrand, Andrey Konovalov On Fri, 26 Mar 2021 17:25:22 +0000 Sergei Trofimovich <slyfox@gentoo.org> wrote: > On Fri, 26 Mar 2021 15:17:00 +0100 > Vlastimil Babka <vbabka@suse.cz> 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 <akpm@linux-foundation.org> > > > CC: linux-mm@kvack.org > > > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 2021-03-27 18:03 ` Sergei Trofimovich @ 2021-03-27 18:21 ` Sergei Trofimovich 2021-03-29 9:23 ` David Hildenbrand 2021-03-29 11:59 ` Vlastimil Babka 0 siblings, 2 replies; 19+ messages in thread From: Sergei Trofimovich @ 2021-03-27 18:21 UTC (permalink / raw) To: Vlastimil Babka, Andrew Morton, David Hildenbrand, Andrey Konovalov Cc: linux-kernel, Sergei Trofimovich, linux-mm 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 <vbabka@suse.cz> CC: Andrew Morton <akpm@linux-foundation.org> CC: linux-mm@kvack.org CC: David Hildenbrand <david@redhat.com> CC: Andrey Konovalov <andreyknvl@gmail.com> Link: https://lkml.org/lkml/2021/3/26/443 Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 2021-03-27 18:21 ` [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 Sergei Trofimovich @ 2021-03-29 9:23 ` David Hildenbrand 2021-03-29 11:59 ` Vlastimil Babka 1 sibling, 0 replies; 19+ messages in thread From: David Hildenbrand @ 2021-03-29 9:23 UTC (permalink / raw) To: Sergei Trofimovich, Vlastimil Babka, Andrew Morton, Andrey Konovalov Cc: linux-kernel, linux-mm 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 <vbabka@suse.cz> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: linux-mm@kvack.org > CC: David Hildenbrand <david@redhat.com> > CC: Andrey Konovalov <andreyknvl@gmail.com> > Link: https://lkml.org/lkml/2021/3/26/443 Again, Fixes: tag? IOW, which commit initially broke it. > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 2021-03-27 18:21 ` [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 Sergei Trofimovich 2021-03-29 9:23 ` David Hildenbrand @ 2021-03-29 11:59 ` Vlastimil Babka 2021-03-29 22:25 ` [PATCH v3] " Sergei Trofimovich 1 sibling, 1 reply; 19+ messages in thread From: Vlastimil Babka @ 2021-03-29 11:59 UTC (permalink / raw) To: Sergei Trofimovich, Andrew Morton, David Hildenbrand, Andrey Konovalov Cc: linux-kernel, linux-mm 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 <vbabka@suse.cz> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: linux-mm@kvack.org > CC: David Hildenbrand <david@redhat.com> > CC: Andrey Konovalov <andreyknvl@gmail.com> > Link: https://lkml.org/lkml/2021/3/26/443 > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> 8db26a3d4735 ("mm, page_poison: use static key more efficiently") Cc: <stable@vger.kernel.org> > --- > 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; > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 2021-03-29 11:59 ` Vlastimil Babka @ 2021-03-29 22:25 ` Sergei Trofimovich 2021-03-30 8:39 ` David Hildenbrand 0 siblings, 1 reply; 19+ messages in thread From: Sergei Trofimovich @ 2021-03-29 22:25 UTC (permalink / raw) To: Vlastimil Babka, Andrew Morton, David Hildenbrand, Andrey Konovalov Cc: linux-kernel, Sergei Trofimovich, stable, linux-mm 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 <vbabka@suse.cz> Fixes: 8db26a3d4735 ("mm, page_poison: use static key more efficiently") Cc: <stable@vger.kernel.org> CC: Andrew Morton <akpm@linux-foundation.org> CC: linux-mm@kvack.org CC: David Hildenbrand <david@redhat.com> CC: Andrey Konovalov <andreyknvl@gmail.com> Link: https://lkml.org/lkml/2021/3/26/443 Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 2021-03-29 22:25 ` [PATCH v3] " Sergei Trofimovich @ 2021-03-30 8:39 ` David Hildenbrand 0 siblings, 0 replies; 19+ messages in thread From: David Hildenbrand @ 2021-03-30 8:39 UTC (permalink / raw) To: Sergei Trofimovich, Vlastimil Babka, Andrew Morton, Andrey Konovalov Cc: linux-kernel, stable, linux-mm 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 <vbabka@suse.cz> > Fixes: 8db26a3d4735 ("mm, page_poison: use static key more efficiently") > Cc: <stable@vger.kernel.org> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: linux-mm@kvack.org > CC: David Hildenbrand <david@redhat.com> > CC: Andrey Konovalov <andreyknvl@gmail.com> > Link: https://lkml.org/lkml/2021/3/26/443 > > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > 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 <david@redhat.com> -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-03-30 14:48 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-26 11:26 [PATCH] mm: page_alloc: ignore init_on_free=1 for page alloc Sergei Trofimovich 2021-03-26 13:48 ` David Hildenbrand 2021-03-26 15:00 ` Andrey Konovalov 2021-03-26 15:00 ` Andrey Konovalov 2021-03-26 16:42 ` Sergei Trofimovich 2021-03-29 12:10 ` Vlastimil Babka 2021-03-29 22:00 ` Andrey Konovalov 2021-03-29 22:00 ` Andrey Konovalov 2021-03-29 22:07 ` Vlastimil Babka 2021-03-30 14:48 ` Andrey Konovalov 2021-03-30 14:48 ` Andrey Konovalov 2021-03-26 14:17 ` Vlastimil Babka 2021-03-26 17:25 ` Sergei Trofimovich 2021-03-27 18:03 ` Sergei Trofimovich 2021-03-27 18:21 ` [PATCH v2] mm: page_alloc: ignore init_on_free=1 for debug_pagealloc=1 Sergei Trofimovich 2021-03-29 9:23 ` David Hildenbrand 2021-03-29 11:59 ` Vlastimil Babka 2021-03-29 22:25 ` [PATCH v3] " Sergei Trofimovich 2021-03-30 8:39 ` David Hildenbrand
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.