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

[Dave Hansen <dave.hansen@intel.com>]

>  #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.

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.

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


> @@ -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.