The helper used by the core-mm to strip tag bits and get the address to
the canonical shape. In only handles userspace addresses.
For LAM, the address gets sanitized according to the thread features.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/page_32.h | 3 +++
arch/x86/include/asm/page_64.h | 20 ++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
index df42f8aa99e4..2d35059b90c1 100644
--- a/arch/x86/include/asm/page_32.h
+++ b/arch/x86/include/asm/page_32.h
@@ -15,6 +15,9 @@ extern unsigned long __phys_addr(unsigned long);
#define __phys_addr_symbol(x) __phys_addr(x)
#define __phys_reloc_hide(x) RELOC_HIDE((x), 0)
+#define untagged_addr(addr) (addr)
+#define untagged_ptr(ptr) (ptr)
+
#ifdef CONFIG_FLATMEM
#define pfn_valid(pfn) ((pfn) < max_mapnr)
#endif /* CONFIG_FLATMEM */
diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index e9c86299b835..3a40c958b24a 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -7,6 +7,7 @@
#ifndef __ASSEMBLY__
#include <asm/cpufeatures.h>
#include <asm/alternative.h>
+#include <uapi/asm/prctl.h>
/* duplicated to the one in bootmem.h */
extern unsigned long max_pfn;
@@ -90,6 +91,25 @@ static __always_inline unsigned long task_size_max(void)
}
#endif /* CONFIG_X86_5LEVEL */
+#define __untagged_addr(addr, n) \
+ ((__force __typeof__(addr))sign_extend64((__force u64)(addr), n))
+
+#define untagged_addr(addr) ({ \
+ u64 __addr = (__force u64)(addr); \
+ if (__addr >> 63 == 0) { \
+ if (current->thread.features & X86_THREAD_LAM_U57) \
+ __addr &= __untagged_addr(__addr, 56); \
+ else if (current->thread.features & X86_THREAD_LAM_U48) \
+ __addr &= __untagged_addr(__addr, 47); \
+ } \
+ (__force __typeof__(addr))__addr; \
+})
+
+#define untagged_ptr(ptr) ({ \
+ u64 __ptrval = (__force u64)(ptr); \
+ __ptrval = untagged_addr(__ptrval); \
+ (__force __typeof__(*(ptr)) *)__ptrval; \
+})
#endif /* !__ASSEMBLY__ */
#ifdef CONFIG_X86_VSYSCALL_EMULATION
--
2.35.1
On Wed, May 11, 2022 at 09:21:16AM +0200, Peter Zijlstra wrote:
> On Wed, May 11, 2022 at 05:27:46AM +0300, Kirill A. Shutemov wrote:
> > +#define __untagged_addr(addr, n) \
> > + ((__force __typeof__(addr))sign_extend64((__force u64)(addr), n))
> > +
> > +#define untagged_addr(addr) ({ \
> > + u64 __addr = (__force u64)(addr); \
> > + if (__addr >> 63 == 0) { \
> > + if (current->thread.features & X86_THREAD_LAM_U57) \
> > + __addr &= __untagged_addr(__addr, 56); \
> > + else if (current->thread.features & X86_THREAD_LAM_U48) \
> > + __addr &= __untagged_addr(__addr, 47); \
> > + } \
> > + (__force __typeof__(addr))__addr; \
> > +})
>
> Assuming you got your bits in hardware order:
>
> u64 __addr = addr;
> if ((s64)__addr >= 0) {
> int lam = (current->thread.features >> X86_THREAD_LAM_U57) & 3;
That needs a _BIT suffix or something, same in the previous reply.
> if (lam)
> __addr &= sign_extend64(__addr, 65 - 9*lam);
> }
> __addr;
>
> has less branches on and should definitely result in better code (or I
> need more morning juice).
I definitely needs more morning juice :-)
On Wed, May 11, 2022 at 05:27:46AM +0300, Kirill A. Shutemov wrote:
> +#define __untagged_addr(addr, n) \
> + ((__force __typeof__(addr))sign_extend64((__force u64)(addr), n))
> +
> +#define untagged_addr(addr) ({ \
> + u64 __addr = (__force u64)(addr); \
> + if (__addr >> 63 == 0) { \
> + if (current->thread.features & X86_THREAD_LAM_U57) \
> + __addr &= __untagged_addr(__addr, 56); \
> + else if (current->thread.features & X86_THREAD_LAM_U48) \
> + __addr &= __untagged_addr(__addr, 47); \
> + } \
> + (__force __typeof__(addr))__addr; \
> +})
Assuming you got your bits in hardware order:
u64 __addr = addr;
if ((s64)__addr >= 0) {
int lam = (current->thread.features >> X86_THREAD_LAM_U57) & 3;
if (lam)
__addr &= sign_extend64(__addr, 65 - 9*lam);
}
__addr;
has less branches on and should definitely result in better code (or I
need more morning juice).
> +
> +#define untagged_ptr(ptr) ({ \
> + u64 __ptrval = (__force u64)(ptr); \
> + __ptrval = untagged_addr(__ptrval); \
> + (__force __typeof__(*(ptr)) *)__ptrval; \
> +})
> #endif /* !__ASSEMBLY__ */
>
> #ifdef CONFIG_X86_VSYSCALL_EMULATION
> --
> 2.35.1
>
On Wed, May 11 2022 at 05:27, Kirill A. Shutemov wrote:
> +#define __untagged_addr(addr, n) \
> + ((__force __typeof__(addr))sign_extend64((__force u64)(addr), n))
How is this supposed to be correct? This sign extends based on bit 47
resp. 56, i.e. the topmost bit of the userspace address space for the
LAM mode.
So if that bit _is_ set, then the result has bit 48-63 resp. 57-63 set
as well. Not really what you want, right?
This has to mask out bit 48-62 resp. 57-62 and leave all other bits
alone.
> +#define untagged_addr(addr) ({ \
> + u64 __addr = (__force u64)(addr); \
> + if (__addr >> 63 == 0) { \
> + if (current->thread.features & X86_THREAD_LAM_U57) \
> + __addr &= __untagged_addr(__addr, 56); \
> + else if (current->thread.features & X86_THREAD_LAM_U48) \
> + __addr &= __untagged_addr(__addr, 47); \
> + } \
> + (__force __typeof__(addr))__addr; \
> +})
So this wants something like this:
#define untagged_addr(addr) ({ \
u64 __addr = (__force u64)(addr); \
\
__addr &= current->thread.lam_untag_mask; \
(__force __typeof__(addr))__addr; \
})
No conditionals, fast _and_ correct. Setting this untag mask up once
when LAM is enabled is not rocket science.
Thanks,
tglx
On Thu, May 12 2022 at 16:23, Peter Zijlstra wrote:
> On Thu, May 12, 2022 at 03:06:38PM +0200, Thomas Gleixner wrote:
>
>> #define untagged_addr(addr) ({ \
>> u64 __addr = (__force u64)(addr); \
>> \
>> __addr &= current->thread.lam_untag_mask; \
>> (__force __typeof__(addr))__addr; \
>> })
>>
>> No conditionals, fast _and_ correct. Setting this untag mask up once
>> when LAM is enabled is not rocket science.
>
> But that goes wrong if someone ever wants to untag a kernel address and
> not use the result for access_ok().
>
> I'd feel better about something like:
>
> s64 __addr = (addr);
> s64 __sign = __addr;
>
> __sign >>= 63;
> __sign &= lam_untag_mask;
that needs to be
__sign &= ~lam_untag_mask;
> __addr &= lam_untag_mask;
> __addr |= __sign;
>
> __addr;
>
> Which simply extends bit 63 downwards -- although possibly there's an
> easier way to do that, this is pretty gross.
For the price of a conditional:
__addr &= lam_untag_mask;
if (__addr & BIT(63))
__addr |= ~lam_untag_mask;
Now you have the choice between gross and ugly.
Thanks,
tglx
From: Thomas Gleixner
> Sent: 13 May 2022 00:15
...
> But whatever we chose, it's sad, that we need to have support for
> interfaces which swallow any pointer (user or kernel) because otherwise
> this really boils down to a single OR resp. AND operation plus the
> according mov to retrieve the mask.
Are there any of those left?
Most will have gone with setfs(KERNEL_DS) removal.
Almost all code has to know whether an address is user
or kernel - the value can't be used because of architectures
that use the same address values in user and kernel.
How often do addresses actually need de-tagging?
Probably only code that is looking for page table
entries for virtual addresses?
How often does that happen for user addresses?
If the hardware is ignoring the bits then you don't
need to remove them before memory accesses.
That would include all userspace accesses.
Clearly access_ok() has to work with tagged addresses,
but that doesn't require the tag be masked off.
It can just check the transfer doesn't cross 1u<<63.
It (probably) just requires the fault handler to treat
non-canonical address faults as page faults.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, May 12, 2022 at 03:06:38PM +0200, Thomas Gleixner wrote:
> #define untagged_addr(addr) ({ \
> u64 __addr = (__force u64)(addr); \
> \
> __addr &= current->thread.lam_untag_mask; \
> (__force __typeof__(addr))__addr; \
> })
>
> No conditionals, fast _and_ correct. Setting this untag mask up once
> when LAM is enabled is not rocket science.
But that goes wrong if someone ever wants to untag a kernel address and
not use the result for access_ok().
I'd feel better about something like:
s64 __addr = (addr);
s64 __sign = __addr;
__sign >>= 63;
__sign &= lam_untag_mask;
__addr &= lam_untag_mask;
__addr |= __sign;
__addr;
Which simply extends bit 63 downwards -- although possibly there's an
easier way to do that, this is pretty gross.
On Thu, May 12 2022 at 17:16, Thomas Gleixner wrote:
> On Thu, May 12 2022 at 16:23, Peter Zijlstra wrote:
>> On Thu, May 12, 2022 at 03:06:38PM +0200, Thomas Gleixner wrote:
>>
>>> #define untagged_addr(addr) ({ \
>>> u64 __addr = (__force u64)(addr); \
>>> \
>>> __addr &= current->thread.lam_untag_mask; \
>>> (__force __typeof__(addr))__addr; \
>>> })
>>>
>>> No conditionals, fast _and_ correct. Setting this untag mask up once
>>> when LAM is enabled is not rocket science.
>>
>> But that goes wrong if someone ever wants to untag a kernel address and
>> not use the result for access_ok().
>>
>> I'd feel better about something like:
>>
>> s64 __addr = (addr);
>> s64 __sign = __addr;
>>
>> __sign >>= 63;
>> __sign &= lam_untag_mask;
>
> that needs to be
>
> __sign &= ~lam_untag_mask;
>
>> __addr &= lam_untag_mask;
>> __addr |= __sign;
>>
>> __addr;
>>
>> Which simply extends bit 63 downwards -- although possibly there's an
>> easier way to do that, this is pretty gross.
>
> For the price of a conditional:
>
> __addr &= lam_untag_mask;
> if (__addr & BIT(63))
> __addr |= ~lam_untag_mask;
>
> Now you have the choice between gross and ugly.
Though we can also replace your flavour of gross with a different
flavour of gross:
s64 sign = (s64)(addr) >> 63;
addr ^= sign;
addr &= mask;
addr ^= sign;
After twisting my brain around replacing gross by something differently
gross and coming up with the gem above I actually did compile the
variants and discovered that GCC compiles your flavour of gross exactly
to this:
mov %rdi,%rax
sar $0x3f,%rax
xor %rax,%rdi
and %rsi,%rdi
xor %rdi,%rax
I have to admit that compilers are sometimes pretty smart. I might have
to rethink my prejudice. :)
But then clang converts your flavour of 'gross' to:
mov %rsi,%rax
mov %rsi,%rcx
and %rdi,%rax
sar $0x3f,%rdi
not %rcx
and %rdi,%rcx
or %rcx,%rax
and my explicit flavour to:
mov %rdi,%rax
mov %rdi,%rcx
sar $0x3f,%rcx
xor %rcx,%rax
and %rsi,%rax
xor %rcx,%rax
which is at least slightly less retarted, but still has a pointless mov
there. Note, that this was compiled in user space with noinline
functions. I did some inlined variants as well and clang still insists
on using an extra register for no obvious reason. This might be more
efficient in reality, but I haven't bothered to write a test which
might give an answer via perf.
The ugly with the conditional resolves for both compilers to:
mov %rsi,%rax
mov %rsi,%rcx
not %rcx
or %rdi,%rcx
and %rdi,%rax
test %rdi,%rdi
cmovs %rcx,%rax
At least they agree on that one.
But whatever we chose, it's sad, that we need to have support for
interfaces which swallow any pointer (user or kernel) because otherwise
this really boils down to a single OR resp. AND operation plus the
according mov to retrieve the mask.
Thanks,
tglx