Re: [PATCH v10 09/13] x86/mm: Reset tags in a canonical address helper call

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

On 2/24/26 00:57, Maciej Wieczor-Retman wrote:
> On 2026-02-23 at 12:39:22 -0800, Dave Hansen wrote:
>> On 2/4/26 11:20, Maciej Wieczor-Retman wrote:
>>> -	return __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
>>> +	return __is_canonical_address(__tag_reset(vaddr), boot_cpu_data.x86_virt_bits);
>>>  }
>> I think we should just do this in __canonical_address() itself instead
>> of changing its callers. There are a whole host of ways to get in there.
> Cc: Sean Christopherson <seanjc@google.com>
> 
> I tried doing that before and Sean said it'd be wrong from KVM perspective [1].
> Not sure if there is some other way to do it so that it makes sense in KVM?

Looking at this some more, I think it's fine to leave
__is_canonical_address() alone. There are several kinds of canonical
checks and __is_canonical_address() should help with all of them.

That said, this patch is still buggy. copy_from_kernel_nofault_allowed()
does:

        if (is_vsyscall_vaddr(vaddr))
                return false;

which is a plain equality check:

	static inline bool is_vsyscall_vaddr(unsigned long vaddr)
	{
	        return unlikely((vaddr & PAGE_MASK) == VSYSCALL_ADDR);
	}

So a tagged 'vaddr' is not going to ever be is_vsyscall_vaddr(). The
untagging needs to happen before this check. Further, I do wonder if we
want checks in things like is_vsyscall_vaddr() for tagged addresses. If
they get into there, it's always a bug I think.

Aside: I think copy_from_kernel_nofault_allowed() is buggy. It's using
'boot_cpu_data.x86_virt_bits' which I think is the "CPU canonical" bits,
not the "paging canonical" bits. So a 5-level hardware system running in
4-level mode can pass non-paging-canonical address through
copy_from_kernel_nofault_allowed(). So I think the
'boot_cpu_data.x86_virt_bits' use in here became a buglet when
5-level-paging got enabled.

In a perfect world, we'd stop using EX_TYPE_UACCESS for kernel accesses.
But the uaccess.h macros are enough of a mess that  I understand why
nobody wanted to plumb a new exception type into them and wanted to
reuse them as-is.