Re: [PATCH 4/5] powerpc/64: Add VIRTUAL_BUG_ON checks for __va and __pa addresses

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

Le 25/12/2021 à 11:10, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of December 24, 2021 11:24 pm:
>> Hi Nic,
>>
>> Le 24/07/2019 à 10:46, Nicholas Piggin a écrit :
>>> Ensure __va is given a physical address below PAGE_OFFSET, and __pa is
>>> given a virtual address above PAGE_OFFSET.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    arch/powerpc/include/asm/page.h | 14 ++++++++++++--
>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>>> index 0d52f57fca04..c8bb14ff4713 100644
>>> --- a/arch/powerpc/include/asm/page.h
>>> +++ b/arch/powerpc/include/asm/page.h
>>> @@ -215,9 +215,19 @@ static inline bool pfn_valid(unsigned long pfn)
>>>    /*
>>>     * gcc miscompiles (unsigned long)(&static_var) - PAGE_OFFSET
>>>     * with -mcmodel=medium, so we use & and | instead of - and + on 64-bit.
>>> + * This also results in better code generation.
>>>     */
>>> -#define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET))
>>> -#define __pa(x) ((unsigned long)(x) & 0x0fffffffffffffffUL)
>>> +#define __va(x)								\
>>> +({									\
>>> +	VIRTUAL_BUG_ON((unsigned long)(x) >= PAGE_OFFSET);		\
>>> +	(void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET);	\
>>> +})
>>> +
>>> +#define __pa(x)								\
>>> +({									\
>>> +	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
>>
>> With this, it is likely that virt_addr_valid() BUGs on a non valid address.
>>
>> I think the purpose of virt_addr_valid() is to check addresses
>> seamlessly, see check_heap_object()
> 
> Looks like you're right. How did you catch that?

I caught that while looking at the problem reported by Kefeng where he 
says that virt_addr_valid() reports true on vmalloced memory on book3e/64


> 
> We could change virt_addr_valid() to make that test first. x86 and arm64
> both do checking rather than relying on !pfn_valid for < PAGE_OFFSET
> addresses.

That should work.

Maybe also we should implement a __pa_nodebug() like x86 and arm64 ?

Christophe