Re: [PATCH v2 05/18] x86: remove __range_not_ok()
On Fri, Feb 18, 2022 at 7:28 AM Christoph Hellwig wrote:
> On Wed, Feb 16, 2022 at 02:13:19PM +0100, Arnd Bergmann wrote:
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -2794,7 +2794,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
> > *entry, struct pt_regs *re
> > static inline int
> > valid_user_frame(const void __user *fp, unsigned long size)
> > {
> > - return (__range_not_ok(fp, size, TASK_SIZE) == 0);
> > + return __access_ok(fp, size);
> > }
>
> valid_user_frame just need to go away and the following __get_user calls
> replaced with normal get_user ones.
As I understand it, that would not work here because get_user() calls
access_ok() rather than __access_ok(), and on x86 that can not be
called in NMI context.
It is a bit odd that x86 is the only architecture that has this check,
but adding
it was clearly intentional, see 7c4788950ba5 ("x86/uaccess, sched/preempt:
Verify access_ok() context").
> > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> > index 53de044e5654..da534fb7b5c6 100644
> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -85,7 +85,7 @@ static int copy_code(struct pt_regs *regs, u8 *buf,
> > unsigned long src,
> >* Make sure userspace isn't trying to trick us into dumping kernel
> >* memory by pointing the userspace instruction pointer at it.
> >*/
> > - if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX))
> > + if (!__access_ok((void __user *)src, nbytes))
> > return -EINVAL;
>
> This one is not needed at all as copy_from_user_nmi already checks the
> access range.
Ok, removing this.
> > 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.
Same as the first one, I think we can't do this in NMI context.
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()
On Fri, Feb 18, 2022 at 7:34 AM Christoph Hellwig wrote: > > > +#include > > Instead of the asm-generic games, shouldn't we just define access_ok in > if not already defined by the architecture? I tried, but couldn't actually make it work because asm/uaccess.h tends to contain inline functions that rely on access_ok(). It could work once we move all the high-level functions into linux/uaccess.h, but that would likely require another long patch series. One option that can work is to require architectures to have an asm/access_ok.h header that gets included by linux/uaccess.h. On most architectures, that would be redirected to asm-generic/access_ok.h, as only ia64, x86, arm64 and um need to override the definition. Arnd ___ 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 Thu, Feb 17, 2022 at 8:15 PM Andy Lutomirski wrote: > > On Wed, Feb 16, 2022 at 5:19 AM Arnd Bergmann wrote: > > > > From: Arnd Bergmann > > > > There are many different ways that access_ok() is defined across > > architectures, but in the end, they all just compare against the > > user_addr_max() value or they accept anything. > > > > Provide one definition that works for most architectures, checking > > against TASK_SIZE_MAX for user processes or skipping the check inside > > of uaccess_kernel() sections. > > > > For architectures without CONFIG_SET_FS(), this should be the fastest > > check, as it comes down to a single comparison of a pointer against a > > compile-time constant, while the architecture specific versions tend to > > do something more complex for historic reasons or get something wrong. > > This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre > constant that has a very specific value to work around a bug^Wdesign > error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at > which userspace is permitted to allocate memory, but there is a huge > gap between user and kernel addresses, and any value in the gap would > be adequate for the comparison. If we wanted to optimize this, simply > checking the high bit (which x86 can do without any immediate > constants at all) would be sufficient and, for an access known to fit > in 32 bits, one could get even fancier and completely ignore the size > of the access. (For accesses not known to fit in 32 bits, I suspect > some creativity could still come up with a construction that's > substantially faster than the one in your patch.) > > So there's plenty of room for optimization here. > > (This is not in any respect a NAK -- it's just an observation that > this could be even better.) Thank you for taking a look! As you can see in the patch that changes the algorithm on x86 [1], it was already using TASK_SIZE_MAX as the limit, only the order in which the comparison is done, hopefully leading to better code already. I have looked at trivial examples on x86 that showed an improvement for constant sizes, but only looked at arm64 in detail for the overall result. It may be worth checking if using LONG_MAX as the limit produces better code, but it's probably best to do the optimization in the common code in a portable way to keep it from diverging again. Arnd [1] https://lore.kernel.org/lkml/20220216131332.1489939-7-a...@kernel.org/ ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 14/18] lib/test_lockup: fix kernel pointer check for separate address spaces
On Fri, Feb 18, 2022 at 7:35 AM Christoph Hellwig wrote: > > On Wed, Feb 16, 2022 at 02:13:28PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > test_kernel_ptr() uses access_ok() to figure out if a given address > > points to user space instead of kernel space. However on architectures > > that set CONFIG_ALTERNATE_USER_ADDRESS_SPACE, a pointer can be valid > > for both, and the check always fails because access_ok() returns true. > > > > Make the check for user space pointers conditional on the type of > > address space layout. > > What is this code even trying to do? It looks extremly broken. As I understand it, this is only meant for debugging, and the module contains intentionally broken lock usage to test whether the watchdog and lockup detection in the kernel is able to find them. I did not try that hard to understand how it works though. Arnd ___ 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
On Fri, Feb 18, 2022 at 7:37 AM Christoph Hellwig wrote: > > s/maining/remaining/ ? > > Or maybe rather: > > uaccess: remove CONFIG_SET_FS > > because it is all gone now. > > > With CONFIG_SET_FS gone, so drop all remaining references to > > set_fs()/get_fs(), mm_segment_t and uaccess_kernel(). > > And this sentence does not parse. Both fixed now, thanks! Arnd ___ 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
s/maining/remaining/ ? Or maybe rather: uaccess: remove CONFIG_SET_FS because it is all gone now. > With CONFIG_SET_FS gone, so drop all remaining references to > set_fs()/get_fs(), mm_segment_t and uaccess_kernel(). And this sentence does not parse. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 16/18] sh: remove CONFIG_SET_FS support
Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 14/18] lib/test_lockup: fix kernel pointer check for separate address spaces
On Wed, Feb 16, 2022 at 02:13:28PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > test_kernel_ptr() uses access_ok() to figure out if a given address > points to user space instead of kernel space. However on architectures > that set CONFIG_ALTERNATE_USER_ADDRESS_SPACE, a pointer can be valid > for both, and the check always fails because access_ok() returns true. > > Make the check for user space pointers conditional on the type of > address space layout. What is this code even trying to do? It looks extremly broken. ___ 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()
> +#include Instead of the asm-generic games, shouldn't we just define access_ok in if not already defined by the architecture? ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 12/18] uaccess: fix type mismatch warnings from access_ok()
Looks good, Reviewed-by: Christoph Hellwig ___ 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
Looks good, Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 08/18] uaccess: add generic __{get,put}_kernel_nofault
Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 07/18] nios2: drop access_ok() check from __put_user()
On Wed, Feb 16, 2022 at 02:13:21PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > Unlike other architectures, the nios2 version of __put_user() has an > extra check for access_ok(), preventing it from being used to implement > __put_kernel_nofault(). > > Split up put_user() along the same lines as __get_user()/get_user() Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 06/18] x86: use more conventional access_ok() definition
Looks good, Reviewed-by: Christoph Hellwig ___ 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()
On Wed, Feb 16, 2022 at 02:13:19PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The __range_not_ok() helper is an x86 (and sparc64) specific interface
> that does roughly the same thing as __access_ok(), but with different
> calling conventions.
>
> Change this to use the normal interface in order for consistency as we
> clean up all access_ok() implementations.
>
> This changes the limit from TASK_SIZE to TASK_SIZE_MAX, which Al points
> out is the right thing do do here anyway.
>
> The callers have to use __access_ok() instead of the normal access_ok()
> though, because on x86 that contains a WARN_ON_IN_IRQ() check that cannot
> be used inside of NMI context while tracing.
>
> Suggested-by: Al Viro
> Suggested-by: Christoph Hellwig
> Link: https://lore.kernel.org/lkml/ygsukcxgr7r4n...@zeniv-ca.linux.org.uk/
> Signed-off-by: Arnd Bergmann
> ---
> arch/x86/events/core.c | 2 +-
> arch/x86/include/asm/uaccess.h | 10 ++
> arch/x86/kernel/dumpstack.c| 2 +-
> arch/x86/kernel/stacktrace.c | 2 +-
> arch/x86/lib/usercopy.c| 2 +-
> 5 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index e686c5e0537b..eef816fc216d 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2794,7 +2794,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx
> *entry, struct pt_regs *re
> static inline int
> valid_user_frame(const void __user *fp, unsigned long size)
> {
> - return (__range_not_ok(fp, size, TASK_SIZE) == 0);
> + return __access_ok(fp, size);
> }
valid_user_frame just need to go away and the following __get_user calls
replaced with normal get_user ones.
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 53de044e5654..da534fb7b5c6 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -85,7 +85,7 @@ static int copy_code(struct pt_regs *regs, u8 *buf,
> unsigned long src,
>* Make sure userspace isn't trying to trick us into dumping kernel
>* memory by pointing the userspace instruction pointer at it.
>*/
> - if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX))
> + if (!__access_ok((void __user *)src, nbytes))
> return -EINVAL;
This one is not needed at all as copy_from_user_nmi already checks the
access range.
> 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.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 03/18] nds32: fix access_ok() checks in get/put_user
On Wed, Feb 16, 2022 at 02:13:17PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > 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(). Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 02/18] uaccess: fix nios2 and microblaze get_user_8()
Looks good: Reviewed-by: Christoph Hellwig ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RFC PATCH 0/3] arc: remove CONFIG_SET_FS
IIUC it makes sense to drop this patch series from your for-next branch. These changes have been superseded by generic approach posted by Arnd Bergmann, see: https://lore.kernel.org/linux-arch/CAHk-=whXYWoP6of7js=f4zov62on97mnfrsvrwhy75wogm6...@mail.gmail.com/T/#t I tested patches by Arnd on ARC700/ARCHS platforms, so far so good. Yep I've been following that thread and have dropped your ARC patchset. Thx for testing. You can reply to Arnd with a Tested-by: < your-name> #arc -Vineet ___ 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
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.
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...
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.
___
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
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... ___ 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
Arnd Bergmann writes:
> 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().
For the bits I have looked at recently, and think I understand.
Acked-by: "Eric W. Biederman"
>
> Signed-off-by: Arnd Bergmann
> ---
> fs/exec.c | 6 --
> kernel/exit.c | 14 -
> kernel/kthread.c | 5 --
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 79f2c9483302..bc68a0c089ac 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1303,12 +1303,6 @@ int begin_new_exec(struct linux_binprm * bprm)
> if (retval)
> goto out_unlock;
>
> - /*
> - * Ensure that the uaccess routines can actually operate on userspace
> - * pointers:
> - */
> - force_uaccess_begin();
> -
> if (me->flags & PF_KTHREAD)
> free_kthread_struct(me);
> me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
> diff --git a/kernel/exit.c b/kernel/exit.c
> index b00a25bb4ab9..0884a75bc2f8 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -737,20 +737,6 @@ void __noreturn do_exit(long code)
>
> WARN_ON(blk_needs_flush_plug(tsk));
>
> - /*
> - * If do_dead is called because this processes oopsed, it's possible
> - * that get_fs() was left as KERNEL_DS, so reset it to USER_DS before
> - * continuing. Amongst other possible reasons, this is to prevent
> - * mm_release()->clear_child_tid() from writing to a user-controlled
> - * kernel address.
> - *
> - * On uptodate architectures force_uaccess_begin is a noop. On
> - * architectures that still have set_fs/get_fs in addition to handling
> - * oopses handles kernel threads that run as set_fs(KERNEL_DS) by
> - * default.
> - */
> - force_uaccess_begin();
> -
> kcov_task_exit(tsk);
>
> coredump_task_exit(tsk);
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 38c6dd822da8..16c2275d4b50 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -55,7 +55,6 @@ struct kthread {
> int result;
> int (*threadfn)(void *);
> void *data;
> - mm_segment_t oldfs;
> struct completion parked;
> struct completion exited;
> #ifdef CONFIG_BLK_CGROUP
> @@ -1441,8 +1440,6 @@ void kthread_use_mm(struct mm_struct *mm)
> mmdrop(active_mm);
> else
> smp_mb();
> -
> - to_kthread(tsk)->oldfs = force_uaccess_begin();
> }
> EXPORT_SYMBOL_GPL(kthread_use_mm);
>
> @@ -1457,8 +1454,6 @@ void kthread_unuse_mm(struct mm_struct *mm)
> WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD));
> WARN_ON_ONCE(!tsk->mm);
>
> - force_uaccess_end(to_kthread(tsk)->oldfs);
> -
> task_lock(tsk);
> /*
>* When a kthread stops operating on an address space, the loop
___
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 5:19 AM Arnd Bergmann wrote: > > From: Arnd Bergmann > > There are many different ways that access_ok() is defined across > architectures, but in the end, they all just compare against the > user_addr_max() value or they accept anything. > > Provide one definition that works for most architectures, checking > against TASK_SIZE_MAX for user processes or skipping the check inside > of uaccess_kernel() sections. > > For architectures without CONFIG_SET_FS(), this should be the fastest > check, as it comes down to a single comparison of a pointer against a > compile-time constant, while the architecture specific versions tend to > do something more complex for historic reasons or get something wrong. This isn't actually optimal. On x86, TASK_SIZE_MAX is a bizarre constant that has a very specific value to work around a bug^Wdesign error^Wfeature of Intel CPUs. TASK_SIZE_MAX is the maximum address at which userspace is permitted to allocate memory, but there is a huge gap between user and kernel addresses, and any value in the gap would be adequate for the comparison. If we wanted to optimize this, simply checking the high bit (which x86 can do without any immediate constants at all) would be sufficient and, for an access known to fit in 32 bits, one could get even fancier and completely ignore the size of the access. (For accesses not known to fit in 32 bits, I suspect some creativity could still come up with a construction that's substantially faster than the one in your patch.) So there's plenty of room for optimization here. (This is not in any respect a NAK -- it's just an observation that this could be even better.) ___ 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
On Wed, Feb 16, 2022 at 2:13 PM Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> Christoph Hellwig and a few others spent a huge effort on removing
> set_fs() from most of the important architectures, but about half the
> other architectures were never completed even though most of them don't
> actually use set_fs() at all.
>
> I did a patch for microblaze at some point, which turned out to be fairly
> generic, and now ported it to most other architectures, using new generic
> implementations of access_ok() and __{get,put}_kernel_nocheck().
>
> Three architectures (sparc64, ia64, and sh) needed some extra work,
> which I also completed.
>
> The final series contains extra cleanup changes that touch all
> architectures. Please review and test these, so we can merge them
> for v5.18.
>
> The series is available at
> https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=set_fs-2
> for testing.
I've added the updated contents to my asm-generic tree now to put them
into linux-next.
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
