Re: [PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages() with init_on_alloc=1 or __GFP_ZERO

[David Hildenbrand]

On 11.11.20 10:59, Mike Rapoport wrote:
> On Tue, Nov 10, 2020 at 08:32:40PM +0100, 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 and add support for __GFP_ZERO.
> >
> > 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.
> >
> > 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>
> > ---
> >   mm/page_alloc.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index eed4f4075b3c..0361b119b74e 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -8453,6 +8453,19 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
> >   	return 0;
> >   }
> >   
> > +static void __alloc_contig_clear_range(unsigned long start_pfn,
> > +				       unsigned long end_pfn)
>
> Maybe clear_contig_range() ?

I chose the naming to match "__alloc_contig_migrate_range", but I agree 
that your version sounds better.

> > +{
> > +	unsigned long pfn;
> > +
> > +	for (pfn = start_pfn; pfn < end_pfn; pfn += MAX_ORDER_NR_PAGES) {
> > +		cond_resched();
> > +		kernel_init_free_pages(pfn_to_page(pfn),
> > +				       min_t(unsigned long, end_pfn - pfn,
> > +					     MAX_ORDER_NR_PAGES));
> > +	}
> > +}
> > +
> >   /**
> >    * alloc_contig_range() -- tries to allocate given range of pages
> >    * @start:	start PFN to allocate
> > @@ -8461,7 +8474,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
> >    *			#MIGRATE_MOVABLE or #MIGRATE_CMA).  All pageblocks
> >    *			in range must have the same migratetype and it must
> >    *			be either of the two.
> > - * @gfp_mask:	GFP mask to use during compaction
> > + * @gfp_mask:	GFP mask to use during compaction. __GFP_ZERO clears allocated
> > + *		pages.
>
> "__GFP_ZERO is not passed to compaction but rather clears allocated pages"

Bought! Thanks :)


--
Thanks,

David / dhildenb