x86/ioremap: Use is_vmalloc_addr in iounmap

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

Signed-off-by Max Ramanouski <[email protected]>

---
 arch/x86/mm/ioremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/mm/ioremap.c +1 -1

index aa7d27932..0b596a1d2 100644
@@ -457,7 +457,7 @@ void iounmap(volatile void __iomem *addr)
 {
 	struct vm_struct *p, *o;
 
-	if ((void __force *)addr <= high_memory)
+	if (!is_vmalloc_addr((void __force *)addr))
 		return;
 
 	/*

Quoted 6 lines · Max Ramanouski

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

This can also manifest in more subtle ways because the conflict does not always result in an error. Sometimes for example it can cause ioremap_uc() to actually return WC mappings if for example the early iounmap exit leaves a WC mapping behind. This is because is_new_memtype_allowed() returns true in most cases.

Quoted 3 lines

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

This seems correct to me, but I'm not an expert in the x86 memory map. However it does fix the issue we observed that lead us to finding this fix, and it doesn't seem to introduce any other obvious problems so please add:

Tested-by Alistair Popple <[email protected]>

Quoted 13 lines

Signed-off-by Max Ramanouski <[email protected]>

---
  arch/x86/mm/ioremap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/mm/ioremap.c +0 -0

index aa7d27932..0b596a1d2 100644
@@ -457,7 +457,7 @@ void iounmap(volatile void __iomem *addr)
  {
  	struct vm_struct *p, *o;
 

arch/x86/mm/ioremap.c void iounmap(volatile void __iomem *addr) · line 463

Quoted 5 lines

arch/x86/mm/ioremap.c +1 -1

-	if ((void __force *)addr <= high_memory)
+	if (!is_vmalloc_addr((void __force *)addr))
  		return;
 
  	/*

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

Quoted 10 lines · Max Ramanouski

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

So this lacks a Fixes tag and some deep analysis of similar potential problems. While at it please use func() notation for functions. In the middle of a sentence iounmap does not immediately stand out, but iounmap() does. It's documented ...

This add_pages() hackery in pagemap_range() is really nasty as it ends up violating historical assumptions about max_pfn and high_memory.

Dan, who did the analysis of this when the device private memory muck was added?

Quoted 18 lines · Thomas Gleixner

Quoted 10 lines · Max Ramanouski

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

So this lacks a Fixes tag and some deep analysis of similar potential problems. While at it please use func() notation for functions. In the middle of a sentence iounmap does not immediately stand out, but iounmap() does. It's documented ...

Quoted 5 lines

This add_pages() hackery in pagemap_range() is really nasty as it ends up violating historical assumptions about max_pfn and high_memory.

Dan, who did the analysis of this when the device private memory muck was added?

So that plain add_pages() usage originated here:

4ef589dc9b10 mm/hmm/devmem: device memory hotplug using ZONE_DEVICE

The original memremap_pages() only ever used arch_add_memory()

    41e94a851304 add devm_memremap_pages

When HMM merged into memremap_pages() I indeed did not pick up on the nuance that HMM was wrong to use add_pages() instead of arch_add_memory() which updates the high_memory variable:

69324b8f4833 mm, devm_memremap_pages: add MEMORY_DEVICE_PRIVATE support

Quoted 33 lines · Dan Williams

Quoted 18 lines · Thomas Gleixner

Quoted 10 lines · Max Ramanouski

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

So this lacks a Fixes tag and some deep analysis of similar potential problems. While at it please use func() notation for functions. In the middle of a sentence iounmap does not immediately stand out, but iounmap() does. It's documented ...

Quoted 5 lines

This add_pages() hackery in pagemap_range() is really nasty as it ends up violating historical assumptions about max_pfn and high_memory.

Dan, who did the analysis of this when the device private memory muck was added?

So that plain add_pages() usage originated here:

4ef589dc9b10 mm/hmm/devmem: device memory hotplug using ZONE_DEVICE

The original memremap_pages() only ever used arch_add_memory()

    41e94a851304 add devm_memremap_pages

Quoted 5 lines

When HMM merged into memremap_pages() I indeed did not pick up on the nuance that HMM was wrong to use add_pages() instead of arch_add_memory() which updates the high_memory variable:

69324b8f4833 mm, devm_memremap_pages: add MEMORY_DEVICE_PRIVATE support

arch_add_memory() calls add_pages() ...

Quoted 34 lines · Thomas Gleixner

Quoted 33 lines · Dan Williams

Quoted 18 lines · Thomas Gleixner

Quoted 10 lines · Max Ramanouski

On systems that use HMM (most notably amdgpu driver) high_memory can jump over VMALLOC_START. That causes some iounmap to exit early. This in addition to leaking, causes problems with rebinding devices to vfio_pci from other drivers with error of conflicting memtypes, as they aren't freed in iounmap.

Replace comparison against high_memory with is_vmalloc_addr to fix the issue and make x86 iounmap implementation more similar to generic one, it also uses is_vmalloc_addr to validate pointer.

So this lacks a Fixes tag and some deep analysis of similar potential problems. While at it please use func() notation for functions. In the middle of a sentence iounmap does not immediately stand out, but iounmap() does. It's documented ...

Quoted 5 lines

This add_pages() hackery in pagemap_range() is really nasty as it ends up violating historical assumptions about max_pfn and high_memory.

Dan, who did the analysis of this when the device private memory muck was added?

So that plain add_pages() usage originated here:

4ef589dc9b10 mm/hmm/devmem: device memory hotplug using ZONE_DEVICE

The original memremap_pages() only ever used arch_add_memory()

    41e94a851304 add devm_memremap_pages

Quoted 7 lines

Quoted 5 lines

When HMM merged into memremap_pages() I indeed did not pick up on the nuance that HMM was wrong to use add_pages() instead of arch_add_memory() which updates the high_memory variable:

69324b8f4833 mm, devm_memremap_pages: add MEMORY_DEVICE_PRIVATE support

arch_add_memory() calls add_pages() ...

Apologies was trying to quickly reverse engineer how private memory might be different than typical memremap_pages(), but it is indeed the same in this aspect.

So the real difference is that the private memory case tries to allocate physical memory by searching for holes in the iomem_resource starting from U64_MAX. That might explain why only the private memory case is violating assumptions with respect to high_memory spilling into vmalloc space.

Quoted 4 lines · Dan Williams

Apologies was trying to quickly reverse engineer how private memory might be different than typical memremap_pages(), but it is indeed the same in this aspect.

Quoted 5 lines

So the real difference is that the private memory case tries to allocate physical memory by searching for holes in the iomem_resource starting from U64_MAX. That might explain why only the private memory case is violating assumptions with respect to high_memory spilling into vmalloc space.

Not U64_MAX, but it starts searching for free physical address space starting at MAX_PHYSMEM_BITS, see gfr_start().

That aspect of private memory has always bothered me, but I am not sure that is the historical assumption violation you are referring to above.

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Apologies was trying to quickly reverse engineer how private memory might be different than typical memremap_pages(), but it is indeed the same in this aspect.

Quoted 8 lines

Quoted 5 lines

So the real difference is that the private memory case tries to allocate physical memory by searching for holes in the iomem_resource starting from U64_MAX. That might explain why only the private memory case is violating assumptions with respect to high_memory spilling into vmalloc space.

Not U64_MAX, but it starts searching for free physical address space starting at MAX_PHYSMEM_BITS, see gfr_start().

Wait. MAX_PHYSMEM_BITS is either 46 (4-level) or 52 (5-level), which is fully covered by the identity map space.

So even if the search starts from top of that space, how do we end up with high_memory > VMALLOC_START?

That does not make any sense at all

[ add Alistair and John ]

Quoted 18 lines · Thomas Gleixner

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Apologies was trying to quickly reverse engineer how private memory might be different than typical memremap_pages(), but it is indeed the same in this aspect.

Quoted 8 lines

Quoted 5 lines

So the real difference is that the private memory case tries to allocate physical memory by searching for holes in the iomem_resource starting from U64_MAX. That might explain why only the private memory case is violating assumptions with respect to high_memory spilling into vmalloc space.

Not U64_MAX, but it starts searching for free physical address space starting at MAX_PHYSMEM_BITS, see gfr_start().

Wait. MAX_PHYSMEM_BITS is either 46 (4-level) or 52 (5-level), which is fully covered by the identity map space.

Quoted 4 lines

So even if the search starts from top of that space, how do we end up with high_memory > VMALLOC_START?

That does not make any sense at all

Max, or Alistair can you provide more details of how private memory spills over into the VMALLOC space on these platforms?

I too would have thought that MAX_PHYSMEM_BITS protects against this?

Quoted 21 lines · Dan Williams

[ add Alistair and John ]

Quoted 18 lines · Thomas Gleixner

Quoted 5 lines · Dan Williams

Quoted 4 lines · Dan Williams

Apologies was trying to quickly reverse engineer how private memory might be different than typical memremap_pages(), but it is indeed the same in this aspect.

Quoted 8 lines

Quoted 5 lines

So the real difference is that the private memory case tries to allocate physical memory by searching for holes in the iomem_resource starting from U64_MAX. That might explain why only the private memory case is violating assumptions with respect to high_memory spilling into vmalloc space.

Not U64_MAX, but it starts searching for free physical address space starting at MAX_PHYSMEM_BITS, see gfr_start().

Wait. MAX_PHYSMEM_BITS is either 46 (4-level) or 52 (5-level), which is fully covered by the identity map space.

Quoted 7 lines

Quoted 4 lines

So even if the search starts from top of that space, how do we end up with high_memory > VMALLOC_START?

That does not make any sense at all

Max, or Alistair can you provide more details of how private memory spills over into the VMALLOC space on these platforms?

Well I was hoping pleading ignorance on x86 memory maps would get me out of having to look too deeply :-) But alas...

It appears the problem originates in KASLR which can cause the VMALLOC region to overlap with the top of the linear map.

Quoted 1 line

I too would have thought that MAX_PHYSMEM_BITS protects against this?

Me too, until about an hour ago. As noted above request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 down. Therefore VMALLOC_START needs to be greater than PAGE_OFFSET + (1 << MAX_PHYSMEM_BITS) - 1. However the default configuration for KASLR as set by RANDOMIZE_MEMORY_PHYSICAL_PADDING is to only provide 10TB above what max_pfn is set to at boot time (and even then only if CONFIG_MEMORY_HOTPLUG is enabled).

Obviously ZONE_DEVICE memory ends up being way above that and crosses into the VMALLOC region. So I think the actual fix is something like:

---

arch/x86/Kconfig +0 -0

index e36261b4ea14..c58d7b0f5bca 100644

Quoted 21 lines · Alistair Popple

Quoted 11 lines · Dan Williams

Quoted 7 lines · Thomas Gleixner

Wait. MAX_PHYSMEM_BITS is either 46 (4-level) or 52 (5-level), which is fully covered by the identity map space.

So even if the search starts from top of that space, how do we end up with high_memory > VMALLOC_START?

That does not make any sense at all

Max, or Alistair can you provide more details of how private memory spills over into the VMALLOC space on these platforms?

Well I was hoping pleading ignorance on x86 memory maps would get me out of having to look too deeply :-) But alas...

It appears the problem originates in KASLR which can cause the VMALLOC region to overlap with the top of the linear map.

Quoted 1 line

I too would have thought that MAX_PHYSMEM_BITS protects against this?

Quoted 7 lines

Me too, until about an hour ago. As noted above request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 down. Therefore VMALLOC_START needs to be greater than PAGE_OFFSET + (1 << MAX_PHYSMEM_BITS) - 1. However the default configuration for KASLR as set by RANDOMIZE_MEMORY_PHYSICAL_PADDING is to only provide 10TB above what max_pfn is set to at boot time (and even then only if CONFIG_MEMORY_HOTPLUG is enabled).

Quoted 2 lines

Obviously ZONE_DEVICE memory ends up being way above that and crosses into the VMALLOC region.

Quoted 8 lines

	@@ -2277,6 +2277,7 @@ config RANDOMIZE_MEMORY_PHYSICAL_PADDING
		depends on RANDOMIZE_MEMORY
		default "0xa" if MEMORY_HOTPLUG
		default "0x0"
	+       range 0x40 0x40 if GET_FREE_REGION
		range 0x1 0x40 if MEMORY_HOTPLUG
		range 0x0 0x40
		help

	 @@ -1824,10 +1824,11 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
					  resource_size_t align, unsigned long flags)
	  {
		 if (flags & GFR_DESCENDING) {

	 +               u64 kaslr_pad = CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING << 40;
			 resource_size_t end;

			 end = min_t(resource_size_t, base->end,
	 -                           (1ULL << MAX_PHYSMEM_BITS) - 1);
	 +                           (1ULL << MAX_PHYSMEM_BITS) - kaslr_pad - 1);
			 return end - size + 1;
		 }

Quoted 22 lines · Dan Williams

Quoted 21 lines · Alistair Popple

Quoted 11 lines · Dan Williams

Quoted 7 lines · Thomas Gleixner

Wait. MAX_PHYSMEM_BITS is either 46 (4-level) or 52 (5-level), which is fully covered by the identity map space.

So even if the search starts from top of that space, how do we end up with high_memory > VMALLOC_START?

That does not make any sense at all

Max, or Alistair can you provide more details of how private memory spills over into the VMALLOC space on these platforms?

Well I was hoping pleading ignorance on x86 memory maps would get me out of having to look too deeply :-) But alas...

It appears the problem originates in KASLR which can cause the VMALLOC region to overlap with the top of the linear map.

Quoted 1 line

I too would have thought that MAX_PHYSMEM_BITS protects against this?

Quoted 7 lines

Quoted 7 lines

Me too, until about an hour ago. As noted above request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 down. Therefore VMALLOC_START needs to be greater than PAGE_OFFSET + (1 << MAX_PHYSMEM_BITS) - 1. However the default configuration for KASLR as set by RANDOMIZE_MEMORY_PHYSICAL_PADDING is to only provide 10TB above what max_pfn is set to at boot time (and even then only if CONFIG_MEMORY_HOTPLUG is enabled).

Duh.

Quoted 2 lines

Quoted 2 lines

Obviously ZONE_DEVICE memory ends up being way above that and crosses into the VMALLOC region.

So we need to go through all usage sites of MAX_PHYSMEM_BITS and fix them up...

Quoted 8 lines

Quoted 8 lines

	@@ -2277,6 +2277,7 @@ config RANDOMIZE_MEMORY_PHYSICAL_PADDING
		depends on RANDOMIZE_MEMORY
		default "0xa" if MEMORY_HOTPLUG
		default "0x0"
	+       range 0x40 0x40 if GET_FREE_REGION
		range 0x1 0x40 if MEMORY_HOTPLUG
		range 0x0 0x40
		help

That reduces the entropy to the minimum and papers over the problem with 4-level paging, but it won't solve the problem on systems with 5-level paging.

There the 64T of padding are just not cutting it. MAX_PHYSMEM_BITS is 52 for 5 level ....

Quoted 4 lines

	 @@ -1824,10 +1824,11 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
					  resource_size_t align, unsigned long flags)
	  {
		 if (flags & GFR_DESCENDING) {

Quoted 8 lines

	 +               u64 kaslr_pad = CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING << 40;
			 resource_size_t end;

			 end = min_t(resource_size_t, base->end,
	 -                           (1ULL << MAX_PHYSMEM_BITS) - 1);
	 +                           (1ULL << MAX_PHYSMEM_BITS) - kaslr_pad - 1);
			 return end - size + 1;
		 }

This needs a fixup of gfr_continue() too, but it does not work at all. The size of the direct map is calculated as:

memory_tb = DIV_ROUND_UP(max_pfn << PAGE_SHIFT, 1UL << TB_SHIFT) +
CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;

That size is used to calculate the address above which the vmalloc area is placed.

So assume 6T RAM installed + 10T space for hotplug memory. That means the vmalloc area can start right above 16T. But 64T - 10T = 54T which is slightly larger than 16T :)

I came up with the uncompiled below. I fixed up the obvious places, but did not go through all usage sites of MAX_PHYSMEM_BITS yet.

arch/x86/include/asm/pgtable_64_types.h +1 -0

+
 #define MODULES_VADDR		(__START_KERNEL_map + KERNEL_IMAGE_SIZE)
 /* The module sections ends with the start of the fixmap */
 #ifndef CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP

include/linux/mm.h +4 -0

@@ -97,6 +97,10 @@ extern const int mmap_rnd_compat_bits_ma
 extern int mmap_rnd_compat_bits __read_mostly;
 #endif
 
+#ifndef DIRECT_MAP_END
+# define DIRECT_MAP_END	((1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT)) - 1)
+#endif
+
 #include <asm/page.h>
 #include <asm/processor.h>
 

kernel/resource.c +1 -2

 		return end - size + 1;
 	}
 

