| Message ID | 20201103152237.9853-4-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | cleanup page poisoning | expand |
On Tue, Nov 3, 2020 at 4:24 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > Page poisoning used to be incompatible with hibernation, as the state of > poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces > CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes > variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter > restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow > hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit > 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure > free pages are cleared after resume. > > We can use the same mechanism to instead poison free pages with PAGE_POISON > after resume. This covers both zero and 0xAA patterns. Thus we can remove the > Kconfig restriction that disables page poison sanity checking when hibernation > is enabled. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: <linux-pm@vger.kernel.org> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> for the hibernation part and I'm assuming that this patch will be routed through the mm tree. > --- > kernel/power/hibernate.c | 2 +- > kernel/power/power.h | 2 +- > kernel/power/snapshot.c | 14 ++++++++++---- > mm/Kconfig.debug | 1 - > 4 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index 2fc7d509a34f..da0b41914177 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -326,7 +326,7 @@ static int create_image(int platform_mode) > > if (!in_suspend) { > events_check_enabled = false; > - clear_free_pages(); > + clear_or_poison_free_pages(); > } > > platform_leave(platform_mode); > diff --git a/kernel/power/power.h b/kernel/power/power.h > index 24f12d534515..778bf431ec02 100644 > --- a/kernel/power/power.h > +++ b/kernel/power/power.h > @@ -106,7 +106,7 @@ extern int create_basic_memory_bitmaps(void); > extern void free_basic_memory_bitmaps(void); > extern int hibernate_preallocate_memory(void); > > -extern void clear_free_pages(void); > +extern void clear_or_poison_free_pages(void); > > /** > * Auxiliary structure used for reading the snapshot image data and > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 46b1804c1ddf..6b1c84afa891 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -1144,7 +1144,7 @@ void free_basic_memory_bitmaps(void) > pr_debug("Basic memory bitmaps freed\n"); > } > > -void clear_free_pages(void) > +void clear_or_poison_free_pages(void) > { > struct memory_bitmap *bm = free_pages_map; > unsigned long pfn; > @@ -1152,12 +1152,18 @@ void clear_free_pages(void) > if (WARN_ON(!(free_pages_map))) > return; > > - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { > + if (page_poisoning_enabled() || want_init_on_free()) { > memory_bm_position_reset(bm); > pfn = memory_bm_next_pfn(bm); > while (pfn != BM_END_OF_MAP) { > - if (pfn_valid(pfn)) > - clear_highpage(pfn_to_page(pfn)); > + if (pfn_valid(pfn)) { > + struct page *page = pfn_to_page(pfn); > + if (page_poisoning_enabled_static()) > + kernel_poison_pages(page, 1); > + else if (want_init_on_free()) > + clear_highpage(page); > + > + } > > pfn = memory_bm_next_pfn(bm); > } > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug > index 864f129f1937..c57786ad5be9 100644 > --- a/mm/Kconfig.debug > +++ b/mm/Kconfig.debug > @@ -64,7 +64,6 @@ config PAGE_OWNER > > config PAGE_POISONING > bool "Poison pages after freeing" > - select PAGE_POISONING_NO_SANITY if HIBERNATION > help > Fill the pages with poison patterns after free_pages() and verify > the patterns before alloc_pages. The filling of the memory helps > -- > 2.29.1 >
On 03.11.20 16:22, Vlastimil Babka wrote: > Page poisoning used to be incompatible with hibernation, as the state of > poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces > CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes > variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter > restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow > hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit > 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure > free pages are cleared after resume. > > We can use the same mechanism to instead poison free pages with PAGE_POISON > after resume. This covers both zero and 0xAA patterns. Thus we can remove the > Kconfig restriction that disables page poison sanity checking when hibernation > is enabled. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: <linux-pm@vger.kernel.org> > --- > kernel/power/hibernate.c | 2 +- > kernel/power/power.h | 2 +- > kernel/power/snapshot.c | 14 ++++++++++---- > mm/Kconfig.debug | 1 - > 4 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index 2fc7d509a34f..da0b41914177 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -326,7 +326,7 @@ static int create_image(int platform_mode) > > if (!in_suspend) { > events_check_enabled = false; > - clear_free_pages(); > + clear_or_poison_free_pages(); > } > > platform_leave(platform_mode); > diff --git a/kernel/power/power.h b/kernel/power/power.h > index 24f12d534515..778bf431ec02 100644 > --- a/kernel/power/power.h > +++ b/kernel/power/power.h > @@ -106,7 +106,7 @@ extern int create_basic_memory_bitmaps(void); > extern void free_basic_memory_bitmaps(void); > extern int hibernate_preallocate_memory(void); > > -extern void clear_free_pages(void); > +extern void clear_or_poison_free_pages(void); > > /** > * Auxiliary structure used for reading the snapshot image data and > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 46b1804c1ddf..6b1c84afa891 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -1144,7 +1144,7 @@ void free_basic_memory_bitmaps(void) > pr_debug("Basic memory bitmaps freed\n"); > } > > -void clear_free_pages(void) > +void clear_or_poison_free_pages(void) > { > struct memory_bitmap *bm = free_pages_map; > unsigned long pfn; > @@ -1152,12 +1152,18 @@ void clear_free_pages(void) > if (WARN_ON(!(free_pages_map))) > return; > > - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { > + if (page_poisoning_enabled() || want_init_on_free()) { > memory_bm_position_reset(bm); > pfn = memory_bm_next_pfn(bm); > while (pfn != BM_END_OF_MAP) { > - if (pfn_valid(pfn)) > - clear_highpage(pfn_to_page(pfn)); > + if (pfn_valid(pfn)) { > + struct page *page = pfn_to_page(pfn); ^ empty line missing. And at least I prefer to declare all variables in the function header. I'd even suggest to move this into a separate function like clear_or_poison_free_page(struct page *page) > + if (page_poisoning_enabled_static()) > + kernel_poison_pages(page, 1); > + else if (want_init_on_free()) > + clear_highpage(page); > + > + } > > pfn = memory_bm_next_pfn(bm); > } > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug > index 864f129f1937..c57786ad5be9 100644 > --- a/mm/Kconfig.debug > +++ b/mm/Kconfig.debug > @@ -64,7 +64,6 @@ config PAGE_OWNER > > config PAGE_POISONING > bool "Poison pages after freeing" > - select PAGE_POISONING_NO_SANITY if HIBERNATION > help > Fill the pages with poison patterns after free_pages() and verify > the patterns before alloc_pages. The filling of the memory helps > Unless I am missing something important, this should work! Thanks!
On 11/11/20 4:42 PM, David Hildenbrand wrote: ... >> @@ -1152,12 +1152,18 @@ void clear_free_pages(void) >> if (WARN_ON(!(free_pages_map))) >> return; >> >> - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { >> + if (page_poisoning_enabled() || want_init_on_free()) { >> memory_bm_position_reset(bm); >> pfn = memory_bm_next_pfn(bm); >> while (pfn != BM_END_OF_MAP) { >> - if (pfn_valid(pfn)) >> - clear_highpage(pfn_to_page(pfn)); >> + if (pfn_valid(pfn)) { >> + struct page *page = pfn_to_page(pfn); > > ^ empty line missing. And at least I prefer to declare all variables in > the function header. > > I'd even suggest to move this into a separate function like > > clear_or_poison_free_page(struct page *page) > > Ok, fixup below. ----8<---- From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Thu, 12 Nov 2020 15:33:07 +0100 Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity checking-fix Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page() per David Hildenbrand. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- include/linux/mm.h | 1 + kernel/power/snapshot.c | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 861b9392b5dc..d4cfb06a611e 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages) #else static inline bool page_poisoning_enabled(void) { return false; } static inline bool page_poisoning_enabled_static(void) { return false; } +static inline void __kernel_poison_pages(struct page *page, int nunmpages) { } static inline void kernel_poison_pages(struct page *page, int numpages) { } static inline void kernel_unpoison_pages(struct page *page, int numpages) { } #endif diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index 6b1c84afa891..a3491b29c5cc 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void) pr_debug("Basic memory bitmaps freed\n"); } +static void clear_or_poison_free_page(struct page *page) +{ + if (page_poisoning_enabled_static()) + __kernel_poison_pages(page, 1); + else if (want_init_on_free()) + clear_highpage(page); +} + void clear_or_poison_free_pages(void) { struct memory_bitmap *bm = free_pages_map; @@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void) memory_bm_position_reset(bm); pfn = memory_bm_next_pfn(bm); while (pfn != BM_END_OF_MAP) { - if (pfn_valid(pfn)) { - struct page *page = pfn_to_page(pfn); - if (page_poisoning_enabled_static()) - kernel_poison_pages(page, 1); - else if (want_init_on_free()) - clear_highpage(page); - - } + if (pfn_valid(pfn)) + clear_or_poison_free_page(pfn_to_page(pfn)); pfn = memory_bm_next_pfn(bm); }
On Thu, Nov 12, 2020 at 3:42 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 11/11/20 4:42 PM, David Hildenbrand wrote: > ... > >> @@ -1152,12 +1152,18 @@ void clear_free_pages(void) > >> if (WARN_ON(!(free_pages_map))) > >> return; > >> > >> - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { > >> + if (page_poisoning_enabled() || want_init_on_free()) { > >> memory_bm_position_reset(bm); > >> pfn = memory_bm_next_pfn(bm); > >> while (pfn != BM_END_OF_MAP) { > >> - if (pfn_valid(pfn)) > >> - clear_highpage(pfn_to_page(pfn)); > >> + if (pfn_valid(pfn)) { > >> + struct page *page = pfn_to_page(pfn); > > > > ^ empty line missing. And at least I prefer to declare all variables in > > the function header. > > > > I'd even suggest to move this into a separate function like > > > > clear_or_poison_free_page(struct page *page) > > > > > > Ok, fixup below. > > ----8<---- > From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 12 Nov 2020 15:33:07 +0100 > Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity > checking-fix > > Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page() > per David Hildenbrand. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Still Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > include/linux/mm.h | 1 + > kernel/power/snapshot.c | 18 ++++++++++-------- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 861b9392b5dc..d4cfb06a611e 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages) > #else > static inline bool page_poisoning_enabled(void) { return false; } > static inline bool page_poisoning_enabled_static(void) { return false; } > +static inline void __kernel_poison_pages(struct page *page, int nunmpages) { } > static inline void kernel_poison_pages(struct page *page, int numpages) { } > static inline void kernel_unpoison_pages(struct page *page, int numpages) { } > #endif > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 6b1c84afa891..a3491b29c5cc 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void) > pr_debug("Basic memory bitmaps freed\n"); > } > > +static void clear_or_poison_free_page(struct page *page) > +{ > + if (page_poisoning_enabled_static()) > + __kernel_poison_pages(page, 1); > + else if (want_init_on_free()) > + clear_highpage(page); > +} > + > void clear_or_poison_free_pages(void) > { > struct memory_bitmap *bm = free_pages_map; > @@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void) > memory_bm_position_reset(bm); > pfn = memory_bm_next_pfn(bm); > while (pfn != BM_END_OF_MAP) { > - if (pfn_valid(pfn)) { > - struct page *page = pfn_to_page(pfn); > - if (page_poisoning_enabled_static()) > - kernel_poison_pages(page, 1); > - else if (want_init_on_free()) > - clear_highpage(page); > - > - } > + if (pfn_valid(pfn)) > + clear_or_poison_free_page(pfn_to_page(pfn)); > > pfn = memory_bm_next_pfn(bm); > } > -- > 2.29.1 > >
On 12.11.20 15:39, Vlastimil Babka wrote: > On 11/11/20 4:42 PM, David Hildenbrand wrote: > ... >>> @@ -1152,12 +1152,18 @@ void clear_free_pages(void) >>> if (WARN_ON(!(free_pages_map))) >>> return; >>> >>> - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { >>> + if (page_poisoning_enabled() || want_init_on_free()) { >>> memory_bm_position_reset(bm); >>> pfn = memory_bm_next_pfn(bm); >>> while (pfn != BM_END_OF_MAP) { >>> - if (pfn_valid(pfn)) >>> - clear_highpage(pfn_to_page(pfn)); >>> + if (pfn_valid(pfn)) { >>> + struct page *page = pfn_to_page(pfn); >> >> ^ empty line missing. And at least I prefer to declare all variables in >> the function header. >> >> I'd even suggest to move this into a separate function like >> >> clear_or_poison_free_page(struct page *page) >> >> > > Ok, fixup below. > > ----8<---- > From cae1e8ccfa57c28ed1b2f5f8a47319b86cbdcfbf Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 12 Nov 2020 15:33:07 +0100 > Subject: [PATCH] kernel/power: allow hibernation with page_poison sanity > checking-fix > > Adapt to __kernel_unpoison_pages fixup. Split out clear_or_poison_free_page() > per David Hildenbrand. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > include/linux/mm.h | 1 + > kernel/power/snapshot.c | 18 ++++++++++-------- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 861b9392b5dc..d4cfb06a611e 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2896,6 +2896,7 @@ static inline void kernel_unpoison_pages(struct page *page, int numpages) > #else > static inline bool page_poisoning_enabled(void) { return false; } > static inline bool page_poisoning_enabled_static(void) { return false; } > +static inline void __kernel_poison_pages(struct page *page, int nunmpages) { } > static inline void kernel_poison_pages(struct page *page, int numpages) { } > static inline void kernel_unpoison_pages(struct page *page, int numpages) { } > #endif > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 6b1c84afa891..a3491b29c5cc 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -1144,6 +1144,14 @@ void free_basic_memory_bitmaps(void) > pr_debug("Basic memory bitmaps freed\n"); > } > > +static void clear_or_poison_free_page(struct page *page) > +{ > + if (page_poisoning_enabled_static()) > + __kernel_poison_pages(page, 1); > + else if (want_init_on_free()) > + clear_highpage(page); > +} > + > void clear_or_poison_free_pages(void) > { > struct memory_bitmap *bm = free_pages_map; > @@ -1156,14 +1164,8 @@ void clear_or_poison_free_pages(void) > memory_bm_position_reset(bm); > pfn = memory_bm_next_pfn(bm); > while (pfn != BM_END_OF_MAP) { > - if (pfn_valid(pfn)) { > - struct page *page = pfn_to_page(pfn); > - if (page_poisoning_enabled_static()) > - kernel_poison_pages(page, 1); > - else if (want_init_on_free()) > - clear_highpage(page); > - > - } > + if (pfn_valid(pfn)) > + clear_or_poison_free_page(pfn_to_page(pfn)); > > pfn = memory_bm_next_pfn(bm); > } > LGTM, thanks!
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index 2fc7d509a34f..da0b41914177 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -326,7 +326,7 @@ static int create_image(int platform_mode) if (!in_suspend) { events_check_enabled = false; - clear_free_pages(); + clear_or_poison_free_pages(); } platform_leave(platform_mode); diff --git a/kernel/power/power.h b/kernel/power/power.h index 24f12d534515..778bf431ec02 100644 --- a/kernel/power/power.h +++ b/kernel/power/power.h @@ -106,7 +106,7 @@ extern int create_basic_memory_bitmaps(void); extern void free_basic_memory_bitmaps(void); extern int hibernate_preallocate_memory(void); -extern void clear_free_pages(void); +extern void clear_or_poison_free_pages(void); /** * Auxiliary structure used for reading the snapshot image data and diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index 46b1804c1ddf..6b1c84afa891 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -1144,7 +1144,7 @@ void free_basic_memory_bitmaps(void) pr_debug("Basic memory bitmaps freed\n"); } -void clear_free_pages(void) +void clear_or_poison_free_pages(void) { struct memory_bitmap *bm = free_pages_map; unsigned long pfn; @@ -1152,12 +1152,18 @@ void clear_free_pages(void) if (WARN_ON(!(free_pages_map))) return; - if (IS_ENABLED(CONFIG_PAGE_POISONING_ZERO) || want_init_on_free()) { + if (page_poisoning_enabled() || want_init_on_free()) { memory_bm_position_reset(bm); pfn = memory_bm_next_pfn(bm); while (pfn != BM_END_OF_MAP) { - if (pfn_valid(pfn)) - clear_highpage(pfn_to_page(pfn)); + if (pfn_valid(pfn)) { + struct page *page = pfn_to_page(pfn); + if (page_poisoning_enabled_static()) + kernel_poison_pages(page, 1); + else if (want_init_on_free()) + clear_highpage(page); + + } pfn = memory_bm_next_pfn(bm); } diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug index 864f129f1937..c57786ad5be9 100644 --- a/mm/Kconfig.debug +++ b/mm/Kconfig.debug @@ -64,7 +64,6 @@ config PAGE_OWNER config PAGE_POISONING bool "Poison pages after freeing" - select PAGE_POISONING_NO_SANITY if HIBERNATION help Fill the pages with poison patterns after free_pages() and verify the patterns before alloc_pages. The filling of the memory helps
Page poisoning used to be incompatible with hibernation, as the state of poisoned pages was lost after resume, thus enabling CONFIG_HIBERNATION forces CONFIG_PAGE_POISONING_NO_SANITY. For the same reason, the poisoning with zeroes variant CONFIG_PAGE_POISONING_ZERO used to disable hibernation. The latter restriction was removed by commit 1ad1410f632d ("PM / Hibernate: allow hibernation with PAGE_POISONING_ZERO") and similarly for init_on_free by commit 18451f9f9e58 ("PM: hibernate: fix crashes with init_on_free=1") by making sure free pages are cleared after resume. We can use the same mechanism to instead poison free pages with PAGE_POISON after resume. This covers both zero and 0xAA patterns. Thus we can remove the Kconfig restriction that disables page poison sanity checking when hibernation is enabled. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <len.brown@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: <linux-pm@vger.kernel.org> --- kernel/power/hibernate.c | 2 +- kernel/power/power.h | 2 +- kernel/power/snapshot.c | 14 ++++++++++---- mm/Kconfig.debug | 1 - 4 files changed, 12 insertions(+), 7 deletions(-)