Re: [PATCH v3 2/6] x86/uaccess: Avoid barrier_nospec() in 64-bit __get_user()

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Thu, Nov 21, 2024 at 02:16:12PM -0800, Linus Torvalds wrote:
>     mov    %gs:0x0,%rax                 # current
>     incl   0x1a9c(%rax)                 # current->pagefault_disable++
>     movabs $0x123456789abcdef,%rcx      # magic virtual address size
>     cmp    %rsi,%rcx                    # address masking
>     sbb    %rcx,%rcx
>     or     %rsi,%rcx
>     stac                                # enable user space acccess
>     mov    (%rcx),%ecx                  # get the value
>     clac                                # disable user space access
>     decl   0x1a9c(%rax)                 # current->pagefault_disable--
>     mov    %ecx,(%rdi)                  # save the value
>     xor    %eax,%eax                    # return 0
>     ret

The asm looks good, but the C exploded a bit... why not just have an
inline get_user()?

> If you can test this and verify that it actually help, I'll take it as
> a patch. Consider it signed-off after testing.

Let me see if I can recreate the original report (or get the automatic
testing to see the commit).

> > Also, is there any harm in speeding up __get_user()?  It still has ~80
> > callers and it's likely to be slowing down things we don't know about.
> 
> How would you speed it up?  We definitely can't replace the fence with
> addressing tricks. So we can't just replace it with "get_user()",
> because of those horrid architecture-specific kernel uses.

I'm not sure if you saw the example code snippet I posted up-thread,
here it is below.

It adds a conditional branch to test if it's a user address.  If so, it
does pointer masking.  If not, it does LFENCE.  So "bad" users get the
slow path, and we could even add a WARN_ON().

Yes, another conditional branch isn't ideal, but it's still much faster
than the current code, and it would root out any bad users with a
WARN_ON() so that eventually it can just become a get_user() alias.

.macro __get_user_nocheck_nospec
#ifdef CONFIG_X86_64
	movq $0x0123456789abcdef, %rdx
 1:
.pushsection runtime_ptr_USER_PTR_MAX, "a"
	.long 1b - 8 - .
.popsection
	cmp %rax, %rdx
	jb 10f
	sbb %rdx, %rdx
	or %rdx, %rax
	jmp 11f
10:	/*
	 * Stop access_ok() branch misprediction -- both of them ;-)
	 *
	 * As a benefit this also punishes callers who intentionally call this
	 * with a kernel address.  Once they're rooted out, __get_user() can
	 * just become an alias of get_user().
	 *
	 * TODO: Add WARN_ON()
	 */
#endif
	ASM_BARRIER_NOSPEC
11:
.endm

/* .. and the same for __get_user, just without the range checks */
SYM_FUNC_START(__get_user_nocheck_1)
	__get_user_nocheck_nospec
	ASM_STAC
	UACCESS movzbl (%_ASM_AX),%edx
	xor %eax,%eax
	ASM_CLAC
	RET
SYM_FUNC_END(__get_user_nocheck_1)
EXPORT_SYMBOL(__get_user_nocheck_1)

-- 
Josh