* Re: [PATCH 1/3] cgroup: unify attach permission checking
From: Christian Brauner @ 2019-12-18 23:46 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo; +Cc: Li Zefan, Johannes Weiner, cgroups
In-Reply-To: <20191218173516.7875-2-christian.brauner@ubuntu.com>
On Wed, Dec 18, 2019 at 06:35:14PM +0100, Christian Brauner wrote:
> The core codepaths to check whether a process can be attached to a
> cgroup are the same for threads and thread-group leaders. Only a small
> piece of code verifying that source and destination cgroup are in the
> same domain differentiates the thread permission checking from
> thread-group leader permission checking.
> Since cgroup_migrate_vet_dst() only matters cgroup2 - it is a noop on
> cgroup1 - we can move it out of cgroup_attach_task().
> All checks can now be consolidated into a new helper
> cgroup_attach_permissions() callable from both cgroup_procs_write() and
> cgroup_threads_write().
>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: cgroups@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> kernel/cgroup/cgroup.c | 46 +++++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 735af8f15f95..5ee06c1f7456 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2719,11 +2719,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader,
> {
> DEFINE_CGROUP_MGCTX(mgctx);
> struct task_struct *task;
> - int ret;
> -
> - ret = cgroup_migrate_vet_dst(dst_cgrp);
> - if (ret)
> - return ret;
> + int ret = 0;
>
> /* look up all src csets */
> spin_lock_irq(&css_set_lock);
> @@ -4690,6 +4686,33 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
> return 0;
> }
>
> +static inline bool cgroup_same_domain(const struct cgroup *src_cgrp,
> + const struct cgroup *dst_cgrp)
> +{
> + return src_cgrp->dom_cgrp == dst_cgrp->dom_cgrp;
> +}
> +
> +static int cgroup_attach_permissions(struct cgroup *src_cgrp,
> + struct cgroup *dst_cgrp,
> + struct super_block *sb, bool thread)
> +{
> + int ret;
> +
> + ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb);
> + if (ret)
> + return ret;
> +
> + ret = cgroup_migrate_vet_dst(dst_cgrp);
> + if (ret)
> + return ret;
> +
> + if (thread &&
> + !cgroup_same_domain(src_cgrp->dom_cgrp, dst_cgrp->dom_cgrp))
> + ret = -EOPNOTSUPP;
> +
> + return 0;
> +}
> +
> static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off)
> {
> @@ -4712,8 +4735,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
> src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root);
> spin_unlock_irq(&css_set_lock);
>
> - ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp,
> - of->file->f_path.dentry->d_sb);
> + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
> + of->file->f_path.dentry->d_sb, true);
typo: s/true/false/
^ permalink raw reply
* [PATCH 3/3] selftests/cgroup: add tests for cloning into cgroups
From: Christian Brauner @ 2019-12-18 17:35 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Christian Brauner, Roman Gushchin, Shuah Khan, cgroups,
linux-kselftest
In-Reply-To: <20191218173516.7875-1-christian.brauner@ubuntu.com>
Expand the cgroup test-suite to include tests for CLONE_INTO_CGROUP.
This adds the following tests:
- CLONE_INTO_CGROUP manages to clone a process directly into a correctly
delegated cgroup
- CLONE_INTO_CGROUP fails to clone a process into a cgroup that has been
removed after we've opened an fd to it
- CLONE_INTO_CGROUP fails to clone a process into an invalid domain
cgroup
- CLONE_INTO_CGROUP adheres to the no internal process constraint
- CLONE_INTO_CGROUP works with the freezer feature
Cc: Roman Gushchin <guro@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: cgroups@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
tools/testing/selftests/cgroup/Makefile | 6 +-
tools/testing/selftests/cgroup/cgroup_util.c | 126 ++++++++++++++++++
tools/testing/selftests/cgroup/cgroup_util.h | 4 +
tools/testing/selftests/cgroup/test_core.c | 67 ++++++++++
.../selftests/clone3/clone3_selftests.h | 19 ++-
5 files changed, 217 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
index 66aafe1f5746..967f268fde74 100644
--- a/tools/testing/selftests/cgroup/Makefile
+++ b/tools/testing/selftests/cgroup/Makefile
@@ -11,6 +11,6 @@ TEST_GEN_PROGS += test_freezer
include ../lib.mk
-$(OUTPUT)/test_memcontrol: cgroup_util.c
-$(OUTPUT)/test_core: cgroup_util.c
-$(OUTPUT)/test_freezer: cgroup_util.c
+$(OUTPUT)/test_memcontrol: cgroup_util.c ../clone3/clone3_selftests.h
+$(OUTPUT)/test_core: cgroup_util.c ../clone3/clone3_selftests.h
+$(OUTPUT)/test_freezer: cgroup_util.c ../clone3/clone3_selftests.h
diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
index 8f7131dcf1ff..8a637ca7d73a 100644
--- a/tools/testing/selftests/cgroup/cgroup_util.c
+++ b/tools/testing/selftests/cgroup/cgroup_util.c
@@ -15,6 +15,7 @@
#include <unistd.h>
#include "cgroup_util.h"
+#include "../clone3/clone3_selftests.h"
static ssize_t read_text(const char *path, char *buf, size_t max_len)
{
@@ -331,12 +332,112 @@ int cg_run(const char *cgroup,
}
}
+pid_t clone_into_cgroup(int cgroup_fd)
+{
+#ifdef CLONE_ARGS_SIZE_VER2
+ pid_t pid;
+
+ struct clone_args args = {
+ .flags = CLONE_INTO_CGROUP,
+ .exit_signal = SIGCHLD,
+ .cgroup = cgroup_fd,
+ };
+
+ pid = sys_clone3(&args, sizeof(struct clone_args));
+ /*
+ * Verify that this is a genuine test failure:
+ * ENOSYS -> clone3() not available
+ * E2BIG -> CLONE_INTO_CGROUP not available
+ */
+ if (pid < 0 && (errno == ENOSYS || errno == E2BIG))
+ goto pretend_enosys;
+
+ return pid;
+
+pretend_enosys:
+#endif
+ errno = ENOSYS;
+ return -ENOSYS;
+}
+
+int clone_reap(pid_t pid, int options)
+{
+ int ret;
+ siginfo_t info = {
+ .si_signo = 0,
+ };
+
+again:
+ ret = waitid(P_PID, pid, &info, options | __WALL | __WNOTHREAD);
+ if (ret < 0) {
+ if (errno == EINTR)
+ goto again;
+ return -1;
+ }
+
+ if (options & WEXITED) {
+ if (WIFEXITED(info.si_status))
+ return WEXITSTATUS(info.si_status);
+ }
+
+ if (options & WSTOPPED) {
+ if (WIFSTOPPED(info.si_status))
+ return WSTOPSIG(info.si_status);
+ }
+
+ if (options & WCONTINUED) {
+ if (WIFCONTINUED(info.si_status))
+ return 0;
+ }
+
+ return -1;
+}
+
+int dirfd_open_opath(const char *dir)
+{
+ return open(dir, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW | O_PATH);
+}
+
+#define close_prot_errno(fd) \
+ if (fd >= 0) { \
+ int _e_ = errno; \
+ close(fd); \
+ errno = _e_; \
+ }
+
+static int clone_into_cgroup_run_nowait(const char *cgroup,
+ int (*fn)(const char *cgroup, void *arg),
+ void *arg)
+{
+ int cgroup_fd;
+ pid_t pid;
+
+ cgroup_fd = dirfd_open_opath(cgroup);
+ if (cgroup_fd < 0)
+ return -1;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ close_prot_errno(cgroup_fd);
+ if (pid == 0)
+ exit(fn(cgroup, arg));
+
+ return pid;
+}
+
int cg_run_nowait(const char *cgroup,
int (*fn)(const char *cgroup, void *arg),
void *arg)
{
int pid;
+ pid = clone_into_cgroup_run_nowait(cgroup, fn, arg);
+ if (pid > 0)
+ return pid;
+
+ /* Genuine test failure. */
+ if (pid < 0 && errno != ENOSYS)
+ return -1;
+
pid = fork();
if (pid == 0) {
char buf[64];
@@ -450,3 +551,28 @@ int proc_read_strstr(int pid, bool thread, const char *item, const char *needle)
return strstr(buf, needle) ? 0 : -1;
}
+
+int clone_into_cgroup_run_wait(const char *cgroup)
+{
+ int cgroup_fd;
+ pid_t pid;
+
+ cgroup_fd = dirfd_open_opath(cgroup);
+ if (cgroup_fd < 0)
+ return -1;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ close_prot_errno(cgroup_fd);
+ if (pid < 0)
+ return -1;
+
+ if (pid == 0)
+ exit(EXIT_SUCCESS);
+
+ /*
+ * We don't care whether this fails. We only care whether the initial
+ * clone succeeded.
+ */
+ (void)clone_reap(pid, WEXITED);
+ return 0;
+}
diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h
index 49c54fbdb229..5a1305dd1f0b 100644
--- a/tools/testing/selftests/cgroup/cgroup_util.h
+++ b/tools/testing/selftests/cgroup/cgroup_util.h
@@ -50,3 +50,7 @@ extern int cg_wait_for_proc_count(const char *cgroup, int count);
extern int cg_killall(const char *cgroup);
extern ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size);
extern int proc_read_strstr(int pid, bool thread, const char *item, const char *needle);
+extern pid_t clone_into_cgroup(int cgroup_fd);
+extern int clone_reap(pid_t pid, int options);
+extern int clone_into_cgroup_run_wait(const char *cgroup);
+extern int dirfd_open_opath(const char *dir);
diff --git a/tools/testing/selftests/cgroup/test_core.c b/tools/testing/selftests/cgroup/test_core.c
index c5ca669feb2b..eea53a86c4b3 100644
--- a/tools/testing/selftests/cgroup/test_core.c
+++ b/tools/testing/selftests/cgroup/test_core.c
@@ -25,8 +25,11 @@
static int test_cgcore_populated(const char *root)
{
int ret = KSFT_FAIL;
+ int err;
char *cg_test_a = NULL, *cg_test_b = NULL;
char *cg_test_c = NULL, *cg_test_d = NULL;
+ int cgroup_fd = -EBADF;
+ pid_t pid;
cg_test_a = cg_name(root, "cg_test_a");
cg_test_b = cg_name(root, "cg_test_a/cg_test_b");
@@ -78,6 +81,52 @@ static int test_cgcore_populated(const char *root)
if (cg_read_strcmp(cg_test_d, "cgroup.events", "populated 0\n"))
goto cleanup;
+ /* Test that we can directly clone into a new cgroup. */
+ cgroup_fd = dirfd_open_opath(cg_test_d);
+ if (cgroup_fd < 0)
+ goto cleanup;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ if (pid < 0) {
+ if (errno == ENOSYS)
+ goto cleanup_pass;
+ goto cleanup;
+ }
+
+ if (pid == 0) {
+ if (raise(SIGSTOP))
+ exit(EXIT_FAILURE);
+ exit(EXIT_SUCCESS);
+ }
+
+ err = cg_read_strcmp(cg_test_d, "cgroup.events", "populated 1\n");
+
+ (void)clone_reap(pid, WSTOPPED);
+ (void)kill(pid, SIGCONT);
+ (void)clone_reap(pid, WEXITED);
+
+ if (err)
+ goto cleanup;
+
+ if (cg_read_strcmp(cg_test_d, "cgroup.events", "populated 0\n"))
+ goto cleanup;
+
+ /* Remove cgroup. */
+ if (cg_test_d) {
+ cg_destroy(cg_test_d);
+ free(cg_test_d);
+ cg_test_d = NULL;
+ }
+
+ pid = clone_into_cgroup(cgroup_fd);
+ if (pid < 0)
+ goto cleanup_pass;
+ if (pid == 0)
+ exit(EXIT_SUCCESS);
+ (void)clone_reap(pid, WEXITED);
+ goto cleanup;
+
+cleanup_pass:
ret = KSFT_PASS;
cleanup:
@@ -93,6 +142,8 @@ static int test_cgcore_populated(const char *root)
free(cg_test_c);
free(cg_test_b);
free(cg_test_a);
+ if (cgroup_fd >= 0)
+ close(cgroup_fd);
return ret;
}
@@ -136,6 +187,16 @@ static int test_cgcore_invalid_domain(const char *root)
if (errno != EOPNOTSUPP)
goto cleanup;
+ if (!clone_into_cgroup_run_wait(child))
+ goto cleanup;
+
+ if (errno == ENOSYS)
+ goto cleanup_pass;
+
+ if (errno != EOPNOTSUPP)
+ goto cleanup;
+
+cleanup_pass:
ret = KSFT_PASS;
cleanup:
@@ -345,6 +406,12 @@ static int test_cgcore_internal_process_constraint(const char *root)
if (!cg_enter_current(parent))
goto cleanup;
+ if (!clone_into_cgroup_run_wait(parent))
+ goto cleanup;
+
+ if (errno == ENOSYS)
+ goto cleanup;
+
ret = KSFT_PASS;
cleanup:
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index a3f2c8ad8bcc..91c1a78ddb39 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -5,12 +5,24 @@
#define _GNU_SOURCE
#include <sched.h>
+#include <linux/sched.h>
+#include <linux/types.h>
#include <stdint.h>
#include <syscall.h>
-#include <linux/types.h>
+#include <sys/wait.h>
+
+#include "../kselftest.h"
#define ptr_to_u64(ptr) ((__u64)((uintptr_t)(ptr)))
+#ifndef CLONE_INTO_CGROUP
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given the right permissions. */
+#endif
+
+#ifndef CLONE_ARGS_SIZE_VER0
+#define CLONE_ARGS_SIZE_VER0 64
+#endif
+
#ifndef __NR_clone3
#define __NR_clone3 -1
struct clone_args {
@@ -22,10 +34,13 @@ struct clone_args {
__aligned_u64 stack;
__aligned_u64 stack_size;
__aligned_u64 tls;
+#define CLONE_ARGS_SIZE_VER1 80
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
+#define CLONE_ARGS_SIZE_VER2 88
+ __aligned_u64 cgroup;
};
-#endif
+#endif /* __NR_clone3 */
static pid_t sys_clone3(struct clone_args *args, size_t size)
{
--
2.24.0
^ permalink raw reply related
* [PATCH 2/3] clone3: allow spawning processes into cgroups
From: Christian Brauner @ 2019-12-18 17:35 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Christian Brauner, Ingo Molnar, Oleg Nesterov, Johannes Weiner,
Li Zefan, Peter Zijlstra, cgroups
In-Reply-To: <20191218173516.7875-1-christian.brauner@ubuntu.com>
This adds support for creating a process in a different cgroup than its
parent. Callers can limit and account processes and threads right from
the moment they are spawned:
- A service manager can directly spawn new services into dedicated
cgroups.
- A process can be directly created in a frozen cgroup and will be
frozen as well.
- The initial accounting jitter experienced by process supervisors and
daemons is eliminated with this.
- Threaded applications or even thread implementations can choose to
create a specific cgroup layout where each thread is spawned
directly into a dedicated cgroup.
This feature is limited to the unified hierarchy. Callers need to pass
an directory file descriptor for the target cgroup. The caller can
choose to pass an O_PATH file descriptor. All usual migration
restrictions apply, i.e. there can be no processes in inner nodes. In
general, creating a process directly in a target cgroup adheres to all
migration restrictions.
Cc: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
include/linux/cgroup-defs.h | 7 +-
include/linux/cgroup.h | 25 +++-
include/linux/sched/task.h | 4 +
include/uapi/linux/sched.h | 5 +
kernel/cgroup/cgroup.c | 254 +++++++++++++++++++++++++++++++-----
kernel/cgroup/pids.c | 25 +++-
kernel/fork.c | 18 ++-
7 files changed, 287 insertions(+), 51 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 63097cb243cb..cd848c6bac4a 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -33,6 +33,7 @@ struct kernfs_ops;
struct kernfs_open_file;
struct seq_file;
struct poll_table_struct;
+struct kernel_clone_args;
#define MAX_CGROUP_TYPE_NAMELEN 32
#define MAX_CGROUP_ROOT_NAMELEN 64
@@ -628,8 +629,10 @@ struct cgroup_subsys {
void (*cancel_attach)(struct cgroup_taskset *tset);
void (*attach)(struct cgroup_taskset *tset);
void (*post_attach)(void);
- int (*can_fork)(struct task_struct *task);
- void (*cancel_fork)(struct task_struct *task);
+ int (*can_fork)(struct task_struct *parent, struct task_struct *child,
+ struct kernel_clone_args *kargs);
+ void (*cancel_fork)(struct task_struct *child,
+ struct kernel_clone_args *kargs);
void (*fork)(struct task_struct *task);
void (*exit)(struct task_struct *task);
void (*release)(struct task_struct *task);
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index d7ddebd0cdec..69b97941addb 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -70,6 +70,7 @@ struct css_task_iter {
extern struct cgroup_root cgrp_dfl_root;
extern struct css_set init_css_set;
+struct kernel_clone_args;
#define SUBSYS(_x) extern struct cgroup_subsys _x ## _cgrp_subsys;
#include <linux/cgroup_subsys.h>
@@ -121,9 +122,15 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *tsk);
void cgroup_fork(struct task_struct *p);
-extern int cgroup_can_fork(struct task_struct *p);
-extern void cgroup_cancel_fork(struct task_struct *p);
-extern void cgroup_post_fork(struct task_struct *p);
+extern int cgroup_can_fork(struct task_struct *parent,
+ struct task_struct *child,
+ struct kernel_clone_args *kargs);
+extern void cgroup_cancel_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs);
+extern void cgroup_post_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs);
+extern int cgroup_lock_fork(struct kernel_clone_args *kargs);
+extern void cgroup_unlock_fork(struct kernel_clone_args *kargs);
void cgroup_exit(struct task_struct *p);
void cgroup_release(struct task_struct *p);
void cgroup_free(struct task_struct *p);
@@ -707,9 +714,15 @@ static inline int cgroupstats_build(struct cgroupstats *stats,
struct dentry *dentry) { return -EINVAL; }
static inline void cgroup_fork(struct task_struct *p) {}
-static inline int cgroup_can_fork(struct task_struct *p) { return 0; }
-static inline void cgroup_cancel_fork(struct task_struct *p) {}
-static inline void cgroup_post_fork(struct task_struct *p) {}
+static inline int cgroup_can_fork(struct task_struct *parent,
+ struct task_struct *child,
+ struct kernel_clone_args *kargs) { return 0; }
+static inline void cgroup_cancel_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs) {}
+static inline void cgroup_post_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs) {}
+static int cgroup_lock_fork(struct kernel_clone_args *kargs) { return 0; }
+static void cgroup_unlock_fork(struct kernel_clone_args *kargs) {}
static inline void cgroup_exit(struct task_struct *p) {}
static inline void cgroup_release(struct task_struct *p) {}
static inline void cgroup_free(struct task_struct *p) {}
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index f1879884238e..38359071236a 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -13,6 +13,7 @@
struct task_struct;
struct rusage;
union thread_union;
+struct css_set;
/* All the bits taken by the old clone syscall. */
#define CLONE_LEGACY_FLAGS 0xffffffffULL
@@ -29,6 +30,9 @@ struct kernel_clone_args {
pid_t *set_tid;
/* Number of elements in *set_tid */
size_t set_tid_size;
+ int cgroup;
+ struct cgroup *cgrp;
+ struct css_set *cset;
};
/*
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 4a0217832464..08620c220f30 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -35,6 +35,7 @@
/* Flags for the clone3() syscall. */
#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and reset to SIG_DFL. */
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given the right permissions. */
#ifndef __ASSEMBLY__
/**
@@ -75,6 +76,8 @@
* @set_tid_size: This defines the size of the array referenced
* in @set_tid. This cannot be larger than the
* kernel's limit of nested PID namespaces.
+ * @cgroup: If CLONE_INTO_CGROUP is specified set this to
+ * a file descriptor for the cgroup.
*
* The structure is versioned by size and thus extensible.
* New struct members must go at the end of the struct and
@@ -91,11 +94,13 @@ struct clone_args {
__aligned_u64 tls;
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
+ __aligned_u64 cgroup;
};
#endif
#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
/*
* Scheduling policies
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 5ee06c1f7456..db3b697d6a51 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5882,21 +5882,155 @@ void cgroup_fork(struct task_struct *child)
INIT_LIST_HEAD(&child->cg_list);
}
+static struct cgroup *cgroup_get_from_file(struct file *f)
+{
+ struct cgroup_subsys_state *css;
+ struct cgroup *cgrp;
+
+ css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
+ if (IS_ERR(css))
+ return ERR_CAST(css);
+
+ cgrp = css->cgroup;
+ if (!cgroup_on_dfl(cgrp)) {
+ cgroup_put(cgrp);
+ return ERR_PTR(-EBADF);
+ }
+
+ return cgrp;
+}
+
/**
- * cgroup_can_fork - called on a new task before the process is exposed
- * @child: the task in question.
+ * cgroup_css_set_fork - find or create a css_set for a child process
+ * @parent: the parent of the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * This functions finds or creates a new css_set which the child
+ * process will be attached to in cgroup_post_fork(). By default,
+ * the child process will be given the same css_set as its parent.
+ *
+ * If CLONE_INTO_CGROUP is specified this function will try to find an
+ * existing css_set which includes the request cgorup and if not create
+ * new css_set that the child will be attached to. After this function
+ * returns when CLONE_INTO_CGROUP is used we will hold a reference to the
+ * target cgroup. This is done so we can check whether the cgroup is
+ * still alive when we retake the cgroup_mutex in cgroup_lock_fork().
+ * The reference is dropped in cgroup_post_fork().
*
- * This calls the subsystem can_fork() callbacks. If the can_fork() callback
- * returns an error, the fork aborts with that error code. This allows for
- * a cgroup subsystem to conditionally allow or deny new forks.
*/
-int cgroup_can_fork(struct task_struct *child)
+static int cgroup_css_set_fork(struct task_struct *parent,
+ struct kernel_clone_args *kargs)
+ __acquires(&cgroup_mutex) __releases(&cgroup_mutex)
+{
+ int ret;
+ struct cgroup *dst_cgrp, *src_cgrp;
+ struct css_set *cset;
+ struct super_block *sb;
+ struct file *f;
+
+ spin_lock_irq(&css_set_lock);
+ cset = task_css_set(parent);
+ get_css_set(cset);
+ spin_unlock_irq(&css_set_lock);
+
+ if (!(kargs->flags & CLONE_INTO_CGROUP)) {
+ kargs->cset = cset;
+ return 0;
+ }
+
+ f = fget_raw(kargs->cgroup);
+ if (!f) {
+ put_css_set(cset);
+ return -EBADF;
+ }
+ sb = f->f_path.dentry->d_sb;
+
+ dst_cgrp = cgroup_get_from_file(f);
+ if (IS_ERR(dst_cgrp)) {
+ put_css_set(cset);
+ fput(f);
+ return PTR_ERR(dst_cgrp);
+ }
+
+ mutex_lock(&cgroup_mutex);
+
+ spin_lock_irq(&css_set_lock);
+ src_cgrp = task_cgroup_from_root(parent, &cgrp_dfl_root);
+ spin_unlock_irq(&css_set_lock);
+
+ ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, sb,
+ !!(kargs->flags & CLONE_THREAD));
+ if (!ret)
+ kargs->cset = find_css_set(cset, dst_cgrp);
+
+ mutex_unlock(&cgroup_mutex);
+
+ put_css_set(cset);
+ fput(f);
+
+ if (!ret && !kargs->cset)
+ ret = -ENOMEM;
+
+ if (ret)
+ cgroup_put(dst_cgrp);
+ else
+ kargs->cgrp = dst_cgrp;
+
+ return ret;
+}
+
+/**
+ * cgroup_css_set_put_fork - drop references we took during fork
+ * @parent: the parent of the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * Drop references to the prepared css_set and target cgroup if
+ * CLONE_INTO_CGROUP was requested.
+ * This is only valid to call before fork()'s point of no return.
+ */
+static void cgroup_css_set_put_fork(struct kernel_clone_args *kargs)
+{
+ struct cgroup *cgrp;
+ struct css_set *cset;
+
+ if (!(kargs->flags & CLONE_INTO_CGROUP))
+ return;
+
+ cset = kargs->cset;
+ if (cset)
+ put_css_set(cset);
+ kargs->cset = NULL;
+
+ cgrp = kargs->cgrp;
+ if (cgrp)
+ cgroup_put(cgrp);
+ kargs->cgrp = NULL;
+}
+
+/**
+ * cgroup_can_fork - called on a new task before the process is exposed
+ * @parent: the parent process of @child
+ * @child: the child process of @parent
+ * @kargs: the arguments passed to create the child process
+ *
+ * This prepares a new css_set for the child process which the child will
+ * be attached to in cgroup_post_fork().
+ * This calls the subsystem can_fork() callbacks. If the cgroup_can_fork()
+ * callback returns an error, the fork aborts with that error code. This allows
+ * for a cgroup subsystem to conditionally allow or deny new forks.
+ */
+int cgroup_can_fork(struct task_struct *parent, struct task_struct *child,
+ struct kernel_clone_args *kargs)
{
struct cgroup_subsys *ss;
int i, j, ret;
+ ret = cgroup_css_set_fork(parent, kargs);
+ if (ret)
+ return ret;
+
do_each_subsys_mask(ss, i, have_canfork_callback) {
- ret = ss->can_fork(child);
+ ret = ss->can_fork(parent, child, kargs);
if (ret)
goto out_revert;
} while_each_subsys_mask();
@@ -5908,50 +6042,110 @@ int cgroup_can_fork(struct task_struct *child)
if (j >= i)
break;
if (ss->cancel_fork)
- ss->cancel_fork(child);
+ ss->cancel_fork(child, kargs);
}
+ cgroup_css_set_put_fork(kargs);
+
return ret;
}
/**
* cgroup_cancel_fork - called if a fork failed after cgroup_can_fork()
- * @child: the task in question
+ * @child: the child process of @parent
+ * @kargs: the arguments passed to create the child process
*
* This calls the cancel_fork() callbacks if a fork failed *after*
- * cgroup_can_fork() succeded.
+ * cgroup_can_fork() succeded and cleans up references we took to
+ * prepare a new css_set for the child process in cgroup_can_fork().
*/
-void cgroup_cancel_fork(struct task_struct *child)
+void cgroup_cancel_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs)
{
struct cgroup_subsys *ss;
int i;
for_each_subsys(ss, i)
if (ss->cancel_fork)
- ss->cancel_fork(child);
+ ss->cancel_fork(child, kargs);
+
+ cgroup_css_set_put_fork(kargs);
}
/**
- * cgroup_post_fork - called on a new task after adding it to the task list
- * @child: the task in question
- *
- * Adds the task to the list running through its css_set if necessary and
- * call the subsystem fork() callbacks. Has to be after the task is
- * visible on the task list in case we race with the first call to
- * cgroup_task_iter_start() - to guarantee that the new task ends up on its
- * list.
+ * cgroup_lock_fork - take cgroup mutex and verify cgroup is alive
+ * @kargs: the arguments passed to create the child process
+ *
+ * If CLONE_INTO_CGROUP was specified we take the cgroup mutex and
+ * check whether the target cgroup is still alive. If this function
+ * returns successfully we are protected against cgroup removal
+ * since rmdir acquires the cgroup mutex. cgroup_post_fork() can then
+ * safely attach the child process to its css_set which includes the
+ * new cgroup.
+ * Only call right before fork()'s point of no return.
*/
-void cgroup_post_fork(struct task_struct *child)
+int cgroup_lock_fork(struct kernel_clone_args *kargs)
+ __acquires(&cgroup_mutex)
+{
+ struct cgroup *cgrp;
+
+ if (!(kargs->flags & CLONE_INTO_CGROUP))
+ return 0;
+
+ cgrp = kargs->cgrp;
+ if (!cgrp)
+ return 0;
+
+ mutex_lock(&cgroup_mutex);
+
+ if (!cgroup_is_dead(cgrp))
+ return 0;
+
+ mutex_unlock(&cgroup_mutex);
+ return -ENODEV;
+}
+
+/**
+ * cgroup_unlock_fork - drop the cgroup mutex if we had to take it
+ * @kargs: the arguments passed to create the child process
+ *
+ * If CLONE_INTO_CGROUP was specified drop the reference
+ * we took on the target cgroup in cgroup_css_set_fork() and
+ * release the cgroup mutex.
+ */
+void cgroup_unlock_fork(struct kernel_clone_args *kargs)
+ __releases(&cgroup_mutex)
+{
+ struct cgroup *cgrp;
+
+ if (!(kargs->flags & CLONE_INTO_CGROUP))
+ return;
+
+ mutex_unlock(&cgroup_mutex);
+
+ cgrp = kargs->cgrp;
+ cgroup_put(cgrp);
+ kargs->cgrp = NULL;
+}
+
+/**
+ * cgroup_post_fork - finalize cgroup setup for the child process
+ * @child: the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * Attach the child process to its css_set calling the subsystem fork()
+ * callbacks.
+ */
+void cgroup_post_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs)
{
struct cgroup_subsys *ss;
- struct css_set *cset;
+ struct css_set *cset = kargs->cset;
int i;
spin_lock_irq(&css_set_lock);
WARN_ON_ONCE(!list_empty(&child->cg_list));
- cset = task_css_set(current); /* current is @child's parent */
- get_css_set(cset);
cset->nr_tasks++;
css_set_move_task(child, NULL, cset, false);
@@ -6170,7 +6364,6 @@ EXPORT_SYMBOL_GPL(cgroup_get_from_path);
*/
struct cgroup *cgroup_get_from_fd(int fd)
{
- struct cgroup_subsys_state *css;
struct cgroup *cgrp;
struct file *f;
@@ -6178,17 +6371,8 @@ struct cgroup *cgroup_get_from_fd(int fd)
if (!f)
return ERR_PTR(-EBADF);
- css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
+ cgrp = cgroup_get_from_file(f);
fput(f);
- if (IS_ERR(css))
- return ERR_CAST(css);
-
- cgrp = css->cgroup;
- if (!cgroup_on_dfl(cgrp)) {
- cgroup_put(cgrp);
- return ERR_PTR(-EBADF);
- }
-
return cgrp;
}
EXPORT_SYMBOL_GPL(cgroup_get_from_fd);
diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
index 138059eb730d..e5955bc1fb00 100644
--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -33,6 +33,7 @@
#include <linux/atomic.h>
#include <linux/cgroup.h>
#include <linux/slab.h>
+#include <linux/sched/task.h>
#define PIDS_MAX (PID_MAX_LIMIT + 1ULL)
#define PIDS_MAX_STR "max"
@@ -214,13 +215,21 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
* task_css_check(true) in pids_can_fork() and pids_cancel_fork() relies
* on cgroup_threadgroup_change_begin() held by the copy_process().
*/
-static int pids_can_fork(struct task_struct *task)
+static int pids_can_fork(struct task_struct *parent, struct task_struct *child,
+ struct kernel_clone_args *args)
{
+ struct css_set *new_cset = NULL;
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
int err;
- css = task_css_check(current, pids_cgrp_id, true);
+ if (args)
+ new_cset = args->cset;
+
+ if (!new_cset)
+ css = task_css_check(current, pids_cgrp_id, true);
+ else
+ css = new_cset->subsys[pids_cgrp_id];
pids = css_pids(css);
err = pids_try_charge(pids, 1);
if (err) {
@@ -235,12 +244,20 @@ static int pids_can_fork(struct task_struct *task)
return err;
}
-static void pids_cancel_fork(struct task_struct *task)
+static void pids_cancel_fork(struct task_struct *task,
+ struct kernel_clone_args *args)
{
+ struct css_set *new_cset = NULL;
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
- css = task_css_check(current, pids_cgrp_id, true);
+ if (args)
+ new_cset = args->cset;
+
+ if (!new_cset)
+ css = task_css_check(current, pids_cgrp_id, true);
+ else
+ css = new_cset->subsys[pids_cgrp_id];
pids = css_pids(css);
pids_uncharge(pids, 1);
}
diff --git a/kernel/fork.c b/kernel/fork.c
index 2508a4f238a3..59868af9ac4f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2172,7 +2172,7 @@ static __latent_entropy struct task_struct *copy_process(
* between here and cgroup_post_fork() if an organisation operation is in
* progress.
*/
- retval = cgroup_can_fork(p);
+ retval = cgroup_can_fork(current, p, args);
if (retval)
goto bad_fork_cgroup_threadgroup_change_end;
@@ -2226,6 +2226,10 @@ static __latent_entropy struct task_struct *copy_process(
goto bad_fork_cancel_cgroup;
}
+ retval = cgroup_lock_fork(args);
+ if (retval)
+ goto bad_fork_cancel_cgroup;
+
/* past the last point of failure */
if (pidfile)
fd_install(pidfd, pidfile);
@@ -2279,7 +2283,8 @@ static __latent_entropy struct task_struct *copy_process(
write_unlock_irq(&tasklist_lock);
proc_fork_connector(p);
- cgroup_post_fork(p);
+ cgroup_post_fork(p, args);
+ cgroup_unlock_fork(args);
cgroup_threadgroup_change_end(current);
perf_event_fork(p);
@@ -2291,7 +2296,7 @@ static __latent_entropy struct task_struct *copy_process(
bad_fork_cancel_cgroup:
spin_unlock(¤t->sighand->siglock);
write_unlock_irq(&tasklist_lock);
- cgroup_cancel_fork(p);
+ cgroup_cancel_fork(p, args);
bad_fork_cgroup_threadgroup_change_end:
cgroup_threadgroup_change_end(current);
bad_fork_put_pidfd:
@@ -2612,6 +2617,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
!valid_signal(args.exit_signal)))
return -EINVAL;
+ if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
+ return -EINVAL;
+
*kargs = (struct kernel_clone_args){
.flags = args.flags,
.pidfd = u64_to_user_ptr(args.pidfd),
@@ -2622,6 +2630,7 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
.stack_size = args.stack_size,
.tls = args.tls,
.set_tid_size = args.set_tid_size,
+ .cgroup = args.cgroup,
};
if (args.set_tid &&
@@ -2665,7 +2674,8 @@ 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))
+ if (kargs->flags &
+ ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP))
return false;
/*
--
2.24.0
^ permalink raw reply related
* [PATCH 1/3] cgroup: unify attach permission checking
From: Christian Brauner @ 2019-12-18 17:35 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Christian Brauner, Li Zefan, Johannes Weiner, cgroups
In-Reply-To: <20191218173516.7875-1-christian.brauner@ubuntu.com>
The core codepaths to check whether a process can be attached to a
cgroup are the same for threads and thread-group leaders. Only a small
piece of code verifying that source and destination cgroup are in the
same domain differentiates the thread permission checking from
thread-group leader permission checking.
Since cgroup_migrate_vet_dst() only matters cgroup2 - it is a noop on
cgroup1 - we can move it out of cgroup_attach_task().
All checks can now be consolidated into a new helper
cgroup_attach_permissions() callable from both cgroup_procs_write() and
cgroup_threads_write().
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
kernel/cgroup/cgroup.c | 46 +++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 735af8f15f95..5ee06c1f7456 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2719,11 +2719,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader,
{
DEFINE_CGROUP_MGCTX(mgctx);
struct task_struct *task;
- int ret;
-
- ret = cgroup_migrate_vet_dst(dst_cgrp);
- if (ret)
- return ret;
+ int ret = 0;
/* look up all src csets */
spin_lock_irq(&css_set_lock);
@@ -4690,6 +4686,33 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
return 0;
}
+static inline bool cgroup_same_domain(const struct cgroup *src_cgrp,
+ const struct cgroup *dst_cgrp)
+{
+ return src_cgrp->dom_cgrp == dst_cgrp->dom_cgrp;
+}
+
+static int cgroup_attach_permissions(struct cgroup *src_cgrp,
+ struct cgroup *dst_cgrp,
+ struct super_block *sb, bool thread)
+{
+ int ret;
+
+ ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb);
+ if (ret)
+ return ret;
+
+ ret = cgroup_migrate_vet_dst(dst_cgrp);
+ if (ret)
+ return ret;
+
+ if (thread &&
+ !cgroup_same_domain(src_cgrp->dom_cgrp, dst_cgrp->dom_cgrp))
+ ret = -EOPNOTSUPP;
+
+ return 0;
+}
+
static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off)
{
@@ -4712,8 +4735,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root);
spin_unlock_irq(&css_set_lock);
- ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp,
- of->file->f_path.dentry->d_sb);
+ ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
+ of->file->f_path.dentry->d_sb, true);
if (ret)
goto out_finish;
@@ -4757,16 +4780,11 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of,
spin_unlock_irq(&css_set_lock);
/* thread migrations follow the cgroup.procs delegation rule */
- ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp,
- of->file->f_path.dentry->d_sb);
+ ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
+ of->file->f_path.dentry->d_sb, true);
if (ret)
goto out_finish;
- /* and must be contained in the same domain */
- ret = -EOPNOTSUPP;
- if (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp)
- goto out_finish;
-
ret = cgroup_attach_task(dst_cgrp, task, false);
out_finish:
--
2.24.0
^ permalink raw reply related
* [PATCH 0/3] clone3 & cgroups: allow spawning processes into cgroups
From: Christian Brauner @ 2019-12-18 17:35 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo; +Cc: Christian Brauner
Hey Tejun,
This is the promised series to enable spawning processes into a target
cgroup different from the parent's cgroup. With this cgroup migration
will be a lot easier, and accounting will be more exact. It also allows
for nice features such as creating a frozen process by spawning it into
a frozen cgroup.
The code simplifies container creation and exec logic quite a bit as
well.
I've tried to contain all core changes for this features in
kernel/cgroup/* to avoid exposing cgroup internals. This has mostly
worked.
When a new process is supposed to be spawned in a cgroup different from
the parent's then we briefly acquire the cgroup mutex right before
fork()'s point of no return and drop it once the child process has been
attached to the tasklist and to its css_set. This is done to ensure that
the cgroup isn't removed behind our back. The cgroup mutex is _only_
held in this case; the usual case, where the child is created in the
same cgroup as the parent does not acquire it since the cgroup can't be
removed.
The series already comes with proper testing. Once we've decided that
this approach is good I'll expand the test-suite even more.
(This is a pre-holiday patchset and I'm moving so I might be a little
slower responding to reviews but I wanted to send this out before the
new year.)
The branch can be found in the following locations:
[1]: kernel.org: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone_into_cgroup
[2]: github.com: https://github.com/brauner/linux/tree/clone_into_cgroup
[3]: gitlab.com: https://gitlab.com/brauner/linux/commits/clone_into_cgroup
Thanks!
Christian
Christian Brauner (3):
cgroup: unify attach permission checking
clone3: allow spawning processes into cgroups
selftests/cgroup: add tests for cloning into cgroups
include/linux/cgroup-defs.h | 7 +-
include/linux/cgroup.h | 25 +-
include/linux/sched/task.h | 4 +
include/uapi/linux/sched.h | 5 +
kernel/cgroup/cgroup.c | 300 +++++++++++++++---
kernel/cgroup/pids.c | 25 +-
kernel/fork.c | 18 +-
tools/testing/selftests/cgroup/Makefile | 6 +-
tools/testing/selftests/cgroup/cgroup_util.c | 126 ++++++++
tools/testing/selftests/cgroup/cgroup_util.h | 4 +
tools/testing/selftests/cgroup/test_core.c | 67 ++++
.../selftests/clone3/clone3_selftests.h | 19 +-
12 files changed, 536 insertions(+), 70 deletions(-)
base-commit: d1eef1c619749b2a57e514a3fa67d9a516ffa919
--
2.24.0
^ permalink raw reply
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Aleksa Sarai @ 2019-12-18 17:31 UTC (permalink / raw)
To: David Laight
Cc: 'Aleksa Sarai', Rasmus Villemoes, Alexander Viro,
Jeff Layton, J. Bruce Fields, Shuah Khan, dev@opencontainers.org,
containers@lists.linux-foundation.org, linux-api@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest@vger.kernel.org
In-Reply-To: <6630d0573b5b40da8efc58fc20ac445e@AcuMS.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 4770 bytes --]
On 2019-12-17, David Laight <David.Laight@ACULAB.COM> wrote:
> From Aleksa Sarai
> > Sent: 17 December 2019 06:47
> ...
> > > Just use u64 for all the fields.
> >
> > That is an option (and is the one that clone3 went with), but it's a bit
> > awkward because umode_t is a u16 -- and it would be a waste of 6 bytes
> > to store it as a u64. Arguably it could be extended but I personally
> > find that to be very unlikely (and lots of other syscalls would need be
> > updated).
>
> 6 bytes on interface structure will make almost no difference.
> There is no reason to save more than 16 bits anywhere else.
You have a point, and clone3's way of dealing with it does make life
easier. It also removes the need to care about explicit padding and
padding holes entirely.
> You could error values with high bits set.
Of course we'll give -EINVAL with invalid values, that's one of the
reasons openat2(2) exists after all. :P
> > I'm just going to move the padding to the end and change the error for
> > non-zero padding to -E2BIG.
>
> The padding had to be after the u16 field.
Right, I was suggesting to move the u16 field later in the struct too.
But after thinking about it some more, it doesn't help with
extensibility at all (a subsequent non-u16 extension will leave holes).
So I'm probably just going to go with either the -E2BIG patch or switch
to u64s.
> > > Use 'flags' bits to indicate whether the additional fields should be looked at.
> > > Error if a 'flags' bit requires a value that isn't passed in the structure.
> > >
> > > Then you can add an extra field and old source code recompiled with the
> > > new headers will still work - because the 'junk' value isn't looked at.
> >
> > This problem is already handled entirely by copy_struct_from_user().
> >
> > It is true that for some new fields it will be necessary to add a new
> > flag (such as passing fds -- where 0 is a valid value) but for most new
> > fields (especially pointer or flag fields) it will not be necessary
> > because the 0 value is equivalent to the old behaviour. It also allows
> > us to entirely avoid accepting junk from userspace.
>
> Only if userspace is guaranteed to memset the entire structure before
> making the call - rather than just fill in all the fields it knows
> about. If it doesn't use memset() then recompiling old code with new
> headers will pass garbage to the kernel. copy_struct_from_user()
> cannot solve that problem.
You don't need to /explicitly/ memset(), since
struct open_how how = { .flags = O_RDWR, .resolve = RESOLVE_IN_ROOT };
or even
struct open_how how = {}; /* or { 0 } if you prefer. */
will clear all of the unused fields.
But, I can add a NOTE to the man-page to clarify that this is how users
should fill their structs (or rather, that they should zero-fill them
somehow to avoid this problem).
While this might be a little annoying, I would argue that given the
openat2(2) man page explains how extensions work (in great detail) and
mentions several times that the structure may have new fields added to
it in the future -- programs which don't zero-fill the struct should be
simply seen as buggy. Note that those buggy programs *will still work*
on new kernels -- until you recompile them with new headers (because
they made an incorrect assumption about the structures they were using).
As an aside, the other downside from the uapi side is that we would
probably have to spend flag bits *that are shared with openat(2)* for
such extensions, so I'd like to avoid that as much as necessary.
> You'll never be able to guarantee that all code actually clears the
> entire structure - so at some point extending it will break recompilations
> of old code - annoying.
Only if they're explicitly doing something like
struct open_how how;
how.flags = O_RDWR;
how.resolve = RESOLVE_IN_ROOT;
memset(how.__padding, 0, sizeof(how.__padding));
As above, given the description of extensions in the man-page, I would
consider that style of struct initialisation to be eyebrow-raising at
best.
I'm sorry, but I'm simply against the idea of silently ignoring garbage
that userspace passes to the kernel -- even if it's tied to a flag. That
has proven to be an awful idea and in fact openat2(2) was written
precisely to fix this problem. To be honest, this reminds me of
(hypothetical) code like:
int flags;
flags |= O_PATH | O_CLOEXEC;
open("foo", flags); /* yay, mystery fds! */
IMHO that shouldn't have ever worked, and the only way to stop userspace
from passing garbage is to always reject it.
--
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: statx struct's stx_size pointer compatibility with uint64_t/size_t
From: Florian Weimer @ 2019-12-18 11:51 UTC (permalink / raw)
To: Dominique Martinet
Cc: linux-api, libc-alpha, Quentin Bouget, Jeff Layton, David Howells
In-Reply-To: <20191217165350.GA10729@nautica>
* Dominique Martinet:
> This makes sense to me to avoid multiplying header files for the
> different arches, so if anything I would be tempted to ask 'why is
> stdint.h uint64_t defined with just long'?
It's not a compiler-provided header. When it was added to glibc in the
90s, I don't think long long support was universal among 64-bit
compilers, and you could not just drop the type (which might have been
acceptable on 32-bit architectures).
Anyway, looking at this, it looks like we should define struct statx
with unsigned long long int in our copy instead of uint64_t. I filed
bug 25292 to track this. I guess it's just another thing to keep in
mind when adding system call support to glibc headers.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Michal Hocko @ 2019-12-18 10:17 UTC (permalink / raw)
To: John Hubbard
Cc: Michael Kerrisk (man-pages), Yang Shi, cl, cai, akpm, linux-man,
linux-api, linux-mm, linux-kernel
In-Reply-To: <51dd767a-221f-882d-c7f6-45bd0c217a67@nvidia.com>
On Tue 17-12-19 23:36:09, John Hubbard wrote:
[...]
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468fa..1bf1053f2 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> .B ENODEV
> One of the target nodes is not online.
> .TP
> -.B ENOENT
> -No pages were found that require moving.
> -All pages are either already
> -on the target node, not present, had an invalid address or could not be
> -moved because they were mapped by multiple processes.
> -.TP
> .B EPERM
> The caller specified
> .B MPOL_MF_MOVE_ALL
>
> ...But I'm not sure if we should change the implementation, instead, so
> that it *can* return ENOENT. That's the main question to resolve before
> creating any more patches, I think.
I would start by dropping any note about ENOENT first. I am not really
sure there is a reasonable usecase for it but maybe somebody comes up
with something and only then we should consider it.
Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
ideally with a kernel commit which removed the ENOENT.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: John Hubbard @ 2019-12-18 7:36 UTC (permalink / raw)
To: Michael Kerrisk (man-pages), Yang Shi, cl, mhocko, cai, akpm
Cc: linux-man, linux-api, linux-mm, linux-kernel
In-Reply-To: <092adc11-7039-9343-7067-0e0199c9dc13@gmail.com>
On 12/13/19 5:55 PM, Michael Kerrisk (man-pages) wrote:
...
>>> whoa, hold on. If I'm reading through the various error paths
>>> correctly, then this
>>> code is *never* going to return ENOENT for the whole function. It can
>>> fill in that
>>> value per-page, in the status array, but that's all. Did I get that
>>> right?
>>
>> Nice catch. Yes, you are right.
>>
>>>
>>> If so, we need to redo this part of the man page.
>>
>> Yes.
>
> So where are things at with this? Is an improved man-pages
> patch on the way, or is some other action (on the API) planned?
>
I was waiting to see if Yang was going to respond...anyway, I think
we're looking at approximately this sort of change:
diff --git a/man2/move_pages.2 b/man2/move_pages.2
index 2d96468fa..1bf1053f2 100644
--- a/man2/move_pages.2
+++ b/man2/move_pages.2
@@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
.B ENODEV
One of the target nodes is not online.
.TP
-.B ENOENT
-No pages were found that require moving.
-All pages are either already
-on the target node, not present, had an invalid address or could not be
-moved because they were mapped by multiple processes.
-.TP
.B EPERM
The caller specified
.B MPOL_MF_MOVE_ALL
...But I'm not sure if we should change the implementation, instead, so
that it *can* return ENOENT. That's the main question to resolve before
creating any more patches, I think.
In addition, Michal mentioned that the page states in the status array also
need updated documentation.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply related
* Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS
From: Richard Guy Briggs @ 2019-12-17 19:56 UTC (permalink / raw)
To: Steve Grubb
Cc: Neil Horman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, mpatel, Serge Hallyn
In-Reply-To: <2318345.msVmMTmnKu@x2>
On 2019-12-17 14:25, Steve Grubb wrote:
> On Tuesday, December 17, 2019 1:45:41 PM EST Richard Guy Briggs wrote:
> > On 2019-11-08 12:49, Paul Moore wrote:
> > > On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com>
> wrote:
> > > > On 2019-10-10 20:38, Paul Moore wrote:
> > > > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com>
> wrote:
> > > > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> > > > > > > Set an arbitrary limit on the number of audit container
> > > > > > > identifiers to
> > > > > > > limit abuse.
> > > > > > >
> > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > > ---
> > > > > > > kernel/audit.c | 8 ++++++++
> > > > > > > kernel/audit.h | 4 ++++
> > > > > > > 2 files changed, 12 insertions(+)
> > > > > > >
> > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > > index 53d13d638c63..329916534dd2 100644
> > > > > > > --- a/kernel/audit.c
> > > > > > > +++ b/kernel/audit.c
> > > > >
> > > > > ...
> > > > >
> > > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct
> > > > > > > *task, u64 contid) newcont->owner = current;
> > > > > > > refcount_set(&newcont->refcount, 1);
> > > > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> > > > > > > + audit_contid_count++;
> > > > > > > } else {
> > > > > > > rc = -ENOMEM;
> > > > > > > goto conterror;
> > > > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > > > index 162de8366b32..543f1334ba47 100644
> > > > > > > --- a/kernel/audit.h
> > > > > > > +++ b/kernel/audit.h
> > > > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64
> > > > > > > contid)
> > > > > > > return (contid & (AUDIT_CONTID_BUCKETS-1));
> > > > > > > }
> > > > > > >
> > > > > > > +extern int audit_contid_count;
> > > > > > > +
> > > > > > > +#define AUDIT_CONTID_COUNT 1 << 16
> > > > > > > +
> > > > > >
> > > > > > Just to ask the question, since it wasn't clear in the changelog,
> > > > > > what
> > > > > > abuse are you avoiding here? Ostensibly you should be able to
> > > > > > create as
> > > > > > many container ids as you have space for, and the simple creation
> > > > > > of
> > > > > > container ids doesn't seem like the resource strain I would be
> > > > > > concerned
> > > > > > about here, given that an orchestrator can still create as many
> > > > > > containers as the system will otherwise allow, which will consume
> > > > > > significantly more ram/disk/etc.
> > > > >
> > > > > I've got a similar question. Up to this point in the patchset, there
> > > > > is a potential issue of hash bucket chain lengths and traversing them
> > > > > with a spinlock held, but it seems like we shouldn't be putting an
> > > > > arbitrary limit on audit container IDs unless we have a good reason
> > > > > for it. If for some reason we do want to enforce a limit, it should
> > > > > probably be a tunable value like a sysctl, or similar.
> > > >
> > > > Can you separate and clarify the concerns here?
> > >
> > > "Why are you doing this?" is about as simple as I can pose the question.
> >
> > It was more of a concern for total system resources, primarily memory,
> > but this is self-limiting and an arbitrary concern.
> >
> > The other limit of depth of nesting has different concerns that arise
> > depending on how reporting is done.
>
> Well, there is a limit on the audit record size. So, whatever is being sent
> in the record plus the size of the timestamp deducted from
> MAX_AUDIT_MESSAGE_LENGTH (8970) is the limit. That can be divided by however
> many ID's fit in that space and you have the real limit.
This will be addressed in the v8 patch set.
> -Steve
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS
From: Steve Grubb @ 2019-12-17 19:25 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, Neil Horman, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, omosnace, dhowells, simo, Eric Paris,
Serge Hallyn, ebiederm, Dan Walsh, mpatel
In-Reply-To: <20191217184541.tagssqt4zujbanf6@madcap2.tricolour.ca>
On Tuesday, December 17, 2019 1:45:41 PM EST Richard Guy Briggs wrote:
> On 2019-11-08 12:49, Paul Moore wrote:
> > On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com>
wrote:
> > > On 2019-10-10 20:38, Paul Moore wrote:
> > > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com>
wrote:
> > > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> > > > > > Set an arbitrary limit on the number of audit container
> > > > > > identifiers to
> > > > > > limit abuse.
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > > kernel/audit.c | 8 ++++++++
> > > > > > kernel/audit.h | 4 ++++
> > > > > > 2 files changed, 12 insertions(+)
> > > > > >
> > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > index 53d13d638c63..329916534dd2 100644
> > > > > > --- a/kernel/audit.c
> > > > > > +++ b/kernel/audit.c
> > > >
> > > > ...
> > > >
> > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct
> > > > > > *task, u64 contid) newcont->owner = current;
> > > > > > refcount_set(&newcont->refcount, 1);
> > > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> > > > > > + audit_contid_count++;
> > > > > > } else {
> > > > > > rc = -ENOMEM;
> > > > > > goto conterror;
> > > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > > index 162de8366b32..543f1334ba47 100644
> > > > > > --- a/kernel/audit.h
> > > > > > +++ b/kernel/audit.h
> > > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64
> > > > > > contid)
> > > > > > return (contid & (AUDIT_CONTID_BUCKETS-1));
> > > > > > }
> > > > > >
> > > > > > +extern int audit_contid_count;
> > > > > > +
> > > > > > +#define AUDIT_CONTID_COUNT 1 << 16
> > > > > > +
> > > > >
> > > > > Just to ask the question, since it wasn't clear in the changelog,
> > > > > what
> > > > > abuse are you avoiding here? Ostensibly you should be able to
> > > > > create as
> > > > > many container ids as you have space for, and the simple creation
> > > > > of
> > > > > container ids doesn't seem like the resource strain I would be
> > > > > concerned
> > > > > about here, given that an orchestrator can still create as many
> > > > > containers as the system will otherwise allow, which will consume
> > > > > significantly more ram/disk/etc.
> > > >
> > > > I've got a similar question. Up to this point in the patchset, there
> > > > is a potential issue of hash bucket chain lengths and traversing them
> > > > with a spinlock held, but it seems like we shouldn't be putting an
> > > > arbitrary limit on audit container IDs unless we have a good reason
> > > > for it. If for some reason we do want to enforce a limit, it should
> > > > probably be a tunable value like a sysctl, or similar.
> > >
> > > Can you separate and clarify the concerns here?
> >
> > "Why are you doing this?" is about as simple as I can pose the question.
>
> It was more of a concern for total system resources, primarily memory,
> but this is self-limiting and an arbitrary concern.
>
> The other limit of depth of nesting has different concerns that arise
> depending on how reporting is done.
Well, there is a limit on the audit record size. So, whatever is being sent
in the record plus the size of the timestamp deducted from
MAX_AUDIT_MESSAGE_LENGTH (8970) is the limit. That can be divided by however
many ID's fit in that space and you have the real limit.
-Steve
-Steve
^ permalink raw reply
* Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS
From: Richard Guy Briggs @ 2019-12-17 18:45 UTC (permalink / raw)
To: Paul Moore
Cc: Neil Horman, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, netfilter-devel, sgrubb, omosnace,
dhowells, simo, Eric Paris, Serge Hallyn, ebiederm, Dan Walsh,
mpatel
In-Reply-To: <CAHC9VhTrKVQNvTPoX5xdx-TUX_ukpMv2tNFFqLa2Njs17GuQMg@mail.gmail.com>
On 2019-11-08 12:49, Paul Moore wrote:
> On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-10-10 20:38, Paul Moore wrote:
> > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:
> > > > > Set an arbitrary limit on the number of audit container identifiers to
> > > > > limit abuse.
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > kernel/audit.c | 8 ++++++++
> > > > > kernel/audit.h | 4 ++++
> > > > > 2 files changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 53d13d638c63..329916534dd2 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > >
> > > ...
> > >
> > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > > > > newcont->owner = current;
> > > > > refcount_set(&newcont->refcount, 1);
> > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> > > > > + audit_contid_count++;
> > > > > } else {
> > > > > rc = -ENOMEM;
> > > > > goto conterror;
> > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > index 162de8366b32..543f1334ba47 100644
> > > > > --- a/kernel/audit.h
> > > > > +++ b/kernel/audit.h
> > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
> > > > > return (contid & (AUDIT_CONTID_BUCKETS-1));
> > > > > }
> > > > >
> > > > > +extern int audit_contid_count;
> > > > > +
> > > > > +#define AUDIT_CONTID_COUNT 1 << 16
> > > > > +
> > > >
> > > > Just to ask the question, since it wasn't clear in the changelog, what
> > > > abuse are you avoiding here? Ostensibly you should be able to create as
> > > > many container ids as you have space for, and the simple creation of
> > > > container ids doesn't seem like the resource strain I would be concerned
> > > > about here, given that an orchestrator can still create as many
> > > > containers as the system will otherwise allow, which will consume
> > > > significantly more ram/disk/etc.
> > >
> > > I've got a similar question. Up to this point in the patchset, there
> > > is a potential issue of hash bucket chain lengths and traversing them
> > > with a spinlock held, but it seems like we shouldn't be putting an
> > > arbitrary limit on audit container IDs unless we have a good reason
> > > for it. If for some reason we do want to enforce a limit, it should
> > > probably be a tunable value like a sysctl, or similar.
> >
> > Can you separate and clarify the concerns here?
>
> "Why are you doing this?" is about as simple as I can pose the question.
It was more of a concern for total system resources, primarily memory,
but this is self-limiting and an arbitrary concern.
The other limit of depth of nesting has different concerns that arise
depending on how reporting is done.
> > I plan to move this patch to the end of the patchset and make it
> > optional, possibly adding a tuning mechanism. Like the migration from
> > /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric
> > Biederman's concern and suggested mitigation.
>
> Okay, let's just drop it. I *really* don't like this approach of
> tossing questionable stuff at the end of the patchset; I get why you
> are doing it, but I think we really need to focus on keeping this
> changeset small. If the number of ACIDs (heh) become unwieldy the
> right solution is to improve the algorithms/structures, if we can't do
> that for some reason, *then* we can fall back to a limiting knob in a
> latter release.
Ok, I've dropped it. There are mitigations in place for large numbers
of contids and it can be limited later without breaking anything.
> > As for the first issue of the bucket chain length traversal while
> > holding the list spin-lock, would you prefer to use the rcu lock to
> > traverse the list and then only hold the spin-lock when modifying the
> > list, and possibly even make the spin-lock more fine-grained per list?
>
> Until we have a better idea of how this is going to be used, I think
> it's okay for now. It's also internal to the kernel so we can change
> it at any time. My comments about the locking/structs was only to try
> and think of some reason why one might want to limit the number of
> ACIDs since neither you or Eric provided any reasoning that I could
> see.
I've switched to using an rcu read lock on the list traversal and
spin-lock on list update.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: statx struct's stx_size pointer compatibility with uint64_t/size_t
From: David Howells @ 2019-12-17 18:17 UTC (permalink / raw)
To: Dominique Martinet
Cc: dhowells, linux-api, libc-alpha, Quentin Bouget, Jeff Layton,
Florian Weimer
In-Reply-To: <20191217165350.GA10729@nautica>
Dominique Martinet <asmadeus@codewreck.org> wrote:
> Looking at this from a kernel's point of view, it looks like there
> really was a will to simplify 64-bit ints handling over all arches and
> have them all define 64-bit ints as long long a few years back.
Think printk() too. Do you use "%lu", "%Lu" or "%llu"? It's a lot easier if
__u64 is consistently unsigned long long - then it's always "%llu".
The problem with defining it as "unsigned long" on some platforms and
"unsigned long long" on others is that you're guaranteed warnings on one arch
or another.
David
^ permalink raw reply
* Re: statx struct's stx_size pointer compatibility with uint64_t/size_t
From: Dominique Martinet @ 2019-12-17 16:53 UTC (permalink / raw)
To: linux-api, libc-alpha
Cc: Quentin Bouget, Jeff Layton, David Howells, Florian Weimer
In-Reply-To: <20191217152154.GB25518@nautica>
Dominique Martinet wrote on Tue, Dec 17, 2019:
> Florian Weimer wrote on Tue, Dec 17, 2019:
> > I do not know why the kernel definition of __u64 does not follow that
> > of uint64_t in <stdint.h> (or why we even have that __u64 type), and
> > whether the kernel definition can be changed at this point. We can
> > fix this issue with preprocessor magic, but I am not entirely sure if
> > this is a good idea.
Looking at this from a kernel's point of view, it looks like there
really was a will to simplify 64-bit ints handling over all arches and
have them all define 64-bit ints as long long a few years back.
See for example linux commit 0c79a8e29 ("asm/types.h: Remove
include/asm-generic/int-l64.h")[1] that describes the removal of '64bit
ints as long' there.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf
This makes sense to me to avoid multiplying header files for the
different arches, so if anything I would be tempted to ask 'why is
stdint.h uint64_t defined with just long'? -- although from what I see,
musl and uClibc both also define it as just long so there must also be
some logic in using the smallest possible type that fits?
If someone happens to know why then perhaps the same reason could also
make sense with the kernel, I don't know. Tricky, as you pointed out...
(size_t is another issue and I agree it's best not to rely on it being
64 bits long anyway)
Thanks,
--
Dominique
^ permalink raw reply
* statx struct's stx_size pointer compatibility with uint64_t/size_t
From: Dominique Martinet @ 2019-12-17 15:21 UTC (permalink / raw)
To: linux-api, libc-alpha
Cc: Quentin Bouget, Jeff Layton, David Howells, Florian Weimer
In-Reply-To: <87r213aykv.fsf@oldenburg2.str.redhat.com>
I originally asked this on libc-help@, Florian redirected me to
linux-api@ and libc-alpha@; resending my first mail and quoting his
reply at the end.
A coworker ran into an incompatible-pointer-type compiler warning when
trying to pass &statxbuf.stx_size to a function expecting a size_t *,
which boils down to this:
-------------
#define _GNU_SOURCE
#include <stdint.h>
#include <sys/stat.h>
static void test(uint64_t *size) {
(void)size;
}
int main() {
struct statx statxbuf;
test(&statxbuf.stx_size);
return 0;
}
-------------
Giving this warning:
-------------
t.c: In function ‘main’:
t.c:12:10: warning: passing argument 1 of ‘test’ from incompatible
pointer type [-Wincompatible-pointer-types]
12 | test(&statxbuf.stx_size);
| ^~~~~~~~~~~~~~~~~~
| |
| __u64 * {aka long long unsigned int *}
t.c:5:28: note: expected ‘uint64_t *’ {aka ‘long unsigned int *’} but
argument is of type ‘__u64 *’ {aka ‘long long unsigned int *’}
5 | static void test(uint64_t *size) {
| ~~~~~~~~~~^~~~
-------------
(same happens with size_t)
The final warning is probably some standard defining long and long long
cannot be treated as compatible even on archs where it is, but it's a
bit of a shame that manipulating __u64 and uint64_t yield such errors
when passing pointers around -- the types pretty much guarantee they
have to be compatible and it is just an arbitrary choice that made one
be long and the other long long on x86_64 unless I misunderstood
something?
I'm a bit at loss of what to advise here.
We need to pass the value as a pointer because it can be updated, our
use case is that the symlink size can be wrong in /proc/x/fd/ and we
will want the correct value back in a statx struct here[1].
What would be the "recommended" way of doing this?
Any chance the field could change to be uint64_t-compatible in the
future? Not sure what that implies regarding e.g. backwards
compatibility though...
[1] https://github.com/cea-hpc/robinhood/blob/1ed74893c088d78783acd2e25e8009a483510ff7/src/backends/posix.c#L248
Florian Weimer wrote on Tue, Dec 17, 2019:
> * Dominique Martinet:
> > What would be the "recommended" way of doing this?
> > Any chance the field could change to be uint64_t-compatible in the
> > future? Not sure what that implies regarding e.g. backwards
> > compatibility though...
>
> This is a tricky subject. We already have a copy of the type with
> uint64_t fields in the installed glibc headers, but this is only used
> if the kernel definition is not available.
>
> We do not want to duplicate kernel headers too much because it causes
> problems if both glibc headers and kernel headers are included in the
> same translation unit. That in turn makes it difficult to use new
> kernel features by only updating the kernel headers.
>
> I do not know why the kernel definition of __u64 does not follow that
> of uint64_t in <stdint.h> (or why we even have that __u64 type), and
> whether the kernel definition can be changed at this point. We can
> fix this issue with preprocessor magic, but I am not entirely sure if
> this is a good idea.
Thanks,
--
Dominique
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-17 11:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Sargun Dhillon, LKML, Linux Containers, Linux API,
Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Oleg Nesterov, Andy Lutomirski, Al Viro, gpascutto,
ealvarez, Florian Weimer, jld
In-Reply-To: <CAK8P3a3G-W8s0G2-XKuDw9dRmupZSyiF6FRRAnvDt9=kMMzS8w@mail.gmail.com>
On Tue, Dec 17, 2019 at 09:54:40AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 17, 2019 at 3:50 AM Sargun Dhillon <sargun@sargun.me> wrote:
> > On Mon, Dec 16, 2019 at 5:50 PM Christian Brauner <christian.brauner@ubuntu.com> wrote:
> Finally, there is the question whether this should be an ioctl
> operation at all, or
> if it would better be done as a proper syscall. Functionally the two
> are the same
> here, but doing such a fundamental operation as an ioctl doesn't feel
> quite right
> to me. As a system call, this could be something like
>
> int pidfd_get_fd(int pidfd, int their_fd, int flags);
>
> along the lines of dup3().
Thanks for taking a look, Arnd!
Yeah, Oleg hinted at this in the first version as well. I originally
disagreed but we can sure also do this as a separate syscall.
What we should keep in mind is that people already brought up adding new
fds to a task. Which is not a problem just something to remember as it
might potentially mean another syscall.
Christian
^ permalink raw reply
* RE: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: David Laight @ 2019-12-17 10:14 UTC (permalink / raw)
To: 'Aleksa Sarai'
Cc: 'Aleksa Sarai', Rasmus Villemoes, Alexander Viro,
Jeff Layton, J. Bruce Fields, Shuah Khan, dev@opencontainers.org,
containers@lists.linux-foundation.org, linux-api@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest@vger.kernel.org
In-Reply-To: <20191217064650.cd4bfb5d2koe6j7h@yavin.dot.cyphar.com>
>From Aleksa Sarai
> Sent: 17 December 2019 06:47
...
> > Just use u64 for all the fields.
>
> That is an option (and is the one that clone3 went with), but it's a bit
> awkward because umode_t is a u16 -- and it would be a waste of 6 bytes
> to store it as a u64. Arguably it could be extended but I personally
> find that to be very unlikely (and lots of other syscalls would need be
> updated).
6 bytes on interface structure will make almost no difference.
There is no reason to save more than 16 bits anywhere else.
You could error values with high bits set.
> I'm just going to move the padding to the end and change the error for
> non-zero padding to -E2BIG.
The padding had to be after the u16 field.
> > Use 'flags' bits to indicate whether the additional fields should be looked at.
> > Error if a 'flags' bit requires a value that isn't passed in the structure.
> >
> > Then you can add an extra field and old source code recompiled with the
> > new headers will still work - because the 'junk' value isn't looked at.
>
> This problem is already handled entirely by copy_struct_from_user().
>
> It is true that for some new fields it will be necessary to add a new
> flag (such as passing fds -- where 0 is a valid value) but for most new
> fields (especially pointer or flag fields) it will not be necessary
> because the 0 value is equivalent to the old behaviour. It also allows
> us to entirely avoid accepting junk from userspace.
Only if userspace is guaranteed to memset the entire structure
before making the call - rather than just fill in all the fields it knows about.
If it doesn't use memset() then recompiling old code with new headers
will pass garbage to the kernel.
copy_struct_from_user() cannot solve that problem.
You'll never be able to guarantee that all code actually clears the
entire structure - so at some point extending it will break recompilations
of old code - annoying.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Arnd Bergmann @ 2019-12-17 8:54 UTC (permalink / raw)
To: Sargun Dhillon
Cc: Christian Brauner, LKML, Linux Containers, Linux API,
Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Oleg Nesterov, Andy Lutomirski, Al Viro, gpascutto,
ealvarez, Florian Weimer, jld
In-Reply-To: <CAMp4zn8fzeiJVSn6EtRi6UAGh6AL3QWu=PZxw+=TAYJORjn_Sw@mail.gmail.com>
On Tue, Dec 17, 2019 at 3:50 AM Sargun Dhillon <sargun@sargun.me> wrote:
> On Mon, Dec 16, 2019 at 5:50 PM Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/ioctl.h>
> > > +
> > > +/* options to pass in to pidfd_getfd_args flags */
> > > +#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
> >
> > Please, make them cloexec by default unless there's a very good reason
> > not to.
> >
> For now then, should I have flags, and just say "reserved for future usage",
> or would you prefer that I drop flags entirely?
There is no need for adding reserved fields in an ioctl, just add a new ioctl
number if you need it later.
> > > +
> > > +struct pidfd_getfd_args {
> > > + __u32 size; /* sizeof(pidfd_getfd_args) */
> > > + __u32 fd; /* the tracee's file descriptor to get */
> > > + __u32 flags;
> > > +};
> >
> > I think you want to either want to pad this
> >
> > +struct pidfd_getfd_args {
> > + __u32 size; /* sizeof(pidfd_getfd_args) */
> > + __u32 fd; /* the tracee's file descriptor to get */
> > + __u32 flags;
> > __u32 reserved;
> > +};
> >
> > or use __aligned_u64 everywhere which I'd personally prefer instead of
> > this manual padding everywhere.
No, don't make ioctl structures extensible. If there is no 64-bit member
in it, 32-bit alignment is sufficient.
Also, having implicit padding is dangerous because it makes it easier to
leave it uninitialized, leaking kernel stack information on the copy_to_user().
Please drop the '__u32 size' argument, too: the size is fixed by definition
(through the _IOWR macro) and if you need to extend it you get a new
command anyway.
> Wouldn't __attribute__((packed)) achieve a similar thing of making sure
> the struct is a constant size across all compilers?
>
> I'll go with __aligned_u64 instead of packed, if you don't want to use packed.
__attribute__((packed)) is worse because it forces compilers to use byte
access on architectures that have no fast unaligned 32-bit load/store.
Basically you should never put __packed on a structure, but instead add
it to members that need to be unaligned within a sturct for compatibility
reasons.
> > > +
> > > +#define PIDFD_IOC_MAGIC 'p'
> > > +#define PIDFD_IO(nr) _IO(PIDFD_IOC_MAGIC, nr)
> > > +#define PIDFD_IOR(nr, type) _IOR(PIDFD_IOC_MAGIC, nr, type)
> > > +#define PIDFD_IOW(nr, type) _IOW(PIDFD_IOC_MAGIC, nr, type)
> > > +#define PIDFD_IOWR(nr, type) _IOWR(PIDFD_IOC_MAGIC, nr, type)
Drop these macros, they just make it harder to grep or script around the use
of _IOWR/_IOR/_IOW
> > > +#define PIDFD_IOCTL_GETFD PIDFD_IOWR(0xb0, \
> > > + struct pidfd_getfd_args)
Without the size and flag members, this can become the simpler
#define PIDFD_IOCTL_GETFD _IOWR('p', 0xb0, __u32)
> > > +
> > > const struct file_operations pidfd_fops = {
> > > .release = pidfd_release,
> > > .poll = pidfd_poll,
> > > + .unlocked_ioctl = pidfd_ioctl,
This needs
+ .compat_ioctl = compat_ptr_ioctl,
To work on compat tasks.
Finally, there is the question whether this should be an ioctl
operation at all, or
if it would better be done as a proper syscall. Functionally the two
are the same
here, but doing such a fundamental operation as an ioctl doesn't feel
quite right
to me. As a system call, this could be something like
int pidfd_get_fd(int pidfd, int their_fd, int flags);
along the lines of dup3().
Arnd
^ permalink raw reply
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Aleksa Sarai @ 2019-12-17 6:46 UTC (permalink / raw)
To: David Laight
Cc: 'Aleksa Sarai', Rasmus Villemoes, Alexander Viro,
Jeff Layton, J. Bruce Fields, Shuah Khan, dev@opencontainers.org,
containers@lists.linux-foundation.org, linux-api@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest@vger.kernel.org
In-Reply-To: <b26ef210ec5b42009cf09b1015065768@AcuMS.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 3660 bytes --]
On 2019-12-16, David Laight <David.Laight@ACULAB.COM> wrote:
> From: Aleksa Sarai
> > Sent: 15 December 2019 12:35
> > On 2019-12-14, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > > On 13/12/2019 23.23, Aleksa Sarai wrote:
> > > > The design of the original open_how struct layout was such that it
> > > > ensured that there would be no un-labelled (and thus potentially
> > > > non-zero) padding to avoid issues with struct expansion, as well as
> > > > providing a uniform representation on all architectures (to avoid
> > > > complications with OPEN_HOW_SIZE versioning).
> > > >
> > > > However, there were a few other desirable features which were not
> > > > fulfilled by the previous struct layout:
> > > >
> > > > * Adding new features (other than new flags) should always result in
> > > > the struct getting larger. However, by including a padding field, it
> > > > was possible for new fields to be added without expanding the
> > > > structure. This would somewhat complicate version-number based
> > > > checking of feature support.
> > > >
> > > > * A non-zero bit in __padding yielded -EINVAL when it should arguably
> > > > have been -E2BIG (because the padding bits are effectively
> > > > yet-to-be-used fields). However, the semantics are not entirely clear
> > > > because userspace may expect -E2BIG to only signify that the
> > > > structure is too big. It's much simpler to just provide the guarantee
> > > > that new fields will always result in a struct size increase, and
> > > > -E2BIG indicates you're using a field that's too recent for an older
> > > > kernel.
> > >
> > > And when the first extension adds another u64 field, that padding has to
> > > be added back in and checked for being 0, at which point the padding is
> > > again yet-to-be-used fields.
> >
> > Maybe I'm missing something, but what is the issue with
> >
> > struct open_how {
> > u64 flags;
> > u64 resolve;
> > u16 mode;
> > u64 next_extension;
> > } __attribute__((packed));
>
> Compile anything that accesses it for (say) sparc and look at the object code.
> You really, really, REALLY, don't want to EVER use 'packed'.
Right, so it's related to the "garbage code" problem. As mentioned
above, I wasn't aware it was as bad as folks in this thread have
mentioned.
> Just use u64 for all the fields.
That is an option (and is the one that clone3 went with), but it's a bit
awkward because umode_t is a u16 -- and it would be a waste of 6 bytes
to store it as a u64. Arguably it could be extended but I personally
find that to be very unlikely (and lots of other syscalls would need be
updated).
I'm just going to move the padding to the end and change the error for
non-zero padding to -E2BIG.
> Use 'flags' bits to indicate whether the additional fields should be looked at.
> Error if a 'flags' bit requires a value that isn't passed in the structure.
>
> Then you can add an extra field and old source code recompiled with the
> new headers will still work - because the 'junk' value isn't looked at.
This problem is already handled entirely by copy_struct_from_user().
It is true that for some new fields it will be necessary to add a new
flag (such as passing fds -- where 0 is a valid value) but for most new
fields (especially pointer or flag fields) it will not be necessary
because the 0 value is equivalent to the old behaviour. It also allows
us to entirely avoid accepting junk from userspace.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v18 11/13] open: introduce openat2(2) syscall
From: Aleksa Sarai @ 2019-12-17 6:39 UTC (permalink / raw)
To: Florian Weimer
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet, linux-ia64, linux-doc, Alexander Shishkin,
Rasmus Villemoes, linux-kernel
In-Reply-To: <20191216192158.B9F19832924A@oldenburg2.str.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]
On 2019-12-16, Florian Weimer <fweimer@redhat.com> wrote:
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 1d338357df8a..58c3a0e543c6 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -93,5 +93,40 @@
> >
> > #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> >
> > +/*
> > + * Arguments for how openat2(2) should open the target path. If @resolve is
> > + * zero, then openat2(2) operates very similarly to openat(2).
> > + *
> > + * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather
> > + * than being silently ignored. @mode must be zero unless one of {O_CREAT,
> > + * O_TMPFILE} are set.
> > + *
> > + * @flags: O_* flags.
> > + * @mode: O_CREAT/O_TMPFILE file mode.
> > + * @resolve: RESOLVE_* flags.
> > + */
> > +struct open_how {
> > + __aligned_u64 flags;
> > + __u16 mode;
> > + __u16 __padding[3]; /* must be zeroed */
> > + __aligned_u64 resolve;
> > +};
> > +
> > +#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
> > +#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
> > +
> > +/* how->resolve flags for openat2(2). */
> > +#define RESOLVE_NO_XDEV 0x01 /* Block mount-point crossings
> > + (includes bind-mounts). */
> > +#define RESOLVE_NO_MAGICLINKS 0x02 /* Block traversal through procfs-style
> > + "magic-links". */
> > +#define RESOLVE_NO_SYMLINKS 0x04 /* Block traversal through all symlinks
> > + (implies OEXT_NO_MAGICLINKS) */
> > +#define RESOLVE_BENEATH 0x08 /* Block "lexical" trickery like
> > + "..", symlinks, and absolute
> > + paths which escape the dirfd. */
> > +#define RESOLVE_IN_ROOT 0x10 /* Make all jumps to "/" and ".."
> > + be scoped inside the dirfd
> > + (similar to chroot(2)). */
> >
> > #endif /* _UAPI_LINUX_FCNTL_H */
>
> Would it be possible to move these to a new UAPI header?
>
> In glibc, we currently do not #include <linux/fcntl.h>. We need some of
> the AT_* constants in POSIX mode, and the header is not necessarily
> namespace-clean. If there was a separate header for openat2 support, we
> could use that easily, and we would only have to maintain the baseline
> definitions (which never change).
Sure, (assuming nobody objects) I can move it to "linux/openat2.h".
--
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 v3 0/4] Add pidfd getfd ioctl (Was Add ptrace get_fd request)
From: Christian Brauner @ 2019-12-17 3:02 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld
In-Reply-To: <20191217005842.GA14379@ircssh-2.c.rugged-nimbus-611.internal>
On Tue, Dec 17, 2019 at 12:58:45AM +0000, Sargun Dhillon wrote:
> This patchset introduces a mechanism to capture file descriptors from other
> processes by pidfd and ioctl. Although this can be achieved using
I like the idea in general as it's quite useful in general. And also for
the seccomp notifier and probably for CRIU too.
A few things that crossed my mind.
A thing I'm worried about is that this will be a stepping stone for
people argue for an fd-replacement feature though I think that
fd-injection not replacement might be sufficient.
I wonder whether we need to worry about special file descriptors, i.e.
anything anon-inode based, or devpts devices but I guess those concerns
already apply to ptrace anyway.
One more thing, with GETFD it seems useful to me that later we can add
a new flag - like I suggested in the previous version - to the seccomp
notifier that would allow a caller to request that with each seccomp
message received via the notifier ioctl() from the kernel a pidfd is
sent along. This would make it quite elegant to get fds for the
supervised task.
Christian
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-17 3:00 UTC (permalink / raw)
To: Sargun Dhillon
Cc: LKML, Linux Containers, Linux API, linux-fsdevel, Tycho Andersen,
Jann Horn, cyphar, oleg, Andy Lutomirski, viro, gpascutto,
ealvarez, fweimer, jld, Arnd Bergmann
In-Reply-To: <CAMp4zn8fzeiJVSn6EtRi6UAGh6AL3QWu=PZxw+=TAYJORjn_Sw@mail.gmail.com>
On Mon, Dec 16, 2019 at 06:49:37PM -0800, Sargun Dhillon wrote:
> On Mon, Dec 16, 2019 at 5:50 PM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > [Cc Arnd since he fiddled with ioctl()s quite a bit recently.]
> >
> >
> > That should be pidfd.h and the resulting new file be placed under the
> > pidfd entry in maintainers:
> > +F: include/uapi/linux/pidfd.h
> >
> > >
> > > enum pid_type
> > > {
> > > diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h
> > > new file mode 100644
> > > index 000000000000..4ec02ed8b39a
> > > --- /dev/null
> > > +++ b/include/uapi/linux/pid.h
> > > @@ -0,0 +1,26 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +#ifndef _UAPI_LINUX_PID_H
> > > +#define _UAPI_LINUX_PID_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/ioctl.h>
> > > +
> > > +/* options to pass in to pidfd_getfd_args flags */
> > > +#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
> >
> > Please, make them cloexec by default unless there's a very good reason
> > not to.
> >
> For now then, should I have flags, and just say "reserved for future usage",
> or would you prefer that I drop flags entirely?
Hm, you can leave the flags argument imho but maybe someone else has
stronger opinions about this.
>
> > > +
> > > +struct pidfd_getfd_args {
> > > + __u32 size; /* sizeof(pidfd_getfd_args) */
> > > + __u32 fd; /* the tracee's file descriptor to get */
> > > + __u32 flags;
> > > +};
> >
> > I think you want to either want to pad this
> >
> > +struct pidfd_getfd_args {
> > + __u32 size; /* sizeof(pidfd_getfd_args) */
> > + __u32 fd; /* the tracee's file descriptor to get */
> > + __u32 flags;
> > __u32 reserved;
> > +};
> >
> > or use __aligned_u64 everywhere which I'd personally prefer instead of
> > this manual padding everywhere.
> >
> Wouldn't __attribute__((packed)) achieve a similar thing of making sure
> the struct is a constant size across all compilers?
>
> I'll go with __aligned_u64 instead of packed, if you don't want to use packed.
We had a discussion about this in relation to the openat2()
patchset just recently. Florian and a few others raised good points why
we might not want to use packed:
https://lore.kernel.org/lkml/87o8w9bcaf.fsf@mid.deneb.enyo.de/
https://lore.kernel.org/lkml/a328b91d-fd8f-4f27-b3c2-91a9c45f18c0@rasmusvillemoes.dk/
Christian
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Sargun Dhillon @ 2019-12-17 2:49 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Linux Containers, Linux API, linux-fsdevel, Tycho Andersen,
Jann Horn, cyphar, oleg, Andy Lutomirski, viro, gpascutto,
ealvarez, fweimer, jld, Arnd Bergmann
In-Reply-To: <20191217015001.sp6mrhuiqrivkq3u@wittgenstein>
On Mon, Dec 16, 2019 at 5:50 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> [Cc Arnd since he fiddled with ioctl()s quite a bit recently.]
>
>
> That should be pidfd.h and the resulting new file be placed under the
> pidfd entry in maintainers:
> +F: include/uapi/linux/pidfd.h
>
> >
> > enum pid_type
> > {
> > diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h
> > new file mode 100644
> > index 000000000000..4ec02ed8b39a
> > --- /dev/null
> > +++ b/include/uapi/linux/pid.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_LINUX_PID_H
> > +#define _UAPI_LINUX_PID_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/ioctl.h>
> > +
> > +/* options to pass in to pidfd_getfd_args flags */
> > +#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
>
> Please, make them cloexec by default unless there's a very good reason
> not to.
>
For now then, should I have flags, and just say "reserved for future usage",
or would you prefer that I drop flags entirely?
> > +
> > +struct pidfd_getfd_args {
> > + __u32 size; /* sizeof(pidfd_getfd_args) */
> > + __u32 fd; /* the tracee's file descriptor to get */
> > + __u32 flags;
> > +};
>
> I think you want to either want to pad this
>
> +struct pidfd_getfd_args {
> + __u32 size; /* sizeof(pidfd_getfd_args) */
> + __u32 fd; /* the tracee's file descriptor to get */
> + __u32 flags;
> __u32 reserved;
> +};
>
> or use __aligned_u64 everywhere which I'd personally prefer instead of
> this manual padding everywhere.
>
Wouldn't __attribute__((packed)) achieve a similar thing of making sure
the struct is a constant size across all compilers?
I'll go with __aligned_u64 instead of packed, if you don't want to use packed.
> > +
> > +#define PIDFD_IOC_MAGIC 'p'
> > +#define PIDFD_IO(nr) _IO(PIDFD_IOC_MAGIC, nr)
> > +#define PIDFD_IOR(nr, type) _IOR(PIDFD_IOC_MAGIC, nr, type)
> > +#define PIDFD_IOW(nr, type) _IOW(PIDFD_IOC_MAGIC, nr, type)
> > +#define PIDFD_IOWR(nr, type) _IOWR(PIDFD_IOC_MAGIC, nr, type)
> > +
> > +#define PIDFD_IOCTL_GETFD PIDFD_IOWR(0xb0, \
> > + struct pidfd_getfd_args)
> > +
> > +#endif /* _UAPI_LINUX_PID_H */
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 6cabc124378c..d9971e664e82 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1726,9 +1726,81 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> > return poll_flags;
> > }
> >
> > +static long pidfd_getfd(struct pid *pid, struct pidfd_getfd_args __user *buf)
> > +{
> > + struct pidfd_getfd_args args;
> > + unsigned int fd_flags = 0;
> > + struct task_struct *task;
> > + struct file *file;
> > + u32 user_size;
> > + int ret, fd;
> > +
> > + ret = get_user(user_size, &buf->size);
> > + if (ret)
> > + return ret;
> > +
> > + ret = copy_struct_from_user(&args, sizeof(args), buf, user_size);
> > + if (ret)
> > + return ret;
> > + if ((args.flags & ~(PIDFD_GETFD_CLOEXEC)) != 0)
> > + return -EINVAL;
>
> Nit: It's more common - especially in this file - to do
>
> if (args.flags & ~PIDFD_GETFD_CLOEXEC)
> return -EINVAL;
>
> > + if (args.flags & PIDFD_GETFD_CLOEXEC)
> > + fd_flags |= O_CLOEXEC;
> > +
I'll drop this bit, and just make it CLOEXEC by default.
> > + task = get_pid_task(pid, PIDTYPE_PID);
> > + if (!task)
> > + return -ESRCH;
>
> \n
>
> > + ret = -EPERM;
> > + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> > + goto out;
>
> \n
>
> Please don't pre-set errors unless they are used by multiple exit paths.
> if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
> ret = -EPERM;
> goto out;
> }
>
> > + ret = -EBADF;
> > + file = fget_task(task, args.fd);
> > + if (!file)
> > + goto out;
>
> Same.
>
> > +
> > + fd = get_unused_fd_flags(fd_flags);
> > + if (fd < 0) {
> > + ret = fd;
> > + goto out_put_file;
> > + }
>
> \n
>
> > + /*
> > + * security_file_receive must come last since it may have side effects
> > + * and cannot be reversed.
> > + */
> > + ret = security_file_receive(file);
> > + if (ret)
> > + goto out_put_fd;
> > +
> > + fd_install(fd, file);
> > + put_task_struct(task);
> > + return fd;
> > +
> > +out_put_fd:
> > + put_unused_fd(fd);
> > +out_put_file:
> > + fput(file);
> > +out:
> > + put_task_struct(task);
> > + return ret;
> > +}
> > +
> > +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > + struct pid *pid = file->private_data;
> > + void __user *buf = (void __user *)arg;
> > +
> > + switch (cmd) {
> > + case PIDFD_IOCTL_GETFD:
> > + return pidfd_getfd(pid, buf);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > const struct file_operations pidfd_fops = {
> > .release = pidfd_release,
> > .poll = pidfd_poll,
> > + .unlocked_ioctl = pidfd_ioctl,
> > #ifdef CONFIG_PROC_FS
> > .show_fdinfo = pidfd_show_fdinfo,
> > #endif
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH v3 4/4] samples: Add example of using pidfd getfd in conjunction with user trap
From: Christian Brauner @ 2019-12-17 2:07 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld
In-Reply-To: <20191217010025.GA14479@ircssh-2.c.rugged-nimbus-611.internal>
On Tue, Dec 17, 2019 at 01:00:27AM +0000, Sargun Dhillon wrote:
> This sample adds the usage of SECCOMP_RET_USER_NOTIF together with pidfd
> GETFD ioctl. It shows trapping a syscall, and handling it by extracting
> the FD into the parent process without stopping the child process.
> Although, in this example, there's no explicit policy separation in
> the two processes, it can be generalized into the example of a transparent
> proxy.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> samples/seccomp/.gitignore | 1 +
> samples/seccomp/Makefile | 9 +-
> samples/seccomp/user-trap-pidfd.c | 190 ++++++++++++++++++++++++++++++
This is a great sample.
Could you please also add tests without seccomp to the pidfd-testsuite
itself under tools/testing/selftests/pidfd. I want to have all pidfd
features tested in one place so that people don't need to run
seccomp-bpf, or compile the optional samples but can just run the pidfd
test-suite and it'll bang on all features.
Christian
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-17 1:50 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191217010001.GA14461@ircssh-2.c.rugged-nimbus-611.internal>
[Cc Arnd since he fiddled with ioctl()s quite a bit recently.]
On Tue, Dec 17, 2019 at 01:00:04AM +0000, Sargun Dhillon wrote:
> This adds an ioctl which allows file descriptors to be extracted
> from processes based on their pidfd.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. For example, it can be used
> to bind a socket to a privileged port. This is similar to ptrace, and
> using ptrace parasitic code injection to extract a file descriptor from a
> process, but without breaking debuggers, or paying the ptrace overhead
> cost.
>
> You must have the ability to ptrace the process in order to extract any
> file descriptors from it. ptrace can already be used to extract file
> descriptors based on parasitic code injections, so the permissions
> model is aligned.
>
> The ioctl takes a pointer to pidfd_getfd_args. pidfd_getfd_args contains
> a size, which allows for gradual evolution of the API. There is an options
> field, which can be used to state whether the fd should be opened with
> CLOEXEC, or not. An additional options field may be added in the future
> to include the ability to clear cgroup information about the file
> descriptor at a later point. If the structure is from a newer kernel, and
> includes members which make it larger than the structure that's known to
> this kernel version, E2BIG will be returned.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> Documentation/ioctl/ioctl-number.rst | 1 +
> include/linux/pid.h | 1 +
> include/uapi/linux/pid.h | 26 ++++++++++
> kernel/fork.c | 72 ++++++++++++++++++++++++++++
> 4 files changed, 100 insertions(+)
> create mode 100644 include/uapi/linux/pid.h
>
> diff --git a/Documentation/ioctl/ioctl-number.rst b/Documentation/ioctl/ioctl-number.rst
> index bef79cd4c6b4..be2efb93acd1 100644
> --- a/Documentation/ioctl/ioctl-number.rst
> +++ b/Documentation/ioctl/ioctl-number.rst
> @@ -272,6 +272,7 @@ Code Seq# Include File Comments
> <mailto:tim@cyberelk.net>
> 'p' A1-A5 linux/pps.h LinuxPPS
> <mailto:giometti@linux.it>
> +'p' B0-CF uapi/linux/pid.h
> 'q' 00-1F linux/serio.h
> 'q' 80-FF linux/telephony.h Internet PhoneJACK, Internet LineJACK
> linux/ixjuser.h <http://web.archive.org/web/%2A/http://www.quicknet.net>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index 9645b1194c98..65f1a73040c9 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -5,6 +5,7 @@
> #include <linux/rculist.h>
> #include <linux/wait.h>
> #include <linux/refcount.h>
> +#include <uapi/linux/pid.h>
That should be pidfd.h and the resulting new file be placed under the
pidfd entry in maintainers:
+F: include/uapi/linux/pidfd.h
>
> enum pid_type
> {
> diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h
> new file mode 100644
> index 000000000000..4ec02ed8b39a
> --- /dev/null
> +++ b/include/uapi/linux/pid.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_PID_H
> +#define _UAPI_LINUX_PID_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* options to pass in to pidfd_getfd_args flags */
> +#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
Please, make them cloexec by default unless there's a very good reason
not to.
> +
> +struct pidfd_getfd_args {
> + __u32 size; /* sizeof(pidfd_getfd_args) */
> + __u32 fd; /* the tracee's file descriptor to get */
> + __u32 flags;
> +};
I think you want to either want to pad this
+struct pidfd_getfd_args {
+ __u32 size; /* sizeof(pidfd_getfd_args) */
+ __u32 fd; /* the tracee's file descriptor to get */
+ __u32 flags;
__u32 reserved;
+};
or use __aligned_u64 everywhere which I'd personally prefer instead of
this manual padding everywhere.
> +
> +#define PIDFD_IOC_MAGIC 'p'
> +#define PIDFD_IO(nr) _IO(PIDFD_IOC_MAGIC, nr)
> +#define PIDFD_IOR(nr, type) _IOR(PIDFD_IOC_MAGIC, nr, type)
> +#define PIDFD_IOW(nr, type) _IOW(PIDFD_IOC_MAGIC, nr, type)
> +#define PIDFD_IOWR(nr, type) _IOWR(PIDFD_IOC_MAGIC, nr, type)
> +
> +#define PIDFD_IOCTL_GETFD PIDFD_IOWR(0xb0, \
> + struct pidfd_getfd_args)
> +
> +#endif /* _UAPI_LINUX_PID_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6cabc124378c..d9971e664e82 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1726,9 +1726,81 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> return poll_flags;
> }
>
> +static long pidfd_getfd(struct pid *pid, struct pidfd_getfd_args __user *buf)
> +{
> + struct pidfd_getfd_args args;
> + unsigned int fd_flags = 0;
> + struct task_struct *task;
> + struct file *file;
> + u32 user_size;
> + int ret, fd;
> +
> + ret = get_user(user_size, &buf->size);
> + if (ret)
> + return ret;
> +
> + ret = copy_struct_from_user(&args, sizeof(args), buf, user_size);
> + if (ret)
> + return ret;
> + if ((args.flags & ~(PIDFD_GETFD_CLOEXEC)) != 0)
> + return -EINVAL;
Nit: It's more common - especially in this file - to do
if (args.flags & ~PIDFD_GETFD_CLOEXEC)
return -EINVAL;
> + if (args.flags & PIDFD_GETFD_CLOEXEC)
> + fd_flags |= O_CLOEXEC;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
\n
> + ret = -EPERM;
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> + goto out;
\n
Please don't pre-set errors unless they are used by multiple exit paths.
if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
ret = -EPERM;
goto out;
}
> + ret = -EBADF;
> + file = fget_task(task, args.fd);
> + if (!file)
> + goto out;
Same.
> +
> + fd = get_unused_fd_flags(fd_flags);
> + if (fd < 0) {
> + ret = fd;
> + goto out_put_file;
> + }
\n
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
> + if (ret)
> + goto out_put_fd;
> +
> + fd_install(fd, file);
> + put_task_struct(task);
> + return fd;
> +
> +out_put_fd:
> + put_unused_fd(fd);
> +out_put_file:
> + fput(file);
> +out:
> + put_task_struct(task);
> + return ret;
> +}
> +
> +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pid *pid = file->private_data;
> + void __user *buf = (void __user *)arg;
> +
> + switch (cmd) {
> + case PIDFD_IOCTL_GETFD:
> + return pidfd_getfd(pid, buf);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> const struct file_operations pidfd_fops = {
> .release = pidfd_release,
> .poll = pidfd_poll,
> + .unlocked_ioctl = pidfd_ioctl,
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = pidfd_show_fdinfo,
> #endif
> --
> 2.20.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox