[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 11 Nov 2020 11:06:45 +0100
From: David Hildenbrand <david@...hat.com>
To: Mike Rapoport <rppt@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
Alexander Potapenko <glider@...gle.com>,
Michal Hocko <mhocko@...e.com>,
Mike Kravetz <mike.kravetz@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>,
Oscar Salvador <osalvador@...e.de>,
Kees Cook <keescook@...omium.org>,
Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages()
with init_on_alloc=1 or __GFP_ZERO
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@...ux-foundation.org>
>> Cc: Alexander Potapenko <glider@...gle.com>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Mike Kravetz <mike.kravetz@...cle.com>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Mike Rapoport <rppt@...ux.ibm.com>
>> Cc: Oscar Salvador <osalvador@...e.de>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Michael Ellerman <mpe@...erman.id.au>
>> Signed-off-by: David Hildenbrand <david@...hat.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
Powered by blists - more mailing lists