Re: [PATCH v9 05/13] x86/mm: Reset tag for virtual to physical address conversions

[Maciej Wieczor-Retman]

On 2026-02-23 at 12:33:01 -0800, Dave Hansen wrote:
>>  #ifdef CONFIG_X86_64
>>  #include <asm/page_64.h>
>> @@ -65,6 +66,13 @@ 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.
>>   */
>> +
>> +#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
>
>Can we pretty please keep this in arch-independent code?
>
>The idea of tags is not x86-specific and I can almost guarantee that x86
>won't be the last one needing this.

Do you mean adding a KASAN oriented page_to_virt() that's arch independent? I'd
guess next to the regular one, in include/linux/mm.h?

arm64's pretty similar so I suppose it should work.

>On to the rest...
>
>> 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.
>
>Nit: there's no "macro" for the rest. They're functions. I also don't
>_care_ how __tag_reset() works here. Don't explain implementation details.

Okay, I'll rewrite it.

>> index 2f0e47be79a4..01f9e6233bba 100644
>> --- a/arch/x86/include/asm/page_64.h
>> +++ b/arch/x86/include/asm/page_64.h
>> @@ -22,6 +22,7 @@ extern unsigned long direct_map_physmem_end;
>>  
>>  static __always_inline unsigned long __phys_addr_nodebug(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 */
>> diff --git a/arch/x86/mm/physaddr.c b/arch/x86/mm/physaddr.c
>> index 8d31c6b9e184..8f18273be0d2 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;
>
>I know all the virt-to/from-phys functions are a mess. But could we
>please take a wee peek here at refactoring them so this doesn't need to
>be done *twice*?
>
>I also think the changelog here needs to include something about the
>idea that arbitrary kernel pointers are assumed to be tagged and that
>these functions mostly accept those arbitrary pointers.
>

Okay, I'll add that in.

>
>> @@ -35,6 +36,7 @@ EXPORT_SYMBOL(__phys_addr);
>>  
>>  bool __virt_addr_valid(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 */
>
>It also occurs to me that having a helper that does:
>
>	x - __START_KERNEL_map;
>
>it could do the tag reset, which might consolidate all of these sites.

Thanks! It's a good idea, I'll do that.

-- 
Kind regards
Maciej Wieczór-Retman