* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Mark Rutland @ 2022-02-15 11:24 UTC (permalink / raw)
To: David Laight
Cc: Rich Felker, linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org,
Peter Zijlstra, Linux Kernel Mailing List,
Linux Memory Management List, Guo Ren,
open list:SPARC + UltraSPARC (sparc/sparc64), linux-riscv,
linux-api@vger.kernel.org, Will Deacon, Christoph Hellwig,
linux-arch, open list:S390, Brian Cain,
linux-hexagon@vger.kernel.org, Helge Deller, X86 ML, Russell King,
linux-csky@vger.kernel.org, 'Ard Biesheuvel',
Linus Torvalds, Ingo Molnar, Geert Uytterhoeven,
linux-snps-arc@lists.infradead.org,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, Richard Weinberger, linux-m68k,
openrisc@lists.librecores.org, Greentime Hu, Stafford Horne,
Linux ARM, Arnd Bergmann, monstr@monstr.eu, Thomas Bogendoerfer,
Nick Hu, open list:PARISC ARCHITECTURE, Max Filippov,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), open list:MIPS,
dinguyen@kernel.org, Eric W. Biederman, alpha, Andrew Morton,
Robin Murphy, David S. Miller
In-Reply-To: <153bb1887f484ed79ce8224845a4b2ea@AcuMS.aculab.com>
On Tue, Feb 15, 2022 at 09:30:41AM +0000, David Laight wrote:
> From: Ard Biesheuvel
> > Sent: 15 February 2022 08:18
> >
> > On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > From: Arnd Bergmann <arnd@arndb.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@arndb.de>
> > > ---
> > > 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
> > >
> >
> > With set_fs() out of the picture, wouldn't it be sufficient to check
> > that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
> > That would also remove the need to strip the tag from the address.
> >
> > Something like
> >
> > asm goto("tbnz %0, #55, %2 \n"
> > "tbnz %1, #55, %2 \n"
> > :: "r"(addr), "r"(addr + size - 1) :: notok);
> > return 1;
> > notok:
> > return 0;
> >
> > with an additional sanity check on the size which the compiler could
> > eliminate for compile-time constant values.
>
> Is there are reason not to just use:
> size < 1u << 48 && !((addr | (addr + size - 1)) & 1u << 55)
That has a few problems, including being an ABI change for tasks not using the
relaxed tag ABI and not working for 52-bit VAs.
If we really want to relax the tag checking aspect, there are simpler options,
including variations on Ard's approach above.
> Ugg, is arm64 addressing as horrid as it looks - with the 'kernel'
> bit in the middle of the virtual address space?
It's just sign-extension/canonical addressing, except bits [63:56] are
configurable between a few uses, so the achitecture says bit 55 is the one to
look at in all configurations to figure out if an address is high/low (in
addition to checking the remaining bits are canonical).
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Mark Rutland @ 2022-02-15 11:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: dalias, linux-ia64, linux-sh, peterz, linux-mips, linux-mm,
guoren, sparclinux, linux-hexagon, linux-riscv, will,
Christoph Hellwig, linux-arch, linux-s390, bcain, deller, x86,
linux, linux-csky, ardb, mingo, geert, linux-snps-arc,
linux-xtensa, arnd, hca, linux-alpha, linux-um, linuxppc-dev,
linux-m68k, openrisc, green.hu, shorne, linux-arm-kernel, monstr,
tsbogend, linux-parisc, nickhu, jcmvbkbc, linux-api, linux-kernel,
dinguyen, ebiederm, richard, akpm, Linus Torvalds, davem
In-Reply-To: <20220214163452.1568807-9-arnd@kernel.org>
On Mon, Feb 14, 2022 at 05:34:46PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.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@arndb.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@arm.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
>
^ permalink raw reply
* Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
From: Michael Ellerman @ 2022-02-15 11:05 UTC (permalink / raw)
To: Christophe Leroy, Naveen N. Rao, Jiri Kosina, Joe Lawrence,
Josh Poimboeuf, Miroslav Benes, Ingo Molnar, Petr Mladek,
Steven Rostedt
Cc: live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1b28f52a-f8b7-6b5c-e726-feac4123517d@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>>> of livepatching.
>>>
>>> Also note that powerpc being the last one to convert to
>>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>>> klp_arch_set_pc() on all architectures.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> arch/powerpc/Kconfig | 1 +
>>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>>> arch/powerpc/include/asm/livepatch.h | 4 +---
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index cdac2115eb00..e2b1792b2aae 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -210,6 +210,7 @@ config PPC
>>> select HAVE_DEBUG_KMEMLEAK
>>> select HAVE_DEBUG_STACKOVERFLOW
>>> select HAVE_DYNAMIC_FTRACE
>>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>>> select HAVE_EBPF_JIT
>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>>> && POWER7_CPU)
>>> diff --git a/arch/powerpc/include/asm/ftrace.h
>>> b/arch/powerpc/include/asm/ftrace.h
>>> index b3f6184f77ea..45c3d6f11daa 100644
>>> --- a/arch/powerpc/include/asm/ftrace.h
>>> +++ b/arch/powerpc/include/asm/ftrace.h
>>> @@ -22,6 +22,23 @@ static inline unsigned long
>>> ftrace_call_adjust(unsigned long addr)
>>> struct dyn_arch_ftrace {
>>> struct module *mod;
>>> };
>>> +
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>>> +struct ftrace_regs {
>>> + struct pt_regs regs;
>>> +};
>>> +
>>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>>> ftrace_regs *fregs)
>>> +{
>>> + return &fregs->regs;
>>> +}
>>
>> I think this is wrong. We need to differentiate between ftrace_caller()
>> and ftrace_regs_caller() here, and only return pt_regs if coming in
>> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
>
> Not sure I follow you.
>
> This is based on 5740a7c71ab6 ("s390/ftrace: add
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>
> It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
> with ftrace_caller().
>
> Sure you only have the params, but that's the same on s390, so what did
> I miss ?
I already have this series in next, I can pull it out, but I'd rather
not.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
>>> +static __always_inline void ftrace_instruction_pointer_set(struct
>>> ftrace_regs *fregs,
>>> + unsigned long ip)
>>> +{
>>> + regs_set_return_ip(&fregs->regs, ip);
>>
>> Should we use that helper here? regs_set_return_ip() also updates some
>> other state related to taking interrupts and I don't think it makes
>> sense for use with ftrace.
>
>
> Today we have:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> struct pt_regs *regs = ftrace_get_regs(fregs);
>
> regs_set_return_ip(regs, ip);
> }
>
>
> Which like x86 and s390 becomes:
>
> static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
> long ip)
> {
> ftrace_instruction_pointer_set(fregs, ip);
> }
>
>
>
> That's the reason why I've been using regs_set_return_ip(). Do you think
> it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
>
> That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
> registers if they are still valid")
It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.
But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.
So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.
cheers
^ permalink raw reply
* Re: [PATCH 07/14] uaccess: generalize access_ok()
From: Mark Rutland @ 2022-02-15 10:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: dalias, linux-ia64, linux-sh, peterz, linux-mips, linux-mm,
guoren, sparclinux, linux-hexagon, linux-riscv, will,
Christoph Hellwig, linux-arch, linux-s390, bcain, deller, x86,
linux, linux-csky, ardb, mingo, geert, linux-snps-arc,
linux-xtensa, arnd, hca, linux-alpha, linux-um, linuxppc-dev,
linux-m68k, openrisc, green.hu, shorne, linux-arm-kernel, monstr,
tsbogend, linux-parisc, nickhu, jcmvbkbc, linux-api, linux-kernel,
dinguyen, ebiederm, richard, akpm, Linus Torvalds, davem
In-Reply-To: <20220214163452.1568807-8-arnd@kernel.org>
On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
As discussed over IRC, the generic sequence looks good to me, and likewise for
the arm64 change, so:
Acked-by: Mark Rutland <mark.rutland@arm.com> [arm64, asm-generic]
Thanks,
Mark.
> ---
> arch/alpha/include/asm/uaccess.h | 34 +++------------
> arch/arc/include/asm/uaccess.h | 29 -------------
> arch/arm/include/asm/uaccess.h | 20 +--------
> arch/arm/kernel/swp_emulate.c | 2 +-
> arch/arm/kernel/traps.c | 2 +-
> arch/arm64/include/asm/uaccess.h | 5 ++-
> arch/csky/include/asm/uaccess.h | 8 ----
> arch/csky/kernel/signal.c | 2 +-
> arch/hexagon/include/asm/uaccess.h | 25 ------------
> arch/ia64/include/asm/uaccess.h | 5 +--
> arch/m68k/include/asm/uaccess.h | 5 ++-
> 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 +----
> arch/nios2/kernel/signal.c | 20 +++++----
> arch/openrisc/include/asm/uaccess.h | 19 +--------
> arch/parisc/include/asm/uaccess.h | 10 +++--
> arch/powerpc/include/asm/uaccess.h | 11 +----
> arch/powerpc/lib/sstep.c | 4 +-
> arch/riscv/include/asm/uaccess.h | 31 +-------------
> arch/riscv/kernel/perf_callchain.c | 2 +-
> arch/s390/include/asm/uaccess.h | 11 ++---
> arch/sh/include/asm/uaccess.h | 22 +---------
> arch/sparc/include/asm/uaccess.h | 3 --
> arch/sparc/include/asm/uaccess_32.h | 18 ++------
> arch/sparc/include/asm/uaccess_64.h | 35 ++++------------
> arch/sparc/kernel/signal_32.c | 2 +-
> arch/um/include/asm/uaccess.h | 5 ++-
> arch/x86/include/asm/uaccess.h | 14 +------
> arch/xtensa/include/asm/uaccess.h | 10 +----
> include/asm-generic/access_ok.h | 59 +++++++++++++++++++++++++++
> include/asm-generic/uaccess.h | 21 +---------
> include/linux/uaccess.h | 7 ----
> 34 files changed, 130 insertions(+), 366 deletions(-)
> create mode 100644 include/asm-generic/access_ok.h
>
> diff --git a/arch/alpha/include/asm/uaccess.h b/arch/alpha/include/asm/uaccess.h
> index 1b6f25efa247..82c5743fc9cd 100644
> --- a/arch/alpha/include/asm/uaccess.h
> +++ b/arch/alpha/include/asm/uaccess.h
> @@ -20,28 +20,7 @@
> #define get_fs() (current_thread_info()->addr_limit)
> #define set_fs(x) (current_thread_info()->addr_limit = (x))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> -/*
> - * Is a address valid? This does a straightforward calculation rather
> - * than tests.
> - *
> - * Address valid if:
> - * - "addr" doesn't have any high-bits set
> - * - AND "size" doesn't have any high-bits set
> - * - AND "addr+size-(size != 0)" doesn't have any high-bits set
> - * - OR we are in kernel mode.
> - */
> -#define __access_ok(addr, size) ({ \
> - unsigned long __ao_a = (addr), __ao_b = (size); \
> - unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b; \
> - (get_fs().seg & (__ao_a | __ao_b | __ao_end)) == 0; })
> -
> -#define access_ok(addr, size) \
> -({ \
> - __chk_user_ptr(addr); \
> - __access_ok(((unsigned long)(addr)), (size)); \
> -})
> +#include <asm-generic/access_ok.h>
>
> /*
> * These are the main single-value transfer routines. They automatically
> @@ -105,7 +84,7 @@ extern void __get_user_unknown(void);
> long __gu_err = -EFAULT; \
> unsigned long __gu_val = 0; \
> const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
> - if (__access_ok((unsigned long)__gu_addr, size)) { \
> + if (__access_ok(__gu_addr, size)) { \
> __gu_err = 0; \
> switch (size) { \
> case 1: __get_user_8(__gu_addr); break; \
> @@ -200,7 +179,7 @@ extern void __put_user_unknown(void);
> ({ \
> long __pu_err = -EFAULT; \
> __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
> - if (__access_ok((unsigned long)__pu_addr, size)) { \
> + if (__access_ok(__pu_addr, size)) { \
> __pu_err = 0; \
> switch (size) { \
> case 1: __put_user_8(x, __pu_addr); break; \
> @@ -316,17 +295,14 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long len)
>
> extern long __clear_user(void __user *to, long len);
>
> -extern inline long
> +static inline long
> clear_user(void __user *to, long len)
> {
> - if (__access_ok((unsigned long)to, len))
> + if (__access_ok(to, len))
> len = __clear_user(to, len);
> return len;
> }
>
> -#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/arch/arc/include/asm/uaccess.h b/arch/arc/include/asm/uaccess.h
> index 783bfdb3bfa3..30f80b4be2ab 100644
> --- a/arch/arc/include/asm/uaccess.h
> +++ b/arch/arc/include/asm/uaccess.h
> @@ -23,35 +23,6 @@
>
> #include <linux/string.h> /* for generic string functions */
>
> -
> -#define __kernel_ok (uaccess_kernel())
> -
> -/*
> - * Algorithmically, for __user_ok() we want do:
> - * (start < TASK_SIZE) && (start+len < TASK_SIZE)
> - * where TASK_SIZE could either be retrieved from thread_info->addr_limit or
> - * emitted directly in code.
> - *
> - * This can however be rewritten as follows:
> - * (len <= TASK_SIZE) && (start+len < TASK_SIZE)
> - *
> - * Because it essentially checks if buffer end is within limit and @len is
> - * non-ngeative, which implies that buffer start will be within limit too.
> - *
> - * The reason for rewriting being, for majority of cases, @len is generally
> - * compile time constant, causing first sub-expression to be compile time
> - * subsumed.
> - *
> - * The second part would generate weird large LIMMs e.g. (0x6000_0000 - 0x10),
> - * so we check for TASK_SIZE using get_fs() since the addr_limit load from mem
> - * would already have been done at this call site for __kernel_ok()
> - *
> - */
> -#define __user_ok(addr, sz) (((sz) <= TASK_SIZE) && \
> - ((addr) <= (get_fs() - (sz))))
> -#define __access_ok(addr, sz) (unlikely(__kernel_ok) || \
> - likely(__user_ok((addr), (sz))))
> -
> /*********** Single byte/hword/word copies ******************/
>
> #define __get_user_fn(sz, u, k) \
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index d20d78c34b94..2fcbec9c306c 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -55,21 +55,6 @@ extern int __put_user_bad(void);
>
> #ifdef CONFIG_MMU
>
> -/*
> - * We use 33-bit arithmetic here. Success returns zero, failure returns
> - * addr_limit. We take advantage that addr_limit will be zero for KERNEL_DS,
> - * so this will always return success in that case.
> - */
> -#define __range_ok(addr, size) ({ \
> - unsigned long flag, roksum; \
> - __chk_user_ptr(addr); \
> - __asm__(".syntax unified\n" \
> - "adds %1, %2, %3; sbcscc %1, %1, %0; movcc %0, #0" \
> - : "=&r" (flag), "=&r" (roksum) \
> - : "r" (addr), "Ir" (size), "0" (TASK_SIZE) \
> - : "cc"); \
> - flag; })
> -
> /*
> * This is a type: either unsigned long, if the argument fits into
> * that type, or otherwise unsigned long long.
> @@ -241,15 +226,12 @@ extern int __put_user_8(void *, unsigned long long);
>
> #else /* CONFIG_MMU */
>
> -#define __addr_ok(addr) ((void)(addr), 1)
> -#define __range_ok(addr, size) ((void)(addr), 0)
> -
> #define get_user(x, p) __get_user(x, p)
> #define __put_user_check __put_user_nocheck
>
> #endif /* CONFIG_MMU */
>
> -#define access_ok(addr, size) (__range_ok(addr, size) == 0)
> +#include <asm-generic/access_ok.h>
>
> #ifdef CONFIG_CPU_SPECTRE
> /*
> diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
> index 6166ba38bf99..b74bfcf94fb1 100644
> --- a/arch/arm/kernel/swp_emulate.c
> +++ b/arch/arm/kernel/swp_emulate.c
> @@ -195,7 +195,7 @@ static int swp_handler(struct pt_regs *regs, unsigned int instr)
> destreg, EXTRACT_REG_NUM(instr, RT2_OFFSET), data);
>
> /* Check access in reasonable access range for both SWP and SWPB */
> - if (!access_ok((address & ~3), 4)) {
> + if (!access_ok((void __user *)(address & ~3), 4)) {
> pr_debug("SWP{B} emulation: access to %p not allowed!\n",
> (void *)address);
> res = -EFAULT;
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index da04ed85855a..26c8c8276297 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -576,7 +576,7 @@ do_cache_op(unsigned long start, unsigned long end, int flags)
> if (end < start || flags)
> return -EINVAL;
>
> - if (!access_ok(start, end - start))
> + if (!access_ok((void __user *)start, end - start))
> return -EFAULT;
>
> return __do_cache_op(start, end);
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 2e20879fe3cf..357f7bd9c981 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -33,7 +33,7 @@
> * This is equivalent to the following test:
> * (u65)addr + (u65)size <= (u65)TASK_SIZE_MAX
> */
> -static inline unsigned long __range_ok(const void __user *addr, unsigned long size)
> +static inline unsigned long __access_ok(const void __user *addr, unsigned long size)
> {
> unsigned long ret, limit = TASK_SIZE_MAX - 1;
>
> @@ -66,8 +66,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
>
> return ret;
> }
> +#define __access_ok __access_ok
>
> -#define access_ok(addr, size) __range_ok(addr, size)
> +#include <asm-generic/access_ok.h>
>
> /*
> * User access enabling/disabling.
> diff --git a/arch/csky/include/asm/uaccess.h b/arch/csky/include/asm/uaccess.h
> index ac5a54f57d40..fec8f77ffc99 100644
> --- a/arch/csky/include/asm/uaccess.h
> +++ b/arch/csky/include/asm/uaccess.h
> @@ -5,14 +5,6 @@
>
> #define user_addr_max() (current_thread_info()->addr_limit.seg)
>
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> -{
> - unsigned long limit = user_addr_max();
> -
> - return (size <= limit) && (addr <= (limit - size));
> -}
> -#define __access_ok __access_ok
> -
> /*
> * __put_user_fn
> */
> diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> index c7b763d2f526..8867ddf3e6c7 100644
> --- a/arch/csky/kernel/signal.c
> +++ b/arch/csky/kernel/signal.c
> @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal *ksig,
> static int
> setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> {
> - struct rt_sigframe *frame;
> + struct rt_sigframe __user *frame;
> int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
> diff --git a/arch/hexagon/include/asm/uaccess.h b/arch/hexagon/include/asm/uaccess.h
> index 719ba3f3c45c..bff77efc0d9a 100644
> --- a/arch/hexagon/include/asm/uaccess.h
> +++ b/arch/hexagon/include/asm/uaccess.h
> @@ -12,31 +12,6 @@
> */
> #include <asm/sections.h>
>
> -/*
> - * access_ok: - Checks if a user space pointer is valid
> - * @addr: User space pointer to start of block to check
> - * @size: Size of block to check
> - *
> - * Context: User context only. This function may sleep if pagefaults are
> - * enabled.
> - *
> - * Checks if a pointer to a block of memory in user space is valid.
> - *
> - * Returns true (nonzero) if the memory block *may* be valid, false (zero)
> - * if it is definitely invalid.
> - *
> - */
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -#define user_addr_max() (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> -{
> - unsigned long limit = TASK_SIZE;
> -
> - return (size <= limit) && (addr <= (limit - size));
> -}
> -#define __access_ok __access_ok
> -
> /*
> * When a kernel-mode page fault is taken, the faulting instruction
> * address is checked against a table of exception_table_entries.
> diff --git a/arch/ia64/include/asm/uaccess.h b/arch/ia64/include/asm/uaccess.h
> index e19d2dcc0ced..e242a3cc1330 100644
> --- a/arch/ia64/include/asm/uaccess.h
> +++ b/arch/ia64/include/asm/uaccess.h
> @@ -50,8 +50,6 @@
> #define get_fs() (current_thread_info()->addr_limit)
> #define set_fs(x) (current_thread_info()->addr_limit = (x))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> /*
> * When accessing user memory, we need to make sure the entire area really is in
> * user-level space. In order to do this efficiently, we make sure that the page at
> @@ -65,7 +63,8 @@ static inline int __access_ok(const void __user *p, unsigned long size)
> return likely(addr <= seg) &&
> (seg == KERNEL_DS.seg || likely(REGION_OFFSET(addr) < RGN_MAP_LIMIT));
> }
> -#define access_ok(addr, size) __access_ok((addr), (size))
> +#define __access_ok __access_ok
> +#include <asm-generic/access_ok.h>
>
> /*
> * These are the main single-value transfer routines. They automatically
> diff --git a/arch/m68k/include/asm/uaccess.h b/arch/m68k/include/asm/uaccess.h
> index 79617c0b2f91..d6bb5720365a 100644
> --- a/arch/m68k/include/asm/uaccess.h
> +++ b/arch/m68k/include/asm/uaccess.h
> @@ -12,15 +12,18 @@
> #include <asm/extable.h>
>
> /* We let the MMU do all checking */
> -static inline int access_ok(const void __user *addr,
> +static inline int __access_ok(const void __user *addr,
> unsigned long size)
> {
> /*
> * XXX: for !CONFIG_CPU_HAS_ADDRESS_SPACES this really needs to check
> * for TASK_SIZE!
> + * Removing this helper is probably sufficient.
> */
> return 1;
> }
> +#define __access_ok __access_ok
> +#include <asm-generic/access_ok.h>
>
> /*
> * Not all varients of the 68k family support the notion of address spaces.
> diff --git a/arch/microblaze/include/asm/uaccess.h b/arch/microblaze/include/asm/uaccess.h
> index 5b6e0e7788f4..dd82e90adb52 100644
> --- a/arch/microblaze/include/asm/uaccess.h
> +++ b/arch/microblaze/include/asm/uaccess.h
> @@ -39,13 +39,7 @@
>
> # define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
>
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> -{
> - unsigned long limit = user_addr_max();
> -
> - return (size <= limit) && (addr <= (limit - size));
> -}
> -#define access_ok(addr, size) __access_ok((unsigned long)addr, size)
> +#include <asm-generic/access_ok.h>
>
> # define __FIXUP_SECTION ".section .fixup,\"ax\"\n"
> # define __EX_TABLE_SECTION ".section __ex_table,\"a\"\n"
> diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
> index d7c89dc3426c..436248652b28 100644
> --- a/arch/mips/include/asm/uaccess.h
> +++ b/arch/mips/include/asm/uaccess.h
> @@ -44,34 +44,7 @@ extern u64 __ua_limit;
>
> #endif /* CONFIG_64BIT */
>
> -/*
> - * access_ok: - Checks if a user space pointer is valid
> - * @addr: User space pointer to start of block to check
> - * @size: Size of block to check
> - *
> - * Context: User context only. This function may sleep if pagefaults are
> - * enabled.
> - *
> - * Checks if a pointer to a block of memory in user space is valid.
> - *
> - * Returns true (nonzero) if the memory block may be valid, false (zero)
> - * if it is definitely invalid.
> - *
> - * Note that, depending on architecture, this function probably just
> - * checks that the pointer is in the user space range - after calling
> - * this function, memory access functions may still return -EFAULT.
> - */
> -
> -static inline int __access_ok(const void __user *p, unsigned long size)
> -{
> - unsigned long addr = (unsigned long)p;
> - unsigned long limit = TASK_SIZE_MAX;
> -
> - return (size <= limit) && (addr <= (limit - size));
> -}
> -
> -#define access_ok(addr, size) \
> - likely(__access_ok((addr), (size)))
> +#include <asm-generic/access_ok.h>
>
> /*
> * put_user: - Write a simple value into user space.
> diff --git a/arch/nds32/include/asm/uaccess.h b/arch/nds32/include/asm/uaccess.h
> index 37a40981deb3..832d642a4068 100644
> --- a/arch/nds32/include/asm/uaccess.h
> +++ b/arch/nds32/include/asm/uaccess.h
> @@ -38,18 +38,15 @@ extern int fixup_exception(struct pt_regs *regs);
>
> #define get_fs() (current_thread_info()->addr_limit)
> #define user_addr_max get_fs
> +#define uaccess_kernel() (get_fs() == KERNEL_DS)
>
> static inline void set_fs(mm_segment_t fs)
> {
> current_thread_info()->addr_limit = fs;
> }
>
> -#define uaccess_kernel() (get_fs() == KERNEL_DS)
> +#include <asm-generic/access_ok.h>
>
> -#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs() -size))
> -
> -#define access_ok(addr, size) \
> - __range_ok((unsigned long)addr, (unsigned long)size)
> /*
> * Single-value transfer routines. They automatically use the right
> * size if we just have the right pointer type. Note that the functions
> diff --git a/arch/nios2/include/asm/uaccess.h b/arch/nios2/include/asm/uaccess.h
> index ba9340e96fd4..9a7658df7f8d 100644
> --- a/arch/nios2/include/asm/uaccess.h
> +++ b/arch/nios2/include/asm/uaccess.h
> @@ -30,19 +30,10 @@
> #define get_fs() (current_thread_info()->addr_limit)
> #define set_fs(seg) (current_thread_info()->addr_limit = (seg))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> -#define __access_ok(addr, len) \
> - (((signed long)(((long)get_fs().seg) & \
> - ((long)(addr) | (((long)(addr)) + (len)) | (len)))) == 0)
> -
> -#define access_ok(addr, len) \
> - likely(__access_ok((unsigned long)(addr), (unsigned long)(len)))
> +#include <asm-generic/access_ok.h>
>
> # define __EX_TABLE_SECTION ".section __ex_table,\"a\"\n"
>
> -#define user_addr_max() (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -
> /*
> * Zero Userspace
> */
> diff --git a/arch/nios2/kernel/signal.c b/arch/nios2/kernel/signal.c
> index 2009ae2d3c3b..386e46443b60 100644
> --- a/arch/nios2/kernel/signal.c
> +++ b/arch/nios2/kernel/signal.c
> @@ -36,10 +36,10 @@ struct rt_sigframe {
>
> static inline int rt_restore_ucontext(struct pt_regs *regs,
> struct switch_stack *sw,
> - struct ucontext *uc, int *pr2)
> + struct ucontext __user *uc, int *pr2)
> {
> int temp;
> - unsigned long *gregs = uc->uc_mcontext.gregs;
> + unsigned long __user *gregs = uc->uc_mcontext.gregs;
> int err;
>
> /* Always make any pending restarted system calls return -EINTR */
> @@ -102,10 +102,11 @@ asmlinkage int do_rt_sigreturn(struct switch_stack *sw)
> {
> struct pt_regs *regs = (struct pt_regs *)(sw + 1);
> /* Verify, can we follow the stack back */
> - struct rt_sigframe *frame = (struct rt_sigframe *) regs->sp;
> + struct rt_sigframe __user *frame;
> sigset_t set;
> int rval;
>
> + frame = (struct rt_sigframe __user *) regs->sp;
> if (!access_ok(frame, sizeof(*frame)))
> goto badframe;
>
> @@ -124,10 +125,10 @@ asmlinkage int do_rt_sigreturn(struct switch_stack *sw)
> return 0;
> }
>
> -static inline int rt_setup_ucontext(struct ucontext *uc, struct pt_regs *regs)
> +static inline int rt_setup_ucontext(struct ucontext __user *uc, struct pt_regs *regs)
> {
> struct switch_stack *sw = (struct switch_stack *)regs - 1;
> - unsigned long *gregs = uc->uc_mcontext.gregs;
> + unsigned long __user *gregs = uc->uc_mcontext.gregs;
> int err = 0;
>
> err |= __put_user(MCONTEXT_VERSION, &uc->uc_mcontext.version);
> @@ -162,8 +163,9 @@ static inline int rt_setup_ucontext(struct ucontext *uc, struct pt_regs *regs)
> return err;
> }
>
> -static inline void *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
> - size_t frame_size)
> +static inline void __user *get_sigframe(struct ksignal *ksig,
> + struct pt_regs *regs,
> + size_t frame_size)
> {
> unsigned long usp;
>
> @@ -174,13 +176,13 @@ static inline void *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
> usp = sigsp(usp, ksig);
>
> /* Verify, is it 32 or 64 bit aligned */
> - return (void *)((usp - frame_size) & -8UL);
> + return (void __user *)((usp - frame_size) & -8UL);
> }
>
> static int setup_rt_frame(struct ksignal *ksig, sigset_t *set,
> struct pt_regs *regs)
> {
> - struct rt_sigframe *frame;
> + struct rt_sigframe __user *frame;
> int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
> 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 <asm-generic/access_ok.h>
>
> /*
> * 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/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
> index 0925bbd6db67..b68f19e11361 100644
> --- a/arch/parisc/include/asm/uaccess.h
> +++ b/arch/parisc/include/asm/uaccess.h
> @@ -17,9 +17,13 @@
> * We just let the page fault handler do the right thing. This also means
> * that put_user is the same as __put_user, etc.
> */
> -
> -#define access_ok(uaddr, size) \
> - ( (uaddr) == (uaddr) )
> +static inline int __access_ok(const void __user *addr, unsigned long size)
> +{
> + return 1;
> +}
> +#define __access_ok __access_ok
> +#define TASK_SIZE_MAX DEFAULT_TASK_SIZE
> +#include <asm-generic/access_ok.h>
>
> #define put_user __put_user
> #define get_user __get_user
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index a0032c2e7550..2e83217f52de 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -11,18 +11,9 @@
> #ifdef __powerpc64__
> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
> #define TASK_SIZE_MAX TASK_SIZE_USER64
> -#else
> -#define TASK_SIZE_MAX TASK_SIZE
> #endif
>
> -static inline bool __access_ok(unsigned long addr, unsigned long size)
> -{
> - return addr < TASK_SIZE_MAX && size <= TASK_SIZE_MAX - addr;
> -}
> -
> -#define access_ok(addr, size) \
> - (__chk_user_ptr(addr), \
> - __access_ok((unsigned long)(addr), (size)))
> +#include <asm-generic/access_ok.h>
>
> /*
> * These are the main single-value transfer routines. They automatically
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index a94b0cd0bdc5..022d23ae300b 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -112,9 +112,9 @@ static nokprobe_inline long address_ok(struct pt_regs *regs,
> {
> if (!user_mode(regs))
> return 1;
> - if (__access_ok(ea, nb))
> + if (access_ok((void __user *)ea, nb))
> return 1;
> - if (__access_ok(ea, 1))
> + if (access_ok((void __user *)ea, 1))
> /* Access overlaps the end of the user region */
> regs->dar = TASK_SIZE_MAX - 1;
> else
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 4407b9e48d2c..855450bed9f5 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -21,42 +21,13 @@
> #include <asm/byteorder.h>
> #include <asm/extable.h>
> #include <asm/asm.h>
> +#include <asm-generic/access_ok.h>
>
> #define __enable_user_access() \
> __asm__ __volatile__ ("csrs sstatus, %0" : : "r" (SR_SUM) : "memory")
> #define __disable_user_access() \
> __asm__ __volatile__ ("csrc sstatus, %0" : : "r" (SR_SUM) : "memory")
>
> -/**
> - * access_ok: - Checks if a user space pointer is valid
> - * @addr: User space pointer to start of block to check
> - * @size: Size of block to check
> - *
> - * Context: User context only. This function may sleep.
> - *
> - * Checks if a pointer to a block of memory in user space is valid.
> - *
> - * Returns true (nonzero) if the memory block may be valid, false (zero)
> - * if it is definitely invalid.
> - *
> - * Note that, depending on architecture, this function probably just
> - * checks that the pointer is in the user space range - after calling
> - * this function, memory access functions may still return -EFAULT.
> - */
> -#define access_ok(addr, size) ({ \
> - __chk_user_ptr(addr); \
> - likely(__access_ok((unsigned long __force)(addr), (size))); \
> -})
> -
> -/*
> - * Ensure that the range [addr, addr+size) is within the process's
> - * address space
> - */
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> -{
> - return size <= TASK_SIZE && addr <= TASK_SIZE - size;
> -}
> -
> /*
> * The exception table consists of pairs of addresses: the first is the
> * address of an instruction that is allowed to fault, and the second is
> diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> index 1fc075b8f764..f0c7bb98119a 100644
> --- a/arch/riscv/kernel/perf_callchain.c
> +++ b/arch/riscv/kernel/perf_callchain.c
> @@ -15,7 +15,7 @@ static unsigned long user_backtrace(struct perf_callchain_entry_ctx *entry,
> {
> struct stackframe buftail;
> unsigned long ra = 0;
> - unsigned long *user_frame_tail =
> + unsigned long __user *user_frame_tail =
> (unsigned long *)(fp - sizeof(struct stackframe));
>
> /* Check accessibility of one struct frame_tail beyond */
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index 29332edf46f0..f84d70c8e188 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -20,18 +20,13 @@
>
> void debug_user_asce(int exit);
>
> -static inline int __range_ok(unsigned long addr, unsigned long size)
> +static inline int __access_ok(const void __user *addr, unsigned long size)
> {
> return 1;
> }
> +#define __access_ok __access_ok
>
> -#define __access_ok(addr, size) \
> -({ \
> - __chk_user_ptr(addr); \
> - __range_ok((unsigned long)(addr), (size)); \
> -})
> -
> -#define access_ok(addr, size) __access_ok(addr, size)
> +#include <asm-generic/access_ok.h>
>
> unsigned long __must_check
> raw_copy_from_user(void *to, const void __user *from, unsigned long n);
> diff --git a/arch/sh/include/asm/uaccess.h b/arch/sh/include/asm/uaccess.h
> index 8867bb04b00e..ccd219d74851 100644
> --- a/arch/sh/include/asm/uaccess.h
> +++ b/arch/sh/include/asm/uaccess.h
> @@ -5,28 +5,10 @@
> #include <asm/segment.h>
> #include <asm/extable.h>
>
> -#define __addr_ok(addr) \
> - ((unsigned long __force)(addr) < current_thread_info()->addr_limit.seg)
> -
> -/*
> - * __access_ok: Check if address with size is OK or not.
> - *
> - * Uhhuh, this needs 33-bit arithmetic. We have a carry..
> - *
> - * sum := addr + size; carry? --> flag = true;
> - * if (sum >= addr_limit) flag = true;
> - */
> -#define __access_ok(addr, size) ({ \
> - unsigned long __ao_a = (addr), __ao_b = (size); \
> - unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b; \
> - __ao_end >= __ao_a && __addr_ok(__ao_end); })
> -
> -#define access_ok(addr, size) \
> - (__chk_user_ptr(addr), \
> - __access_ok((unsigned long __force)(addr), (size)))
> -
> #define user_addr_max() (current_thread_info()->addr_limit.seg)
>
> +#include <asm-generic/access_ok.h>
> +
> /*
> * Uh, these should become the main single-value transfer routines ...
> * They automatically use the right size if we just have the right
> diff --git a/arch/sparc/include/asm/uaccess.h b/arch/sparc/include/asm/uaccess.h
> index 390094200fc4..ee75f69e3fcd 100644
> --- a/arch/sparc/include/asm/uaccess.h
> +++ b/arch/sparc/include/asm/uaccess.h
> @@ -10,9 +10,6 @@
> #include <asm/uaccess_32.h>
> #endif
>
> -#define user_addr_max() \
> - (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -
> long strncpy_from_user(char *dest, const char __user *src, long count);
>
> #endif
> diff --git a/arch/sparc/include/asm/uaccess_32.h b/arch/sparc/include/asm/uaccess_32.h
> index 4a12346bb69c..367747116260 100644
> --- a/arch/sparc/include/asm/uaccess_32.h
> +++ b/arch/sparc/include/asm/uaccess_32.h
> @@ -25,17 +25,7 @@
> #define get_fs() (current->thread.current_ds)
> #define set_fs(val) ((current->thread.current_ds) = (val))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> -/* We have there a nice not-mapped page at PAGE_OFFSET - PAGE_SIZE, so that this test
> - * can be fairly lightweight.
> - * No one can read/write anything from userland in the kernel space by setting
> - * large size and address near to PAGE_OFFSET - a fault will break his intentions.
> - */
> -#define __user_ok(addr, size) ({ (void)(size); (addr) < STACK_TOP; })
> -#define __kernel_ok (uaccess_kernel())
> -#define __access_ok(addr, size) (__user_ok((addr) & get_fs().seg, (size)))
> -#define access_ok(addr, size) __access_ok((unsigned long)(addr), size)
> +#include <asm-generic/access_ok.h>
>
> /* Uh, these should become the main single-value transfer routines..
> * They automatically use the right size if we just have the right
> @@ -47,13 +37,13 @@
> * and hide all the ugliness from the user.
> */
> #define put_user(x, ptr) ({ \
> - unsigned long __pu_addr = (unsigned long)(ptr); \
> + void __user *__pu_addr = (ptr); \
> __chk_user_ptr(ptr); \
> __put_user_check((__typeof__(*(ptr)))(x), __pu_addr, sizeof(*(ptr))); \
> })
>
> #define get_user(x, ptr) ({ \
> - unsigned long __gu_addr = (unsigned long)(ptr); \
> + const void __user *__gu_addr = (ptr); \
> __chk_user_ptr(ptr); \
> __get_user_check((x), __gu_addr, sizeof(*(ptr)), __typeof__(*(ptr))); \
> })
> @@ -232,7 +222,7 @@ static inline unsigned long __clear_user(void __user *addr, unsigned long size)
>
> static inline unsigned long clear_user(void __user *addr, unsigned long n)
> {
> - if (n && __access_ok((unsigned long) addr, n))
> + if (n && __access_ok(addr, n))
> return __clear_user(addr, n);
> else
> return n;
> diff --git a/arch/sparc/include/asm/uaccess_64.h b/arch/sparc/include/asm/uaccess_64.h
> index 5c12fb46bc61..000bac67cf31 100644
> --- a/arch/sparc/include/asm/uaccess_64.h
> +++ b/arch/sparc/include/asm/uaccess_64.h
> @@ -31,7 +31,12 @@
>
> #define get_fs() ((mm_segment_t){(current_thread_info()->current_ds)})
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> +static inline int __access_ok(const void __user *addr, unsigned long size)
> +{
> + return 1;
> +}
> +#define __access_ok __access_ok
> +#include <asm-generic/access_ok.h>
>
> #define set_fs(val) \
> do { \
> @@ -43,33 +48,7 @@ do { \
> * Test whether a block of memory is a valid user space address.
> * Returns 0 if the range is valid, nonzero otherwise.
> */
> -static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, unsigned long limit)
> -{
> - if (__builtin_constant_p(size))
> - return addr > limit - size;
> -
> - addr += size;
> - if (addr < size)
> - return true;
> -
> - return addr > limit;
> -}
> -
> -#define __range_not_ok(addr, size, limit) \
> -({ \
> - __chk_user_ptr(addr); \
> - __chk_range_not_ok((unsigned long __force)(addr), size, limit); \
> -})
> -
> -static inline int __access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> -
> -static inline int access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
>
> void __retl_efault(void);
>
> diff --git a/arch/sparc/kernel/signal_32.c b/arch/sparc/kernel/signal_32.c
> index ffab16369bea..74f80443b195 100644
> --- a/arch/sparc/kernel/signal_32.c
> +++ b/arch/sparc/kernel/signal_32.c
> @@ -65,7 +65,7 @@ struct rt_signal_frame {
> */
> static inline bool invalid_frame_pointer(void __user *fp, int fplen)
> {
> - if ((((unsigned long) fp) & 15) || !__access_ok((unsigned long)fp, fplen))
> + if ((((unsigned long) fp) & 15) || !access_ok(fp, fplen))
> return true;
>
> return false;
> diff --git a/arch/um/include/asm/uaccess.h b/arch/um/include/asm/uaccess.h
> index 1ecfc96bcc50..7d9d60e41e4e 100644
> --- a/arch/um/include/asm/uaccess.h
> +++ b/arch/um/include/asm/uaccess.h
> @@ -25,7 +25,7 @@
> extern unsigned long raw_copy_from_user(void *to, const void __user *from, unsigned long n);
> extern unsigned long raw_copy_to_user(void __user *to, const void *from, unsigned long n);
> extern unsigned long __clear_user(void __user *mem, unsigned long len);
> -static inline int __access_ok(unsigned long addr, unsigned long size);
> +static inline int __access_ok(const void __user *ptr, unsigned long size);
>
> /* Teach asm-generic/uaccess.h that we have C functions for these. */
> #define __access_ok __access_ok
> @@ -36,8 +36,9 @@ static inline int __access_ok(unsigned long addr, unsigned long size);
>
> #include <asm-generic/uaccess.h>
>
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> +static inline int __access_ok(const void __user *ptr, unsigned long size)
> {
> + unsigned long addr = (unsigned long)ptr;
> return __addr_range_nowrap(addr, size) &&
> (__under_task_size(addr, size) ||
> __access_ok_vsyscall(addr, size));
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index c6d9dc42724d..c5e4bb7161bc 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -12,18 +12,6 @@
> #include <asm/smap.h>
> #include <asm/extable.h>
>
> -/*
> - * Test whether a block of memory is a valid user space address.
> - * Returns 0 if the range is valid, nonzero otherwise.
> - */
> -static inline bool __access_ok(void __user *ptr, unsigned long size)
> -{
> - unsigned long limit = TASK_SIZE_MAX;
> - unsigned long addr = ptr;
> -
> - return (size <= limit) && (addr <= (limit - size));
> -}
> -
> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> static inline bool pagefault_disabled(void);
> # define WARN_ON_IN_IRQ() \
> @@ -55,6 +43,8 @@ static inline bool pagefault_disabled(void);
> likely(__access_ok(addr, size));\
> })
>
> +#include <asm-generic/access_ok.h>
> +
> #define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
> #define __chk_range_not_ok(addr, size, limit) (!__access_ok((void __user *)addr, size))
>
> diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
> index 75bd8fbf52ba..0edd9e4b23d0 100644
> --- a/arch/xtensa/include/asm/uaccess.h
> +++ b/arch/xtensa/include/asm/uaccess.h
> @@ -35,15 +35,7 @@
> #define get_fs() (current->thread.current_ds)
> #define set_fs(val) (current->thread.current_ds = (val))
>
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -
> -#define __kernel_ok (uaccess_kernel())
> -#define __user_ok(addr, size) \
> - (((size) <= TASK_SIZE)&&((addr) <= TASK_SIZE-(size)))
> -#define __access_ok(addr, size) (__kernel_ok || __user_ok((addr), (size)))
> -#define access_ok(addr, size) __access_ok((unsigned long)(addr), (size))
> -
> -#define user_addr_max() (uaccess_kernel() ? ~0UL : TASK_SIZE)
> +#include <asm-generic/access_ok.h>
>
> /*
> * These are the main single-value transfer routines. They
> diff --git a/include/asm-generic/access_ok.h b/include/asm-generic/access_ok.h
> new file mode 100644
> index 000000000000..883b573af5fe
> --- /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_MAX TASK_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();
> + unsigned long addr = (unsigned long)ptr;
> +
> + if (limit == ULONG_MAX)
> + return true;
> +
> + return (size <= limit) && (addr <= (limit - size));
> +}
> +#endif
> +
> +#ifndef access_ok
> +#define access_ok(addr, size) likely(__access_ok(addr, size))
> +#endif
> +
> +#endif
> diff --git a/include/asm-generic/uaccess.h b/include/asm-generic/uaccess.h
> index 0870fa11a7c5..ebc685dc8d74 100644
> --- a/include/asm-generic/uaccess.h
> +++ b/include/asm-generic/uaccess.h
> @@ -114,28 +114,9 @@ static inline void set_fs(mm_segment_t fs)
> }
> #endif
>
> -#ifndef uaccess_kernel
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -#endif
> -
> -#ifndef user_addr_max
> -#define user_addr_max() (uaccess_kernel() ? ~0UL : TASK_SIZE)
> -#endif
> -
> #endif /* CONFIG_SET_FS */
>
> -#define access_ok(addr, size) __access_ok((unsigned long)(addr),(size))
> -
> -/*
> - * The architecture should really override this if possible, at least
> - * doing a check on the get_fs()
> - */
> -#ifndef __access_ok
> -static inline int __access_ok(unsigned long addr, unsigned long size)
> -{
> - return 1;
> -}
> -#endif
> +#include <asm-generic/access_ok.h>
>
> /*
> * These are the main single-value transfer routines. They automatically
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 67e9bc94dc40..2c31667e62e0 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -33,13 +33,6 @@ typedef struct {
> /* empty dummy */
> } mm_segment_t;
>
> -#ifndef TASK_SIZE_MAX
> -#define TASK_SIZE_MAX TASK_SIZE
> -#endif
> -
> -#define uaccess_kernel() (false)
> -#define user_addr_max() (TASK_SIZE_MAX)
> -
> static inline mm_segment_t force_uaccess_begin(void)
> {
> return (mm_segment_t) { };
> --
> 2.29.2
>
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Mark Rutland @ 2022-02-15 10:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, linux-ia64, Linux-sh list, Peter Zijlstra,
open list:MIPS, Linux Memory Management List, Guo Ren,
open list:SPARC + UltraSPARC (sparc/sparc64),
open list:QUALCOMM HEXAGON..., linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, open list:S390, Brian Cain,
Helge Deller, X86 ML, Russell King, linux-csky, Ard Biesheuvel,
Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE, Robin Murphy,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, open list:PARISC ARCHITECTURE, Nick Hu,
Max Filippov, Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <CAK8P3a0NTqK_m7q909d8FN6is8k4_u3zeckC9XOrjEi7kqSvmg@mail.gmail.com>
On Tue, Feb 15, 2022 at 10:39:46AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 15, 2022 at 10:21 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Tue, 15 Feb 2022 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > arm64 also has this leading up to the range check, and I think we'd no
> > longer need it:
> >
> > if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
> > (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
> > addr = untagged_addr(addr);
>
> I suspect the expensive part here is checking the two flags, as untagged_addr()
> seems to always just add a sbfx instruction. Would this work?
>
> #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
> #define access_ok(ptr, size) __access_ok(untagged_addr(ptr), (size))
> #else // the else path is the default, this can be left out.
> #define access_ok(ptr, size) __access_ok((ptr), (size))
> #endif
This would be an ABI change, e.g. for tasks without TIF_TAGGED_ADDR.
I don't think we should change this as part of this series.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Mark Rutland @ 2022-02-15 10:37 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Rich Felker, linux-ia64, Linux-sh list, Peter Zijlstra,
open list:MIPS, Linux Memory Management List, Guo Ren,
open list:SPARC + UltraSPARC (sparc/sparc64),
open list:QUALCOMM HEXAGON..., linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, open list:S390, Brian Cain,
Helge Deller, X86 ML, Russell King, linux-csky, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
Robin Murphy, open list:TENSILICA XTENSA PORT (xtensa),
Arnd Bergmann, Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Arnd Bergmann,
Michal Simek, Thomas Bogendoerfer, open list:PARISC ARCHITECTURE,
Nick Hu, Max Filippov, Linux API, Linux Kernel Mailing List,
Dinh Nguyen, Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <CAMj1kXGkG0KMD2rnKAJc3V7X9LP1grbcHTNYMnj_q4GiYfG2pQ@mail.gmail.com>
On Tue, Feb 15, 2022 at 10:21:16AM +0100, Ard Biesheuvel wrote:
> On Tue, 15 Feb 2022 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > On Tue, Feb 15, 2022 at 9:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > >
> > >
> > > With set_fs() out of the picture, wouldn't it be sufficient to check
> > > that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
> > > That would also remove the need to strip the tag from the address.
> > >
> > > Something like
> > >
> > > asm goto("tbnz %0, #55, %2 \n"
> > > "tbnz %1, #55, %2 \n"
> > > :: "r"(addr), "r"(addr + size - 1) :: notok);
> > > return 1;
> > > notok:
> > > return 0;
> > >
> > > with an additional sanity check on the size which the compiler could
> > > eliminate for compile-time constant values.
> >
> > That should work, but I don't see it as a clear enough advantage to
> > have a custom implementation. For the constant-size case, it probably
> > isn't better than a compiler-scheduled comparison against a
> > constant limit, but it does hurt maintainability when the next person
> > wants to change the behavior of access_ok() globally.
> >
>
> arm64 also has this leading up to the range check, and I think we'd no
> longer need it:
>
> if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
> (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
> addr = untagged_addr(addr);
>
ABI-wise, we aim to *reject* tagged pointers unless the task is using the
tagged addr ABI, so we need to retain both the untagging logic and the full
pointer check (to actually check the tag bits) unless we relax that ABI
decision generally (or go context-switch the TCR_EL1.TBI* bits).
Since that has subtle ABI implications, I don't think we should change that
within this series.
If we *did* relax things, we could just check bit 55 here, and unconditionally
clear that in uaccess_mask_ptr(), since LDTR/STTR should fault on kernel memory.
On parts with meltdown those might not fault until committed, and so we need
masking to avoid speculative access to a kernel pointer, and that requires the
prior explciit check.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH 03/14] nds32: fix access_ok() checks in get/put_user
From: Greg KH @ 2022-02-15 10:25 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Christoph Hellwig, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, linuxppc-dev, Richard Weinberger,
linux-m68k, Openrisc, Greentime Hu, Stafford Horne, Linux ARM,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, # 3.4.x, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, Linus Torvalds,
David Miller
In-Reply-To: <CAK8P3a1XkWNQcFEhJQ0+qWzih1YRQDS_N8xiosN7FHn3yoTJpQ@mail.gmail.com>
On Tue, Feb 15, 2022 at 10:18:15AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 14, 2022 at 6:01 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The get_user()/put_user() functions are meant to check for
> > > access_ok(), while the __get_user()/__put_user() functions
> > > don't.
> > >
> > > This broke in 4.19 for nds32, when it gained an extraneous
> > > check in __get_user(), but lost the check it needs in
> > > __put_user().
> >
> > Can we follow the lead of MIPS (which this was originally copied
> > from I think) and kill the pointless __get/put_user_check wrapper
> > that just obsfucate the code?
>
> I had another look, but I think that would be a bigger change than
> I want to have in a fix for stable backports, as nds32 also uses
> the _check versions in __{get,put}_user_error.
Don't worry about stable backports first, get it correct and merged and
then worry about them if you really have to.
If someone cares about nds32 for stable kernels, they can do the
backport work :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 09/14] m68k: drop custom __access_ok()
From: Arnd Bergmann @ 2022-02-15 10:02 UTC (permalink / raw)
To: Al Viro
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Linux-MM,
Guo Ren, sparclinux, open list:QUALCOMM HEXAGON..., linux-riscv,
Will Deacon, Christoph Hellwig, linux-arch, linux-s390,
Brian Cain, Helge Deller, the arch/x86 maintainers,
Russell King - ARM Linux, linux-csky, Ard Biesheuvel, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, alpha, linux-um, linuxppc-dev, linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Parisc List, Nick Hu, Max Filippov,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David Miller
In-Reply-To: <YgtSpk0boDjsyjFK@zeniv-ca.linux.org.uk>
On Tue, Feb 15, 2022 at 8:13 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Feb 15, 2022 at 07:29:42AM +0100, Christoph Hellwig wrote:
> > On Tue, Feb 15, 2022 at 12:37:41AM +0000, Al Viro wrote:
> > > Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
> > > (and trim the comment down to "coldfire and 68000 will pick generic
> > > variant")?
> >
> > I wonder if we should invert CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE,
> > select the separate address space config for s390, sparc64, non-coldfire
> > m68k and mips with EVA and then just have one single access_ok for
> > overlapping address space (as added by Arnd) and non-overlapping ones
> > (always return true).
>
> parisc is also such... How about
>
> select ALTERNATE_SPACE_USERLAND
>
> for that bunch?
Either of those works for me. My current version has this keyed off
TASK_SIZE_MAX==ULONG_MAX, but a CONFIG_ symbol does
look more descriptive.
> While we are at it, how many unusual access_ok() instances are
> left after this series? arm64, itanic, um, anything else?
x86 adds a WARN_ON_IN_IRQ() check in there. This could be
made generic, but it's not obvious what exactly the exceptions are
that other architectures need. The arm64 tagged pointers could
probably also get integrated into the generic version.
> FWIW, sparc32 has a slightly unusual instance (see uaccess_32.h there); it's
> obviously cheaper than generic and I wonder if the trick is legitimate (and
> applicable elsewhere, perhaps)...
Right, a few others have the same, but I wasn't convinced that this
is actually safe for call possible cases: it's trivial to construct a caller
that works on other architectures but not this one, if you pass a large
enough size value and don't access the contents in sequence.
Also, like the ((addr | (addr + size)) & MASK) check on some other
architectures, it is less portable because it makes assumptions about
the actual layout beyond a fixed address limit.
Arnd
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Arnd Bergmann @ 2022-02-15 9:39 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:MIPS, Linux Memory Management List,
Guo Ren, open list:SPARC + UltraSPARC (sparc/sparc64),
open list:QUALCOMM HEXAGON..., linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, open list:S390, Brian Cain,
Helge Deller, X86 ML, Russell King, linux-csky, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
Robin Murphy, open list:TENSILICA XTENSA PORT (xtensa),
Arnd Bergmann, Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, open list:PARISC ARCHITECTURE, Nick Hu,
Max Filippov, Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <CAMj1kXGkG0KMD2rnKAJc3V7X9LP1grbcHTNYMnj_q4GiYfG2pQ@mail.gmail.com>
On Tue, Feb 15, 2022 at 10:21 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Tue, 15 Feb 2022 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote:
>
> arm64 also has this leading up to the range check, and I think we'd no
> longer need it:
>
> if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
> (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
> addr = untagged_addr(addr);
I suspect the expensive part here is checking the two flags, as untagged_addr()
seems to always just add a sbfx instruction. Would this work?
#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
#define access_ok(ptr, size) __access_ok(untagged_addr(ptr), (size))
#else // the else path is the default, this can be left out.
#define access_ok(ptr, size) __access_ok((ptr), (size))
#endif
Arnd
^ permalink raw reply
* Re: [PATCH v2] i2c: pasemi: Drop I2C classes from platform driver variant
From: Wolfram Sang @ 2022-02-15 9:31 UTC (permalink / raw)
To: Martin Povišer
Cc: Sven Peter, Hector Martin, linux-kernel, Paul Mackerras,
linux-i2c, linuxppc-dev, Alyssa Rosenzweig
In-Reply-To: <20220204095914.5678-1-povik+lin@cutebit.org>
[-- Attachment #1: Type: text/plain, Size: 512 bytes --]
On Fri, Feb 04, 2022 at 10:59:14AM +0100, Martin Povišer wrote:
> Drop I2C device-probing classes from platform variant of the PASemi
> controller as it is only used on platforms where I2C devices should
> be instantiated in devicetree. (The I2C_CLASS_DEPRECATED flag is not
> raised as up to this point no devices relied on the old behavior.)
>
> Fixes: d88ae2932df0 ("i2c: pasemi: Add Apple platform driver")
> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH 08/14] arm64: simplify access_ok()
From: David Laight @ 2022-02-15 9:30 UTC (permalink / raw)
To: 'Ard Biesheuvel', Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64@vger.kernel.org,
linux-sh@vger.kernel.org, Peter Zijlstra,
Linux Kernel Mailing List, Linux Memory Management List, Guo Ren,
open list:SPARC + UltraSPARC (sparc/sparc64), linux-riscv,
linux-api@vger.kernel.org, Will Deacon, Christoph Hellwig,
linux-arch, open list:S390, Brian Cain,
linux-hexagon@vger.kernel.org, Helge Deller, X86 ML, Russell King,
linux-csky@vger.kernel.org, Linus Torvalds, Ingo Molnar,
Geert Uytterhoeven, linux-snps-arc@lists.infradead.org,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, Richard Weinberger, linux-m68k,
openrisc@lists.librecores.org, Greentime Hu, Stafford Horne,
Linux ARM, monstr@monstr.eu, Thomas Bogendoerfer, Nick Hu,
open list:PARISC ARCHITECTURE, Max Filippov,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), open list:MIPS,
dinguyen@kernel.org, Eric W. Biederman, alpha, Andrew Morton,
Robin Murphy, David S. Miller
In-Reply-To: <CAMj1kXHixUFjV=4m3tzfGz7AiRWc-VczymbKuZq7dyZZNuLKxQ@mail.gmail.com>
From: Ard Biesheuvel
> Sent: 15 February 2022 08:18
>
> On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > From: Arnd Bergmann <arnd@arndb.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@arndb.de>
> > ---
> > 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
> >
>
> With set_fs() out of the picture, wouldn't it be sufficient to check
> that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
> That would also remove the need to strip the tag from the address.
>
> Something like
>
> asm goto("tbnz %0, #55, %2 \n"
> "tbnz %1, #55, %2 \n"
> :: "r"(addr), "r"(addr + size - 1) :: notok);
> return 1;
> notok:
> return 0;
>
> with an additional sanity check on the size which the compiler could
> eliminate for compile-time constant values.
Is there are reason not to just use:
size < 1u << 48 && !((addr | (addr + size - 1)) & 1u << 55)
(The -1 can be removed if the last user page is never mapped)
Ugg, is arm64 addressing as horrid as it looks - with the 'kernel'
bit in the middle of the virtual address space?
It seems to be:
<zero:4><tag:4><kernel:1><ignored:7><address:48>
Although I found some references to 44 bit VA and to code using the
'ignored' bits as tags - relying on the hardware ignoring them.
There might be some feature that uses the top 4 bits as well.
Another option is assuming that accesses are 'reasonably sequential',
removing the length check and ensuring there is an unmapped page
between valid user and kernel addresses.
That probably requires and unmapped page at the bottom of kernel space
which may not be achievable.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Ard Biesheuvel @ 2022-02-15 9:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:MIPS, Linux Memory Management List,
Guo Ren, open list:SPARC + UltraSPARC (sparc/sparc64),
open list:QUALCOMM HEXAGON..., linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, open list:S390, Brian Cain,
Helge Deller, X86 ML, Russell King, linux-csky, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
Robin Murphy, open list:TENSILICA XTENSA PORT (xtensa),
Arnd Bergmann, Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, open list:PARISC ARCHITECTURE, Nick Hu,
Max Filippov, Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <CAK8P3a2VfvDkueaJNTA9SiB+PFsi_Q17AX+aL46ueooW2ahmQw@mail.gmail.com>
On Tue, 15 Feb 2022 at 10:13, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Tue, Feb 15, 2022 at 9:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> >
> > With set_fs() out of the picture, wouldn't it be sufficient to check
> > that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
> > That would also remove the need to strip the tag from the address.
> >
> > Something like
> >
> > asm goto("tbnz %0, #55, %2 \n"
> > "tbnz %1, #55, %2 \n"
> > :: "r"(addr), "r"(addr + size - 1) :: notok);
> > return 1;
> > notok:
> > return 0;
> >
> > with an additional sanity check on the size which the compiler could
> > eliminate for compile-time constant values.
>
> That should work, but I don't see it as a clear enough advantage to
> have a custom implementation. For the constant-size case, it probably
> isn't better than a compiler-scheduled comparison against a
> constant limit, but it does hurt maintainability when the next person
> wants to change the behavior of access_ok() globally.
>
arm64 also has this leading up to the range check, and I think we'd no
longer need it:
if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
(current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
addr = untagged_addr(addr);
> If we want to get into micro-optimizing uaccess, I think a better target
> would be a CONFIG_CC_HAS_ASM_GOTO_OUTPUT version
> of __get_user()/__put_user as we have on x86 and powerpc.
>
> Arnd
^ permalink raw reply
* Re: [PATCH 03/14] nds32: fix access_ok() checks in get/put_user
From: Arnd Bergmann @ 2022-02-15 9:18 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, linuxppc-dev, Richard Weinberger,
linux-m68k, Openrisc, Greentime Hu, Stafford Horne, Linux ARM,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, # 3.4.x, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, Linus Torvalds,
David Miller
In-Reply-To: <YgqK1ihlJvRFHJ9h@infradead.org>
On Mon, Feb 14, 2022 at 6:01 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The get_user()/put_user() functions are meant to check for
> > access_ok(), while the __get_user()/__put_user() functions
> > don't.
> >
> > This broke in 4.19 for nds32, when it gained an extraneous
> > check in __get_user(), but lost the check it needs in
> > __put_user().
>
> Can we follow the lead of MIPS (which this was originally copied
> from I think) and kill the pointless __get/put_user_check wrapper
> that just obsfucate the code?
I had another look, but I think that would be a bigger change than
I want to have in a fix for stable backports, as nds32 also uses
the _check versions in __{get,put}_user_error.
If we instead clean it up in a separate patch, it should be done for
all eight architectures that do the same thing, but at that point,
the time seems better spent at coming up with a new set of
calling conventions that work with asm-goto.
Arnd
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Arnd Bergmann @ 2022-02-15 9:12 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:MIPS, Linux Memory Management List,
Guo Ren, open list:SPARC + UltraSPARC (sparc/sparc64),
open list:QUALCOMM HEXAGON..., linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, open list:S390, Brian Cain,
Helge Deller, X86 ML, Russell King, linux-csky, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
Robin Murphy, open list:TENSILICA XTENSA PORT (xtensa),
Arnd Bergmann, Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, open list:PARISC ARCHITECTURE, Nick Hu,
Max Filippov, Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <CAMj1kXHixUFjV=4m3tzfGz7AiRWc-VczymbKuZq7dyZZNuLKxQ@mail.gmail.com>
On Tue, Feb 15, 2022 at 9:17 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
>
> With set_fs() out of the picture, wouldn't it be sufficient to check
> that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
> That would also remove the need to strip the tag from the address.
>
> Something like
>
> asm goto("tbnz %0, #55, %2 \n"
> "tbnz %1, #55, %2 \n"
> :: "r"(addr), "r"(addr + size - 1) :: notok);
> return 1;
> notok:
> return 0;
>
> with an additional sanity check on the size which the compiler could
> eliminate for compile-time constant values.
That should work, but I don't see it as a clear enough advantage to
have a custom implementation. For the constant-size case, it probably
isn't better than a compiler-scheduled comparison against a
constant limit, but it does hurt maintainability when the next person
wants to change the behavior of access_ok() globally.
If we want to get into micro-optimizing uaccess, I think a better target
would be a CONFIG_CC_HAS_ASM_GOTO_OUTPUT version
of __get_user()/__put_user as we have on x86 and powerpc.
Arnd
^ permalink raw reply
* Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
From: Christophe Leroy @ 2022-02-15 8:33 UTC (permalink / raw)
To: Naveen N. Rao, Jiri Kosina, Joe Lawrence, Josh Poimboeuf,
Miroslav Benes, Ingo Molnar, Petr Mladek, Steven Rostedt
Cc: live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1644860537.hyunv1mld0.naveen@linux.ibm.com>
Le 14/02/2022 à 18:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> PPC64 mprofile versions and PPC32 are very similar.
>>
>> Modify PPC64 version so that if can be reused for PPC32.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
>> 1 file changed, 51 insertions(+), 22 deletions(-)
>
> While I agree that ppc32 and -mprofile-kernel ftrace code are very
> similar, I think this patch adds way too many #ifdefs. IMHO, this
> makes the resultant code quite difficult to follow.
Ok, I can introduce some GAS macros for a few of them in a followup patch.
Christophe
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Ard Biesheuvel @ 2022-02-15 8:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
open list:MIPS, Linux Memory Management List, Guo Ren,
open list:SPARC + UltraSPARC (sparc/sparc64), linux-hexagon,
linux-riscv, Will Deacon, Christoph Hellwig, linux-arch,
open list:S390, Brian Cain, Helge Deller, X86 ML, Russell King,
linux-csky, Ingo Molnar, Geert Uytterhoeven, linux-snps-arc,
Robin Murphy, open list:TENSILICA XTENSA PORT (xtensa),
Arnd Bergmann, Heiko Carstens, alpha, linux-um,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-m68k,
openrisc, Greentime Hu, Stafford Horne, Linux ARM, monstr,
Thomas Bogendoerfer, open list:PARISC ARCHITECTURE, Nick Hu,
Max Filippov, linux-api, Linux Kernel Mailing List, dinguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David S. Miller
In-Reply-To: <20220214163452.1568807-9-arnd@kernel.org>
On Mon, 14 Feb 2022 at 17:37, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.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@arndb.de>
> ---
> 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
>
With set_fs() out of the picture, wouldn't it be sufficient to check
that bit #55 is clear? (the bit that selects between TTBR0 and TTBR1)
That would also remove the need to strip the tag from the address.
Something like
asm goto("tbnz %0, #55, %2 \n"
"tbnz %1, #55, %2 \n"
:: "r"(addr), "r"(addr + size - 1) :: notok);
return 1;
notok:
return 0;
with an additional sanity check on the size which the compiler could
eliminate for compile-time constant values.
^ permalink raw reply
* Re: [PATCH 14/14] uaccess: drop set_fs leftovers
From: Arnd Bergmann @ 2022-02-15 8:10 UTC (permalink / raw)
To: Helge Deller
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Linux-MM,
Guo Ren, sparclinux, open list:QUALCOMM HEXAGON..., linux-riscv,
Will Deacon, Christoph Hellwig, linux-arch, linux-s390,
Brian Cain, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Ard Biesheuvel, Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, alpha, linux-um, linuxppc-dev, linux-m68k,
Openrisc, Al Viro, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Parisc List, Nick Hu, Max Filippov,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David Miller, Greentime Hu
In-Reply-To: <215c0ddc-54b1-bcb1-c5aa-bd17c6b100a8@gmx.de>
On Tue, Feb 15, 2022 at 8:46 AM Helge Deller <deller@gmx.de> wrote:
>
> On 2/15/22 04:03, Al Viro wrote:
> > On Mon, Feb 14, 2022 at 05:34:52PM +0100, Arnd Bergmann wrote:
> >> diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
> >> index b5835325d44b..2f4a1b1ef387 100644
> >> --- a/arch/parisc/include/asm/futex.h
> >> +++ b/arch/parisc/include/asm/futex.h
> >> @@ -99,7 +99,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> >> /* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is
> >> * our gateway page, and causes no end of trouble...
> >> */
> >> - if (uaccess_kernel() && !uaddr)
> >> + if (!uaddr)
> >> return -EFAULT;
> >
> > Huh? uaccess_kernel() is removed since it becomes always false now,
> > so this looks odd.
> >
> > AFAICS, the comment above that check refers to futex_detect_cmpxchg()
> > -> cmpxchg_futex_value_locked() -> futex_atomic_cmpxchg_inatomic() call chain.
> > Which had been gone since commit 3297481d688a (futex: Remove futex_cmpxchg
> > detection). The comment *and* the check should've been killed off back
> > then.
> > Let's make sure to get both now...
>
> Right. Arnd, can you drop this if() and the comment above it?
Done.
Arnd
^ permalink raw reply
* Re: [PATCH v2 09/13] powerpc/ftrace: Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS
From: Christophe Leroy @ 2022-02-15 8:00 UTC (permalink / raw)
To: Naveen N. Rao, Jiri Kosina, Joe Lawrence, Josh Poimboeuf,
Miroslav Benes, Ingo Molnar, Petr Mladek, Steven Rostedt
Cc: live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1644852011.qg7ud9elo2.naveen@linux.ibm.com>
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
>> of livepatching.
>>
>> Also note that powerpc being the last one to convert to
>> CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
>> klp_arch_set_pc() on all architectures.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
>> arch/powerpc/include/asm/livepatch.h | 4 +---
>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index cdac2115eb00..e2b1792b2aae 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -210,6 +210,7 @@ config PPC
>> select HAVE_DEBUG_KMEMLEAK
>> select HAVE_DEBUG_STACKOVERFLOW
>> select HAVE_DYNAMIC_FTRACE
>> + select HAVE_DYNAMIC_FTRACE_WITH_ARGS if MPROFILE_KERNEL || PPC32
>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if MPROFILE_KERNEL || PPC32
>> select HAVE_EBPF_JIT
>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN
>> && POWER7_CPU)
>> diff --git a/arch/powerpc/include/asm/ftrace.h
>> b/arch/powerpc/include/asm/ftrace.h
>> index b3f6184f77ea..45c3d6f11daa 100644
>> --- a/arch/powerpc/include/asm/ftrace.h
>> +++ b/arch/powerpc/include/asm/ftrace.h
>> @@ -22,6 +22,23 @@ static inline unsigned long
>> ftrace_call_adjust(unsigned long addr)
>> struct dyn_arch_ftrace {
>> struct module *mod;
>> };
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
>> +struct ftrace_regs {
>> + struct pt_regs regs;
>> +};
>> +
>> +static __always_inline struct pt_regs *arch_ftrace_get_regs(struct
>> ftrace_regs *fregs)
>> +{
>> + return &fregs->regs;
>> +}
>
> I think this is wrong. We need to differentiate between ftrace_caller()
> and ftrace_regs_caller() here, and only return pt_regs if coming in
> through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
>
>> +
>> +static __always_inline void ftrace_instruction_pointer_set(struct
>> ftrace_regs *fregs,
>> + unsigned long ip)
>> +{
>> + regs_set_return_ip(&fregs->regs, ip);
>
> Should we use that helper here? regs_set_return_ip() also updates some
> other state related to taking interrupts and I don't think it makes
> sense for use with ftrace.
>
Today we have:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
struct pt_regs *regs = ftrace_get_regs(fregs);
regs_set_return_ip(regs, ip);
}
Which like x86 and s390 becomes:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
ftrace_instruction_pointer_set(fregs, ip);
}
That's the reason why I've been using regs_set_return_ip(). Do you think
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
registers if they are still valid")
Christophe
^ permalink raw reply
* Re: [PATCH 14/14] uaccess: drop set_fs leftovers
From: Helge Deller @ 2022-02-15 7:46 UTC (permalink / raw)
To: Al Viro, Arnd Bergmann
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
linux-mm, guoren, sparclinux, linux-hexagon, linux-riscv, will,
Christoph Hellwig, linux-arch, linux-s390, bcain, x86, linux,
linux-csky, ardb, mingo, geert, linux-snps-arc, linux-xtensa,
arnd, hca, linux-alpha, linux-um, linuxppc-dev, linux-m68k,
openrisc, green.hu, shorne, linux-arm-kernel, monstr, tsbogend,
linux-parisc, nickhu, jcmvbkbc, linux-api, linux-kernel, dinguyen,
ebiederm, richard, akpm, Linus Torvalds, davem
In-Reply-To: <YgsYD2nW9GjWJtn5@zeniv-ca.linux.org.uk>
On 2/15/22 04:03, Al Viro wrote:
> On Mon, Feb 14, 2022 at 05:34:52PM +0100, Arnd Bergmann wrote:
>> diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
>> index b5835325d44b..2f4a1b1ef387 100644
>> --- a/arch/parisc/include/asm/futex.h
>> +++ b/arch/parisc/include/asm/futex.h
>> @@ -99,7 +99,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>> /* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is
>> * our gateway page, and causes no end of trouble...
>> */
>> - if (uaccess_kernel() && !uaddr)
>> + if (!uaddr)
>> return -EFAULT;
>
> Huh? uaccess_kernel() is removed since it becomes always false now,
> so this looks odd.
>
> AFAICS, the comment above that check refers to futex_detect_cmpxchg()
> -> cmpxchg_futex_value_locked() -> futex_atomic_cmpxchg_inatomic() call chain.
> Which had been gone since commit 3297481d688a (futex: Remove futex_cmpxchg
> detection). The comment *and* the check should've been killed off back
> then.
> Let's make sure to get both now...
Right. Arnd, can you drop this if() and the comment above it?
Thanks,
Helge
^ permalink raw reply
* Re: [PATCH 09/14] m68k: drop custom __access_ok()
From: Al Viro @ 2022-02-15 7:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
linux-mm, guoren, sparclinux, linux-hexagon, linux-riscv, will,
ardb, linux-arch, linux-s390, bcain, deller, x86, linux,
linux-csky, mingo, geert, linux-snps-arc, linux-xtensa, arnd, hca,
linux-alpha, linux-um, linuxppc-dev, linux-m68k, openrisc,
green.hu, shorne, linux-arm-kernel, Arnd Bergmann, monstr,
tsbogend, linux-parisc, nickhu, jcmvbkbc, linux-api, linux-kernel,
dinguyen, ebiederm, richard, akpm, Linus Torvalds, davem
In-Reply-To: <20220215062942.GA12551@lst.de>
On Tue, Feb 15, 2022 at 07:29:42AM +0100, Christoph Hellwig wrote:
> On Tue, Feb 15, 2022 at 12:37:41AM +0000, Al Viro wrote:
> > Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
> > (and trim the comment down to "coldfire and 68000 will pick generic
> > variant")?
>
> I wonder if we should invert CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE,
> select the separate address space config for s390, sparc64, non-coldfire
> m68k and mips with EVA and then just have one single access_ok for
> overlapping address space (as added by Arnd) and non-overlapping ones
> (always return true).
parisc is also such... How about
select ALTERNATE_SPACE_USERLAND
for that bunch? While we are at it, how many unusual access_ok() instances are
left after this series? arm64, itanic, um, anything else?
FWIW, sparc32 has a slightly unusual instance (see uaccess_32.h there); it's
obviously cheaper than generic and I wonder if the trick is legitimate (and
applicable elsewhere, perhaps)...
^ permalink raw reply
* Re: [PATCH 09/14] m68k: drop custom __access_ok()
From: Christoph Hellwig @ 2022-02-15 6:29 UTC (permalink / raw)
To: Al Viro
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
linux-mm, guoren, sparclinux, linux-hexagon, linux-riscv, will,
Christoph Hellwig, linux-arch, linux-s390, bcain, deller, x86,
linux, linux-csky, ardb, mingo, geert, linux-snps-arc,
linux-xtensa, arnd, hca, linux-alpha, linux-um, linuxppc-dev,
linux-m68k, openrisc, green.hu, shorne, linux-arm-kernel,
Arnd Bergmann, monstr, tsbogend, linux-parisc, nickhu, jcmvbkbc,
linux-api, linux-kernel, dinguyen, ebiederm, richard, akpm,
Linus Torvalds, davem
In-Reply-To: <Ygr11RGjj3C9uAUg@zeniv-ca.linux.org.uk>
On Tue, Feb 15, 2022 at 12:37:41AM +0000, Al Viro wrote:
> Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
> (and trim the comment down to "coldfire and 68000 will pick generic
> variant")?
I wonder if we should invert CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE,
select the separate address space config for s390, sparc64, non-coldfire
m68k and mips with EVA and then just have one single access_ok for
overlapping address space (as added by Arnd) and non-overlapping ones
(always return true).
^ permalink raw reply
* Re: [PATCH] powerpc/module_64: use module_init_section instead of patching names
From: Michael Ellerman @ 2022-02-15 5:26 UTC (permalink / raw)
To: mpe, Wedson Almeida Filho; +Cc: linuxppc-dev, linux-kernel, paulus
In-Reply-To: <20220202055123.2144842-1-wedsonaf@google.com>
On Wed, 2 Feb 2022 05:51:23 +0000, Wedson Almeida Filho wrote:
> Without this patch, module init sections are disabled by patching their
> names in arch-specific code when they're loaded (which prevents code in
> layout_sections from finding init sections). This patch uses the new
> arch-specific module_init_section instead.
>
> This allows modules that have .init_array sections to have the
> initialisers properly called (on load, before init). Without this patch,
> the initialisers are not called because .init_array is renamed to
> _init_array, and thus isn't found by code in find_module_sections().
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/module_64: use module_init_section instead of patching names
https://git.kernel.org/powerpc/c/d4be60fe66b7380530868ceebe549f8eebccacc5
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: epapr: A typo fix
From: Michael Ellerman @ 2022-02-15 5:26 UTC (permalink / raw)
To: rdunlap, paulus, mpe, benh, linuxppc-dev, Bhaskar Chowdhury,
linux-kernel
In-Reply-To: <20210320213932.22697-1-unixbhaskar@gmail.com>
On Sun, 21 Mar 2021 03:09:32 +0530, Bhaskar Chowdhury wrote:
> s/parmeters/parameters/
>
>
Applied to powerpc/next.
[1/1] powerpc: epapr: A typo fix
https://git.kernel.org/powerpc/c/a1c414093370ed50e5b952d96d4ae775c7a18420
cheers
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/e500/qemu-e500: allow core to idle without waiting
From: Michael Ellerman @ 2022-02-15 5:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Scott Wood, linuxppc-dev,
Michael Ellerman, Paul Mackerras, Joachim Wiberg
Cc: Tobias Waldekranz
In-Reply-To: <20220112112459.1033754-1-troglobit@gmail.com>
On Wed, 12 Jan 2022 12:24:59 +0100, Joachim Wiberg wrote:
> From: Tobias Waldekranz <tobias@waldekranz.com>
>
> This means an idle guest won't needlessly consume an entire core on
> the host, waiting for work to show up.
>
>
Applied to powerpc/next.
[1/1] powerpc/e500/qemu-e500: allow core to idle without waiting
https://git.kernel.org/powerpc/c/f529edd1b69ddf832c3257dcd34e15100038d6b7
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: dts: Fix some I2C unit addresses
From: Michael Ellerman @ 2022-02-15 5:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Thierry Reding,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20211220134036.683309-1-thierry.reding@gmail.com>
On Mon, 20 Dec 2021 14:40:36 +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The unit-address for the Maxim MAX1237 ADCs on XPedite5200 boards don't
> match the value in the "reg" property and cause a DTC warning.
>
>
Applied to powerpc/next.
[1/1] powerpc: dts: Fix some I2C unit addresses
https://git.kernel.org/powerpc/c/d5342fdd163ae0553a14820021a107e03eb1ea72
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox