[PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

During memory hotplug process, the linear mapping should not be created for
a given memory range if that would fall outside the maximum allowed linear
range. Else it might cause memory corruption in the kernel virtual space.

Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
both its ends but excluding PAGE_END. Max physical range that can be mapped
inside this linear mapping range, must also be derived from its end points.

When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
assumption of 52 bits virtual address space. However, if the CPU does not
support 52 bits, then it falls back using 48 bits instead and the PAGE_END
is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
bits [51..48] are ignored by the MMU and remain unchanged, even though the
effective start address of linear map is now slightly different. Hence, to
reliably check the physical address range mapped by the linear map, the
start address should be calculated using vabits_actual. This ensures that
arch_add_memory() validates memory hot add range for its potential linear
mapping requirement, before creating it with __create_pgd_mapping().

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Steven Price <steven.price@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 75df62fea1b6..d59ffabb9c84 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
 	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
 }
 
+static bool inside_linear_region(u64 start, u64 size)
+{
+	/*
+	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
+	 * accommodating both its ends but excluding PAGE_END. Max physical
+	 * range which can be mapped inside this linear mapping range, must
+	 * also be derived from its end points.
+	 *
+	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
+	 * the assumption of 52 bits virtual address space. However, if the
+	 * CPU does not support 52 bits, it falls back using 48 bits and the
+	 * PAGE_END is updated to reflect this using the vabits_actual. As
+	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
+	 * unchanged, even though the effective start address of linear map
+	 * is now slightly different. Hence, to reliably check the physical
+	 * address range mapped by the linear map, the start address should
+	 * be calculated using vabits_actual.
+	 */
+	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
+			&& ((start + size) <= __pa(PAGE_END - 1)));
+}
+
 int arch_add_memory(int nid, u64 start, u64 size,
 		    struct mhp_params *params)
 {
 	int ret, flags = 0;
 
+	if (!inside_linear_region(start, size)) {
+		pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
+		return -EINVAL;
+	}
+
 	if (rodata_full || debug_pagealloc_enabled())
 		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
 
-- 
2.20.1

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Will Deacon]

On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
> During memory hotplug process, the linear mapping should not be created for
> a given memory range if that would fall outside the maximum allowed linear
> range. Else it might cause memory corruption in the kernel virtual space.
> 
> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
> both its ends but excluding PAGE_END. Max physical range that can be mapped
> inside this linear mapping range, must also be derived from its end points.
> 
> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
> assumption of 52 bits virtual address space. However, if the CPU does not
> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
> bits [51..48] are ignored by the MMU and remain unchanged, even though the
> effective start address of linear map is now slightly different. Hence, to
> reliably check the physical address range mapped by the linear map, the
> start address should be calculated using vabits_actual. This ensures that
> arch_add_memory() validates memory hot add range for its potential linear
> mapping requirement, before creating it with __create_pgd_mapping().
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 75df62fea1b6..d59ffabb9c84 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>  }
>  
> +static bool inside_linear_region(u64 start, u64 size)
> +{
> +	/*
> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
> +	 * accommodating both its ends but excluding PAGE_END. Max physical
> +	 * range which can be mapped inside this linear mapping range, must
> +	 * also be derived from its end points.
> +	 *
> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
> +	 * the assumption of 52 bits virtual address space. However, if the
> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
> +	 * unchanged, even though the effective start address of linear map
> +	 * is now slightly different. Hence, to reliably check the physical
> +	 * address range mapped by the linear map, the start address should
> +	 * be calculated using vabits_actual.
> +	 */
> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> +			&& ((start + size) <= __pa(PAGE_END - 1)));
> +}

Why isn't this implemented using the existing __is_lm_address()?

Will

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 09/29/2020 02:05 AM, Will Deacon wrote:
> On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
>> During memory hotplug process, the linear mapping should not be created for
>> a given memory range if that would fall outside the maximum allowed linear
>> range. Else it might cause memory corruption in the kernel virtual space.
>>
>> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
>> both its ends but excluding PAGE_END. Max physical range that can be mapped
>> inside this linear mapping range, must also be derived from its end points.
>>
>> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
>> assumption of 52 bits virtual address space. However, if the CPU does not
>> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
>> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
>> bits [51..48] are ignored by the MMU and remain unchanged, even though the
>> effective start address of linear map is now slightly different. Hence, to
>> reliably check the physical address range mapped by the linear map, the
>> start address should be calculated using vabits_actual. This ensures that
>> arch_add_memory() validates memory hot add range for its potential linear
>> mapping requirement, before creating it with __create_pgd_mapping().
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: Steven Price <steven.price@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 75df62fea1b6..d59ffabb9c84 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>>  }
>>  
>> +static bool inside_linear_region(u64 start, u64 size)
>> +{
>> +	/*
>> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
>> +	 * accommodating both its ends but excluding PAGE_END. Max physical
>> +	 * range which can be mapped inside this linear mapping range, must
>> +	 * also be derived from its end points.
>> +	 *
>> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
>> +	 * the assumption of 52 bits virtual address space. However, if the
>> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
>> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
>> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
>> +	 * unchanged, even though the effective start address of linear map
>> +	 * is now slightly different. Hence, to reliably check the physical
>> +	 * address range mapped by the linear map, the start address should
>> +	 * be calculated using vabits_actual.
>> +	 */
>> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
>> +			&& ((start + size) <= __pa(PAGE_END - 1)));
>> +}
> 
> Why isn't this implemented using the existing __is_lm_address()?

Not sure, if I understood your suggestion here. The physical address range
[start..start + size] needs to be checked against maximum physical range
that can be represented inside effective boundaries for the linear mapping
i.e [__pa(_PAGE_OFFSET(vabits_actual)..__pa(PAGE_END - 1)].

Are you suggesting [start..start + size] should be first be converted into
a virtual address range and then checked against __is_lm_addresses() ? But
is not deriving the physical range from from know limits of linear mapping
much cleaner ?

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Will Deacon]

