Re: [PATCH] x86/mm/init: respect memblock reserved regions when destroying mappings

[Yinghai Lu <yinghai@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4929 bytes --]

On Fri, Feb 4, 2011 at 5:18 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> On 02/04/2011 03:35 AM, Stefano Stabellini wrote:
>> On Thu, 3 Feb 2011, H. Peter Anvin wrote:
>>> On 02/03/2011 03:25 AM, Stefano Stabellini wrote:
>>>>> How on Earth would you end up with a reserved region *inside the BRK*?
>>>> I think in practice you cannot, but you can have reserved regions at
>>>> _end, that is the main problem I am trying to solve.
>>>> If we have a reserved region at _end and _end is not PMD aligned, then
>>>> we have a problem.
>>>>
>>>> I thought that checking for reserved regions before destroying the
>>>> mapping would be a decent solution (because it wouldn't affect the
>>>> normal case); so I ended up checking between _brk_end and _end too.
>>>>
>>>> Other alternative solutions I thought about but that I discarded because
>>>> they also affect the normal case are:
>>>>
>>>> - never destroy mappings that could go over _end;
>>>> - always PMD align _end.
>>>>
>>>> If none of the above are acceptable, I welcome other suggestions :-)
>>>>
>>> Sounds like the code does the right thing, but the description needs to
>>> be improved.
>>>
>> I tried to improve both the commit message and the comments within the
>> code, this is the result:
>>
>>
>> commit d0136be7b48953f27202dbde285a7379d06cfe98
>> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Date:   Tue Jan 25 12:05:11 2011 +0000
>>
>>     x86/mm/init: respect memblock reserved regions when destroying mappings
>>
>>     In init_memory_mapping we destroy the mappings between _brk_end and
>>     _end, but if _end is not PMD aligned we also destroy mappings for
>>     potentially reserved regions between _end and the following PMD.
>>
>>     In order to avoid this problem, before clearing any PMDs we check if the
>>     corresponding memory area has been reserved and we only destroy the
>>     mapping if hasn't.
>>
>>     We found this issue because under Xen we have a valid mapping at _end,
>>     and if _end is not PMD aligned the current code destroys the initial
>>     part of it.
>
> This description makes more sense, even if the code does exactly the
> same thing.
>
>>
>>     In practice this fix does not have any impact on native.
>>
>>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
>> index 947f42a..65c34f4 100644
>> --- a/arch/x86/mm/init.c
>> +++ b/arch/x86/mm/init.c
>> @@ -283,6 +283,8 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
>>       if (!after_bootmem && !start) {
>>               pud_t *pud;
>>               pmd_t *pmd;
>> +             unsigned long addr;
>> +             u64 size, memblock_addr;
>>
>>               mmu_cr4_features = read_cr4();
>>
>> @@ -291,11 +293,22 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
>>                * located on different 2M pages. cleanup_highmap(), however,
>>                * can only consider _end when it runs, so destroy any
>>                * mappings beyond _brk_end here.
>> +              *
>> +              * If _end is not PMD aligned, we also destroy the mapping of
>> +              * the memory area between _end the next PMD, so before clearing
>> +              * the PMD we make sure that the corresponding memory region has
>> +              * not been reserved.
>>                */
>>               pud = pud_offset(pgd_offset_k(_brk_end), _brk_end);
>>               pmd = pmd_offset(pud, _brk_end - 1);
>> -             while (++pmd <= pmd_offset(pud, (unsigned long)_end - 1))
>> -                     pmd_clear(pmd);
>> +             addr = (_brk_end + PMD_SIZE - 1) & PMD_MASK;
>
> I guess its OK if this is >_end, because the pmd offset will be greater
> than _end's.  But is there an edge condition if the pmd_offset goes off
> the end of the pud, and pud page itself happens to be at the end of the
> address space and it wraps?
>
>> +             while (++pmd <= pmd_offset(pud, (unsigned long)_end - 1)) {
> Could _end be in a different pud from _brk_end?  Could this walk off the
> pud page?
>
> Or is it moot, and there's a guarantee that the whole space is mapped
> out of the same pud page?  I guess the original code has the same
> concern, so this patch leaves the status quo unchanged.
>
>    J
>
>
>> +                     memblock_addr = memblock_x86_find_in_range_size(__pa(addr),
>> +                                     &size, PMD_SIZE);
>> +                     if (memblock_addr == (u64) __pa(addr) && size >= PMD_SIZE)
>> +                             pmd_clear(pmd);
>> +                     addr += PMD_SIZE;
>> +             }
>>       }
>>  #endif
>>       __flush_tlb_all();

