[PATCH v2] mm/page_alloc: clear all pages in post_alloc_hook() with init_on_alloc=1

[PATCH v2] mm/page_alloc: clear all pages in post_alloc_hook() with init_on_alloc=1

[David Hildenbrand]

commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and
init_on_free=1 boot options") resulted with init_on_alloc=1 in all pages
leaving the buddy via alloc_pages() and friends to be
initialized/cleared/zeroed on allocation.

However, the same logic is currently not applied to
alloc_contig_pages(): allocated pages leaving the buddy aren't cleared
with init_on_alloc=1 and init_on_free=0. Let's also properly clear
pages on that allocation path.

To achieve that, let's move clearing into post_alloc_hook(). This will not
only affect alloc_contig_pages() allocations but also any pages used as
migration target in compaction code via compaction_alloc().

While this sounds sub-optimal, it's the very same handling as when
allocating migration targets via alloc_migration_target() - pages will
get properly cleared with init_on_free=1. In case we ever want to optimize
migration in that regard, we should tackle all such migration users - if we
believe migration code can be fully trusted.

With this change, we will see double clearing of pages in some
cases. One example are gigantic pages (either allocated via CMA, or
allocated dynamically via alloc_contig_pages()) - which is the right
thing to do (and to be optimized outside of the buddy in the callers) as
discussed in:
  https://lkml.kernel.org/r/20201019182853.7467-1-gpiccoli@canonical.com

This change implies that with init_on_alloc=1
- All CMA allocations will be cleared
- Gigantic pages allocated via alloc_contig_pages() will be cleared
- virtio-mem memory to be unplugged will be cleared. While this is
  suboptimal, it's similar to memory balloon drivers handling, where
  all pages to be inflated will get cleared as well.
- Pages isolated for compaction will be cleared

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexander Potapenko <glider@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

This is the follow-up of:
  "[PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages() with
  init_on_alloc=1 or __GFP_ZERO"

v1 -> v2:
- Let's clear anything that leaves the buddy, also affecting compaction.
- Don't implement __GFP_ZERO support for now

---
 mm/page_alloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index eaa227a479e4..108b81c0dfa8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2275,6 +2275,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	kasan_alloc_pages(page, order);
 	kernel_poison_pages(page, 1 << order, 1);
 	set_page_owner(page, order, gfp_flags);
+
+	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
+		kernel_init_free_pages(page, 1 << order);
 }
 
 static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
