[PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[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.

Signed-off-by: Max Ramanouski <max8rr8@gmail.com>
---
 arch/x86/mm/ioremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index aa7d27932..0b596a1d2 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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;
 
 	/*
-- 
2.41.0

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Alistair Popple]

Max Ramanouski <max8rr8@gmail.com> writes:

> 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.

> 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 <apopple@nvidia.com>

> Signed-off-by: Max Ramanouski <max8rr8@gmail.com>
> ---
>  arch/x86/mm/ioremap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index aa7d27932..0b596a1d2 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -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;
>  
>  	/*

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[John Hubbard]

On 8/7/24 11:12 PM, Alistair Popple wrote:
> 
> Max Ramanouski <max8rr8@gmail.com> writes:
> 
>> 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.
>

I would add that this is really a serious bug. Alistair and the team
here were just getting ready to post a similar fix, after quite some
effort to locate the root cause.


>> 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 <apopple@nvidia.com>
> 
>> Signed-off-by: Max Ramanouski <max8rr8@gmail.com>
>> ---
>>   arch/x86/mm/ioremap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index aa7d27932..0b596a1d2 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -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;
>>   
>>   	/*
> 

I'm also Cc'ing Jason Gunthorpe, as this will be of interest over in RDMA.

Acked-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:

> 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?

Clearly floppy.h has the same issue. It probably does not matter much,
but it's far from correct. memtype_kernel_map_sync() looks fishy too.

Thanks,

        tglx

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

Thomas Gleixner wrote:
> On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:
> 
> > 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?

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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Thu, Aug 08 2024 at 08:58, Dan Williams wrote:
> Thomas Gleixner wrote:
>> On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:
>> 
>> > 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?
>
> 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

arch_add_memory() calls add_pages() ...

Thanks,

        tglx

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

Thomas Gleixner wrote:
> On Thu, Aug 08 2024 at 08:58, Dan Williams wrote:
> > Thomas Gleixner wrote:
> >> On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:
> >> 
> >> > 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?
> >
> > 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
> 
> 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.

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

Dan Williams wrote:
> Thomas Gleixner wrote:
> > On Thu, Aug 08 2024 at 08:58, Dan Williams wrote:
> > > Thomas Gleixner wrote:
> > >> On Thu, Aug 10 2023 at 13:00, Max Ramanouski wrote:
> > >> 
> > >> > 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?
> > >
> > > 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
> > 
> > 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.

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.

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Thu, Aug 08 2024 at 09:39, Dan Williams wrote:
> Dan Williams wrote:
>> 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.
>
> 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.

> 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.

The historical assumption about max_pfn and high_memory is that this is
the end of the actual memory space.

Thanks,

        tglx

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

[ add Alistair and John ]

Thomas Gleixner wrote:
> On Thu, Aug 08 2024 at 09:39, Dan Williams wrote:
> > Dan Williams wrote:
> >> 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.
> >
> > 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

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?

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Alistair Popple]

Dan Williams <dan.j.williams@intel.com> writes:

> [ add Alistair and John ]
>
> Thomas Gleixner wrote:
>> On Thu, Aug 08 2024 at 09:39, Dan Williams wrote:
>> > Dan Williams wrote:
>> >> 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.
>> >
>> > 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
>
> 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.

> 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:

---

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e36261b4ea14..c58d7b0f5bca 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Dan Williams]

Alistair Popple wrote:
> 
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > [ add Alistair and John ]
> >
> > Thomas Gleixner wrote:
> >> On Thu, Aug 08 2024 at 09:39, Dan Williams wrote:
> >> > Dan Williams wrote:
> >> >> 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.
> >> >
> >> > 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
> >
> > 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.
> 
> > 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:
> 
> ---
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index e36261b4ea14..c58d7b0f5bca 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -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

Oh, good find! Alternatively if you wanted some kaslr + private memory
then something like this? (untested):

diff --git a/kernel/resource.c b/kernel/resource.c
index 14777afb0a99..2eeb8d8a40d4 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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;
        }
 

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Thu, Aug 08 2024 at 20:55, Dan Williams wrote:
> Alistair Popple wrote:
>> Dan Williams <dan.j.williams@intel.com> writes:
>> > Thomas Gleixner wrote:
>> >> 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.
>> 
>> > 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).

Duh.

>> 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...

>> 	@@ -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 ....

> 	 @@ -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;
> 		 }

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.

Thanks,

        tglx
---
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -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)
+
 #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
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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>
 
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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, DIRECT_MAP_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, DIRECT_MAP_END);
 }
 
 static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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) {
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Christoph Hellwig]

(Somehow I can't find the original in my inbox, so replying here).

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.

More long term, but I really hate how x86 is not using more of the
generic ioremap code.  kmsan_iounmap_page_range should really move
to generic_iounmap, as should memtype_free after adding a properly
stubbed out API for it.

With that x86 iounmap would just contain a hook for the magic
ISA region, the mmio trace call and then call into generic_ioremap
and we'd get rid of these open coded checks.

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Alistair Popple]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Thu, Aug 08 2024 at 20:55, Dan Williams wrote:
>> Alistair Popple wrote:
>>> Dan Williams <dan.j.williams@intel.com> writes:
>>> > Thomas Gleixner wrote:
>>> >> 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.
>>> 
>>> > 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).
>
> Duh.
>
>>> 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...
>
>>> 	@@ -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.

Actually I've never observed this problem on systems with 5-level page
tables, although I haven't tested extensively there. I don't know of any
reason we wouldn't though, so I guess with the increased entropy I've
just been lucky.

> There the 64T of padding are just not cutting it. MAX_PHYSMEM_BITS is 52
> for 5 level ....
>
>> 	 @@ -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;
>> 		 }
>
> 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.

I looked at more of the MAX_PHYSMEM_BITS usage today but nothing else
stood out as obviously wrong.

> Thanks,
>
>         tglx
> ---
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -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.

> +
>  #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
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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>
>  
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -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, DIRECT_MAP_END);

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:

+		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?

>  		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,
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -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) {
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>> ---
>> --- a/arch/x86/include/asm/pgtable_64_types.h
>> +++ b/arch/x86/include/asm/pgtable_64_types.h
>> @@ -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.

Right. I forgot about the hole.

> 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:
>
> +		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 :)

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Alistair Popple]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
>> Thomas Gleixner <tglx@linutronix.de> writes:
>>> ---
>>> --- a/arch/x86/include/asm/pgtable_64_types.h
>>> +++ b/arch/x86/include/asm/pgtable_64_types.h
>>> @@ -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.
>
> Right. I forgot about the hole.
>
>> 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:
>>
>> +		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 :)

It's a much more plesent 7C in the upside down half of the world atm :)

Anyway I will put together a proper patch to do the above.

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Max R]

> 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 ...

Yep, will do, not sure which commit to attribute to Fixes though. Probably:

41e94a851304 add devm_memremap_pages

As it seems to be the first one that might cause high_memory to overlap
with VMALLOC.

> 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.

And will update that too. The patch is over a year old, and at the time of
submission the generic implementation used is_vmalloc_addr.

Best regards,
Max

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Mon, Aug 12 2024 at 12:03, Thomas Gleixner wrote:
> On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
>> 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 :)

Thanks,

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

--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -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)
 {
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -134,10 +134,12 @@ extern unsigned int ptrs_per_p4d;
 # define VMALLOC_START		vmalloc_base
 # define VMALLOC_SIZE_TB	(pgtable_l5_enabled() ? VMALLOC_SIZE_TB_L5 : VMALLOC_SIZE_TB_L4)
 # define VMEMMAP_START		vmemmap_base
+# define PHYSMEM_END		physmem_end
 #else
 # define VMALLOC_START		__VMALLOC_BASE_L4
 # define VMALLOC_SIZE_TB	VMALLOC_SIZE_TB_L4
 # define VMEMMAP_START		__VMEMMAP_BASE_L4
+# define PHYSMEM_END		((1ULL << MAX_PHYSMEM_BITS) - 1)
 #endif /* CONFIG_DYNAMIC_MEMORY_LAYOUT */
 
 /*
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -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;
 	}
 }
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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	(1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT))
+#endif
+
 #include <asm/page.h>
 #include <asm/processor.h>
 
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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,
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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) {
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Alistair Popple]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Mon, Aug 12 2024 at 12:03, Thomas Gleixner wrote:
>> On Mon, Aug 12 2024 at 17:41, Alistair Popple wrote:
>>> 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 :)

To be fair the previous version did actually build and run fine, it just
didn't fix the issue. However this version does so feel free to add:

Tested-by: Alistair Popple <apopple@nvidia.com>

[...]

> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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	(1UL << (MAX_PHYSMEM_BITS-PAGE_SHIFT))

However I think this should be:

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

 - Alistair

> +#endif
> +
>  #include <asm/page.h>
>  #include <asm/processor.h>
>  
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -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,
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -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) {
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -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

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Tue, Aug 13 2024 at 11:33, Alistair Popple wrote:
>> On Mon, Aug 12 2024 at 12:03, Thomas Gleixner wrote:
>>  
>> +#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.

Re: [PATCH 1/1] x86/ioremap: Use is_vmalloc_addr in iounmap

[Thomas Gleixner]

On Tue, Aug 13 2024 at 10:20, Thomas Gleixner wrote:

> On Tue, Aug 13 2024 at 11:33, Alistair Popple wrote:
>>> On Mon, Aug 12 2024 at 12:03, Thomas Gleixner wrote:
>>>  
>>> +#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.

Aside of that the x86 define must not be inside
CONFIG_DYNAMIC_MEMORY_LAYOUT, it needs to be:

#ifdef CONFIG_RANDOMIZE_MEMORY
# define PHYSMEM_END            physmem_end
#endif

because CONFIG_DYNAMIC_MEMORY_LAYOUT is also selected by 5-level page
tables independent of KASLR. It does not need an else clause either
because that is covered by the generic fallback.

I'll send out a proper patch later.

Thanks,

        tglx

x86/kaslr: Expose and use the end of the physical memory address space

[Thomas Gleixner]

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 <max8rr8@gmail.com>
Reported-by: Alistair Popple <apopple@nvidia.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 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(-)

--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -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)
 {
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -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.
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -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);
 
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -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;
 	}
 }
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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>
 
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -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,
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -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) {
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -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

Re: x86/kaslr: Expose and use the end of the physical memory address space

[Alistair Popple]

Thomas Gleixner <tglx@linutronix.de> writes:

> 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 <max8rr8@gmail.com>
> Reported-by: Alistair Popple <apopple@nvidia.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Thanks Thomas. Looks good and it fixes the issue on a system which
always ran into the iounmap problem. So feel free to add:

Reviewed-by: Alistair Popple <apopple@nvidia.com>
Tested-by: Alistair Popple <apopple@nvidia.com>

> ---
>  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(-)
>
> --- a/arch/x86/include/asm/page_64.h
> +++ b/arch/x86/include/asm/page_64.h
> @@ -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)
>  {
> --- a/arch/x86/include/asm/pgtable_64_types.h
> +++ b/arch/x86/include/asm/pgtable_64_types.h
> @@ -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.
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -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);
>  
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
> @@ -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;
>  	}
>  }
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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>
>  
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -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,
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -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) {
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -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

Re: x86/kaslr: Expose and use the end of the physical memory address space

[Dan Williams]

Thomas Gleixner wrote:
> 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 <max8rr8@gmail.com>
> Reported-by: Alistair Popple <apopple@nvidia.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[..]
> --- a/arch/x86/mm/kaslr.c
> +++ b/arch/x86/mm/kaslr.c
[..]
> @@ -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;

In the context of the patch it is clear that this is physmem_end, when
someone comes to read this later maybe a comment like:

/* 
 * KASLR trims the maximum possible size of the direct-map record that
 * physmem_end boundary here
 */

With or without that the patch looks good to me:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: x86/kaslr: Expose and use the end of the physical memory address space

[Kees Cook]

On Wed, Aug 14, 2024 at 12:29:36AM +0200, Thomas Gleixner wrote:
> 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 <max8rr8@gmail.com>
> Reported-by: Alistair Popple <apopple@nvidia.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Nice fix; thanks!

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

Re: x86/kaslr: Expose and use the end of the physical memory address space

[Max R]

Thanks, tested against my system and it fixed the iounmap problem:

Tested-By: Max Ramanouski <max8rr8@gmail.com>

Best regards,
Max

Re: x86/kaslr: Expose and use the end of the physical memory address space

[David Hildenbrand]

> 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.
> 

Hi Thomas,

> 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.

Skimming this patch, I got confused why physmem_end is introduced.

I can see a single modification of physmem_end:

	physmem_end = ((1ULL << MAX_PHYSMEM_BITS) - 1);


Which is exactly the same as

	#ifndef PHYSMEM_END
	# define PHYSMEM_END	((1ULL << MAX_PHYSMEM_BITS) - 1)
	#endif

And that left me confused why that is required. So we can catch the 
transition "physmem_end == 0" -> "physmem_end != 0"?


I'm probably missing something important, so sorry for the (likely) 
stupid question.


Apart from that, nothing jumped at me.

-- 
Cheers,

David / dhildenb

[tip: x86/urgent] x86/kaslr: Expose and use the end of the physical memory address space

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     dfb3911c3692e45b027f13c7dca3230921533953
Gitweb:        https://git.kernel.org/tip/dfb3911c3692e45b027f13c7dca3230921533953
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 14 Aug 2024 00:29:36 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Fri, 16 Aug 2024 11:33:33 +02:00

x86/kaslr: Expose and use the end of the physical memory address space

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 <max8rr8@gmail.com>
Reported-by: Alistair Popple <apopple@nvidia.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-By: Max Ramanouski <max8rr8@gmail.com> 
Tested-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/87ed6soy3z.ffs@tglx
---
 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                     | 26 +++++++++++++++++++++---
 include/linux/mm.h                      |  4 ++++-
 kernel/resource.c                       |  6 ++----
 mm/memory_hotplug.c                     |  2 +-
 mm/sparse.c                             |  2 +-
 8 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index af4302d..f3d257c 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -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)
 {
diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 9053dfe..a98e534 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -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.
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index d8dbeac..ff25364 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -958,8 +958,12 @@ static void update_end_of_memory_vars(u64 start, u64 size)
 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);
 
diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 37db264..0f2a3a4 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -47,13 +47,24 @@ static const unsigned long vaddr_end = CPU_ENTRY_AREA_BASE;
  */
 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,13 @@ void __init kernel_randomize_memory(void)
 		 */
 		vaddr += get_padding(&kaslr_regions[i]);
 		vaddr = round_up(vaddr + 1, PUD_SIZE);
+
+		/*
+		 * KASLR trims the maximum possible size of the
+		 * direct-map. Update the physmem_end boundary.
+		 */
+		if (kaslr_regions[i].end)
+			*kaslr_regions[i].end = __pa(vaddr) - 1;
 		remain_entropy -= entropy;
 	}
 }
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c4b238a..b386415 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -97,6 +97,10 @@ extern const int mmap_rnd_compat_bits_max;
 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>
 
