Message-ID: <YguJYMxJsNj9L4Ee@FVFF77S0Q05N>
Date:   Tue, 15 Feb 2022 11:07:12 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>, linux-arch@...r.kernel.org,
        linux-mm@...ck.org, linux-api@...r.kernel.org, arnd@...db.de,
        linux-kernel@...r.kernel.org, linux@...linux.org.uk,
        will@...nel.org, guoren@...nel.org, bcain@...eaurora.org,
        geert@...ux-m68k.org, monstr@...str.eu, tsbogend@...ha.franken.de,
        nickhu@...estech.com, green.hu@...il.com, dinguyen@...nel.org,
        shorne@...il.com, deller@....de, mpe@...erman.id.au,
        peterz@...radead.org, mingo@...hat.com, hca@...ux.ibm.com,
        dalias@...c.org, davem@...emloft.net, richard@....at,
        x86@...nel.org, jcmvbkbc@...il.com, ebiederm@...ssion.com,
        akpm@...ux-foundation.org, ardb@...nel.org,
        linux-alpha@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-csky@...r.kernel.org,
        linux-hexagon@...r.kernel.org, linux-ia64@...r.kernel.org,
        linux-m68k@...ts.linux-m68k.org, linux-mips@...r.kernel.org,
        openrisc@...ts.librecores.org, linux-parisc@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, linux-riscv@...ts.infradead.org,
        linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
        sparclinux@...r.kernel.org, linux-um@...ts.infradead.org,
        linux-xtensa@...ux-xtensa.org
Subject: Re: [PATCH 08/14] arm64: simplify access_ok()

On Mon, Feb 14, 2022 at 05:34:46PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> arm64 has an inline asm implementation of access_ok() that is derived from
> the 32-bit arm version and optimized for the case that both the limit and
> the size are variable. With set_fs() gone, the limit is always constant,
> and the size usually is as well, so just using the default implementation
> reduces the check into a comparison against a constant that can be
> scheduled by the compiler.
> 
> On a defconfig build, this saves over 28KB of .text.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

I had a play around with this and a number of alternative options that had
previously been discussed (e.g. using uint128_t for the check to allow the
compiler to use the carry flag), and:

* Any sequences which we significantly simpler involved an ABI change (e.g. not
  checking tags for tasks not using the relaxed tag ABI), or didn't interact
  well with the uaccess pointer masking we do for speculation hardening.

* For all constant-size cases, this was joint-best for codegen.

* For variable-size cases the difference between options (which did not change
  ABI or break pointer masking) fell in the noise and really depended on what
  you were optimizing for.

This patch itself is clear, I believe the logic is sound and does not result in
a behavioural change, so for this as-is:

Acked-by: Mark Rutland <mark.rutland@....com>

As on other replies, I think that if we want to make further changes to this,
we should do that as follow-ups, since there are a number of subtleties in this
area w.r.t. tag management and speculation with potential ABI implications.

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/uaccess.h | 28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 357f7bd9c981..e8dce0cc5eaa 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -26,6 +26,8 @@
>  #include <asm/memory.h>
>  #include <asm/extable.h>
>  
> +static inline int __access_ok(const void __user *ptr, unsigned long size);
> +
>  /*
>   * Test whether a block of memory is a valid user space address.
>   * Returns 1 if the range is valid, 0 otherwise.
> @@ -33,10 +35,8 @@
>   * This is equivalent to the following test:
>   * (u65)addr + (u65)size <= (u65)TASK_SIZE_MAX
>   */
> -static inline unsigned long __access_ok(const void __user *addr, unsigned long size)
> +static inline int access_ok(const void __user *addr, unsigned long size)
>  {
> -	unsigned long ret, limit = TASK_SIZE_MAX - 1;
> -
>  	/*
>  	 * Asynchronous I/O running in a kernel thread does not have the
>  	 * TIF_TAGGED_ADDR flag of the process owning the mm, so always untag
> @@ -46,27 +46,9 @@ static inline unsigned long __access_ok(const void __user *addr, unsigned long s
>  	    (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
>  		addr = untagged_addr(addr);
>  
> -	__chk_user_ptr(addr);
> -	asm volatile(
> -	// A + B <= C + 1 for all A,B,C, in four easy steps:
> -	// 1: X = A + B; X' = X % 2^64
> -	"	adds	%0, %3, %2\n"
> -	// 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
> -	"	csel	%1, xzr, %1, hi\n"
> -	// 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
> -	//    to compensate for the carry flag being set in step 4. For
> -	//    X > 2^64, X' merely has to remain nonzero, which it does.
> -	"	csinv	%0, %0, xzr, cc\n"
> -	// 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
> -	//    comes from the carry in being clear. Otherwise, we are
> -	//    testing X' - C == 0, subject to the previous adjustments.
> -	"	sbcs	xzr, %0, %1\n"
> -	"	cset	%0, ls\n"
> -	: "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
> -
> -	return ret;
> +	return likely(__access_ok(addr, size));
>  }
> -#define __access_ok __access_ok
> +#define access_ok access_ok
>  
>  #include <asm-generic/access_ok.h>
>  
> -- 
> 2.29.2
> 

Powered by blists - more mailing lists