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

[David Hildenbrand <david@redhat.com>]

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@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.
>>
>> 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>
>> ---
>>   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