@@ -1844,8 +1843,7 @@ static bool gfr_continue(struct resource
 	 * @size did not wrap 0.
 	 */
 	return addr > addr - size &&
-	       addr <= min_t(resource_size_t, base->end,
-			     (1ULL << MAX_PHYSMEM_BITS) - 1);
+	       addr <= min_t(resource_size_t, base->end, DIRECT_MAP_END);
 }
 
 static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,

mm/memory_hotplug.c +1 -1

@@ -1681,7 +1681,7 @@ struct range __weak arch_get_mappable_ra
 
 struct range mhp_get_pluggable_range(bool need_mapping)
 {
-	const u64 max_phys = (1ULL << MAX_PHYSMEM_BITS) - 1;
+	const u64 max_phys = DIRECT_MAP_END;
 	struct range mhp_range;
 
 	if (need_mapping) {

mm/sparse.c +1 -1

@@ -129,7 +129,7 @@ static inline int sparse_early_nid(struc
 static void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
 						unsigned long *end_pfn)
 {
-	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+	unsigned long max_sparsemem_pfn = (DIRECT_MAP_END + 1) >> PAGE_SHIFT;
 
 	/*
 	 * Sanity checks - do not allow an architecture to pass

arch/x86/include/asm/pgtable_64_types.h extern unsigned int ptrs_per_p4d; · line 190

Quoted 8 lines · Thomas Gleixner

---

arch/x86/include/asm/pgtable_64_types.h +1 -0

@@ -187,6 +187,8 @@ extern unsigned int ptrs_per_p4d;
 #define KMSAN_MODULES_ORIGIN_START	(KMSAN_MODULES_SHADOW_START + MODULES_LEN)
 #endif /* CONFIG_KMSAN */
 
+#define DIRECT_MAP_END		(VMALLOC_START - 1)

If I'm understanding the KASLR implementation correctly then this doesn't seem quite right - the entropy means there is a hole from the end of the direct map (ie. the range PAGE_OFFSET to PAGE_OFFSET+kaslr_regions[0].size_tb) and VMALLOC_START which shouldn't be used.

In practice hotplugging memory into that range probably works, but it seems like it would set us up for future bugs. It also means memory hotplug could fail intermittently based on the per-boot entropy.

For example with CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING set to 10T one could imagine hotplugging 16T would mostly work except on some system boots if KASLR happens to randomly place VMALLOC_START close to the end of the direct map.

Therefore to keep memory hotplug deterministic I think it would be better to define DIRECT_MAP_END as a variable that KASLR sets/updates.

This does not look right to me - isn't DIRECT_MAP_END a virtual address where as the resource ranges are physical addresses? Ie. I think this should be:

(quoted diff) +1 -0

+		end = min_t(resource_size_t, base->end, __pa(DIRECT_MAP_END));

The same applies to the rest of the DIRECT_MAP_END users here. Perhaps it would be better to define this as DIRECT_MAP_SIZE and calculate this based off PAGE_OFFSET instead?

Quoted 20 lines · Alistair Popple

arch/x86/include/asm/pgtable_64_types.h extern unsigned int ptrs_per_p4d; · line 190

Quoted 8 lines · Thomas Gleixner

---

arch/x86/include/asm/pgtable_64_types.h +1 -0

@@ -187,6 +187,8 @@ extern unsigned int ptrs_per_p4d;
 #define KMSAN_MODULES_ORIGIN_START	(KMSAN_MODULES_SHADOW_START + MODULES_LEN)
 #endif /* CONFIG_KMSAN */
 
+#define DIRECT_MAP_END		(VMALLOC_START - 1)

If I'm understanding the KASLR implementation correctly then this doesn't seem quite right - the entropy means there is a hole from the end of the direct map (ie. the range PAGE_OFFSET to PAGE_OFFSET+kaslr_regions[0].size_tb) and VMALLOC_START which shouldn't be used.

In practice hotplugging memory into that range probably works, but it seems like it would set us up for future bugs. It also means memory hotplug could fail intermittently based on the per-boot entropy.

Quoted 7 lines

For example with CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING set to 10T one could imagine hotplugging 16T would mostly work except on some system boots if KASLR happens to randomly place VMALLOC_START close to the end of the direct map.

Therefore to keep memory hotplug deterministic I think it would be better to define DIRECT_MAP_END as a variable that KASLR sets/updates.

Right. I forgot about the hole.

Quoted 4 lines

This does not look right to me - isn't DIRECT_MAP_END a virtual address where as the resource ranges are physical addresses? Ie. I think this should be:

Quoted 5 lines

(quoted diff) +1 -0

+		end = min_t(resource_size_t, base->end, __pa(DIRECT_MAP_END));

The same applies to the rest of the DIRECT_MAP_END users here. Perhaps it would be better to define this as DIRECT_MAP_SIZE and calculate this based off PAGE_OFFSET instead?

Duh, yes. I shouldn't try to write patches at 30C :)

So this lacks a Fixes tag and some deep analysis of similar potential problems. While at it please use func() notation for functions. In the middle of a sentence iounmap does not immediately stand out, but iounmap() does. It's documented ...

The correct helper to use is is_ioremap_addr, as in the generic iounmap implementation. This won't matter for x86 currently, but it clearly documents what the code is doing.

Quoted 3 lines · Alistair Popple

The same applies to the rest of the DIRECT_MAP_END users here. Perhaps it would be better to define this as DIRECT_MAP_SIZE and calculate this based off PAGE_OFFSET instead?

Duh, yes. I shouldn't try to write patches at 30C :)

Quoted 6 lines · Thomas Gleixner

Quoted 3 lines · Alistair Popple

The same applies to the rest of the DIRECT_MAP_END users here. Perhaps it would be better to define this as DIRECT_MAP_SIZE and calculate this based off PAGE_OFFSET instead?

Duh, yes. I shouldn't try to write patches at 30C :)

We can avoid the calculation and expose the end of the physical address space for memory. This time I actually built and ran it :)

include/linux/mm.h +2 -0

+#endif
+
 #include <asm/page.h>
 #include <asm/processor.h>
 

kernel/resource.c +2 -4

@@ -1826,8 +1826,7 @@ static resource_size_t gfr_start(struct
 	if (flags & GFR_DESCENDING) {
 		resource_size_t end;
 
-		end = min_t(resource_size_t, base->end,
-			    (1ULL << MAX_PHYSMEM_BITS) - 1);
+		end = min_t(resource_size_t, base->end, PHYSMEM_END);
 		return end - size + 1;
 	}
 

@@ -1844,8 +1843,7 @@ static bool gfr_continue(struct resource
 	 * @size did not wrap 0.
 	 */
 	return addr > addr - size &&
-	       addr <= min_t(resource_size_t, base->end,
-			     (1ULL << MAX_PHYSMEM_BITS) - 1);
+	       addr <= min_t(resource_size_t, base->end, PHYSMEM_END);
 }
 
 static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,

mm/memory_hotplug.c +1 -1

@@ -1681,7 +1681,7 @@ struct range __weak arch_get_mappable_ra
 
 struct range mhp_get_pluggable_range(bool need_mapping)
 {
-	const u64 max_phys = (1ULL << MAX_PHYSMEM_BITS) - 1;
+	const u64 max_phys = PHYSMEM_END;
 	struct range mhp_range;
 
 	if (need_mapping) {

mm/sparse.c +1 -1

@@ -129,7 +129,7 @@ static inline int sparse_early_nid(struc
 static void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
 						unsigned long *end_pfn)
 {
-	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+	unsigned long max_sparsemem_pfn = (PHYSMEM_END + 1) >> PAGE_SHIFT;
 
 	/*
 	 * Sanity checks - do not allow an architecture to pass

include/linux/mm.h extern const int mmap_rnd_compat_bits_ma · line 101

Quoted 4 lines · Thomas Gleixner

include/linux/mm.h +2 -0

+#ifndef PHYSMEM_END
+# define PHYSMEM_END	(1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT))

However I think this should be:

# define PHYSMEM_END	((1UL << MAX_PHYSMEM_BITS) - 1)

Quoted 8 lines · Alistair Popple

include/linux/mm.h extern const int mmap_rnd_compat_bits_ma · line 101

Quoted 4 lines · Thomas Gleixner

include/linux/mm.h +2 -0

+#ifndef PHYSMEM_END
+# define PHYSMEM_END	(1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT))

However I think this should be:

# define PHYSMEM_END	((1UL << MAX_PHYSMEM_BITS) - 1)

Bah.

iounmap() on x86 occasionally fails to unmap because the provided valid ioremap address is not below high_memory. It turned out that this happens due to KASLR.

KASLR uses the full address space between PAGE_OFFSET and vaddr_end to randomize the starting points of the direct map, vmalloc and vmemmap regions. It thereby limits the size of the direct map by using the installed memory size plus an extra configurable margin for hot-plug memory. This limitation is done to gain more randomization space because otherwise only the holes between the direct map, vmalloc, vmemmap and vaddr_end would be usable for randomizing.

The limited direct map size is not exposed to the rest of the kernel, so the memory hot-plug and resource management related code paths still operate under the assumption that the available address space can be determined with MAX_PHYSMEM_BITS.

request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 downwards. That means the first allocation happens past the end of the direct map and if unlucky this address is in the vmalloc space, which causes high_memory to become greater than VMALLOC_START and consequently causes iounmap() to fail for valid ioremap addresses.

MAX_PHYSMEM_BITS cannot be changed for that because the randomization does not align with address bit boundaries and there are other places which actually require to know the maximum number of address bits. All remaining usage sites of MAX_PHYSMEM_BITS have been analyzed and found to be correct.

Cure this by exposing the end of the direct map via PHYSMEM_END and use that for the memory hot-plug and resource management related places instead of relying on MAX_PHYSMEM_BITS. In the KASLR case PHYSMEM_END maps to a variable which is initialized by the KASLR initialization and otherwise it is based on MAX_PHYSMEM_BITS as before.

To prevent future hickups add a check into add_pages() to catch callers trying to add memory above PHYSMEM_END.

Fixes 0483e1fa6e09 ("x86/mm: Implement ASLR for kernel memory regions")

Reported-by Max Ramanouski <[email protected]>

Reported-by Alistair Popple <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/page_64.h          |    1 +
 arch/x86/include/asm/pgtable_64_types.h |    4 ++++
 arch/x86/mm/init_64.c                   |    4 ++++
 arch/x86/mm/kaslr.c                     |   21 ++++++++++++++++++---
 include/linux/mm.h                      |    4 ++++
 kernel/resource.c                       |    6 ++----
 mm/memory_hotplug.c                     |    2 +-
 mm/sparse.c                             |    2 +-
 8 files changed, 35 insertions(+), 9 deletions(-)

arch/x86/include/asm/page_64.h +1 -0

@@ -17,6 +17,7 @@ extern unsigned long phys_base;
 extern unsigned long page_offset_base;
 extern unsigned long vmalloc_base;
 extern unsigned long vmemmap_base;
+extern unsigned long physmem_end;
 
 static __always_inline unsigned long __phys_addr_nodebug(unsigned long x)
 {

arch/x86/include/asm/pgtable_64_types.h +4 -0

@@ -140,6 +140,10 @@ extern unsigned int ptrs_per_p4d;
 # define VMEMMAP_START		__VMEMMAP_BASE_L4
 #endif /* CONFIG_DYNAMIC_MEMORY_LAYOUT */
 
+#ifdef CONFIG_RANDOMIZE_MEMORY
+# define PHYSMEM_END		physmem_end
+#endif
+
 /*
  * End of the region for which vmalloc page tables are pre-allocated.
  * For non-KMSAN builds, this is the same as VMALLOC_END.

arch/x86/mm/init_64.c +4 -0

@@ -958,8 +958,12 @@ static void update_end_of_memory_vars(u6
 int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
 	      struct mhp_params *params)
 {
+	unsigned long end = ((start_pfn + nr_pages) << PAGE_SHIFT) - 1;
 	int ret;
 
+	if (WARN_ON_ONCE(end > PHYSMEM_END))
+		return -ERANGE;
+
 	ret = __add_pages(nid, start_pfn, nr_pages, params);
 	WARN_ON_ONCE(ret);
 

arch/x86/mm/kaslr.c +18 -3

@@ -47,13 +47,24 @@ static const unsigned long vaddr_end = C
  */
 static __initdata struct kaslr_memory_region {
 	unsigned long *base;
+	unsigned long *end;
 	unsigned long size_tb;
 } kaslr_regions[] = {
-	{ &page_offset_base, 0 },
-	{ &vmalloc_base, 0 },
-	{ &vmemmap_base, 0 },
+	{
+		.base	= &page_offset_base,
+		.end	= &physmem_end,
+	},
+	{
+		.base	= &vmalloc_base,
+	},
+	{
+		.base	= &vmemmap_base,
+	},
 };
 
+/* The end of the possible address space for physical memory */
+unsigned long physmem_end __ro_after_init;
+
 /* Get size in bytes used by the memory region */
 static inline unsigned long get_padding(struct kaslr_memory_region *region)
 {

@@ -82,6 +93,8 @@ void __init kernel_randomize_memory(void
 	BUILD_BUG_ON(vaddr_end != CPU_ENTRY_AREA_BASE);
 	BUILD_BUG_ON(vaddr_end > __START_KERNEL_map);
 
+	/* Preset the end of the possible address space for physical memory */
+	physmem_end = ((1ULL << MAX_PHYSMEM_BITS) - 1);
 	if (!kaslr_memory_enabled())
 		return;
 

@@ -134,6 +147,8 @@ void __init kernel_randomize_memory(void
 		 */
 		vaddr += get_padding(&kaslr_regions[i]);
 		vaddr = round_up(vaddr + 1, PUD_SIZE);
+		if (kaslr_regions[i].end)
+			*kaslr_regions[i].end = __pa(vaddr) - 1;
 		remain_entropy -= entropy;
 	}
 }

include/linux/mm.h +4 -0

@@ -97,6 +97,10 @@ extern const int mmap_rnd_compat_bits_ma
 extern int mmap_rnd_compat_bits __read_mostly;
 #endif
 
+#ifndef PHYSMEM_END
+# define PHYSMEM_END	((1ULL << MAX_PHYSMEM_BITS) - 1)
+#endif
+
 #include <asm/page.h>
 #include <asm/processor.h>
 

kernel/resource.c +2 -4

@@ -1826,8 +1826,7 @@ static resource_size_t gfr_start(struct
 	if (flags & GFR_DESCENDING) {
 		resource_size_t end;
 
-		end = min_t(resource_size_t, base->end,
-			    (1ULL << MAX_PHYSMEM_BITS) - 1);
+		end = min_t(resource_size_t, base->end, PHYSMEM_END);
 		return end - size + 1;
 	}
 

@@ -1844,8 +1843,7 @@ static bool gfr_continue(struct resource
 	 * @size did not wrap 0.
 	 */
 	return addr > addr - size &&
-	       addr <= min_t(resource_size_t, base->end,
-			     (1ULL << MAX_PHYSMEM_BITS) - 1);
+	       addr <= min_t(resource_size_t, base->end, PHYSMEM_END);
 }
 
 static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,

mm/memory_hotplug.c +1 -1

@@ -1681,7 +1681,7 @@ struct range __weak arch_get_mappable_ra
 
 struct range mhp_get_pluggable_range(bool need_mapping)
 {
-	const u64 max_phys = (1ULL << MAX_PHYSMEM_BITS) - 1;
+	const u64 max_phys = PHYSMEM_END;
 	struct range mhp_range;
 
 	if (need_mapping) {

mm/sparse.c +1 -1

@@ -129,7 +129,7 @@ static inline int sparse_early_nid(struc
 static void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
 						unsigned long *end_pfn)
 {
-	unsigned long max_sparsemem_pfn = 1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT);
+	unsigned long max_sparsemem_pfn = (PHYSMEM_END + 1) >> PAGE_SHIFT;
 
 	/*
 	 * Sanity checks - do not allow an architecture to pass

iounmap() on x86 occasionally fails to unmap because the provided valid ioremap address is not below high_memory. It turned out that this happens due to KASLR.

KASLR uses the full address space between PAGE_OFFSET and vaddr_end to randomize the starting points of the direct map, vmalloc and vmemmap regions. It thereby limits the size of the direct map by using the installed memory size plus an extra configurable margin for hot-plug memory. This limitation is done to gain more randomization space because otherwise only the holes between the direct map, vmalloc, vmemmap and vaddr_end would be usable for randomizing.

The limited direct map size is not exposed to the rest of the kernel, so the memory hot-plug and resource management related code paths still operate under the assumption that the available address space can be determined with MAX_PHYSMEM_BITS.

request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 downwards. That means the first allocation happens past the end of the direct map and if unlucky this address is in the vmalloc space, which causes high_memory to become greater than VMALLOC_START and consequently causes iounmap() to fail for valid ioremap addresses.

MAX_PHYSMEM_BITS cannot be changed for that because the randomization does not align with address bit boundaries and there are other places which actually require to know the maximum number of address bits. All remaining usage sites of MAX_PHYSMEM_BITS have been analyzed and found to be correct.

Cure this by exposing the end of the direct map via PHYSMEM_END and use that for the memory hot-plug and resource management related places instead of relying on MAX_PHYSMEM_BITS. In the KASLR case PHYSMEM_END maps to a variable which is initialized by the KASLR initialization and otherwise it is based on MAX_PHYSMEM_BITS as before.

To prevent future hickups add a check into add_pages() to catch callers trying to add memory above PHYSMEM_END.

Fixes 0483e1fa6e09 ("x86/mm: Implement ASLR for kernel memory regions")

Reported-by Max Ramanouski <[email protected]>

Reported-by Alistair Popple <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

arch/x86/mm/kaslr.c +0 -0

iounmap() on x86 occasionally fails to unmap because the provided valid ioremap address is not below high_memory. It turned out that this happens due to KASLR.

KASLR uses the full address space between PAGE_OFFSET and vaddr_end to randomize the starting points of the direct map, vmalloc and vmemmap regions. It thereby limits the size of the direct map by using the installed memory size plus an extra configurable margin for hot-plug memory. This limitation is done to gain more randomization space because otherwise only the holes between the direct map, vmalloc, vmemmap and vaddr_end would be usable for randomizing.

The limited direct map size is not exposed to the rest of the kernel, so the memory hot-plug and resource management related code paths still operate under the assumption that the available address space can be determined with MAX_PHYSMEM_BITS.

request_free_mem_region() allocates from (1 << MAX_PHYSMEM_BITS) - 1 downwards. That means the first allocation happens past the end of the direct map and if unlucky this address is in the vmalloc space, which causes high_memory to become greater than VMALLOC_START and consequently causes iounmap() to fail for valid ioremap addresses.

MAX_PHYSMEM_BITS cannot be changed for that because the randomization does not align with address bit boundaries and there are other places which actually require to know the maximum number of address bits. All remaining usage sites of MAX_PHYSMEM_BITS have been analyzed and found to be correct.

Cure this by exposing the end of the direct map via PHYSMEM_END and use that for the memory hot-plug and resource management related places instead of relying on MAX_PHYSMEM_BITS. In the KASLR case PHYSMEM_END maps to a variable which is initialized by the KASLR initialization and otherwise it is based on MAX_PHYSMEM_BITS as before.

To prevent future hickups add a check into add_pages() to catch callers trying to add memory above PHYSMEM_END.

Fixes 0483e1fa6e09 ("x86/mm: Implement ASLR for kernel memory regions")

Reported-by Max Ramanouski <[email protected]>

Reported-by Alistair Popple <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

MAX_PHYSMEM_BITS cannot be changed for that because the randomization does not align with address bit boundaries and there are other places which actually require to know the maximum number of address bits. All remaining usage sites of MAX_PHYSMEM_BITS have been analyzed and found to be correct.

Cure this by exposing the end of the direct map via PHYSMEM_END and use that for the memory hot-plug and resource management related places instead of relying on MAX_PHYSMEM_BITS. In the KASLR case PHYSMEM_END maps to a variable which is initialized by the KASLR initialization and otherwise it is based on MAX_PHYSMEM_BITS as before.

iounmap() on x86 occasionally fails to unmap because the provided valid ioremap address is not below high_memory. It turned out that this happens due to KASLR.