On Tue, Sep 29, 2020 at 01:34:24PM +0530, Anshuman Khandual wrote:
> 
> 
> On 09/29/2020 02:05 AM, Will Deacon wrote:
> > On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
> >> During memory hotplug process, the linear mapping should not be created for
> >> a given memory range if that would fall outside the maximum allowed linear
> >> range. Else it might cause memory corruption in the kernel virtual space.
> >>
> >> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
> >> both its ends but excluding PAGE_END. Max physical range that can be mapped
> >> inside this linear mapping range, must also be derived from its end points.
> >>
> >> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
> >> assumption of 52 bits virtual address space. However, if the CPU does not
> >> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
> >> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
> >> bits [51..48] are ignored by the MMU and remain unchanged, even though the
> >> effective start address of linear map is now slightly different. Hence, to
> >> reliably check the physical address range mapped by the linear map, the
> >> start address should be calculated using vabits_actual. This ensures that
> >> arch_add_memory() validates memory hot add range for its potential linear
> >> mapping requirement, before creating it with __create_pgd_mapping().
> >>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Ard Biesheuvel <ardb@kernel.org>
> >> Cc: Steven Price <steven.price@arm.com>
> >> Cc: Robin Murphy <robin.murphy@arm.com>
> >> Cc: David Hildenbrand <david@redhat.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >> index 75df62fea1b6..d59ffabb9c84 100644
> >> --- a/arch/arm64/mm/mmu.c
> >> +++ b/arch/arm64/mm/mmu.c
> >> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
> >>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
> >>  }
> >>  
> >> +static bool inside_linear_region(u64 start, u64 size)
> >> +{
> >> +	/*
> >> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
> >> +	 * accommodating both its ends but excluding PAGE_END. Max physical
> >> +	 * range which can be mapped inside this linear mapping range, must
> >> +	 * also be derived from its end points.
> >> +	 *
> >> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
> >> +	 * the assumption of 52 bits virtual address space. However, if the
> >> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
> >> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
> >> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
> >> +	 * unchanged, even though the effective start address of linear map
> >> +	 * is now slightly different. Hence, to reliably check the physical
> >> +	 * address range mapped by the linear map, the start address should
> >> +	 * be calculated using vabits_actual.
> >> +	 */
> >> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> >> +			&& ((start + size) <= __pa(PAGE_END - 1)));
> >> +}
> > 
> > Why isn't this implemented using the existing __is_lm_address()?
> 
> Not sure, if I understood your suggestion here. The physical address range
> [start..start + size] needs to be checked against maximum physical range
> that can be represented inside effective boundaries for the linear mapping
> i.e [__pa(_PAGE_OFFSET(vabits_actual)..__pa(PAGE_END - 1)].
> 
> Are you suggesting [start..start + size] should be first be converted into
> a virtual address range and then checked against __is_lm_addresses() ? But
> is not deriving the physical range from from know limits of linear mapping
> much cleaner ?

I just think having a function called "inside_linear_region()" as well as a
macro called "__is_lm_address()" is weird when they have completely separate
implementations. They're obviously trying to do the same thing, just the
first one gets given physical address as parameters.

Implementing one in terms of the other is much better for maintenance.

Will

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 09/29/2020 08:52 PM, Will Deacon wrote:
> On Tue, Sep 29, 2020 at 01:34:24PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 09/29/2020 02:05 AM, Will Deacon wrote:
>>> On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
>>>> During memory hotplug process, the linear mapping should not be created for
>>>> a given memory range if that would fall outside the maximum allowed linear
>>>> range. Else it might cause memory corruption in the kernel virtual space.
>>>>
>>>> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
>>>> both its ends but excluding PAGE_END. Max physical range that can be mapped
>>>> inside this linear mapping range, must also be derived from its end points.
>>>>
>>>> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
>>>> assumption of 52 bits virtual address space. However, if the CPU does not
>>>> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
>>>> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
>>>> bits [51..48] are ignored by the MMU and remain unchanged, even though the
>>>> effective start address of linear map is now slightly different. Hence, to
>>>> reliably check the physical address range mapped by the linear map, the
>>>> start address should be calculated using vabits_actual. This ensures that
>>>> arch_add_memory() validates memory hot add range for its potential linear
>>>> mapping requirement, before creating it with __create_pgd_mapping().
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>> Cc: Steven Price <steven.price@arm.com>
>>>> Cc: Robin Murphy <robin.murphy@arm.com>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>>>>  1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>> index 75df62fea1b6..d59ffabb9c84 100644
>>>> --- a/arch/arm64/mm/mmu.c
>>>> +++ b/arch/arm64/mm/mmu.c
>>>> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>>>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>>>>  }
>>>>  
>>>> +static bool inside_linear_region(u64 start, u64 size)
>>>> +{
>>>> +	/*
>>>> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
>>>> +	 * accommodating both its ends but excluding PAGE_END. Max physical
>>>> +	 * range which can be mapped inside this linear mapping range, must
>>>> +	 * also be derived from its end points.
>>>> +	 *
>>>> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
>>>> +	 * the assumption of 52 bits virtual address space. However, if the
>>>> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
>>>> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
>>>> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
>>>> +	 * unchanged, even though the effective start address of linear map
>>>> +	 * is now slightly different. Hence, to reliably check the physical
>>>> +	 * address range mapped by the linear map, the start address should
>>>> +	 * be calculated using vabits_actual.
>>>> +	 */
>>>> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
>>>> +			&& ((start + size) <= __pa(PAGE_END - 1)));
>>>> +}
>>>
>>> Why isn't this implemented using the existing __is_lm_address()?
>>
>> Not sure, if I understood your suggestion here. The physical address range
>> [start..start + size] needs to be checked against maximum physical range
>> that can be represented inside effective boundaries for the linear mapping
>> i.e [__pa(_PAGE_OFFSET(vabits_actual)..__pa(PAGE_END - 1)].
>>
>> Are you suggesting [start..start + size] should be first be converted into
>> a virtual address range and then checked against __is_lm_addresses() ? But
>> is not deriving the physical range from from know limits of linear mapping
>> much cleaner ?
> 
> I just think having a function called "inside_linear_region()" as well as a
> macro called "__is_lm_address()" is weird when they have completely separate
> implementations. They're obviously trying to do the same thing, just the
> first one gets given physical address as parameters.
> 
> Implementing one in terms of the other is much better for maintenance.

/*
 * The linear kernel range starts at the bottom of the virtual address
 * space. Testing the top bit for the start of the region is a
 * sufficient check and avoids having to worry about the tag.
 */
#define __is_lm_address(addr)   (!(((u64)addr) & BIT(vabits_actual - 1)))

__is_lm_address() currently just check the highest bit in a virtual address
where the linear mapping ends i.e the lower half and all other kernel mapping
starts i.e the upper half. But I would believe, it misses the blind range
[_PAGE_OFFSET(VA_BITS).._PAGE_OFFSET(vabits_actual)] in some configurations,
even though it does not really affect anything because it gets ignored by the
MMU. Hence in current form __is_lm_address() cannot be used to derive maximum
linear range and it's corresponding physical range for hotplug range check.

But if __is_lm_address() checks against the effective linear range instead
i.e [_PAGE_OFFSET(vabits_actual)..(PAGE_END - 1)], it can be used for hot
plug physical range check there after. Perhaps something like this, though
not tested properly.

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index afa722504bfd..6da046b479d4 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -238,7 +238,10 @@ static inline const void *__tag_set(const void *addr, u8 tag)
  * space. Testing the top bit for the start of the region is a
  * sufficient check and avoids having to worry about the tag.
  */
-#define __is_lm_address(addr)  (!(((u64)addr) & BIT(vabits_actual - 1)))
+static inline bool __is_lm_address(unsigned long addr)
+{
+       return ((addr >= _PAGE_OFFSET(vabits_actual)) && (addr <= (PAGE_END - 1)));
+}
 
 #define __lm_to_phys(addr)     (((addr) + physvirt_offset))
 #define __kimg_to_phys(addr)   ((addr) - kimage_voffset)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index d59ffabb9c84..5750370a7e8c 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1451,8 +1451,7 @@ static bool inside_linear_region(u64 start, u64 size)
         * address range mapped by the linear map, the start address should
         * be calculated using vabits_actual.
         */
-       return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
-                       && ((start + size) <= __pa(PAGE_END - 1)));
+       return __is_lm_address(__va(start)) && __is_lm_address(__va(start + size));
 }
 
 int arch_add_memory(int nid, u64 start, u64 size,

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Ard Biesheuvel]

