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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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