Re: [PATCH 2/4] arm64/mm: wire up CONFIG_ARCH_HAS_SET_DIRECT_MAP

From: Ard Biesheuvel
Date: Tue May 28 2019 - 05:01:46 EST

On 5/28/19 10:41 AM, Anshuman Khandual wrote:


On 05/28/2019 01:50 PM, Ard Biesheuvel wrote:
On 5/28/19 10:10 AM, Anshuman Khandual wrote:


On 05/23/2019 03:52 PM, Ard Biesheuvel wrote:
Wire up the special helper functions to manipulate aliases of vmalloc
regions in the linear map.

IMHO the commit message here could be bit more descriptive because of the
amount of changes this patch brings in.


Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxx>
---
 arch/arm64/Kconfig | 1 +
 arch/arm64/include/asm/cacheflush.h | 3 ++
 arch/arm64/mm/pageattr.c | 48 ++++++++++++++++----
 mm/vmalloc.c | 11 -----
 4 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index ca9c175fb949..4ab32180eabd 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -26,6 +26,7 @@ config ARM64
ÂÂÂÂÂ select ARCH_HAS_MEMBARRIER_SYNC_CORE
ÂÂÂÂÂ select ARCH_HAS_PTE_SPECIAL
ÂÂÂÂÂ select ARCH_HAS_SETUP_DMA_OPS
+ÂÂÂ select ARCH_HAS_SET_DIRECT_MAP
ÂÂÂÂÂ select ARCH_HAS_SET_MEMORY
ÂÂÂÂÂ select ARCH_HAS_STRICT_KERNEL_RWX
ÂÂÂÂÂ select ARCH_HAS_STRICT_MODULE_RWX
diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index 19844211a4e6..b9ee5510067f 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -187,4 +187,7 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
  int set_memory_valid(unsigned long addr, int numpages, int enable);
 +int set_direct_map_invalid_noflush(struct page *page);
+int set_direct_map_default_noflush(struct page *page);
+
 #endif
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 6cd645edcf35..9c6b9039ec8f 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -159,17 +159,48 @@ int set_memory_valid(unsigned long addr, int numpages, int enable)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __pgprot(PTE_VALID));
 }
 -#ifdef CONFIG_DEBUG_PAGEALLOC
+int set_direct_map_invalid_noflush(struct page *page)
+{
+ÂÂÂ struct page_change_data data = {
+ÂÂÂÂÂÂÂ .set_mask = __pgprot(0),
+ÂÂÂÂÂÂÂ .clear_mask = __pgprot(PTE_VALID),
+ÂÂÂ };
+
+ÂÂÂ if (!rodata_full)
+ÂÂÂÂÂÂÂ return 0;

Why rodata_full needs to be probed here ? Should not we still require the following
transition even if rodata_full is not enabled. Just wondering whether we can use
VM_FLUSH_RESET_PERMS feature without these required transitions.

ÂÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂÂ * Set direct map to something invalid so that it won't be cached if
ÂÂÂÂÂÂÂÂÂ * there are any accesses after the TLB flush, then flush the TLB and
ÂÂÂÂÂÂÂÂÂ * reset the direct map permissions to the default.
ÂÂÂÂÂÂÂÂÂ */
ÂÂÂÂÂÂÂÂ set_area_direct_map(area, set_direct_map_invalid_noflush);
ÂÂÂÂÂÂÂÂ _vm_unmap_aliases(start, end, 1);
ÂÂÂÂÂÂÂÂ set_area_direct_map(area, set_direct_map_default_noflush);

 > +

How would that work? With rodata_full disabled, the linear region is not mapped down to pages, and so there is no way we can manipulate linear aliases at page granularity.

Then should not we check debug_pagealloc_enabled() as well as in case
for __kernel_map_pages() later in the patch.


Yes, what we could do technically is to permit the direct_map_xx() calls to proceed if debug_pagealloc_enabled() returns true, since we are guaranteed to have a page granular mapping of the entire linear region.

However, that means we will be manipulating the linear aliases of vmalloc mappings even if the user has requested us not to, i.e., by passing rodata=on rather than rodata=full, so I don't think we should be doing that.



+ÂÂÂ return apply_to_page_range(&init_mm,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (unsigned long)page_address(page),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PAGE_SIZE, change_page_range, &data);
+}
+
+int set_direct_map_default_noflush(struct page *page)
+{
+ÂÂÂ struct page_change_data data = {
+ÂÂÂÂÂÂÂ .set_mask = __pgprot(PTE_VALID | PTE_WRITE),
+ÂÂÂÂÂÂÂ .clear_mask = __pgprot(PTE_RDONLY),

Replace __pgprot(PTE_VALID | PTE_WRITE) with PAGE_KERNEL instead !


This is a delta mask, so no need to pull in the PTE_MAYBE_NG or other attributes that we know we haven't changed.

+ÂÂÂ };
+
+ÂÂÂ if (!rodata_full)
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ return apply_to_page_range(&init_mm,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (unsigned long)page_address(page),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PAGE_SIZE, change_page_range, &data);
+}
+