On Wed, 30 Sep 2020 at 10:03, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
> On 09/29/2020 08:52 PM, Will Deacon wrote:
> > On Tue, Sep 29, 2020 at 01:34:24PM +0530, Anshuman Khandual wrote:
> >>
> >>
> >> On 09/29/2020 02:05 AM, Will Deacon wrote:
> >>> On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
> >>>> During memory hotplug process, the linear mapping should not be created for
> >>>> a given memory range if that would fall outside the maximum allowed linear
> >>>> range. Else it might cause memory corruption in the kernel virtual space.
> >>>>
> >>>> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
> >>>> both its ends but excluding PAGE_END. Max physical range that can be mapped
> >>>> inside this linear mapping range, must also be derived from its end points.
> >>>>
> >>>> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
> >>>> assumption of 52 bits virtual address space. However, if the CPU does not
> >>>> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
> >>>> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
> >>>> bits [51..48] are ignored by the MMU and remain unchanged, even though the
> >>>> effective start address of linear map is now slightly different. Hence, to
> >>>> reliably check the physical address range mapped by the linear map, the
> >>>> start address should be calculated using vabits_actual. This ensures that
> >>>> arch_add_memory() validates memory hot add range for its potential linear
> >>>> mapping requirement, before creating it with __create_pgd_mapping().
> >>>>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: Ard Biesheuvel <ardb@kernel.org>
> >>>> Cc: Steven Price <steven.price@arm.com>
> >>>> Cc: Robin Murphy <robin.murphy@arm.com>
> >>>> Cc: David Hildenbrand <david@redhat.com>
> >>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
> >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >>>> ---
> >>>>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
> >>>>  1 file changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>>> index 75df62fea1b6..d59ffabb9c84 100644
> >>>> --- a/arch/arm64/mm/mmu.c
> >>>> +++ b/arch/arm64/mm/mmu.c
> >>>> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
> >>>>    free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
> >>>>  }
> >>>>
> >>>> +static bool inside_linear_region(u64 start, u64 size)
> >>>> +{
> >>>> +  /*
> >>>> +   * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
> >>>> +   * accommodating both its ends but excluding PAGE_END. Max physical
> >>>> +   * range which can be mapped inside this linear mapping range, must
> >>>> +   * also be derived from its end points.
> >>>> +   *
> >>>> +   * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
> >>>> +   * the assumption of 52 bits virtual address space. However, if the
> >>>> +   * CPU does not support 52 bits, it falls back using 48 bits and the
> >>>> +   * PAGE_END is updated to reflect this using the vabits_actual. As
> >>>> +   * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
> >>>> +   * unchanged, even though the effective start address of linear map
> >>>> +   * is now slightly different. Hence, to reliably check the physical
> >>>> +   * address range mapped by the linear map, the start address should
> >>>> +   * be calculated using vabits_actual.
> >>>> +   */
> >>>> +  return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> >>>> +                  && ((start + size) <= __pa(PAGE_END - 1)));
> >>>> +}
> >>>
> >>> Why isn't this implemented using the existing __is_lm_address()?
> >>
> >> Not sure, if I understood your suggestion here. The physical address range
> >> [start..start + size] needs to be checked against maximum physical range
> >> that can be represented inside effective boundaries for the linear mapping
> >> i.e [__pa(_PAGE_OFFSET(vabits_actual)..__pa(PAGE_END - 1)].
> >>
> >> Are you suggesting [start..start + size] should be first be converted into
> >> a virtual address range and then checked against __is_lm_addresses() ? But
> >> is not deriving the physical range from from know limits of linear mapping
> >> much cleaner ?
> >
> > I just think having a function called "inside_linear_region()" as well as a
> > macro called "__is_lm_address()" is weird when they have completely separate
> > implementations. They're obviously trying to do the same thing, just the
> > first one gets given physical address as parameters.
> >
> > Implementing one in terms of the other is much better for maintenance.
>
> /*
>  * The linear kernel range starts at the bottom of the virtual address
>  * space. Testing the top bit for the start of the region is a
>  * sufficient check and avoids having to worry about the tag.
>  */
> #define __is_lm_address(addr)   (!(((u64)addr) & BIT(vabits_actual - 1)))
>
> __is_lm_address() currently just check the highest bit in a virtual address
> where the linear mapping ends i.e the lower half and all other kernel mapping
> starts i.e the upper half. But I would believe, it misses the blind range
> [_PAGE_OFFSET(VA_BITS).._PAGE_OFFSET(vabits_actual)] in some configurations,
> even though it does not really affect anything because it gets ignored by the
> MMU. Hence in current form __is_lm_address() cannot be used to derive maximum
> linear range and it's corresponding physical range for hotplug range check.
>

This is actually something that I brought up when the 52-bit VA
changes were under review: currently, we split the VA space in half,
and use the lower half for the linear range, and the upper half for
vmalloc, kernel, modules, vmemmap etc

When we run a 48-bit kernel on 52-bit capable hardware, we mostly
stick with the same arrangement: 2^51 bytes of linear range, but only
2^47 bytes of vmalloc range (as the size is fixed), and so we are
wasting 15/16 of the vmalloc region (2^51 - 2^47), by not using it to
map anything.

So it would be better to get away from this notion that the VA space
is divided evenly between linear and vmalloc regions, and use the
entire range between ~0 << 52 and ~0 << 47 for the linear region (Note
that the KASAN region defimnition will overlap the linear region in
this case, by we should be able to sort that at runtime)

-- 
Ard.

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 09/30/2020 04:31 PM, Ard Biesheuvel wrote:
> On Wed, 30 Sep 2020 at 10:03, Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>>
>> On 09/29/2020 08:52 PM, Will Deacon wrote:
>>> On Tue, Sep 29, 2020 at 01:34:24PM +0530, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 09/29/2020 02:05 AM, Will Deacon wrote:
>>>>> On Thu, Sep 17, 2020 at 02:16:42PM +0530, Anshuman Khandual wrote:
>>>>>> During memory hotplug process, the linear mapping should not be created for
>>>>>> a given memory range if that would fall outside the maximum allowed linear
>>>>>> range. Else it might cause memory corruption in the kernel virtual space.
>>>>>>
>>>>>> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
>>>>>> both its ends but excluding PAGE_END. Max physical range that can be mapped
>>>>>> inside this linear mapping range, must also be derived from its end points.
>>>>>>
>>>>>> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
>>>>>> assumption of 52 bits virtual address space. However, if the CPU does not
>>>>>> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
>>>>>> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
>>>>>> bits [51..48] are ignored by the MMU and remain unchanged, even though the
>>>>>> effective start address of linear map is now slightly different. Hence, to
>>>>>> reliably check the physical address range mapped by the linear map, the
>>>>>> start address should be calculated using vabits_actual. This ensures that
>>>>>> arch_add_memory() validates memory hot add range for its potential linear
>>>>>> mapping requirement, before creating it with __create_pgd_mapping().
>>>>>>
>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>>>> Cc: Steven Price <steven.price@arm.com>
>>>>>> Cc: Robin Murphy <robin.murphy@arm.com>
>>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>> Cc: linux-kernel@vger.kernel.org
>>>>>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
>>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>>> ---
>>>>>>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>>>>>>  1 file changed, 27 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>>>>> index 75df62fea1b6..d59ffabb9c84 100644
>>>>>> --- a/arch/arm64/mm/mmu.c
>>>>>> +++ b/arch/arm64/mm/mmu.c
>>>>>> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>>>>>    free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>>>>>>  }
>>>>>>
>>>>>> +static bool inside_linear_region(u64 start, u64 size)
>>>>>> +{
>>>>>> +  /*
>>>>>> +   * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
>>>>>> +   * accommodating both its ends but excluding PAGE_END. Max physical
>>>>>> +   * range which can be mapped inside this linear mapping range, must
>>>>>> +   * also be derived from its end points.
>>>>>> +   *
>>>>>> +   * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
>>>>>> +   * the assumption of 52 bits virtual address space. However, if the
>>>>>> +   * CPU does not support 52 bits, it falls back using 48 bits and the
>>>>>> +   * PAGE_END is updated to reflect this using the vabits_actual. As
>>>>>> +   * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
>>>>>> +   * unchanged, even though the effective start address of linear map
>>>>>> +   * is now slightly different. Hence, to reliably check the physical
>>>>>> +   * address range mapped by the linear map, the start address should
>>>>>> +   * be calculated using vabits_actual.
>>>>>> +   */
>>>>>> +  return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
>>>>>> +                  && ((start + size) <= __pa(PAGE_END - 1)));
>>>>>> +}
>>>>>
>>>>> Why isn't this implemented using the existing __is_lm_address()?
>>>>
>>>> Not sure, if I understood your suggestion here. The physical address range
>>>> [start..start + size] needs to be checked against maximum physical range
>>>> that can be represented inside effective boundaries for the linear mapping
>>>> i.e [__pa(_PAGE_OFFSET(vabits_actual)..__pa(PAGE_END - 1)].
>>>>
>>>> Are you suggesting [start..start + size] should be first be converted into
>>>> a virtual address range and then checked against __is_lm_addresses() ? But
>>>> is not deriving the physical range from from know limits of linear mapping
>>>> much cleaner ?
>>>
>>> I just think having a function called "inside_linear_region()" as well as a
>>> macro called "__is_lm_address()" is weird when they have completely separate
>>> implementations. They're obviously trying to do the same thing, just the
>>> first one gets given physical address as parameters.
>>>
>>> Implementing one in terms of the other is much better for maintenance.
>>
>> /*
>>  * The linear kernel range starts at the bottom of the virtual address
>>  * space. Testing the top bit for the start of the region is a
>>  * sufficient check and avoids having to worry about the tag.
>>  */
>> #define __is_lm_address(addr)   (!(((u64)addr) & BIT(vabits_actual - 1)))
>>
>> __is_lm_address() currently just check the highest bit in a virtual address
>> where the linear mapping ends i.e the lower half and all other kernel mapping
>> starts i.e the upper half. But I would believe, it misses the blind range
>> [_PAGE_OFFSET(VA_BITS).._PAGE_OFFSET(vabits_actual)] in some configurations,
>> even though it does not really affect anything because it gets ignored by the
>> MMU. Hence in current form __is_lm_address() cannot be used to derive maximum
>> linear range and it's corresponding physical range for hotplug range check.
>>
> 
> This is actually something that I brought up when the 52-bit VA
> changes were under review: currently, we split the VA space in half,
> and use the lower half for the linear range, and the upper half for
> vmalloc, kernel, modules, vmemmap etc

Right.

> 
> When we run a 48-bit kernel on 52-bit capable hardware, we mostly
> stick with the same arrangement: 2^51 bytes of linear range, but only
> 2^47 bytes of vmalloc range (as the size is fixed), and so we are
> wasting 15/16 of the vmalloc region (2^51 - 2^47), by not using it to
> map anything.

Right, there are unused gaps in the kernel virtual space with current
arrangement for various sections.

> 
> So it would be better to get away from this notion that the VA space
> is divided evenly between linear and vmalloc regions, and use the
> entire range between ~0 << 52 and ~0 << 47 for the linear region (Note
> that the KASAN region defimnition will overlap the linear region in
> this case, by we should be able to sort that at runtime)

Right, kernel virtual space management needs rethink for optimal address
range utilization while reducing unused areas. I have been experimenting
with this for a while but nothing particular to propose yet. Nonetheless
for now, we need to fix this address range problem for memory hotplug.

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 09/30/2020 01:32 PM, Anshuman Khandual wrote:
> But if __is_lm_address() checks against the effective linear range instead
> i.e [_PAGE_OFFSET(vabits_actual)..(PAGE_END - 1)], it can be used for hot
> plug physical range check there after. Perhaps something like this, though
> not tested properly.
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index afa722504bfd..6da046b479d4 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -238,7 +238,10 @@ static inline const void *__tag_set(const void *addr, u8 tag)
>   * space. Testing the top bit for the start of the region is a
>   * sufficient check and avoids having to worry about the tag.
>   */
> -#define __is_lm_address(addr)  (!(((u64)addr) & BIT(vabits_actual - 1)))
> +static inline bool __is_lm_address(unsigned long addr)
> +{
> +       return ((addr >= _PAGE_OFFSET(vabits_actual)) && (addr <= (PAGE_END - 1)));
> +}
>  
>  #define __lm_to_phys(addr)     (((addr) + physvirt_offset))
>  #define __kimg_to_phys(addr)   ((addr) - kimage_voffset)
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index d59ffabb9c84..5750370a7e8c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1451,8 +1451,7 @@ static bool inside_linear_region(u64 start, u64 size)
>          * address range mapped by the linear map, the start address should
>          * be calculated using vabits_actual.
>          */
> -       return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> -                       && ((start + size) <= __pa(PAGE_END - 1)));
> +       return __is_lm_address(__va(start)) && __is_lm_address(__va(start + size));
>  }
>  
>  int arch_add_memory(int nid, u64 start, u64 size,

Will/Ard,

Any thoughts about this ? __is_lm_address() now checks for a range instead
of a bit. This will be compatible later on, even if linear mapping range
changes from current lower half scheme.

- Anshuman

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Ard Biesheuvel]

