* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Christian Brauner @ 2019-09-05 11:29 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Aleksa Sarai, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov
In-Reply-To: <ae415ea8-4442-d81c-3b46-2ae5fb35bbdf@rasmusvillemoes.dk>
On Thu, Sep 05, 2019 at 01:17:38PM +0200, Rasmus Villemoes wrote:
> On 05/09/2019 13.05, Christian Brauner wrote:
> > On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
>
> >> + if (unlikely(!access_ok(dst, usize)))
> >> + return -EFAULT;
> >> +
> >> + /* Deal with trailing bytes. */
> >> + if (usize < ksize) {
> >> + if (memchr_inv(src + size, 0, rest))
> >> + return -EFBIG;
> >> + } else if (usize > ksize) {
> >> + if (__memzero_user(dst + size, rest))
> >> + return -EFAULT;
> >
> > Is zeroing that memory really our job? Seems to me we should just check
> > it is zeroed.
>
> Of course it is, otherwise you'd require userspace to clear the output
> buffer it gives us, which in the majority of cases is wasted work. It's
> much easier to reason about if we just say "the kernel populates [uaddr,
> uaddr + usize)".
I don't really mind either way so sure. :)
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 11:27 UTC (permalink / raw)
To: Christian Brauner
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Rasmus Villemoes,
Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg
In-Reply-To: <20190905110915.4vvhicg4ldmpi5u6@wittgenstein>
[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]
On 2019-09-05, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > A common pattern for syscall extensions is increasing the size of a
> > struct passed from userspace, such that the zero-value of the new fields
> > result in the old kernel behaviour (allowing for a mix of userspace and
> > kernel vintages to operate on one another in most cases). This is done
> > in both directions -- hence two helpers -- though it's more common to
> > have to copy user space structs into kernel space.
> >
> > Previously there was no common lib/ function that implemented
> > the necessary extension-checking semantics (and different syscalls
> > implemented them slightly differently or incompletely[1]). A future
> > patch replaces all of the common uses of this pattern to use the new
> > copy_struct_{to,from}_user() helpers.
> >
> > [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> > similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> > always rejects differently-sized struct arguments.
> >
> > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
>
> I would probably split this out into a separate patchset. It can very
> well go in before openat2(). Thoughts?
Yeah, I'll split this and the related patches out -- though I will admit
I'm not sure how you're supposed to deal with multiple independent
patchsets that depend on each other. How will folks reviewing openat2(2)
know to include the lib/struct_user.c changes?
Also, whose tree should it go through?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Rasmus Villemoes @ 2019-09-05 11:17 UTC (permalink / raw)
To: Christian Brauner, Aleksa Sarai
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Alexander Shishkin <alex>
In-Reply-To: <20190905110544.d6c5t7rx25kvywmi@wittgenstein>
On 05/09/2019 13.05, Christian Brauner wrote:
> On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
>> + if (unlikely(!access_ok(dst, usize)))
>> + return -EFAULT;
>> +
>> + /* Deal with trailing bytes. */
>> + if (usize < ksize) {
>> + if (memchr_inv(src + size, 0, rest))
>> + return -EFBIG;
>> + } else if (usize > ksize) {
>> + if (__memzero_user(dst + size, rest))
>> + return -EFAULT;
>
> Is zeroing that memory really our job? Seems to me we should just check
> it is zeroed.
Of course it is, otherwise you'd require userspace to clear the output
buffer it gives us, which in the majority of cases is wasted work. It's
much easier to reason about if we just say "the kernel populates [uaddr,
uaddr + usize)".
It's completely symmetric to copy_struct_from_user doing a memset() of
the tail of the kernel buffer in case of ksize>usize - you wouldn't want
to require the kernel callers to pass a zeroed buffer to
copy_struct_from_user() - it's just that when we memset(__user*),
there's an error check to do.
Rasmus
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Christian Brauner @ 2019-09-05 11:09 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Shuah Khan, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Jiri Olsa, Alexander Shishkin, Ingo Molnar,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k, Al Viro, Andy
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases). This is done
> in both directions -- hence two helpers -- though it's more common to
> have to copy user space structs into kernel space.
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[1]). A future
> patch replaces all of the common uses of this pattern to use the new
> copy_struct_{to,from}_user() helpers.
>
> [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
I would probably split this out into a separate patchset. It can very
well go in before openat2(). Thoughts?
Christian
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Christian Brauner @ 2019-09-05 11:05 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Rasmus Villemoes,
Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases). This is done
> in both directions -- hence two helpers -- though it's more common to
> have to copy user space structs into kernel space.
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[1]). A future
> patch replaces all of the common uses of this pattern to use the new
> copy_struct_{to,from}_user() helpers.
>
> [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> include/linux/uaccess.h | 5 ++
> lib/Makefile | 2 +-
> lib/struct_user.c | 182 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 188 insertions(+), 1 deletion(-)
> create mode 100644 lib/struct_user.c
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 34a038563d97..0ad9544a1aee 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -230,6 +230,11 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
>
> #endif /* ARCH_HAS_NOCACHE_UACCESS */
>
> +extern int copy_struct_to_user(void __user *dst, size_t usize,
> + const void *src, size_t ksize);
> +extern int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize);
> +
> /*
> * probe_kernel_read(): safely attempt to read from a location
> * @dst: pointer to the buffer that shall take the data
> diff --git a/lib/Makefile b/lib/Makefile
> index 29c02a924973..d86c71feaf0a 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -28,7 +28,7 @@ endif
> CFLAGS_string.o := $(call cc-option, -fno-stack-protector)
> endif
>
> -lib-y := ctype.o string.o vsprintf.o cmdline.o \
> +lib-y := ctype.o string.o struct_user.o vsprintf.o cmdline.o \
> rbtree.o radix-tree.o timerqueue.o xarray.o \
> idr.o extable.o \
> sha1.o chacha.o irq_regs.o argv_split.o \
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..7301ab1bbe98
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +#define BUFFER_SIZE 64
> +
> +/*
> + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> + * checked access_ok(p, size).
> + */
> +static int __memzero_user(void __user *p, size_t s)
> +{
> + const char zeros[BUFFER_SIZE] = {};
> + while (s > 0) {
> + size_t n = min(s, sizeof(zeros));
> +
> + if (__copy_to_user(p, zeros, n))
> + return -EFAULT;
> +
> + p += n;
> + s -= n;
> + }
> + return 0;
> +}
> +
> +/**
> + * copy_struct_to_user: copy a struct to user space
> + * @dst: Destination address, in user space.
> + * @usize: Size of @dst struct.
> + * @src: Source address, in kernel space.
> + * @ksize: Size of @src struct.
> + *
> + * Copies a struct from kernel space to user space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * // do something with karg
> + *
> + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> + * older user space. In order to avoid user space getting incomplete
> + * information (new fields might be important), all trailing bytes in @src
> + * (@ksize - @usize) must be zerored, otherwise -EFBIG is returned.
> + * * If @usize > @ksize, then the kernel is "returning" an older struct to a
> + * newer user space. The trailing bytes in @dst (@usize - @ksize) will be
> + * zero-filled.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_to_user(void __user *dst, size_t usize,
> + const void *src, size_t ksize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
> +
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
Looks like this should be -EFBIG.
> + if (unlikely(!access_ok(dst, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize) {
> + if (memchr_inv(src + size, 0, rest))
> + return -EFBIG;
> + } else if (usize > ksize) {
> + if (__memzero_user(dst + size, rest))
> + return -EFAULT;
Is zeroing that memory really our job? Seems to me we should just check
it is zeroed.
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_to_user(dst, src, size))
> + return -EFAULT;
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_struct_to_user);
> +
> +/**
> + * copy_struct_from_user: copy a struct from user space
> + * @dst: Destination address, in kernel space. This buffer must be @ksize
> + * bytes long.
> + * @ksize: Size of @dst struct.
> + * @src: Source address, in user space.
> + * @usize: (Alleged) size of @src struct.
> + *
> + * Copies a struct from user space to kernel space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then the user space has passed an old struct to a
> + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> + * are to be zero-filled.
> + * * If @usize > @ksize, then the user space has passed a new struct to an
> + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
> +
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
That should be -E2BIG.
> + if (unlikely(!access_ok(src, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize)
> + memset(dst + size, 0, rest);
I think kernel style mandates that if one branch in an if-else ladder
requires {} all other must use {} as well. So this should be:
if () {
// one line
} else {
// one line
// another line
}
That's a change in behavior for clone3() and sched at least, no? Unless
- which I guess you might have done - you have moved the "error out when
the struct is too small" part before the call to copy_struct_from_user()
for them.
> + else if (usize > ksize) {
> + const void __user *addr = src + size;
> + char buffer[BUFFER_SIZE] = {};
> +
> + while (rest > 0) {
> + size_t bufsize = min(rest, sizeof(buffer));
> +
> + if (__copy_from_user(buffer, addr, bufsize))
> + return -EFAULT;
> + if (memchr_inv(buffer, 0, bufsize))
> + return -E2BIG;
> +
> + addr += bufsize;
> + rest -= bufsize;
> + }
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_from_user(dst, src, size))
> + return -EFAULT;
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_struct_from_user);
> --
> 2.23.0
>
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Peter Zijlstra @ 2019-09-05 10:57 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Alexander Shishkin, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Ingo Molnar, linux-arm-kernel,
linux-mips, linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn,
linuxppc-dev, linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan
In-Reply-To: <20190905094305.GJ2349@hirez.programming.kicks-ass.net>
On Thu, Sep 05, 2019 at 11:43:05AM +0200, Peter Zijlstra wrote:
> On Thu, Sep 05, 2019 at 07:26:22PM +1000, Aleksa Sarai wrote:
> > On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > > > +/**
> > > > + * copy_struct_to_user: copy a struct to user space
> > > > + * @dst: Destination address, in user space.
> > > > + * @usize: Size of @dst struct.
> > > > + * @src: Source address, in kernel space.
> > > > + * @ksize: Size of @src struct.
> > > > + *
> > > > + * Copies a struct from kernel space to user space, in a way that guarantees
> > > > + * backwards-compatibility for struct syscall arguments (as long as future
> > > > + * struct extensions are made such that all new fields are *appended* to the
> > > > + * old struct, and zeroed-out new fields have the same meaning as the old
> > > > + * struct).
> > > > + *
> > > > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > > > + * The recommended usage is something like the following:
> > > > + *
> > > > + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> > > > + * {
> > > > + * int err;
> > > > + * struct foo karg = {};
> > > > + *
> > > > + * // do something with karg
> > > > + *
> > > > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > > > + * if (err)
> > > > + * return err;
> > > > + *
> > > > + * // ...
> > > > + * }
> > > > + *
> > > > + * There are three cases to consider:
> > > > + * * If @usize == @ksize, then it's copied verbatim.
> > > > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > > > + * older user space. In order to avoid user space getting incomplete
> > > > + * information (new fields might be important), all trailing bytes in @src
> > > > + * (@ksize - @usize) must be zerored
> > >
> > > s/zerored/zero/, right?
> >
> > It should've been "zeroed".
>
> That reads wrong to me; that way it reads like this function must take
> that action and zero out the 'rest'; which is just wrong.
>
> This function must verify those bytes are zero, not make them zero.
>
> > > > , otherwise -EFBIG is returned.
> > >
> > > 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
> >
> > This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
> > a "too big" struct passed to the kernel, and EFBIG for a "too big"
> > struct passed to user-space. I would personally have preferred EMSGSIZE
> > instead of EFBIG, but felt using the existing error codes would be less
> > confusing.
>
> Sadly a recent commit:
>
> 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and robustify sched_read_attr() ABI logic and code")
>
> Made the situation even 'worse'.
And thinking more about things; I'm not convinced the above patch is
actually right.
Do we really want to simply truncate all the attributes of the task?
And should we not at least set sched_flags when there are non-default
clamp values applied?
See; that is I think the primary bug that had chrt failing; we tried to
publish the default clamp values as !0.
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Christian Brauner @ 2019-09-05 10:45 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Rasmus Villemoes, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min, Oleg
In-Reply-To: <20190905095026.gjemg2gqua2vufxb@yavin.dot.cyphar.com>
On Thu, Sep 05, 2019 at 07:50:26PM +1000, Aleksa Sarai wrote:
> On 2019-09-05, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > On 04/09/2019 22.19, Aleksa Sarai wrote:
> > > A common pattern for syscall extensions is increasing the size of a
> > > struct passed from userspace, such that the zero-value of the new fields
> > > result in the old kernel behaviour (allowing for a mix of userspace and
> > > kernel vintages to operate on one another in most cases). This is done
> > > in both directions -- hence two helpers -- though it's more common to
> > > have to copy user space structs into kernel space.
> > >
> > > Previously there was no common lib/ function that implemented
> > > the necessary extension-checking semantics (and different syscalls
> > > implemented them slightly differently or incompletely[1]). A future
> > > patch replaces all of the common uses of this pattern to use the new
> > > copy_struct_{to,from}_user() helpers.
> > >
> > > [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> > > similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> > > always rejects differently-sized struct arguments.
> > >
> > > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > ---
> > > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > > new file mode 100644
> > > index 000000000000..7301ab1bbe98
> > > --- /dev/null
> > > +++ b/lib/struct_user.c
> > > @@ -0,0 +1,182 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Copyright (C) 2019 SUSE LLC
> > > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > > + */
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/export.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/string.h>
> > > +
> > > +#define BUFFER_SIZE 64
> > > +
> > > +/*
> > > + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> > > + * checked access_ok(p, size).
> > > + */
> >
> > Isn't this __clear_user() exactly (perhaps except for the return value)?
> > Perhaps not every arch has that?
>
> I didn't know about clear_user() -- I will switch to it.
>
> > > +static int __memzero_user(void __user *p, size_t s)
> > > +{
> > > + const char zeros[BUFFER_SIZE] = {};
> > > + while (s > 0) {
> > > + size_t n = min(s, sizeof(zeros));
> > > +
> > > + if (__copy_to_user(p, zeros, n))
> > > + return -EFAULT;
> > > +
> > > + p += n;
> > > + s -= n;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * copy_struct_to_user: copy a struct to user space
> > > + * @dst: Destination address, in user space.
> > > + * @usize: Size of @dst struct.
> > > + * @src: Source address, in kernel space.
> > > + * @ksize: Size of @src struct.
> > > + *
> > > + * Returns (in all cases, some data may have been copied):
> > > + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> > > + * * -EFAULT: access to user space failed.
> > > + */
> > > +int copy_struct_to_user(void __user *dst, size_t usize,
> > > + const void *src, size_t ksize)
> > > +{
> > > + size_t size = min(ksize, usize);
> > > + size_t rest = abs(ksize - usize);
> >
> > Eh, I'd avoid abs() here due to the funkiness of the implicit type
> > conversions - ksize-usize has type size_t, then that's coerced to an int
> > (or a long maybe?), the abs is applied which return an int/long (or
> > unsigned versions?). Something like "rest = max(ksize, usize) - size;"
> > is more obviously correct and doesn't fall into any
> > narrowing/widening/sign extending traps.
>
> Yeah, I originally used "max(ksize, usize) - size" for that reason but
> was worried it looked too funky (and some quick tests showed that abs()
> gives the right results in most cases -- though I just realised it would
> probably not give the right results around SIZE_MAX). I'll switch back.
>
> > > + if (unlikely(usize > PAGE_SIZE))
> > > + return -EFAULT;
> >
> > Please don't. That is a restriction on all future extensions - once a
> > kernel is shipped with a syscall using this helper with that arbitrary
> > restriction in place, that syscall is forever prevented from extending
> > its arg struct beyond PAGE_SIZE (which is arch-dependent anyway). Sure,
> > it's hard to imagine, but who'd have thought 32 O_* or CLONE_* bits
> > weren't enough for everybody?
> >
> > This is only for future compatibility, and if someone runs an app
> > compiled against 7.3 headers on a 5.4 kernel, they probably don't care
> > about performance, but they would like their app to run.
>
> I'm not sure I agree that the limit is in place *forever* -- it's
> generally not a break in compatibility to convert an error into a
> success (though, there are counterexamples such as mknod(2) -- but that
> was a very specific case).
>
> You're right that it would mean that some very new code won't run on
> very ancient kernels (assuming we ever pass around structs that
> massive), but there should be a reasonable trade-off here IMHO.
Passing a struct larger than a PAGE_SIZE right now (at least for all
those calls that would make use of this helper at the moment) is to be
considered a bug.
The PAGE_SIZE check is a reasonable heuristic. It's an assumption that
is pretty common in the kernel in other places as well. Plus the
possibility of DoS.
>
> If we allow very large sizes, a program could probably DoS the kernel by
> allocating a moderately-large block of memory and then spawning a bunch
> of threads that all cause the kernel to re-check that the same 1GB block
> of memory is zeroed. I haven't tried, but it seems like it's best to
> avoid the possibility altogether.
>
> > > + }
> > > + /* Copy the interoperable parts of the struct. */
> > > + if (__copy_to_user(dst, src, size))
> > > + return -EFAULT;
> >
> > I think I understand why you put this last instead of handling the
> > buffer in the "natural" order. However,
> > I'm wondering whether we should actually do this copy before checking
> > that the extra kernel bytes are 0 - the user will still be told that
> > there was some extra information via the -EFBIG/-E2BIG return, but maybe
> > in some cases the part he understands is good enough. But I also guess
> > we have to look to existing users to see whether that would prevent them
> > from being converted to using this helper.
> >
> > linux-api folks, WDYT?
>
> Regarding the order, I just copied what sched and perf already do. I
> wouldn't mind doing it the other way around -- though I am a little
> cautious about implicitly making guarantees like that. The syscall that
> uses copy_struct_to_user() might not want to make that guarantee (it
> might not make sense for them), and there are some -E2BIG returns that
> won't result in data being copied (usize > PAGE_SIZE).
>
> As for feedback, this is syscall-dependent at the moment. The sched and
> perf users explicitly return the size of the kernel structure (by
> overwriting uattr->size if -E2BIG is returned) for copies in either
> direction. So users arguably already have some kind of feedback about
> size issues. clone3() on the other hand doesn't do that (though it
> doesn't copy anything to user-space so this isn't relevant to this
> particular question).
>
> Effectively, I'd like to see someone argue that this is something that
> they would personally want (before we do it).
I think the order you have right now is fine. I don't see the point of
doing work first before we have verified that things are sane.
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to, from}_user helpers
From: Gabriel Paubert @ 2019-09-05 10:13 UTC (permalink / raw)
To: Andreas Schwab
Cc: linux-ia64, linux-sh, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, linux-mips, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Alexander Shishkin, Ingo Molnar,
Christian Brauner, linux-xtensa, Kees Cook, Arnd Bergmann,
Jann Horn, Oleg Nesterov, Aleksa Sarai, Al Viro, Andy Lutomirski
In-Reply-To: <mvma7bj85yo.fsf@linux-m68k.org>
On Thu, Sep 05, 2019 at 11:09:35AM +0200, Andreas Schwab wrote:
> On Sep 05 2019, Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > new file mode 100644
> > index 000000000000..7301ab1bbe98
> > --- /dev/null
> > +++ b/lib/struct_user.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 SUSE LLC
> > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/export.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +#define BUFFER_SIZE 64
> > +
> > +/*
> > + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> > + * checked access_ok(p, size).
> > + */
> > +static int __memzero_user(void __user *p, size_t s)
> > +{
> > + const char zeros[BUFFER_SIZE] = {};
>
> Perhaps make that static?
On SMP? It should at least be per cpu, and I'm not even sure
with preemption.
Gabriel
>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 9:50 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Alexander Shishkin <alex>
In-Reply-To: <57ba3752-c4a6-d2a4-1a4d-a0e13bccd473@rasmusvillemoes.dk>
[-- Attachment #1: Type: text/plain, Size: 11250 bytes --]
On 2019-09-05, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> On 04/09/2019 22.19, Aleksa Sarai wrote:
> > A common pattern for syscall extensions is increasing the size of a
> > struct passed from userspace, such that the zero-value of the new fields
> > result in the old kernel behaviour (allowing for a mix of userspace and
> > kernel vintages to operate on one another in most cases). This is done
> > in both directions -- hence two helpers -- though it's more common to
> > have to copy user space structs into kernel space.
> >
> > Previously there was no common lib/ function that implemented
> > the necessary extension-checking semantics (and different syscalls
> > implemented them slightly differently or incompletely[1]). A future
> > patch replaces all of the common uses of this pattern to use the new
> > copy_struct_{to,from}_user() helpers.
> >
> > [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> > similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> > always rejects differently-sized struct arguments.
> >
> > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > new file mode 100644
> > index 000000000000..7301ab1bbe98
> > --- /dev/null
> > +++ b/lib/struct_user.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 SUSE LLC
> > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/export.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +#define BUFFER_SIZE 64
> > +
> > +/*
> > + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> > + * checked access_ok(p, size).
> > + */
>
> Isn't this __clear_user() exactly (perhaps except for the return value)?
> Perhaps not every arch has that?
I didn't know about clear_user() -- I will switch to it.
> > +static int __memzero_user(void __user *p, size_t s)
> > +{
> > + const char zeros[BUFFER_SIZE] = {};
> > + while (s > 0) {
> > + size_t n = min(s, sizeof(zeros));
> > +
> > + if (__copy_to_user(p, zeros, n))
> > + return -EFAULT;
> > +
> > + p += n;
> > + s -= n;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * copy_struct_to_user: copy a struct to user space
> > + * @dst: Destination address, in user space.
> > + * @usize: Size of @dst struct.
> > + * @src: Source address, in kernel space.
> > + * @ksize: Size of @src struct.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_to_user(void __user *dst, size_t usize,
> > + const void *src, size_t ksize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
>
> Eh, I'd avoid abs() here due to the funkiness of the implicit type
> conversions - ksize-usize has type size_t, then that's coerced to an int
> (or a long maybe?), the abs is applied which return an int/long (or
> unsigned versions?). Something like "rest = max(ksize, usize) - size;"
> is more obviously correct and doesn't fall into any
> narrowing/widening/sign extending traps.
Yeah, I originally used "max(ksize, usize) - size" for that reason but
was worried it looked too funky (and some quick tests showed that abs()
gives the right results in most cases -- though I just realised it would
probably not give the right results around SIZE_MAX). I'll switch back.
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Please don't. That is a restriction on all future extensions - once a
> kernel is shipped with a syscall using this helper with that arbitrary
> restriction in place, that syscall is forever prevented from extending
> its arg struct beyond PAGE_SIZE (which is arch-dependent anyway). Sure,
> it's hard to imagine, but who'd have thought 32 O_* or CLONE_* bits
> weren't enough for everybody?
>
> This is only for future compatibility, and if someone runs an app
> compiled against 7.3 headers on a 5.4 kernel, they probably don't care
> about performance, but they would like their app to run.
I'm not sure I agree that the limit is in place *forever* -- it's
generally not a break in compatibility to convert an error into a
success (though, there are counterexamples such as mknod(2) -- but that
was a very specific case).
You're right that it would mean that some very new code won't run on
very ancient kernels (assuming we ever pass around structs that
massive), but there should be a reasonable trade-off here IMHO.
If we allow very large sizes, a program could probably DoS the kernel by
allocating a moderately-large block of memory and then spawning a bunch
of threads that all cause the kernel to re-check that the same 1GB block
of memory is zeroed. I haven't tried, but it seems like it's best to
avoid the possibility altogether.
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_to_user(dst, src, size))
> > + return -EFAULT;
>
> I think I understand why you put this last instead of handling the
> buffer in the "natural" order. However,
> I'm wondering whether we should actually do this copy before checking
> that the extra kernel bytes are 0 - the user will still be told that
> there was some extra information via the -EFBIG/-E2BIG return, but maybe
> in some cases the part he understands is good enough. But I also guess
> we have to look to existing users to see whether that would prevent them
> from being converted to using this helper.
>
> linux-api folks, WDYT?
Regarding the order, I just copied what sched and perf already do. I
wouldn't mind doing it the other way around -- though I am a little
cautious about implicitly making guarantees like that. The syscall that
uses copy_struct_to_user() might not want to make that guarantee (it
might not make sense for them), and there are some -E2BIG returns that
won't result in data being copied (usize > PAGE_SIZE).
As for feedback, this is syscall-dependent at the moment. The sched and
perf users explicitly return the size of the kernel structure (by
overwriting uattr->size if -E2BIG is returned) for copies in either
direction. So users arguably already have some kind of feedback about
size issues. clone3() on the other hand doesn't do that (though it
doesn't copy anything to user-space so this isn't relevant to this
particular question).
Effectively, I'd like to see someone argue that this is something that
they would personally want (before we do it).
> > + return 0;
>
> Maybe more useful to "return size;", some users might want to know/pass
> on how much was actually copied.
Even though it is "just" min(ksize, usize), I don't see any harm in
returning it. Will do.
> > +}
> > +EXPORT_SYMBOL(copy_struct_to_user);
>
> Can't we wait with this until a modular user shows up? The primary users
> are syscalls, which can't be modular AFAIK.
Yeah, I'll drop it. You could use them for ioctl()s but we can always
add EXPORT_SYMBOL() later.
> > +/**
> > + * copy_struct_from_user: copy a struct from user space
> > + * @dst: Destination address, in kernel space. This buffer must be @ksize
> > + * bytes long.
> > + * @ksize: Size of @dst struct.
> > + * @src: Source address, in user space.
> > + * @usize: (Alleged) size of @src struct.
> > + *
> > + * Copies a struct from user space to kernel space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then the user space has passed an old struct to a
> > + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> > + * are to be zero-filled.
> > + * * If @usize > @ksize, then the user space has passed a new struct to an
> > + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> > + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> > + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_from_user(void *dst, size_t ksize,
> > + const void __user *src, size_t usize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
>
> As above.
>
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> As above.
>
> > + if (unlikely(!access_ok(src, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize)
> > + memset(dst + size, 0, rest);
> > + else if (usize > ksize) {
> > + const void __user *addr = src + size;
> > + char buffer[BUFFER_SIZE] = {};
> > +
> > + while (rest > 0) {
> > + size_t bufsize = min(rest, sizeof(buffer));
> > +
> > + if (__copy_from_user(buffer, addr, bufsize))
> > + return -EFAULT;
> > + if (memchr_inv(buffer, 0, bufsize))
> > + return -E2BIG;
> > +
> > + addr += bufsize;
> > + rest -= bufsize;
> > + }
>
> I'd create a __user_is_zero() helper for this - that way the two
> branches in the two helpers become nicely symmetric, each just calling a
> single helper that deals appropriately with the tail. And we can discuss
> how to implement __user_is_zero() in another bikeshed.
Will do.
>
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_from_user(dst, src, size))
> > + return -EFAULT;
>
> If you do move up the __copy_to_user(), please move this as well - on
> the kernel side, we certainly don't care that we copied some bytes to a
> local buffer which we then ignore because the user had a non-zero tail.
> But if __copy_to_user() is kept last in copy_struct_to_user(), this
> should stay for symmetry.
I will keep that in mind.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Peter Zijlstra @ 2019-09-05 9:43 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Alexander Shishkin, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Ingo Molnar, linux-arm-kernel,
linux-mips, linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn,
linuxppc-dev, linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan
In-Reply-To: <20190905092622.tlb6nn3uisssdfbu@yavin.dot.cyphar.com>
On Thu, Sep 05, 2019 at 07:26:22PM +1000, Aleksa Sarai wrote:
> On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > > +/**
> > > + * copy_struct_to_user: copy a struct to user space
> > > + * @dst: Destination address, in user space.
> > > + * @usize: Size of @dst struct.
> > > + * @src: Source address, in kernel space.
> > > + * @ksize: Size of @src struct.
> > > + *
> > > + * Copies a struct from kernel space to user space, in a way that guarantees
> > > + * backwards-compatibility for struct syscall arguments (as long as future
> > > + * struct extensions are made such that all new fields are *appended* to the
> > > + * old struct, and zeroed-out new fields have the same meaning as the old
> > > + * struct).
> > > + *
> > > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > > + * The recommended usage is something like the following:
> > > + *
> > > + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> > > + * {
> > > + * int err;
> > > + * struct foo karg = {};
> > > + *
> > > + * // do something with karg
> > > + *
> > > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > > + * if (err)
> > > + * return err;
> > > + *
> > > + * // ...
> > > + * }
> > > + *
> > > + * There are three cases to consider:
> > > + * * If @usize == @ksize, then it's copied verbatim.
> > > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > > + * older user space. In order to avoid user space getting incomplete
> > > + * information (new fields might be important), all trailing bytes in @src
> > > + * (@ksize - @usize) must be zerored
> >
> > s/zerored/zero/, right?
>
> It should've been "zeroed".
That reads wrong to me; that way it reads like this function must take
that action and zero out the 'rest'; which is just wrong.
This function must verify those bytes are zero, not make them zero.
> > > , otherwise -EFBIG is returned.
> >
> > 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
>
> This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
> a "too big" struct passed to the kernel, and EFBIG for a "too big"
> struct passed to user-space. I would personally have preferred EMSGSIZE
> instead of EFBIG, but felt using the existing error codes would be less
> confusing.
Sadly a recent commit:
1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and robustify sched_read_attr() ABI logic and code")
Made the situation even 'worse'.
> > > + if (unlikely(!access_ok(src, usize)))
> > > + return -EFAULT;
> > > +
> > > + /* Deal with trailing bytes. */
> > > + if (usize < ksize)
> > > + memset(dst + size, 0, rest);
> > > + else if (usize > ksize) {
> > > + const void __user *addr = src + size;
> > > + char buffer[BUFFER_SIZE] = {};
> >
> > Isn't that too big for on-stack?
>
> Is a 64-byte buffer too big? I picked the number "at random" to be the
> size of a cache line, but I could shrink it down to 32 bytes if the size
> is an issue (I wanted to avoid needless allocations -- hence it being
> on-stack).
Ah, my ctags gave me a definition of BUFFER_SIZE that was 512. I suppose
64 should be OK.
> > > +
> > > + while (rest > 0) {
> > > + size_t bufsize = min(rest, sizeof(buffer));
> > > +
> > > + if (__copy_from_user(buffer, addr, bufsize))
> > > + return -EFAULT;
> > > + if (memchr_inv(buffer, 0, bufsize))
> > > + return -E2BIG;
> > > +
> > > + addr += bufsize;
> > > + rest -= bufsize;
> > > + }
> >
> > The perf implementation uses get_user(); but if that is too slow, surely
> > we can do something with uaccess_try() here?
>
> Is there a non-x86-specific way to do that (unless I'm mistaken only x86
> has uaccess_try() or the other *_try() wrappers)? The main "performance
> improvement" (if you can even call it that) is that we use memchr_inv()
> which finds non-matching characters more efficiently than just doing a
> loop.
Oh, you're right, that's x86 only :/
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 9:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Christian Brauner, Rasmus Villemoes, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Alexander
In-Reply-To: <20190905073205.GY2332@hirez.programming.kicks-ass.net>
[-- Attachment #1: Type: text/plain, Size: 7846 bytes --]
On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > +/**
> > + * copy_struct_to_user: copy a struct to user space
> > + * @dst: Destination address, in user space.
> > + * @usize: Size of @dst struct.
> > + * @src: Source address, in kernel space.
> > + * @ksize: Size of @src struct.
> > + *
> > + * Copies a struct from kernel space to user space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * // do something with karg
> > + *
> > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > + * older user space. In order to avoid user space getting incomplete
> > + * information (new fields might be important), all trailing bytes in @src
> > + * (@ksize - @usize) must be zerored
>
> s/zerored/zero/, right?
It should've been "zeroed".
> > , otherwise -EFBIG is returned.
>
> 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
a "too big" struct passed to the kernel, and EFBIG for a "too big"
struct passed to user-space. I would personally have preferred EMSGSIZE
instead of EFBIG, but felt using the existing error codes would be less
confusing.
>
> > + * * If @usize > @ksize, then the kernel is "returning" an older struct to a
> > + * newer user space. The trailing bytes in @dst (@usize - @ksize) will be
> > + * zero-filled.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_to_user(void __user *dst, size_t usize,
> > + const void *src, size_t ksize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
> > +
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Not documented above. Implementation consistent with *from*, but see
> below.
Will update the kernel-doc.
> > + if (unlikely(!access_ok(dst, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize) {
> > + if (memchr_inv(src + size, 0, rest))
> > + return -EFBIG;
> > + } else if (usize > ksize) {
> > + if (__memzero_user(dst + size, rest))
> > + return -EFAULT;
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_to_user(dst, src, size))
> > + return -EFAULT;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(copy_struct_to_user);
> > +
> > +/**
> > + * copy_struct_from_user: copy a struct from user space
> > + * @dst: Destination address, in kernel space. This buffer must be @ksize
> > + * bytes long.
> > + * @ksize: Size of @dst struct.
> > + * @src: Source address, in user space.
> > + * @usize: (Alleged) size of @src struct.
> > + *
> > + * Copies a struct from user space to kernel space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then the user space has passed an old struct to a
> > + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> > + * are to be zero-filled.
> > + * * If @usize > @ksize, then the user space has passed a new struct to an
> > + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> > + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> > + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_from_user(void *dst, size_t ksize,
> > + const void __user *src, size_t usize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
> > +
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Documented above as returning -E2BIG.
I will switch this (and to) back to -E2BIG -- I must've had a brain-fart
when doing some refactoring.
>
> > + if (unlikely(!access_ok(src, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize)
> > + memset(dst + size, 0, rest);
> > + else if (usize > ksize) {
> > + const void __user *addr = src + size;
> > + char buffer[BUFFER_SIZE] = {};
>
> Isn't that too big for on-stack?
Is a 64-byte buffer too big? I picked the number "at random" to be the
size of a cache line, but I could shrink it down to 32 bytes if the size
is an issue (I wanted to avoid needless allocations -- hence it being
on-stack).
> > +
> > + while (rest > 0) {
> > + size_t bufsize = min(rest, sizeof(buffer));
> > +
> > + if (__copy_from_user(buffer, addr, bufsize))
> > + return -EFAULT;
> > + if (memchr_inv(buffer, 0, bufsize))
> > + return -E2BIG;
> > +
> > + addr += bufsize;
> > + rest -= bufsize;
> > + }
>
> The perf implementation uses get_user(); but if that is too slow, surely
> we can do something with uaccess_try() here?
Is there a non-x86-specific way to do that (unless I'm mistaken only x86
has uaccess_try() or the other *_try() wrappers)? The main "performance
improvement" (if you can even call it that) is that we use memchr_inv()
which finds non-matching characters more efficiently than just doing a
loop.
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_from_user(dst, src, size))
> > + return -EFAULT;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(copy_struct_from_user);
>
> And personally I'm not a big fan of EXPORT_SYMBOL().
I don't have much of an opinion (after all, it only really makes sense a
lot of sense for syscalls) -- though out-of-tree modules that define
ioctl()s wouldn't be able to make use of them.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Andreas Schwab @ 2019-09-05 9:09 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Rasmus Villemoes,
Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On Sep 05 2019, Aleksa Sarai <cyphar@cyphar.com> wrote:
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..7301ab1bbe98
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +#define BUFFER_SIZE 64
> +
> +/*
> + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> + * checked access_ok(p, size).
> + */
> +static int __memzero_user(void __user *p, size_t s)
> +{
> + const char zeros[BUFFER_SIZE] = {};
Perhaps make that static?
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Rasmus Villemoes @ 2019-09-05 8:43 UTC (permalink / raw)
To: Aleksa Sarai, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Tycho Andersen,
David Drysdale, Chanho Min, Oleg Nesterov, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Aleksa Sarai, Linus Torvalds, containers,
linux-alpha, linux-api, linux-arch, linux-arm-kernel,
linux-fsdevel, linux-ia6
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On 04/09/2019 22.19, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases). This is done
> in both directions -- hence two helpers -- though it's more common to
> have to copy user space structs into kernel space.
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[1]). A future
> patch replaces all of the common uses of this pattern to use the new
> copy_struct_{to,from}_user() helpers.
>
> [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..7301ab1bbe98
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +#define BUFFER_SIZE 64
> +
> +/*
> + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> + * checked access_ok(p, size).
> + */
Isn't this __clear_user() exactly (perhaps except for the return value)?
Perhaps not every arch has that?
> +static int __memzero_user(void __user *p, size_t s)
> +{
> + const char zeros[BUFFER_SIZE] = {};
> + while (s > 0) {
> + size_t n = min(s, sizeof(zeros));
> +
> + if (__copy_to_user(p, zeros, n))
> + return -EFAULT;
> +
> + p += n;
> + s -= n;
> + }
> + return 0;
> +}
> +
> +/**
> + * copy_struct_to_user: copy a struct to user space
> + * @dst: Destination address, in user space.
> + * @usize: Size of @dst struct.
> + * @src: Source address, in kernel space.
> + * @ksize: Size of @src struct.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_to_user(void __user *dst, size_t usize,
> + const void *src, size_t ksize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
Eh, I'd avoid abs() here due to the funkiness of the implicit type
conversions - ksize-usize has type size_t, then that's coerced to an int
(or a long maybe?), the abs is applied which return an int/long (or
unsigned versions?). Something like "rest = max(ksize, usize) - size;"
is more obviously correct and doesn't fall into any
narrowing/widening/sign extending traps.
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
Please don't. That is a restriction on all future extensions - once a
kernel is shipped with a syscall using this helper with that arbitrary
restriction in place, that syscall is forever prevented from extending
its arg struct beyond PAGE_SIZE (which is arch-dependent anyway). Sure,
it's hard to imagine, but who'd have thought 32 O_* or CLONE_* bits
weren't enough for everybody?
This is only for future compatibility, and if someone runs an app
compiled against 7.3 headers on a 5.4 kernel, they probably don't care
about performance, but they would like their app to run.
[If we ever create such a large ABI struct that doesn't fit on stack,
we'd have to extend our API a little to create a dup_struct_from_user()
that does the kmalloc() for us and then calls copy_struct_from_user() -
but we might want that long before we hit PAGE_SIZE structs].
> + if (unlikely(!access_ok(dst, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize) {
> + if (memchr_inv(src + size, 0, rest))
> + return -EFBIG;
> + } else if (usize > ksize) {
> + if (__memzero_user(dst + size, rest))
> + return -EFAULT;
I think that could simply be __clear_user().
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_to_user(dst, src, size))
> + return -EFAULT;
I think I understand why you put this last instead of handling the
buffer in the "natural" order. However,
I'm wondering whether we should actually do this copy before checking
that the extra kernel bytes are 0 - the user will still be told that
there was some extra information via the -EFBIG/-E2BIG return, but maybe
in some cases the part he understands is good enough. But I also guess
we have to look to existing users to see whether that would prevent them
from being converted to using this helper.
linux-api folks, WDYT?
> + return 0;
Maybe more useful to "return size;", some users might want to know/pass
on how much was actually copied.
> +}
> +EXPORT_SYMBOL(copy_struct_to_user);
Can't we wait with this until a modular user shows up? The primary users
are syscalls, which can't be modular AFAIK.
> +/**
> + * copy_struct_from_user: copy a struct from user space
> + * @dst: Destination address, in kernel space. This buffer must be @ksize
> + * bytes long.
> + * @ksize: Size of @dst struct.
> + * @src: Source address, in user space.
> + * @usize: (Alleged) size of @src struct.
> + *
> + * Copies a struct from user space to kernel space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then the user space has passed an old struct to a
> + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> + * are to be zero-filled.
> + * * If @usize > @ksize, then the user space has passed a new struct to an
> + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
As above.
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
As above.
> + if (unlikely(!access_ok(src, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize)
> + memset(dst + size, 0, rest);
> + else if (usize > ksize) {
> + const void __user *addr = src + size;
> + char buffer[BUFFER_SIZE] = {};
> +
> + while (rest > 0) {
> + size_t bufsize = min(rest, sizeof(buffer));
> +
> + if (__copy_from_user(buffer, addr, bufsize))
> + return -EFAULT;
> + if (memchr_inv(buffer, 0, bufsize))
> + return -E2BIG;
> +
> + addr += bufsize;
> + rest -= bufsize;
> + }
I'd create a __user_is_zero() helper for this - that way the two
branches in the two helpers become nicely symmetric, each just calling a
single helper that deals appropriately with the tail. And we can discuss
how to implement __user_is_zero() in another bikeshed.
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_from_user(dst, src, size))
> + return -EFAULT;
If you do move up the __copy_to_user(), please move this as well - on
the kernel side, we certainly don't care that we copied some bytes to a
local buffer which we then ignore because the user had a non-zero tail.
But if __copy_to_user() is kept last in copy_struct_to_user(), this
should stay for symmetry.
> + return 0;
As above.
> +}
> +EXPORT_SYMBOL(copy_struct_from_user);
As above.
Rasmus
^ permalink raw reply
* Re: [PATCH v2 bpf-next 2/3] bpf: implement CAP_BPF
From: Daniel Borkmann @ 2019-09-05 8:37 UTC (permalink / raw)
To: Alexei Starovoitov, nicolas.dichtel@6wind.com, Alexei Starovoitov
Cc: Alexei Starovoitov, luto@amacapital.net, davem@davemloft.net,
peterz@infradead.org, rostedt@goodmis.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team, linux-api@vger.kernel.org
In-Reply-To: <99acd443-69d7-f53a-1af0-263e0b73abef@fb.com>
On 9/4/19 5:21 PM, Alexei Starovoitov wrote:
> On 9/4/19 8:16 AM, Daniel Borkmann wrote:
>> opening/creating BPF maps" error="Unable to create map
>> /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted"
>> subsys=daemon
>> 2019-09-04T14:11:47.28178666Z level=fatal msg="Error while creating
>> daemon" error="Unable to create map
>> /run/cilium/bpffs/tc/globals/cilium_lxc: operation not permitted"
>> subsys=daemon
>
> Ok. We have to include caps in both cap_sys_admin and cap_bpf then.
>
>> And /same/ deployment with reverted patches, hence no CAP_BPF gets it up
>> and running again:
>>
>> # kubectl get pods --all-namespaces -o wide
>
> Can you share what this magic commands do underneath?
What do you mean by magic commands? Latter is showing all pods in the cluster:
https://kubernetes.io/docs/reference/kubectl/cheatsheet/#viewing-finding-resources
I've only been using the normal kubeadm guide for setup, it's pretty straight
forward, just the kubeadm init to bootstrap and then the kubectl create for
deploying if you need to give it a spin for testing:
https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/create-cluster-kubeadm/#tabs-pod-install-4
> What user do they pick to start under? and what caps are granted?
The deployment is using a 'securityContext' with 'privileged: true' for the
the container spec as majority of CNIs do. My understanding is that this is
passed down to the underlying container runtime e.g. docker as one option.
Checking moby go code, it seems to exec with GetAllCapabilities which returns
all of the capabilities it is aware of, and afaics, they seem to be using
the below go library to get the hard-coded list from where obviously CAP_BPF
is unknown which might also explain the breakage I've been seeing:
https://github.com/syndtr/gocapability/blob/33e07d32887e1e06b7c025f27ce52f62c7990bc0/capability/enum_gen.go
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Peter Zijlstra @ 2019-09-05 7:32 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Christian Brauner, Rasmus Villemoes, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Kees Cook,
Jann Horn, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Alexander
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> +/**
> + * copy_struct_to_user: copy a struct to user space
> + * @dst: Destination address, in user space.
> + * @usize: Size of @dst struct.
> + * @src: Source address, in kernel space.
> + * @ksize: Size of @src struct.
> + *
> + * Copies a struct from kernel space to user space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * // do something with karg
> + *
> + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> + * older user space. In order to avoid user space getting incomplete
> + * information (new fields might be important), all trailing bytes in @src
> + * (@ksize - @usize) must be zerored
s/zerored/zero/, right?
> , otherwise -EFBIG is returned.
'Funny' that, copy_struct_from_user() below seems to use E2BIG.
> + * * If @usize > @ksize, then the kernel is "returning" an older struct to a
> + * newer user space. The trailing bytes in @dst (@usize - @ksize) will be
> + * zero-filled.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_to_user(void __user *dst, size_t usize,
> + const void *src, size_t ksize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
> +
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
Not documented above. Implementation consistent with *from*, but see
below.
> + if (unlikely(!access_ok(dst, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize) {
> + if (memchr_inv(src + size, 0, rest))
> + return -EFBIG;
> + } else if (usize > ksize) {
> + if (__memzero_user(dst + size, rest))
> + return -EFAULT;
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_to_user(dst, src, size))
> + return -EFAULT;
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_struct_to_user);
> +
> +/**
> + * copy_struct_from_user: copy a struct from user space
> + * @dst: Destination address, in kernel space. This buffer must be @ksize
> + * bytes long.
> + * @ksize: Size of @dst struct.
> + * @src: Source address, in user space.
> + * @usize: (Alleged) size of @src struct.
> + *
> + * Copies a struct from user space to kernel space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then the user space has passed an old struct to a
> + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> + * are to be zero-filled.
> + * * If @usize > @ksize, then the user space has passed a new struct to an
> + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
> +
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
Documented above as returning -E2BIG.
> + if (unlikely(!access_ok(src, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize)
> + memset(dst + size, 0, rest);
> + else if (usize > ksize) {
> + const void __user *addr = src + size;
> + char buffer[BUFFER_SIZE] = {};
Isn't that too big for on-stack?
> +
> + while (rest > 0) {
> + size_t bufsize = min(rest, sizeof(buffer));
> +
> + if (__copy_from_user(buffer, addr, bufsize))
> + return -EFAULT;
> + if (memchr_inv(buffer, 0, bufsize))
> + return -E2BIG;
> +
> + addr += bufsize;
> + rest -= bufsize;
> + }
The perf implementation uses get_user(); but if that is too slow, surely
we can do something with uaccess_try() here?
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_from_user(dst, src, size))
> + return -EFAULT;
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_struct_from_user);
And personally I'm not a big fan of EXPORT_SYMBOL().
^ permalink raw reply
* Re: [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-09-05 3:15 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, David S . Miller, Daniel Borkmann,
Peter Zijlstra, Andy Lutomirski, Networking, bpf, Kernel Team,
linux-api@vger.kernel.org
In-Reply-To: <E342EC2A-24F6-4581-BFDC-119B5E02B560@fb.com>
On Thu, Sep 05, 2019 at 02:51:51AM +0000, Song Liu wrote:
>
>
> > On Sep 4, 2019, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Sep 05, 2019 at 12:34:36AM +0000, Song Liu wrote:
> >>
> >>
> >>> On Sep 4, 2019, at 11:43 AM, Alexei Starovoitov <ast@kernel.org> wrote:
> >>>
> >>> Implement permissions as stated in uapi/linux/capability.h
> >>>
> >>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >>>
> >>
> >> [...]
> >>
> >>> @@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
> >>> is_gpl = license_is_gpl_compatible(license);
> >>>
> >>> if (attr->insn_cnt == 0 ||
> >>> - attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> >>> + attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> >>> return -E2BIG;
> >>> if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> >>> type != BPF_PROG_TYPE_CGROUP_SKB &&
> >>> - !capable(CAP_SYS_ADMIN))
> >>> + !capable_bpf())
> >>> return -EPERM;
> >>
> >> Do we allow load BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB
> >> without CAP_BPF? If so, maybe highlight in the header?
> >
> > of course. there is no change in behavior.
> > 'highlight in the header'?
> > you mean in commit log?
> > I think it's a bit weird to describe things in commit that patch
> > is _not_ changing vs things that patch does actually change.
> > This type of comment would be great in a doc though.
> > The doc will be coming separately in the follow up assuming
> > the whole thing lands. I'll remember to note that bit.
>
> I meant capability.h:
>
> + * CAP_BPF allows the following BPF operations:
> + * - Loading all types of BPF programs
>
> But CAP_BPF is not required to load all types of programs.
yes, but above statement is still correct, right?
And right below it says:
* CAP_BPF allows the following BPF operations:
* - Loading all types of BPF programs
* - Creating all types of BPF maps except:
* - stackmap that needs CAP_TRACING
* - devmap that needs CAP_NET_ADMIN
* - cpumap that needs CAP_SYS_ADMIN
which is also correct, but CAP_BPF is not required
for array, hash, prog_array, percpu, map-in-map ...
except their lru variants...
and except if they contain bpf_spin_lock...
and if they need BTF it currently can be loaded with cap_sys_admin only...
If we say something about socket_filter, cg_skb progs in capability.h
we should clarify maps as well, but then it will become too big for .h
The comments in capability.h already look too long to me.
All that info and a lot more belongs in the doc.
> On a second thought, I am not sure whether we will keep capability.h
> up to date with all features. So this is probably OK.
It's clearly not for cap_sys_admin.
cap_net_admin is not up to date either.
These two are kitchen sink of anything root-like and networking-root like.
Developers didn't bother updating that .h
With CAP_BPF we can enforce through patch acceptance policy that
major new things (like big verifier features) should have a line
in capability.h
Though .h is not a replacement for proper doc which will come as follow up.
Unlike bpf.h that serves as a template for auto-generating man pages
capability.h is not such thing. I won't change often either.
So normal doc is a better way to document all details.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF
From: Song Liu @ 2019-09-05 2:51 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, David S . Miller, Daniel Borkmann,
Peter Zijlstra, Andy Lutomirski, Networking, bpf, Kernel Team,
linux-api@vger.kernel.org
In-Reply-To: <20190905013245.wguhhcxvxt5rnc6h@ast-mbp.dhcp.thefacebook.com>
> On Sep 4, 2019, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Sep 05, 2019 at 12:34:36AM +0000, Song Liu wrote:
>>
>>
>>> On Sep 4, 2019, at 11:43 AM, Alexei Starovoitov <ast@kernel.org> wrote:
>>>
>>> Implement permissions as stated in uapi/linux/capability.h
>>>
>>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>>>
>>
>> [...]
>>
>>> @@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>>> is_gpl = license_is_gpl_compatible(license);
>>>
>>> if (attr->insn_cnt == 0 ||
>>> - attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
>>> + attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
>>> return -E2BIG;
>>> if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
>>> type != BPF_PROG_TYPE_CGROUP_SKB &&
>>> - !capable(CAP_SYS_ADMIN))
>>> + !capable_bpf())
>>> return -EPERM;
>>
>> Do we allow load BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB
>> without CAP_BPF? If so, maybe highlight in the header?
>
> of course. there is no change in behavior.
> 'highlight in the header'?
> you mean in commit log?
> I think it's a bit weird to describe things in commit that patch
> is _not_ changing vs things that patch does actually change.
> This type of comment would be great in a doc though.
> The doc will be coming separately in the follow up assuming
> the whole thing lands. I'll remember to note that bit.
I meant capability.h:
+ * CAP_BPF allows the following BPF operations:
+ * - Loading all types of BPF programs
But CAP_BPF is not required to load all types of programs.
On a second thought, I am not sure whether we will keep capability.h
up to date with all features. So this is probably OK.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF
From: Alexei Starovoitov @ 2019-09-05 1:32 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, David S . Miller, Daniel Borkmann,
Peter Zijlstra, Andy Lutomirski, Networking, bpf, Kernel Team,
linux-api@vger.kernel.org
In-Reply-To: <CE3B644F-D1A5-49F7-96B6-FD663C5F8961@fb.com>
On Thu, Sep 05, 2019 at 12:34:36AM +0000, Song Liu wrote:
>
>
> > On Sep 4, 2019, at 11:43 AM, Alexei Starovoitov <ast@kernel.org> wrote:
> >
> > Implement permissions as stated in uapi/linux/capability.h
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >
>
> [...]
>
> > @@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
> > is_gpl = license_is_gpl_compatible(license);
> >
> > if (attr->insn_cnt == 0 ||
> > - attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> > + attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> > return -E2BIG;
> > if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> > type != BPF_PROG_TYPE_CGROUP_SKB &&
> > - !capable(CAP_SYS_ADMIN))
> > + !capable_bpf())
> > return -EPERM;
>
> Do we allow load BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB
> without CAP_BPF? If so, maybe highlight in the header?
of course. there is no change in behavior.
'highlight in the header'?
you mean in commit log?
I think it's a bit weird to describe things in commit that patch
is _not_ changing vs things that patch does actually change.
This type of comment would be great in a doc though.
The doc will be coming separately in the follow up assuming
the whole thing lands. I'll remember to note that bit.
^ permalink raw reply
* [RFC] Add critical process prctl
From: Daniel Colascione @ 2019-09-05 0:53 UTC (permalink / raw)
To: dancol, timmurray, surenb, linux-kernel, linux-api
A task with CAP_SYS_ADMIN can mark itself PR_SET_TASK_CRITICAL,
meaning that if the task ever exits, the kernel panics. This facility
is intended for use by low-level core system processes that cannot
gracefully restart without a reboot. This prctl allows these processes
to ensure that the system restarts when they die regardless of whether
the rest of userspace is operational.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
include/linux/sched.h | 5 +++++
include/uapi/linux/prctl.h | 5 +++++
kernel/exit.c | 2 ++
kernel/sys.c | 19 +++++++++++++++++++
4 files changed, 31 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..29420b9ebb63 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1526,6 +1526,7 @@ static inline bool is_percpu_thread(void)
#define PFA_SPEC_IB_DISABLE 5 /* Indirect branch speculation restricted */
#define PFA_SPEC_IB_FORCE_DISABLE 6 /* Indirect branch speculation permanently restricted */
#define PFA_SPEC_SSB_NOEXEC 7 /* Speculative Store Bypass clear on execve() */
+#define PFA_CRITICAL 8 /* Panic system if process exits */
#define TASK_PFA_TEST(name, func) \
static inline bool task_##func(struct task_struct *p) \
@@ -1568,6 +1569,10 @@ TASK_PFA_CLEAR(SPEC_IB_DISABLE, spec_ib_disable)
TASK_PFA_TEST(SPEC_IB_FORCE_DISABLE, spec_ib_force_disable)
TASK_PFA_SET(SPEC_IB_FORCE_DISABLE, spec_ib_force_disable)
+TASK_PFA_TEST(CRITICAL, critical)
+TASK_PFA_SET(CRITICAL, critical)
+TASK_PFA_CLEAR(CRITICAL, critical)
+
static inline void
current_restore_flags(unsigned long orig_flags, unsigned long flags)
{
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 094bb03b9cc2..4964723bbd47 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -229,4 +229,9 @@ struct prctl_mm_map {
# define PR_PAC_APDBKEY (1UL << 3)
# define PR_PAC_APGAKEY (1UL << 4)
+/* Per-task criticality control */
+#define PR_SET_TASK_CRITICAL 55
+#define PR_CRITICAL_NOT_CRITICAL 0
+#define PR_CRITICAL_CRITICAL 1
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index 5b4a5dcce8f8..9b3d3411d935 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -788,6 +788,8 @@ void __noreturn do_exit(long code)
panic("Aiee, killing interrupt handler!");
if (unlikely(!tsk->pid))
panic("Attempted to kill the idle task!");
+ if (unlikely(task_critical(tsk)))
+ panic("Critical task died!");
/*
* If do_exit is called because this processes oopsed, it's possible
diff --git a/kernel/sys.c b/kernel/sys.c
index 2969304c29fe..097e05ebaf94 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2269,6 +2269,20 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which,
return -EINVAL;
}
+int task_do_set_critical(struct task_struct *t, unsigned long opt)
+{
+ if (opt != PR_CRITICAL_NOT_CRITICAL &&
+ opt != PR_CRITICAL_CRITICAL)
+ return -EINVAL;
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ if (opt == PR_CRITICAL_NOT_CRITICAL)
+ task_clear_critical(t);
+ else
+ task_set_critical(t);
+ return 0;
+}
+
SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
unsigned long, arg4, unsigned long, arg5)
{
@@ -2492,6 +2506,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
error = PAC_RESET_KEYS(me, arg2);
break;
+ case PR_SET_TASK_CRITICAL:
+ if (arg3 || arg4 || arg5)
+ return -EINVAL;
+ error = task_do_set_critical(me, arg2);
+ break;
default:
error = -EINVAL;
break;
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* Re: [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF
From: Song Liu @ 2019-09-05 0:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S . Miller, Daniel Borkmann, Peter Zijlstra,
Andy Lutomirski, Networking, bpf, Kernel Team,
linux-api@vger.kernel.org
In-Reply-To: <20190904184335.360074-2-ast@kernel.org>
> On Sep 4, 2019, at 11:43 AM, Alexei Starovoitov <ast@kernel.org> wrote:
>
> Implement permissions as stated in uapi/linux/capability.h
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
[...]
> @@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
> is_gpl = license_is_gpl_compatible(license);
>
> if (attr->insn_cnt == 0 ||
> - attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> + attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
> return -E2BIG;
> if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> type != BPF_PROG_TYPE_CGROUP_SKB &&
> - !capable(CAP_SYS_ADMIN))
> + !capable_bpf())
> return -EPERM;
Do we allow load BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB
without CAP_BPF? If so, maybe highlight in the header?
Thanks,
Song
^ permalink raw reply
* Re: [PATCH v12 10/12] namei: aggressively check for nd->root escape on ".." resolution
From: Linus Torvalds @ 2019-09-04 23:44 UTC (permalink / raw)
To: Al Viro
Cc: linux-ia64, Linux-sh list, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, Linux List Kernel Mailing, J. Bruce Fields,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai, Jiri Olsa,
Alexander Shishkin, Ingo Molnar, Linux ARM, linux-mips,
linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn
In-Reply-To: <20190904232911.GN1131@ZenIV.linux.org.uk>
On Wed, Sep 4, 2019 at 4:29 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 04, 2019 at 03:38:20PM -0700, Linus Torvalds wrote:
> > On Wed, Sep 4, 2019 at 3:31 PM David Howells <dhowells@redhat.com> wrote:
> > >
> > > It ought to be reasonably easy to make them per-sb at least, I think. We
> > > don't allow cross-super rename, right?
> >
> > Right now the sequence count handling very much depends on it being a
> > global entity on the reader side, at least.
> >
> > And while the rename sequence count could (and probably should) be
> > per-sb, the same is very much not true of the mount one.
>
> Huh? That will cost us having to have a per-superblock dentry
> hash table; recall that lockless lockup can give false negatives
> if something gets moved from chain to chain, and rename_lock is
> first and foremost used to catch those and retry. If we split
> it on per-superblock basis, we can't have dentries from different
> superblocks in the same chain anymore...
That's exactly the "very much depends on it being a global entity on
the reader side" thing.
I'm not convinced that's the _only_ way to handle things. Maybe a
combination of (wild handwaving) per-hashqueue sequence count and some
clever scheme for pathname handling could work.
I've not personally seen a load where the global rename lock has been
a problem (very few things really do a lot of renames), but
system-wide locks do make me nervous.
We have other (and worse) ones. tasklist_lock comes to mind.
Linus
^ permalink raw reply
* Re: [PATCH v12 10/12] namei: aggressively check for nd->root escape on ".." resolution
From: Al Viro @ 2019-09-04 23:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Aleksa Sarai, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Jann Horn, Kees Cook,
Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus Villemoes
In-Reply-To: <CAHk-=wg7Wq1kj8kZ+SSpfU_o991woW60NWca9yBA2ccs2eNx8Q@mail.gmail.com>
On Wed, Sep 04, 2019 at 03:38:20PM -0700, Linus Torvalds wrote:
> On Wed, Sep 4, 2019 at 3:31 PM David Howells <dhowells@redhat.com> wrote:
> >
> > It ought to be reasonably easy to make them per-sb at least, I think. We
> > don't allow cross-super rename, right?
>
> Right now the sequence count handling very much depends on it being a
> global entity on the reader side, at least.
>
> And while the rename sequence count could (and probably should) be
> per-sb, the same is very much not true of the mount one.
Huh? That will cost us having to have a per-superblock dentry
hash table; recall that lockless lockup can give false negatives
if something gets moved from chain to chain, and rename_lock is
first and foremost used to catch those and retry. If we split
it on per-superblock basis, we can't have dentries from different
superblocks in the same chain anymore...
^ permalink raw reply
* Re: [PATCH v12 10/12] namei: aggressively check for nd->root escape on ".." resolution
From: Linus Torvalds @ 2019-09-04 22:38 UTC (permalink / raw)
To: David Howells
Cc: linux-ia64, Linux-sh list, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, Linux List Kernel Mailing, J. Bruce Fields,
open list:KERNEL SELFTEST FRAMEWORK, sparclinux, Shuah Khan,
linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai, Jiri Olsa,
Alexander Shishkin, Ingo Molnar, Linux ARM, linux-mips,
linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn
In-Reply-To: <20592.1567636276@warthog.procyon.org.uk>
On Wed, Sep 4, 2019 at 3:31 PM David Howells <dhowells@redhat.com> wrote:
>
> It ought to be reasonably easy to make them per-sb at least, I think. We
> don't allow cross-super rename, right?
Right now the sequence count handling very much depends on it being a
global entity on the reader side, at least.
And while the rename sequence count could (and probably should) be
per-sb, the same is very much not true of the mount one.
So the rename seqcount is likely easier to fix than the mount one, but
neither of them are entirely trivial, afaik.
Linus
^ permalink raw reply
* Re: [PATCH v12 10/12] namei: aggressively check for nd->root escape on ".." resolution
From: David Howells @ 2019-09-04 22:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Aleksa Sarai, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner, Jann Horn, Kees Cook,
Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Tycho Andersen, David Drysdale, Chanho Min,
Oleg Nesterov, Rasmus
In-Reply-To: <CAHk-=wgcJq21Hydh7Tx5-o8empoPp7ULDBw0Am-du_Pa+fcftQ@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > Hinting to userspace to do a retry (with -EAGAIN as you mention in your
> > other mail) wouldn't be a bad thing at all, though you'd almost
> > certainly get quite a few spurious -EAGAINs -- &{mount,rename}_lock are
> > global for the entire machine, after all.
>
> I'd hope that we have some future (possibly very long-term)
> alternative that is not quite system-global, but yes, right now they
> are.
It ought to be reasonably easy to make them per-sb at least, I think. We
don't allow cross-super rename, right?
David
^ permalink raw reply
* Re: [PATCH 00/11] Keyrings, Block and USB notifications [ver #8]
From: Linus Torvalds @ 2019-09-04 22:28 UTC (permalink / raw)
To: David Howells
Cc: keyrings, linux-usb, linux-block, Casey Schaufler,
Stephen Smalley, Greg Kroah-Hartman, nicolas.dichtel, raven,
Christian Brauner, LSM List, linux-fsdevel, Linux API,
Linux List Kernel Mailing
In-Reply-To: <156763534546.18676.3530557439501101639.stgit@warthog.procyon.org.uk>
On Wed, Sep 4, 2019 at 3:15 PM David Howells <dhowells@redhat.com> wrote:
>
>
> Here's a set of patches to add a general notification queue concept and to
> add event sources such as:
Why?
I'm just going to be very blunt about this, and say that there is no
way I can merge any of this *ever*, unless other people stand up and
say that
(a) they'll use it
and
(b) they'll actively develop it and participate in testing and coding
Because I'm simply not willing to have the same situation that
happened with the keyring ACL stuff this merge window happen with some
other random feature some day in the future.
That change never had anybody else that showed any interest in it, it
was never really clear why it was made, and it broke booting for me.
That had better never happen again, and I'm tired of seeing
unexplained random changes to key handling that have one single author
and nobody else involved.
And there is this whole long cover letter to explain what the code
does, what you can do with it, and what the changes have been in
revisions, but AT NO POINT does it explain what the point of the
feature is at all.
Why would we want this, and what is the advantage over udev etc that
already has event handling for things like block events and USB
events?
What's the advantage of another random character device, and what's
the use? Who is asking for this, and who would use it? Why are keys
special, and why should you be able to track events on keys in the
first place? Who is co-developing and testing this, and what's the
point?
Fundamentally, I'm not even interested in seeing "Reviewed-by". New
features need actual users and explanations for what they are, over
and beyond the developer itself.
IOW, you need to have an outside person step in and say "yes, I need
this". No more of these "David makes random changes without any
external input" series.
Linus
^ 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