diff --git a/kernel/resource.c b/kernel/resource.c
index 14777af..a83040f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1826,8 +1826,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
 	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 *base, resource_size_t addr,
 	 * @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,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 66267c2..951878a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1681,7 +1681,7 @@ struct range __weak arch_get_mappable_range(void)
 
 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) {
diff --git a/mm/sparse.c b/mm/sparse.c
index e4b8300..0c3bff8 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -129,7 +129,7 @@ static inline int sparse_early_nid(struct mem_section *section)
 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

[tip: x86/urgent] x86/kaslr: Expose and use the end of the physical memory address space

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     ea72ce5da22806d5713f3ffb39a6d5ae73841f93
Gitweb:        https://git.kernel.org/tip/ea72ce5da22806d5713f3ffb39a6d5ae73841f93
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 14 Aug 2024 00:29:36 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 20 Aug 2024 13:44:57 +02:00

x86/kaslr: Expose and use the end of the physical memory address space

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 <max8rr8@gmail.com>
Reported-by: Alistair Popple <apopple@nvidia.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-By: Max Ramanouski <max8rr8@gmail.com>
Tested-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/87ed6soy3z.ffs@tglx
---
 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                     | 32 +++++++++++++++++++-----
 include/linux/mm.h                      |  4 +++-
 kernel/resource.c                       |  6 +----
 mm/memory_hotplug.c                     |  2 +-
 mm/sparse.c                             |  2 +-
 8 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index af4302d..f3d257c 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -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)
 {
diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 9053dfe..a98e534 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -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.
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index d8dbeac..ff25364 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -958,8 +958,12 @@ static void update_end_of_memory_vars(u64 start, u64 size)
 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);
 
diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
index 37db264..230f1de 100644
--- a/arch/x86/mm/kaslr.c
+++ b/arch/x86/mm/kaslr.c
@@ -47,13 +47,24 @@ static const unsigned long vaddr_end = CPU_ENTRY_AREA_BASE;
  */
 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;
 
@@ -128,11 +141,18 @@ void __init kernel_randomize_memory(void)
 		vaddr += entropy;
 		*kaslr_regions[i].base = vaddr;
 
+		/* Calculate the end of the region */
+		vaddr += get_padding(&kaslr_regions[i]);
 		/*
-		 * Jump the region and add a minimum padding based on
-		 * randomization alignment.
+		 * KASLR trims the maximum possible size of the
+		 * direct-map. Update the physmem_end boundary.
+		 * No rounding required as the region starts
+		 * PUD aligned and size is in units of TB.
 		 */
-		vaddr += get_padding(&kaslr_regions[i]);
+		if (kaslr_regions[i].end)
+			*kaslr_regions[i].end = __pa_nodebug(vaddr - 1);
+
+		/* Add a minimum padding based on randomization alignment. */
 		vaddr = round_up(vaddr + 1, PUD_SIZE);
 		remain_entropy -= entropy;
 	}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c4b238a..b386415 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -97,6 +97,10 @@ extern const int mmap_rnd_compat_bits_max;
 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>
 