On Tue, 6 Oct 2020 at 08:36, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
>
> On 09/30/2020 01:32 PM, Anshuman Khandual wrote:
> > But if __is_lm_address() checks against the effective linear range instead
> > i.e [_PAGE_OFFSET(vabits_actual)..(PAGE_END - 1)], it can be used for hot
> > plug physical range check there after. Perhaps something like this, though
> > not tested properly.
> >
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index afa722504bfd..6da046b479d4 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -238,7 +238,10 @@ static inline const void *__tag_set(const void *addr, u8 tag)
> >   * space. Testing the top bit for the start of the region is a
> >   * sufficient check and avoids having to worry about the tag.
> >   */
> > -#define __is_lm_address(addr)  (!(((u64)addr) & BIT(vabits_actual - 1)))
> > +static inline bool __is_lm_address(unsigned long addr)
> > +{
> > +       return ((addr >= _PAGE_OFFSET(vabits_actual)) && (addr <= (PAGE_END - 1)));
> > +}
> >
> >  #define __lm_to_phys(addr)     (((addr) + physvirt_offset))
> >  #define __kimg_to_phys(addr)   ((addr) - kimage_voffset)
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index d59ffabb9c84..5750370a7e8c 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -1451,8 +1451,7 @@ static bool inside_linear_region(u64 start, u64 size)
> >          * address range mapped by the linear map, the start address should
> >          * be calculated using vabits_actual.
> >          */
> > -       return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> > -                       && ((start + size) <= __pa(PAGE_END - 1)));
> > +       return __is_lm_address(__va(start)) && __is_lm_address(__va(start + size));
> >  }
> >
> >  int arch_add_memory(int nid, u64 start, u64 size,
>
> Will/Ard,
>
> Any thoughts about this ? __is_lm_address() now checks for a range instead
> of a bit. This will be compatible later on, even if linear mapping range
> changes from current lower half scheme.
>

As I'm sure you have noticed, I sent out some patches that get rid of
physvirt_offset, and which simplify __is_lm_address() to only take
compile time constants into account (unless KASAN is enabled). This
means that in the 52-bit VA case, __is_lm_address() does not
distinguish between virtual addresses that can be mapped by the
hardware and ones that cannot.

In the memory hotplug case, we need to decide whether the added memory
will appear in the addressable area, which is a different question. So
it makes sense to duplicate some of the logic that exists in
arm64_memblock_init() (or factor it out) to decide whether this newly
added memory will appear in the addressable window or not.

So I think your original approach makes more sense here, although I
think you want '(start + size - 1) <= __pa(PAGE_END - 1)' in the
comparison above (and please drop the redundant parens)

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 10/12/2020 12:59 PM, Ard Biesheuvel wrote:
> On Tue, 6 Oct 2020 at 08:36, Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
>>
>>
>>
>> On 09/30/2020 01:32 PM, Anshuman Khandual wrote:
>>> But if __is_lm_address() checks against the effective linear range instead
>>> i.e [_PAGE_OFFSET(vabits_actual)..(PAGE_END - 1)], it can be used for hot
>>> plug physical range check there after. Perhaps something like this, though
>>> not tested properly.
>>>
>>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>>> index afa722504bfd..6da046b479d4 100644
>>> --- a/arch/arm64/include/asm/memory.h
>>> +++ b/arch/arm64/include/asm/memory.h
>>> @@ -238,7 +238,10 @@ static inline const void *__tag_set(const void *addr, u8 tag)
>>>   * space. Testing the top bit for the start of the region is a
>>>   * sufficient check and avoids having to worry about the tag.
>>>   */
>>> -#define __is_lm_address(addr)  (!(((u64)addr) & BIT(vabits_actual - 1)))
>>> +static inline bool __is_lm_address(unsigned long addr)
>>> +{
>>> +       return ((addr >= _PAGE_OFFSET(vabits_actual)) && (addr <= (PAGE_END - 1)));
>>> +}
>>>
>>>  #define __lm_to_phys(addr)     (((addr) + physvirt_offset))
>>>  #define __kimg_to_phys(addr)   ((addr) - kimage_voffset)
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index d59ffabb9c84..5750370a7e8c 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -1451,8 +1451,7 @@ static bool inside_linear_region(u64 start, u64 size)
>>>          * address range mapped by the linear map, the start address should
>>>          * be calculated using vabits_actual.
>>>          */
>>> -       return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
>>> -                       && ((start + size) <= __pa(PAGE_END - 1)));
>>> +       return __is_lm_address(__va(start)) && __is_lm_address(__va(start + size));
>>>  }
>>>
>>>  int arch_add_memory(int nid, u64 start, u64 size,
>>
>> Will/Ard,
>>
>> Any thoughts about this ? __is_lm_address() now checks for a range instead
>> of a bit. This will be compatible later on, even if linear mapping range
>> changes from current lower half scheme.
>>
> 
> As I'm sure you have noticed, I sent out some patches that get rid of
> physvirt_offset, and which simplify __is_lm_address() to only take
> compile time constants into account (unless KASAN is enabled). This
> means that in the 52-bit VA case, __is_lm_address() does not
> distinguish between virtual addresses that can be mapped by the
> hardware and ones that cannot.

Yeah, though was bit late in getting to the series. So with that change
there might be areas in the linear mapping which cannot be addressed by
the hardware and hence should also need be checked apart from proposed
linear mapping coverage test, during memory hotplug ?

> 
> In the memory hotplug case, we need to decide whether the added memory
> will appear in the addressable area, which is a different question. So
> it makes sense to duplicate some of the logic that exists in
> arm64_memblock_init() (or factor it out) to decide whether this newly
> added memory will appear in the addressable window or not.

It seems unlikely that any hotplug agent (e.g. firmware) will ever push
through a memory range which is not accessible in the hardware but then
it is not impossible either. In summary, arch_add_memory() should check

1. Range can be covered inside linear mapping
2. Range is accessible by the hardware

Before the VA space organization series, (2) was not necessary as it was
contained inside (1) ?

> 
> So I think your original approach makes more sense here, although I
> think you want '(start + size - 1) <= __pa(PAGE_END - 1)' in the
> comparison above (and please drop the redundant parens)
> 

Sure, will accommodate these changes.

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Ard Biesheuvel]

