* [PATCH RFC 4/8] openat2: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-09-02 7:06 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: <20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
In order for userspace to be able to know what flags and fields the
kernel supports, it is currently necessary for them to do a bunch of
fairly subtle self-checks where you need to get a syscall to return a
non-EINVAL error or no-op for each flag you wish to use. If you get
-EINVAL you know the flag is unsupported, otherwise you know it is
supported.
This doesn't scale well for programs that need to check many flags, and
not all syscalls can be easily checked (how would you check for new
flags for umount2 or clone3 without side-effects?). To solve this
problem, we can take advantage of the extensible struct API used by
copy_struct_from_user() by providing a special CHECK_FIELDS flag to
extensible struct syscalls (like openat2 and clone3) which will:
1. Cause the syscall to fill the structure with every valid bit the
kernel understands. For flag arguments, this is the set of all valid
flag bits. For pointer and file descriptor arguments, this would be
all 0xFF bits (to indicate that any bits are valid). Userspace can
then easily check whether the flag they wanted is supported (by
doing a simple bitwise AND) or if a field itself is supported (by
checking if it is non-zero / all 0xFF).
2. Return a specific no-op error (-EEXTSYS_NOOP) that is not used as an
error by any other kernel code, so that userspace can be absolutely
sure that the kernel supports CHECK_FIELDS.
Rather than passing CHECK_FIELDS using the standard flags arguments for
the syscall, CHECK_FIELDS is instead the highest bit in the provided
struct size. The high bits of the size are never going to be non-zero
(we currently only allow size to be up to PAGE_SIZE, and it seems very
unlikely we will ever allow several exabyte structure arguments).
By passing the flag in the structure size, we can be sure that old
kernels will return a consistent error code (-EFAULT in openat2's case)
and that seccomp can properly filter this syscall mode (which is
guaranteed to be a no-op on all kernels -- it could even force
-EEXTSYS_NOOP to make the userspace program think the kernel doesn't
support any syscall features).
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;
}
}
Link: https://youtu.be/ggD-eb3yPVs
Link: https://lwn.net/Articles/830666/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 15 +++++++++++++++
include/uapi/asm-generic/errno.h | 3 +++
include/uapi/linux/openat2.h | 2 ++
3 files changed, 20 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index 30bfcddd505d..10bfc8d6555c 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1458,6 +1458,21 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
if (unlikely(usize < OPEN_HOW_SIZE_VER0))
return -EINVAL;
+
+ if (unlikely(usize & CHECK_FIELDS)) {
+ usize &= ~CHECK_FIELDS;
+
+ memset(&tmp, 0, sizeof(tmp));
+ tmp = (struct open_how) {
+ .flags = VALID_OPEN_FLAGS,
+ .mode = S_IALLUGO,
+ .resolve = VALID_RESOLVE_FLAGS,
+ };
+
+ err = copy_struct_to_user(how, usize, &tmp, sizeof(tmp), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..f5bfe081e73a 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -120,4 +120,7 @@
#define EHWPOISON 133 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 134 /* Extensible syscall performed no operation */
+
#endif
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..6052a504cfa4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
+#define CHECK_FIELDS (1ULL << 63)
+
/*
* Arguments for how openat2(2) should open the target path. If only @flags and
* @mode are non-zero, then openat2(2) operates very similarly to openat(2).
--
2.46.0
^ permalink raw reply related
* [PATCH RFC 5/8] clone3: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-09-02 7:06 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: <20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
As with openat2(2), this allows userspace to easily figure out what
flags and fields are supported by clone3(2). For fields which are not
flag-based, we simply set every bit in the field so that a naive
bitwise-and would show that any value of the field is valid.
For args->exit_signal, since we have an explicit bitmask for the field
defined already (CSIGNAL) we can indicate that only those bits are
supported by current kernels. If we add some extra bits to exit_signal
in the future, being able to detect them as new features would be quite
useful.
The intended way of using this interface to get feature information
looks something like the following:
static bool clone3_clear_sighand_supported;
static bool clone3_cgroup_supported;
int check_clone3_support(void)
{
int err;
struct clone_args args = {};
err = clone3(&args, CHECK_FIELDS | sizeof(args));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
clone3_clear_sighand_supported = (how.flags & CLONE_CLEAR_SIGHAND);
clone3_cgroup_supported = (how.flags & CLONE_INTO_CGROUP) &&
(how.cgroup != 0);
break;
}
}
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/fork.c | 33 +++++++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cc760491f201..1a170098a1c5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2925,6 +2925,9 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
}
#endif
+
+#define CLONE3_VALID_FLAGS (CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP)
+
noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
struct clone_args __user *uargs,
size_t usize)
@@ -2941,11 +2944,34 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER2);
BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
- if (unlikely(usize > PAGE_SIZE))
- return -E2BIG;
if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
return -EINVAL;
+ if (unlikely(usize & CHECK_FIELDS)) {
+ usize &= ~CHECK_FIELDS;
+
+ memset(&args, 0, sizeof(args));
+ args = (struct clone_args) {
+ .flags = CLONE3_VALID_FLAGS,
+ .pidfd = 0xFFFFFFFFFFFFFFFF,
+ .child_tid = 0xFFFFFFFFFFFFFFFF,
+ .parent_tid = 0xFFFFFFFFFFFFFFFF,
+ .exit_signal = (u64) CSIGNAL,
+ .stack = 0xFFFFFFFFFFFFFFFF,
+ .stack_size = 0xFFFFFFFFFFFFFFFF,
+ .tls = 0xFFFFFFFFFFFFFFFF,
+ .set_tid = 0xFFFFFFFFFFFFFFFF,
+ .set_tid_size = 0xFFFFFFFFFFFFFFFF,
+ .cgroup = 0xFFFFFFFFFFFFFFFF,
+ };
+
+ err = copy_struct_to_user(uargs, usize, &args, sizeof(args), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
+ if (unlikely(usize > PAGE_SIZE))
+ return -E2BIG;
+
err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
if (err)
return err;
@@ -3025,8 +3051,7 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
- if (kargs->flags &
- ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP))
+ if (kargs->flags & ~CLONE3_VALID_FLAGS)
return false;
/*
--
2.46.0
^ permalink raw reply related
* [PATCH RFC 6/8] selftests: openat2: add 0xFF poisoned data after misaligned struct
From: Aleksa Sarai @ 2024-09-02 7:06 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: <20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
We should also verify that poisoned data after a misaligned struct is
also handled correctly by is_zeroed_user(). This test passes with no
kernel changes needed, so is_zeroed_user() was correct already.
Fixes: b28a10aedcd4 ("selftests: add openat2(2) selftests")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/openat2_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 5790ab446527..4ca175a16ad6 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -112,9 +112,9 @@ void test_openat2_struct(void)
*
* This is effectively to check that is_zeroed_user() works.
*/
- copy = malloc(misalign + sizeof(how_ext));
+ copy = malloc(misalign*2 + sizeof(how_ext));
how_copy = copy + misalign;
- memset(copy, 0xff, misalign);
+ memset(copy, 0xff, misalign*2 + sizeof(how_ext));
memcpy(how_copy, &how_ext, sizeof(how_ext));
}
--
2.46.0
^ permalink raw reply related
* [PATCH RFC 7/8] selftests: openat2: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-09-02 7:06 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: <20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/openat2_test.c | 122 ++++++++++++++++++++++++-
1 file changed, 120 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 4ca175a16ad6..8afb41d0958a 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Author: Aleksa Sarai <cyphar@cyphar.com>
- * Copyright (C) 2018-2019 SUSE LLC.
+ * Copyright (C) 2018-2024 SUSE LLC.
*/
#define _GNU_SOURCE
@@ -29,6 +29,14 @@
#define O_LARGEFILE 0x8000
#endif
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
+#ifndef EEXTSYS_NOOP
+#define EEXTSYS_NOOP 134
+#endif
+
struct open_how_ext {
struct open_how inner;
uint32_t extra1;
@@ -45,6 +53,114 @@ struct struct_test {
int err;
};
+#define NUM_OPENAT2_CHECK_FIELDS_TESTS 1
+#define NUM_OPENAT2_CHECK_FIELDS_VARIATIONS 13
+
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+static void test_openat2_check_fields(void)
+{
+ int misalignments[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 11, 17, 87 };
+
+ for (int i = 0; i < ARRAY_LEN(misalignments); i++) {
+ int fd, misalign = misalignments[i];
+ bool failed = false;
+ char *fdpath = NULL;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ struct open_how_ext how_ext = {}, *how_copy = &how_ext;
+ void *copy = NULL;
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ resultfn = ksft_test_result_skip;
+ goto skip;
+ }
+
+ if (misalign) {
+ /*
+ * Explicitly misalign the structure copying it with
+ * the given (mis)alignment offset. The other data is
+ * set to zero and we verify this afterwards to make
+ * sure CHECK_FIELDS doesn't write outside the buffer.
+ */
+ copy = malloc(misalign*2 + sizeof(how_ext));
+ how_copy = copy + misalign;
+ memset(copy, 0x00, misalign*2 + sizeof(how_ext));
+ memcpy(how_copy, &how_ext, sizeof(how_ext));
+ }
+
+ fd = raw_openat2(AT_FDCWD, ".", how_copy, CHECK_FIELDS | sizeof(*how_copy));
+ if (check(&failed, (fd != -EEXTSYS_NOOP)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned wrong error code: %d (%s)",
+ fd, strerror(-fd));
+ if (fd >= 0) {
+ fdpath = fdreadlink(fd);
+ close(fd);
+ }
+
+ if (failed) {
+ ksft_print_msg("openat2(CHECK_FIELDS) unexpectedly returned ");
+ if (fdpath)
+ ksft_print_msg("%d['%s']\n", fd, fdpath);
+ else
+ ksft_print_msg("%d (%s)\n", fd, strerror(-fd));
+ }
+
+ if (check(&failed, !(how_copy->inner.flags & O_PATH)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned flags is missing O_PATH (0x%.16x): 0x%.16llx\n",
+ O_PATH, how_copy->inner.flags);
+
+ if (check(&failed, (how_copy->inner.mode != 07777)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned mode is invalid (0o%o): 0o%.4llo\n",
+ 07777, how_copy->inner.mode);
+
+ if (check(&failed, !(how_copy->inner.resolve & RESOLVE_IN_ROOT)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned resolve flags is missing RESOLVE_IN_ROOT (0x%.16x): 0x%.16llx\n",
+ RESOLVE_IN_ROOT, how_copy->inner.resolve);
+
+ /* Verify that the buffer space outside the struct wasn't written to. */
+ if (check(&failed, how_copy->extra1 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra1): 0x%x\n",
+ how_copy->extra1);
+ if (check(&failed, how_copy->extra2 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra2): 0x%x\n",
+ how_copy->extra2);
+ if (check(&failed, how_copy->extra3 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra3): 0x%x\n",
+ how_copy->extra3);
+
+ if (misalign) {
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + misalign + sizeof(how_ext) + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+skip:
+ resultfn("openat2(CHECK_FIELDS) [misalign=%d]\n", misalign);
+
+ free(copy);
+ free(fdpath);
+ fflush(stdout);
+ }
+}
+
#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
@@ -320,7 +436,8 @@ void test_openat2_flags(void)
}
}
-#define NUM_TESTS (NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
+#define NUM_TESTS (NUM_OPENAT2_CHECK_FIELDS_TESTS * NUM_OPENAT2_CHECK_FIELDS_VARIATIONS + \
+ NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
NUM_OPENAT2_FLAG_TESTS)
int main(int argc, char **argv)
@@ -328,6 +445,7 @@ int main(int argc, char **argv)
ksft_print_header();
ksft_set_plan(NUM_TESTS);
+ test_openat2_check_fields();
test_openat2_struct();
test_openat2_flags();
--
2.46.0
^ permalink raw reply related
* [PATCH RFC 8/8] selftests: clone3: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-09-02 7:06 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: <20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 2 +-
.../testing/selftests/clone3/clone3_check_fields.c | 229 +++++++++++++++++++++
3 files changed, 231 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore
index 83c0f6246055..4ec3e1ecd273 100644
--- a/tools/testing/selftests/clone3/.gitignore
+++ b/tools/testing/selftests/clone3/.gitignore
@@ -3,3 +3,4 @@ clone3
clone3_clear_sighand
clone3_set_tid
clone3_cap_checkpoint_restore
+clone3_check_fields
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
index 84832c369a2e..d310f2268066 100644
--- a/tools/testing/selftests/clone3/Makefile
+++ b/tools/testing/selftests/clone3/Makefile
@@ -3,6 +3,6 @@ CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES)
LDLIBS += -lcap
TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
- clone3_cap_checkpoint_restore
+ clone3_cap_checkpoint_restore clone3_check_fields
include ../lib.mk
diff --git a/tools/testing/selftests/clone3/clone3_check_fields.c b/tools/testing/selftests/clone3/clone3_check_fields.c
new file mode 100644
index 000000000000..78b5cbf807a6
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_check_fields.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Author: Aleksa Sarai <cyphar@cyphar.com>
+ * Copyright (C) 2024 SUSE LLC
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <inttypes.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sched.h>
+
+#include "../kselftest.h"
+#include "clone3_selftests.h"
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
+#ifndef EEXTSYS_NOOP
+#define EEXTSYS_NOOP 134
+#endif
+
+struct __clone_args_v0 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+};
+
+struct __clone_args_v1 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+};
+
+struct __clone_args_v2 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+ __aligned_u64 cgroup;
+};
+
+static int call_clone3(void *clone_args, size_t size)
+{
+ int status;
+ pid_t pid;
+
+ pid = sys_clone3(clone_args, size);
+ if (pid < 0) {
+ ksft_print_msg("%d (%s) - Failed to create new process\n",
+ errno, strerror(errno));
+ return -errno;
+ }
+
+ if (pid == 0) {
+ ksft_print_msg("I am the child, my PID is %d\n", getpid());
+ _exit(EXIT_SUCCESS);
+ }
+
+ ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
+ getpid(), pid);
+
+ if (waitpid(-1, &status, __WALL) < 0) {
+ ksft_print_msg("waitpid() returned %s\n", strerror(errno));
+ return -errno;
+ }
+ if (!WIFEXITED(status)) {
+ ksft_print_msg("Child did not exit normally, status 0x%x\n",
+ status);
+ return EXIT_FAILURE;
+ }
+ if (WEXITSTATUS(status))
+ return WEXITSTATUS(status);
+
+ return 0;
+}
+
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+static void test_clone3_check_fields(const char *test_name, size_t struct_size)
+{
+ size_t bufsize;
+ void *buffer;
+ pid_t pid;
+ bool failed = false;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ /* Allocate some bytes after clone_args to verify that the . */
+ bufsize = struct_size + 16;
+ buffer = malloc(bufsize);
+ memset(buffer, 0, bufsize);
+
+ pid = call_clone3(buffer, CHECK_FIELDS | struct_size);
+ if (check(&failed, (pid != -EEXTSYS_NOOP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) returned the wrong error code: %d (%s)\n",
+ pid, strerror(-pid));
+
+ switch (struct_size) {
+ case sizeof(struct __clone_args_v2): {
+ struct __clone_args_v2 *args = buffer;
+
+ if (check(&failed, (args->cgroup != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong cgroup field: 0x%.16llx != 0x%.16llx\n",
+ args->cgroup, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v1): {
+ struct __clone_args_v1 *args = buffer;
+
+ if (check(&failed, (args->set_tid != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->set_tid_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid_size field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid_size, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v0): {
+ struct __clone_args_v0 *args = buffer;
+
+ if (check(&failed, !(args->flags & CLONE_NEWUSER)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_NEWUSER in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_NEWUSER);
+ if (check(&failed, !(args->flags & CLONE_THREAD)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_THREAD in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_THREAD);
+ /*
+ * CLONE_INTO_CGROUP was added in v2, but it will be set even
+ * with smaller structure sizes.
+ */
+ if (check(&failed, !(args->flags & CLONE_INTO_CGROUP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_INTO_CGROUP in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_INTO_CGROUP);
+
+ if (check(&failed, (args->exit_signal != 0xFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong exit_signal field: 0x%.16llx != 0x%.16llx\n",
+ args->exit_signal, 0xFF);
+
+ if (check(&failed, (args->stack != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack field: 0x%.16llx != 0x%.16llx\n",
+ args->stack, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->stack_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack_size field: 0x%.16llx != 0x%.16llx\n",
+ args->stack_size, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->tls != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong tls field: 0x%.16llx != 0x%.16llx\n",
+ args->tls, 0xFFFFFFFFFFFFFFFF);
+
+ break;
+ }
+ default:
+ fprintf(stderr, "INVALID STRUCTURE SIZE: %d\n", struct_size);
+ abort();
+ }
+
+ /* Verify that the trailing parts of the buffer are still 0. */
+ for (size_t i = struct_size; i < bufsize; i++) {
+ char ch = ((char *)buffer)[i];
+ if (check(&failed, (ch != '\x00')))
+ ksft_print_msg("clone3(CHECK_FIELDS) touched a byte outside the size: buffer[%d] = 0x%.2x\n",
+ i, ch);
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+ resultfn("clone3(CHECK_FIELDS) with %s\n", test_name);
+ free(buffer);
+}
+
+struct check_fields_test {
+ const char *name;
+ size_t struct_size;
+};
+
+static struct check_fields_test check_fields_tests[] = {
+ {"struct v0", sizeof(struct __clone_args_v0)},
+ {"struct v1", sizeof(struct __clone_args_v1)},
+ {"struct v2", sizeof(struct __clone_args_v2)},
+};
+
+int main(void)
+{
+ ksft_print_header();
+ ksft_set_plan(ARRAY_SIZE(check_fields_tests));
+ test_clone3_supported();
+
+ for (int i = 0; i < ARRAY_SIZE(check_fields_tests); i++) {
+ struct check_fields_test *test = &check_fields_tests[i];
+ test_clone3_check_fields(test->name, test->struct_size);
+ }
+
+ ksft_finished();
+}
--
2.46.0
^ permalink raw reply related
* Re: [PATCH RFC 1/8] uaccess: add copy_struct_to_user helper
From: Arnd Bergmann @ 2024-09-02 8:55 UTC (permalink / raw)
To: Aleksa Sarai, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Benjamin Segall, Mel Gorman, Valentin Schneider, Alexander Viro,
Christian Brauner, Jan Kara, shuah
Cc: Kees Cook, Florian Weimer, Mark Rutland, linux-kernel, linux-api,
linux-fsdevel, Linux-Arch, linux-kselftest
In-Reply-To: <20240902-extensible-structs-check_fields-v1-1-545e93ede2f2@cyphar.com>
On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
> 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.
I'm not sure if EMSGSIZE is the best choice here, my feeling is that
the kernel should instead try to behave the same way as an older kernel
that did not know about the extra fields:
- if the structure has always been longer than the provided buffer,
-EMSGSIZE should likely have been returned all along. If older
kernels just provided a short buffer, changing it now is an ABI
change that would only affect intentionally broken callers, and
I think keeping the behavior unchanged is more consistent.
- if an extra flag was added along with the larger buffer size,
the old kernel would likely have rejected the new flag with -EINVAL,
so I think returning the same thing for userspace built against
the old kernel headers is more consistent.
> +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)
This feels like the kind of function that doesn't need to be inline
at all and could be moved to lib/usercopy.c instead. It should clearly
stay in the same place as copy_struct_from_user(), but we could move
that as well.
> +{
> + 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;
I guess the __builtin_object_size() check is the reason for making
it __always_inline, but that could be done in a trivial inline
wrapper around the extern function. If ksize is always expected
to be a constant for all callers, the check could even become a
compile-time check instead of a WARN_ON_ONCE() that feels wrong
here: if there is a code path where this can get triggered, there
is clearly a kernel bug, but the only way to find out is to have
a userspace caller that triggers the code path.
Again, the same code is already in copy_struct_from_user(), so
this is not something you are adding but rather something we
may want to change for both.
Arnd
^ permalink raw reply
* Re: [PATCH RFC 3/8] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Arnd Bergmann @ 2024-09-02 9:09 UTC (permalink / raw)
To: Aleksa Sarai, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Benjamin Segall, Mel Gorman, Valentin Schneider, Alexander Viro,
Christian Brauner, Jan Kara, shuah
Cc: Kees Cook, Florian Weimer, Mark Rutland, linux-kernel, linux-api,
linux-fsdevel, Linux-Arch, linux-kselftest, stable
In-Reply-To: <20240902-extensible-structs-check_fields-v1-3-545e93ede2f2@cyphar.com>
On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
> While we do currently return -EFAULT in this case, it seems prudent to
> follow the behaviour of other syscalls like clone3. It seems quite
> unlikely that anyone depends on this error code being EFAULT, but we can
> always revert this if it turns out to be an issue.
Right, it's probably a good idea to have a limit there rather than
having a busy loop with a user-provided length when the only bound is
the available virtual memory.
> if (unlikely(usize < OPEN_HOW_SIZE_VER0))
> return -EINVAL;
> + if (unlikely(usize > PAGE_SIZE))
> + return -E2BIG;
>
Is PAGE_SIZE significant here? If there is a need to enforce a limit,
I would expect this to be the same regardless of kernel configuration,
since the structure layout is also independent of the configuration.
Where is the current -EFAULT for users passing more than a page?
I only see it for reads beyond the VMA, but not e.g. when checking
terabytes of zero pages from an anonymous mapping.
Arnd
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Florian Weimer @ 2024-09-02 12:07 UTC (permalink / raw)
To: Rich Felker
Cc: H.J. Lu, Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <20240831154101.GN32249@brightrain.aerifal.cx>
* Rich Felker:
> This is ambiguously worded (does "operating system" mean kernel?) and
> does not agree with POSIX, which defines it as:
>
> Minimum stack size for a signal handler.
>
> And otherwise just specifies that sigaltstack shall fail if given a
> smaller size.
>
> The POSIX definition is also underspecified but it's clear that it
> should be possible to execute at least a do-nothing signal handler
> (like one which immediately returns and whose sole purpose is to
> induce EINTR when intalled without SA_RESTART), or even a minimal one
> that does something like storing to a global variable, with such a
> small stack. Allowing a size where even a do-nothing signal handler
> results in a memory-clobbering overflow or access fault seems
> non-conforming to me.
POSIX does not specify what happens on a stack overflow (or more
generally, if most resource limits are exceeded), so I think the
behavior is conforming on a technicality.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH RESEND v3 1/2] uapi: explain how per-syscall AT_* flags should be allocated
From: Jan Kara @ 2024-09-02 12:46 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-nfs,
linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-1-10c2c4c16708@cyphar.com>
On Wed 28-08-24 20:19:42, Aleksa Sarai wrote:
> Unfortunately, the way we have gone about adding new AT_* flags has
> been a little messy. In the beginning, all of the AT_* flags had generic
> meanings and so it made sense to share the flag bits indiscriminately.
> However, we inevitably ran into syscalls that needed their own
> syscall-specific flags. Due to the lack of a planned out policy, we
> ended up with the following situations:
>
> * Existing syscalls adding new features tended to use new AT_* bits,
> with some effort taken to try to re-use bits for flags that were so
> obviously syscall specific that they only make sense for a single
> syscall (such as the AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID triplet).
>
> Given the constraints of bitflags, this works well in practice, but
> ideally (to avoid future confusion) we would plan ahead and define a
> set of "per-syscall bits" ahead of time so that when allocating new
> bits we don't end up with a complete mish-mash of which bits are
> supposed to be per-syscall and which aren't.
>
> * New syscalls dealt with this in several ways:
>
> - Some syscalls (like renameat2(2), move_mount(2), fsopen(2), and
> fspick(2)) created their separate own flag spaces that have no
> overlap with the AT_* flags. Most of these ended up allocating
> their bits sequentually.
>
> In the case of move_mount(2) and fspick(2), several flags have
> identical meanings to AT_* flags but were allocated in their own
> flag space.
>
> This makes sense for syscalls that will never share AT_* flags, but
> for some syscalls this leads to duplication with AT_* flags in a
> way that could cause confusion (if renameat2(2) grew a
> RENAME_EMPTY_PATH it seems likely that users could mistake it for
> AT_EMPTY_PATH since it is an *at(2) syscall).
>
> - Some syscalls unfortunately ended up both creating their own flag
> space while also using bits from other flag spaces. The most
> obvious example is open_tree(2), where the standard usage ends up
> using flags from *THREE* separate flag spaces:
>
> open_tree(AT_FDCWD, "/foo", OPEN_TREE_CLONE|O_CLOEXEC|AT_RECURSIVE);
>
> (Note that O_CLOEXEC is also platform-specific, so several future
> OPEN_TREE_* bits are also made unusable in one fell swoop.)
>
> It's not entirely clear to me what the "right" choice is for new
> syscalls. Just saying that all future VFS syscalls should use AT_* flags
> doesn't seem practical. openat2(2) has RESOLVE_* flags (many of which
> don't make much sense to burn generic AT_* flags for) and move_mount(2)
> has separate AT_*-like flags for both the source and target so separate
> flags are needed anyway (though it seems possible that renameat2(2)
> could grow *_EMPTY_PATH flags at some point, and it's a bit of a shame
> they can't be reused).
>
> But at least for syscalls that _do_ choose to use AT_* flags, we should
> explicitly state the policy that 0x2ff is currently intended for
> per-syscall flags and that new flags should err on the side of
> overlapping with existing flag bits (so we can extend the scope of
> generic flags in the future if necessary).
>
> And add AT_* aliases for the RENAME_* flags to further cement that
> renameat2(2) is an *at(2) flag, just with its own per-syscall flags.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> include/uapi/linux/fcntl.h | 80 ++++++++++++++-------
> tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 83 +++++++++++++++-------
> 2 files changed, 115 insertions(+), 48 deletions(-)
>
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index e55a3314bcb0..38a6d66d9e88 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -90,37 +90,69 @@
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> +#define AT_FDCWD -100 /* Special value for dirfd used to
> + indicate openat should use the
> + current working directory. */
> +
> +
> +/* Generic flags for the *at(2) family of syscalls. */
> +
> +/* Reserved for per-syscall flags 0xff. */
> +#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic
> + links. */
> +/* Reserved for per-syscall flags 0x200 */
> +#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> +#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount
> + traversal. */
> +#define AT_EMPTY_PATH 0x1000 /* Allow empty relative
> + pathname to operate on dirfd
> + directly. */
> /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> - * unlinkat. The two functions do completely different things and therefore,
> - * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
> - * faccessat would be undefined behavior and thus treating it equivalent to
> - * AT_EACCESS is valid undefined behavior.
> + * These flags are currently statx(2)-specific, but they could be made generic
> + * in the future and so they should not be used for other per-syscall flags.
> */
> -#define AT_FDCWD -100 /* Special value used to indicate
> - openat should use the current
> - working directory. */
> -#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> +#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
> +#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
> +#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> +#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
> +
> +#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> +
> +/*
> + * Per-syscall flags for the *at(2) family of syscalls.
> + *
> + * These are flags that are so syscall-specific that a user passing these flags
> + * to the wrong syscall is so "clearly wrong" that we can safely call such
> + * usage "undefined behaviour".
> + *
> + * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
> + * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
> + * only to unlinkat. The two functions do completely different things and
> + * therefore, the flags can be allowed to overlap. For example, passing
> + * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
> + * equivalent to AT_EACCESS is valid undefined behavior.
> + *
> + * Note for implementers: When picking a new per-syscall AT_* flag, try to
> + * reuse already existing flags first. This leaves us with as many unused bits
> + * as possible, so we can use them for generic bits in the future if necessary.
> + */
> +
> +/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> +#define AT_RENAME_NOREPLACE 0x0001
> +#define AT_RENAME_EXCHANGE 0x0002
> +#define AT_RENAME_WHITEOUT 0x0004
> +
> +/* Flag for faccessat(2). */
> #define AT_EACCESS 0x200 /* Test access permitted for
> effective IDs, not real IDs. */
> +/* Flag for unlinkat(2). */
> #define AT_REMOVEDIR 0x200 /* Remove directory instead of
> unlinking file. */
> -#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> -#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
> -#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
> -
> -#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
> -#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
> -#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> -#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
> -
> -#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> +/* Flags for name_to_handle_at(2). */
> +#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
> + object identity and may not be
> + usable with open_by_handle_at(2). */
>
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> -#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
> - compare object identity and may not
> - be usable to open_by_handle_at(2) */
> #if defined(__KERNEL__)
> #define AT_GETATTR_NOSEC 0x80000000
> #endif
> diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> index c0bcc185fa48..38a6d66d9e88 100644
> --- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> @@ -16,6 +16,9 @@
>
> #define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
>
> +/* Was the file just created? */
> +#define F_CREATED_QUERY (F_LINUX_SPECIFIC_BASE + 4)
> +
> /*
> * Cancel a blocking posix lock; internal use only until we expose an
> * asynchronous lock api to userspace:
> @@ -87,37 +90,69 @@
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> +#define AT_FDCWD -100 /* Special value for dirfd used to
> + indicate openat should use the
> + current working directory. */
> +
> +
> +/* Generic flags for the *at(2) family of syscalls. */
> +
> +/* Reserved for per-syscall flags 0xff. */
> +#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic
> + links. */
> +/* Reserved for per-syscall flags 0x200 */
> +#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> +#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount
> + traversal. */
> +#define AT_EMPTY_PATH 0x1000 /* Allow empty relative
> + pathname to operate on dirfd
> + directly. */
> +/*
> + * These flags are currently statx(2)-specific, but they could be made generic
> + * in the future and so they should not be used for other per-syscall flags.
> + */
> +#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
> +#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
> +#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> +#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
> +
> +#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> +
> /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> - * unlinkat. The two functions do completely different things and therefore,
> - * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
> - * faccessat would be undefined behavior and thus treating it equivalent to
> - * AT_EACCESS is valid undefined behavior.
> + * Per-syscall flags for the *at(2) family of syscalls.
> + *
> + * These are flags that are so syscall-specific that a user passing these flags
> + * to the wrong syscall is so "clearly wrong" that we can safely call such
> + * usage "undefined behaviour".
> + *
> + * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
> + * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
> + * only to unlinkat. The two functions do completely different things and
> + * therefore, the flags can be allowed to overlap. For example, passing
> + * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
> + * equivalent to AT_EACCESS is valid undefined behavior.
> + *
> + * Note for implementers: When picking a new per-syscall AT_* flag, try to
> + * reuse already existing flags first. This leaves us with as many unused bits
> + * as possible, so we can use them for generic bits in the future if necessary.
> */
> -#define AT_FDCWD -100 /* Special value used to indicate
> - openat should use the current
> - working directory. */
> -#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> +
> +/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> +#define AT_RENAME_NOREPLACE 0x0001
> +#define AT_RENAME_EXCHANGE 0x0002
> +#define AT_RENAME_WHITEOUT 0x0004
> +
> +/* Flag for faccessat(2). */
> #define AT_EACCESS 0x200 /* Test access permitted for
> effective IDs, not real IDs. */
> +/* Flag for unlinkat(2). */
> #define AT_REMOVEDIR 0x200 /* Remove directory instead of
> unlinking file. */
> -#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> -#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
> -#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
> -
> -#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
> -#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
> -#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> -#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
> -
> -#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> +/* Flags for name_to_handle_at(2). */
> +#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
> + object identity and may not be
> + usable with open_by_handle_at(2). */
>
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> -#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
> - compare object identity and may not
> - be usable to open_by_handle_at(2) */
> #if defined(__KERNEL__)
> #define AT_GETATTR_NOSEC 0x80000000
> #endif
>
> --
> 2.46.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Rich Felker @ 2024-09-02 12:51 UTC (permalink / raw)
To: Florian Weimer
Cc: H.J. Lu, Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <87v7zetg1j.fsf@oldenburg3.str.redhat.com>
On Mon, Sep 02, 2024 at 02:07:36PM +0200, Florian Weimer wrote:
> * Rich Felker:
>
> > This is ambiguously worded (does "operating system" mean kernel?) and
> > does not agree with POSIX, which defines it as:
> >
> > Minimum stack size for a signal handler.
> >
> > And otherwise just specifies that sigaltstack shall fail if given a
> > smaller size.
> >
> > The POSIX definition is also underspecified but it's clear that it
> > should be possible to execute at least a do-nothing signal handler
> > (like one which immediately returns and whose sole purpose is to
> > induce EINTR when intalled without SA_RESTART), or even a minimal one
> > that does something like storing to a global variable, with such a
> > small stack. Allowing a size where even a do-nothing signal handler
> > results in a memory-clobbering overflow or access fault seems
> > non-conforming to me.
>
> POSIX does not specify what happens on a stack overflow (or more
> generally, if most resource limits are exceeded), so I think the
> behavior is conforming on a technicality.
It doesn't specify what happens on overflow. It does specify what
happens on non-overflow: the program executes correctly. Failure to do
that is the problem here, not failure to trap on fault.
Rich
^ permalink raw reply
* Re: [PATCH RESEND v3 2/2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Jan Kara @ 2024-09-02 12:54 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-nfs,
linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-2-10c2c4c16708@cyphar.com>
On Wed 28-08-24 20:19:43, Aleksa Sarai wrote:
> Now that we provide a unique 64-bit mount ID interface in statx(2), we
> can now provide a race-free way for name_to_handle_at(2) to provide a
> file handle and corresponding mount without needing to worry about
> racing with /proc/mountinfo parsing or having to open a file just to do
> statx(2).
>
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
>
> err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
> AT_HANDLE_MNT_ID_UNIQUE);
>
> into
>
> int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
> err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
> err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
> mntid = statxbuf.stx_mnt_id;
> close(fd);
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/fhandle.c | 29 ++++++++++++++++------
> include/linux/syscalls.h | 2 +-
> include/uapi/linux/fcntl.h | 1 +
> tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 1 +
> 4 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 6e8cea16790e..8cb665629f4a 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -16,7 +16,8 @@
>
> static long do_sys_name_to_handle(const struct path *path,
> struct file_handle __user *ufh,
> - int __user *mnt_id, int fh_flags)
> + void __user *mnt_id, bool unique_mntid,
> + int fh_flags)
> {
> long retval;
> struct file_handle f_handle;
> @@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
> } else
> retval = 0;
> /* copy the mount id */
> - if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
> - copy_to_user(ufh, handle,
> - struct_size(handle, f_handle, handle_bytes)))
> + if (unique_mntid) {
> + if (put_user(real_mount(path->mnt)->mnt_id_unique,
> + (u64 __user *) mnt_id))
> + retval = -EFAULT;
> + } else {
> + if (put_user(real_mount(path->mnt)->mnt_id,
> + (int __user *) mnt_id))
> + retval = -EFAULT;
> + }
> + /* copy the handle */
> + if (retval != -EFAULT &&
> + copy_to_user(ufh, handle,
> + struct_size(handle, f_handle, handle_bytes)))
> retval = -EFAULT;
> kfree(handle);
> return retval;
> @@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
> * @name: name that should be converted to handle.
> * @handle: resulting file handle
> * @mnt_id: mount id of the file system containing the file
> + * (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
> * @flag: flag value to indicate whether to follow symlink or not
> * and whether a decodable file handle is required.
> *
> @@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
> * value required.
> */
> SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> - struct file_handle __user *, handle, int __user *, mnt_id,
> + struct file_handle __user *, handle, void __user *, mnt_id,
> int, flag)
> {
> struct path path;
> @@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> int fh_flags;
> int err;
>
> - if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
> + if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> + AT_HANDLE_MNT_ID_UNIQUE))
> return -EINVAL;
>
> lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> @@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> lookup_flags |= LOOKUP_EMPTY;
> err = user_path_at(dfd, name, lookup_flags, &path);
> if (!err) {
> - err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
> + err = do_sys_name_to_handle(&path, handle, mnt_id,
> + flag & AT_HANDLE_MNT_ID_UNIQUE,
> + fh_flags);
> path_put(&path);
> }
> return err;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 4bcf6754738d..5758104921e6 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -870,7 +870,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
> #endif
> asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
> struct file_handle __user *handle,
> - int __user *mnt_id, int flag);
> + void __user *mnt_id, int flag);
> asmlinkage long sys_open_by_handle_at(int mountdirfd,
> struct file_handle __user *handle,
> int flags);
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 38a6d66d9e88..87e2dec79fea 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -152,6 +152,7 @@
> #define AT_HANDLE_FID 0x200 /* File handle is needed to compare
> object identity and may not be
> usable with open_by_handle_at(2). */
> +#define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
>
> #if defined(__KERNEL__)
> #define AT_GETATTR_NOSEC 0x80000000
> diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> index 38a6d66d9e88..87e2dec79fea 100644
> --- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> @@ -152,6 +152,7 @@
> #define AT_HANDLE_FID 0x200 /* File handle is needed to compare
> object identity and may not be
> usable with open_by_handle_at(2). */
> +#define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
>
> #if defined(__KERNEL__)
> #define AT_GETATTR_NOSEC 0x80000000
>
> --
> 2.46.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH RESEND v3 0/2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-09-02 14:16 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users,
Alexander Viro, Jan Kara, Chuck Lever, Jeff Layton,
Amir Goldstein, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com>
On Wed, 28 Aug 2024 20:19:41 +1000, Aleksa Sarai wrote:
> Now that we provide a unique 64-bit mount ID interface in statx(2), we
> can now provide a race-free way for name_to_handle_at(2) to provide a
> file handle and corresponding mount without needing to worry about
> racing with /proc/mountinfo parsing or having to open a file just to do
> statx(2).
>
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
>
> [...]
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/2] uapi: explain how per-syscall AT_* flags should be allocated
https://git.kernel.org/vfs/vfs/c/34cf40849654
[2/2] fhandle: expose u64 mount id to name_to_handle_at(2)
https://git.kernel.org/vfs/vfs/c/9cde4ebc6f4f
^ permalink raw reply
* Re: [PATCH RFC 1/8] uaccess: add copy_struct_to_user helper
From: Aleksa Sarai @ 2024-09-02 16:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
shuah, Kees Cook, Florian Weimer, Mark Rutland, linux-kernel,
linux-api, linux-fsdevel, Linux-Arch, linux-kselftest
In-Reply-To: <319c0da6-3d9c-4b45-a14c-07c5bbc3afb7@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]
On 2024-09-02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
> > 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.
>
> I'm not sure if EMSGSIZE is the best choice here, my feeling is that
> the kernel should instead try to behave the same way as an older kernel
> that did not know about the extra fields:
I agree this API is not ideal for syscalls because it can lead to
backward-compatibility issues, but that is how lsm_list_modules(2)
works. I suspect most syscalls will go with designs (1) or (3).
> - if the structure has always been longer than the provided buffer,
> -EMSGSIZE should likely have been returned all along. If older
> kernels just provided a short buffer, changing it now is an ABI
> change that would only affect intentionally broken callers, and
> I think keeping the behavior unchanged is more consistent.
>
> - if an extra flag was added along with the larger buffer size,
> the old kernel would likely have rejected the new flag with -EINVAL,
> so I think returning the same thing for userspace built against
> the old kernel headers is more consistent.
>
>
> > +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)
>
> This feels like the kind of function that doesn't need to be inline
> at all and could be moved to lib/usercopy.c instead. It should clearly
> stay in the same place as copy_struct_from_user(), but we could move
> that as well.
IIRC Kees suggested copy_struct_from_user() be inline when I first
included it, though I would have to dig through the old threads to find
the reasoning. __builtin_object_size() was added some time after it was
merged so that wasn't the original reason.
> > +{
> > + 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;
>
> I guess the __builtin_object_size() check is the reason for making
> it __always_inline, but that could be done in a trivial inline
> wrapper around the extern function. If ksize is always expected
> to be a constant for all callers, the check could even become a
> compile-time check instead of a WARN_ON_ONCE() that feels wrong
> here: if there is a code path where this can get triggered, there
> is clearly a kernel bug, but the only way to find out is to have
> a userspace caller that triggers the code path.
>
> Again, the same code is already in copy_struct_from_user(), so
> this is not something you are adding but rather something we
> may want to change for both.
>
> Arnd
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 3/8] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Aleksa Sarai @ 2024-09-02 16:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
shuah, Kees Cook, Florian Weimer, Mark Rutland, linux-kernel,
linux-api, linux-fsdevel, Linux-Arch, linux-kselftest, stable
In-Reply-To: <63193b87-7057-4ad0-aef2-fdb5d15138c3@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]
On 2024-09-02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
> > While we do currently return -EFAULT in this case, it seems prudent to
> > follow the behaviour of other syscalls like clone3. It seems quite
> > unlikely that anyone depends on this error code being EFAULT, but we can
> > always revert this if it turns out to be an issue.
>
> Right, it's probably a good idea to have a limit there rather than
> having a busy loop with a user-provided length when the only bound is
> the available virtual memory.
>
> > if (unlikely(usize < OPEN_HOW_SIZE_VER0))
> > return -EINVAL;
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -E2BIG;
> >
>
> Is PAGE_SIZE significant here? If there is a need to enforce a limit,
> I would expect this to be the same regardless of kernel configuration,
> since the structure layout is also independent of the configuration.
PAGE_SIZE is what clone3, perf_event_open, sched_setattr, bpf, etc all
use. The idea was that PAGE_SIZE is the absolute limit of any reasonable
extensible structure size because we are never going to have argument
structures that are larger than a page (I think this was discussed in
the original copy_struct_from_user() patchset thread in late 2019, but I
can't find the reference at the moment.)
I simply forgot to add this when I first submitted openat2, the original
intention was to just match the other syscalls.
> Where is the current -EFAULT for users passing more than a page?
> I only see it for reads beyond the VMA, but not e.g. when checking
> terabytes of zero pages from an anonymous mapping.
I meant that we in practice return -EFAULT if you pass a really large
size (because you end up running off the end of mapped memory). There is
no explicit -EFAULT for large sizes, which is exactly the problem. :P
>
> Arnd
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH xfstests v2 1/2] statx: update headers to include newer statx fields
From: Aleksa Sarai @ 2024-09-02 16:45 UTC (permalink / raw)
To: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan
Cc: Aleksa Sarai, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com>
These come from Linux v6.11-rc5.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
src/open_by_handle.c | 4 +++-
src/statx.h | 22 ++++++++++++++++++++--
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..d9c802ca9bd1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -82,12 +82,14 @@ Examples:
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
-#include <sys/stat.h>
#include <sys/types.h>
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <sys/stat.h>
+#include "statx.h"
+
#define MAXFILES 1024
struct handle {
diff --git a/src/statx.h b/src/statx.h
index 3f239d791dfe..935cb2ed415e 100644
--- a/src/statx.h
+++ b/src/statx.h
@@ -102,7 +102,7 @@ struct statx {
__u64 stx_ino; /* Inode number */
__u64 stx_size; /* File size */
__u64 stx_blocks; /* Number of 512-byte blocks allocated */
- __u64 __spare1[1];
+ __u64 stx_attributes_mask; /* Mask to show what's supported in stx_attributes */
/* 0x40 */
struct statx_timestamp stx_atime; /* Last access time */
struct statx_timestamp stx_btime; /* File creation time */
@@ -114,7 +114,18 @@ struct statx {
__u32 stx_dev_major; /* ID of device containing file [uncond] */
__u32 stx_dev_minor;
/* 0x90 */
- __u64 __spare2[14]; /* Spare space for future expansion */
+ __u64 stx_mnt_id;
+ __u32 stx_dio_mem_align; /* Memory buffer alignment for direct I/O */
+ __u32 stx_dio_offset_align; /* File offset alignment for direct I/O */
+ /* 0xa0 */
+ __u64 stx_subvol; /* Subvolume identifier */
+ __u32 stx_atomic_write_unit_min; /* Min atomic write unit in bytes */
+ __u32 stx_atomic_write_unit_max; /* Max atomic write unit in bytes */
+ /* 0xb0 */
+ __u32 stx_atomic_write_segments_max; /* Max atomic write segment count */
+ __u32 __spare1[1];
+ /* 0xb8 */
+ __u64 __spare3[9]; /* Spare space for future expansion */
/* 0x100 */
};
@@ -139,6 +150,13 @@ struct statx {
#define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
#define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
#define STATX_BTIME 0x00000800U /* Want/got stx_btime */
+#define STATX_MNT_ID 0x00001000U /* Got stx_mnt_id */
+#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
+#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
+#define STATX_SUBVOL 0x00008000U /* Want/got stx_subvol */
+#define STATX_WRITE_ATOMIC 0x00010000U /* Want/got atomic_write_* fields */
+
+#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
#define STATX_ALL 0x00000fffU /* All currently supported flags */
/*
--
2.46.0
^ permalink raw reply related
* [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-02 16:45 UTC (permalink / raw)
To: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan
Cc: Aleksa Sarai, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240902164554.928371-1-cyphar@cyphar.com>
Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests.
Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
1 file changed, 99 insertions(+), 29 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index d9c802ca9bd1..0ad591da632e 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -86,10 +86,16 @@ Examples:
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <stdint.h>
+#include <stdbool.h>
#include <sys/stat.h>
#include "statx.h"
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+# define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
+
#define MAXFILES 1024
struct handle {
@@ -120,6 +126,94 @@ void usage(void)
exit(EXIT_FAILURE);
}
+int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
+{
+ int ret;
+ int mntid_short;
+
+ static bool skip_mntid_unique;
+
+ uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+ struct statx statxbuf;
+
+ /* Get both the short and unique mount id. */
+ if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+ fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
+ return EXIT_FAILURE;
+ }
+ statx_mntid_short = statxbuf.stx_mnt_id;
+
+ if (!skip_mntid_unique) {
+ if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ /*
+ * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+ * kernel doesn't give us a unique mount ID just skip it.
+ */
+ if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
+ printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
+ else
+ statx_mntid_unique = statxbuf.stx_mnt_id;
+ }
+
+ fh->handle_bytes = bufsz;
+ ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+ if (bufsz < fh->handle_bytes) {
+ /* Query the filesystem required bufsz and the file handle */
+ if (ret != -1 || errno != EOVERFLOW) {
+ fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+ return EXIT_FAILURE;
+ }
+ ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+ }
+ if (ret < 0) {
+ fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+ return EXIT_FAILURE;
+ }
+
+ if (mntid_short != (int) statx_mntid_short) {
+ fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+ return EXIT_FAILURE;
+ }
+
+ if (!skip_mntid_unique && statx_mntid_unique != 0) {
+ struct handle dummy_fh;
+ uint64_t mntid_unique = 0;
+
+ /*
+ * Get the unique mount ID. We don't need to get another copy of the
+ * handle so store it in a dummy struct.
+ */
+ dummy_fh.fh.handle_bytes = fh->handle_bytes;
+ ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+ if (ret < 0) {
+ if (errno != EINVAL) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ /*
+ * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
+ * the check in that case.
+ */
+ printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
+ skip_mntid_unique = true;
+ } else {
+ if (mntid_unique != statx_mntid_unique) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+ return EXIT_FAILURE;
+ }
+ }
+ }
+
+ return 0;
+}
+
int main(int argc, char **argv)
{
int i, c;
@@ -132,7 +226,7 @@ int main(int argc, char **argv)
char fname2[PATH_MAX];
char *test_dir;
char *mount_dir;
- int mount_fd, mount_id;
+ int mount_fd;
char *infile = NULL, *outfile = NULL;
int in_fd = 0, out_fd = 0;
int numfiles = 1;
@@ -307,21 +401,9 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- handle[i].fh.handle_bytes = bufsz;
- ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
- if (bufsz < handle[i].fh.handle_bytes) {
- /* Query the filesystem required bufsz and the file handle */
- if (ret != -1 || errno != EOVERFLOW) {
- fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
- return EXIT_FAILURE;
- }
- ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
- }
- if (ret < 0) {
- strcat(fname, ": name_to_handle");
- perror(fname);
+ ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+ if (ret < 0)
return EXIT_FAILURE;
- }
}
if (keepopen) {
/* Open without close to keep unlinked files around */
@@ -349,21 +431,9 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- dir_handle.fh.handle_bytes = bufsz;
- ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
- if (bufsz < dir_handle.fh.handle_bytes) {
- /* Query the filesystem required bufsz and the file handle */
- if (ret != -1 || errno != EOVERFLOW) {
- fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
- return EXIT_FAILURE;
- }
- ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
- }
- if (ret < 0) {
- strcat(dname, ": name_to_handle");
- perror(dname);
+ ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+ if (ret < 0)
return EXIT_FAILURE;
- }
}
if (out_fd) {
ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
--
2.46.0
^ permalink raw reply related
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-02 17:21 UTC (permalink / raw)
To: Aleksa Sarai
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240902164554.928371-2-cyphar@cyphar.com>
On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
Looks good.
You may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
It'd be nice to get a verification that this is indeed tested on the latest
upstream and does not regress the tests that run the open_by_handle program.
Thanks,
Amir.
>
> src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 99 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..0ad591da632e 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,16 @@ Examples:
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.h>
>
> #include <sys/stat.h>
> #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
> #define MAXFILES 1024
>
> struct handle {
> @@ -120,6 +126,94 @@ void usage(void)
> exit(EXIT_FAILURE);
> }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> +{
> + int ret;
> + int mntid_short;
> +
> + static bool skip_mntid_unique;
> +
> + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> + struct statx statxbuf;
> +
> + /* Get both the short and unique mount id. */
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> + return EXIT_FAILURE;
> + }
> + statx_mntid_short = statxbuf.stx_mnt_id;
> +
> + if (!skip_mntid_unique) {
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + /*
> + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> + * kernel doesn't give us a unique mount ID just skip it.
> + */
> + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> + else
> + statx_mntid_unique = statxbuf.stx_mnt_id;
> + }
> +
> + fh->handle_bytes = bufsz;
> + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> + if (bufsz < fh->handle_bytes) {
> + /* Query the filesystem required bufsz and the file handle */
> + if (ret != -1 || errno != EOVERFLOW) {
> + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> + return EXIT_FAILURE;
> + }
> + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> + }
> + if (ret < 0) {
> + fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> + return EXIT_FAILURE;
> + }
> +
> + if (mntid_short != (int) statx_mntid_short) {
> + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> + return EXIT_FAILURE;
> + }
> +
> + if (!skip_mntid_unique && statx_mntid_unique != 0) {
> + struct handle dummy_fh;
> + uint64_t mntid_unique = 0;
> +
> + /*
> + * Get the unique mount ID. We don't need to get another copy of the
> + * handle so store it in a dummy struct.
> + */
> + dummy_fh.fh.handle_bytes = fh->handle_bytes;
> + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> + if (ret < 0) {
> + if (errno != EINVAL) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + /*
> + * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
> + * the check in that case.
> + */
> + printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> + skip_mntid_unique = true;
> + } else {
> + if (mntid_unique != statx_mntid_unique) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> + return EXIT_FAILURE;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> int main(int argc, char **argv)
> {
> int i, c;
> @@ -132,7 +226,7 @@ int main(int argc, char **argv)
> char fname2[PATH_MAX];
> char *test_dir;
> char *mount_dir;
> - int mount_fd, mount_id;
> + int mount_fd;
> char *infile = NULL, *outfile = NULL;
> int in_fd = 0, out_fd = 0;
> int numfiles = 1;
> @@ -307,21 +401,9 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - handle[i].fh.handle_bytes = bufsz;
> - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> - if (bufsz < handle[i].fh.handle_bytes) {
> - /* Query the filesystem required bufsz and the file handle */
> - if (ret != -1 || errno != EOVERFLOW) {
> - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> - return EXIT_FAILURE;
> - }
> - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> - }
> - if (ret < 0) {
> - strcat(fname, ": name_to_handle");
> - perror(fname);
> + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> + if (ret < 0)
> return EXIT_FAILURE;
> - }
> }
> if (keepopen) {
> /* Open without close to keep unlinked files around */
> @@ -349,21 +431,9 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - dir_handle.fh.handle_bytes = bufsz;
> - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> - if (bufsz < dir_handle.fh.handle_bytes) {
> - /* Query the filesystem required bufsz and the file handle */
> - if (ret != -1 || errno != EOVERFLOW) {
> - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> - return EXIT_FAILURE;
> - }
> - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> - }
> - if (ret < 0) {
> - strcat(dname, ": name_to_handle");
> - perror(dname);
> + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> + if (ret < 0)
> return EXIT_FAILURE;
> - }
> }
> if (out_fd) {
> ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> --
> 2.46.0
>
^ permalink raw reply
* Re: [PATCH RFC 3/8] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Arnd Bergmann @ 2024-09-02 19:23 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
shuah, Kees Cook, Florian Weimer, Mark Rutland, linux-kernel,
linux-api, linux-fsdevel, Linux-Arch, linux-kselftest, stable
In-Reply-To: <20240902.160305-cuddly.doc.quaint.provider-RsRaXpw78cll@cyphar.com>
On Mon, Sep 2, 2024, at 16:08, Aleksa Sarai wrote:
>> > if (unlikely(usize < OPEN_HOW_SIZE_VER0))
>> > return -EINVAL;
>> > + if (unlikely(usize > PAGE_SIZE))
>> > + return -E2BIG;
>> >
>>
>> Is PAGE_SIZE significant here? If there is a need to enforce a limit,
>> I would expect this to be the same regardless of kernel configuration,
>> since the structure layout is also independent of the configuration.
>
> PAGE_SIZE is what clone3, perf_event_open, sched_setattr, bpf, etc all
> use. The idea was that PAGE_SIZE is the absolute limit of any reasonable
> extensible structure size because we are never going to have argument
> structures that are larger than a page (I think this was discussed in
> the original copy_struct_from_user() patchset thread in late 2019, but I
> can't find the reference at the moment.)
>
> I simply forgot to add this when I first submitted openat2, the original
> intention was to just match the other syscalls.
Ok, I see. I guess it makes sense to keep this one consistent with the
other ones, but we may want to revisit this in the future and
come up with something that is independent of CONFIG_PAGE_SIZE.
>> Where is the current -EFAULT for users passing more than a page?
>> I only see it for reads beyond the VMA, but not e.g. when checking
>> terabytes of zero pages from an anonymous mapping.
>
> I meant that we in practice return -EFAULT if you pass a really large
> size (because you end up running off the end of mapped memory). There is
> no explicit -EFAULT for large sizes, which is exactly the problem. :P
Got it, thanks.
Arnd
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-03 6:41 UTC (permalink / raw)
To: Amir Goldstein
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <CAOQ4uxgS6DvsbUsEoM1Vr2wcd_7Bj=xFXMAy4z9PphTu+G6RaQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10128 bytes --]
On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > v2:
> > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
>
> Looks good.
>
> You may add:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>
> It'd be nice to get a verification that this is indeed tested on the latest
> upstream and does not regress the tests that run the open_by_handle program.
I've tested that the fallback works on mainline and correctly does the
test on patched kernels (by running open_by_handle directly) but I
haven't run the suite yet (still getting my mkosi testing setup working
to run fstests...).
> Thanks,
> Amir.
>
> >
> > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> > 1 file changed, 99 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..0ad591da632e 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,16 @@ Examples:
> > #include <errno.h>
> > #include <linux/limits.h>
> > #include <libgen.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> >
> > #include <sys/stat.h>
> > #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> > #define MAXFILES 1024
> >
> > struct handle {
> > @@ -120,6 +126,94 @@ void usage(void)
> > exit(EXIT_FAILURE);
> > }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> > +{
> > + int ret;
> > + int mntid_short;
> > +
> > + static bool skip_mntid_unique;
> > +
> > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > + struct statx statxbuf;
> > +
> > + /* Get both the short and unique mount id. */
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > + if (!skip_mntid_unique) {
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + /*
> > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > + * kernel doesn't give us a unique mount ID just skip it.
> > + */
> > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> > + else
> > + statx_mntid_unique = statxbuf.stx_mnt_id;
> > + }
> > +
> > + fh->handle_bytes = bufsz;
> > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > + if (bufsz < fh->handle_bytes) {
> > + /* Query the filesystem required bufsz and the file handle */
> > + if (ret != -1 || errno != EOVERFLOW) {
> > + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> > + return EXIT_FAILURE;
> > + }
> > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > + }
> > + if (ret < 0) {
> > + fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (mntid_short != (int) statx_mntid_short) {
> > + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (!skip_mntid_unique && statx_mntid_unique != 0) {
> > + struct handle dummy_fh;
> > + uint64_t mntid_unique = 0;
> > +
> > + /*
> > + * Get the unique mount ID. We don't need to get another copy of the
> > + * handle so store it in a dummy struct.
> > + */
> > + dummy_fh.fh.handle_bytes = fh->handle_bytes;
> > + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> > + if (ret < 0) {
> > + if (errno != EINVAL) {
> > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + /*
> > + * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
> > + * the check in that case.
> > + */
> > + printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> > + skip_mntid_unique = true;
> > + } else {
> > + if (mntid_unique != statx_mntid_unique) {
> > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> > + return EXIT_FAILURE;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int i, c;
> > @@ -132,7 +226,7 @@ int main(int argc, char **argv)
> > char fname2[PATH_MAX];
> > char *test_dir;
> > char *mount_dir;
> > - int mount_fd, mount_id;
> > + int mount_fd;
> > char *infile = NULL, *outfile = NULL;
> > int in_fd = 0, out_fd = 0;
> > int numfiles = 1;
> > @@ -307,21 +401,9 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - handle[i].fh.handle_bytes = bufsz;
> > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > - if (bufsz < handle[i].fh.handle_bytes) {
> > - /* Query the filesystem required bufsz and the file handle */
> > - if (ret != -1 || errno != EOVERFLOW) {
> > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> > - return EXIT_FAILURE;
> > - }
> > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > - }
> > - if (ret < 0) {
> > - strcat(fname, ": name_to_handle");
> > - perror(fname);
> > + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> > + if (ret < 0)
> > return EXIT_FAILURE;
> > - }
> > }
> > if (keepopen) {
> > /* Open without close to keep unlinked files around */
> > @@ -349,21 +431,9 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - dir_handle.fh.handle_bytes = bufsz;
> > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > - if (bufsz < dir_handle.fh.handle_bytes) {
> > - /* Query the filesystem required bufsz and the file handle */
> > - if (ret != -1 || errno != EOVERFLOW) {
> > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> > - return EXIT_FAILURE;
> > - }
> > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > - }
> > - if (ret < 0) {
> > - strcat(dname, ": name_to_handle");
> > - perror(dname);
> > + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> > + if (ret < 0)
> > return EXIT_FAILURE;
> > - }
> > }
> > if (out_fd) {
> > ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> > --
> > 2.46.0
> >
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH xfstests v2 1/2] statx: update headers to include newer statx fields
From: Amir Goldstein @ 2024-09-03 6:49 UTC (permalink / raw)
To: Aleksa Sarai
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240902164554.928371-1-cyphar@cyphar.com>
On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> These come from Linux v6.11-rc5.
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> src/open_by_handle.c | 4 +++-
> src/statx.h | 22 ++++++++++++++++++++--
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
This patch conflicts with commit
873e36c9 - statx.h: update to latest kernel UAPI
already in for-next branch (this is the branch to base patches on)
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 0f74ed08b1f0..d9c802ca9bd1 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -82,12 +82,14 @@ Examples:
> #include <string.h>
> #include <fcntl.h>
> #include <unistd.h>
> -#include <sys/stat.h>
> #include <sys/types.h>
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
>
> +#include <sys/stat.h>
> +#include "statx.h"
> +
So probably best to squash this one liner into the 2nd patch.
I guess Zorro can do it on commit if needed.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-03 7:54 UTC (permalink / raw)
To: Aleksa Sarai
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240902164554.928371-2-cyphar@cyphar.com>
On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
>
> src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 99 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..0ad591da632e 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,16 @@ Examples:
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.h>
>
> #include <sys/stat.h>
> #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
> #define MAXFILES 1024
>
> struct handle {
> @@ -120,6 +126,94 @@ void usage(void)
> exit(EXIT_FAILURE);
> }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> +{
> + int ret;
> + int mntid_short;
> +
> + static bool skip_mntid_unique;
> +
> + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> + struct statx statxbuf;
> +
> + /* Get both the short and unique mount id. */
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
This fails build on top of latest for-next branch with commit
873e36c9 - statx.h: update to latest kernel UAPI
It can be fixed by changing to use the private xfstests_statx()
implementation, same as in stat_test.c.
I am not sure how elegant this is, but that's the easy fix.
> + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> + return EXIT_FAILURE;
> + }
> + statx_mntid_short = statxbuf.stx_mnt_id;
> +
> + if (!skip_mntid_unique) {
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + /*
> + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> + * kernel doesn't give us a unique mount ID just skip it.
> + */
> + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
This verbose print breaks all existing "exportfs" tests which do not
expect it in the golden output.
I understand that silently ignoring the failure is not good, but I also
would like to add this test coverage to all the existing tests.
One solution is to resurrect the command line option -M from v1,
but instead of meaning "test unique mount id" let it mean
"do not allow to skip unique mount id" test.
Then you can add a new test that runs open_by_handle -M, but also
implement a helper _require_unique_mntid similar to _require_btime
which is needed for the new test to run only on new kernels.
I'm sorry for this complication, but fstest is a testsuite that runs on
disto and stable kernels as well and we need to allow test coverage
of new features along with stability of the test env.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-03 9:08 UTC (permalink / raw)
To: Aleksa Sarai
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240903.044647-some.sprint.silent.snacks-jdKnAVp7XuBZ@cyphar.com>
On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > make sure they match properly as part of the regular open_by_handle
> > > tests.
> > >
> > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > ---
> > > v2:
> > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> > Looks good.
> >
> > You may add:
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > It'd be nice to get a verification that this is indeed tested on the latest
> > upstream and does not regress the tests that run the open_by_handle program.
>
> I've tested that the fallback works on mainline and correctly does the
> test on patched kernels (by running open_by_handle directly) but I
> haven't run the suite yet (still getting my mkosi testing setup working
> to run fstests...).
I am afraid this has to be tested.
I started testing myself and found that it breaks existing tests.
Even if you make the test completely opt-in as in v1 it need to be
tested and _notrun on old kernels.
If you have a new version, I can test it until you get your fstests setup
ready, because anyway I would want to check that your test also
works with overlayfs which has some specialized exportfs tests.
Test by running ./check -overlay -g exportfs, but I can also do that for you.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-03 9:11 UTC (permalink / raw)
To: Amir Goldstein
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <CAOQ4uxi291jBJ5ycZgiicVebjkcRQjhXJRgOgvSPBV4-TOcQvA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > v2:
> > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> > 1 file changed, 99 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..0ad591da632e 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,16 @@ Examples:
> > #include <errno.h>
> > #include <linux/limits.h>
> > #include <libgen.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> >
> > #include <sys/stat.h>
> > #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> > #define MAXFILES 1024
> >
> > struct handle {
> > @@ -120,6 +126,94 @@ void usage(void)
> > exit(EXIT_FAILURE);
> > }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> > +{
> > + int ret;
> > + int mntid_short;
> > +
> > + static bool skip_mntid_unique;
> > +
> > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > + struct statx statxbuf;
> > +
> > + /* Get both the short and unique mount id. */
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
>
> This fails build on top of latest for-next branch with commit
> 873e36c9 - statx.h: update to latest kernel UAPI
>
> It can be fixed by changing to use the private xfstests_statx()
> implementation, same as in stat_test.c.
>
> I am not sure how elegant this is, but that's the easy fix.
Ah, I was using master as the base. Sorry about that...
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > + if (!skip_mntid_unique) {
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + /*
> > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > + * kernel doesn't give us a unique mount ID just skip it.
> > + */
> > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
>
> This verbose print breaks all existing "exportfs" tests which do not
> expect it in the golden output.
>
> I understand that silently ignoring the failure is not good, but I also
> would like to add this test coverage to all the existing tests.
>
> One solution is to resurrect the command line option -M from v1,
> but instead of meaning "test unique mount id" let it mean
> "do not allow to skip unique mount id" test.
>
> Then you can add a new test that runs open_by_handle -M, but also
> implement a helper _require_unique_mntid similar to _require_btime
> which is needed for the new test to run only on new kernels.
>
> I'm sorry for this complication, but fstest is a testsuite that runs on
> disto and stable kernels as well and we need to allow test coverage
> of new features along with stability of the test env.
No worries, I'll write it up. I'm not familiar with the exact
requirements of xfstests, sorry for the noise! (^_^")
>
> Thanks,
> Amir.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] Add provision to busyloop for events in ep_poll.
From: Joe Damato @ 2024-09-04 15:58 UTC (permalink / raw)
To: Naman Gulati
Cc: Alexander Viro, Christian Brauner, Jan Kara, David S. Miller,
Eric Dumazet, netdev, Stanislav Fomichev, linux-kernel, skhawaja,
Willem de Bruijn, mkarsten, linux-api
In-Reply-To: <CAMP57yUuvvE-n-Xx--GRUsHLC2n4LgaNF=uViDhggvbG=5r9zQ@mail.gmail.com>
On Tue, Sep 03, 2024 at 07:18:14PM -0700, Naman Gulati wrote:
> Thanks all for the comments and apologies for the delay in replying.
> Stanislav and Joe I’ve addressed some of the common concerns below.
>
> On Thu, Aug 29, 2024 at 3:40 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Wed, Aug 28, 2024 at 06:10:11PM +0000, Naman Gulati wrote:
> > > NAPI busypolling in ep_busy_loop loops on napi_poll and checks for new
> > > epoll events after every napi poll. Checking just for epoll events in a
> > > tight loop in the kernel context delivers latency gains to applications
> > > that are not interested in napi busypolling with epoll.
> > >
> > > This patch adds an option to loop just for new events inside
> > > ep_busy_loop, guarded by the EPIOCSPARAMS ioctl that controls epoll napi
> > > busypolling.
> >
> > This makes an API change, so I think that linux-api@vger.kernel.org
> > needs to be CC'd ?
> >
> > > A comparison with neper tcp_rr shows that busylooping for events in
> > > epoll_wait boosted throughput by ~3-7% and reduced median latency by
> > > ~10%.
> > >
> > > To demonstrate the latency and throughput improvements, a comparison was
> > > made of neper tcp_rr running with:
> > > 1. (baseline) No busylooping
> >
> > Is there NAPI-based steering to threads via SO_INCOMING_NAPI_ID in
> > this case? More details, please, on locality. If there is no
> > NAPI-based flow steering in this case, perhaps the improvements you
> > are seeing are a result of both syscall overhead avoidance and data
> > locality?
>
> The benchmarks were run with no NAPI steering.
>
> Regarding syscall overhead, I reproduced the above experiment with
> mitigations=off
> and found similar results as above. Pointing to the fact that the above
> gains are
> materialized from more than just avoiding syscall overhead.
I think the cover letter needs more benchmark cases, data points,
and documentation about precisely how the benchmark is being run and
configured. The data feels quite sparse, the explanation of the
experiment leaves too much out, and - with respect - I am not
convinced by the results posted.
> By locality are you referring to Numa locality?
CPU cache locality.
> >
> > > 2. (epoll busylooping) enabling the epoll busy looping on all epoll
> > > fd's
> >
> > This is the case you've added, event_poll_only ? It seems like in
> > this case you aren't busy looping exactly, you are essentially
> > allowing IRQ/softIRQ to drive processing and checking on wakeup that
> > events are available.
> >
> > IMHO, I'm not sure if "epoll busylooping" is an appropriate
> > description.
>
> I see your point, perhaps "spinning" or just “looping” could be a closer
> word?
I don't know how to describe the patch, to be totally honest. I
think the basis of the mechanism needs to be more thoroughly
understood. See below.
> >
> > > 3. (userspace busylooping) looping on epoll_wait in userspace
> > > with timeout=0
> >
> > Same question as Stanislav; timeout=0 should get ep_loop to transfer
> > events immediately (if there are any) and return without actually
> > invoking busy poll. So, it would seem that your ioctl change
> > shouldn't be necessary since the equivalent behavior is already
> > possible with timeout=0.
> >
> > I'd probably investigate both syscall overhead and data locality
> > before approving this patch because it seems a bit suspicious to me.
> >
> > >
> > > Stats for two machines with 100Gbps NICs running tcp_rr with 5 threads
> > > and varying flows:
> > >
> > > Type Flows Throughput Latency (μs)
> > > (B/s) P50 P90 P99 P99.9
> P99.99
> > > baseline 15 272145 57.2 71.9 91.4 100.6
> 111.6
> > > baseline 30 464952 66.8 78.8 98.1 113.4
> 122.4
> > > baseline 60 695920 80.9 118.5 143.4 161.8
> 174.6
> > > epoll busyloop 15 301751 44.7 70.6 84.3 95.4
> 106.5
> > > epoll busyloop 30 508392 58.9 76.9 96.2 109.3
> 118.5
> > > epoll busyloop 60 745731 77.4 106.2 127.5 143.1
> 155.9
> > > userspace busyloop 15 279202 55.4 73.1 85.2 98.3
> 109.6
> > > userspace busyloop 30 472440 63.7 78.2 96.5 112.2
> 120.1
> > > userspace busyloop 60 720779 77.9 113.5 134.9 152.6
> 165.7
> > >
> > > Per the above data epoll busyloop outperforms baseline and userspace
> > > busylooping in both throughput and latency. As the density of flows per
> > > thread increased, the median latency of all three epoll mechanisms
> > > converges. However epoll busylooping is better at capturing the tail
> > > latencies at high flow counts.
> > >
> > > Signed-off-by: Naman Gulati <namangulati@google.com>
> > > ---
> > > fs/eventpoll.c | 53 ++++++++++++++++++++++++++--------
> > > include/uapi/linux/eventpoll.h | 3 +-
> > > 2 files changed, 43 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > > index f53ca4f7fcedd..6cba79261817a 100644
> > > --- a/fs/eventpoll.c
> > > +++ b/fs/eventpoll.c
> > > @@ -232,7 +232,10 @@ struct eventpoll {
> > > u32 busy_poll_usecs;
> > > /* busy poll packet budget */
> > > u16 busy_poll_budget;
> > > - bool prefer_busy_poll;
> > > + /* prefer to busypoll in napi poll */
> > > + bool napi_prefer_busy_poll;
> >
> > Adding napi seems slightly redundant to me but I could be convinced either
> > way, I suppose.
>
> With the two different polling booleans in the struct, I felt it's better
> to be explicit
> about the scope of each.
>
> >
> > > + /* avoid napi poll when busy looping and poll only for events */
> > > + bool event_poll_only;
> >
> > I'm not sure about this overall; this isn't exactly what I think of
> > when I think about the word "polling" but maybe I'm being too
> > nit-picky.
>
> I'm not sure how else to categorize the operation, is there some other
> phrasing
> you'd recommend?
>
> >
> > > #endif
> > >
> > > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > @@ -430,6 +433,24 @@ static bool ep_busy_loop_end(void *p, unsigned
> long start_time)
> > > return ep_events_available(ep) ||
> busy_loop_ep_timeout(start_time, ep);
> > > }
> > >
> > > +/**
> > > + * ep_event_busy_loop - loop until events available or busy poll
> > > + * times out.
> > > + *
> > > + * @ep: Pointer to the eventpoll context.
> > > + *
> > > + * Return: true if events available, false otherwise.
> > > + */
> > > +static bool ep_event_busy_loop(struct eventpoll *ep)
> > > +{
> > > + unsigned long start_time = busy_loop_current_time();
> > > +
> > > + while (!ep_busy_loop_end(ep, start_time))
> > > + cond_resched();
> > > +
> > > + return ep_events_available(ep);
> > > +}
> > > +
> > > /*
> > > * Busy poll if globally on and supporting sockets found && no events,
> > > * busy loop will return if need_resched or ep_events_available.
> > > @@ -440,23 +461,29 @@ static bool ep_busy_loop(struct eventpoll *ep,
> int nonblock)
> > > {
> > > unsigned int napi_id = READ_ONCE(ep->napi_id);
> > > u16 budget = READ_ONCE(ep->busy_poll_budget);
> > > - bool prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
> > > + bool event_poll_only = READ_ONCE(ep->event_poll_only);
> > >
> > > if (!budget)
> > > budget = BUSY_POLL_BUDGET;
> > >
> > > - if (napi_id >= MIN_NAPI_ID && ep_busy_loop_on(ep)) {
> > > + if (!ep_busy_loop_on(ep))
> > > + return false;
> > > +
> > > + if (event_poll_only) {
> > > + return ep_event_busy_loop(ep);
> > > + } else if (napi_id >= MIN_NAPI_ID) {
> > > + bool napi_prefer_busy_poll =
> READ_ONCE(ep->napi_prefer_busy_poll);
> > > +
> > > napi_busy_loop(napi_id, nonblock ? NULL :
> ep_busy_loop_end,
> > > - ep, prefer_busy_poll, budget);
> > > + ep, napi_prefer_busy_poll, budget);
> > > if (ep_events_available(ep))
> > > return true;
> > > /*
> > > - * Busy poll timed out. Drop NAPI ID for now, we can add
> > > - * it back in when we have moved a socket with a valid
> NAPI
> > > - * ID onto the ready list.
> > > - */
> > > + * Busy poll timed out. Drop NAPI ID for now, we can add
> > > + * it back in when we have moved a socket with a valid NAPI
> > > + * ID onto the ready list.
> > > + */
> > > ep->napi_id = 0;
> > > - return false;
> > > }
> > > return false;
> > > }
> > > @@ -523,13 +550,15 @@ static long ep_eventpoll_bp_ioctl(struct file
> *file, unsigned int cmd,
> > >
> > > WRITE_ONCE(ep->busy_poll_usecs,
> epoll_params.busy_poll_usecs);
> > > WRITE_ONCE(ep->busy_poll_budget,
> epoll_params.busy_poll_budget);
> > > - WRITE_ONCE(ep->prefer_busy_poll,
> epoll_params.prefer_busy_poll);
> > > + WRITE_ONCE(ep->napi_prefer_busy_poll,
> epoll_params.prefer_busy_poll);
> > > + WRITE_ONCE(ep->event_poll_only,
> epoll_params.event_poll_only);
> > > return 0;
> > > case EPIOCGPARAMS:
> > > memset(&epoll_params, 0, sizeof(epoll_params));
> > > epoll_params.busy_poll_usecs =
> READ_ONCE(ep->busy_poll_usecs);
> > > epoll_params.busy_poll_budget =
> READ_ONCE(ep->busy_poll_budget);
> > > - epoll_params.prefer_busy_poll =
> READ_ONCE(ep->prefer_busy_poll);
> > > + epoll_params.prefer_busy_poll =
> READ_ONCE(ep->napi_prefer_busy_poll);
> > > + epoll_params.event_poll_only =
> READ_ONCE(ep->event_poll_only);
> > > if (copy_to_user(uarg, &epoll_params,
> sizeof(epoll_params)))
> > > return -EFAULT;
> > > return 0;
> > > @@ -2203,7 +2232,7 @@ static int do_epoll_create(int flags)
> > > #ifdef CONFIG_NET_RX_BUSY_POLL
> > > ep->busy_poll_usecs = 0;
> > > ep->busy_poll_budget = 0;
> > > - ep->prefer_busy_poll = false;
> > > + ep->napi_prefer_busy_poll = false;
> > > #endif
> >
> > Just FYI: This is going to conflict with a patch I've sent to VFS
> > that hasn't quite made its way back to net-next just yet.
> >
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc&id=4eb76c5d9a8851735fd3ec5833ecf412e8921655
> >
> Acknowledged.
>
> > > ep->file = file;
> > > fd_install(fd, file);
> > > diff --git a/include/uapi/linux/eventpoll.h
> b/include/uapi/linux/eventpoll.h
> > > index 4f4b948ef3811..3bc0f4eed976c 100644
> > > --- a/include/uapi/linux/eventpoll.h
> > > +++ b/include/uapi/linux/eventpoll.h
> > > @@ -89,9 +89,10 @@ struct epoll_params {
> > > __u32 busy_poll_usecs;
> > > __u16 busy_poll_budget;
> > > __u8 prefer_busy_poll;
> > > + __u8 event_poll_only:1;
> > >
> > > /* pad the struct to a multiple of 64bits */
> > > - __u8 __pad;
> > > + __u8 __pad:7;
> > > };
> >
> > If the above is accepted then a similar change should make its way
> > into glibc, uclibc-ng, and musl. It might be easier to add an
> > entirely new ioctl.
>
> Adding a new ioctl seems preferable, I can look into reworking the code
> accordingly.
My advice is prior to reworking the code: figure out why the results
show an improvement because I can't really see a clear explanation.
If I understand the patch correctly (and perhaps I've gotten it
wrong) it is proposing to add a new mechanism for packet processing
that is quite strange:
Let softIRQ do the work (so there's no 'polling'), but avoid
returning to userland and call cond_resched() to defer execution
to other things until there's events to be returned.
Couldn't this be approximated with a smaller batch size (maxevents)
and a timeout of 0?
Perhaps consider comparing performance with a more real world
example (i.e. not tcp_rr) and use NAPI-steering in the base case to
eliminate cache locality as a variable? Maybe even compare against
the suspend mechanism Martin and I proposed?
- Joe
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-04 16:30 UTC (permalink / raw)
To: Amir Goldstein
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <CAOQ4uxhXa-1Xjd58p8oGd9Q4hgfDtGnae1YrmDWwQp3t5uGHeg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >
> > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > > make sure they match properly as part of the regular open_by_handle
> > > > tests.
> > > >
> > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > ---
> > > > v2:
> > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > >
> > > Looks good.
> > >
> > > You may add:
> > >
> > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > It'd be nice to get a verification that this is indeed tested on the latest
> > > upstream and does not regress the tests that run the open_by_handle program.
> >
> > I've tested that the fallback works on mainline and correctly does the
> > test on patched kernels (by running open_by_handle directly) but I
> > haven't run the suite yet (still getting my mkosi testing setup working
> > to run fstests...).
>
> I am afraid this has to be tested.
> I started testing myself and found that it breaks existing tests.
> Even if you make the test completely opt-in as in v1 it need to be
> tested and _notrun on old kernels.
>
> If you have a new version, I can test it until you get your fstests setup
> ready, because anyway I would want to check that your test also
> works with overlayfs which has some specialized exportfs tests.
> Test by running ./check -overlay -g exportfs, but I can also do that for you.
I managed to get fstests running, sorry about that...
For the v3 I have ready (which includes a new test using -M), the
following runs work in my VM:
- ./check -g exportfs
- ./check -overlay -g exportfs
Should I check anything else before sending it?
Also, when running the tests I think I may have found a bug? Using
overlayfs+xfs leads to the following error when doing ./check -overlay
if the scratch device is XFS:
./common/rc: line 299: _xfs_has_feature: command not found
not run: upper fs needs to support d_type
The fix I applied was simply:
diff --git a/common/rc b/common/rc
index 0beaf2ff1126..e6af1b16918f 100644
--- a/common/rc
+++ b/common/rc
@@ -296,6 +296,7 @@ _supports_filetype()
local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
case "$fstyp" in
xfs)
+ . common/xfs
_xfs_has_feature $dir ftype
;;
ext2|ext3|ext4)
Should I include this patch as well, or did I make a mistake somewhere?
(I could add the import to the top instead if you'd prefer that.)
Thanks.
>
> Thanks,
> Amir.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related
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