Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On 2/16/22 07:13, Arnd Bergmann wrote: From: Arnd Bergmann There are many different ways that access_ok() is defined across architectures, but in the end, they all just compare against the user_addr_max() value or they accept anything. Provide one definition that works for most architectures, checking against TASK_SIZE_MAX for user processes or skipping the check inside of uaccess_kernel() sections. For architectures without CONFIG_SET_FS(), this should be the fastest check, as it comes down to a single comparison of a pointer against a compile-time constant, while the architecture specific versions tend to do something more complex for historic reasons or get something wrong. Type checking for __user annotations is handled inconsistently across architectures, but this is easily simplified as well by using an inline function that takes a 'const void __user *' argument. A handful of callers need an extra __user annotation for this. Some architectures had trick to use 33-bit or 65-bit arithmetic on the addresses to calculate the overflow, however this simpler version uses fewer registers, which means it can produce better object code in the end despite needing a second (statically predicted) branch. Reviewed-by: Christoph Hellwig Acked-by: Mark Rutland [arm64, asm-generic] Signed-off-by: Arnd Bergmann --- arch/Kconfig | 7 arch/alpha/include/asm/uaccess.h | 34 +++ arch/arc/include/asm/uaccess.h| 29 - arch/arm/include/asm/uaccess.h| 20 + arch/arm64/include/asm/uaccess.h | 11 ++--- arch/csky/include/asm/uaccess.h | 8 arch/hexagon/include/asm/uaccess.h| 25 arch/ia64/include/asm/uaccess.h | 5 +-- arch/m68k/Kconfig.cpu | 1 + arch/m68k/include/asm/uaccess.h | 19 + arch/microblaze/include/asm/uaccess.h | 8 +--- arch/mips/include/asm/uaccess.h | 29 + arch/nds32/include/asm/uaccess.h | 7 +--- arch/nios2/include/asm/uaccess.h | 11 + Acked-by: Dinh Nguyen
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Thu, Feb 24, 2022 at 9:29 AM Stafford Horne wrote:
> > -
> > -#define access_ok(addr, size)
> > \
> > -({ \
> > - __chk_user_ptr(addr); \
> > - __range_ok((unsigned long)(addr), (size)); \
> > -})
> > +#include
>
> I was going to ask why we are missing __chk_user_ptr in the generic version.
> But this is basically now a no-op so I think its OK.
Correct, the type checking is implied by making __access_ok() an inline
function that takes a __user pointer.
> Acked-by: Stafford Horne [openrisc, asm-generic]
Thanks!
Arnd
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 02:13:27PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> There are many different ways that access_ok() is defined across
> architectures, but in the end, they all just compare against the
> user_addr_max() value or they accept anything.
>
> Provide one definition that works for most architectures, checking
> against TASK_SIZE_MAX for user processes or skipping the check inside
> of uaccess_kernel() sections.
>
> For architectures without CONFIG_SET_FS(), this should be the fastest
> check, as it comes down to a single comparison of a pointer against a
> compile-time constant, while the architecture specific versions tend to
> do something more complex for historic reasons or get something wrong.
>
> Type checking for __user annotations is handled inconsistently across
> architectures, but this is easily simplified as well by using an inline
> function that takes a 'const void __user *' argument. A handful of
> callers need an extra __user annotation for this.
>
> Some architectures had trick to use 33-bit or 65-bit arithmetic on the
> addresses to calculate the overflow, however this simpler version uses
> fewer registers, which means it can produce better object code in the
> end despite needing a second (statically predicted) branch.
>
> Reviewed-by: Christoph Hellwig
> Acked-by: Mark Rutland [arm64, asm-generic]
> Signed-off-by: Arnd Bergmann
> ---
...
> arch/openrisc/include/asm/uaccess.h | 19 +
...
> include/asm-generic/access_ok.h | 59 +++
> include/asm-generic/uaccess.h | 21 +-
> include/linux/uaccess.h | 7
> 32 files changed, 109 insertions(+), 366 deletions(-)
>
...
> diff --git a/arch/openrisc/include/asm/uaccess.h
> b/arch/openrisc/include/asm/uaccess.h
> index 120f5005461b..8f049ec99b3e 100644
> --- a/arch/openrisc/include/asm/uaccess.h
> +++ b/arch/openrisc/include/asm/uaccess.h
> @@ -45,21 +45,7 @@
>
> #define uaccess_kernel() (get_fs() == KERNEL_DS)
>
> -/* Ensure that the range from addr to addr+size is all within the process'
> - * address space
> - */
> -static inline int __range_ok(unsigned long addr, unsigned long size)
> -{
> - const mm_segment_t fs = get_fs();
> -
> - return size <= fs && addr <= (fs - size);
> -}
> -
> -#define access_ok(addr, size)
> \
> -({ \
> - __chk_user_ptr(addr); \
> - __range_ok((unsigned long)(addr), (size)); \
> -})
> +#include
I was going to ask why we are missing __chk_user_ptr in the generic version.
But this is basically now a no-op so I think its OK.
> /*
> * These are the main single-value transfer routines. They automatically
> @@ -268,9 +254,6 @@ clear_user(void __user *addr, unsigned long size)
> return size;
> }
>
> -#define user_addr_max() \
> - (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -
> extern long strncpy_from_user(char *dest, const char __user *src, long
> count);
>
> extern __must_check long strnlen_user(const char __user *str, long n);
...
> diff --git a/include/asm-generic/access_ok.h b/include/asm-generic/access_ok.h
> new file mode 100644
> index ..1aad8964d2ed
> --- /dev/null
> +++ b/include/asm-generic/access_ok.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_GENERIC_ACCESS_OK_H__
> +#define __ASM_GENERIC_ACCESS_OK_H__
> +
> +/*
> + * Checking whether a pointer is valid for user space access.
> + * These definitions work on most architectures, but overrides can
> + * be used where necessary.
> + */
> +
> +/*
> + * architectures with compat tasks have a variable TASK_SIZE and should
> + * override this to a constant.
> + */
> +#ifndef TASK_SIZE_MAX
> +#define TASK_SIZE_MAXTASK_SIZE
> +#endif
> +
> +#ifndef uaccess_kernel
> +#ifdef CONFIG_SET_FS
> +#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> +#else
> +#define uaccess_kernel() (0)
> +#endif
> +#endif
> +
> +#ifndef user_addr_max
> +#define user_addr_max() (uaccess_kernel() ? ~0UL :
> TASK_SIZE_MAX)
> +#endif
> +
> +#ifndef __access_ok
> +/*
> + * 'size' is a compile-time constant for most callers, so optimize for
> + * this case to turn the check into a single comparison against a constant
> + * limit and catch all possible overflows.
> + * On architectures with separate user address space (m68k, s390, parisc,
> + * sparc64) or those without an MMU, this should always return true.
> + *
> + * This version was originally contributed by Jonas Bonn for the
> + * OpenRISC architecture, and was found to be the most efficient
> + * for constant 'size' and 'limit' values.
> + */
> +static inline int __access_ok(const void __user *ptr, unsigned long size)
> +{
> + unsigned long limit = user_addr_max();
> +
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Fri, Feb 18, 2022 at 1:30 AM David Laight wrote: > > From: Andy Lutomirski > > Sent: 17 February 2022 19:15 > ... > > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > > constant that has a very specific value to work around a bug^Wdesign > > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > > which userspace is permitted to allocate memory, but there is a huge > > gap between user and kernel addresses, and any value in the gap would > > be adequate for the comparison. If we wanted to optimize this, simply > > checking the high bit (which x86 can do without any immediate > > constants at all) would be sufficient and, for an access known to fit > > in 32 bits, one could get even fancier and completely ignore the size > > of the access. (For accesses not known to fit in 32 bits, I suspect > > some creativity could still come up with a construction that's > > substantially faster than the one in your patch.) > > > > So there's plenty of room for optimization here. > > > > (This is not in any respect a NAK -- it's just an observation that > > this could be even better.) > > For 64bit arch that use the top bit to separate user/kernel > you can test '(addr | size) >> 62)'. > The compiler optimises out constant sizes. > > This has all been mentioned a lot of times. > You do get different fault types. > > OTOH an explicit check for constant size (less than something big) > can use the cheaper test of the sign bit. > Big constant sizes could be compile time errors. The different fault type issue may well be a real problem. Right now the core x86 fault code reserves the right to grouch if we get #GP instead of #PF. We could change that.
RE: [PATCH v2 13/18] uaccess: generalize access_ok()
From: Andy Lutomirski > Sent: 17 February 2022 19:15 ... > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > constant that has a very specific value to work around a bug^Wdesign > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > which userspace is permitted to allocate memory, but there is a huge > gap between user and kernel addresses, and any value in the gap would > be adequate for the comparison. If we wanted to optimize this, simply > checking the high bit (which x86 can do without any immediate > constants at all) would be sufficient and, for an access known to fit > in 32 bits, one could get even fancier and completely ignore the size > of the access. (For accesses not known to fit in 32 bits, I suspect > some creativity could still come up with a construction that's > substantially faster than the one in your patch.) > > So there's plenty of room for optimization here. > > (This is not in any respect a NAK -- it's just an observation that > this could be even better.) For 64bit arch that use the top bit to separate user/kernel you can test '(addr | size) >> 62)'. The compiler optimises out constant sizes. This has all been mentioned a lot of times. You do get different fault types. OTOH an explicit check for constant size (less than something big) can use the cheaper test of the sign bit. Big constant sizes could be compile time errors. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 2:17 PM Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> There are many different ways that access_ok() is defined across
> architectures, but in the end, they all just compare against the
> user_addr_max() value or they accept anything.
>
> Provide one definition that works for most architectures, checking
> against TASK_SIZE_MAX for user processes or skipping the check inside
> of uaccess_kernel() sections.
>
> For architectures without CONFIG_SET_FS(), this should be the fastest
> check, as it comes down to a single comparison of a pointer against a
> compile-time constant, while the architecture specific versions tend to
> do something more complex for historic reasons or get something wrong.
>
> Type checking for __user annotations is handled inconsistently across
> architectures, but this is easily simplified as well by using an inline
> function that takes a 'const void __user *' argument. A handful of
> callers need an extra __user annotation for this.
>
> Some architectures had trick to use 33-bit or 65-bit arithmetic on the
> addresses to calculate the overflow, however this simpler version uses
> fewer registers, which means it can produce better object code in the
> end despite needing a second (statically predicted) branch.
>
> Reviewed-by: Christoph Hellwig
> Acked-by: Mark Rutland [arm64, asm-generic]
> Signed-off-by: Arnd Bergmann
> arch/m68k/Kconfig.cpu | 1 +
> arch/m68k/include/asm/uaccess.h | 19 +
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Fri, Feb 18, 2022 at 7:34 AM Christoph Hellwig wrote: > > > +#include > > Instead of the asm-generic games, shouldn't we just define access_ok in > if not already defined by the architecture? I tried, but couldn't actually make it work because asm/uaccess.h tends to contain inline functions that rely on access_ok(). It could work once we move all the high-level functions into linux/uaccess.h, but that would likely require another long patch series. One option that can work is to require architectures to have an asm/access_ok.h header that gets included by linux/uaccess.h. On most architectures, that would be redirected to asm-generic/access_ok.h, as only ia64, x86, arm64 and um need to override the definition. Arnd
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Thu, Feb 17, 2022 at 8:15 PM Andy Lutomirski wrote: > > On Wed, Feb 16, 2022 at 5:19 AM Arnd Bergmann wrote: > > > > From: Arnd Bergmann > > > > There are many different ways that access_ok() is defined across > > architectures, but in the end, they all just compare against the > > user_addr_max() value or they accept anything. > > > > Provide one definition that works for most architectures, checking > > against TASK_SIZE_MAX for user processes or skipping the check inside > > of uaccess_kernel() sections. > > > > For architectures without CONFIG_SET_FS(), this should be the fastest > > check, as it comes down to a single comparison of a pointer against a > > compile-time constant, while the architecture specific versions tend to > > do something more complex for historic reasons or get something wrong. > > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > constant that has a very specific value to work around a bug^Wdesign > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > which userspace is permitted to allocate memory, but there is a huge > gap between user and kernel addresses, and any value in the gap would > be adequate for the comparison. If we wanted to optimize this, simply > checking the high bit (which x86 can do without any immediate > constants at all) would be sufficient and, for an access known to fit > in 32 bits, one could get even fancier and completely ignore the size > of the access. (For accesses not known to fit in 32 bits, I suspect > some creativity could still come up with a construction that's > substantially faster than the one in your patch.) > > So there's plenty of room for optimization here. > > (This is not in any respect a NAK -- it's just an observation that > this could be even better.) Thank you for taking a look! As you can see in the patch that changes the algorithm on x86 [1], it was already using TASK_SIZE_MAX as the limit, only the order in which the comparison is done, hopefully leading to better code already. I have looked at trivial examples on x86 that showed an improvement for constant sizes, but only looked at arm64 in detail for the overall result. It may be worth checking if using LONG_MAX as the limit produces better code, but it's probably best to do the optimization in the common code in a portable way to keep it from diverging again. Arnd [1] https://lore.kernel.org/lkml/20220216131332.1489939-7-a...@kernel.org/
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
> +#include Instead of the asm-generic games, shouldn't we just define access_ok in if not already defined by the architecture?
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 5:19 AM Arnd Bergmann wrote: > > From: Arnd Bergmann > > There are many different ways that access_ok() is defined across > architectures, but in the end, they all just compare against the > user_addr_max() value or they accept anything. > > Provide one definition that works for most architectures, checking > against TASK_SIZE_MAX for user processes or skipping the check inside > of uaccess_kernel() sections. > > For architectures without CONFIG_SET_FS(), this should be the fastest > check, as it comes down to a single comparison of a pointer against a > compile-time constant, while the architecture specific versions tend to > do something more complex for historic reasons or get something wrong. This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre constant that has a very specific value to work around a bug^Wdesign error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at which userspace is permitted to allocate memory, but there is a huge gap between user and kernel addresses, and any value in the gap would be adequate for the comparison. If we wanted to optimize this, simply checking the high bit (which x86 can do without any immediate constants at all) would be sufficient and, for an access known to fit in 32 bits, one could get even fancier and completely ignore the size of the access. (For accesses not known to fit in 32 bits, I suspect some creativity could still come up with a construction that's substantially faster than the one in your patch.) So there's plenty of room for optimization here. (This is not in any respect a NAK -- it's just an observation that this could be even better.)
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 2:13 PM Arnd Bergmann wrote:
> + * limit and catch all possible overflows.
> + * On architectures with separate user address space (m68k, s390, parisc,
> + * sparc64) or those without an MMU, this should always return true.
...
> +static inline int __access_ok(const void __user *ptr, unsigned long size)
> +{
> + unsigned long limit = user_addr_max();
> + unsigned long addr = (unsigned long)ptr;
> +
> + if (IS_ENABLED(CONFIG_ALTERNATE_USER_ADDRESS_SPACE))
> + return true;
I noticed that I'm missing the check for !CONFIG_MMU here, despite
mentioning that in the comment above it. I've added it now.
Arnd