On Wed, 14 Oct 2020 at 07:07, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
>
> On 10/12/2020 12:59 PM, Ard Biesheuvel wrote:
> > On Tue, 6 Oct 2020 at 08:36, Anshuman Khandual
> > <anshuman.khandual@arm.com> wrote:
> >>
> >>
> >>
> >> On 09/30/2020 01:32 PM, Anshuman Khandual wrote:
> >>> But if __is_lm_address() checks against the effective linear range instead
> >>> i.e [_PAGE_OFFSET(vabits_actual)..(PAGE_END - 1)], it can be used for hot
> >>> plug physical range check there after. Perhaps something like this, though
> >>> not tested properly.
> >>>
> >>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> >>> index afa722504bfd..6da046b479d4 100644
> >>> --- a/arch/arm64/include/asm/memory.h
> >>> +++ b/arch/arm64/include/asm/memory.h
> >>> @@ -238,7 +238,10 @@ static inline const void *__tag_set(const void *addr, u8 tag)
> >>>   * space. Testing the top bit for the start of the region is a
> >>>   * sufficient check and avoids having to worry about the tag.
> >>>   */
> >>> -#define __is_lm_address(addr)  (!(((u64)addr) & BIT(vabits_actual - 1)))
> >>> +static inline bool __is_lm_address(unsigned long addr)
> >>> +{
> >>> +       return ((addr >= _PAGE_OFFSET(vabits_actual)) && (addr <= (PAGE_END - 1)));
> >>> +}
> >>>
> >>>  #define __lm_to_phys(addr)     (((addr) + physvirt_offset))
> >>>  #define __kimg_to_phys(addr)   ((addr) - kimage_voffset)
> >>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> >>> index d59ffabb9c84..5750370a7e8c 100644
> >>> --- a/arch/arm64/mm/mmu.c
> >>> +++ b/arch/arm64/mm/mmu.c
> >>> @@ -1451,8 +1451,7 @@ static bool inside_linear_region(u64 start, u64 size)
> >>>          * address range mapped by the linear map, the start address should
> >>>          * be calculated using vabits_actual.
> >>>          */
> >>> -       return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> >>> -                       && ((start + size) <= __pa(PAGE_END - 1)));
> >>> +       return __is_lm_address(__va(start)) && __is_lm_address(__va(start + size));
> >>>  }
> >>>
> >>>  int arch_add_memory(int nid, u64 start, u64 size,
> >>
> >> Will/Ard,
> >>
> >> Any thoughts about this ? __is_lm_address() now checks for a range instead
> >> of a bit. This will be compatible later on, even if linear mapping range
> >> changes from current lower half scheme.
> >>
> >
> > As I'm sure you have noticed, I sent out some patches that get rid of
> > physvirt_offset, and which simplify __is_lm_address() to only take
> > compile time constants into account (unless KASAN is enabled). This
> > means that in the 52-bit VA case, __is_lm_address() does not
> > distinguish between virtual addresses that can be mapped by the
> > hardware and ones that cannot.
>
> Yeah, though was bit late in getting to the series. So with that change
> there might be areas in the linear mapping which cannot be addressed by
> the hardware and hence should also need be checked apart from proposed
> linear mapping coverage test, during memory hotplug ?
>

Yes.

> >
> > In the memory hotplug case, we need to decide whether the added memory
> > will appear in the addressable area, which is a different question. So
> > it makes sense to duplicate some of the logic that exists in
> > arm64_memblock_init() (or factor it out) to decide whether this newly
> > added memory will appear in the addressable window or not.
>
> It seems unlikely that any hotplug agent (e.g. firmware) will ever push
> through a memory range which is not accessible in the hardware but then
> it is not impossible either. In summary, arch_add_memory() should check
>
> 1. Range can be covered inside linear mapping
> 2. Range is accessible by the hardware
>
> Before the VA space organization series, (2) was not necessary as it was
> contained inside (1) ?
>

Not really. We have a problem with KASLR randomization of the linear
region, which may choose memstart_addr in such a way that we lose
access to regions that are beyond the boot time value of
memblock_end_of_DRAM().

I think we should probably rework that code to take
ID_AA64MMFR0_EL1.PARange into account instead.

> >
> > So I think your original approach makes more sense here, although I
> > think you want '(start + size - 1) <= __pa(PAGE_END - 1)' in the
> > comparison above (and please drop the redundant parens)
> >
>
> Sure, will accommodate these changes.

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[David Hildenbrand]

On 17.09.20 10:46, Anshuman Khandual wrote:
> During memory hotplug process, the linear mapping should not be created for
> a given memory range if that would fall outside the maximum allowed linear
> range. Else it might cause memory corruption in the kernel virtual space.
> 
> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
> both its ends but excluding PAGE_END. Max physical range that can be mapped
> inside this linear mapping range, must also be derived from its end points.
> 
> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
> assumption of 52 bits virtual address space. However, if the CPU does not
> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
> bits [51..48] are ignored by the MMU and remain unchanged, even though the
> effective start address of linear map is now slightly different. Hence, to
> reliably check the physical address range mapped by the linear map, the
> start address should be calculated using vabits_actual. This ensures that
> arch_add_memory() validates memory hot add range for its potential linear
> mapping requirement, before creating it with __create_pgd_mapping().
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Steven Price <steven.price@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 75df62fea1b6..d59ffabb9c84 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>  }
>  
> +static bool inside_linear_region(u64 start, u64 size)
> +{
> +	/*
> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
> +	 * accommodating both its ends but excluding PAGE_END. Max physical
> +	 * range which can be mapped inside this linear mapping range, must
> +	 * also be derived from its end points.
> +	 *
> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
> +	 * the assumption of 52 bits virtual address space. However, if the
> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
> +	 * unchanged, even though the effective start address of linear map
> +	 * is now slightly different. Hence, to reliably check the physical
> +	 * address range mapped by the linear map, the start address should
> +	 * be calculated using vabits_actual.
> +	 */
> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
> +			&& ((start + size) <= __pa(PAGE_END - 1)));
> +}
> +
>  int arch_add_memory(int nid, u64 start, u64 size,
>  		    struct mhp_params *params)
>  {
>  	int ret, flags = 0;
>  
> +	if (!inside_linear_region(start, size)) {
> +		pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
> +		return -EINVAL;
> +	}
> +
>  	if (rodata_full || debug_pagealloc_enabled())
>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
> 

Can we please provide a generic way to figure limits like that out,
especially, before calling add_memory() and friends?

We do have __add_pages()->check_hotplug_memory_addressable() where we
already check against MAX_PHYSMEM_BITS.

What I'd prefer is a way to get

1. Lower address limit we can use for add_memory() and friends
2. Upper address limit we can use for add_memory() and friends

something like


struct range memhp_get_addressable_range(void)
{
	const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
	struct range range = arch_get_mappable_range();

	if (range.start > max_phys) {
		range.start = 0;
		range.end = 0;
	}
	range.end = max_t(u64, range.end, max_phys);

	return range;
}


That, we can use in check_hotplug_memory_addressable(), and also allow
add_memory*() users to make use of it.

-- 
Thanks,

David / dhildenb

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 10/06/2020 09:04 PM, David Hildenbrand wrote:
> On 17.09.20 10:46, Anshuman Khandual wrote:
>> During memory hotplug process, the linear mapping should not be created for
>> a given memory range if that would fall outside the maximum allowed linear
>> range. Else it might cause memory corruption in the kernel virtual space.
>>
>> Maximum linear mapping region is [PAGE_OFFSET..(PAGE_END -1)] accommodating
>> both its ends but excluding PAGE_END. Max physical range that can be mapped
>> inside this linear mapping range, must also be derived from its end points.
>>
>> When CONFIG_ARM64_VA_BITS_52 is enabled, PAGE_OFFSET is computed with the
>> assumption of 52 bits virtual address space. However, if the CPU does not
>> support 52 bits, then it falls back using 48 bits instead and the PAGE_END
>> is updated to reflect this using the vabits_actual. As for PAGE_OFFSET,
>> bits [51..48] are ignored by the MMU and remain unchanged, even though the
>> effective start address of linear map is now slightly different. Hence, to
>> reliably check the physical address range mapped by the linear map, the
>> start address should be calculated using vabits_actual. This ensures that
>> arch_add_memory() validates memory hot add range for its potential linear
>> mapping requirement, before creating it with __create_pgd_mapping().
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: Steven Price <steven.price@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 75df62fea1b6..d59ffabb9c84 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1433,11 +1433,38 @@ static void __remove_pgd_mapping(pgd_t *pgdir, unsigned long start, u64 size)
>>  	free_empty_tables(start, end, PAGE_OFFSET, PAGE_END);
>>  }
>>  
>> +static bool inside_linear_region(u64 start, u64 size)
>> +{
>> +	/*
>> +	 * Linear mapping region is the range [PAGE_OFFSET..(PAGE_END - 1)]
>> +	 * accommodating both its ends but excluding PAGE_END. Max physical
>> +	 * range which can be mapped inside this linear mapping range, must
>> +	 * also be derived from its end points.
>> +	 *
>> +	 * With CONFIG_ARM64_VA_BITS_52 enabled, PAGE_OFFSET is defined with
>> +	 * the assumption of 52 bits virtual address space. However, if the
>> +	 * CPU does not support 52 bits, it falls back using 48 bits and the
>> +	 * PAGE_END is updated to reflect this using the vabits_actual. As
>> +	 * for PAGE_OFFSET, bits [51..48] are ignored by the MMU and remain
>> +	 * unchanged, even though the effective start address of linear map
>> +	 * is now slightly different. Hence, to reliably check the physical
>> +	 * address range mapped by the linear map, the start address should
>> +	 * be calculated using vabits_actual.
>> +	 */
>> +	return ((start >= __pa(_PAGE_OFFSET(vabits_actual)))
>> +			&& ((start + size) <= __pa(PAGE_END - 1)));
>> +}
>> +
>>  int arch_add_memory(int nid, u64 start, u64 size,
>>  		    struct mhp_params *params)
>>  {
>>  	int ret, flags = 0;
>>  
>> +	if (!inside_linear_region(start, size)) {
>> +		pr_err("[%llx %llx] is outside linear mapping region\n", start, start + size);
>> +		return -EINVAL;
>> +	}
>> +
>>  	if (rodata_full || debug_pagealloc_enabled())
>>  		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>  
>>
> 
> Can we please provide a generic way to figure limits like that out,
> especially, before calling add_memory() and friends?
> 
> We do have __add_pages()->check_hotplug_memory_addressable() where we
> already check against MAX_PHYSMEM_BITS.

Initially, I thought about check_hotplug_memory_addressable() but the
existing check that asserts end of hotplug wrt MAX_PHYSMEM_BITS, is
generic in nature. AFAIK the linear mapping problem is arm64 specific,
hence I was not sure whether to add an arch specific callback which
will give platform an opportunity to weigh in for these ranges.

But hold on, check_hotplug_memory_addressable() only gets called from
__add_pages() after linear mapping creation in arch_add_memory(). How
would it help ? We need some thing for add_memory(), its variants and
also possibly for memremap_pages() when it calls arch_add_memory().

> 
> What I'd prefer is a way to get
> 
> 1. Lower address limit we can use for add_memory() and friends
> 2. Upper address limit we can use for add_memory() and friends

A structure based range.

> 
> something like
> 
> 
> struct range memhp_get_addressable_range(void)
> {
> 	const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
> 	struct range range = arch_get_mappable_range();

What would you suggest as the default fallback range if a platform
does not define this callback.

> 
> 	if (range.start > max_phys) {
> 		range.start = 0;
> 		range.end = 0;
> 	}
> 	range.end = max_t(u64, range.end, max_phys);

min_t instead ?

> 
> 	return range;
> }
> 
> 
> That, we can use in check_hotplug_memory_addressable(), and also allow
> add_memory*() users to make use of it.

So this check would happen twice during a hotplug ?

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[David Hildenbrand]

>> We do have __add_pages()->check_hotplug_memory_addressable() where we
>> already check against MAX_PHYSMEM_BITS.
> 
> Initially, I thought about check_hotplug_memory_addressable() but the
> existing check that asserts end of hotplug wrt MAX_PHYSMEM_BITS, is
> generic in nature. AFAIK the linear mapping problem is arm64 specific,
> hence I was not sure whether to add an arch specific callback which
> will give platform an opportunity to weigh in for these ranges.

Also on s390x, the range where you can create an identity mapping depends on
- early kernel setup
- kasan

(I assume it's the same for all archs)

See arch/s390/mm/vmem.c:vmem_add_mapping(), which contains similar
checks (VMEM_MAX_PHYS).

> 
> But hold on, check_hotplug_memory_addressable() only gets called from
> __add_pages() after linear mapping creation in arch_add_memory(). How
> would it help ? We need some thing for add_memory(), its variants and
> also possibly for memremap_pages() when it calls arch_add_memory().
> 

Good point. We chose that place for simplicity when adding it (I was
favoring calling it at two places back then). Now, we might have good
reason to move the checks further up the call chain.

Most probably,

struct range memhp_get_addressable_range(bool need_mapping)
{
	...
}

Would make sense, to deal with memremap_pages() without identity mappings.

We have two options:

1. Generalize the checks, check early in applicable functions. Have a
single way to get applicable ranges, both in callers, and inside the
functions.

2. Keep the checks where they are. Add memhp_get_addressable_range() so
callers can figure limits out. It's less clear what the relation between
the different checks is. And it's likely if things change at one place
that we miss the other place.

>> struct range memhp_get_addressable_range(void)
>> {
>> 	const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
>> 	struct range range = arch_get_mappable_range();
> 
> What would you suggest as the default fallback range if a platform
> does not define this callback.

Just the largest possible range until we implement them. IIRC, an s390x
version should be easy to add.

> 
>>
>> 	if (range.start > max_phys) {
>> 		range.start = 0;
>> 		range.end = 0;
>> 	}
>> 	range.end = max_t(u64, range.end, max_phys);
> 
> min_t instead ?

Yeah :)

> 
>>
>> 	return range;
>> }
>>
>>
>> That, we can use in check_hotplug_memory_addressable(), and also allow
>> add_memory*() users to make use of it.
> 
> So this check would happen twice during a hotplug ?

Right now it's like calling a function with wrong arguments - you just
don't have a clue what valid arguments are, because non-obvious errors
(besides -ENOMEM, which is a temporary error) pop up deep down the call
chain.

For example, virito-mem would use it to detect during device
initialization the usable device range, and warn the user accordingly.
It currently manually checks for MAX_PHYSMEM_BITS, but that's just ugly.
Failing at random add_memory() calls (permanently!) is not so nice.

In case of DIMMs, we could use it to detect if adding parts of a DIMM
won't work (and warn the user early). We could try to add as much as
possible.

-- 
Thanks,

David / dhildenb

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[Anshuman Khandual]

On 10/07/2020 02:09 PM, David Hildenbrand wrote:
>>> We do have __add_pages()->check_hotplug_memory_addressable() where we
>>> already check against MAX_PHYSMEM_BITS.
>>
>> Initially, I thought about check_hotplug_memory_addressable() but the
>> existing check that asserts end of hotplug wrt MAX_PHYSMEM_BITS, is
>> generic in nature. AFAIK the linear mapping problem is arm64 specific,
>> hence I was not sure whether to add an arch specific callback which
>> will give platform an opportunity to weigh in for these ranges.
> 
> Also on s390x, the range where you can create an identity mapping depends on
> - early kernel setup
> - kasan
> 
> (I assume it's the same for all archs)
> 
> See arch/s390/mm/vmem.c:vmem_add_mapping(), which contains similar
> checks (VMEM_MAX_PHYS).

Once there is a high level function, all these platform specific
checks should go in their arch_get_mappable_range() instead.

> 
>>
>> But hold on, check_hotplug_memory_addressable() only gets called from
>> __add_pages() after linear mapping creation in arch_add_memory(). How
>> would it help ? We need some thing for add_memory(), its variants and
>> also possibly for memremap_pages() when it calls arch_add_memory().
>>
> 
> Good point. We chose that place for simplicity when adding it (I was
> favoring calling it at two places back then). Now, we might have good
> reason to move the checks further up the call chain.

check_hotplug_memory_addressable() check in add_pages() does not add
much as linear mapping creation must have been completed by then. I
guess moving this check inside the single high level function should
be better.

But checking against MAX_PHYSMEM_BITS might no longer be required, as
the range would have been validated against applicable memhp_range.   

> 
> Most probably,
> 
> struct range memhp_get_addressable_range(bool need_mapping)
> {
> 	...
> }

Something like this...

+struct memhp_range {
+       u64 start;
+       u64 end;
+};
+
+#ifndef arch_get_addressable_range
+static inline struct memhp_range arch_get_mappable_range(bool need_mapping)
+{
+       struct memhp_range range = {
+               .start = 0UL,
+               .end = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1,
+       };
+       return range;
+}
+#endif
+
+static inline struct memhp_range memhp_get_mappable_range(bool need_mapping)
+{
+       const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
+       struct memhp_range range = arch_get_mappable_range(need_mapping);
+
+       if (range.start > max_phys) {
+               range.start = 0;
+               range.end = 0;
+       }
+       range.end = min_t(u64, range.end, max_phys);
+       return range;
+}
+
+static inline bool memhp_range_allowed(u64 start, u64 end, bool need_mapping)
+{
+       struct memhp_range range = memhp_get_mappable_range(need_mapping);
+
+       return (start <= end) && (start >= range.start) && (end <= range.end);
+}

> 
> Would make sense, to deal with memremap_pages() without identity mappings.
> 
> We have two options:
> 
> 1. Generalize the checks, check early in applicable functions. Have a
> single way to get applicable ranges, both in callers, and inside the
> functions.
Inside the functions, check_hotplug_memory_addressable() in add_pages() ?
We could just drop that. Single generalized check with an arch callback
makes more sense IMHO.

> 
> 2. Keep the checks where they are. Add memhp_get_addressable_range() so
> callers can figure limits out. It's less clear what the relation between
> the different checks is. And it's likely if things change at one place
> that we miss the other place.

Right, does not sound like a good idea :)

> 
>>> struct range memhp_get_addressable_range(void)
>>> {
>>> 	const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
>>> 	struct range range = arch_get_mappable_range();
>>
>> What would you suggest as the default fallback range if a platform
>> does not define this callback.
> 
> Just the largest possible range until we implement them. IIRC, an s390x
> version should be easy to add.

[0UL...(1ull << (MAX_PHYSMEM_BITS + 1)) - 1] is the largest possible
hotplug range.

> 
>>
>>>
>>> 	if (range.start > max_phys) {
>>> 		range.start = 0;
>>> 		range.end = 0;
>>> 	}
>>> 	range.end = max_t(u64, range.end, max_phys);
>>
>> min_t instead ?
> 
> Yeah :)
> 
>>
>>>
>>> 	return range;
>>> }
>>>
>>>
>>> That, we can use in check_hotplug_memory_addressable(), and also allow
>>> add_memory*() users to make use of it.
>>
>> So this check would happen twice during a hotplug ?
> 
> Right now it's like calling a function with wrong arguments - you just
> don't have a clue what valid arguments are, because non-obvious errors
> (besides -ENOMEM, which is a temporary error) pop up deep down the call
> chain.
> 
> For example, virito-mem would use it to detect during device
> initialization the usable device range, and warn the user accordingly.
> It currently manually checks for MAX_PHYSMEM_BITS, but that's just ugly.
> Failing at random add_memory() calls (permanently!) is not so nice.
> 
> In case of DIMMs, we could use it to detect if adding parts of a DIMM
> won't work (and warn the user early). We could try to add as much as
> possible.

