Re: [PATCH v2 08/18] uaccess: add generic __{get,put}_kernel_nofault
On Wed, Feb 16, 2022 at 2:17 PM Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Nine architectures are still missing __{get,put}_kernel_nofault:
> alpha, ia64, microblaze, nds32, nios2, openrisc, sh, sparc32, xtensa.
>
> Add a generic version that lets everything use the normal
> copy_{from,to}_kernel_nofault() code based on these, removing the last
> use of get_fs()/set_fs() from architecture-independent code.
>
> Reviewed-by: Christoph Hellwig
> Signed-off-by: Arnd Bergmann
> arch/m68k/include/asm/uaccess.h | 2 -
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 10/18] m68k: fix access_ok for coldfire
Hi Arnd,
On Wed, Feb 16, 2022 at 2:17 PM Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> While most m68k platforms use separate address spaces for user
> and kernel space, at least coldfire does not, and the other
> ones have a TASK_SIZE that is less than the entire 4GB address
> range.
>
> Using the default implementation of __access_ok() stops coldfire
> user space from trivially accessing kernel memory.
>
> Signed-off-by: Arnd Bergmann
Thanks for your patch!
> --- a/arch/m68k/include/asm/uaccess.h
> +++ b/arch/m68k/include/asm/uaccess.h
> @@ -12,14 +12,21 @@
> #include
>
> /* We let the MMU do all checking */
> -static inline int access_ok(const void __user *addr,
> +static inline int access_ok(const void __user *ptr,
> unsigned long size)
> {
> + unsigned long limit = TASK_SIZE;
> + unsigned long addr = (unsigned long)ptr;
> +
> /*
> * XXX: for !CONFIG_CPU_HAS_ADDRESS_SPACES this really needs to check
> * for TASK_SIZE!
> +* Removing this helper is probably sufficient.
> */
Shouldn't the above comment block be removed completely,
as this is now implemented below?
> - return 1;
> + if (IS_ENABLED(CONFIG_CPU_HAS_ADDRESS_SPACES))
> + return 1;
> +
> + return (size <= limit) && (addr <= (limit - size));
> }
Any pesky compilers that warn (or worse with -Werror) about
"condition always true" for TASK_SIZE = 0xUL?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Wed, Feb 16, 2022 at 2:17 PM Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> There are many different ways that access_ok() is defined across
> architectures, but in the end, they all just compare against the
> user_addr_max() value or they accept anything.
>
> Provide one definition that works for most architectures, checking
> against TASK_SIZE_MAX for user processes or skipping the check inside
> of uaccess_kernel() sections.
>
> For architectures without CONFIG_SET_FS(), this should be the fastest
> check, as it comes down to a single comparison of a pointer against a
> compile-time constant, while the architecture specific versions tend to
> do something more complex for historic reasons or get something wrong.
>
> Type checking for __user annotations is handled inconsistently across
> architectures, but this is easily simplified as well by using an inline
> function that takes a 'const void __user *' argument. A handful of
> callers need an extra __user annotation for this.
>
> Some architectures had trick to use 33-bit or 65-bit arithmetic on the
> addresses to calculate the overflow, however this simpler version uses
> fewer registers, which means it can produce better object code in the
> end despite needing a second (statically predicted) branch.
>
> Reviewed-by: Christoph Hellwig
> Acked-by: Mark Rutland [arm64, asm-generic]
> Signed-off-by: Arnd Bergmann
> arch/m68k/Kconfig.cpu | 1 +
> arch/m68k/include/asm/uaccess.h | 19 +
Acked-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/18] clean up asm/uaccess.h, kill set_fs for good
n Fri, Feb 18, 2022 at 3:21 AM Al Viro wrote:
>
> On Thu, Feb 17, 2022 at 08:49:59AM +0100, Arnd Bergmann wrote:
>
> > Same here: architectures can already provide a __put_user_fn()
> > and __get_user_fn(), to get the generic versions of the interface,
> > but few architectures use that. You can actually get all the interfaces
> > by just providing raw_copy_from_user() and raw_copy_to_user(),
> > but the get_user/put_user versions you get from that are fairly
> > inefficient.
>
> FWIW, __{get,put}_user_{8,16,32,64} would probably make it easier to
> unify. That's where the really variable part tends to be, anyway.
> IMO __get_user_fn() had been a mistake.
I've prototyped this now, to see what this might look like, see
https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/commit/?h=generic-get_user-prototype
This adds generic inline version of {__get,get,__put,put}_user()
and converts x86 to (optionally) use it. This builds with gcc-5
through gcc-11 on 32-bit and 64-bit x86, using asm-goto with
outputs where possible, and requiring a minimum set of macro
definitions from the architecture. Compiling with clang produces
no warnings but does cause a linker issue at the moment, so
there is probably at least one bug in it.
Aside from compile-testing, I have not tried to verify if this
is correct or efficient, but let me know if you think this is headed
in the right direction.
> One thing I somewhat dislike about the series is the boilerplate in
> asm/uaccess.h instances - #include in
> a lot of them might make sense as a transitory state, but getting
> stuck with those indefinitely...
Christoph also complained about it, the problem for now is that
asm-generic/access_ok.h must first see the macro definitions for
architectures that override any of the contents, but access_ok()
itself is used at least in some of the asm/uaccess.h files as well,
so it must be included in the middle of it, until more of the uaccess.h
implementation is moved to linux/uaccess.h in an architecture
independent way.
Would you prefer having an asm/access_ok.h that falls back to
the asm-generic version but can have an architecture specific
override when needed (ia64, arm64, x86, um)?
> BTW, do we need user_addr_max() anymore? The definition in
> asm-generic/access-ok.h is the only one, so ifndef around it is pointless.
Right, the v2 changes got rid of the last override, so it could get
hardcoded to TASK_SIZE_MAX, or we can convert the five
references to just use that instead and remove it altogether:
arch/arm64/kernel/traps.c: if (address >= user_addr_max()) {
\
arch/parisc/kernel/signal.c:if (start >= user_addr_max() - sigframe_size)
arch/parisc/kernel/signal.c:if (A(&usp[0]) >=
user_addr_max() - 5 * sizeof(int))
lib/strncpy_from_user.c:max_addr = user_addr_max();
lib/strnlen_user.c: max_addr = user_addr_max();
user_addr_max() first showed up in architecture-independent code in
c5389831cda3 ("sparc: Fix user_addr_max() definition."), and from that
I think the original intent is no longer useful.
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 10/18] m68k: fix access_ok for coldfire
On Fri, Feb 18, 2022 at 10:00 AM Geert Uytterhoeven
wrote:
> > /* We let the MMU do all checking */
> > -static inline int access_ok(const void __user *addr,
> > +static inline int access_ok(const void __user *ptr,
> > unsigned long size)
> > {
> > + unsigned long limit = TASK_SIZE;
> > + unsigned long addr = (unsigned long)ptr;
> > +
> > /*
> > * XXX: for !CONFIG_CPU_HAS_ADDRESS_SPACES this really needs to
> > check
> > * for TASK_SIZE!
> > +* Removing this helper is probably sufficient.
> > */
>
> Shouldn't the above comment block be removed completely,
> as this is now implemented below?
Yes, obviously. Fixed now.
> > - return 1;
> > + if (IS_ENABLED(CONFIG_CPU_HAS_ADDRESS_SPACES))
> > + return 1;
I just noticed this should have the same change that I made for the
generic version, changed it now to
+ if (IS_ENABLED(CONFIG_CPU_HAS_ADDRESS_SPACES) ||
+ !IS_ENABLED(CONFIG_MMU))
This is gone again after the cleanup patch, when the generic version
is used instead.
> > + return (size <= limit) && (addr <= (limit - size));
> > }
>
> Any pesky compilers that warn (or worse with -Werror) about
> "condition always true" for TASK_SIZE = 0xUL?
No, using a local variable avoids this warning.
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH v2 13/18] uaccess: generalize access_ok()
From: Andy Lutomirski > Sent: 17 February 2022 19:15 ... > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > constant that has a very specific value to work around a bug^Wdesign > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > which userspace is permitted to allocate memory, but there is a huge > gap between user and kernel addresses, and any value in the gap would > be adequate for the comparison. If we wanted to optimize this, simply > checking the high bit (which x86 can do without any immediate > constants at all) would be sufficient and, for an access known to fit > in 32 bits, one could get even fancier and completely ignore the size > of the access. (For accesses not known to fit in 32 bits, I suspect > some creativity could still come up with a construction that's > substantially faster than the one in your patch.) > > So there's plenty of room for optimization here. > > (This is not in any respect a NAK -- it's just an observation that > this could be even better.) For 64bit arch that use the top bit to separate user/kernel you can test '(addr | size) >> 62)'. The compiler optimises out constant sizes. This has all been mentioned a lot of times. You do get different fault types. OTOH an explicit check for constant size (less than something big) can use the cheaper test of the sign bit. Big constant sizes could be compile time errors. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/18] clean up asm/uaccess.h, kill set_fs for good
Le 18/02/2022 à 02:50, Al Viro a écrit :
> On Thu, Feb 17, 2022 at 07:20:11AM +, Christophe Leroy wrote:
>
>> And we have also
>> user_access_begin()/user_read_access_begin()/user_write_access_begin()
>> which call access_ok() then do the real work. Could be made generic with
>> call to some arch specific __user_access_begin() and friends after the
>> access_ok() and eventually the might_fault().
>
> Not a good idea, considering the fact that we do not want to invite
> uses of "faster" variants...
I'm not sure I understand your concern.
Today in powerpc we have:
static __must_check inline bool
user_read_access_begin(const void __user *ptr, size_t len)
{
if (unlikely(!access_ok(ptr, len)))
return false;
might_fault();
allow_read_from_user(ptr, len);
return true;
}
We could instead have a generic
static __must_check inline bool
user_read_access_begin(const void __user *ptr, size_t len)
{
if (unlikely(!access_ok(ptr, len)))
return false;
might_fault();
return arch_user_read_access_begin(ptr, len);
}
And then a powerpc specific
static __must_check __always_inline bool
arch_user_read_access_begin(const void __user *ptr, size_t len)
{
allow_read_from_user(ptr, len);
return true;
}
#define arch_user_read_access_begin arch_user_read_access_begin
And a generic fallback for arch_user_read_access_begin() that does
nothing at all.
Do you mean that in that case people might be tempted to use
arch_user_read_access_begin() instead of using user_read_access_begin() ?
If that's the case isn't it something we could verify via checkpatch.pl ?
Today it seems to be problematic that functions in asm/uaccess.h use
access_ok(). Such an approach would allow to get rid of access_ok() use
in architecture's uaccess.h
Christophe
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 18/18] uaccess: drop maining CONFIG_SET_FS users
Hi Arnd, > From: Arnd Bergmann > > There are no remaining callers of set_fs(), so CONFIG_SET_FS > can be removed globally, along with the thread_info field and > any references to it. > > This turns access_ok() into a cheaper check against TASK_SIZE_MAX. > > With CONFIG_SET_FS gone, so drop all remaining references to > set_fs()/get_fs(), mm_segment_t and uaccess_kernel(). > > Signed-off-by: Arnd Bergmann Tested-by: Sergey Matyukevich # for arc changes Regards, Sergey ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
RE: [PATCH v2 05/18] x86: remove __range_not_ok()
From: Christoph Hellwig
> Sent: 18 February 2022 06:29
...
>
> > diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> > index 15b058eefc4e..ee117fcf46ed 100644
> > --- a/arch/x86/kernel/stacktrace.c
> > +++ b/arch/x86/kernel/stacktrace.c
> > @@ -90,7 +90,7 @@ copy_stack_frame(const struct stack_frame_user __user *fp,
> > {
> > int ret;
> >
> > - if (__range_not_ok(fp, sizeof(*frame), TASK_SIZE))
> > + if (!__access_ok(fp, sizeof(*frame)))
> > return 0;
>
> Just switch the __get_user calls below to get_user instead.
Is this worth doing at all?
How much userspace code is actually compiled with stack frames?
Won't work well for a 32bit process on a 64bit kernel either.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 13/18] uaccess: generalize access_ok()
On Fri, Feb 18, 2022 at 1:30 AM David Laight wrote: > > From: Andy Lutomirski > > Sent: 17 February 2022 19:15 > ... > > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > > constant that has a very specific value to work around a bug^Wdesign > > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > > which userspace is permitted to allocate memory, but there is a huge > > gap between user and kernel addresses, and any value in the gap would > > be adequate for the comparison. If we wanted to optimize this, simply > > checking the high bit (which x86 can do without any immediate > > constants at all) would be sufficient and, for an access known to fit > > in 32 bits, one could get even fancier and completely ignore the size > > of the access. (For accesses not known to fit in 32 bits, I suspect > > some creativity could still come up with a construction that's > > substantially faster than the one in your patch.) > > > > So there's plenty of room for optimization here. > > > > (This is not in any respect a NAK -- it's just an observation that > > this could be even better.) > > For 64bit arch that use the top bit to separate user/kernel > you can test '(addr | size) >> 62)'. > The compiler optimises out constant sizes. > > This has all been mentioned a lot of times. > You do get different fault types. > > OTOH an explicit check for constant size (less than something big) > can use the cheaper test of the sign bit. > Big constant sizes could be compile time errors. The different fault type issue may well be a real problem. Right now the core x86 fault code reserves the right to grouch if we get #GP instead of #PF. We could change that. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
