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

[Maciej Wieczor-Retman]

On 2026-02-26 at 11:39:02 -0800, Dave Hansen wrote:
>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@xxxxxxxxxx>
>>
>> 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.

Thanks for catching that, I'll test it some more.

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

I'll test it too and see if it's easy enough to fix. Thanks!

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

-- 
Kind regards
Maciej Wieczór-Retman