[PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[Aneesh Kumar K.V]

Instead of high_memory use is_vmalloc_addr to validate that the address is
not in the vmalloc range.

Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
-
---
 arch/powerpc/include/asm/page.h | 10 ++++------
 arch/powerpc/mm/mem.c           | 11 +++++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index e5f75c70eda8..977835570db3 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn)
 #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
 #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
-
-#define virt_addr_valid(vaddr)	({					\
-	unsigned long _addr = (unsigned long)vaddr;			\
-	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
-	pfn_valid(virt_to_pfn(_addr));					\
-})
+#ifndef __ASSEMBLY__
+extern bool __virt_addr_valid(unsigned long kaddr);
+#endif
+#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
 
 /*
  * On Book-E parts we need __va to parse the device tree and we can't
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 7b0d286bf9ba..622f8bac808b 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn)
  * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed.
  */
 EXPORT_SYMBOL_GPL(walk_system_ram_range);
+
+bool __virt_addr_valid(unsigned long kaddr)
+{
+	if (kaddr < PAGE_OFFSET)
+		return false;
+	if (is_vmalloc_addr((void *) kaddr))
+		return false;
+	return pfn_valid(virt_to_pfn(kaddr));
+}
+EXPORT_SYMBOL(__virt_addr_valid);
+
-- 
2.36.1

Re: [PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[Christophe Leroy]

Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit :
> Instead of high_memory use is_vmalloc_addr to validate that the address is
> not in the vmalloc range.


Do we really need even more extra checks, and a function that is not 
inlined anymore ?

virt_addr_valid() used to be pretty simple. Some extra tests were added 
by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit 
Book3E & 32-bit") in order to work around some corner cases, and the 
commit message say they are temporary.

virt_addr_valid() is there to check that an address is a valid linear 
mapping, not that an address IS NOT a vmalloc address. What will happen 
with your check if you pass an address that is from an ioremap done 
prior to the start of the vmalloc system ?

WIth the series I send last week to add KASAN to book3e/64, we now have 
VMALLOC above PAGE_OFFSET on all platforms so we should be able to come 
back to the original virt_addr_valid(), based exclusively on pfn_valid() 
for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for 
PPC32 having HIGHMEM)


Christophe


> 
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> -
> ---
>   arch/powerpc/include/asm/page.h | 10 ++++------
>   arch/powerpc/mm/mem.c           | 11 +++++++++++
>   2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index e5f75c70eda8..977835570db3 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn)
>   #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>   #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
>   #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
> -
> -#define virt_addr_valid(vaddr)	({					\
> -	unsigned long _addr = (unsigned long)vaddr;			\
> -	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
> -	pfn_valid(virt_to_pfn(_addr));					\
> -})
> +#ifndef __ASSEMBLY__
> +extern bool __virt_addr_valid(unsigned long kaddr);
> +#endif
> +#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
>   
>   /*
>    * On Book-E parts we need __va to parse the device tree and we can't
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 7b0d286bf9ba..622f8bac808b 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn)
>    * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed.
>    */
>   EXPORT_SYMBOL_GPL(walk_system_ram_range);
> +
> +bool __virt_addr_valid(unsigned long kaddr)
> +{
> +	if (kaddr < PAGE_OFFSET)
> +		return false;
> +	if (is_vmalloc_addr((void *) kaddr))
> +		return false;
> +	return pfn_valid(virt_to_pfn(kaddr));
> +}
> +EXPORT_SYMBOL(__virt_addr_valid);
> +

