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

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

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