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@xxxxxxxxxxxxx
> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Cc: Alexander Potapenko <glider@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> Cc: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

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