Re: [PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[Aneesh Kumar K V]

On 7/4/22 12:43 PM, Christophe Leroy wrote:
> 
> 
> Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit :
>> Instead of high_memory use is_vmalloc_addr to validate that the address is
>> not in the vmalloc range.
> 
> 
> Do we really need even more extra checks, and a function that is not 
> inlined anymore ?
> 
> virt_addr_valid() used to be pretty simple. Some extra tests were added 
> by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit 
> Book3E & 32-bit") in order to work around some corner cases, and the 
> commit message say they are temporary.
> 
> virt_addr_valid() is there to check that an address is a valid linear 
> mapping, not that an address IS NOT a vmalloc address. What will happen 
> with your check if you pass an address that is from an ioremap done 
> prior to the start of the vmalloc system ?
> 

I was expecting the io range to be handled by pfn_valid(). IS there a memory layout
ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal with the
change was to make it more explicit what is it being validated. 

> WIth the series I send last week to add KASAN to book3e/64, we now have 
> VMALLOC above PAGE_OFFSET on all platforms so we should be able to come 
> back to the original virt_addr_valid(), based exclusively on pfn_valid() 
> for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for 
> PPC32 having HIGHMEM)
> 
> 
> Christophe
> 
> 
>>
>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> -
>> ---
>>   arch/powerpc/include/asm/page.h | 10 ++++------
>>   arch/powerpc/mm/mem.c           | 11 +++++++++++
>>   2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index e5f75c70eda8..977835570db3 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn)
>>   #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>>   #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
>>   #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
>> -
>> -#define virt_addr_valid(vaddr)	({					\
>> -	unsigned long _addr = (unsigned long)vaddr;			\
>> -	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
>> -	pfn_valid(virt_to_pfn(_addr));					\
>> -})
>> +#ifndef __ASSEMBLY__
>> +extern bool __virt_addr_valid(unsigned long kaddr);
>> +#endif
>> +#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
>>   
>>   /*
>>    * On Book-E parts we need __va to parse the device tree and we can't
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 7b0d286bf9ba..622f8bac808b 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn)
>>    * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed.
>>    */
>>   EXPORT_SYMBOL_GPL(walk_system_ram_range);
>> +
>> +bool __virt_addr_valid(unsigned long kaddr)
>> +{
>> +	if (kaddr < PAGE_OFFSET)
>> +		return false;
>> +	if (is_vmalloc_addr((void *) kaddr))
>> +		return false;
>> +	return pfn_valid(virt_to_pfn(kaddr));
>> +}
>> +EXPORT_SYMBOL(__virt_addr_valid);
>> +

Re: [PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[Christophe Leroy]

Le 04/07/2022 à 09:45, Aneesh Kumar K V a écrit :
> On 7/4/22 12:43 PM, Christophe Leroy wrote:
>>
>>
>> Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit :
>>> Instead of high_memory use is_vmalloc_addr to validate that the address is
>>> not in the vmalloc range.
>>
>>
>> Do we really need even more extra checks, and a function that is not
>> inlined anymore ?
>>
>> virt_addr_valid() used to be pretty simple. Some extra tests were added
>> by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit
>> Book3E & 32-bit") in order to work around some corner cases, and the
>> commit message say they are temporary.
>>
>> virt_addr_valid() is there to check that an address is a valid linear
>> mapping, not that an address IS NOT a vmalloc address. What will happen
>> with your check if you pass an address that is from an ioremap done
>> prior to the start of the vmalloc system ?
>>
> 
> I was expecting the io range to be handled by pfn_valid(). IS there a memory layout
> ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal with the
> change was to make it more explicit what is it being validated.


Yes you are right it should be handled by pfn_valid(), just like the 
entire VMALLOC range indeed. But on PPC32 a valid pfn might hit above 
vmalloc space as well.

You can find the new layout here : 
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=c7b9ed7c34a9f5dbf8222d63e3e313cef9f3150b

The only problem we have with pfn_valid() is for PPC32 because 
pfn_valid() also include highmem memory. That's the reason why we need 
to check that the address is below high_memory in addition.

For everything else, pfn_valid() should be enough.

For PPC64, we may want to add a verification that we are in the 0xc.... 
range, because of the way __pa/__va work. On PPC32 that's not needed.


> 
>> WIth the series I send last week to add KASAN to book3e/64, we now have
>> VMALLOC above PAGE_OFFSET on all platforms so we should be able to come
>> back to the original virt_addr_valid(), based exclusively on pfn_valid()
>> for PPC64, and pfn_valid() && high_memory for PPC32 (Or maybe only for
>> PPC32 having HIGHMEM)
>>
>>
>> Christophe
>>
>>
>>>
>>> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> -
>>> ---
>>>    arch/powerpc/include/asm/page.h | 10 ++++------
>>>    arch/powerpc/mm/mem.c           | 11 +++++++++++
>>>    2 files changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>>> index e5f75c70eda8..977835570db3 100644
>>> --- a/arch/powerpc/include/asm/page.h
>>> +++ b/arch/powerpc/include/asm/page.h
>>> @@ -131,12 +131,10 @@ static inline bool pfn_valid(unsigned long pfn)
>>>    #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>>>    #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
>>>    #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
>>> -
>>> -#define virt_addr_valid(vaddr)	({					\
>>> -	unsigned long _addr = (unsigned long)vaddr;			\
>>> -	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
>>> -	pfn_valid(virt_to_pfn(_addr));					\
>>> -})
>>> +#ifndef __ASSEMBLY__
>>> +extern bool __virt_addr_valid(unsigned long kaddr);
>>> +#endif
>>> +#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))
>>>    
>>>    /*
>>>     * On Book-E parts we need __va to parse the device tree and we can't
>>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>>> index 7b0d286bf9ba..622f8bac808b 100644
>>> --- a/arch/powerpc/mm/mem.c
>>> +++ b/arch/powerpc/mm/mem.c
>>> @@ -406,3 +406,14 @@ int devmem_is_allowed(unsigned long pfn)
>>>     * the EHEA driver. Drop this when drivers/net/ethernet/ibm/ehea is removed.
>>>     */
>>>    EXPORT_SYMBOL_GPL(walk_system_ram_range);
>>> +
>>> +bool __virt_addr_valid(unsigned long kaddr)
>>> +{
>>> +	if (kaddr < PAGE_OFFSET)
>>> +		return false;
>>> +	if (is_vmalloc_addr((void *) kaddr))
>>> +		return false;
>>> +	return pfn_valid(virt_to_pfn(kaddr));
>>> +}
>>> +EXPORT_SYMBOL(__virt_addr_valid);
>>> +
> 