Agreed.

Planning to add memhp_range_allowed() check in add_memory(), __add_memory(),
add_memory_driver_managed() and memremap_pages(). This check might just get
called twice depending on the hotplug path. Wondering if this needs to be
added any where else ?

Re: [PATCH] arm64/mm: Validate hotplug range before creating linear mapping

[David Hildenbrand]

>>
>> Most probably,
>>
>> struct range memhp_get_addressable_range(bool need_mapping)
>> {
>> 	...
>> }
> 
> Something like this...
> 
> +struct memhp_range {
> +       u64 start;
> +       u64 end;
> +};

We do have struct range already in include/linux/range.h

> +
> +#ifndef arch_get_addressable_range
> +static inline struct memhp_range arch_get_mappable_range(bool need_mapping)
> +{
> +       struct memhp_range range = {
> +               .start = 0UL,
> +               .end = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1,

Or just set to -1ULL if it's only used in memhp_get_mappable_range(), to
keep things simple ().

> +       };
> +       return range;
> +}
> +#endif
> +
> +static inline struct memhp_range memhp_get_mappable_range(bool need_mapping)

due to "need_mapping" the function might better be called

memhp_get_pluggable_range()

or similar

> +{
> +       const u64 max_phys = (1ull << (MAX_PHYSMEM_BITS + 1)) - 1;
> +       struct memhp_range range = arch_get_mappable_range(need_mapping);
> +
> +       if (range.start > max_phys) {
> +               range.start = 0;
> +               range.end = 0;
> +       }
> +       range.end = min_t(u64, range.end, max_phys);
> +       return range;
> +}
> +
> +static inline bool memhp_range_allowed(u64 start, u64 end, bool need_mapping)
> +{
> +       struct memhp_range range = memhp_get_mappable_range(need_mapping);
> +
> +       return (start <= end) && (start >= range.start) && (end <= range.end);

Keep in mind that in memory hotplug code, "end" is usually exclusive,
and "end" in "struct range" is inclusive (see range_len(), or how you
calculate max_phys.

So depending on the semantics, you might have to fixup your comparisons.

return start < end && start >= range.start && end <= range.end - 1;


[...]

>> Right now it's like calling a function with wrong arguments - you just
>> don't have a clue what valid arguments are, because non-obvious errors
>> (besides -ENOMEM, which is a temporary error) pop up deep down the call
>> chain.
>>
>> For example, virito-mem would use it to detect during device
>> initialization the usable device range, and warn the user accordingly.
>> It currently manually checks for MAX_PHYSMEM_BITS, but that's just ugly.
>> Failing at random add_memory() calls (permanently!) is not so nice.
>>
>> In case of DIMMs, we could use it to detect if adding parts of a DIMM
>> won't work (and warn the user early). We could try to add as much as
>> possible.
> 
> Agreed.
> 
> Planning to add memhp_range_allowed() check in add_memory(), __add_memory(),
> add_memory_driver_managed() and memremap_pages(). This check might just get
> called twice depending on the hotplug path. Wondering if this needs to be
> added any where else ?

So

add_memory() needs to
- add sections via arch_add_memory()
- create a mapping via arch_add_memory()->add_pages()

memremap_pages() via arch_add_memory() needs to
- add sections via arch_add_memory()
- create a mapping via arch_add_memory()->add_pages()

memremap_pages() via add_pages() needs to
- add sections

I'll reuse the functions from virtio-mem code once in place (exposing
memhp_get_pluggable_range()).


I do agree that having the callers of arch_add_memory() / add_pages()
validate stuff isn't completely nice. I already raised that I would much
rather want to see !arch wrappers for these arch functions that could
validate stuff. But then we would have to do a bigger cleanup to get
naming right.

1. Rename functions for handling system ram like

s/add_memory/add_sysram/
s/remove_memory/remove_sysram/
...

2. Have a new add_memory() that validates + calls arch_add_memory()

3. s/add_pages/arch_add_pages/

4. Have a new add_pages() that validates + calls arch_add_pages()

...


Long story short, handling it in the 2 (!) callers might be easier for now.

-- 
Thanks,

David / dhildenb