Re: [PATCH v3 05/14] x86: Reset tag for virtual to physical address conversions

[Dave Hansen]

On 4/4/25 06:14, Maciej Wieczor-Retman wrote:
> +#ifdef CONFIG_KASAN_SW_TAGS
> +#define page_to_virt(x)	({									\
> +	__typeof__(x) __page = x;								\
> +	void *__addr = __va(page_to_pfn((__typeof__(x))__tag_reset(__page)) << PAGE_SHIFT);	\
> +	(void *)__tag_set((const void *)__addr, page_kasan_tag(__page));			\
> +})
> +#endif

Is this #ifdef needed?

I thought there were stub versions of all of those tag functions. So it
should be harmless to use this page_to_virt() implementation with or
without KASAN. Right?

I'm also confused by the implementation. This is one reason why I rather
dislike macros. Why does this act like the type of 'x' is variable?
Isn't it always a 'struct page *'? If so, then why all of the
__typeof__()'s?

Are struct page pointers _ever_ tagged? If they are, then doesn't
page_to_pfn() need to handle untagging as well? If they aren't, then
there's no reason to __tag_reset() in here.

What was the thinking behind this cast:

	(const void *)__addr

?

Are any of these casts _doing_ anything? I'm struggling to find anything
wrong with:

#define page_to_virt(x)	({													
	void *__addr = __va(page_to_pfn(__page) << PAGE_SHIFT);
	__tag_set(__addr, page_kasan_tag(x))
})

... which made me look back at:

	static inline const void *__tag_set(const void *addr, u8 tag)

from patch 3. I don't think the 'const' makes any sense on the return
value here. Surely the memory pointed at by a tagged pointer doesn't
need to be const. Why should the tag setting function be returning a
const pointer?

I can see why it would *take* a const pointer since it's not modifying
the memory, but I don't see why it is returning one.