IIUC set_direct_map_invalid_noflush() and set_direct_map_default_noflush()
should set *appropriate* permissions as seen fit from platform perspective
to implement this transition.

In here set_direct_map_invalid_noflush() makes the entry invalid preventing
further MMU walks (hence new TLB entries). set_direct_map_default_noflush()
makes it a valid write entry. Though it looks similar to PAGE_KERNEL which
is the default permission for linear mapping on arm64 via __map_memblock().
Should not PAGE_KERNEL be used explicitly as suggested above.


No. We should restore the attributes that we cleared when manipulating the linear aliases. There isn't a lot of code that does that, so I don't see the need to use PAGE_KERNEL here.

Fair enough.


 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
+ÂÂÂ if (!debug_pagealloc_enabled() && !rodata_full)
+ÂÂÂÂÂÂÂ return;
+

I guess this is not related to CONFIG_ARCH_HAS_SET_DIRECT_MAP here and should
be a fix or an enhancement to CONFIG_DEBUG_PAGEALLOC implementation. Just
curious, !rodata_full check here to ensure that linear mapping does not have
block or contig mappings and should be backed by regular pages only ?


It is related. CONFIG_ARCH_HAS_SET_DIRECT_MAP introduces references to __kernel_map_pages() in generic code, so enabling CONFIG_ARCH_HAS_SET_DIRECT_MAP should make __kernel_map_pages() available unconditionally as well.

Ahh, got it.


ÂÂÂÂÂ set_memory_valid((unsigned long)page_address(page), numpages, enable);
 }
-#ifdef CONFIG_HIBERNATION
+
 /*
- * When built with CONFIG_DEBUG_PAGEALLOC and CONFIG_HIBERNATION, this function
- * is used to determine if a linear map page has been marked as not-valid by
- * CONFIG_DEBUG_PAGEALLOC. Walk the page table and check the PTE_VALID bit.
- * This is based on kern_addr_valid(), which almost does what we need.
+ * This function is used to determine if a linear map page has been marked as
+ * not-valid. Walk the page table and check the PTE_VALID bit. This is based
+ * on kern_addr_valid(), which almost does what we need.
ÂÂ *
 * Because this is only called on the kernel linear map, p?d_sect() implies
ÂÂ * p?d_present(). When debug_pagealloc is enabled, sections mappings are
@@ -183,6 +214,9 @@ bool kernel_page_present(struct page *page)
ÂÂÂÂÂ pte_t *ptep;
ÂÂÂÂÂ unsigned long addr = (unsigned long)page_address(page);
 + if (!debug_pagealloc_enabled() && !rodata_full)
+ÂÂÂÂÂÂÂ return true;
+

Ditto, not related to CONFIG_ARCH_HAS_SET_DIRECT_MAP.


It is related.

Never mind.