diff --git a/kernel/resource.c b/kernel/resource.c
index 14777af..a83040f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1826,8 +1826,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
 	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 *base, resource_size_t addr,
 	 * @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,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 66267c2..951878a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1681,7 +1681,7 @@ struct range __weak arch_get_mappable_range(void)
 
 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) {
diff --git a/mm/sparse.c b/mm/sparse.c
index e4b8300..0c3bff8 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -129,7 +129,7 @@ static inline int sparse_early_nid(struct mem_section *section)
 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

Re: x86/kaslr: Expose and use the end of the physical memory address space

[Guenter Roeck]

On Wed, Aug 14, 2024 at 12:29:36AM +0200, Thomas Gleixner wrote:
> 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.
> 
[ ... ]
>  
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -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;
>  	}

When trying to build arm:allmodconfig or mips:allmodconfig (and probably
others):

Building arm:allmodconfig ... failed
--------------
Error log:
In file included from include/linux/ioport.h:15,
                 from kernel/resource.c:15:
kernel/resource.c: In function 'gfr_start':
include/linux/minmax.h:93:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Werror=overflow]
   93 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
      |                                     ^
include/linux/minmax.h:96:9: note: in expansion of macro '__cmp_once_unique'
   96 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
      |         ^~~~~~~~~~~~~~~~~
include/linux/minmax.h:213:27: note: in expansion of macro '__cmp_once'
  213 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
      |                           ^~~~~~~~~~
kernel/resource.c:1874:23: note: in expansion of macro 'min_t'
 1874 |                 end = min_t(resource_size_t, base->end, PHYSMEM_END);
      |                       ^~~~~
kernel/resource.c: In function 'gfr_continue':
include/linux/minmax.h:93:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Werror=overflow]
   93 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
      |                                     ^
include/linux/minmax.h:96:9: note: in expansion of macro '__cmp_once_unique'
   96 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
      |         ^~~~~~~~~~~~~~~~~
include/linux/minmax.h:213:27: note: in expansion of macro '__cmp_once'
  213 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
      |                           ^~~~~~~~~~
kernel/resource.c:1891:24: note: in expansion of macro 'min_t'
 1891 |                addr <= min_t(resource_size_t, base->end, PHYSMEM_END);
      |                        ^~~~~

Guenter