* [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
sched_getattr(2) doesn't care about trailing non-zero bytes in the
(ksize > usize) case, so just use copy_struct_to_user() without checking
ignored_trailing.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/sched/syscalls.c | 42 ++----------------------------------------
1 file changed, 2 insertions(+), 40 deletions(-)
diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index ae1b42775ef9..4ccc058bae16 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -1147,45 +1147,6 @@ SYSCALL_DEFINE2(sched_getparam, pid_t, pid, struct sched_param __user *, param)
return copy_to_user(param, &lp, sizeof(*param)) ? -EFAULT : 0;
}
-/*
- * Copy the kernel size attribute structure (which might be larger
- * than what user-space knows about) to user-space.
- *
- * Note that all cases are valid: user-space buffer can be larger or
- * smaller than the kernel-space buffer. The usual case is that both
- * have the same size.
- */
-static int
-sched_attr_copy_to_user(struct sched_attr __user *uattr,
- struct sched_attr *kattr,
- unsigned int usize)
-{
- unsigned int ksize = sizeof(*kattr);
-
- if (!access_ok(uattr, usize))
- return -EFAULT;
-
- /*
- * sched_getattr() ABI forwards and backwards compatibility:
- *
- * If usize == ksize then we just copy everything to user-space and all is good.
- *
- * If usize < ksize then we only copy as much as user-space has space for,
- * this keeps ABI compatibility as well. We skip the rest.
- *
- * If usize > ksize then user-space is using a newer version of the ABI,
- * which part the kernel doesn't know about. Just ignore it - tooling can
- * detect the kernel's knowledge of attributes from the attr->size value
- * which is set to ksize in this case.
- */
- kattr->size = min(usize, ksize);
-
- if (copy_to_user(uattr, kattr, kattr->size))
- return -EFAULT;
-
- return 0;
-}
-
/**
* sys_sched_getattr - similar to sched_getparam, but with sched_attr
* @pid: the pid in question.
@@ -1230,7 +1191,8 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
#endif
}
- return sched_attr_copy_to_user(uattr, &kattr, usize);
+ kattr.size = min(usize, sizeof(kattr));
+ return copy_struct_to_user(uattr, usize, &kattr, sizeof(kattr), NULL);
}
#ifdef CONFIG_SMP
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 01/10] uaccess: add copy_struct_to_user helper
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
This is based on copy_struct_from_user(), but there is one additional
case to consider when creating a syscall that returns an
extensible-struct to userspace -- how should data in the struct that
cannot fit into the userspace struct be handled (ksize > usize)?
There are three possibilies:
1. The interface is like sched_getattr(2), where new information will
be silently not provided to userspace. This is probably what most
interfaces will want to do, as it provides the most possible
backwards-compatibility.
2. The interface is like lsm_list_modules(2), where you want to return
an error like -EMSGSIZE if not providing information could result in
the userspace program making a serious mistake (such as one that
could lead to a security problem) or if you want to provide some
flag to userspace so they know that they are missing some
information.
3. The interface is like statx(2), where there some kind of a request
mask that indicates what data userspace would like. One could
imagine that statx2(2) (using extensible structs) would want to
return -EMSGSIZE if the user explicitly requested a field that their
structure is too small to fit, but not return an error if the field
was not explicitly requested. This is kind of a mix between (1) and
(2) based on the requested mask.
The copy_struct_to_user() helper includes a an extra argument that is
used to return a boolean flag indicating whether there was a non-zero
byte in the trailing bytes that were not copied to userspace. This can
be used in the following ways to handle all three cases, respectively:
1. Just pass NULL, as you don't care about this case.
2. Return an error (say -EMSGSIZE) if the argument was set to true by
copy_struct_to_user().
3. If the argument was set to true by copy_struct_to_user(), check if
there is a flag that implies a field larger than usize.
This is the only case where callers of copy_struct_to_user() should
check usize themselves. This will probably require scanning an array
that specifies what flags were added for each version of the flags
struct and returning an error if the request mask matches any of the
flags that were added in versions of the struct that are larger than
usize.
At the moment we don't have any users of (3), so this patch doesn't
include any helpers to make the necessary scanning easier, but it should
be fairly easy to add some if necessary.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
include/linux/uaccess.h | 97 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d8e4105a2f21..cf0994605c10 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -387,6 +387,103 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,
return 0;
}
+/**
+ * copy_struct_to_user: copy a struct to userspace
+ * @dst: Destination address, in userspace. This buffer must be @ksize
+ * bytes long.
+ * @usize: (Alleged) size of @dst struct.
+ * @src: Source address, in kernel space.
+ * @ksize: Size of @src struct.
+ * @ignored_trailing: Set to %true if there was a non-zero byte in @src that
+ * userspace cannot see because they are using an smaller struct.
+ *
+ * Copies a struct from kernel space to userspace, 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).
+ *
+ * Some syscalls may wish to make sure that userspace knows about everything in
+ * the struct, and if there is a non-zero value that userspce doesn't know
+ * about, they want to return an error (such as -EMSGSIZE) or have some other
+ * fallback (such as adding a "you're missing some information" flag). If
+ * @ignored_trailing is non-%NULL, it will be set to %true if there was a
+ * non-zero byte that could not be copied to userspace (ie. was past @usize).
+ *
+ * While unconditionally returning an error in this case is the simplest
+ * solution, for maximum backward compatibility you should try to only return
+ * -EMSGSIZE if the user explicitly requested the data that couldn't be copied.
+ * Note that structure sizes can change due to header changes and simple
+ * recompilations without code changes(!), so if you care about
+ * @ignored_trailing you probably want to make sure that any new field data is
+ * associated with a flag. Otherwise you might assume that a program knows
+ * about data it does not.
+ *
+ * @ksize is just sizeof(*src), and @usize should've been passed by userspace.
+ * The recommended usage is something like the following:
+ *
+ * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
+ * {
+ * int err;
+ * bool ignored_trailing;
+ * struct foo karg = {};
+ *
+ * if (usize > PAGE_SIZE)
+ * return -E2BIG;
+ * if (usize < FOO_SIZE_VER0)
+ * return -EINVAL;
+ *
+ * // ... modify karg somehow ...
+ *
+ * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg),
+ * &ignored_trailing);
+ * if (err)
+ * return err;
+ * if (ignored_trailing)
+ * return -EMSGSIZE:
+ *
+ * // ...
+ * }
+ *
+ * There are three cases to consider:
+ * * If @usize == @ksize, then it's copied verbatim.
+ * * If @usize < @ksize, then the kernel is trying to pass userspace a newer
+ * struct than it supports. Thus we only copy the interoperable portions
+ * (@usize) and ignore the rest (but @ignored_trailing is set to %true if
+ * any of the trailing (@ksize - @usize) bytes are non-zero).
+ * * If @usize > @ksize, then the kernel is trying to pass userspace an older
+ * struct than userspace supports. In order to make sure the
+ * unknown-to-the-kernel fields don't contain garbage values, we zero the
+ * trailing (@usize - @ksize) bytes.
+ *
+ * Returns (in all cases, some data may have been copied):
+ * * -EFAULT: access to userspace failed.
+ */
+static __always_inline __must_check int
+copy_struct_to_user(void __user *dst, size_t usize, const void *src,
+ size_t ksize, bool *ignored_trailing)
+{
+ size_t size = min(ksize, usize);
+ size_t rest = max(ksize, usize) - size;
+
+ /* Double check if ksize is larger than a known object size. */
+ if (WARN_ON_ONCE(ksize > __builtin_object_size(src, 1)))
+ return -E2BIG;
+
+ /* Deal with trailing bytes. */
+ if (usize > ksize) {
+ if (clear_user(dst + size, rest))
+ return -EFAULT;
+ }
+ if (ignored_trailing)
+ *ignored_trailing = ksize < usize &&
+ memchr_inv(src + size, 0, rest) != NULL;
+ /* Copy the interoperable parts of the struct. */
+ if (copy_to_user(dst, src, size))
+ return -EFAULT;
+ return 0;
+}
+
bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size);
long copy_from_kernel_nofault(void *dst, const void *src, size_t size);
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 00/10] extensible syscalls: CHECK_FIELDS to allow for easier feature detection
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai, stable
This is something that I've been thinking about for a while. We had a
discussion at LPC 2020 about this[1] but the proposals suggested there
never materialised.
In short, it is quite difficult for userspace to detect the feature
capability of syscalls at runtime. This is something a lot of programs
want to do, but they are forced to create elaborate scenarios to try to
figure out if a feature is supported without causing damage to the
system. For the vast majority of cases, each individual feature also
needs to be tested individually (because syscall results are
all-or-nothing), so testing even a single syscall's feature set can
easily inflate the startup time of programs.
This patchset implements the fairly minimal design I proposed in this
talk[2] and in some old LKML threads (though I can't find the exact
references ATM). The general flow looks like:
1. Userspace will indicate to the kernel that a syscall should a be
no-op by setting the top bit of the extensible struct size argument.
We will almost certainly never support exabyte sized structs, so the
top bits are free for us to use as makeshift flag bits. This is
preferable to using the per-syscall flag field inside the structure
because seccomp can easily detect the bit in the flag and allow the
probe or forcefully return -EEXTSYS_NOOP.
2. The kernel will then fill the provided structure with every valid
bit pattern that the current kernel understands.
For flags or other bitflag-like fields, this is the set of valid
flags or bits. For pointer fields or fields that take an arbitrary
value, the field has every bit set (0xFF... to fill the field) to
indicate that any value is valid in the field.
3. The syscall then returns -EEXTSYS_NOOP which is an errno that will
only ever be used for this purpose (so userspace can be sure that
the request succeeded).
On older kernels, the syscall will return a different error (usually
-E2BIG or -EFAULT) and userspace can do their old-fashioned checks.
4. Userspace can then check which flags and fields are supported by
looking at the fields in the returned structure. Flags are checked
by doing an AND with the flags field, and field support can checked
by comparing to 0. In principle you could just AND the entire
structure if you wanted to do this check generically without caring
about the structure contents (this is what libraries might consider
doing).
Userspace can even find out the internal kernel structure size by
passing a PAGE_SIZE buffer and seeing how many bytes are non-zero.
As with copy_struct_from_user(), this is designed to be forward- and
backwards- compatible.
This allows programas to get a one-shot understanding of what features a
syscall supports without having to do any elaborate setups or tricks to
detect support for destructive features. Flags can simply be ANDed to
check if they are in the supported set, and fields can just be checked
to see if they are non-zero.
This patchset is IMHO the simplest way we can add the ability to
introspect the feature set of extensible struct (copy_struct_from_user)
syscalls. It doesn't preclude the chance of a more generic mechanism
being added later.
The intended way of using this interface to get feature information
looks something like the following (imagine that openat2 has gained a
new field and a new flag in the future):
static bool openat2_no_automount_supported;
static bool openat2_cwd_fd_supported;
int check_openat2_support(void)
{
int err;
struct open_how how = {};
err = openat2(AT_FDCWD, ".", &how, CHECK_FIELDS | sizeof(how));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
openat2_no_automount_supported = (how.flags & RESOLVE_NO_AUTOMOUNT);
openat2_cwd_fd_supported = (how.cwd_fd != 0);
break;
}
}
This series adds CHECK_FIELDS support for the following extensible
struct syscalls, as they are quite likely to grow flags in the near
future:
* openat2
* clone3
* mount_setattr
[1]: https://lwn.net/Articles/830666/
[2]: https://youtu.be/ggD-eb3yPVs
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changes in v3:
- Fix copy_struct_to_user() return values in case of clear_user() failure.
- v2: <https://lore.kernel.org/r/20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
Changes in v2:
- Add CHECK_FIELDS support to mount_setattr(2).
- Fix build failure on architectures with custom errno values.
- Rework selftests to use the tools/ uAPI headers rather than custom
defining EEXTSYS_NOOP.
- Make sure we return -EINVAL and -E2BIG for invalid sizes even if
CHECK_FIELDS is set, and add some tests for that.
- v1: <https://lore.kernel.org/r/20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
---
Aleksa Sarai (10):
uaccess: add copy_struct_to_user helper
sched_getattr: port to copy_struct_to_user
openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
openat2: add CHECK_FIELDS flag to usize argument
selftests: openat2: add 0xFF poisoned data after misaligned struct
selftests: openat2: add CHECK_FIELDS selftests
clone3: add CHECK_FIELDS flag to usize argument
selftests: clone3: add CHECK_FIELDS selftests
mount_setattr: add CHECK_FIELDS flag to usize argument
selftests: mount_setattr: add CHECK_FIELDS selftest
arch/alpha/include/uapi/asm/errno.h | 3 +
arch/mips/include/uapi/asm/errno.h | 3 +
arch/parisc/include/uapi/asm/errno.h | 3 +
arch/sparc/include/uapi/asm/errno.h | 3 +
fs/namespace.c | 17 ++
fs/open.c | 18 ++
include/linux/uaccess.h | 97 ++++++++
include/uapi/asm-generic/errno.h | 3 +
include/uapi/linux/openat2.h | 2 +
kernel/fork.c | 30 ++-
kernel/sched/syscalls.c | 42 +---
tools/arch/alpha/include/uapi/asm/errno.h | 3 +
tools/arch/mips/include/uapi/asm/errno.h | 3 +
tools/arch/parisc/include/uapi/asm/errno.h | 3 +
tools/arch/sparc/include/uapi/asm/errno.h | 3 +
tools/include/uapi/asm-generic/errno.h | 3 +
tools/include/uapi/asm-generic/posix_types.h | 101 ++++++++
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../testing/selftests/clone3/clone3_check_fields.c | 264 +++++++++++++++++++++
tools/testing/selftests/mount_setattr/Makefile | 2 +-
.../selftests/mount_setattr/mount_setattr_test.c | 53 ++++-
tools/testing/selftests/openat2/Makefile | 2 +
tools/testing/selftests/openat2/openat2_test.c | 165 ++++++++++++-
24 files changed, 777 insertions(+), 51 deletions(-)
---
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
change-id: 20240803-extensible-structs-check_fields-a47e94cef691
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply
* Re: [viro-vfs:work.xattr2] [fs/xattr] 64d47e878a: xfstests.xfs.046.fail
From: Oliver Sang @ 2024-10-09 14:31 UTC (permalink / raw)
To: Al Viro
Cc: Christian Göttsche, oe-lkp, lkp, linux-fsdevel,
Arnd Bergmann, Christian Brauner, linux-alpha, linux-arm-kernel,
linux-m68k, linux-kernel, linux-mips, linux-parisc, linuxppc-dev,
linux-s390, linux-sh, sparclinux, audit, linux-api, linux-arch,
oliver.sang
In-Reply-To: <20241006145904.GE4017910@ZenIV>
hi, Al Viro,
On Sun, Oct 06, 2024 at 03:59:04PM +0100, Al Viro wrote:
> On Sun, Oct 06, 2024 at 10:20:57PM +0800, kernel test robot wrote:
>
> > xfs/046 - output mismatch (see /lkp/benchmarks/xfstests/results//xfs/046.out.bad)
> > --- tests/xfs/046.out 2024-09-30 21:13:44.000000000 +0000
> > +++ /lkp/benchmarks/xfstests/results//xfs/046.out.bad 2024-10-06 05:31:50.379495110 +0000
> > @@ -34,4 +34,8 @@
> > xfsrestore: restore complete: SECS seconds elapsed
> > xfsrestore: Restore Status: SUCCESS
> > Comparing listing of dump directory with restore directory
> > +ls: /fs/scratch/dumpdir/sub/a-link: No such file or directory
> > +ls: /fs/scratch/dumpdir/sub/b-link: No such file or directory
> > +ls: /fs/scratch/restoredir/dumpdir/sub/a-link: No such file or directory
> > +ls: /fs/scratch/restoredir/dumpdir/sub/b-link: No such file or directory
> > ...
> > (Run 'diff -u /lkp/benchmarks/xfstests/tests/xfs/046.out /lkp/benchmarks/xfstests/results//xfs/046.out.bad' to see the entire diff)
> > Ran: xfs/046
> > Failures: xfs/046
> > Failed 1 of 1 tests
>
> *stares*
>
> D'oh... Inverted sense for AT_SYMLINK_NOFOLLOW => LOOKUP_FLAGS...
>
> Try this:
we confirm below patch can fix the issue
Tested-by: kernel test robot <oliver.sang@intel.com>
=========================================================================================
compiler/disk/fs/kconfig/rootfs/tbox_group/test/testcase:
gcc-12/4HDD/xfs/x86_64-rhel-8.3-func/debian-12-x86_64-20240206.cgz/lkp-skl-d06/xfs-046/xfstests
commit:
4b7d06e4b7a1c ("new helpers: file_removexattr(), filename_removexattr()")
64d47e878a819 ("fs/xattr: add *at family syscalls")
6f4ccbcfda377 <--- your patch
4b7d06e4b7a1c647 64d47e878a8196f374879bfdd0e 6f4ccbcfda377af1167d1b7fd48
---------------- --------------------------- ---------------------------
fail:runs %reproduction fail:runs %reproduction fail:runs
| | | | |
:6 100% 6:6 0% :10 xfstests.xfs.046.fail
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 0b506b6565b7..b96cca3f4bf8 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -721,7 +721,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
> if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> return -EINVAL;
>
> - if (at_flags & AT_SYMLINK_NOFOLLOW)
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> lookup_flags = LOOKUP_FOLLOW;
>
> error = setxattr_copy(name, &ctx);
> @@ -880,7 +880,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
> return file_getxattr(fd_file(f), &ctx);
> } else {
> int lookup_flags = 0;
> - if (at_flags & AT_SYMLINK_NOFOLLOW)
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> lookup_flags = LOOKUP_FOLLOW;
> return filename_getxattr(dfd, filename, lookup_flags, &ctx);
> }
^ permalink raw reply
* Re: [viro-vfs:work.xattr2] [fs/xattr] 64d47e878a: xfstests.xfs.046.fail
From: Al Viro @ 2024-10-06 14:59 UTC (permalink / raw)
To: kernel test robot
Cc: Christian Göttsche, oe-lkp, lkp, linux-fsdevel,
Arnd Bergmann, Christian Brauner, linux-alpha, linux-arm-kernel,
linux-m68k, linux-kernel, linux-mips, linux-parisc, linuxppc-dev,
linux-s390, linux-sh, sparclinux, audit, linux-api, linux-arch
In-Reply-To: <202410062250.ee92fca7-oliver.sang@intel.com>
On Sun, Oct 06, 2024 at 10:20:57PM +0800, kernel test robot wrote:
> xfs/046 - output mismatch (see /lkp/benchmarks/xfstests/results//xfs/046.out.bad)
> --- tests/xfs/046.out 2024-09-30 21:13:44.000000000 +0000
> +++ /lkp/benchmarks/xfstests/results//xfs/046.out.bad 2024-10-06 05:31:50.379495110 +0000
> @@ -34,4 +34,8 @@
> xfsrestore: restore complete: SECS seconds elapsed
> xfsrestore: Restore Status: SUCCESS
> Comparing listing of dump directory with restore directory
> +ls: /fs/scratch/dumpdir/sub/a-link: No such file or directory
> +ls: /fs/scratch/dumpdir/sub/b-link: No such file or directory
> +ls: /fs/scratch/restoredir/dumpdir/sub/a-link: No such file or directory
> +ls: /fs/scratch/restoredir/dumpdir/sub/b-link: No such file or directory
> ...
> (Run 'diff -u /lkp/benchmarks/xfstests/tests/xfs/046.out /lkp/benchmarks/xfstests/results//xfs/046.out.bad' to see the entire diff)
> Ran: xfs/046
> Failures: xfs/046
> Failed 1 of 1 tests
*stares*
D'oh... Inverted sense for AT_SYMLINK_NOFOLLOW => LOOKUP_FLAGS...
Try this:
diff --git a/fs/xattr.c b/fs/xattr.c
index 0b506b6565b7..b96cca3f4bf8 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -721,7 +721,7 @@ static int path_setxattrat(int dfd, const char __user *pathname,
if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
return -EINVAL;
- if (at_flags & AT_SYMLINK_NOFOLLOW)
+ if (!(at_flags & AT_SYMLINK_NOFOLLOW))
lookup_flags = LOOKUP_FOLLOW;
error = setxattr_copy(name, &ctx);
@@ -880,7 +880,7 @@ static ssize_t path_getxattrat(int dfd, const char __user *pathname,
return file_getxattr(fd_file(f), &ctx);
} else {
int lookup_flags = 0;
- if (at_flags & AT_SYMLINK_NOFOLLOW)
+ if (!(at_flags & AT_SYMLINK_NOFOLLOW))
lookup_flags = LOOKUP_FOLLOW;
return filename_getxattr(dfd, filename, lookup_flags, &ctx);
}
^ permalink raw reply related
* [viro-vfs:work.xattr2] [fs/xattr] 64d47e878a: xfstests.xfs.046.fail
From: kernel test robot @ 2024-10-06 14:20 UTC (permalink / raw)
To: Christian Göttsche
Cc: oe-lkp, lkp, linux-fsdevel, Al Viro, Arnd Bergmann,
Christian Brauner, linux-alpha, linux-arm-kernel, linux-m68k,
linux-kernel, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, audit, linux-api, linux-arch, oliver.sang
Hello,
kernel test robot noticed "xfstests.xfs.046.fail" on:
commit: 64d47e878a8196f374879bfdd0ea754ba77682d1 ("fs/xattr: add *at family syscalls")
https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git work.xattr2
in testcase: xfstests
version: xfstests-x86_64-f71228e3-1_20240930
with following parameters:
disk: 4HDD
fs: xfs
test: xfs-046
compiler: gcc-12
test machine: 4 threads Intel(R) Xeon(R) CPU E3-1225 v5 @ 3.30GHz (Skylake) with 16G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410062250.ee92fca7-oliver.sang@intel.com
2024-10-06 05:31:34 export TEST_DIR=/fs/sda1
2024-10-06 05:31:34 export TEST_DEV=/dev/sda1
2024-10-06 05:31:34 export FSTYP=xfs
2024-10-06 05:31:34 export SCRATCH_MNT=/fs/scratch
2024-10-06 05:31:34 mkdir /fs/scratch -p
2024-10-06 05:31:34 export SCRATCH_DEV=/dev/sda4
2024-10-06 05:31:34 export SCRATCH_LOGDEV=/dev/sda2
2024-10-06 05:31:34 export SCRATCH_XFS_LIST_METADATA_FIELDS=u3.sfdir3.hdr.parent.i4
2024-10-06 05:31:34 export SCRATCH_XFS_LIST_FUZZ_VERBS=random
2024-10-06 05:31:34 echo xfs/046
2024-10-06 05:31:34 ./check xfs/046
FSTYP -- xfs (debug)
PLATFORM -- Linux/x86_64 lkp-skl-d06 6.12.0-rc1-00009-g64d47e878a81 #1 SMP PREEMPT_DYNAMIC Sun Oct 6 12:37:17 CST 2024
MKFS_OPTIONS -- -f /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /fs/scratch
xfs/046 - output mismatch (see /lkp/benchmarks/xfstests/results//xfs/046.out.bad)
--- tests/xfs/046.out 2024-09-30 21:13:44.000000000 +0000
+++ /lkp/benchmarks/xfstests/results//xfs/046.out.bad 2024-10-06 05:31:50.379495110 +0000
@@ -34,4 +34,8 @@
xfsrestore: restore complete: SECS seconds elapsed
xfsrestore: Restore Status: SUCCESS
Comparing listing of dump directory with restore directory
+ls: /fs/scratch/dumpdir/sub/a-link: No such file or directory
+ls: /fs/scratch/dumpdir/sub/b-link: No such file or directory
+ls: /fs/scratch/restoredir/dumpdir/sub/a-link: No such file or directory
+ls: /fs/scratch/restoredir/dumpdir/sub/b-link: No such file or directory
...
(Run 'diff -u /lkp/benchmarks/xfstests/tests/xfs/046.out /lkp/benchmarks/xfstests/results//xfs/046.out.bad' to see the entire diff)
Ran: xfs/046
Failures: xfs/046
Failed 1 of 1 tests
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241006/202410062250.ee92fca7-oliver.sang@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [PATCH RFT v11 8/8] selftests/clone3: Test shadow stack support
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
Add basic test coverage for specifying the shadow stack for a newly
created thread via clone3(), including coverage of the newly extended
argument structure. We check that a user specified shadow stack can be
provided, and that invalid combinations of parameters are rejected.
In order to facilitate testing on systems without userspace shadow stack
support we manually enable shadow stacks on startup, this is architecture
specific due to the use of an arch_prctl() on x86. Due to interactions with
potential userspace locking of features we actually detect support for
shadow stacks on the running system by attempting to allocate a shadow
stack page during initialisation using map_shadow_stack(), warning if this
succeeds when the enable failed.
In order to allow testing of user configured shadow stacks on
architectures with that feature we need to ensure that we do not return
from the function where the clone3() syscall is called in the child
process, doing so would trigger a shadow stack underflow. To do this we
use inline assembly rather than the standard syscall wrapper to call
clone3(). In order to avoid surprises we also use a syscall rather than
the libc exit() function., this should be overly cautious.
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 143 +++++++++++++++++++++-
tools/testing/selftests/clone3/clone3_selftests.h | 63 ++++++++++
2 files changed, 205 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index 5b8b7d640e70132242fc6939450669acd0c534f9..b0378d7418cc8b00caebc6f92f58280bc04b0f80 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -3,6 +3,7 @@
/* Based on Christian Brauner's clone3() example */
#define _GNU_SOURCE
+#include <asm/mman.h>
#include <errno.h>
#include <inttypes.h>
#include <linux/types.h>
@@ -11,6 +12,7 @@
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
+#include <sys/mman.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/un.h>
@@ -19,8 +21,12 @@
#include <sched.h>
#include "../kselftest.h"
+#include "../ksft_shstk.h"
#include "clone3_selftests.h"
+static bool shadow_stack_supported;
+static size_t max_supported_args_size;
+
enum test_mode {
CLONE3_ARGS_NO_TEST,
CLONE3_ARGS_ALL_0,
@@ -28,6 +34,10 @@ enum test_mode {
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NEG,
CLONE3_ARGS_INVAL_EXIT_SIGNAL_CSIG,
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG,
+ CLONE3_ARGS_SHADOW_STACK,
+ CLONE3_ARGS_SHADOW_STACK_MISALIGNED,
+ CLONE3_ARGS_SHADOW_STACK_NO_TOKEN,
+ CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY,
};
typedef bool (*filter_function)(void);
@@ -44,6 +54,44 @@ struct test {
filter_function filter;
};
+
+/*
+ * We check for shadow stack support by attempting to use
+ * map_shadow_stack() since features may have been locked by the
+ * dynamic linker resulting in spurious errors when we attempt to
+ * enable on startup. We warn if the enable failed.
+ */
+static void test_shadow_stack_supported(void)
+{
+ long ret;
+
+ ret = syscall(__NR_map_shadow_stack, 0, getpagesize(), 0);
+ if (ret == -1) {
+ ksft_print_msg("map_shadow_stack() not supported\n");
+ } else if ((void *)ret == MAP_FAILED) {
+ ksft_print_msg("Failed to map shadow stack\n");
+ } else {
+ ksft_print_msg("Shadow stack supportd\n");
+ shadow_stack_supported = true;
+
+ if (!shadow_stack_enabled)
+ ksft_print_msg("Mapped but did not enable shadow stack\n");
+ }
+}
+
+static void *get_shadow_stack_page(unsigned long flags)
+{
+ unsigned long long page;
+
+ page = syscall(__NR_map_shadow_stack, 0, getpagesize(), flags);
+ if ((void *)page == MAP_FAILED) {
+ ksft_print_msg("map_shadow_stack() failed: %d\n", errno);
+ return 0;
+ }
+
+ return (void *)page;
+}
+
static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
{
struct __clone_args args = {
@@ -57,6 +105,7 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
} args_ext;
pid_t pid = -1;
+ void *p;
int status;
memset(&args_ext, 0, sizeof(args_ext));
@@ -89,6 +138,26 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
case CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG:
args.exit_signal = 0x00000000000000f0ULL;
break;
+ case CLONE3_ARGS_SHADOW_STACK:
+ p = get_shadow_stack_page(SHADOW_STACK_SET_TOKEN);
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_pointer = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_MISALIGNED:
+ p = get_shadow_stack_page(SHADOW_STACK_SET_TOKEN);
+ p += getpagesize() - sizeof(void *) - 1;
+ args.shadow_stack_pointer = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY:
+ p = malloc(getpagesize());
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_pointer = (unsigned long long)p;
+ break;
+ case CLONE3_ARGS_SHADOW_STACK_NO_TOKEN:
+ p = get_shadow_stack_page(0);
+ p += getpagesize() - sizeof(void *);
+ args.shadow_stack_pointer = (unsigned long long)p;
+ break;
}
memcpy(&args_ext.args, &args, sizeof(struct __clone_args));
@@ -102,7 +171,12 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
if (pid == 0) {
ksft_print_msg("I am the child, my PID is %d\n", getpid());
- _exit(EXIT_SUCCESS);
+ /*
+ * Use a raw syscall to ensure we don't get issues
+ * with manually specified shadow stack and exit handlers.
+ */
+ syscall(__NR_exit, EXIT_SUCCESS);
+ ksft_print_msg("CHILD FAILED TO EXIT PID is %d\n", getpid());
}
ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
@@ -184,6 +258,26 @@ static bool no_timenamespace(void)
return true;
}
+static bool have_shadow_stack(void)
+{
+ if (shadow_stack_supported) {
+ ksft_print_msg("Shadow stack supported\n");
+ return true;
+ }
+
+ return false;
+}
+
+static bool no_shadow_stack(void)
+{
+ if (!shadow_stack_supported) {
+ ksft_print_msg("Shadow stack not supported\n");
+ return true;
+ }
+
+ return false;
+}
+
static size_t page_size_plus_8(void)
{
return getpagesize() + 8;
@@ -327,6 +421,50 @@ static const struct test tests[] = {
.expected = -EINVAL,
.test_mode = CLONE3_ARGS_NO_TEST,
},
+ {
+ .name = "Shadow stack on system with shadow stack",
+ .size = 0,
+ .expected = 0,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with misaligned address",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EINVAL,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_MISALIGNED,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with normal memory",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EFAULT,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_NORMAL_MEMORY,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack with no token",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EINVAL,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK_NO_TOKEN,
+ .filter = no_shadow_stack,
+ },
+ {
+ .name = "Shadow stack on system without shadow stack",
+ .flags = CLONE_VM,
+ .size = 0,
+ .expected = -EINVAL,
+ .e2big_valid = true,
+ .test_mode = CLONE3_ARGS_SHADOW_STACK,
+ .filter = have_shadow_stack,
+ },
};
int main(int argc, char *argv[])
@@ -334,9 +472,12 @@ int main(int argc, char *argv[])
size_t size;
int i;
+ enable_shadow_stack();
+
ksft_print_header();
ksft_set_plan(ARRAY_SIZE(tests));
test_clone3_supported();
+ test_shadow_stack_supported();
for (i = 0; i < ARRAY_SIZE(tests); i++)
test_clone3(&tests[i]);
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index 39b5dcba663c30b9fc2542d9a0d2686105ce5761..26ff1554408a59af26bd708dc9c852210e370828 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -31,12 +31,75 @@ struct __clone_args {
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
+#ifndef CLONE_ARGS_SIZE_VER2
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#endif
+ __aligned_u64 shadow_stack_pointer;
+#ifndef CLONE_ARGS_SIZE_VER3
+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
+#endif
};
+/*
+ * For architectures with shadow stack support we need to be
+ * absolutely sure that the clone3() syscall will be inline and not a
+ * function call so we open code.
+ */
+#ifdef __x86_64__
+static pid_t __always_inline sys_clone3(struct __clone_args *args, size_t size)
+{
+ long ret;
+ register long _num __asm__ ("rax") = __NR_clone3;
+ register long _args __asm__ ("rdi") = (long)(args);
+ register long _size __asm__ ("rsi") = (long)(size);
+
+ __asm__ volatile (
+ "syscall\n"
+ : "=a"(ret)
+ : "r"(_args), "r"(_size),
+ "0"(_num)
+ : "rcx", "r11", "memory", "cc"
+ );
+
+ if (ret < 0) {
+ errno = -ret;
+ return -1;
+ }
+
+ return ret;
+}
+#elif defined(__aarch64__)
+static pid_t __always_inline sys_clone3(struct __clone_args *args, size_t size)
+{
+ register long _num __asm__ ("x8") = __NR_clone3;
+ register long _args __asm__ ("x0") = (long)(args);
+ register long _size __asm__ ("x1") = (long)(size);
+ register long arg2 __asm__ ("x2") = 0;
+ register long arg3 __asm__ ("x3") = 0;
+ register long arg4 __asm__ ("x4") = 0;
+
+ __asm__ volatile (
+ "svc #0\n"
+ : "=r"(_args)
+ : "r"(_args), "r"(_size),
+ "r"(_num), "r"(arg2),
+ "r"(arg3), "r"(arg4)
+ : "memory", "cc"
+ );
+
+ if ((int)_args < 0) {
+ errno = -((int)_args);
+ return -1;
+ }
+
+ return _args;
+}
+#else
static pid_t sys_clone3(struct __clone_args *args, size_t size)
{
return syscall(__NR_clone3, args, size);
}
+#endif
static inline void test_clone3_supported(void)
{
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 7/8] selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
The clone_args structure is extensible, with the syscall passing in the
length of the structure. Inside the kernel we use copy_struct_from_user()
to read the struct but this has the unfortunate side effect of silently
accepting some overrun in the structure size providing the extra data is
all zeros. This means that we can't discover the clone3() features that
the running kernel supports by simply probing with various struct sizes.
We need to check this for the benefit of test systems which run newer
kselftests on old kernels.
Add a flag which can be set on a test to indicate that clone3() may return
-E2BIG due to the use of newer struct versions. Currently no tests need
this but it will become an issue for testing clone3() support for shadow
stacks, the support for shadow stacks is already present on x86.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index e066b201fa64eb17c55939b7cec18ac5d109613b..5b8b7d640e70132242fc6939450669acd0c534f9 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -39,6 +39,7 @@ struct test {
size_t size;
size_function size_function;
int expected;
+ bool e2big_valid;
enum test_mode test_mode;
filter_function filter;
};
@@ -146,6 +147,11 @@ static void test_clone3(const struct test *test)
ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n",
getpid(), ret, test->expected);
if (ret != test->expected) {
+ if (test->e2big_valid && ret == -E2BIG) {
+ ksft_print_msg("Test reported -E2BIG\n");
+ ksft_test_result_skip("%s\n", test->name);
+ return;
+ }
ksft_print_msg(
"[%d] Result (%d) is different than expected (%d)\n",
getpid(), ret, test->expected);
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 6/8] selftests/clone3: Factor more of main loop into test_clone3()
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
In order to make it easier to add more configuration for the tests and
more support for runtime detection of when tests can be run pass the
structure describing the tests into test_clone3() rather than picking
the arguments out of it and have that function do all the per-test work.
No functional change.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3.c | 77 ++++++++++++++++-----------------
1 file changed, 37 insertions(+), 40 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c
index e61f07973ce5e27aff30047b35e03b1b51875c15..e066b201fa64eb17c55939b7cec18ac5d109613b 100644
--- a/tools/testing/selftests/clone3/clone3.c
+++ b/tools/testing/selftests/clone3/clone3.c
@@ -30,6 +30,19 @@ enum test_mode {
CLONE3_ARGS_INVAL_EXIT_SIGNAL_NSIG,
};
+typedef bool (*filter_function)(void);
+typedef size_t (*size_function)(void);
+
+struct test {
+ const char *name;
+ uint64_t flags;
+ size_t size;
+ size_function size_function;
+ int expected;
+ enum test_mode test_mode;
+ filter_function filter;
+};
+
static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
{
struct __clone_args args = {
@@ -109,30 +122,40 @@ static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode)
return 0;
}
-static bool test_clone3(uint64_t flags, size_t size, int expected,
- enum test_mode test_mode)
+static void test_clone3(const struct test *test)
{
+ size_t size;
int ret;
+ if (test->filter && test->filter()) {
+ ksft_test_result_skip("%s\n", test->name);
+ return;
+ }
+
+ if (test->size_function)
+ size = test->size_function();
+ else
+ size = test->size;
+
+ ksft_print_msg("Running test '%s'\n", test->name);
+
ksft_print_msg(
"[%d] Trying clone3() with flags %#" PRIx64 " (size %zu)\n",
- getpid(), flags, size);
- ret = call_clone3(flags, size, test_mode);
+ getpid(), test->flags, size);
+ ret = call_clone3(test->flags, size, test->test_mode);
ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n",
- getpid(), ret, expected);
- if (ret != expected) {
+ getpid(), ret, test->expected);
+ if (ret != test->expected) {
ksft_print_msg(
"[%d] Result (%d) is different than expected (%d)\n",
- getpid(), ret, expected);
- return false;
+ getpid(), ret, test->expected);
+ ksft_test_result_fail("%s\n", test->name);
+ return;
}
- return true;
+ ksft_test_result_pass("%s\n", test->name);
}
-typedef bool (*filter_function)(void);
-typedef size_t (*size_function)(void);
-
static bool not_root(void)
{
if (getuid() != 0) {
@@ -160,16 +183,6 @@ static size_t page_size_plus_8(void)
return getpagesize() + 8;
}
-struct test {
- const char *name;
- uint64_t flags;
- size_t size;
- size_function size_function;
- int expected;
- enum test_mode test_mode;
- filter_function filter;
-};
-
static const struct test tests[] = {
{
.name = "simple clone3()",
@@ -319,24 +332,8 @@ int main(int argc, char *argv[])
ksft_set_plan(ARRAY_SIZE(tests));
test_clone3_supported();
- for (i = 0; i < ARRAY_SIZE(tests); i++) {
- if (tests[i].filter && tests[i].filter()) {
- ksft_test_result_skip("%s\n", tests[i].name);
- continue;
- }
-
- if (tests[i].size_function)
- size = tests[i].size_function();
- else
- size = tests[i].size;
-
- ksft_print_msg("Running test '%s'\n", tests[i].name);
-
- ksft_test_result(test_clone3(tests[i].flags, size,
- tests[i].expected,
- tests[i].test_mode),
- "%s\n", tests[i].name);
- }
+ for (i = 0; i < ARRAY_SIZE(tests); i++)
+ test_clone3(&tests[i]);
ksft_finished();
}
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 5/8] selftests/clone3: Remove redundant flushes of output streams
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
Since there were widespread issues with output not being flushed the
kselftest framework was modified to explicitly set the output streams
unbuffered in commit 58e2847ad2e6 ("selftests: line buffer test
program's stdout") so there is no need to explicitly flush in the clone3
tests.
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/clone3/clone3_selftests.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index 3d2663fe50ba56f011629e4f2eb68a72bcceb087..39b5dcba663c30b9fc2542d9a0d2686105ce5761 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -35,8 +35,6 @@ struct __clone_args {
static pid_t sys_clone3(struct __clone_args *args, size_t size)
{
- fflush(stdout);
- fflush(stderr);
return syscall(__NR_clone3, args, size);
}
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
Unlike with the normal stack there is no API for configuring the the shadow
stack for a new thread, instead the kernel will dynamically allocate a new
shadow stack with the same size as the normal stack. This appears to be due
to the shadow stack series having been in development since before the more
extensible clone3() was added rather than anything more deliberate.
Add a paramter to clone3() specifying the shadow stack pointer to use
for the new thread, this is inconsistent with the way we specify the
normal stack but during review concerns were expressed about having to
identify where the shadow stack pointer should be placed especially in
cases where the shadow stack has been previously active. If no shadow
stack is specified then the existing implicit allocation behaviour is
maintained.
If a shadow stack pointer is specified then it is required to have an
architecture defined token placed on the stack, this will be consumed by
the new task. If no valid token is present then this will be reported
with -EINVAL. This token prevents new threads being created pointing at
the shadow stack of an existing running thread.
If the architecture does not support shadow stacks the shadow stack
pointer must be not be specified, architectures that do support the
feature are expected to enforce the same requirement on individual
systems that lack shadow stack support.
Update the existing arm64 and x86 implementations to pay attention to
the newly added arguments, in order to maintain compatibility we use the
existing behaviour if no shadow stack is specified. Since we are now
using more fields from the kernel_clone_args we pass that into the
shadow stack code rather than individual fields.
Portions of the x86 architecture code were written by Rick Edgecombe.
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/mm/gcs.c | 54 +++++++++++++++++++++-
arch/x86/include/asm/shstk.h | 11 +++--
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 57 +++++++++++++++++++++---
include/asm-generic/cacheflush.h | 11 +++++
include/linux/sched/task.h | 17 +++++++
include/uapi/linux/sched.h | 10 +++--
kernel/fork.c | 96 +++++++++++++++++++++++++++++++++++-----
8 files changed, 232 insertions(+), 26 deletions(-)
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 1f633a482558b59aac5427963d42b37fce08c8a6..c4e93b7ce05c5dfa1128923ad587f9b5a7fb0051 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -43,8 +43,24 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
{
unsigned long addr, size;
- if (!system_supports_gcs())
+ if (!system_supports_gcs()) {
+ if (args->shadow_stack_pointer)
+ return -EINVAL;
+
+ return 0;
+ }
+
+ /*
+ * If the user specified a GCS then use it, otherwise fall
+ * back to a default allocation strategy. Validation is done
+ * in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_pointer) {
+ tsk->thread.gcs_base = 0;
+ tsk->thread.gcs_size = 0;
+ tsk->thread.gcspr_el0 = args->shadow_stack_pointer;
return 0;
+ }
if (!task_gcs_el0_enabled(tsk))
return 0;
@@ -68,6 +84,42 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
return 0;
}
+static bool gcs_consume_token(struct vm_area_struct *vma, struct page *page,
+ unsigned long user_addr)
+{
+ u64 expected = GCS_CAP(user_addr);
+ u64 *token = page_address(page) + offset_in_page(user_addr);
+
+ if (!cmpxchg_to_user_page(vma, page, user_addr, token, expected, 0))
+ return false;
+ set_page_dirty_lock(page);
+
+ return true;
+}
+
+int arch_shstk_validate_clone(struct task_struct *tsk,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ unsigned long gcspr_el0;
+ int ret = 0;
+
+ /* Ensure that a token written as a result of a pivot is visible */
+ gcsb_dsync();
+
+ gcspr_el0 = args->shadow_stack_pointer;
+ if (!gcs_consume_token(vma, page, gcspr_el0))
+ return -EINVAL;
+
+ tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
+
+ /* Ensure that our token consumption visible */
+ gcsb_dsync();
+
+ return ret;
+}
+
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
{
unsigned long alloc_size;
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 4cb77e004615dff003426a2eb594460ca1015f4e..252feeda69991e939942c74556e23e27c835e766 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -6,6 +6,7 @@
#include <linux/types.h>
struct task_struct;
+struct kernel_clone_args;
struct ksignal;
#ifdef CONFIG_X86_USER_SHADOW_STACK
@@ -16,8 +17,8 @@ struct thread_shstk {
long shstk_prctl(struct task_struct *task, int option, unsigned long arg2);
void reset_thread_features(void);
-unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
- unsigned long stack_size);
+unsigned long shstk_alloc_thread_stack(struct task_struct *p,
+ const struct kernel_clone_args *args);
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
@@ -28,8 +29,10 @@ static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
static inline void reset_thread_features(void) {}
static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
- unsigned long clone_flags,
- unsigned long stack_size) { return 0; }
+ const struct kernel_clone_args *args)
+{
+ return 0;
+}
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index f63f8fd00a91f3d1171f307b92179556ba2d716d..59456ab8d93faee29c3b223b64eb41659df76032 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -207,7 +207,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
* is disabled, new_ssp will remain 0, and fpu_clone() will know not to
* update it.
*/
- new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
+ new_ssp = shstk_alloc_thread_stack(p, args);
if (IS_ERR_VALUE(new_ssp))
return PTR_ERR((void *)new_ssp);
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 059685612362d7b1865eabf400888fbfa0659c1e..056e2c9ec30531d0901297da07f1842b47d2fcd5 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -191,18 +191,65 @@ void reset_thread_features(void)
current->thread.features_locked = 0;
}
-unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags,
- unsigned long stack_size)
+int arch_shstk_validate_clone(struct task_struct *t,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ /*
+ * SSP is aligned, so reserved bits and mode bit are a zero, just mark
+ * the token 64-bit.
+ */
+ void *maddr = kmap_local_page(page);
+ int offset;
+ unsigned long addr, ssp;
+ u64 expected;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = args->shadow_stack_pointer;
+ addr = ssp - SS_FRAME_SIZE;
+ expected = ssp | BIT(0);
+ offset = offset_in_page(addr);
+
+ if (!cmpxchg_to_user_page(vma, page, addr, (unsigned long *)(maddr + offset),
+ expected, 0))
+ return -EINVAL;
+ set_page_dirty_lock(page);
+
+ return 0;
+}
+
+unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
struct thread_shstk *shstk = &tsk->thread.shstk;
+ unsigned long clone_flags = args->flags;
unsigned long addr, size;
/*
* If shadow stack is not enabled on the new thread, skip any
- * switch to a new shadow stack.
+ * implicit switch to a new shadow stack and reject attempts to
+ * explicitly specify one.
*/
- if (!features_enabled(ARCH_SHSTK_SHSTK))
+ if (!features_enabled(ARCH_SHSTK_SHSTK)) {
+ if (args->shadow_stack_pointer)
+ return (unsigned long)ERR_PTR(-EINVAL);
+
return 0;
+ }
+
+ /*
+ * If the user specified a shadow stack then use it, otherwise
+ * fall back to a default allocation strategy. Validation is
+ * done in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_pointer) {
+ shstk->base = 0;
+ shstk->size = 0;
+ return args->shadow_stack_pointer;
+ }
/*
* For CLONE_VFORK the child will share the parents shadow stack.
@@ -222,7 +269,7 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long cl
if (!(clone_flags & CLONE_VM))
return 0;
- size = adjust_shstk_size(stack_size);
+ size = adjust_shstk_size(args->stack_size);
addr = alloc_shstk(0, size, 0, false);
if (IS_ERR_VALUE(addr))
return addr;
diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h
index 7ee8a179d1036e1d8010b8b18a8f3022e41c1695..96cc0c7a5c90fd7e899d0c5fe7c706302265efcf 100644
--- a/include/asm-generic/cacheflush.h
+++ b/include/asm-generic/cacheflush.h
@@ -124,4 +124,15 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
} while (0)
#endif
+#ifndef cmpxchg_to_user_page
+#define cmpxchg_to_user_page(vma, page, vaddr, ptr, old, new) \
+({ \
+ bool ret; \
+ \
+ ret = try_cmpxchg(ptr, &old, new); \
+ flush_icache_user_page(vma, page, vaddr, sizeof(*ptr)); \
+ ret; \
+})
+#endif
+
#endif /* _ASM_GENERIC_CACHEFLUSH_H */
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index 0f2aeb37bbb047335a399326b31bc8df81b75a3a..cd36389619d5c97401f7b90e177c6027c232783b 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -16,6 +16,7 @@ struct task_struct;
struct rusage;
union thread_union;
struct css_set;
+struct vm_area_struct;
/* All the bits taken by the old clone syscall. */
#define CLONE_LEGACY_FLAGS 0xffffffffULL
@@ -43,6 +44,7 @@ struct kernel_clone_args {
void *fn_arg;
struct cgroup *cgrp;
struct css_set *cset;
+ unsigned long shadow_stack_pointer;
};
/*
@@ -236,4 +238,19 @@ static inline void task_unlock(struct task_struct *p)
DEFINE_GUARD(task_lock, struct task_struct *, task_lock(_T), task_unlock(_T))
+#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
+int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args);
+#else
+static inline int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_SCHED_TASK_H */
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 359a14cc76a4038aeacef14b2915d5ce60d0cf44..586a1c05a4e4ca05584d4d500223bcf6c3add54c 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -84,6 +84,8 @@
* kernel's limit of nested PID namespaces.
* @cgroup: If CLONE_INTO_CGROUP is specified set this to
* a file descriptor for the cgroup.
+ * @shadow_stack_pointer: Value to use for shadow stack pointer in the
+ * child process.
*
* The structure is versioned by size and thus extensible.
* New struct members must go at the end of the struct and
@@ -101,12 +103,14 @@ struct clone_args {
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
+ __aligned_u64 shadow_stack_pointer;
};
#endif
-#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
-#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
-#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
/*
* Scheduling policies
diff --git a/kernel/fork.c b/kernel/fork.c
index 60c0b4868fd4993920f7a615a47f7e915b9a47b5..d925e0c52a5ac4394abd1d46a5eb14386d7ba2ca 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2109,6 +2109,51 @@ static void rv_task_fork(struct task_struct *p)
#define rv_task_fork(p) do {} while (0)
#endif
+static int shstk_validate_clone(struct task_struct *p,
+ struct kernel_clone_args *args)
+{
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ struct page *page;
+ unsigned long addr;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
+ return 0;
+
+ if (!args->shadow_stack_pointer)
+ return 0;
+
+ mm = get_task_mm(p);
+ if (!mm)
+ return -EFAULT;
+
+ mmap_read_lock(mm);
+
+ addr = untagged_addr_remote(mm, args->shadow_stack_pointer);
+ page = get_user_page_vma_remote(mm, addr, FOLL_FORCE | FOLL_WRITE,
+ &vma);
+ if (IS_ERR(page)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (!(vma->vm_flags & VM_SHADOW_STACK) ||
+ !(vma->vm_flags & VM_WRITE)) {
+ ret = -EFAULT;
+ goto out_page;
+ }
+
+ ret = arch_shstk_validate_clone(p, vma, page, args);
+
+out_page:
+ put_page(page);
+out:
+ mmap_read_unlock(mm);
+ mmput(mm);
+ return ret;
+}
+
/*
* This creates a new process as a copy of the old one,
* but does not actually start it yet.
@@ -2382,6 +2427,9 @@ __latent_entropy struct task_struct *copy_process(
if (retval)
goto bad_fork_cleanup_namespaces;
retval = copy_thread(p, args);
+ if (retval)
+ goto bad_fork_cleanup_io;
+ retval = shstk_validate_clone(p, args);
if (retval)
goto bad_fork_cleanup_io;
@@ -2945,7 +2993,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER1);
BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=
CLONE_ARGS_SIZE_VER2);
- BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
+ BUILD_BUG_ON(offsetofend(struct clone_args, shadow_stack_pointer) !=
+ CLONE_ARGS_SIZE_VER3);
+ BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER3);
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
@@ -2978,16 +3028,17 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
return -EINVAL;
*kargs = (struct kernel_clone_args){
- .flags = args.flags,
- .pidfd = u64_to_user_ptr(args.pidfd),
- .child_tid = u64_to_user_ptr(args.child_tid),
- .parent_tid = u64_to_user_ptr(args.parent_tid),
- .exit_signal = args.exit_signal,
- .stack = args.stack,
- .stack_size = args.stack_size,
- .tls = args.tls,
- .set_tid_size = args.set_tid_size,
- .cgroup = args.cgroup,
+ .flags = args.flags,
+ .pidfd = u64_to_user_ptr(args.pidfd),
+ .child_tid = u64_to_user_ptr(args.child_tid),
+ .parent_tid = u64_to_user_ptr(args.parent_tid),
+ .exit_signal = args.exit_signal,
+ .stack = args.stack,
+ .stack_size = args.stack_size,
+ .tls = args.tls,
+ .set_tid_size = args.set_tid_size,
+ .cgroup = args.cgroup,
+ .shadow_stack_pointer = args.shadow_stack_pointer,
};
if (args.set_tid &&
@@ -3028,6 +3079,27 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
return true;
}
+/**
+ * clone3_shadow_stack_valid - check and prepare shadow stack
+ * @kargs: kernel clone args
+ *
+ * Verify that shadow stacks are only enabled if supported.
+ */
+static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs)
+{
+ if (!kargs->shadow_stack_pointer)
+ return true;
+
+ if (!IS_ALIGNED(kargs->shadow_stack_pointer, sizeof(void *)))
+ return false;
+
+ /*
+ * The architecture must check support on the specific
+ * machine.
+ */
+ return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
+}
+
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
@@ -3050,7 +3122,7 @@ static bool clone3_args_valid(struct kernel_clone_args *kargs)
kargs->exit_signal)
return false;
- if (!clone3_stack_valid(kargs))
+ if (!clone3_stack_valid(kargs) || !clone3_shadow_stack_valid(kargs))
return false;
return true;
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 3/8] selftests: Provide helper header for shadow stack testing
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
While almost all users of shadow stacks should be relying on the dynamic
linker and libc to enable the feature there are several low level test
programs where it is useful to enable without any libc support, allowing
testing without full system enablement. This low level testing is helpful
during bringup of the support itself, and also in enabling coverage by
automated testing without needing all system components in the target root
filesystems to have enablement.
Provide a header with helpers for this purpose, intended for use only by
test programs directly exercising shadow stack interfaces.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/tools/testing/selftests/ksft_shstk.h b/tools/testing/selftests/ksft_shstk.h
new file mode 100644
index 0000000000000000000000000000000000000000..869ecea2bf3ea3d30cead9819d2b3a75f5397754
--- /dev/null
+++ b/tools/testing/selftests/ksft_shstk.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Helpers for shadow stack enablement, this is intended to only be
+ * used by low level test programs directly exercising interfaces for
+ * working with shadow stacks.
+ *
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#ifndef __KSFT_SHSTK_H
+#define __KSFT_SHSTK_H
+
+#include <asm/mman.h>
+
+/* This is currently only defined for x86 */
+#ifndef SHADOW_STACK_SET_TOKEN
+#define SHADOW_STACK_SET_TOKEN (1ULL << 0)
+#endif
+
+static bool shadow_stack_enabled;
+
+#ifdef __x86_64__
+#define ARCH_SHSTK_ENABLE 0x5001
+#define ARCH_SHSTK_SHSTK (1ULL << 0)
+
+#define ARCH_PRCTL(arg1, arg2) \
+({ \
+ long _ret; \
+ register long _num asm("eax") = __NR_arch_prctl; \
+ register long _arg1 asm("rdi") = (long)(arg1); \
+ register long _arg2 asm("rsi") = (long)(arg2); \
+ \
+ asm volatile ( \
+ "syscall\n" \
+ : "=a"(_ret) \
+ : "r"(_arg1), "r"(_arg2), \
+ "0"(_num) \
+ : "rcx", "r11", "memory", "cc" \
+ ); \
+ _ret; \
+})
+
+#define ENABLE_SHADOW_STACK
+static inline __attribute__((always_inline)) void enable_shadow_stack(void)
+{
+ int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifdef __aarch64__
+#define PR_SET_SHADOW_STACK_STATUS 75
+# define PR_SHADOW_STACK_ENABLE (1UL << 0)
+
+#define my_syscall2(num, arg1, arg2) \
+({ \
+ register long _num __asm__ ("x8") = (num); \
+ register long _arg1 __asm__ ("x0") = (long)(arg1); \
+ register long _arg2 __asm__ ("x1") = (long)(arg2); \
+ register long _arg3 __asm__ ("x2") = 0; \
+ register long _arg4 __asm__ ("x3") = 0; \
+ register long _arg5 __asm__ ("x4") = 0; \
+ \
+ __asm__ volatile ( \
+ "svc #0\n" \
+ : "=r"(_arg1) \
+ : "r"(_arg1), "r"(_arg2), \
+ "r"(_arg3), "r"(_arg4), \
+ "r"(_arg5), "r"(_num) \
+ : "memory", "cc" \
+ ); \
+ _arg1; \
+})
+
+#define ENABLE_SHADOW_STACK
+static inline __attribute__((always_inline)) void enable_shadow_stack(void)
+{
+ int ret;
+
+ ret = my_syscall2(__NR_prctl, PR_SET_SHADOW_STACK_STATUS,
+ PR_SHADOW_STACK_ENABLE);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifndef __NR_map_shadow_stack
+#define __NR_map_shadow_stack 453
+#endif
+
+#ifndef ENABLE_SHADOW_STACK
+static inline void enable_shadow_stack(void) { }
+#endif
+
+#endif
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 2/8] Documentation: userspace-api: Add shadow stack API documentation
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
There are a number of architectures with shadow stack features which we are
presenting to userspace with as consistent an API as we can (though there
are some architecture specifics). Especially given that there are some
important considerations for userspace code interacting directly with the
feature let's provide some documentation covering the common aspects.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 41 ++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index 274cc7546efc2a042d2dc00aa67c71c52372179a..c39709bfba2c5682d0d1a22444db17c17bcf01ce 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -59,6 +59,7 @@ Everything else
ELF
netlink/index
+ shadow_stack
sysfs-platform_profile
vduse
futex2
diff --git a/Documentation/userspace-api/shadow_stack.rst b/Documentation/userspace-api/shadow_stack.rst
new file mode 100644
index 0000000000000000000000000000000000000000..c576ad3d7ec12f0f75bffa4e2bafd0c9d7230c9f
--- /dev/null
+++ b/Documentation/userspace-api/shadow_stack.rst
@@ -0,0 +1,41 @@
+=============
+Shadow Stacks
+=============
+
+Introduction
+============
+
+Several architectures have features which provide backward edge
+control flow protection through a hardware maintained stack, only
+writeable by userspace through very limited operations. This feature
+is referred to as shadow stacks on Linux, on x86 it is part of Intel
+Control Enforcement Technology (CET), on arm64 it is Guarded Control
+Stacks feature (FEAT_GCS) and for RISC-V it is the Zicfiss extension.
+It is expected that this feature will normally be managed by the
+system dynamic linker and libc in ways broadly transparent to
+application code, this document covers interfaces and considerations.
+
+
+Enabling
+========
+
+Shadow stacks default to disabled when a userspace process is
+executed, they can be enabled for the current thread with a syscall:
+
+ - For x86 the ARCH_SHSTK_ENABLE arch_prctl()
+
+It is expected that this will normally be done by the dynamic linker.
+Any new threads created by a thread with shadow stacks enabled will
+themselves have shadow stacks enabled.
+
+
+Enablement considerations
+=========================
+
+- Returning from the function that enables shadow stacks without first
+ disabling them will cause a shadow stack exception. This includes
+ any syscall wrapper or other library functions, the syscall will need
+ to be inlined.
+- A lock feature allows userspace to prevent disabling of shadow stacks.
+- Those that change the stack context like longjmp() or use of ucontext
+ changes on signal return will need support from libc.
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 1/8] arm64/gcs: Return a success value from gcs_alloc_thread_stack()
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook
In-Reply-To: <20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org>
Currently as a result of templating from x86 code gcs_alloc_thread_stack()
returns a pointer as an unsigned int however on arm64 we don't actually use
this pointer value as anything other than a pass/fail flag. Simplify the
interface to just return an int with 0 on success and a negative error code
on failure.
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/include/asm/gcs.h | 8 ++++----
arch/arm64/kernel/process.c | 8 ++++----
arch/arm64/mm/gcs.c | 8 ++++----
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index f50660603ecf5dc09a92740062df3a089b02b219..d8923b5f03b776252aca76ce316ef57399d71fa9 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -64,8 +64,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
void gcs_set_el0_mode(struct task_struct *task);
void gcs_free(struct task_struct *task);
void gcs_preserve_current_state(void);
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args);
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args);
static inline int gcs_check_locked(struct task_struct *task,
unsigned long new_val)
@@ -91,8 +91,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
static inline void gcs_set_el0_mode(struct task_struct *task) { }
static inline void gcs_free(struct task_struct *task) { }
static inline void gcs_preserve_current_state(void) { }
-static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+static inline int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
return -ENOTSUPP;
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index fdd095480c3ffb8c13fd4e7c9abc79e88143e08b..8ebd11c29792524dfeeade9cc7826b007329aa6a 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -297,7 +297,7 @@ static void flush_gcs(void)
static int copy_thread_gcs(struct task_struct *p,
const struct kernel_clone_args *args)
{
- unsigned long gcs;
+ int ret;
if (!system_supports_gcs())
return 0;
@@ -305,9 +305,9 @@ static int copy_thread_gcs(struct task_struct *p,
p->thread.gcs_base = 0;
p->thread.gcs_size = 0;
- gcs = gcs_alloc_thread_stack(p, args);
- if (IS_ERR_VALUE(gcs))
- return PTR_ERR((void *)gcs);
+ ret = gcs_alloc_thread_stack(p, args);
+ if (ret != 0)
+ return ret;
p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 5c46ec527b1cdaa8f52cff445d70ba0f8509d086..1f633a482558b59aac5427963d42b37fce08c8a6 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -38,8 +38,8 @@ static unsigned long gcs_size(unsigned long size)
return max(PAGE_SIZE, size);
}
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
unsigned long addr, size;
@@ -59,13 +59,13 @@ unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
size = gcs_size(size);
addr = alloc_gcs(0, size);
if (IS_ERR_VALUE(addr))
- return addr;
+ return PTR_ERR((void *)addr);
tsk->thread.gcs_base = addr;
tsk->thread.gcs_size = size;
tsk->thread.gcspr_el0 = addr + size - sizeof(u64);
- return addr;
+ return 0;
}
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
--
2.39.2
^ permalink raw reply related
* [PATCH RFT v11 0/8] fork: Support shadow stacks in clone3()
From: Mark Brown @ 2024-10-05 10:31 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
The kernel has recently added support for shadow stacks, currently
x86 only using their CET feature but both arm64 and RISC-V have
equivalent features (GCS and Zicfiss respectively), I am actively
working on GCS[1]. With shadow stacks the hardware maintains an
additional stack containing only the return addresses for branch
instructions which is not generally writeable by userspace and ensures
that any returns are to the recorded addresses. This provides some
protection against ROP attacks and making it easier to collect call
stacks. These shadow stacks are allocated in the address space of the
userspace process.
Our API for shadow stacks does not currently offer userspace any
flexiblity for managing the allocation of shadow stacks for newly
created threads, instead the kernel allocates a new shadow stack with
the same size as the normal stack whenever a thread is created with the
feature enabled. The stacks allocated in this way are freed by the
kernel when the thread exits or shadow stacks are disabled for the
thread. This lack of flexibility and control isn't ideal, in the vast
majority of cases the shadow stack will be over allocated and the
implicit allocation and deallocation is not consistent with other
interfaces. As far as I can tell the interface is done in this manner
mainly because the shadow stack patches were in development since before
clone3() was implemented.
Since clone3() is readily extensible let's add support for specifying a
shadow stack when creating a new thread or process, keeping the current
implicit allocation behaviour if one is not specified either with
clone3() or through the use of clone(). The user must provide a shadow
stack pointer, this must point to memory mapped for use as a shadow
stackby map_shadow_stack() with an architecture specified shadow stack
token at the top of the stack.
Please note that the x86 portions of this code are build tested only, I
don't appear to have a system that can run CET available to me.
[1] https://lore.kernel.org/linux-arm-kernel/20241001-arm64-gcs-v13-0-222b78d87eee@kernel.org/T/#mc58f97f27461749ccf400ebabf6f9f937116a86b
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v11:
- Rebase onto arm64 for-next/gcs, which is based on v6.12-rc1, and
integrate arm64 support.
- Rework the interface to specify a shadow stack pointer rather than a
base and size like we do for the regular stack.
- Link to v10: https://lore.kernel.org/r/20240821-clone3-shadow-stack-v10-0-06e8797b9445@kernel.org
Changes in v10:
- Integrate fixes & improvements for the x86 implementation from Rick
Edgecombe.
- Require that the shadow stack be VM_WRITE.
- Require that the shadow stack base and size be sizeof(void *) aligned.
- Clean up trailing newline.
- Link to v9: https://lore.kernel.org/r/20240819-clone3-shadow-stack-v9-0-962d74f99464@kernel.org
Changes in v9:
- Pull token validation earlier and report problems with an error return
to parent rather than signal delivery to the child.
- Verify that the top of the supplied shadow stack is VM_SHADOW_STACK.
- Rework token validation to only do the page mapping once.
- Drop no longer needed support for testing for signals in selftest.
- Fix typo in comments.
- Link to v8: https://lore.kernel.org/r/20240808-clone3-shadow-stack-v8-0-0acf37caf14c@kernel.org
Changes in v8:
- Fix token verification with user specified shadow stack.
- Don't track user managed shadow stacks for child processes.
- Link to v7: https://lore.kernel.org/r/20240731-clone3-shadow-stack-v7-0-a9532eebfb1d@kernel.org
Changes in v7:
- Rebase onto v6.11-rc1.
- Typo fixes.
- Link to v6: https://lore.kernel.org/r/20240623-clone3-shadow-stack-v6-0-9ee7783b1fb9@kernel.org
Changes in v6:
- Rebase onto v6.10-rc3.
- Ensure we don't try to free the parent shadow stack in error paths of
x86 arch code.
- Spelling fixes in userspace API document.
- Additional cleanups and improvements to the clone3() tests to support
the shadow stack tests.
- Link to v5: https://lore.kernel.org/r/20240203-clone3-shadow-stack-v5-0-322c69598e4b@kernel.org
Changes in v5:
- Rebase onto v6.8-rc2.
- Rework ABI to have the user allocate the shadow stack memory with
map_shadow_stack() and a token.
- Force inlining of the x86 shadow stack enablement.
- Move shadow stack enablement out into a shared header for reuse by
other tests.
- Link to v4: https://lore.kernel.org/r/20231128-clone3-shadow-stack-v4-0-8b28ffe4f676@kernel.org
Changes in v4:
- Formatting changes.
- Use a define for minimum shadow stack size and move some basic
validation to fork.c.
- Link to v3: https://lore.kernel.org/r/20231120-clone3-shadow-stack-v3-0-a7b8ed3e2acc@kernel.org
Changes in v3:
- Rebase onto v6.7-rc2.
- Remove stale shadow_stack in internal kargs.
- If a shadow stack is specified unconditionally use it regardless of
CLONE_ parameters.
- Force enable shadow stacks in the selftest.
- Update changelogs for RISC-V feature rename.
- Link to v2: https://lore.kernel.org/r/20231114-clone3-shadow-stack-v2-0-b613f8681155@kernel.org
Changes in v2:
- Rebase onto v6.7-rc1.
- Remove ability to provide preallocated shadow stack, just specify the
desired size.
- Link to v1: https://lore.kernel.org/r/20231023-clone3-shadow-stack-v1-0-d867d0b5d4d0@kernel.org
---
Mark Brown (8):
arm64/gcs: Return a success value from gcs_alloc_thread_stack()
Documentation: userspace-api: Add shadow stack API documentation
selftests: Provide helper header for shadow stack testing
fork: Add shadow stack support to clone3()
selftests/clone3: Remove redundant flushes of output streams
selftests/clone3: Factor more of main loop into test_clone3()
selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
selftests/clone3: Test shadow stack support
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 41 ++++
arch/arm64/include/asm/gcs.h | 8 +-
arch/arm64/kernel/process.c | 8 +-
arch/arm64/mm/gcs.c | 62 +++++-
arch/x86/include/asm/shstk.h | 11 +-
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 57 +++++-
include/asm-generic/cacheflush.h | 11 ++
include/linux/sched/task.h | 17 ++
include/uapi/linux/sched.h | 10 +-
kernel/fork.c | 96 +++++++--
tools/testing/selftests/clone3/clone3.c | 226 ++++++++++++++++++----
tools/testing/selftests/clone3/clone3_selftests.h | 65 ++++++-
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++
15 files changed, 632 insertions(+), 81 deletions(-)
---
base-commit: d17cd7b7cc92d37ee8b2df8f975fc859a261f4dc
change-id: 20231019-clone3-shadow-stack-15d40d2bf536
Best regards,
--
Mark Brown <broonie@kernel.org>
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Yury Khrustalev @ 2024-10-03 16:05 UTC (permalink / raw)
To: Christian Brauner
Cc: Edgecombe, Rick P, Florian Weimer, broonie@kernel.org,
dietmar.eggemann@arm.com, shuah@kernel.org, Szabolcs.Nagy@arm.com,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
linux-api@vger.kernel.org, vincent.guittot@linaro.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
vschneid@redhat.com, catalin.marinas@arm.com, kees@kernel.org,
will@kernel.org, hpa@zytor.com, jannh@google.com,
peterz@infradead.org, bp@alien8.de, wilco.dijkstra@arm.com,
linux-kselftest@vger.kernel.org, bsegall@google.com,
juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <20241001-atheismus-stetig-4f6f3001715c@brauner>
On Tue, Oct 01, 2024 at 05:12:38PM +0200, Christian Brauner wrote:
> > Thanks for the info!
> >
> > >
> > > My preference is to keep the api consistent and require a stack_size for
> > > shadow stacks as well.
> >
> > Did you catch that a token can be at a different offsets location on the stack
> > depending on args passed to map_shadow_stack? So userspace will need something
> > like the code above, but that adjusts the 'shadow_stack_size' such that the
> > kernel looks for the token in the right place. It will be even weirder if
> > someone uses clone3 to switch to a stack that has already been used, and pivoted
> > off of, such that a token was left in the middle of the stack. In that case
> > userspace would have to come up with args disconnected from the actual size of
> > the shadow stack such that the kernel would be cajoled into looking for the
> > token in the right place.
> >
> > A shadow stack size is more symmetric on the surface, but I'm not sure it will
> > be easier for userspace to handle. So I think we should just have a pointer to
> > the token. But it will be a usable implementation either way.
>
> Maybe it's best to let glibc folks decide what is better/more ergonomic for them.
I agree that it would be better to just have a pointer to the token.
My preference would be to avoid having obscure additional arguments that may end up
having misleading name or bear some hidden functionality. If kernel is not going to
use stack size as such, then users should not have to provide it.
Thanks,
Yury
PS Apologies for delayed reply
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Edgecombe, Rick P @ 2024-10-02 21:25 UTC (permalink / raw)
To: broonie@kernel.org
Cc: dietmar.eggemann@arm.com, x86@kernel.org, shuah@kernel.org,
brauner@kernel.org, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, vincent.guittot@linaro.org,
fweimer@redhat.com, linux-kernel@vger.kernel.org,
mingo@redhat.com, hjl.tools@gmail.com, rostedt@goodmis.org,
tglx@linutronix.de, linux-api@vger.kernel.org,
vschneid@redhat.com, Szabolcs.Nagy@arm.com, kees@kernel.org,
will@kernel.org, hpa@zytor.com, catalin.marinas@arm.com,
jannh@google.com, yury.khrustalev@arm.com, peterz@infradead.org,
bp@alien8.de, linux-kselftest@vger.kernel.org,
wilco.dijkstra@arm.com, bsegall@google.com, juri.lelli@redhat.com
In-Reply-To: <Zv20luC6us-LEMqN@finisterre.sirena.org.uk>
On Wed, 2024-10-02 at 22:01 +0100, Mark Brown wrote:
> BTW it's probably also worth noting that at least on arm64 (perhaps x86
> is different here?) the shadow stack of a thread that exited won't have
> a token placed on it so it won't be possible to use it with clone3() at
> all unless another token is written. To get a shadow stack you could
> use with clone3() you'd either need to allocate a new one, pivot away
> from one that's currently in use or enable shadow stack writes and place
> a token.
Hmm, yea. I didn't have a specific idea in mind. But yea, you would have to
switch to something in order to leave a token.
If you enabled WRSS (or similar) you might be able to reuse shadow stacks in
some kind of useful way, but in that case you would probably WRSS the token to
the end of the shadow stack and the start+size would fit better.
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2024-10-02 21:01 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: brauner@kernel.org, dietmar.eggemann@arm.com, shuah@kernel.org,
Szabolcs.Nagy@arm.com, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, linux-api@vger.kernel.org,
vincent.guittot@linaro.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, hjl.tools@gmail.com,
tglx@linutronix.de, fweimer@redhat.com, vschneid@redhat.com,
catalin.marinas@arm.com, kees@kernel.org, will@kernel.org,
hpa@zytor.com, jannh@google.com, peterz@infradead.org,
yury.khrustalev@arm.com, bp@alien8.de, wilco.dijkstra@arm.com,
linux-kselftest@vger.kernel.org, bsegall@google.com,
juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <b7ef38c9-1e87-468f-94a5-a3c7f209d200@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]
On Wed, Oct 02, 2024 at 02:42:58PM +0100, Mark Brown wrote:
> On Tue, Oct 01, 2024 at 11:03:10PM +0000, Edgecombe, Rick P wrote:
> > I'm not so sure. The thing is a regular stack can be re-used in full - just set
> > the RSP to the end and take advantage of the whole stack. A shadow stack can
> > only be used where there is a token.
> Yeah, I'm not sure how appealing it is trying to use a memory pool with
> of shadow stacks - like you say you can't reset the top of the stack so
> you need to keep track of that when the stack becomes unused. If the
> users don't leave the SSP at the top of the stack then unless writes
> have been enabled (which has security issues) then gradually the size of
> the shadow stacks will be eroded which will need to be managed. You
> could do it, but it's clearly not really how things are supposed to
> work. The use case with starting a new worker thread for an existing in
> use state seems much more appealing.
BTW it's probably also worth noting that at least on arm64 (perhaps x86
is different here?) the shadow stack of a thread that exited won't have
a token placed on it so it won't be possible to use it with clone3() at
all unless another token is written. To get a shadow stack you could
use with clone3() you'd either need to allocate a new one, pivot away
from one that's currently in use or enable shadow stack writes and place
a token.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2024-10-02 13:42 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: brauner@kernel.org, dietmar.eggemann@arm.com, shuah@kernel.org,
Szabolcs.Nagy@arm.com, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, linux-api@vger.kernel.org,
vincent.guittot@linaro.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, hjl.tools@gmail.com,
tglx@linutronix.de, fweimer@redhat.com, vschneid@redhat.com,
catalin.marinas@arm.com, kees@kernel.org, will@kernel.org,
hpa@zytor.com, jannh@google.com, peterz@infradead.org,
yury.khrustalev@arm.com, bp@alien8.de, wilco.dijkstra@arm.com,
linux-kselftest@vger.kernel.org, bsegall@google.com,
juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <0999160fd5282ac129aab300b667af35d7251582.camel@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]
On Tue, Oct 01, 2024 at 11:03:10PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2024-10-01 at 18:33 +0100, Mark Brown wrote:
> > My suspicion would be that if we're doing the pivot to a previously used
> > shadow stack we'd also be pivoting the regular stack along with it which
> > would face similar issues with having an unusual method for specifying
> > the stack top so I don't know how much we're really winning.
> I'm not so sure. The thing is a regular stack can be re-used in full - just set
> the RSP to the end and take advantage of the whole stack. A shadow stack can
> only be used where there is a token.
Yeah, I'm not sure how appealing it is trying to use a memory pool with
of shadow stacks - like you say you can't reset the top of the stack so
you need to keep track of that when the stack becomes unused. If the
users don't leave the SSP at the top of the stack then unless writes
have been enabled (which has security issues) then gradually the size of
the shadow stacks will be eroded which will need to be managed. You
could do it, but it's clearly not really how things are supposed to
work. The use case with starting a new worker thread for an existing in
use state seems much more appealing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Edgecombe, Rick P @ 2024-10-01 23:03 UTC (permalink / raw)
To: broonie@kernel.org, brauner@kernel.org
Cc: dietmar.eggemann@arm.com, shuah@kernel.org, Szabolcs.Nagy@arm.com,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
linux-api@vger.kernel.org, vincent.guittot@linaro.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
fweimer@redhat.com, vschneid@redhat.com, catalin.marinas@arm.com,
kees@kernel.org, will@kernel.org, hpa@zytor.com, jannh@google.com,
peterz@infradead.org, yury.khrustalev@arm.com, bp@alien8.de,
wilco.dijkstra@arm.com, linux-kselftest@vger.kernel.org,
bsegall@google.com, juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <6bf15851-03fe-40cd-b95c-f7e2ca40ac54@sirena.org.uk>
On Tue, 2024-10-01 at 18:33 +0100, Mark Brown wrote:
> > > A shadow stack size is more symmetric on the surface, but I'm not sure it
> > > will
> > > be easier for userspace to handle. So I think we should just have a
> > > pointer to
> > > the token. But it will be a usable implementation either way.
>
> My suspicion would be that if we're doing the pivot to a previously used
> shadow stack we'd also be pivoting the regular stack along with it which
> would face similar issues with having an unusual method for specifying
> the stack top so I don't know how much we're really winning.
I'm not so sure. The thing is a regular stack can be re-used in full - just set
the RSP to the end and take advantage of the whole stack. A shadow stack can
only be used where there is a token.
> Like we
> both keep saying either of the interfaces works though, it's just a
> taste question with both having downsides.
Fair enough.
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2024-10-01 17:33 UTC (permalink / raw)
To: Christian Brauner
Cc: Edgecombe, Rick P, Florian Weimer, dietmar.eggemann@arm.com,
shuah@kernel.org, Szabolcs.Nagy@arm.com,
dave.hansen@linux.intel.com, debug@rivosinc.com, mgorman@suse.de,
linux-api@vger.kernel.org, vincent.guittot@linaro.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, hjl.tools@gmail.com, tglx@linutronix.de,
vschneid@redhat.com, catalin.marinas@arm.com, kees@kernel.org,
will@kernel.org, hpa@zytor.com, jannh@google.com,
peterz@infradead.org, yury.khrustalev@arm.com, bp@alien8.de,
wilco.dijkstra@arm.com, linux-kselftest@vger.kernel.org,
bsegall@google.com, juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <20241001-atheismus-stetig-4f6f3001715c@brauner>
[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]
On Tue, Oct 01, 2024 at 05:12:38PM +0200, Christian Brauner wrote:
> On Fri, Sep 27, 2024 at 03:21:59PM GMT, Edgecombe, Rick P wrote:
> > Did you catch that a token can be at a different offsets location on the stack
> > depending on args passed to map_shadow_stack? So userspace will need something
> > like the code above, but that adjusts the 'shadow_stack_size' such that the
> > kernel looks for the token in the right place. It will be even weirder if
> > someone uses clone3 to switch to a stack that has already been used, and pivoted
> > off of, such that a token was left in the middle of the stack. In that case
> > userspace would have to come up with args disconnected from the actual size of
> > the shadow stack such that the kernel would be cajoled into looking for the
> > token in the right place.
> >
> > A shadow stack size is more symmetric on the surface, but I'm not sure it will
> > be easier for userspace to handle. So I think we should just have a pointer to
> > the token. But it will be a usable implementation either way.
My suspicion would be that if we're doing the pivot to a previously used
shadow stack we'd also be pivoting the regular stack along with it which
would face similar issues with having an unusual method for specifying
the stack top so I don't know how much we're really winning. Like we
both keep saying either of the interfaces works though, it's just a
taste question with both having downsides.
> Maybe it's best to let glibc folks decide what is better/more ergonomic for them.
The relevant people are on the thread I think.
I've rebased onto v6.12-rc1, assuming I don't notice anything horrible
in testing I'll post that with the ABI unchanged for now.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFT v9 4/8] fork: Add shadow stack support to clone3()
From: Christian Brauner @ 2024-10-01 15:12 UTC (permalink / raw)
To: Edgecombe, Rick P, Florian Weimer
Cc: broonie@kernel.org, dietmar.eggemann@arm.com, shuah@kernel.org,
Szabolcs.Nagy@arm.com, dave.hansen@linux.intel.com,
debug@rivosinc.com, mgorman@suse.de, linux-api@vger.kernel.org,
vincent.guittot@linaro.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, hjl.tools@gmail.com,
tglx@linutronix.de, fweimer@redhat.com, vschneid@redhat.com,
catalin.marinas@arm.com, kees@kernel.org, will@kernel.org,
hpa@zytor.com, jannh@google.com, peterz@infradead.org,
yury.khrustalev@arm.com, bp@alien8.de, wilco.dijkstra@arm.com,
linux-kselftest@vger.kernel.org, bsegall@google.com,
juri.lelli@redhat.com, x86@kernel.org
In-Reply-To: <727524e9109022632250ab0485f5ecc1c1900092.camel@intel.com>
On Fri, Sep 27, 2024 at 03:21:59PM GMT, Edgecombe, Rick P wrote:
> On Fri, 2024-09-27 at 10:50 +0200, Christian Brauner wrote:
> > The legacy clone system call had required userspace to know in which
> > direction the stack was growing and then pass down the stack pointer
> > appropriately (e.g., parisc grows upwards).
> >
> > And in fact, the old clone() system call did take an additional
> > stack_size argument on specific architectures. For example, on
> > microblaze.
> >
> > Also, when clone3() was done we still had ia64 in the tree which had a
> > separate clone2() system call that also required a stack_size argument.
> >
> > So userspace ended up with code like this or worse:
> >
> > #define __STACK_SIZE (8 * 1024 * 1024)
> > pid_t sys_clone(int (*fn)(void *), void *arg, int flags, int *pidfd)
> > {
> > pid_t ret;
> > void *stack;
> >
> > stack = malloc(__STACK_SIZE);
> > if (!stack)
> > return -ENOMEM;
> >
> > #ifdef __ia64__
> > ret = __clone2(fn, stack, __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
> > #elif defined(__parisc__) /* stack grows up */
> > ret = clone(fn, stack, flags | SIGCHLD, arg, pidfd);
> > #else
> > ret = clone(fn, stack + __STACK_SIZE, flags | SIGCHLD, arg, pidfd);
> > #endif
> > return ret;
> > }
> >
> > So we talked to the glibc folks which preferred the kernel to do all
> > this nonsense for them as it has that knowledge.
>
> Thanks for the info!
>
> >
> > My preference is to keep the api consistent and require a stack_size for
> > shadow stacks as well.
>
> Did you catch that a token can be at a different offsets location on the stack
> depending on args passed to map_shadow_stack? So userspace will need something
> like the code above, but that adjusts the 'shadow_stack_size' such that the
> kernel looks for the token in the right place. It will be even weirder if
> someone uses clone3 to switch to a stack that has already been used, and pivoted
> off of, such that a token was left in the middle of the stack. In that case
> userspace would have to come up with args disconnected from the actual size of
> the shadow stack such that the kernel would be cajoled into looking for the
> token in the right place.
>
> A shadow stack size is more symmetric on the surface, but I'm not sure it will
> be easier for userspace to handle. So I think we should just have a pointer to
> the token. But it will be a usable implementation either way.
Maybe it's best to let glibc folks decide what is better/more ergonomic for them.
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-10-01 14:31 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Aleksa Sarai, Florian Weimer, Christian Brauner,
Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20241001-stapfen-darfst-5fe2a8b2c6ec@brauner>
On Tue, Oct 01, 2024 at 12:21:32PM GMT, Christian Brauner wrote:
> On Mon, Sep 30, 2024 at 03:32:25PM GMT, Lorenzo Stoakes wrote:
> > On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:
[snip]
> > > Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
> > > they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
> > > current's tid)? In principle I guess users might use PIDFD_SELF by
> > > accident but if we mirror the naming with /proc/{,thread-}self that
> > > might not be that big of an issue?
> >
> > Lol, you know I wasn't even aware /proc/thread-self existed...
>
> Wait until you learn that /proc/$TID thread entries exist but aren't
> shown when you do ls -al /proc, only when you explicitly access them.
My God... You're right, that's crazy... :)
>
> >
> > Yeah, that actually makes sense and is consistent, though obviously the
> > concern is people will reflexively use PIDFD_SELF and end up with
> > potentially confusing results.
> >
> > I will obviously be doing a manpage patch for this so we can spell it out
> > there very clearly and also in the header - so I'd actually lean towards
> > doing this myself.
> >
> > Christian, Florian? Thoughts?
>
> I think adding both would be potentially useful. How about:
>
> #define PIDFD_SELF -100
> #define PIDFD_THREAD_GROUP -200
Sure, makes sense to add both.
>
> This will make PIDFD_SELF mean current and PIDFD_THREAD_GROUP mean
> current->pid_links[PIDTYPE_TGID]. I don't think we need to or should
> mirror procfs in any way. pidfds are intended to be usable without
> procfs at all.
Yeah, I think it's important to ensure the _default_ choice, so in this
case PIDFD_SELF clearly, is one that will be least surprising.
The proc thing is sort of pleasing from an aesthetic point of view, but if
you followed it you'd have to _clearly_ document PIDFD_THREAD_SELF as the
default.
Happy to go along with this. PIDFD_THREAD_GROUP is also clearer as it is
distinct from PIDFD_SELF (doesn't reference 'self' at all).
>
> I want to leave one comment on a bit of quirkiness in the api when we
> add this. I don't consider it a big deal but it should be pointed out.
>
> It can be useful to allow PIDFD_SELF or PIDFD_THREAD_GROUP to refer to
> the calling thread even for pidfd_open() to avoid an additional getpid()
> system call:
>
> (1) pidfd_open(PIDFD_SELF, PIDFD_THREAD)
> (2) pidfd_open(PIDFD_SELF, 0)
>
Hm, this is a bit weird, as these are pid_t's and PIDFD_SELF and
PIDFD_THREAD_GROUP are otherwise (pid)fd's.
Being dummy values sort of allows us to put them into service here also,
but it is just weird, we pass what is usually a pidfd to receive a pidfd,
only this time it's an actually concrete one?
I'm not sure I like this, even though as you say it avoids a getpid().
If we did this I'd prefer it to be a separate name, even if it has the same
numeric value (I guess we also might want to check for anything that uses a
negative pid_t to refer to an error or something else too).
Perhaps PID_SELF and PID_THREAD_GROUP?
> So if we allow this (Should we allow it?) then (1) is just redundant but
> whathever. But (2) is at least worth discussing: Should we reject (2) on
> the grounds of contradictory requests or allow it and document that it's
> equivalent to pidfd_open(getpid(), PIDFD_THREAD)? It feels like the
> latter would be ok.
>
> Similar for pidfd_send_signal():
>
> // redundant but ok:
> (1) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD)
>
> // redundant but ok:
> (2) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD_GROUP)
>
> // weird way to spell pidfd_send_signal(PIDFD_THREAD_GROUP, 0)
> (3) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD_GROUP)
>
> // weird way to spell pidfd_send_signal(PIDFD_SELF, 0)
> (4) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD)
>
> I think all of this is ok but does anyone else have a strong opinion?
I think it's fine to allow all 4 and we should get this behaviour by
default (if we have no flags we use the f_flags as a hint, which will be
set correctly).
I think people might find contradictory ones, i.e. 3 and 4, strange, but it
makes sense for the flags to override the pidfd (as they would for a
non-sentinel pidfd) and it makes everything consistent vs. if you were not
using a sentinel value.
So yes I think that's fine.
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Christian Brauner @ 2024-10-01 10:21 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Oleg Nesterov, Aleksa Sarai, Florian Weimer, Christian Brauner,
Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <8b1b376b-3c4f-409a-b8e4-8faf3efecdc8@lucifer.local>
On Mon, Sep 30, 2024 at 03:32:25PM GMT, Lorenzo Stoakes wrote:
> On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:
> > On 2024-09-30, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > > On Mon, Sep 30, 2024 at 02:34:33PM GMT, Christian Brauner wrote:
> > > > On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> > > > > On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > > > > > * Lorenzo Stoakes:
> > > > > >
> > > > > > > If you wish to utilise a pidfd interface to refer to the current process
> > > > > > > (from the point of view of userland - from the kernel point of view - the
> > > > > > > thread group leader), it is rather cumbersome, requiring something like:
> > > > > > >
> > > > > > > int pidfd = pidfd_open(getpid(), 0);
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > close(pidfd);
> > > > > > >
> > > > > > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > > > > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > > > > > simply wish to refer to the current process.
> > > > > >
> > > > > > The descriptor will refer to the current thread, not process, right?
> > > > >
> > > > > No it refers to the current process (i.e. thread group leader from kernel
> > > > > perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
> > > > >
> > > > > >
> > > > > > The distinction matters for pidfd_getfd if a process contains multiple
> > > > > > threads with different file descriptor tables, and probably for
> > > > > > pidfd_send_signal as well.
> > > > >
> > > > > You mean if you did a strange set of flags to clone()? Otherwise these are
> > > > > shared right?
> > > > >
> > > > > Again, we are explicitly looking at process not thread from userland
> > > > > perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> > > > > to implement that.
> > > >
> > > > Florian raises a good point. Currently we have:
> > > >
> > > > (1) int pidfd_tgid = pidfd_open(getpid(), 0);
> > > > (2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
> > > >
> > > > and this instructs:
> > > >
> > > > pidfd_send_signal()
> > > > pidfd_getfd()
> > > >
> > > > to do different things. For pidfd_send_signal() it's whether the
> > > > operation has thread-group scope or thread-scope for pidfd_send_signal()
> > > > and for pidfd_getfd() it determines the fdtable to use.
> > > >
> > > > The thing is that if you pass:
> > > >
> > > > pidfd_getfd(PDIFD_SELF)
> > > >
> > > > and you have:
> > > >
> > > > TGID
> > > >
> > > > T1 {
> > > > clone(CLONE_THREAD)
> > > > unshare(CLONE_FILES)
> > > > }
> > > >
> > > > T2 {
> > > > clone(CLONE_THREAD)
> > > > unshare(CLONE_FILES)
> > > > }
> > > >
> > > > You have 3 threads in the same thread-group that all have distinct file
> > > > descriptor tables from each other.
> > > >
> > > > So if T1 did:
> > > >
> > > > pidfd_getfd(PIDFD_SELF, ...)
> > > >
> > > > and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
> > > > to get the fd from its file descriptor table. IOW, its reasonable to
> > > > expect that T1 is interested in their very own resource, not someone
> > > > else's even if it is the thread-group leader.
> > > >
> > > > But what T1 will get in reality is an fd from TGID's file descriptor
> > > > table (and similar for T2).
> > > >
> > > > Iirc, yes that confusion exists already with /proc/self. But the
> > > > question is whether we should add the same confusion to the pidfd api or
> > > > whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
> > > > calling thread.
> > > >
> > > > My thinking is that if you have the reasonable suspicion that you're
> > > > multi-threaded and that you're interested in the thread-group resource
> > > > then you should be using:
> > > >
> > > > int pidfd = pidfd_open(getpid(), 0)
> > > >
> > > > and hand that thread-group leader pidfd around since you're interested
> > > > in another thread. But if you're really just interested in your own
> > > > resource then pidfd_open(getpid(), 0) makes no sense and you would want
> > > > PIDFD_SELF.
> > > >
> > > > Thoughts?
> > >
> > > I mean from my perspective, my aim is to get current->mm for
> > > process_madvise() so both work for me :) however you both raise a very good
> > > point here (sorry Florian, perhaps I was a little too dismissive as to your
> > > point, you're absolutely right).
> > >
> > > My intent was for PIDFD_SELF to simply mirror the pidfd_open(getpid(), 0)
> > > behaviour, but you and Florian make a strong case that you'd _probably_
> > > find this very confusing had you unshared in this fashion.
> > >
> > > I mean in general this confusion already exists, and is for what
> > > PIDFD_THREAD was created, but I suspect ideally if you could go back you
> > > might actually do this by default Christian + let the TGL behaviour be the
> > > optional thing?
> > >
> > > For most users this will not be an issue, but for those they'd get the same
> > > result whichever they used, but yes actually I think you're both right -
> > > PIDFD_SELF should in effect imply PIDFD_THREAD.
> >
> > Funnily enough we ran into issues with this when running Go code in runc
> > that did precisely this -- /proc/self gave you the wrong fd table in
> > very specific circumstances that were annoying to debug. For languages
> > with green-threading you can't turn off (like Go) these kinds of issues
> > pop up surprisingly often.
>
> Yeah, damn, useful insight that such things do happen in the wild.
>
> >
> > > We can adjust the pidfd_send_signal() call to infer the correct scope
> > > (actually nicely we can do that without any change there, by having
> > > __pidfd_get_pid() set f_flags accordingly).
> > >
> > > So TL;DR: I agree, I will respin with PIDFD_SELF referring to the thread.
> > >
> > > My question in return here then is - should we introduce PIDFD_SELF_PROCESS
> > > also (do advise if you feel this naming isn't quite right) - to provide
> > > thread group leader behaviour?
> >
> > Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
> > they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
> > current's tid)? In principle I guess users might use PIDFD_SELF by
> > accident but if we mirror the naming with /proc/{,thread-}self that
> > might not be that big of an issue?
>
> Lol, you know I wasn't even aware /proc/thread-self existed...
Wait until you learn that /proc/$TID thread entries exist but aren't
shown when you do ls -al /proc, only when you explicitly access them.
>
> Yeah, that actually makes sense and is consistent, though obviously the
> concern is people will reflexively use PIDFD_SELF and end up with
> potentially confusing results.
>
> I will obviously be doing a manpage patch for this so we can spell it out
> there very clearly and also in the header - so I'd actually lean towards
> doing this myself.
>
> Christian, Florian? Thoughts?
I think adding both would be potentially useful. How about:
#define PIDFD_SELF -100
#define PIDFD_THREAD_GROUP -200
This will make PIDFD_SELF mean current and PIDFD_THREAD_GROUP mean
current->pid_links[PIDTYPE_TGID]. I don't think we need to or should
mirror procfs in any way. pidfds are intended to be usable without
procfs at all.
I want to leave one comment on a bit of quirkiness in the api when we
add this. I don't consider it a big deal but it should be pointed out.
It can be useful to allow PIDFD_SELF or PIDFD_THREAD_GROUP to refer to
the calling thread even for pidfd_open() to avoid an additional getpid()
system call:
(1) pidfd_open(PIDFD_SELF, PIDFD_THREAD)
(2) pidfd_open(PIDFD_SELF, 0)
So if we allow this (Should we allow it?) then (1) is just redundant but
whathever. But (2) is at least worth discussing: Should we reject (2) on
the grounds of contradictory requests or allow it and document that it's
equivalent to pidfd_open(getpid(), PIDFD_THREAD)? It feels like the
latter would be ok.
Similar for pidfd_send_signal():
// redundant but ok:
(1) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD)
// redundant but ok:
(2) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD_GROUP)
// weird way to spell pidfd_send_signal(PIDFD_THREAD_GROUP, 0)
(3) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD_GROUP)
// weird way to spell pidfd_send_signal(PIDFD_SELF, 0)
(4) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD)
I think all of this is ok but does anyone else have a strong opinion?
^ permalink raw reply
* Re: [RFC PATCH 0/3] introduce PIDFD_SELF
From: Lorenzo Stoakes @ 2024-09-30 14:32 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, Florian Weimer, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <20240930.141721-salted.birth.growing.forges-5Z29YNO700C@cyphar.com>
On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:
> On 2024-09-30, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > On Mon, Sep 30, 2024 at 02:34:33PM GMT, Christian Brauner wrote:
> > > On Mon, Sep 30, 2024 at 11:39:49AM GMT, Lorenzo Stoakes wrote:
> > > > On Mon, Sep 30, 2024 at 12:33:18PM GMT, Florian Weimer wrote:
> > > > > * Lorenzo Stoakes:
> > > > >
> > > > > > If you wish to utilise a pidfd interface to refer to the current process
> > > > > > (from the point of view of userland - from the kernel point of view - the
> > > > > > thread group leader), it is rather cumbersome, requiring something like:
> > > > > >
> > > > > > int pidfd = pidfd_open(getpid(), 0);
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > close(pidfd);
> > > > > >
> > > > > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > > > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > > > > simply wish to refer to the current process.
> > > > >
> > > > > The descriptor will refer to the current thread, not process, right?
> > > >
> > > > No it refers to the current process (i.e. thread group leader from kernel
> > > > perspective). Unless you specify PIDFD_THREAD, this is the same if you did the above.
> > > >
> > > > >
> > > > > The distinction matters for pidfd_getfd if a process contains multiple
> > > > > threads with different file descriptor tables, and probably for
> > > > > pidfd_send_signal as well.
> > > >
> > > > You mean if you did a strange set of flags to clone()? Otherwise these are
> > > > shared right?
> > > >
> > > > Again, we are explicitly looking at process not thread from userland
> > > > perspective. A PIDFD_SELF_THREAD might be possible, but this series doesn't try
> > > > to implement that.
> > >
> > > Florian raises a good point. Currently we have:
> > >
> > > (1) int pidfd_tgid = pidfd_open(getpid(), 0);
> > > (2) int pidfd_thread = pidfd_open(getpid(), PIDFD_THREAD);
> > >
> > > and this instructs:
> > >
> > > pidfd_send_signal()
> > > pidfd_getfd()
> > >
> > > to do different things. For pidfd_send_signal() it's whether the
> > > operation has thread-group scope or thread-scope for pidfd_send_signal()
> > > and for pidfd_getfd() it determines the fdtable to use.
> > >
> > > The thing is that if you pass:
> > >
> > > pidfd_getfd(PDIFD_SELF)
> > >
> > > and you have:
> > >
> > > TGID
> > >
> > > T1 {
> > > clone(CLONE_THREAD)
> > > unshare(CLONE_FILES)
> > > }
> > >
> > > T2 {
> > > clone(CLONE_THREAD)
> > > unshare(CLONE_FILES)
> > > }
> > >
> > > You have 3 threads in the same thread-group that all have distinct file
> > > descriptor tables from each other.
> > >
> > > So if T1 did:
> > >
> > > pidfd_getfd(PIDFD_SELF, ...)
> > >
> > > and we mirror the PIDTYPE_TGID behavior then T1 will very likely expect
> > > to get the fd from its file descriptor table. IOW, its reasonable to
> > > expect that T1 is interested in their very own resource, not someone
> > > else's even if it is the thread-group leader.
> > >
> > > But what T1 will get in reality is an fd from TGID's file descriptor
> > > table (and similar for T2).
> > >
> > > Iirc, yes that confusion exists already with /proc/self. But the
> > > question is whether we should add the same confusion to the pidfd api or
> > > whether we make PIDFD_SELF actually mean PIDTYPE_PID aka the actual
> > > calling thread.
> > >
> > > My thinking is that if you have the reasonable suspicion that you're
> > > multi-threaded and that you're interested in the thread-group resource
> > > then you should be using:
> > >
> > > int pidfd = pidfd_open(getpid(), 0)
> > >
> > > and hand that thread-group leader pidfd around since you're interested
> > > in another thread. But if you're really just interested in your own
> > > resource then pidfd_open(getpid(), 0) makes no sense and you would want
> > > PIDFD_SELF.
> > >
> > > Thoughts?
> >
> > I mean from my perspective, my aim is to get current->mm for
> > process_madvise() so both work for me :) however you both raise a very good
> > point here (sorry Florian, perhaps I was a little too dismissive as to your
> > point, you're absolutely right).
> >
> > My intent was for PIDFD_SELF to simply mirror the pidfd_open(getpid(), 0)
> > behaviour, but you and Florian make a strong case that you'd _probably_
> > find this very confusing had you unshared in this fashion.
> >
> > I mean in general this confusion already exists, and is for what
> > PIDFD_THREAD was created, but I suspect ideally if you could go back you
> > might actually do this by default Christian + let the TGL behaviour be the
> > optional thing?
> >
> > For most users this will not be an issue, but for those they'd get the same
> > result whichever they used, but yes actually I think you're both right -
> > PIDFD_SELF should in effect imply PIDFD_THREAD.
>
> Funnily enough we ran into issues with this when running Go code in runc
> that did precisely this -- /proc/self gave you the wrong fd table in
> very specific circumstances that were annoying to debug. For languages
> with green-threading you can't turn off (like Go) these kinds of issues
> pop up surprisingly often.
Yeah, damn, useful insight that such things do happen in the wild.
>
> > We can adjust the pidfd_send_signal() call to infer the correct scope
> > (actually nicely we can do that without any change there, by having
> > __pidfd_get_pid() set f_flags accordingly).
> >
> > So TL;DR: I agree, I will respin with PIDFD_SELF referring to the thread.
> >
> > My question in return here then is - should we introduce PIDFD_SELF_PROCESS
> > also (do advise if you feel this naming isn't quite right) - to provide
> > thread group leader behaviour?
>
> Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe
> they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for
> current's tid)? In principle I guess users might use PIDFD_SELF by
> accident but if we mirror the naming with /proc/{,thread-}self that
> might not be that big of an issue?
Lol, you know I wasn't even aware /proc/thread-self existed...
Yeah, that actually makes sense and is consistent, though obviously the
concern is people will reflexively use PIDFD_SELF and end up with
potentially confusing results.
I will obviously be doing a manpage patch for this so we can spell it out
there very clearly and also in the header - so I'd actually lean towards
doing this myself.
Christian, Florian? Thoughts?
Thanks!
>
> Just a thought.
>
> >
> > Thanks!
> >
>
> --
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
^ 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