Message-ID: <Yd7DNesaCACPndv2@FVFF77S0Q05N>
Date:   Wed, 12 Jan 2022 12:01:57 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     will@...nel.org, akpm@...ux-foundation.org, david@...hat.com,
        bodeddub@...zon.com, osalvador@...e.de, mike.kravetz@...cle.com,
        rientjes@...gle.com, catalin.marinas@....com, james.morse@....com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, duanxiongchun@...edance.com,
        fam.zheng@...edance.com
Subject: Re: [PATCH] arm64: mm: hugetlb: add support for free vmemmap pages
 of HugeTLB

Hi,

On Tue, Jan 11, 2022 at 09:16:52PM +0800, Muchun Song wrote:
> The preparation of supporting freeing vmemmap associated with each
> HugeTLB page is ready, so we can support this feature for arm64.
> 
> Signed-off-by: Muchun Song <songmuchun@...edance.com>

It's a bit difficult to understand this commit message, as there's not much
context here.

What is HUGETLB_PAGE_FREE_VMEMMAP intended to achieve? Is this intended to save
memory, find bugs, or some other goal? If this is a memory saving or
performance improvement, can we quantify that benefit?

Does the alloc/free happen dynamically, or does this happen once during kernel
boot? IIUC it's the former, which sounds pretty scary. Especially if we need to
re-allocate the vmmemmap pages later -- can't we run out of memory, and then
fail to free a HugeTLB page?

Are there any requirements upon arch code, e.g. mutual exclusion?

Below there are a bunch of comments trying to explain that this is safe. Having
some of that rationale in the commit message itself would be helpful.

I see that commit:

  6be24bed9da367c2 ("mm: hugetlb: introduce a new config HUGETLB_PAGE_FREE_VMEMMAP")

... has a much more complete description, and cribbing some of that wording
would be helpful.


> ---
> There is already some discussions about this in [1], but there was no
> conclusion in the end. I copied the concern proposed by Anshuman to here.
> 
> 1st concern:
> "
>   But what happens when a hot remove section's vmemmap area (which is being
>   teared down) is nearby another vmemmap area which is either created or
>   being destroyed for HugeTLB alloc/free purpose. As you mentioned HugeTLB
>   pages inside the hot remove section might be safe. But what about other
>   HugeTLB areas whose vmemmap area shares page table entries with vmemmap
>   entries for a section being hot removed ? Massive HugeTLB alloc/use/free
>   test cycle using memory just adjacent to a memory hotplug area, which is
>   always added and removed periodically, should be able to expose this problem.
> "
> My Answer: As you already know HugeTLB pages inside the hot remove section
> is safe.

It would be helpful if you could explain *why* that's safe, since those of us
coming at this cold have no idea whether this is the case.

> Let's talk your question "what about other HugeTLB areas whose
> vmemmap area shares page table entries with vmemmap entries for a section
> being hot removed ?", the question is not established. Why? The minimal
> granularity size of hotplug memory 128MB (on arm64, 4k base page), so any
> HugeTLB smaller than 128MB is within a section, then, there is no share
> (PTE) page tables between HugeTLB in this section and ones in other
> sections and a HugeTLB could not cross two sections.

Am I correct in assuming that in this case we never free the section?

> Any HugeTLB bigger than 128MB (e.g. 1GB) whose size is an integer multible of
> a section and vmemmap area is also an integer multiple of 2MB. At the time
> memory is removed, all huge pages either have been migrated away or
> dissolved. The vmemmap is stable. So there is no problem in this case as
> well.

Are you mention 2MB here because we PMD-map the vmemmap with 4K pages?

IIUC, so long as:

1) HugeTLBs are naturally aligned, power-of-two sizes
2) The HugeTLB size >= the section size
3) The HugeTLB size >= the vmemmap leaf mapping size

... then a HugeTLB will not share any leaf page table entries with *anything
else*, but will share intermediate entries.

Perhaps that's a clearer line of argument?

Regardless, this should be in the commit message.

> 2nd concern:
> "
>   differently, not sure if ptdump would require any synchronization.
> 
>   Dumping an wrong value is probably okay but crashing because a page table
>   entry is being freed after ptdump acquired the pointer is bad. On arm64,
>   ptdump() is protected against hotremove via [get|put]_online_mems().
> "
> My Answer: The ptdump should be fine since vmemmap_remap_free() only exchanges
> PTEs or split the PMD entry (which means allocating a PTE page table). Both
> operations do not free any page tables, so ptdump cannot run into a UAF on
> any page tables. The wrost case is just dumping an wrong value.

This should be in the commit message.

Thanks,
Mark.

> 
> [1] https://lore.kernel.org/linux-mm/b8cdc9c8-853c-8392-a2fa-4f1a8f02057a@arm.com/T/
> 
>  fs/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 7a2b11c0b803..04cfd5bf5ec9 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -247,7 +247,7 @@ config HUGETLB_PAGE
>  
>  config HUGETLB_PAGE_FREE_VMEMMAP
>  	def_bool HUGETLB_PAGE
> -	depends on X86_64
> +	depends on X86_64 || ARM64
>  	depends on SPARSEMEM_VMEMMAP
>  
>  config HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON
> -- 
> 2.11.0
> 

Powered by blists - more mailing lists