why not just move calling cleanup_highmap down?

something like attached patch.

[-- Attachment #2: fix_cleanup_highmap.patch --]
[-- Type: text/x-patch, Size: 3660 bytes --]

---
 arch/x86/include/asm/pgtable_64.h |    2 +-
 arch/x86/kernel/head64.c          |    3 ---
 arch/x86/kernel/setup.c           |    6 ++++++
 arch/x86/mm/init.c                |   19 -------------------
 arch/x86/mm/init_64.c             |    5 +++--
 5 files changed, 10 insertions(+), 25 deletions(-)

Index: linux-2.6/arch/x86/include/asm/pgtable_64.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/pgtable_64.h
+++ linux-2.6/arch/x86/include/asm/pgtable_64.h
@@ -165,7 +165,7 @@ static inline int pgd_large(pgd_t pgd) {
 #define __swp_entry_to_pte(x)		((pte_t) { .pte = (x).val })
 
 extern int kern_addr_valid(unsigned long addr);
-extern void cleanup_highmap(void);
+extern void cleanup_highmap(unsigned long end);
 
 #define HAVE_ARCH_UNMAPPED_AREA
 #define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
Index: linux-2.6/arch/x86/kernel/head64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/head64.c
+++ linux-2.6/arch/x86/kernel/head64.c
@@ -77,9 +77,6 @@ void __init x86_64_start_kernel(char * r
 	/* Make NULL pointers segfault */
 	zap_identity_mappings();
 
-	/* Cleanup the over mapped high alias */
-	cleanup_highmap();
-
 	max_pfn_mapped = KERNEL_IMAGE_SIZE >> PAGE_SHIFT;
 
 	for (i = 0; i < NUM_EXCEPTION_VECTORS; i++) {
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -297,6 +297,9 @@ static void __init init_gbpages(void)
 static inline void init_gbpages(void)
 {
 }
+static void __init cleanup_highmap(unsigned long end)
+{
+}
 #endif
 
 static void __init reserve_brk(void)
@@ -922,6 +925,9 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	reserve_brk();
 
+	/* Cleanup the over mapped high alias after _brk_end*/
+	cleanup_highmap(_brk_end);
+
 	memblock.current_limit = get_max_mapped();
 	memblock_x86_fill();
 
Index: linux-2.6/arch/x86/mm/init.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/init.c
+++ linux-2.6/arch/x86/mm/init.c
@@ -279,25 +279,6 @@ unsigned long __init_refok init_memory_m
 	load_cr3(swapper_pg_dir);
 #endif
 
-#ifdef CONFIG_X86_64
-	if (!after_bootmem && !start) {
-		pud_t *pud;
-		pmd_t *pmd;
-
-		mmu_cr4_features = read_cr4();
-
-		/*
-		 * _brk_end cannot change anymore, but it and _end may be
-		 * located on different 2M pages. cleanup_highmap(), however,
-		 * can only consider _end when it runs, so destroy any
-		 * mappings beyond _brk_end here.
-		 */
-		pud = pud_offset(pgd_offset_k(_brk_end), _brk_end);
-		pmd = pmd_offset(pud, _brk_end - 1);
-		while (++pmd <= pmd_offset(pud, (unsigned long)_end - 1))
-			pmd_clear(pmd);
-	}
-#endif
 	__flush_tlb_all();
 
 	if (!after_bootmem && e820_table_end > e820_table_start)
Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/init_64.c
+++ linux-2.6/arch/x86/mm/init_64.c
@@ -297,13 +297,14 @@ void __init init_extra_mapping_uc(unsign
  * rounded up to the 2MB boundary. This catches the invalid pmds as
  * well, as they are located before _text:
  */
-void __init cleanup_highmap(void)
+void __init cleanup_highmap(unsigned long end)
 {
 	unsigned long vaddr = __START_KERNEL_map;
-	unsigned long end = roundup((unsigned long)_end, PMD_SIZE) - 1;
 	pmd_t *pmd = level2_kernel_pgt;
 	pmd_t *last_pmd = pmd + PTRS_PER_PMD;
 
+	end = roundup(end, PMD_SIZE) - 1;
+
 	for (; pmd < last_pmd; pmd++, vaddr += PMD_SIZE) {
 		if (pmd_none(*pmd))
 			continue;