Hi and thanks for looking at the patches :)
On 2025-08-14 at 10:15:09 +0300, Mike Rapoport wrote:
>On Tue, Aug 12, 2025 at 03:23:42PM +0200, Maciej Wieczor-Retman wrote:
>> Any place where pointer arithmetic is used to convert a virtual address
>> into a physical one can raise errors if the virtual address is tagged.
>>
>> Reset the pointer's tag by sign extending the tag bits in macros that do
>> pointer arithmetic in address conversions. There will be no change in
>> compiled code with KASAN disabled since the compiler will optimize the
>> __tag_reset() out.
>>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>> Changelog v4:
>> - Simplify page_to_virt() by removing pointless casts.
>> - Remove change in __is_canonical_address() because it's taken care of
>> in a later patch due to a LAM compatible definition of canonical.
>>
>> arch/x86/include/asm/page.h | 14 +++++++++++---
>> arch/x86/include/asm/page_64.h | 2 +-
>> arch/x86/mm/physaddr.c | 1 +
>> 3 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/page.h b/arch/x86/include/asm/page.h
>> index 9265f2fca99a..15c95e96fd15 100644
>> --- a/arch/x86/include/asm/page.h
>> +++ b/arch/x86/include/asm/page.h
>> @@ -7,6 +7,7 @@
>> #ifdef __KERNEL__
>>
>> #include <asm/page_types.h>
>> +#include <asm/kasan.h>
>>
>> #ifdef CONFIG_X86_64
>> #include <asm/page_64.h>
>> @@ -41,7 +42,7 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
>> #define __pa(x) __phys_addr((unsigned long)(x))
>> #endif
>>
>> -#define __pa_nodebug(x) __phys_addr_nodebug((unsigned long)(x))
>> +#define __pa_nodebug(x) __phys_addr_nodebug((unsigned long)(__tag_reset(x)))
>
>Why not reset the tag inside __phys_addr_nodebug() and __phys_addr()?
Right, this should be one less line in the changelog and no behavior changes.
I'll fix it.
>
>> /* __pa_symbol should be used for C visible symbols.
>> This seems to be the official gcc blessed way to do such arithmetic. */
>> /*
>> @@ -65,9 +66,16 @@ static inline void copy_user_page(void *to, void *from, unsigned long vaddr,
>> * virt_to_page(kaddr) returns a valid pointer if and only if
>> * virt_addr_valid(kaddr) returns true.
>> */
>> -#define virt_to_page(kaddr) pfn_to_page(__pa(kaddr) >> PAGE_SHIFT)
>> +
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +#define page_to_virt(x) ({ \
>> + void *__addr = __va(page_to_pfn((struct page *)x) << PAGE_SHIFT); \
>> + __tag_set(__addr, page_kasan_tag(x)); \
>> +})
>> +#endif
>> +#define virt_to_page(kaddr) pfn_to_page(__pa((void *)__tag_reset(kaddr)) >> PAGE_SHIFT)
>
>then virt_to_page() will remain the same, no?
Oh, yes, that is redundant with __pa() resetting the tag. Thanks!
>
>> extern bool __virt_addr_valid(unsigned long kaddr);
>> -#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long) (kaddr))
>> +#define virt_addr_valid(kaddr) __virt_addr_valid((unsigned long)(__tag_reset(kaddr)))
>
>The same here, I think tag_reset() should be inside __virt_addr_valid()
Sure, that does sound better.
>
>> static __always_inline void *pfn_to_kaddr(unsigned long pfn)
>> {
>> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
>> index 015d23f3e01f..de68ac40dba2 100644
>> --- a/arch/x86/include/asm/page_64.h
>> +++ b/arch/x86/include/asm/page_64.h
>> @@ -33,7 +33,7 @@ static __always_inline unsigned long __phys_addr_nodebug(unsigned long x)
>> extern unsigned long __phys_addr(unsigned long);
>> extern unsigned long __phys_addr_symbol(unsigned long);
>> #else
>> -#define __phys_addr(x) __phys_addr_nodebug(x)
>> +#define __phys_addr(x) __phys_addr_nodebug(__tag_reset(x))
>> #define __phys_addr_symbol(x) \
>> ((unsigned long)(x) - __START_KERNEL_map + phys_base)
>> #endif
>> diff --git a/arch/x86/mm/physaddr.c b/arch/x86/mm/physaddr.c
>> index fc3f3d3e2ef2..7f2b11308245 100644
>> --- a/arch/x86/mm/physaddr.c
>> +++ b/arch/x86/mm/physaddr.c
>> @@ -14,6 +14,7 @@
>> #ifdef CONFIG_DEBUG_VIRTUAL
>> unsigned long __phys_addr(unsigned long x)
>> {
>> + x = __tag_reset(x);
>> unsigned long y = x - __START_KERNEL_map;
>>
>> /* use the carry flag to determine if x was < __START_KERNEL_map */
>> --
>> 2.50.1
>>
>
>--
>Sincerely yours,
>Mike.
--
Kind regards
Maciej Wieczór-Retman
