Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages()
 with init_on_alloc=1 or __GFP_ZERO
To:     Mike Rapoport <rppt@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Alexander Potapenko <glider@google.com>,
        Michal Hocko <mhocko@suse.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Oscar Salvador <osalvador@suse.de>,
        Kees Cook <keescook@chromium.org>,
        Michael Ellerman <mpe@ellerman.id.au>
References: <20201110193240.25401-1-david@redhat.com>
 <20201111095909.GC4755@linux.ibm.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat GmbH
Message-ID: <3e98187a-2a2f-e5fb-c364-d230d3ba132f@redhat.com>
Date:   Wed, 11 Nov 2020 11:06:45 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20201111095909.GC4755@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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