| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGsJ_4z96yj-z+d704_FQ==BXXN5wCvQnBtj+Az-nAxTKAbwfw@mail.gmail.com>
Date: Fri, 1 Apr 2022 11:31:02 +1300
From: Barry Song <21cnbao@...il.com>
To: Muchun Song <songmuchun@...edance.com>
Cc: Will Deacon <will@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
"Bodeddula, Balasubramaniam" <bodeddub@...zon.com>,
Oscar Salvador <osalvador@...e.de>,
Mike Kravetz <mike.kravetz@...cle.com>,
David Rientjes <rientjes@...gle.com>,
Mark Rutland <mark.rutland@....com>,
Catalin Marinas <catalin.marinas@....com>, james.morse@....com,
LAK <linux-arm-kernel@...ts.infradead.org>,
LKML <linux-kernel@...r.kernel.org>,
Linux-MM <linux-mm@...ck.org>,
Xiongchun duan <duanxiongchun@...edance.com>,
Fam Zheng <fam.zheng@...edance.com>,
Muchun Song <smuchun@...il.com>
Subject: Re: [PATCH v4 2/2] arm64: mm: hugetlb: Enable HUGETLB_PAGE_FREE_VMEMMAP
for arm64
On Thu, Mar 31, 2022 at 7:57 PM Muchun Song <songmuchun@...edance.com> wrote:
>
> The feature of minimizing overhead of struct page associated with each
> HugeTLB page aims to free its vmemmap pages (used as struct page) to
> save memory, where is ~14GB/16GB per 1TB HugeTLB pages (2MB/1GB type).
> In short, when a HugeTLB page is allocated or freed, the vmemmap array
> representing the range associated with the page will need to be remapped.
> When a page is allocated, vmemmap pages are freed after remapping.
> When a page is freed, previously discarded vmemmap pages must be
> allocated before remapping. More implementations and details can be
> found here [1].
>
> The infrastructure of freeing vmemmap pages associated with each HugeTLB
> page is already there, we can easily enable HUGETLB_PAGE_FREE_VMEMMAP
> for arm64, the only thing to be fixed is flush_dcache_page() .
>
> flush_dcache_page() need to be adapted to operate on the head page's
> flags since the tail vmemmap pages are mapped with read-only after the
> feature is enabled (clear operation is not permitted).
>
> There was some discussions about this in the thread [2], but there was
> no conclusion in the end. And I copied the concern proposed by Anshuman
> to here and explain why those concern is superfluous. It is safe to
> enable it for x86_64 as well as arm64.
>
> 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.
> '''
>
> Answer: At the time memory is removed, all HugeTLB pages either have been
> migrated away or dissolved. So there is no race between memory hot remove
> and free_huge_page_vmemmap(). Therefore, HugeTLB pages inside the hot
> remove section is safe. 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. The minimal granularity size of hotplug memory 128MB (on
> arm64, 4k base page), 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 page could not cross two
> sections. In this case, the section cannot be freed. Any HugeTLB bigger
> than 128MB (section size) whose vmemmap pages is an integer multiple of
> 2MB (PMD-mapped). As 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. In this case, at the time memory
> is removed, all HugeTLB pages either have been migrated away or dissolved.
> So there is also no race between memory hot remove and
> free_huge_page_vmemmap().
>
> 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().
> '''
>
> Answer: The ptdump should be fine since vmemmap_remap_free() only exchanges
> PTEs or splits the PMD entry (which means allocating a PTE page table). Both
> operations do not free any page tables (PTE), so ptdump cannot run into a
> UAF on any page tables. The worst case is just dumping an wrong value.
>
> [1] https://lore.kernel.org/all/20210510030027.56044-1-songmuchun@bytedance.com/
> [2] https://lore.kernel.org/all/20210518091826.36937-1-songmuchun@bytedance.com/
>
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
Reviewed-by: Barry Song <baohua@...nel.org>
I ran some testing on this patchset. On a machine with 4GB memory
and I set 2GB hugepages by "hugepagesz=2m hugepages=1024",
I was seeing:
Before the patch,
/sys# cat kernel/debug/kernel_page_tables
---[ vmemmap start ]---
0xfffffc0000000000-0xfffffc1180000000 70G PUD
0xfffffc1180000000-0xfffffc1181000000 16M PMD
0xfffffc1181000000-0xfffffc1185000000 64M PMD RW NX SHD
AF NG BLK UXN MEM/NORMAL
0xfffffc1185000000-0xfffffc11c0000000 944M PMD
0xfffffc11c0000000-0xfffffc8000000000 441G PUD
0xfffffc8000000000-0xfffffe0000000000 1536G PGD
---[ vmemmap end ]---
After the patch:
---[ vmemmap start ]---
...
0xfffffc27e8090000-0xfffffc27e8091000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e8091000-0xfffffc27e8098000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e8098000-0xfffffc27e8099000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e8099000-0xfffffc27e80a0000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80a0000-0xfffffc27e80a1000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80a1000-0xfffffc27e80a8000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80a8000-0xfffffc27e80a9000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80a9000-0xfffffc27e80b0000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80b0000-0xfffffc27e80b1000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80b1000-0xfffffc27e80b8000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80b8000-0xfffffc27e80b9000 4K PTE RW NX SHD
AF NG UXN MEM/NORMAL
0xfffffc27e80b9000-0xfffffc27e80c0000 28K PTE ro NX SHD
AF NG UXN MEM/NORMAL
...
So it works as expected. we are seeing 7 read-only mapping after 1 RW mapping.
Then I tried to check if the patch would break 64KB hugepages by setting
"hugepagesz=64k hugepages=32768", i got:
---[ vmemmap start ]---
0xfffffc0000000000-0xfffffd8000000000 1536G PGD
0xfffffd8000000000-0xfffffd82c0000000 11G PUD
0xfffffd82c0000000-0xfffffd82c3000000 48M PMD
0xfffffd82c3000000-0xfffffd82c7000000 64M PMD RW NX SHD
AF NG BLK UXN MEM/NORMAL
0xfffffd82c7000000-0xfffffd8300000000 912M PMD
0xfffffd8300000000-0xfffffe0000000000 500G PUD
---[ vmemmap end ]---
Obviously it doesn't break this corner case in which we don't need VMEMMAP_FREE.
> ---
> v4:
> - Introduce ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP (implemented in the previous
> patch) to enable this feature for arm64.
>
> v3:
> - Rework patch's subject.
> - Clarify the feature of HUGETLB_PAGE_FREE_VMEMMAP is already there in the
> current code and easyly be enabled for arm64 into commit log.
> - Add hugetlb_free_vmemmap_enabled() check into flush_dcache_page().
>
> Thanks for Barry's suggestions.
>
> v2:
> - Update commit message (Mark Rutland).
> - Fix flush_dcache_page().
>
> arch/arm64/Kconfig | 1 +
> arch/arm64/mm/flush.c | 13 +++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index c842878f8133..37f72e3a75d0 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -94,6 +94,7 @@ config ARM64
> select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> select ARCH_WANT_FRAME_POINTERS
> select ARCH_WANT_HUGE_PMD_SHARE if ARM64_4K_PAGES || (ARM64_16K_PAGES && !ARM64_VA_BITS_36)
> + select ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP
> select ARCH_WANT_LD_ORPHAN_WARN
> select ARCH_WANTS_NO_INSTR
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> index 2aaf950b906c..c67c1ca856c2 100644
> --- a/arch/arm64/mm/flush.c
> +++ b/arch/arm64/mm/flush.c
> @@ -68,6 +68,19 @@ EXPORT_SYMBOL_GPL(__sync_icache_dcache);
> */
> void flush_dcache_page(struct page *page)
> {
> + /*
> + * Only the head page's flags of HugeTLB can be cleared since the tail
> + * vmemmap pages associated with each HugeTLB page are mapped with
> + * read-only when CONFIG_HUGETLB_PAGE_FREE_VMEMMAP is enabled (more
> + * details can refer to vmemmap_remap_pte()). Although
> + * __sync_icache_dcache() only set PG_dcache_clean flag on the head
> + * page struct, some tail page structs still can be seen the flag is
> + * set since the head vmemmap page frame is reused (more details can
> + * refer to the comments above page_fixed_fake_head()).
> + */
> + if (hugetlb_free_vmemmap_enabled() && PageHuge(page))
> + page = compound_head(page);
> +
> if (test_bit(PG_dcache_clean, &page->flags))
> clear_bit(PG_dcache_clean, &page->flags);
> }
> --
> 2.11.0
>
Thanks
Barry
Powered by blists - more mailing lists