* 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 v4 0/5] Add pidfd getfd ioctl (Was Add ptrace get_fd request)
From: Sargun Dhillon @ 2019-12-18 23:53 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This patchset introduces a mechanism to capture file descriptors from other
processes by pidfd and ioctl. Although this can be achieved using
SCM_RIGHTS, and parasitic code injection, this offers a more
straightforward mechanism.
It has a flags mechanism that's only usable to set CLOEXEC on the fd,
but I'm thinking that it could be extended to other aspects. For example,
for sockets, one could want to scrub the cgroup information.
Changes since v3:
* Add self-test
* Move to ioctl passing fd directly, versus args struct
* Shuffle around include files
Changes since v2:
* Move to ioctl on pidfd instead of ptrace function
* Add security check before moving file descriptor
Changes since the RFC v1:
* Introduce a new helper to fs/file.c to fetch a file descriptor from
any process. It largely uses the code suggested by Oleg, with a few
changes to fix locking
* It uses an extensible options struct to supply the FD, and option.
* I added a sample, using the code from the user-ptrace sample
Sargun Dhillon (5):
vfs, fdtable: Add get_task_file helper
pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
samples: split generalized user-trap code into helper file
samples: Add example of using pidfd getfd in conjunction with user
trap
test: Add test for pidfd getfd
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 1 +
fs/file.c | 22 +-
include/linux/file.h | 2 +
include/uapi/linux/pidfd.h | 10 +
kernel/fork.c | 77 ++++++
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 15 +-
samples/seccomp/user-trap-helper.c | 84 +++++++
samples/seccomp/user-trap-helper.h | 13 +
samples/seccomp/user-trap-pidfd.c | 185 ++++++++++++++
samples/seccomp/user-trap.c | 85 +------
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 231 ++++++++++++++++++
15 files changed, 641 insertions(+), 89 deletions(-)
create mode 100644 include/uapi/linux/pidfd.h
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
create mode 100644 samples/seccomp/user-trap-pidfd.c
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
--
2.20.1
^ permalink raw reply
* [PATCH v4 1/5] vfs, fdtable: Add get_task_file helper
From: Sargun Dhillon @ 2019-12-18 23:53 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This introduces a function which can be used to fetch a file, given an
arbitrary task. As long as the user holds a reference (refcnt) to the
task_struct it is safe to call, and will either return NULL on failure,
or a pointer to the file, with a refcnt.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
fs/file.c | 22 ++++++++++++++++++++--
include/linux/file.h | 2 ++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 2f4fcf985079..0ceeb046f4f3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struct *files)
spin_unlock(&files->file_lock);
}
-static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+static struct file *__fget_files(struct files_struct *files, unsigned int fd,
+ fmode_t mask, unsigned int refs)
{
- struct files_struct *files = current->files;
struct file *file;
rcu_read_lock();
@@ -729,6 +729,11 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
return file;
}
+static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+{
+ return __fget_files(current->files, fd, mask, refs);
+}
+
struct file *fget_many(unsigned int fd, unsigned int refs)
{
return __fget(fd, FMODE_PATH, refs);
@@ -746,6 +751,19 @@ struct file *fget_raw(unsigned int fd)
}
EXPORT_SYMBOL(fget_raw);
+struct file *fget_task(struct task_struct *task, unsigned int fd)
+{
+ struct file *file = NULL;
+
+ task_lock(task);
+ if (task->files)
+ file = __fget_files(task->files, fd, 0, 1);
+
+ task_unlock(task);
+
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/file.h b/include/linux/file.h
index 3fcddff56bc4..c6c7b24ea9f7 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -16,6 +16,7 @@ extern void fput(struct file *);
extern void fput_many(struct file *, unsigned int);
struct file_operations;
+struct task_struct;
struct vfsmount;
struct dentry;
struct inode;
@@ -47,6 +48,7 @@ static inline void fdput(struct fd fd)
extern struct file *fget(unsigned int fd);
extern struct file *fget_many(unsigned int fd, unsigned int refs);
extern struct file *fget_raw(unsigned int fd);
+extern struct file *fget_task(struct task_struct *task, unsigned int fd);
extern unsigned long __fdget(unsigned int fd);
extern unsigned long __fdget_raw(unsigned int fd);
extern unsigned long __fdget_pos(unsigned int fd);
--
2.20.1
^ permalink raw reply related
* [PATCH v4 2/5] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Sargun Dhillon @ 2019-12-18 23:55 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
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>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 1 +
include/uapi/linux/pidfd.h | 10 +++
kernel/fork.c | 77 +++++++++++++++++++
4 files changed, 89 insertions(+)
create mode 100644 include/uapi/linux/pidfd.h
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 4ef86433bd67..9f9be681662b 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -273,6 +273,7 @@ Code Seq# Include File Comments
<mailto:tim@cyberelk.net>
'p' A1-A5 linux/pps.h LinuxPPS
<mailto:giometti@linux.it>
+'p' B0-CF linux/pidfd.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/MAINTAINERS b/MAINTAINERS
index cc0a4a8ae06a..bc370ff59dbf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13014,6 +13014,7 @@ M: Christian Brauner <christian@brauner.io>
L: linux-kernel@vger.kernel.org
S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
+F: include/uapi/linux/pidfd.h
F: samples/pidfd/
F: tools/testing/selftests/pidfd/
F: tools/testing/selftests/clone3/
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
new file mode 100644
index 000000000000..90ff535be048
--- /dev/null
+++ b/include/uapi/linux/pidfd.h
@@ -0,0 +1,10 @@
+/* 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>
+
+#define PIDFD_IOCTL_GETFD _IOWR('p', 0xb0, __u32)
+
+#endif /* _UAPI_LINUX_PID_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 2508a4f238a3..09b5f233b5a8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -94,6 +94,7 @@
#include <linux/thread_info.h>
#include <linux/stackleak.h>
#include <linux/kasan.h>
+#include <uapi/linux/pidfd.h>
#include <asm/pgtable.h>
#include <asm/pgalloc.h>
@@ -1790,9 +1791,85 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
return poll_flags;
}
+static struct file *__pidfd_getfd_fget_task(struct task_struct *task, u32 fd)
+{
+ struct file *file;
+ int ret;
+
+ ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
+ file = ERR_PTR(-EPERM);
+ goto out;
+ }
+
+ file = fget_task(task, fd);
+ if (!file)
+ file = ERR_PTR(-EBADF);
+
+out:
+ mutex_unlock(&task->signal->cred_guard_mutex);
+ return file;
+}
+
+static long pidfd_getfd(struct pid *pid, u32 fd)
+{
+ struct task_struct *task;
+ struct file *file;
+ int ret, retfd;
+
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task)
+ return -ESRCH;
+
+ file = __pidfd_getfd_fget_task(task, fd);
+ put_task_struct(task);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ retfd = get_unused_fd_flags(O_CLOEXEC);
+ if (retfd < 0) {
+ ret = retfd;
+ goto out;
+ }
+
+ /*
+ * 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(retfd, file);
+ return retfd;
+
+out_put_fd:
+ put_unused_fd(retfd);
+out:
+ fput(file);
+ return ret;
+}
+
+static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct pid *pid = file->private_data;
+
+ switch (cmd) {
+ case PIDFD_IOCTL_GETFD:
+ return pidfd_getfd(pid, arg);
+ default:
+ return -EINVAL;
+ }
+}
+
const struct file_operations pidfd_fops = {
.release = pidfd_release,
.poll = pidfd_poll,
+ .unlocked_ioctl = pidfd_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
#ifdef CONFIG_PROC_FS
.show_fdinfo = pidfd_show_fdinfo,
#endif
--
2.20.1
^ permalink raw reply related
* [PATCH v4 3/5] samples: split generalized user-trap code into helper file
From: Sargun Dhillon @ 2019-12-18 23:55 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This moves the code for setting up a syscall interceptor with user
notification and sending the user notification file descriptor over a
socket using SCM_RIGHTS into a file that can be shared between multiple
samples.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
samples/seccomp/Makefile | 6 ++-
samples/seccomp/user-trap-helper.c | 84 +++++++++++++++++++++++++++++
samples/seccomp/user-trap-helper.h | 13 +++++
samples/seccomp/user-trap.c | 85 +-----------------------------
4 files changed, 103 insertions(+), 85 deletions(-)
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 009775b52538..82b7347318d1 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -16,9 +16,13 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+
+HOSTCFLAGS_user-trap-helper.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-helper.o += -idirafter $(objtree)/include
+
HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
-user-trap-objs := user-trap.o
+user-trap-objs := user-trap.o user-trap-helper.o
# Try to match the kernel target.
ifndef CONFIG_64BIT
diff --git a/samples/seccomp/user-trap-helper.c b/samples/seccomp/user-trap-helper.c
new file mode 100644
index 000000000000..f91ae9d947c5
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.c
@@ -0,0 +1,84 @@
+#include <linux/seccomp.h>
+#include <linux/filter.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stddef.h>
+#include <sys/types.h>
+#include <sys/syscall.h>
+#include <sys/socket.h>
+#include "user-trap-helper.h"
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+int send_fd(int sock, int fd)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+ *((int *)CMSG_DATA(cmsg)) = fd;
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ if (sendmsg(sock, &msg, 0) < 0) {
+ perror("sendmsg");
+ return -1;
+ }
+
+ return 0;
+}
+
+int recv_fd(int sock)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+
+ if (recvmsg(sock, &msg, 0) < 0) {
+ perror("recvmsg");
+ return -1;
+ }
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+
+ return *((int *)CMSG_DATA(cmsg));
+}
diff --git a/samples/seccomp/user-trap-helper.h b/samples/seccomp/user-trap-helper.h
new file mode 100644
index 000000000000..a5ebda25fdfe
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.h
@@ -0,0 +1,13 @@
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <errno.h>
+
+static inline int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+int user_trap_syscall(int nr, unsigned int flags);
+int send_fd(int sock, int fd);
+int recv_fd(int sock);
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..1b6526587456 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -5,101 +5,18 @@
#include <errno.h>
#include <fcntl.h>
#include <string.h>
-#include <stddef.h>
#include <sys/sysmacros.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/mman.h>
-#include <sys/syscall.h>
#include <sys/user.h>
#include <sys/ioctl.h>
-#include <sys/ptrace.h>
#include <sys/mount.h>
#include <linux/limits.h>
-#include <linux/filter.h>
#include <linux/seccomp.h>
-
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
-
-static int seccomp(unsigned int op, unsigned int flags, void *args)
-{
- errno = 0;
- return syscall(__NR_seccomp, op, flags, args);
-}
-
-static int send_fd(int sock, int fd)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof(int));
- *((int *)CMSG_DATA(cmsg)) = fd;
- msg.msg_controllen = cmsg->cmsg_len;
-
- if (sendmsg(sock, &msg, 0) < 0) {
- perror("sendmsg");
- return -1;
- }
-
- return 0;
-}
-
-static int recv_fd(int sock)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
-
- if (recvmsg(sock, &msg, 0) < 0) {
- perror("recvmsg");
- return -1;
- }
-
- cmsg = CMSG_FIRSTHDR(&msg);
-
- return *((int *)CMSG_DATA(cmsg));
-}
-
-static int user_trap_syscall(int nr, unsigned int flags)
-{
- struct sock_filter filter[] = {
- BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
- offsetof(struct seccomp_data, nr)),
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
- };
-
- struct sock_fprog prog = {
- .len = (unsigned short)ARRAY_SIZE(filter),
- .filter = filter,
- };
-
- return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
-}
+#include "user-trap-helper.h"
static int handle_req(struct seccomp_notif *req,
struct seccomp_notif_resp *resp, int listener)
--
2.20.1
^ permalink raw reply related
* [PATCH v4 4/5] samples: Add example of using pidfd getfd in conjunction with user trap
From: Sargun Dhillon @ 2019-12-18 23:55 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
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 | 185 ++++++++++++++++++++++++++++++
3 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 samples/seccomp/user-trap-pidfd.c
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index d1e2e817d556..f37e3692a1dd 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -2,3 +2,4 @@ bpf-direct
bpf-fancy
dropper
user-trap
+user-trap-pidfd
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 82b7347318d1..c3880869cadc 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-y := bpf-fancy dropper bpf-direct user-trap
+hostprogs-y := bpf-fancy dropper bpf-direct user-trap user-trap-pidfd
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -24,6 +24,11 @@ HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
user-trap-objs := user-trap.o user-trap-helper.o
+HOSTCFLAGS_user-trap-pidfd.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-pidfd.o += -idirafter $(objtree)/include
+user-trap-pidfd-objs := user-trap-pidfd.o user-trap-helper.o
+
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -39,10 +44,12 @@ HOSTCFLAGS_dropper.o += $(MFLAG)
HOSTCFLAGS_bpf-helper.o += $(MFLAG)
HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTCFLAGS_user-trap.o += $(MFLAG)
+HOSTCFLAGS_user-trap-pidfd.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
HOSTLDLIBS_user-trap += $(MFLAG)
+HOSTLDLIBS_user-trap-pidfd += $(MFLAG)
endif
always := $(hostprogs-y)
endif
diff --git a/samples/seccomp/user-trap-pidfd.c b/samples/seccomp/user-trap-pidfd.c
new file mode 100644
index 000000000000..bd5730347a81
--- /dev/null
+++ b/samples/seccomp/user-trap-pidfd.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/seccomp.h>
+#include <linux/prctl.h>
+#include <linux/pidfd.h>
+#include <sys/socket.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/ioctl.h>
+#include <assert.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <netinet/in.h>
+#include "user-trap-helper.h"
+
+#define CHILD_PORT_TRY_BIND 80
+#define CHILD_PORT_ACTUAL_BIND 4998
+
+static int tracee(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(CHILD_PORT_TRY_BIND),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_ANY)
+ }
+ };
+ socklen_t addrlen = sizeof(addr);
+ int sock, ret = 1;
+
+ sock = socket(AF_INET, SOCK_STREAM, 0);
+ if (sock == -1) {
+ perror("socket");
+ goto out;
+ }
+
+
+ if (bind(sock, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out;
+ }
+
+ printf("Child successfully performed bind operation\n");
+ if (getsockname(sock, (struct sockaddr *) &addr, &addrlen)) {
+ perror("getsockname");
+ goto out;
+ }
+
+
+ printf("Socket bound to port %d\n", ntohs(addr.sin_port));
+ assert(ntohs(addr.sin_port) == CHILD_PORT_ACTUAL_BIND);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+static int handle_req(int listener, int pidfd)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(CHILD_PORT_ACTUAL_BIND),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_LOOPBACK)
+ }
+ };
+ struct seccomp_notif_sizes sizes;
+ struct seccomp_notif_resp *resp;
+ struct seccomp_notif *req;
+ int fd, ret = 1;
+
+ if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
+ perror("seccomp(GET_NOTIF_SIZES)");
+ goto out;
+ }
+ req = malloc(sizes.seccomp_notif);
+ if (!req)
+ goto out;
+ memset(req, 0, sizeof(*req));
+
+ resp = malloc(sizes.seccomp_notif_resp);
+ if (!resp)
+ goto out_free_req;
+ memset(resp, 0, sizeof(*resp));
+
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
+ perror("ioctl recv");
+ goto out;
+ }
+ printf("Child tried to call bind with fd: %lld\n", req->data.args[0]);
+ fd = ioctl(pidfd, PIDFD_IOCTL_GETFD, req->data.args[0]);
+ if (fd == -1) {
+ perror("ioctl pidfd");
+ goto out_free_resp;
+ }
+ if (bind(fd, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out_free_resp;
+ }
+
+ resp->id = req->id;
+ resp->error = 0;
+ resp->val = 0;
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0) {
+ perror("ioctl send");
+ goto out_free_resp;
+ }
+
+ ret = 0;
+out_free_resp:
+ free(resp);
+out_free_req:
+ free(req);
+out:
+ return ret;
+}
+
+static int pidfd_open(pid_t pid, unsigned int flags)
+{
+ errno = 0;
+ return syscall(__NR_pidfd_open, pid, flags);
+}
+
+int main(void)
+{
+ int pidfd, listener, sk_pair[2], ret = 1;
+ pid_t pid;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ goto out;
+ }
+
+ pid = fork();
+ if (pid < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+
+ if (pid == 0) {
+ if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+ perror("prctl(NO_NEW_PRIVS)");
+ exit(1);
+ }
+ listener = user_trap_syscall(__NR_bind,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ if (listener < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+ if (send_fd(sk_pair[1], listener) < 0)
+ exit(1);
+ close(listener);
+ exit(tracee());
+ }
+
+ pidfd = pidfd_open(pid, 0);
+ if (pidfd < 0) {
+ perror("pidfd_open");
+ goto kill_child;
+ }
+
+ listener = recv_fd(sk_pair[0]);
+ if (listener < 0)
+ goto kill_child;
+
+ if (handle_req(listener, pidfd))
+ goto kill_child;
+
+ /* Wait for child to finish */
+ waitpid(pid, NULL, 0);
+
+ ret = 0;
+ goto close_pair;
+
+kill_child:
+ kill(pid, SIGKILL);
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+out:
+ return ret;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH v4 5/5] test: Add test for pidfd getfd
From: Sargun Dhillon @ 2019-12-18 23:55 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This adds four tests:
* Fetch FD, and then compare via kcmp
* Read data from FD to make sure it works
* Make sure getfd can be blocked by blocking ptrace_may_access
* Making sure fetching bad FDs fails
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 231 ++++++++++++++++++
3 files changed, 233 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 8d069490e17b..3a779c084d96 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -2,3 +2,4 @@ pidfd_open_test
pidfd_poll_test
pidfd_test
pidfd_wait
+pidfd_getfd_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 43db1b98e845..75a545861375 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
CFLAGS += -g -I../../../../usr/include/ -pthread
-TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait
+TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait pidfd_getfd_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
new file mode 100644
index 000000000000..25c11a030afc
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <linux/pidfd.h>
+#include <linux/kcmp.h>
+#include <linux/capability.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+
+#define WELL_KNOWN_CHILD_FD 100
+#define UNKNOWN_FD 111
+#define SECRET_MESSAGE "secret"
+
+static int kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1,
+ unsigned long idx2)
+{
+ return syscall(SYS_kcmp, pid1, pid2, type, idx1, idx2);
+}
+
+static int child(bool disable_ptrace, int sk)
+{
+ char buf[1024];
+ int ret, fd;
+
+ ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
+ if (ret)
+ ksft_exit_fail_msg("%s: Child could not set DEATHSIG\n",
+ strerror(errno));
+
+ fd = syscall(SYS_memfd_create, "test", 0);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s: Child could not create memfd\n",
+ strerror(errno));
+
+ ret = write(fd, SECRET_MESSAGE, sizeof(SECRET_MESSAGE));
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child could not write secret message\n",
+ strerror(errno));
+
+ ret = dup2(fd, WELL_KNOWN_CHILD_FD);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Could not dup fd into well-known FD\n",
+ strerror(errno));
+
+ ret = close(fd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child could close old fd\n",
+ strerror(errno));
+
+ if (disable_ptrace) {
+ ret = prctl(PR_SET_DUMPABLE, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child failed to disable ptrace\n",
+ strerror(errno));
+ }
+ ret = send(sk, "L", 1, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child failed to send launched message\n",
+ strerror(errno));
+ if (ret == 0)
+ ksft_exit_fail_msg("Failed to send launch message; other side is closed\n");
+
+ close(sk);
+ pause();
+
+ return EXIT_SUCCESS;
+}
+
+static int start_child(bool disable_ptrace, pid_t *childpid)
+{
+ int pidfd, ret, sk_pair[2];
+ char buf[1];
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0)
+ ksft_exit_fail_msg("%s: failed to create socketpair\n",
+ strerror(errno));
+ *childpid = fork();
+ if (*childpid < 0)
+ ksft_exit_fail_msg("%s: failed to fork a child process\n",
+ strerror(errno));
+
+ if (*childpid == 0)
+ exit(child(disable_ptrace, sk_pair[1]));
+
+ close(sk_pair[1]);
+
+ pidfd = sys_pidfd_open(*childpid, 0);
+ if (pidfd < 0)
+ ksft_exit_fail_msg("%s: failed to pidfd_open\n",
+ strerror(errno));
+
+ ret = recv(sk_pair[0], &buf, 1, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed read from launch socket\n",
+ strerror(errno));
+ if (ret == 0)
+ ksft_exit_fail_msg("Failed to read from launch socket, child failed\n");
+
+ return pidfd;
+}
+
+static void test_kcmp_and_fetch_fd(void)
+{
+ char buf[sizeof(SECRET_MESSAGE)];
+ int fd, pidfd, ret;
+ pid_t child_pid;
+
+ pidfd = start_child(false, &child_pid);
+
+ fd = ioctl(pidfd, PIDFD_IOCTL_GETFD, WELL_KNOWN_CHILD_FD);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s: getfd failed\n", strerror(errno));
+
+ ret = kcmp(getpid(), child_pid, KCMP_FILE, fd, WELL_KNOWN_CHILD_FD);
+ if (ret != 0)
+ ksft_exit_fail_msg("Our FD not equal to child FD\n");
+
+ ksft_test_result_pass("kcmp\n");
+
+ ret = lseek(fd, 0, SEEK_SET);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: seek failed\n", strerror(errno));
+ if (ret != 0)
+ ksft_exit_fail_msg("%d: unexpected seek position\n", ret);
+
+ ret = read(fd, buf, sizeof(buf));
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to read secret message\n",
+ strerror(errno));
+
+ if (strncmp(SECRET_MESSAGE, buf, sizeof(buf)) != 0)
+ ksft_exit_fail_msg("%s: Secret message not correct\n", buf);
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ ksft_test_result_pass("fetch_and_read\n");
+}
+
+static void test_no_ptrace(void)
+{
+ int fd, pidfd, ret, uid;
+ pid_t child_pid;
+
+ /* turn into nobody if we're root, to avoid CAP_SYS_PTRACE */
+ uid = getuid();
+ if (uid == 0)
+ seteuid(USHRT_MAX);
+
+ pidfd = start_child(true, &child_pid);
+
+ fd = ioctl(pidfd, PIDFD_IOCTL_GETFD, WELL_KNOWN_CHILD_FD);
+ if (fd != -1)
+ ksft_exit_fail_msg("%s: getfd succeeded when ptrace blocked\n",
+ strerror(errno));
+ if (errno != EPERM)
+ ksft_exit_fail_msg("%s: getfd did not get EPERM\n",
+ strerror(errno));
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ if (uid == 0)
+ seteuid(0);
+
+ ksft_test_result_pass("no_ptrace\n");
+}
+
+static void test_unknown_fd(void)
+{
+ int fd, pidfd, ret;
+ pid_t child_pid;
+
+ pidfd = start_child(false, &child_pid);
+
+ fd = ioctl(pidfd, PIDFD_IOCTL_GETFD, UNKNOWN_FD);
+ if (fd != -1)
+ ksft_exit_fail_msg("%s: getfd succeeded when fetching unknown FD\n",
+ strerror(errno));
+ if (errno != EBADF)
+ ksft_exit_fail_msg("%s: getfd did not get EBADF\n",
+ strerror(errno));
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ ksft_test_result_pass("unknown_fd\n");
+}
+
+int main(int argc, char **argv)
+{
+ char buf[sizeof(SECRET_MESSAGE)];
+ int ret, status, fd, pidfd;
+ pid_t child_pid;
+
+ ksft_print_header();
+ ksft_set_plan(4);
+
+ test_kcmp_and_fetch_fd();
+ test_unknown_fd();
+ test_no_ptrace();
+
+ return ksft_exit_pass();
+}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/3] cgroup: unify attach permission checking
From: Christian Brauner @ 2019-12-19 0:39 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;
This needs to be
ret = 0
> +
> + 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;
and this
return ret;
^ permalink raw reply
* [PATCH] move_pages.2: remove ENOENT from the list of possible return values
From: John Hubbard @ 2019-12-19 5:13 UTC (permalink / raw)
To: Michael Kerrisk
Cc: linux-man, Andrew Morton, Christopher Lameter, linux-api, LKML,
linux-mm, John Hubbard, Michal Hocko, Brice Goglin, Yang Shi
Linux kernel commit e78bbfa82624 ("mm: stop returning -ENOENT from
sys_move_pages() if nothing got migrated") had the effect of *never*
returning -ENOENT, in any situation. So we need to update the man page
to reflect that ENOENT is not a possible return value.
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Brice Goglin <Brice.Goglin@inria.fr>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
Hi,
This fix for the man page was ACK'd by Michal, here:
https://lore.kernel.org/r/20191218101711.GB21485@dhcp22.suse.cz
thanks,
John Hubbard
NVIDIA
man2/move_pages.2 | 6 ------
1 file changed, 6 deletions(-)
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
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v4 2/5] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Arnd Bergmann @ 2019-12-19 8:03 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel@vger.kernel.org, Linux Containers, Linux API,
Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Christian Brauner, Oleg Nesterov, Andy Lutomirski,
Al Viro, gpascutto, ealvarez, Florian Weimer, jld
In-Reply-To: <20191218235459.GA17271@ircssh-2.c.rugged-nimbus-611.internal>
On Thu, Dec 19, 2019 at 12:55 AM Sargun Dhillon <sargun@sargun.me> wrote:
> +#define PIDFD_IOCTL_GETFD _IOWR('p', 0xb0, __u32)
This describes an ioctl command that reads and writes a __u32 variable
using a pointer passed as the argument, which doesn't match the
implementation:
> +static long pidfd_getfd(struct pid *pid, u32 fd)
> +{
...
> + return retfd;
This function passes an fd as the argument and returns a new
fd, so the command number would be
#define PIDFD_IOCTL_GETFD _IO('p', 0xb0)
While this implementation looks easy enough, and it is roughly what
I would do in case of a system call, I would recommend for an ioctl
implementation to use the __u32 pointer instead:
static long pidfd_getfd_ioctl(struct pid *pid, u32 __user *arg)
{
int their_fd, new_fd;
int ret;
ret = get_user(their_fd, arg);
if (ret)
return ret;
new_fd = pidfd_getfd(pid, their_fd);
if (new_fd < 0)
return new_fd;
return put_user(new_fd, arg);
}
Direct argument passing in ioctls may confuse readers because it
is fairly unusual, and it doesn't work with this:
> const struct file_operations pidfd_fops = {
> .release = pidfd_release,
> .poll = pidfd_poll,
> + .unlocked_ioctl = pidfd_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
compat_ptr_ioctl() only works if the argument is a pointer, as it
mangles the argument to turn it from a 32-bit pointer value into
a 64-bit pointer value. These are almost always the same
(arch/s390 being the sole exception), but you should not rely
on it. For now it would be find to do '.compat_ioctl = pidfd_ioctl',
but that in turn is wrong if you ever add another ioctl command
that does pass a pointer.
Arnd
^ permalink raw reply
* Re: [PATCH v4 2/5] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-19 10:23 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, christian.brauner, oleg, luto, viro, gpascutto, ealvarez,
fweimer, jld, arnd
In-Reply-To: <20191218235459.GA17271@ircssh-2.c.rugged-nimbus-611.internal>
On Wed, Dec 18, 2019 at 11:55:01PM +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.
I think that whole part can go now. :)
The rest is just nits (see below).
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> MAINTAINERS | 1 +
> include/uapi/linux/pidfd.h | 10 +++
> kernel/fork.c | 77 +++++++++++++++++++
> 4 files changed, 89 insertions(+)
> create mode 100644 include/uapi/linux/pidfd.h
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 4ef86433bd67..9f9be681662b 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -273,6 +273,7 @@ Code Seq# Include File Comments
> <mailto:tim@cyberelk.net>
> 'p' A1-A5 linux/pps.h LinuxPPS
> <mailto:giometti@linux.it>
> +'p' B0-CF linux/pidfd.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/MAINTAINERS b/MAINTAINERS
> index cc0a4a8ae06a..bc370ff59dbf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13014,6 +13014,7 @@ M: Christian Brauner <christian@brauner.io>
> L: linux-kernel@vger.kernel.org
> S: Maintained
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
> +F: include/uapi/linux/pidfd.h
> F: samples/pidfd/
> F: tools/testing/selftests/pidfd/
> F: tools/testing/selftests/clone3/
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> new file mode 100644
> index 000000000000..90ff535be048
> --- /dev/null
> +++ b/include/uapi/linux/pidfd.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_PID_H
> +#define _UAPI_LINUX_PID_H
I think that wants to be
#ifndef _UAPI_LINUX_PIDFD_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define PIDFD_IOCTL_GETFD _IOWR('p', 0xb0, __u32)
As Arnd mentioned, this defines a read-write ioctl where it just needs
to be a read-ioctl.
> +
> +#endif /* _UAPI_LINUX_PID_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2508a4f238a3..09b5f233b5a8 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -94,6 +94,7 @@
> #include <linux/thread_info.h>
> #include <linux/stackleak.h>
> #include <linux/kasan.h>
> +#include <uapi/linux/pidfd.h>
>
> #include <asm/pgtable.h>
> #include <asm/pgalloc.h>
> @@ -1790,9 +1791,85 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> return poll_flags;
> }
>
> +static struct file *__pidfd_getfd_fget_task(struct task_struct *task, u32 fd)
> +{
> + struct file *file;
> + int ret;
> +
> + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
> + file = ERR_PTR(-EPERM);
> + goto out;
> + }
> +
> + file = fget_task(task, fd);
> + if (!file)
> + file = ERR_PTR(-EBADF);
> +
> +out:
> + mutex_unlock(&task->signal->cred_guard_mutex);
> + return file;
> +}
> +
> +static long pidfd_getfd(struct pid *pid, u32 fd)
> +{
> + struct task_struct *task;
> + struct file *file;
> + int ret, retfd;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
> +
> + file = __pidfd_getfd_fget_task(task, fd);
> + put_task_struct(task);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + retfd = get_unused_fd_flags(O_CLOEXEC);
> + if (retfd < 0) {
> + ret = retfd;
> + goto out;
> + }
> +
> + /*
> + * 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(retfd, file);
> + return retfd;
> +
> +out_put_fd:
> + put_unused_fd(retfd);
> +out:
> + fput(file);
> + return ret;
> +}
> +
> +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pid *pid = file->private_data;
> +
> + switch (cmd) {
> + case PIDFD_IOCTL_GETFD:
> + return pidfd_getfd(pid, arg);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> const struct file_operations pidfd_fops = {
> .release = pidfd_release,
> .poll = pidfd_poll,
> + .unlocked_ioctl = pidfd_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = pidfd_show_fdinfo,
> #endif
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v4 2/5] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-19 10:35 UTC (permalink / raw)
To: Arnd Bergmann, Oleg Nesterov
Cc: Sargun Dhillon, linux-kernel@vger.kernel.org, Linux Containers,
Linux API, Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Christian Brauner, Andy Lutomirski, Al Viro,
gpascutto, ealvarez, Florian Weimer, jld
In-Reply-To: <CAK8P3a2eT=bHkUamyp-P3Y2adNq1KBk7UknCYBY5_aR4zJmYaQ@mail.gmail.com>
On Thu, Dec 19, 2019 at 09:03:09AM +0100, Arnd Bergmann wrote:
> On Thu, Dec 19, 2019 at 12:55 AM Sargun Dhillon <sargun@sargun.me> wrote:
>
> > +#define PIDFD_IOCTL_GETFD _IOWR('p', 0xb0, __u32)
>
> This describes an ioctl command that reads and writes a __u32 variable
> using a pointer passed as the argument, which doesn't match the
> implementation:
>
> > +static long pidfd_getfd(struct pid *pid, u32 fd)
> > +{
> ...
> > + return retfd;
>
> This function passes an fd as the argument and returns a new
> fd, so the command number would be
>
> #define PIDFD_IOCTL_GETFD _IO('p', 0xb0)
>
> While this implementation looks easy enough, and it is roughly what
> I would do in case of a system call, I would recommend for an ioctl
I guess this is the remaining question we should settle, i.e. what do we
prefer.
I still think that adding a new syscall for this seems a bit rich. On
the other hand it seems that a lot more people agree that using a
dedicated syscall instead of an ioctl is the correct way; especially
when it touches core kernel functionality. I mean that was one of the
takeaways from the pidfd API ioctl-vs-syscall discussion.
A syscall is nicer especially for core-kernel code like this.
So I guess the only way to find out is to try the syscall approach and
either get yelled and switch to an ioctl() or have it accepted.
What does everyone else think? Arnd, still in favor of a syscall I take
it. Oleg, you had suggested a syscall too, right? Florian, any
thoughts/worries on/about this from the glibc side?
Christian
^ permalink raw reply
* [PATCH 0/2] openat2: minor uapi cleanups
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, Florian Weimer, David Laight, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
While openat2(2) is still not yet in Linus's tree, we can take this
opportunity to iron out some small warts that weren't noticed earlier:
* A fix was suggested by Florian Weimer, to separate the openat2
definitions so glibc can use the header directly. I've put the
maintainership under VFS but let me know if you'd prefer it belong
ot the fcntl folks.
* Having heterogenous field sizes in an extensible struct results in
"padding hole" problems when adding new fields (in addition the
correct error to use for non-zero padding isn't entirely clear ).
The simplest solution is to just copy clone(3)'s model -- always use
u64s. It will waste a little more space in the struct, but it
removes a possible future headache.
Aleksa Sarai (2):
uapi: split openat2(2) definitions from fcntl.h
openat2: drop open_how->__padding field
MAINTAINERS | 1 +
fs/open.c | 2 -
include/uapi/linux/fcntl.h | 37 +----------------
include/uapi/linux/openat2.h | 40 +++++++++++++++++++
tools/testing/selftests/openat2/helpers.h | 3 +-
.../testing/selftests/openat2/openat2_test.c | 24 ++++-------
6 files changed, 51 insertions(+), 56 deletions(-)
create mode 100644 include/uapi/linux/openat2.h
base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
--
2.24.0
^ permalink raw reply
* [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, Florian Weimer, David Laight, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
Florian mentioned that glibc doesn't use fcntl.h because it has some
issues with namespace cleanliness, and that we should have a separate
header for openat2(2) if possible.
Suggested-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
MAINTAINERS | 1 +
include/uapi/linux/fcntl.h | 37 +-------------------------------
include/uapi/linux/openat2.h | 41 ++++++++++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 36 deletions(-)
create mode 100644 include/uapi/linux/openat2.h
diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e802de..737ada377ac3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6397,6 +6397,7 @@ F: fs/*
F: include/linux/fs.h
F: include/linux/fs_types.h
F: include/uapi/linux/fs.h
+F: include/uapi/linux/openat2.h
FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
M: Riku Voipio <riku.voipio@iki.fi>
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..ca88b7bce553 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -3,6 +3,7 @@
#define _UAPI_LINUX_FCNTL_H
#include <asm/fcntl.h>
+#include <linux/openat2.h>
#define F_SETLEASE (F_LINUX_SPECIFIC_BASE + 0)
#define F_GETLEASE (F_LINUX_SPECIFIC_BASE + 1)
@@ -100,40 +101,4 @@
#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 */
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
new file mode 100644
index 000000000000..19ef775e8e5e
--- /dev/null
+++ b/include/uapi/linux/openat2.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_OPENAT2_H
+#define _UAPI_LINUX_OPENAT2_H
+
+/*
+ * 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_OPENAT2_H */
--
2.24.0
^ permalink raw reply related
* [PATCH 2/2] openat2: drop open_how->__padding field
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, David Laight, Florian Weimer, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
The purpose of explicit padding was to allow us to use the space in the
future (C provides no guarantee about the value of padding bytes and
thus userspace could've provided garbage).
However, the downside of explicit padding is that any extension we wish
to add should fit the space exactly (otherwise we may end up with a u16
which will never be used). In addition, the correct error to return for
non-zero padding is not clear (-EINVAL doesn't imply "you're using an
extension field unsupported by this kernel", but -E2BIG seems a bit odd
if the structure size isn't different).
The simplest solution is to just match the design of clone3(2) -- use
u64s for all fields. The extra few-bytes cost of extra fields is not
significant (it's unlikely configuration structs will ever be extremely
large) and it allows for more flag space if necessary.
As openat2(2) is not yet in Linus's tree, we can iron out these minor
warts before we commit to this as a stable ABI.
Suggested-by: David Laight <david.laight@aculab.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 2 --
include/uapi/linux/openat2.h | 3 +--
tools/testing/selftests/openat2/helpers.h | 3 +--
.../testing/selftests/openat2/openat2_test.c | 24 +++++++------------
4 files changed, 10 insertions(+), 22 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..8cdb2b675867 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
- if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
- return -EINVAL;
/* Deal with the mode. */
if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index 19ef775e8e5e..76fad4ada2d4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -16,8 +16,7 @@
*/
struct open_how {
__aligned_u64 flags;
- __u16 mode;
- __u16 __padding[3]; /* must be zeroed */
+ __aligned_u64 mode;
__aligned_u64 resolve;
};
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..d756775d0725 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -37,8 +37,7 @@
*/
struct open_how {
__aligned_u64 flags;
- __u16 mode;
- __u16 __padding[3]; /* must be zeroed */
+ __aligned_u64 mode;
__aligned_u64 resolve;
};
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..b386367c606b 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -40,7 +40,7 @@ struct struct_test {
int err;
};
-#define NUM_OPENAT2_STRUCT_TESTS 10
+#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
void test_openat2_struct(void)
@@ -57,20 +57,6 @@ void test_openat2_struct(void)
.arg.inner.flags = O_RDONLY,
.size = sizeof(struct open_how_ext) },
- /* Normal struct with broken padding. */
- { .name = "normal struct (non-zero padding[0])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0xa0, 0x00, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[1])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x1a, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[2])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x00, 0xef},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
-
/* TODO: Once expanded, check zero-padding. */
/* Smaller than version-0 struct. */
@@ -169,7 +155,7 @@ struct flag_test {
int err;
};
-#define NUM_OPENAT2_FLAG_TESTS 21
+#define NUM_OPENAT2_FLAG_TESTS 23
void test_openat2_flags(void)
{
@@ -214,9 +200,15 @@ void test_openat2_flags(void)
{ .name = "invalid how.mode and O_CREAT",
.how.flags = O_CREAT,
.how.mode = 0xFFFF, .err = -EINVAL },
+ { .name = "invalid (very large) how.mode and O_CREAT",
+ .how.flags = O_CREAT,
+ .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
{ .name = "invalid how.mode and O_TMPFILE",
.how.flags = O_TMPFILE | O_RDWR,
.how.mode = 0x1337, .err = -EINVAL },
+ { .name = "invalid (very large) how.mode and O_TMPFILE",
+ .how.flags = O_TMPFILE | O_RDWR,
+ .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
/* ->resolve must only contain RESOLVE_* flags. */
{ .name = "invalid how.resolve and O_RDONLY",
--
2.24.0
^ permalink raw reply related
* [PATCH 0/2] openat2: minor uapi cleanups
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, Florian Weimer, David Laight, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
While openat2(2) is still not yet in Linus's tree, we can take this
opportunity to iron out some small warts that weren't noticed earlier:
* A fix was suggested by Florian Weimer, to separate the openat2
definitions so glibc can use the header directly. I've put the
maintainership under VFS but let me know if you'd prefer it belong
ot the fcntl folks.
* Having heterogenous field sizes in an extensible struct results in
"padding hole" problems when adding new fields (in addition the
correct error to use for non-zero padding isn't entirely clear ).
The simplest solution is to just copy clone(3)'s model -- always use
u64s. It will waste a little more space in the struct, but it
removes a possible future headache.
Aleksa Sarai (2):
uapi: split openat2(2) definitions from fcntl.h
openat2: drop open_how->__padding field
MAINTAINERS | 1 +
fs/open.c | 2 -
include/uapi/linux/fcntl.h | 37 +----------------
include/uapi/linux/openat2.h | 40 +++++++++++++++++++
tools/testing/selftests/openat2/helpers.h | 3 +-
.../testing/selftests/openat2/openat2_test.c | 24 ++++-------
6 files changed, 51 insertions(+), 56 deletions(-)
create mode 100644 include/uapi/linux/openat2.h
base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
--
2.24.0
^ permalink raw reply
* [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, Florian Weimer, David Laight, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
Florian mentioned that glibc doesn't use fcntl.h because it has some
issues with namespace cleanliness, and that we should have a separate
header for openat2(2) if possible.
Suggested-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
MAINTAINERS | 1 +
include/uapi/linux/fcntl.h | 37 +-------------------------------
include/uapi/linux/openat2.h | 41 ++++++++++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 36 deletions(-)
create mode 100644 include/uapi/linux/openat2.h
diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e802de..737ada377ac3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6397,6 +6397,7 @@ F: fs/*
F: include/linux/fs.h
F: include/linux/fs_types.h
F: include/uapi/linux/fs.h
+F: include/uapi/linux/openat2.h
FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
M: Riku Voipio <riku.voipio@iki.fi>
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..ca88b7bce553 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -3,6 +3,7 @@
#define _UAPI_LINUX_FCNTL_H
#include <asm/fcntl.h>
+#include <linux/openat2.h>
#define F_SETLEASE (F_LINUX_SPECIFIC_BASE + 0)
#define F_GETLEASE (F_LINUX_SPECIFIC_BASE + 1)
@@ -100,40 +101,4 @@
#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 */
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
new file mode 100644
index 000000000000..19ef775e8e5e
--- /dev/null
+++ b/include/uapi/linux/openat2.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_OPENAT2_H
+#define _UAPI_LINUX_OPENAT2_H
+
+/*
+ * 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_OPENAT2_H */
--
2.24.0
^ permalink raw reply related
* [PATCH 2/2] openat2: drop open_how->__padding field
From: Aleksa Sarai @ 2019-12-19 10:55 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan
Cc: Aleksa Sarai, David Laight, Florian Weimer, Christian Brauner,
dev, containers, libc-alpha, linux-api, linux-fsdevel,
linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
The purpose of explicit padding was to allow us to use the space in the
future (C provides no guarantee about the value of padding bytes and
thus userspace could've provided garbage).
However, the downside of explicit padding is that any extension we wish
to add should fit the space exactly (otherwise we may end up with a u16
which will never be used). In addition, the correct error to return for
non-zero padding is not clear (-EINVAL doesn't imply "you're using an
extension field unsupported by this kernel", but -E2BIG seems a bit odd
if the structure size isn't different).
The simplest solution is to just match the design of clone3(2) -- use
u64s for all fields. The extra few-bytes cost of extra fields is not
significant (it's unlikely configuration structs will ever be extremely
large) and it allows for more flag space if necessary.
As openat2(2) is not yet in Linus's tree, we can iron out these minor
warts before we commit to this as a stable ABI.
Suggested-by: David Laight <david.laight@aculab.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 2 --
include/uapi/linux/openat2.h | 3 +--
tools/testing/selftests/openat2/helpers.h | 3 +--
.../testing/selftests/openat2/openat2_test.c | 24 +++++++------------
4 files changed, 10 insertions(+), 22 deletions(-)
diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..8cdb2b675867 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
- if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
- return -EINVAL;
/* Deal with the mode. */
if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index 19ef775e8e5e..76fad4ada2d4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -16,8 +16,7 @@
*/
struct open_how {
__aligned_u64 flags;
- __u16 mode;
- __u16 __padding[3]; /* must be zeroed */
+ __aligned_u64 mode;
__aligned_u64 resolve;
};
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..d756775d0725 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -37,8 +37,7 @@
*/
struct open_how {
__aligned_u64 flags;
- __u16 mode;
- __u16 __padding[3]; /* must be zeroed */
+ __aligned_u64 mode;
__aligned_u64 resolve;
};
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..b386367c606b 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -40,7 +40,7 @@ struct struct_test {
int err;
};
-#define NUM_OPENAT2_STRUCT_TESTS 10
+#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
void test_openat2_struct(void)
@@ -57,20 +57,6 @@ void test_openat2_struct(void)
.arg.inner.flags = O_RDONLY,
.size = sizeof(struct open_how_ext) },
- /* Normal struct with broken padding. */
- { .name = "normal struct (non-zero padding[0])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0xa0, 0x00, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[1])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x1a, 0x00},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
- { .name = "normal struct (non-zero padding[2])",
- .arg.inner.flags = O_RDONLY,
- .arg.inner.__padding = {0x00, 0x00, 0xef},
- .size = sizeof(struct open_how_ext), .err = -EINVAL },
-
/* TODO: Once expanded, check zero-padding. */
/* Smaller than version-0 struct. */
@@ -169,7 +155,7 @@ struct flag_test {
int err;
};
-#define NUM_OPENAT2_FLAG_TESTS 21
+#define NUM_OPENAT2_FLAG_TESTS 23
void test_openat2_flags(void)
{
@@ -214,9 +200,15 @@ void test_openat2_flags(void)
{ .name = "invalid how.mode and O_CREAT",
.how.flags = O_CREAT,
.how.mode = 0xFFFF, .err = -EINVAL },
+ { .name = "invalid (very large) how.mode and O_CREAT",
+ .how.flags = O_CREAT,
+ .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
{ .name = "invalid how.mode and O_TMPFILE",
.how.flags = O_TMPFILE | O_RDWR,
.how.mode = 0x1337, .err = -EINVAL },
+ { .name = "invalid (very large) how.mode and O_TMPFILE",
+ .how.flags = O_TMPFILE | O_RDWR,
+ .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
/* ->resolve must only contain RESOLVE_* flags. */
{ .name = "invalid how.resolve and O_RDONLY",
--
2.24.0
^ permalink raw reply related
* Re: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h
From: Florian Weimer @ 2019-12-19 11:07 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
David Laight, Christian Brauner, dev, containers, libc-alpha,
linux-api, linux-fsdevel, linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-2-cyphar@cyphar.com>
* Aleksa Sarai:
> diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
> new file mode 100644
> index 000000000000..19ef775e8e5e
> --- /dev/null
> +++ b/include/uapi/linux/openat2.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_OPENAT2_H
> +#define _UAPI_LINUX_OPENAT2_H
I think you should include the relevant header for __align_u64
etc. here.
[…]
> + * 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
Are these really useful for the UAPI header? Is there a situation where
OPEN_HOW_SIZE_LATEST would be different from sizeof (struct open_how)?
The header is not compatible with the assembler anyway, so the numeric
constant does not seem useful.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH 2/2] openat2: drop open_how->__padding field
From: Christian Brauner @ 2019-12-19 11:18 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
David Laight, Florian Weimer, dev, containers, libc-alpha,
linux-api, linux-fsdevel, linux-kernel, linux-kselftest, arnd
In-Reply-To: <20191219105533.12508-3-cyphar@cyphar.com>
On Thu, Dec 19, 2019 at 09:55:30PM +1100, Aleksa Sarai wrote:
> The purpose of explicit padding was to allow us to use the space in the
> future (C provides no guarantee about the value of padding bytes and
> thus userspace could've provided garbage).
>
> However, the downside of explicit padding is that any extension we wish
> to add should fit the space exactly (otherwise we may end up with a u16
> which will never be used). In addition, the correct error to return for
> non-zero padding is not clear (-EINVAL doesn't imply "you're using an
> extension field unsupported by this kernel", but -E2BIG seems a bit odd
> if the structure size isn't different).
>
> The simplest solution is to just match the design of clone3(2) -- use
> u64s for all fields. The extra few-bytes cost of extra fields is not
> significant (it's unlikely configuration structs will ever be extremely
> large) and it allows for more flag space if necessary.
>
> As openat2(2) is not yet in Linus's tree, we can iron out these minor
> warts before we commit to this as a stable ABI.
>
> Suggested-by: David Laight <david.laight@aculab.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Fine with me.
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> fs/open.c | 2 --
> include/uapi/linux/openat2.h | 3 +--
> tools/testing/selftests/openat2/helpers.h | 3 +--
> .../testing/selftests/openat2/openat2_test.c | 24 +++++++------------
> 4 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/fs/open.c b/fs/open.c
> index 50a46501bcc9..8cdb2b675867 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
> return -EINVAL;
> if (how->resolve & ~VALID_RESOLVE_FLAGS)
> return -EINVAL;
> - if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
> - return -EINVAL;
>
> /* Deal with the mode. */
> if (WILL_CREATE(flags)) {
> diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
> index 19ef775e8e5e..76fad4ada2d4 100644
> --- a/include/uapi/linux/openat2.h
> +++ b/include/uapi/linux/openat2.h
> @@ -16,8 +16,7 @@
> */
> struct open_how {
> __aligned_u64 flags;
> - __u16 mode;
> - __u16 __padding[3]; /* must be zeroed */
> + __aligned_u64 mode;
> __aligned_u64 resolve;
> };
>
> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index 43ca5ceab6e3..d756775d0725 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -37,8 +37,7 @@
> */
> struct open_how {
> __aligned_u64 flags;
> - __u16 mode;
> - __u16 __padding[3]; /* must be zeroed */
> + __aligned_u64 mode;
> __aligned_u64 resolve;
> };
>
> diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> index 0b64fedc008b..b386367c606b 100644
> --- a/tools/testing/selftests/openat2/openat2_test.c
> +++ b/tools/testing/selftests/openat2/openat2_test.c
> @@ -40,7 +40,7 @@ struct struct_test {
> int err;
> };
>
> -#define NUM_OPENAT2_STRUCT_TESTS 10
> +#define NUM_OPENAT2_STRUCT_TESTS 7
> #define NUM_OPENAT2_STRUCT_VARIATIONS 13
>
> void test_openat2_struct(void)
> @@ -57,20 +57,6 @@ void test_openat2_struct(void)
> .arg.inner.flags = O_RDONLY,
> .size = sizeof(struct open_how_ext) },
>
> - /* Normal struct with broken padding. */
> - { .name = "normal struct (non-zero padding[0])",
> - .arg.inner.flags = O_RDONLY,
> - .arg.inner.__padding = {0xa0, 0x00, 0x00},
> - .size = sizeof(struct open_how_ext), .err = -EINVAL },
> - { .name = "normal struct (non-zero padding[1])",
> - .arg.inner.flags = O_RDONLY,
> - .arg.inner.__padding = {0x00, 0x1a, 0x00},
> - .size = sizeof(struct open_how_ext), .err = -EINVAL },
> - { .name = "normal struct (non-zero padding[2])",
> - .arg.inner.flags = O_RDONLY,
> - .arg.inner.__padding = {0x00, 0x00, 0xef},
> - .size = sizeof(struct open_how_ext), .err = -EINVAL },
> -
> /* TODO: Once expanded, check zero-padding. */
>
> /* Smaller than version-0 struct. */
> @@ -169,7 +155,7 @@ struct flag_test {
> int err;
> };
>
> -#define NUM_OPENAT2_FLAG_TESTS 21
> +#define NUM_OPENAT2_FLAG_TESTS 23
>
> void test_openat2_flags(void)
> {
> @@ -214,9 +200,15 @@ void test_openat2_flags(void)
> { .name = "invalid how.mode and O_CREAT",
> .how.flags = O_CREAT,
> .how.mode = 0xFFFF, .err = -EINVAL },
> + { .name = "invalid (very large) how.mode and O_CREAT",
> + .how.flags = O_CREAT,
> + .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
> { .name = "invalid how.mode and O_TMPFILE",
> .how.flags = O_TMPFILE | O_RDWR,
> .how.mode = 0x1337, .err = -EINVAL },
> + { .name = "invalid (very large) how.mode and O_TMPFILE",
> + .how.flags = O_TMPFILE | O_RDWR,
> + .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
>
> /* ->resolve must only contain RESOLVE_* flags. */
> { .name = "invalid how.resolve and O_RDONLY",
> --
> 2.24.0
>
^ permalink raw reply
* Re: [PATCH 0/2] openat2: minor uapi cleanups
From: Christian Brauner @ 2019-12-19 11:19 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
Florian Weimer, David Laight, dev, containers, libc-alpha,
linux-api, linux-fsdevel, linux-kernel, linux-kselftest
In-Reply-To: <20191219105533.12508-1-cyphar@cyphar.com>
On Thu, Dec 19, 2019 at 09:55:28PM +1100, Aleksa Sarai wrote:
> While openat2(2) is still not yet in Linus's tree, we can take this
> opportunity to iron out some small warts that weren't noticed earlier:
>
> * A fix was suggested by Florian Weimer, to separate the openat2
> definitions so glibc can use the header directly. I've put the
> maintainership under VFS but let me know if you'd prefer it belong
> ot the fcntl folks.
>
> * Having heterogenous field sizes in an extensible struct results in
> "padding hole" problems when adding new fields (in addition the
> correct error to use for non-zero padding isn't entirely clear ).
> The simplest solution is to just copy clone(3)'s model -- always use
> u64s. It will waste a little more space in the struct, but it
> removes a possible future headache.
Am I imagining things or did I get the same patch series twice?
Christian
^ permalink raw reply
* Re: [PATCH v4 2/5] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Arnd Bergmann @ 2019-12-19 11:31 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Florian Weimer, Sargun Dhillon,
linux-kernel@vger.kernel.org, Linux Containers, Linux API,
Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
Aleksa Sarai, Andy Lutomirski, Al Viro, gpascutto, ealvarez, jld
In-Reply-To: <20191219103525.yqb5f4pbd2dvztkb@wittgenstein>
On Thu, Dec 19, 2019 at 11:35 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> On Thu, Dec 19, 2019 at 09:03:09AM +0100, Arnd Bergmann wrote:
> > On Thu, Dec 19, 2019 at 12:55 AM Sargun Dhillon <sargun@sargun.me> wrote:
> What does everyone else think? Arnd, still in favor of a syscall I take it.
Yes, but I would not object the ioctl if others prefer that.
Arnd
^ permalink raw reply
* Re: [PATCH 0/2] openat2: minor uapi cleanups
From: Aleksa Sarai @ 2019-12-19 13:41 UTC (permalink / raw)
To: Christian Brauner
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
Florian Weimer, David Laight, dev, containers, libc-alpha,
linux-api, linux-fsdevel, linux-kernel, linux-kselftest
In-Reply-To: <20191219111949.auriw6biphxxvdng@wittgenstein>
[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]
On 2019-12-19, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Thu, Dec 19, 2019 at 09:55:28PM +1100, Aleksa Sarai wrote:
> > While openat2(2) is still not yet in Linus's tree, we can take this
> > opportunity to iron out some small warts that weren't noticed earlier:
> >
> > * A fix was suggested by Florian Weimer, to separate the openat2
> > definitions so glibc can use the header directly. I've put the
> > maintainership under VFS but let me know if you'd prefer it belong
> > ot the fcntl folks.
> >
> > * Having heterogenous field sizes in an extensible struct results in
> > "padding hole" problems when adding new fields (in addition the
> > correct error to use for non-zero padding isn't entirely clear ).
> > The simplest solution is to just copy clone(3)'s model -- always use
> > u64s. It will waste a little more space in the struct, but it
> > removes a possible future headache.
>
> Am I imagining things or did I get the same patch series twice?
Not unless it's a coincidence -- I accidentally ran
% git send-email *.patch [some flags] *.patch
--
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 1/2] uapi: split openat2(2) definitions from fcntl.h
From: Aleksa Sarai @ 2019-12-19 13:45 UTC (permalink / raw)
To: Florian Weimer
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
David Laight, Christian Brauner, dev, containers, libc-alpha,
linux-api, linux-fsdevel, linux-kernel, linux-kselftest
In-Reply-To: <87a77oy3oe.fsf@oldenburg2.str.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]
On 2019-12-19, Florian Weimer <fweimer@redhat.com> wrote:
> * Aleksa Sarai:
>
> > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
> > new file mode 100644
> > index 000000000000..19ef775e8e5e
> > --- /dev/null
> > +++ b/include/uapi/linux/openat2.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_LINUX_OPENAT2_H
> > +#define _UAPI_LINUX_OPENAT2_H
>
> I think you should include the relevant header for __align_u64
> etc. here.
Right -- no idea why I forgot to include them.
> […]
> > + * 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
>
> Are these really useful for the UAPI header? Is there a situation where
> OPEN_HOW_SIZE_LATEST would be different from sizeof (struct open_how)?
>
> The header is not compatible with the assembler anyway, so the numeric
> constant does not seem useful.
OPEN_HOW_SIZE_VER0 could conceivably be useful (in the future we may do
size-based checks) but maybe we can just expose it if someone actually
ends up needing it.
I will move them to the in-kernel header (we use them for BUILD_BUG_ONs
to make sure that the sizes are correct).
--
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 1/2] uapi: split openat2(2) definitions from fcntl.h
From: David Laight @ 2019-12-19 14:05 UTC (permalink / raw)
To: 'Aleksa Sarai', Florian Weimer
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
Christian Brauner, dev@opencontainers.org,
containers@lists.linux-foundation.org, libc-alpha@sourceware.org,
linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
In-Reply-To: <20191219134525.mgzmjbsp4wo5b2bw@yavin.dot.cyphar.com>
From: Aleksa Sarai
> Sent: 19 December 2019 13:45
> On 2019-12-19, Florian Weimer <fweimer@redhat.com> wrote:
> > * Aleksa Sarai:
> >
> > > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
> > > new file mode 100644
> > > index 000000000000..19ef775e8e5e
> > > --- /dev/null
> > > +++ b/include/uapi/linux/openat2.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +#ifndef _UAPI_LINUX_OPENAT2_H
> > > +#define _UAPI_LINUX_OPENAT2_H
> >
> > I think you should include the relevant header for __align_u64
> > etc. here.
>
> Right -- no idea why I forgot to include them.
I'm guessing that is just 64bit aligned on 32bit archs like x86?
No need to enforce it provided the structure will have no padding
on archs where the 64bit fields are 64bit aligned.
A plain __u64 should be fine.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ 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;
as well as URLs for NNTP newsgroup(s).