Re: [PATCH v2] powerpc/mm: Use is_vmalloc_addr to validate addr

[Christophe Leroy]

Le 04/07/2022 à 09:55, Christophe Leroy a écrit :
> 
> 
> Le 04/07/2022 à 09:45, Aneesh Kumar K V a écrit :
>> On 7/4/22 12:43 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 04/07/2022 à 08:39, Aneesh Kumar K.V a écrit :
>>>> Instead of high_memory use is_vmalloc_addr to validate that the 
>>>> address is
>>>> not in the vmalloc range.
>>>
>>>
>>> Do we really need even more extra checks, and a function that is not
>>> inlined anymore ?
>>>
>>> virt_addr_valid() used to be pretty simple. Some extra tests were added
>>> by commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit
>>> Book3E & 32-bit") in order to work around some corner cases, and the
>>> commit message say they are temporary.
>>>
>>> virt_addr_valid() is there to check that an address is a valid linear
>>> mapping, not that an address IS NOT a vmalloc address. What will happen
>>> with your check if you pass an address that is from an ioremap done
>>> prior to the start of the vmalloc system ?
>>>
>>
>> I was expecting the io range to be handled by pfn_valid(). IS there a 
>> memory layout
>> ascii diagram of book3e/64 like asm/book3s/64/radix.h:51 ? My goal 
>> with the
>> change was to make it more explicit what is it being validated.
> 
> 
> Yes you are right it should be handled by pfn_valid(), just like the 
> entire VMALLOC range indeed. But on PPC32 a valid pfn might hit above 
> vmalloc space as well.
> 
> You can find the new layout here : 
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=c7b9ed7c34a9f5dbf8222d63e3e313cef9f3150b 
> 
> 
> The only problem we have with pfn_valid() is for PPC32 because 
> pfn_valid() also include highmem memory. That's the reason why we need 
> to check that the address is below high_memory in addition.
> 
> For everything else, pfn_valid() should be enough.
> 
> For PPC64, we may want to add a verification that we are in the 0xc.... 
> range, because of the way __pa/__va work. On PPC32 that's not needed.
> 

So, I would do something like that:

static __always_inline bool __virt_addr_valid(unsigned long addr)
{
	if (IS_ENABLED(CONFIG_PPC64) && lm_alias(addr) != addr)
		return false;
	if (IS_ENABLED(CONFIG_HIGHMEM) && addr >= (unsigned long)high_memory)
		return false;
	return pfn_valid(virt_to_pfn(addr));
}

#define virt_addr_valid(kaddr)	__virt_addr_valid((unsigned long) (kaddr))