@@ -2282,9 +2285,6 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
 {
 	post_alloc_hook(page, order, gfp_flags);
 
-	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
-		kernel_init_free_pages(page, 1 << order);
-
 	if (order && (gfp_flags & __GFP_COMP))
 		prep_compound_page(page, order);
 
-- 
2.26.2

Re: [PATCH v2] mm/page_alloc: clear all pages in post_alloc_hook() with init_on_alloc=1

[Vlastimil Babka]

On 11/20/20 7:04 PM, David Hildenbrand wrote:
> commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and
> init_on_free=1 boot options") resulted with init_on_alloc=1 in all pages
> leaving the buddy via alloc_pages() and friends to be
> initialized/cleared/zeroed on allocation.
> 
> However, the same logic is currently not applied to
> alloc_contig_pages(): allocated pages leaving the buddy aren't cleared
> with init_on_alloc=1 and init_on_free=0. Let's also properly clear
> pages on that allocation path.
> 
> To achieve that, let's move clearing into post_alloc_hook(). This will not
> only affect alloc_contig_pages() allocations but also any pages used as
> migration target in compaction code via compaction_alloc().
> 
> While this sounds sub-optimal, it's the very same handling as when
> allocating migration targets via alloc_migration_target() - pages will
> get properly cleared with init_on_free=1. In case we ever want to optimize
> migration in that regard, we should tackle all such migration users - if we
> believe migration code can be fully trusted.
> 
> With this change, we will see double clearing of pages in some
> cases. One example are gigantic pages (either allocated via CMA, or
> allocated dynamically via alloc_contig_pages()) - which is the right
> thing to do (and to be optimized outside of the buddy in the callers) as
> discussed in:
>    https://lkml.kernel.org/r/20201019182853.7467-1-gpiccoli@canonical.com
> 
> This change implies that with init_on_alloc=1
> - All CMA allocations will be cleared
> - Gigantic pages allocated via alloc_contig_pages() will be cleared
> - virtio-mem memory to be unplugged will be cleared. While this is
>    suboptimal, it's similar to memory balloon drivers handling, where
>    all pages to be inflated will get cleared as well.
> - Pages isolated for compaction will be cleared
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> 
> This is the follow-up of:
>    "[PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages() with
>    init_on_alloc=1 or __GFP_ZERO"
> 
> v1 -> v2:
> - Let's clear anything that leaves the buddy, also affecting compaction.
> - Don't implement __GFP_ZERO support for now
> 
> ---
>   mm/page_alloc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index eaa227a479e4..108b81c0dfa8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2275,6 +2275,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>   	kasan_alloc_pages(page, order);
>   	kernel_poison_pages(page, 1 << order, 1);
>   	set_page_owner(page, order, gfp_flags);
> +
> +	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
> +		kernel_init_free_pages(page, 1 << order);
>   }
>   
>   static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
> @@ -2282,9 +2285,6 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
>   {
>   	post_alloc_hook(page, order, gfp_flags);
>   
> -	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
> -		kernel_init_free_pages(page, 1 << order);
> -
>   	if (order && (gfp_flags & __GFP_COMP))
>   		prep_compound_page(page, order);
>   
> 

Re: [PATCH v2] mm/page_alloc: clear all pages in post_alloc_hook() with init_on_alloc=1

[Michal Hocko]

On Fri 20-11-20 19:04:52, David Hildenbrand wrote:
> commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and
> init_on_free=1 boot options") resulted with init_on_alloc=1 in all pages
> leaving the buddy via alloc_pages() and friends to be
> initialized/cleared/zeroed on allocation.
> 
> However, the same logic is currently not applied to
> alloc_contig_pages(): allocated pages leaving the buddy aren't cleared
> with init_on_alloc=1 and init_on_free=0. Let's also properly clear
> pages on that allocation path.
> 
> To achieve that, let's move clearing into post_alloc_hook(). This will not
> only affect alloc_contig_pages() allocations but also any pages used as
> migration target in compaction code via compaction_alloc().
> 
> While this sounds sub-optimal, it's the very same handling as when
> allocating migration targets via alloc_migration_target() - pages will
> get properly cleared with init_on_free=1. In case we ever want to optimize
> migration in that regard, we should tackle all such migration users - if we
> believe migration code can be fully trusted.
> 
> With this change, we will see double clearing of pages in some
> cases. One example are gigantic pages (either allocated via CMA, or
> allocated dynamically via alloc_contig_pages()) - which is the right
> thing to do (and to be optimized outside of the buddy in the callers) as
> discussed in:
>   https://lkml.kernel.org/r/20201019182853.7467-1-gpiccoli@canonical.com
> 
> This change implies that with init_on_alloc=1
> - All CMA allocations will be cleared
> - Gigantic pages allocated via alloc_contig_pages() will be cleared
> - virtio-mem memory to be unplugged will be cleared. While this is
>   suboptimal, it's similar to memory balloon drivers handling, where
>   all pages to be inflated will get cleared as well.
> - Pages isolated for compaction will be cleared

Yes, this looks much better than the previous version. Thanks for
looking into it deeper!

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Btw. I think we would benefit from a documentation which would explain
what is the purpose of the two parts of the initialization. What does
belong to prep_new_page resp. post_alloc_hook.

Thanks!
> ---
> 
> This is the follow-up of:
>   "[PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages() with
>   init_on_alloc=1 or __GFP_ZERO"
> 
> v1 -> v2:
> - Let's clear anything that leaves the buddy, also affecting compaction.
> - Don't implement __GFP_ZERO support for now
> 
> ---
>  mm/page_alloc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index eaa227a479e4..108b81c0dfa8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2275,6 +2275,9 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>  	kasan_alloc_pages(page, order);
>  	kernel_poison_pages(page, 1 << order, 1);
>  	set_page_owner(page, order, gfp_flags);
> +
> +	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
> +		kernel_init_free_pages(page, 1 << order);
>  }
>  
>  static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags,
> @@ -2282,9 +2285,6 @@ static void prep_new_page(struct page *page, unsigned int order, gfp_t gfp_flags
>  {
>  	post_alloc_hook(page, order, gfp_flags);
>  
> -	if (!free_pages_prezeroed() && want_init_on_alloc(gfp_flags))
> -		kernel_init_free_pages(page, 1 << order);
> -
>  	if (order && (gfp_flags & __GFP_COMP))
>  		prep_compound_page(page, order);
>  
> -- 
> 2.26.2
> 

-- 
Michal Hocko
SUSE Labs