* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 17:39 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327053342.GA17484@kroah.com>
On Tue, Mar 26, 2019 at 10:33 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2019 at 10:29:41PM -0700, Andy Lutomirski wrote:
> >
> >
> > > On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > >> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> > >>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >>>
> > >>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > >>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > >>>> <matthewgarrett@google.com> wrote:
> > >>>>>
> > >>>>> From: Matthew Garrett <mjg59@google.com>
> > >>>>>
> > >>>>> debugfs has not been meaningfully audited in terms of ensuring that
> > >>>>> userland cannot trample over the kernel. At Greg's request, disable
> > >>>>> access to it entirely when the kernel is locked down. This is done at
> > >>>>> open() time rather than init time as the kernel lockdown status may be
> > >>>>> made stricter at runtime.
> > >>>>
> > >>>> Ugh. Some of those files are very useful. Could this perhaps still
> > >>>> allow O_RDONLY if we're in INTEGRITY mode?
> > >>>
> > >>> Useful for what? Debugging, sure, but for "normal operation", no kernel
> > >>> functionality should ever require debugfs. If it does, that's a bug and
> > >>> should be fixed.
> > >>>
> > >>
> > >> I semi-regularly read files in debugfs to diagnose things, and I think
> > >> it would be good for this to work on distro kernels.
> > >
> > > Doing that for debugging is wonderful. People who want this type of
> > > "lock down" are trading potential security for diagnositic ability.
> > >
> >
> > I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
>
> Reading a debugfs file can expose loads of things that can help take
> over a kernel, or at least make it easier. Pointer addresses, internal
> system state, loads of other fun things. And before 4.14 or so, it was
> pretty trivial to use it to oops the kernel as well (not an issue here
> anymore, but people are right to be nervous).
>
> Personally, I think these are all just "confidentiality" type things,
> but who really knows given the wild-west nature of debugfs (which is as
> designed). And given that I think this patch series just crazy anyway,
> I really don't care :)
>
As far as I'm concerned, preventing root from crashing the system
should not be a design goal of lockdown at all. And I think that the
"integrity" mode should be as non-annoying as possible, so I think we
should allow reading from debugfs.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Yann Droneaud @ 2019-03-27 17:21 UTC (permalink / raw)
To: Christian Brauner, jannh, khlebnikov, luto, dhowells, serge,
ebiederm, linux-api, linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol
In-Reply-To: <20190327162147.23198-3-christian@brauner.io>
Le mercredi 27 mars 2019 à 17:21 +0100, Christian Brauner a écrit :
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..c9e24e726aba 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -26,8 +26,10 @@
> +SYSCALL_DEFINE4(pidfd_open, pid_t, pid, int, procfd, int, pidfd,
> unsigned int,
> + flags)
> +{
> + long fd = -EINVAL;
> +
> + if (flags & ~(PIDFD_TO_PROCFD | PROCFD_TO_PIDFD))
> + return -EINVAL;
> +
> + if (!flags) {
> + struct pid *pidfd_pid;
> +
> + if (pid <= 0)
> + return -EINVAL;
> +
> + if (procfd != -1 || pidfd != -1)
> + return -EINVAL;
> +
> + rcu_read_lock();
> + pidfd_pid = get_pid(find_pid_ns(pid, task_active_pid_ns(current)));
> + rcu_read_unlock();
> +
> + fd = pidfd_create_fd(pidfd_pid, O_CLOEXEC);
> + put_pid(pidfd_pid);
> + } else if (flags & PIDFD_TO_PROCFD) {
[...]
> + } else if (flags & PROCFD_TO_PIDFD) {
> + if (flags & ~PROCFD_TO_PIDFD)
> + return -EINVAL;
> +
> + if (pid != -1)
> + return -EINVAL;
> +
> + if (pidfd >= 0)
>
I think it can be stricter with:
if (pidfd != -1)
(and match the check done for flag == 0).
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Jann Horn @ 2019-03-27 17:07 UTC (permalink / raw)
To: Christian Brauner
Cc: Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, kernel list,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Jonathan Kowalski, Dmitry V. Levin,
Andrew Morton, Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai,
Al Viro
In-Reply-To: <20190327162147.23198-3-christian@brauner.io>
On Wed, Mar 27, 2019 at 5:22 PM Christian Brauner <christian@brauner.io> wrote:
> pidfd_open() allows to retrieve pidfds for processes and removes the
> dependency of pidfd on procfs. Multiple people have expressed a desire to
> do this even when pidfd_send_signal() was merged. It is even recorded in
[...]
> IF PROCFD_TO_PIDFD is passed as a flag together with a file descriptor to a
> /proc mount in a given pid namespace and a pidfd pidfd_open() will return a
> file descriptor to the corresponding /proc/<pid> directory in procfs
> mounts' pid namespace. pidfd_open() is very careful to verify that the pid
nit: s/mounts'/mount's/
> hasn't been recycled in between.
> IF PIDFD_TO_PROCFD is passed as a flag together with a file descriptor
> referencing a /proc/<pid> directory a pidfd referencing the struct pid
> stashed in /proc/<pid> of the process will be returned.
nit: s/of the process //?
> The pidfd_open() syscalls in that manner resembles openat() as it uses a
nit: s/syscalls/syscall/
[...]
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..c9e24e726aba 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
[...]
> +static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid,
> + const struct pid *pidfd_pid)
> +{
> + char name[11]; /* int to strlen + \0 */
nit: The comment is a bit off; an unconstrained int needs 1+10+1
bytes, I think? minus sign, 10 digits, nullbyte? But of course that
can't actually happen here.
> + struct file *file;
> + struct pid *proc_pid;
> +
> + snprintf(name, sizeof(name), "%d", pid);
> + file = file_open_root(procf->f_path.dentry, procf->f_path.mnt, name,
> + O_DIRECTORY | O_NOFOLLOW, 0);
Maybe explicitly write the implied O_RDONLY (which is 0) here for clarity?
[...]
> +static int pidfd_to_procfd(pid_t pid, int procfd, int pidfd)
> +{
> + long fd;
> + pid_t ns_pid;
> + struct fd fdproc, fdpid;
> + struct file *file = NULL;
> + struct pid *pidfd_pid = NULL;
> + struct pid_namespace *proc_pid_ns = NULL;
> +
> + fdproc = fdget(procfd);
> + if (!fdproc.file)
> + return -EBADF;
> +
> + fdpid = fdget(pidfd);
> + if (!fdpid.file) {
> + fdput(fdpid);
Typo: s/fdput(fdpid)/fdput(fdproc)/
[...]
> +SYSCALL_DEFINE4(pidfd_open, pid_t, pid, int, procfd, int, pidfd, unsigned int,
> + flags)
[...]
> + if (!flags) {
[...]
> + rcu_read_lock();
> + pidfd_pid = get_pid(find_pid_ns(pid, task_active_pid_ns(current)));
> + rcu_read_unlock();
The previous three lines are equivalent to `pidfd_pid = find_get_pid(pid)`.
> + fd = pidfd_create_fd(pidfd_pid, O_CLOEXEC);
Nit: You could hardcode O_CLOEXEC in pidfd_create_fd() and get rid of
the second function argument if you want to.
> + put_pid(pidfd_pid);
> + } else if (flags & PIDFD_TO_PROCFD) {
[...]
> + fd = pidfd_to_procfd(pid, procfd, pidfd);
The `pid` argument of pidfd_to_procfd() looks unused, maybe it makes
sense to get rid of that?
^ permalink raw reply
* Re: [PATCH V31 19/25] x86/mmiotrace: Lock down the testmmiotrace module
From: Matthew Garrett @ 2019-03-27 16:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: James Morris, LSM List, Linux Kernel Mailing List, David Howells,
Linux API, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86
In-Reply-To: <20190327115749.5770a102@gandalf.local.home>
On Wed, Mar 27, 2019 at 8:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 26 Mar 2019 11:27:35 -0700
> Matthew Garrett <matthewgarrett@google.com> wrote:
>
> > From: David Howells <dhowells@redhat.com>
> >
> > The testmmiotrace module shouldn't be permitted when the kernel is locked
> > down as it can be used to arbitrarily read and write MMIO space. This is
> > a runtime check rather than buildtime in order to allow configurations
> > where the same kernel may be run in both locked down or permissive modes
> > depending on local policy.
> >
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> I'm curious. Should there be a mode to lockdown the tracefs directory
> too? As that can expose addresses.
That sounds like a reasonable thing to do in the confidentiality mode,
I don't think it'd be necessary in the integrity mode.
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: James Morris @ 2019-03-27 16:53 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Andy Lutomirski, Matthew Garrett, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327053342.GA17484@kroah.com>
On Wed, 27 Mar 2019, Greg KH wrote:
> Personally, I think these are all just "confidentiality" type things,
> but who really knows given the wild-west nature of debugfs (which is as
> designed). And given that I think this patch series just crazy anyway,
> I really don't care :)
Why do you think it's crazy?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [PATCH 4/4] tests: add pidfd_open() tests
From: Christian Brauner @ 2019-03-27 16:21 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327162147.23198-1-christian@brauner.io>
This adds a simple test case for pidfd_open().
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com
Cc: David Howells <dhowells@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
tools/testing/selftests/pidfd/Makefile | 2 +-
.../testing/selftests/pidfd/pidfd_open_test.c | 201 ++++++++++++++++++
2 files changed, 202 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index deaf8073bc06..b36c0be70848 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,6 +1,6 @@
CFLAGS += -g -I../../../../usr/include/
-TEST_GEN_PROGS := pidfd_test
+TEST_GEN_PROGS := pidfd_test pidfd_open_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
new file mode 100644
index 000000000000..07a262a9ef2c
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -0,0 +1,201 @@
+/* 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/mount.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "../kselftest.h"
+
+static inline int sys_pidfd_open(pid_t pid, int procfd, int pidfd,
+ unsigned int flags)
+{
+ return syscall(__NR_pidfd_open, pid, procfd, pidfd, flags);
+}
+
+static int safe_int(const char *numstr, int *converted)
+{
+ char *err = NULL;
+ signed long int sli;
+
+ errno = 0;
+ sli = strtol(numstr, &err, 0);
+ if (errno == ERANGE && (sli == LONG_MAX || sli == LONG_MIN))
+ return -ERANGE;
+
+ if (errno != 0 && sli == 0)
+ return -EINVAL;
+
+ if (err == numstr || *err != '\0')
+ return -EINVAL;
+
+ if (sli > INT_MAX || sli < INT_MIN)
+ return -ERANGE;
+
+ *converted = (int)sli;
+ return 0;
+}
+
+static int char_left_gc(const char *buffer, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t')
+ continue;
+
+ return i;
+ }
+
+ return 0;
+}
+
+static int char_right_gc(const char *buffer, size_t len)
+{
+ int i;
+
+ for (i = len - 1; i >= 0; i--) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t' ||
+ buffer[i] == '\n' ||
+ buffer[i] == '\0')
+ continue;
+
+ return i + 1;
+ }
+
+ return 0;
+}
+
+static char *trim_whitespace_in_place(char *buffer)
+{
+ buffer += char_left_gc(buffer, strlen(buffer));
+ buffer[char_right_gc(buffer, strlen(buffer))] = '\0';
+ return buffer;
+}
+
+static pid_t get_pid_from_status_file(int *fd)
+{
+ int ret;
+ FILE *f;
+ size_t n = 0;
+ pid_t result = -1;
+ char *line = NULL;
+
+ /* fd now belongs to FILE and will be closed by fclose() */
+ f = fdopen(*fd, "r");
+ if (!f)
+ return -1;
+
+ while (getline(&line, &n, f) != -1) {
+ char *numstr;
+
+ if (strncmp(line, "Pid:", 4))
+ continue;
+
+ numstr = trim_whitespace_in_place(line + 4);
+ ret = safe_int(numstr, &result);
+ if (ret < 0)
+ goto out;
+
+ break;
+ }
+
+out:
+ free(line);
+ fclose(f);
+ *fd = -1;
+ return result;
+}
+
+int main(int argc, char **argv)
+{
+ int ret = 1;
+ int pidfd = -1, pidfd2 = -1, procfd = -1, procpidfd = -1, statusfd = -1;
+ pid_t pid;
+
+ pidfd = sys_pidfd_open(getpid(), -1, -1, 0);
+ if (pidfd < 0) {
+ ksft_print_msg("%s - failed to open pidfd\n", strerror(errno));
+ goto on_error;
+ }
+
+ procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+ if (procfd < 0) {
+ ksft_print_msg("%s - failed to open /proc\n", strerror(errno));
+ goto on_error;
+ }
+
+ procpidfd = sys_pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
+ if (procpidfd < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve /proc/<pid> from pidfd\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ pidfd2 = sys_pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
+ if (pidfd2 < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve pidfd from procpidfd\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ statusfd = openat(procpidfd, "status", O_CLOEXEC | O_RDONLY);
+ if (statusfd < 0) {
+ ksft_print_msg("%s - failed to open /proc/<pid>/status\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ pid = get_pid_from_status_file(&statusfd);
+ if (pid < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve pid from /proc/<pid>/status\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ if (pid != getpid()) {
+ ksft_print_msg(
+ "%s - actual pid %d does not equal retrieved pid from /proc/<pid>/status\n",
+ strerror(errno), pid, getpid());
+ goto on_error;
+ }
+
+ ret = 0;
+
+on_error:
+ if (pidfd >= 0)
+ close(pidfd);
+
+ if (pidfd2 >= 0)
+ close(pidfd2);
+
+ if (procfd >= 0)
+ close(procfd);
+
+ if (procpidfd >= 0)
+ close(procpidfd);
+
+ if (statusfd >= 0)
+ close(statusfd);
+
+ return !ret ? ksft_exit_pass() : ksft_exit_fail();
+}
--
2.21.0
^ permalink raw reply related
* [PATCH 3/4] signal: support pidfd_open() with pidfd_send_signal()
From: Christian Brauner @ 2019-03-27 16:21 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327162147.23198-1-christian@brauner.io>
Let pidfd_send_signal() use pidfds retrieved via pidfd_open(). With this
patch pidfd_send_signal() becomes independent of procfs. This fullfils the
request made when we merged the pidfd_send_signal() patchset. The
pidfd_send_signal() syscall is now always available allowing for it to be
used by users without procfs mounted or even users without procfs support
compiled into the kernel.
Signed-off-by: Christian Brauner <christian@brauner.io>
Reviewed-by: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
kernel/signal.c | 14 ++++++++++----
kernel/sys_ni.c | 3 ---
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index b7953934aa99..eb97d0cc6ef7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
return kill_something_info(sig, &info, pid);
}
-#ifdef CONFIG_PROC_FS
/*
* Verify that the signaler and signalee either are in the same pid namespace
* or that the signaler's pid namespace is an ancestor of the signalee's pid
@@ -3550,6 +3549,14 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
return copy_siginfo_from_user(kinfo, info);
}
+static struct pid *pidfd_to_pid(const struct file *file)
+{
+ if (file->f_op == &pidfd_fops)
+ return file->private_data;
+
+ return tgid_pidfd_to_pid(file);
+}
+
/**
* sys_pidfd_send_signal - send a signal to a process through a task file
* descriptor
@@ -3581,12 +3588,12 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (flags)
return -EINVAL;
- f = fdget_raw(pidfd);
+ f = fdget(pidfd);
if (!f.file)
return -EBADF;
/* Is this a pidfd? */
- pid = tgid_pidfd_to_pid(f.file);
+ pid = pidfd_to_pid(f.file);
if (IS_ERR(pid)) {
ret = PTR_ERR(pid);
goto err;
@@ -3625,7 +3632,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
fdput(f);
return ret;
}
-#endif /* CONFIG_PROC_FS */
static int
do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index d21f4befaea4..4d9ae5ea6caf 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -167,9 +167,6 @@ COND_SYSCALL(syslog);
/* kernel/sched/core.c */
-/* kernel/signal.c */
-COND_SYSCALL(pidfd_send_signal);
-
/* kernel/sys.c */
COND_SYSCALL(setregid);
COND_SYSCALL(setgid);
--
2.21.0
^ permalink raw reply related
* [PATCH 2/4] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-27 16:21 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327162147.23198-1-christian@brauner.io>
pidfd_open() allows to retrieve pidfds for processes and removes the
dependency of pidfd on procfs. Multiple people have expressed a desire to
do this even when pidfd_send_signal() was merged. It is even recorded in
the commit message for pidfd_send_signal() itself
(cf. commit 3eb39f47934f9d5a3027fe00d906a45fe3a15fad):
Q-06: (Andrew Morton [1])
Is there a cleaner way of obtaining the fd? Another syscall perhaps.
A-06: Userspace can already trivially retrieve file descriptors from procfs
so this is something that we will need to support anyway. Hence,
there's no immediate need to add another syscalls just to make
pidfd_send_signal() not dependent on the presence of procfs. However,
adding a syscalls to get such file descriptors is planned for a
future patchset (cf. [1]).
Alexey made a similar request (cf. [2]). Additionally, Andy made an
argument that we should go forward with non-proc-dirfd file descriptors for
the sake of security and extensibility (cf. [3]).
This will unblock or help move along work on pidfd_wait which is currently
ongoing.
/* pidfds are anon inode file descriptors */
These pidfds are allocated using anon_inode_getfd(), are O_CLOEXEC by
default and can be used with the pidfd_send_signal() syscall. They are not
dirfds and as such have the advantage that we can make them pollable or
readable in the future if we see a need to do so. Currently they do not
support any advanced operations. The pidfds are not associated with a
specific pid namespaces but rather only reference struct pid of a given
process in their private_data member.
/* Process Metadata Access */
One of the oustanding issues has been how to get information about a given
process if pidfds are regular file descriptors and do not provide access to
the process /proc/<pid> directory.
Various solutions have been proposed. The one that most people prefer is to
be able to retrieve a file descriptor to /proc/<pid> based on a pidfd (and
the other way around).
IF PROCFD_TO_PIDFD is passed as a flag together with a file descriptor to a
/proc mount in a given pid namespace and a pidfd pidfd_open() will return a
file descriptor to the corresponding /proc/<pid> directory in procfs
mounts' pid namespace. pidfd_open() is very careful to verify that the pid
hasn't been recycled in between.
IF PIDFD_TO_PROCFD is passed as a flag together with a file descriptor
referencing a /proc/<pid> directory a pidfd referencing the struct pid
stashed in /proc/<pid> of the process will be returned.
The pidfd_open() syscalls in that manner resembles openat() as it uses a
flag argument to modify what type of file descriptor will be returned.
The pidfd_open() implementation together with the flags argument strikes me
as an elegant compromise between splitting this into multiple syscalls and
avoiding ioctls().
/* Examples */
// Retrieve pidfd
int pidfd = pidfd_open(1234, -1, -1, 0);
// Retrieve /proc/<pid> handle for pidfd
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
// Retrieve pidfd for /proc/<pid>
int procpidfd = open("/proc/1234", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int pidfd = pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
/* References */
[1]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
[2]: https://lore.kernel.org/lkml/20190320203910.GA2842@avx2/
[3]: https://lore.kernel.org/lkml/CALCETrXO=V=+qEdLDVPf8eCgLZiB9bOTrUfe0V-U-tUZoeoRDA@mail.gmail.com
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com
Cc: David Howells <dhowells@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/pid.h | 2 +
include/linux/syscalls.h | 2 +
include/uapi/linux/wait.h | 3 +
kernel/pid.c | 247 +++++++++++++++++++++++++
6 files changed, 256 insertions(+)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 1f9607ed087c..c8046f261bee 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -433,3 +433,4 @@
425 i386 io_uring_setup sys_io_uring_setup __ia32_sys_io_uring_setup
426 i386 io_uring_enter sys_io_uring_enter __ia32_sys_io_uring_enter
427 i386 io_uring_register sys_io_uring_register __ia32_sys_io_uring_register
+428 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 92ee0b4378d4..f714a3d57b88 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -349,6 +349,7 @@
425 common io_uring_setup __x64_sys_io_uring_setup
426 common io_uring_enter __x64_sys_io_uring_enter
427 common io_uring_register __x64_sys_io_uring_register
+428 common pidfd_open __x64_sys_pidfd_open
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/pid.h b/include/linux/pid.h
index b6f4ba16065a..3c8ef5a199ca 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -66,6 +66,8 @@ struct pid
extern struct pid init_struct_pid;
+extern const struct file_operations pidfd_fops;
+
static inline struct pid *get_pid(struct pid *pid)
{
if (pid)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e446806a561f..79b274698036 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -929,6 +929,8 @@ asmlinkage long sys_clock_adjtime32(clockid_t which_clock,
struct old_timex32 __user *tx);
asmlinkage long sys_syncfs(int fd);
asmlinkage long sys_setns(int fd, int nstype);
+asmlinkage long sys_pidfd_open(pid_t pid, int procfd, int pidfd,
+ unsigned int flags);
asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
unsigned int vlen, unsigned flags);
asmlinkage long sys_process_vm_readv(pid_t pid,
diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index ac49a220cf2a..8282fc19d8f6 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -18,5 +18,8 @@
#define P_PID 1
#define P_PGID 2
+/* Flags for pidfd_open */
+#define PIDFD_TO_PROCFD 1 /* retrieve file descriptor to /proc/<pid> for pidfd */
+#define PROCFD_TO_PIDFD 2 /* retrieve pidfd for /proc/<pid> */
#endif /* _UAPI_LINUX_WAIT_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 20881598bdfa..c9e24e726aba 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -26,8 +26,10 @@
*
*/
+#include <linux/anon_inodes.h>
#include <linux/mm.h>
#include <linux/export.h>
+#include <linux/fsnotify.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/rculist.h>
@@ -40,6 +42,7 @@
#include <linux/proc_fs.h>
#include <linux/sched/task.h>
#include <linux/idr.h>
+#include <linux/wait.h>
struct pid init_struct_pid = {
.count = ATOMIC_INIT(1),
@@ -451,6 +454,250 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+static int pidfd_release(struct inode *inode, struct file *file)
+{
+ struct pid *pid = file->private_data;
+
+ if (pid) {
+ file->private_data = NULL;
+ put_pid(pid);
+ }
+
+ return 0;
+}
+
+const struct file_operations pidfd_fops = {
+ .release = pidfd_release,
+};
+
+static int pidfd_create_fd(struct pid *pid, unsigned int o_flags)
+{
+ int fd;
+
+ fd = anon_inode_getfd("pidfd", &pidfd_fops, get_pid(pid), O_RDWR | o_flags);
+ if (fd < 0)
+ put_pid(pid);
+
+ return fd;
+}
+
+#ifdef CONFIG_PROC_FS
+static struct pid_namespace *pidfd_get_proc_pid_ns(const struct file *file)
+{
+ struct inode *inode;
+ struct super_block *sb;
+
+ inode = file_inode(file);
+ sb = inode->i_sb;
+ if (sb->s_magic != PROC_SUPER_MAGIC)
+ return ERR_PTR(-EINVAL);
+
+ if (inode->i_ino != PROC_ROOT_INO)
+ return ERR_PTR(-EINVAL);
+
+ return get_pid_ns(inode->i_sb->s_fs_info);
+}
+
+static struct pid *pidfd_get_pid(const struct file *file)
+{
+ if (file->f_op != &pidfd_fops)
+ return ERR_PTR(-EINVAL);
+
+ return get_pid(file->private_data);
+}
+
+static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid,
+ const struct pid *pidfd_pid)
+{
+ char name[11]; /* int to strlen + \0 */
+ struct file *file;
+ struct pid *proc_pid;
+
+ snprintf(name, sizeof(name), "%d", pid);
+ file = file_open_root(procf->f_path.dentry, procf->f_path.mnt, name,
+ O_DIRECTORY | O_NOFOLLOW, 0);
+ if (IS_ERR(file))
+ return file;
+
+ proc_pid = tgid_pidfd_to_pid(file);
+ if (IS_ERR(proc_pid)) {
+ filp_close(file, NULL);
+ return ERR_CAST(proc_pid);
+ }
+
+ if (pidfd_pid != proc_pid) {
+ filp_close(file, NULL);
+ return ERR_PTR(-ESRCH);
+ }
+
+ return file;
+}
+
+static int pidfd_to_procfd(pid_t pid, int procfd, int pidfd)
+{
+ long fd;
+ pid_t ns_pid;
+ struct fd fdproc, fdpid;
+ struct file *file = NULL;
+ struct pid *pidfd_pid = NULL;
+ struct pid_namespace *proc_pid_ns = NULL;
+
+ fdproc = fdget(procfd);
+ if (!fdproc.file)
+ return -EBADF;
+
+ fdpid = fdget(pidfd);
+ if (!fdpid.file) {
+ fdput(fdpid);
+ return -EBADF;
+ }
+
+ proc_pid_ns = pidfd_get_proc_pid_ns(fdproc.file);
+ if (IS_ERR(proc_pid_ns)) {
+ fd = PTR_ERR(proc_pid_ns);
+ proc_pid_ns = NULL;
+ goto err;
+ }
+
+ pidfd_pid = pidfd_get_pid(fdpid.file);
+ if (IS_ERR(pidfd_pid)) {
+ fd = PTR_ERR(pidfd_pid);
+ pidfd_pid = NULL;
+ goto err;
+ }
+
+ ns_pid = pid_nr_ns(pidfd_pid, proc_pid_ns);
+ if (!ns_pid) {
+ fd = -ESRCH;
+ goto err;
+ }
+
+ file = pidfd_open_proc_pid(fdproc.file, ns_pid, pidfd_pid);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ file = NULL;
+ goto err;
+ }
+
+ fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fd < 0)
+ goto err;
+
+ fsnotify_open(file);
+ fd_install(fd, file);
+ file = NULL;
+
+err:
+ fdput(fdproc);
+ fdput(fdpid);
+ if (proc_pid_ns)
+ put_pid_ns(proc_pid_ns);
+ put_pid(pidfd_pid);
+ if (file)
+ filp_close(file, NULL);
+
+ return fd;
+}
+
+static int procfd_to_pidfd(int procfd)
+{
+ int fd;
+ struct fd fdproc;
+ struct pid *proc_pid;
+
+ fdproc = fdget(procfd);
+ if (!fdproc.file)
+ return -EBADF;
+
+ proc_pid = tgid_pidfd_to_pid(fdproc.file);
+ if (IS_ERR(proc_pid)) {
+ fdput(fdproc);
+ return PTR_ERR(proc_pid);
+ }
+
+ fd = pidfd_create_fd(proc_pid, O_CLOEXEC);
+ fdput(fdproc);
+ return fd;
+}
+#else
+static inline int pidfd_to_procfd(pid_t pid, int procfd, int pidfd)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int procfd_to_pidfd(int procfd)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_PROC_FS */
+
+/*
+ * pidfd_open - open a pidfd
+ * @pid: pid for which to retrieve a pidfd
+ * @procfd: procfd file descriptor
+ * @pidfd: pidfd file descriptor
+ * @flags: flags to pass
+ *
+ * Creates a new pidfd or translates between pidfds and procfds.
+ * If no flag is passed, pidfd_open() will return a new pidfd for @pid. If
+ * PROCFD_TO_PIDFD is in @flags then a pidfd for struct pid referenced by
+ * @procfd is created. If PIDFD_TO_PROCFD is passed then a file descriptor to
+ * the process /proc/<pid> directory relative to the procfs referenced by
+ * @procfd will be returned.
+ */
+SYSCALL_DEFINE4(pidfd_open, pid_t, pid, int, procfd, int, pidfd, unsigned int,
+ flags)
+{
+ long fd = -EINVAL;
+
+ if (flags & ~(PIDFD_TO_PROCFD | PROCFD_TO_PIDFD))
+ return -EINVAL;
+
+ if (!flags) {
+ struct pid *pidfd_pid;
+
+ if (pid <= 0)
+ return -EINVAL;
+
+ if (procfd != -1 || pidfd != -1)
+ return -EINVAL;
+
+ rcu_read_lock();
+ pidfd_pid = get_pid(find_pid_ns(pid, task_active_pid_ns(current)));
+ rcu_read_unlock();
+
+ fd = pidfd_create_fd(pidfd_pid, O_CLOEXEC);
+ put_pid(pidfd_pid);
+ } else if (flags & PIDFD_TO_PROCFD) {
+ if (flags & ~PIDFD_TO_PROCFD)
+ return -EINVAL;
+
+ if (pid != -1)
+ return -EINVAL;
+
+ if (procfd < 0 || pidfd < 0)
+ return -EINVAL;
+
+ fd = pidfd_to_procfd(pid, procfd, pidfd);
+ } else if (flags & PROCFD_TO_PIDFD) {
+ if (flags & ~PROCFD_TO_PIDFD)
+ return -EINVAL;
+
+ if (pid != -1)
+ return -EINVAL;
+
+ if (pidfd >= 0)
+ return -EINVAL;
+
+ if (procfd < 0)
+ return -EINVAL;
+
+ fd = procfd_to_pidfd(procfd);
+ }
+
+ return fd;
+}
+
void __init pid_idr_init(void)
{
/* Verify no one has done anything silly: */
--
2.21.0
^ permalink raw reply related
* [PATCH 1/4] Make anon_inodes unconditional
From: Christian Brauner @ 2019-03-27 16:21 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327162147.23198-1-christian@brauner.io>
From: David Howells <dhowells@redhat.com>
Make the anon_inodes facility unconditional so that it can be used by core
VFS code and the pidfd_open() syscall.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[christian@brauner.io: adapt commit message to mention pidfd_open()]
Signed-off-by: Christian Brauner <christian@brauner.io>
---
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
init/Kconfig | 10 ----------
18 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 3f5320f46de2..f591026347a5 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -22,7 +22,6 @@ config KVM
bool "Kernel-based Virtual Machine (KVM) support"
depends on MMU && OF
select PREEMPT_NOTIFIERS
- select ANON_INODES
select ARM_GIC
select ARM_GIC_V3
select ARM_GIC_V3_ITS
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index a3f85624313e..a67121d419a2 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -23,7 +23,6 @@ config KVM
depends on OF
select MMU_NOTIFIER
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_ARCH_TLB_FLUSH_ALL
select KVM_MMIO
diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
index 4528bc9c3cb1..eac25aef21e0 100644
--- a/arch/mips/kvm/Kconfig
+++ b/arch/mips/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
depends on MIPS_FP_SUPPORT
select EXPORT_UASM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select KVM_GENERIC_DIRTYLOG_READ_PROTECT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select KVM_MMIO
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index bfdde04e4905..f53997a8ca62 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -20,7 +20,6 @@ if VIRTUALIZATION
config KVM
bool
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_EVENTFD
select HAVE_KVM_VCPU_ASYNC_IOCTL
select SRCU
diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 767453faacfc..1816ee48eadd 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
prompt "Kernel-based Virtual Machine (KVM) support"
depends on HAVE_KVM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select HAVE_KVM_EVENTFD
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3cf437c..18f2c954464e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -44,7 +44,6 @@ config X86
#
select ACPI_LEGACY_TABLES_LOOKUP if ACPI
select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
- select ANON_INODES
select ARCH_32BIT_OFF_T if X86_32
select ARCH_CLOCKSOURCE_DATA
select ARCH_CLOCKSOURCE_INIT
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 72fa955f4a15..fc042419e670 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -27,7 +27,6 @@ config KVM
depends on X86_LOCAL_APIC
select PREEMPT_NOTIFIERS
select MMU_NOTIFIER
- select ANON_INODES
select HAVE_KVM_IRQCHIP
select HAVE_KVM_IRQFD
select IRQ_BYPASS_MANAGER
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 059700ea3521..03f067da12ee 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -174,7 +174,6 @@ source "drivers/base/regmap/Kconfig"
config DMA_SHARED_BUFFER
bool
default n
- select ANON_INODES
select IRQ_WORK
help
This option enables the framework for buffer-sharing between
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 536e55d3919f..f3e4bc490cf0 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -157,7 +157,6 @@ config TCG_CRB
config TCG_VTPM_PROXY
tristate "VTPM Proxy Interface"
depends on TCG_TPM
- select ANON_INODES
---help---
This driver proxies for an emulated TPM (vTPM) running in userspace.
A device /dev/vtpmx is provided that creates a device pair
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 2e5a0faa2cb1..3fc9c2efc583 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -3,7 +3,6 @@ menu "DMABUF options"
config SYNC_FILE
bool "Explicit Synchronization Framework"
default n
- select ANON_INODES
select DMA_SHARED_BUFFER
---help---
The Sync File Framework adds explicit syncronization via
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..0f91600c27ae 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -12,7 +12,6 @@ config ARCH_HAVE_CUSTOM_GPIO_H
menuconfig GPIOLIB
bool "GPIO Support"
- select ANON_INODES
help
This enables GPIO support through the generic GPIO library.
You only need to enable this, if you also want to enable
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index d08aeb41cd07..1dec0fecb6ef 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -4,7 +4,6 @@
menuconfig IIO
tristate "Industrial I/O support"
- select ANON_INODES
help
The industrial I/O subsystem provides a unified framework for
drivers for many different types of embedded sensors using a
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index a1fb840de45d..d318bab25860 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -25,7 +25,6 @@ config INFINIBAND_USER_MAD
config INFINIBAND_USER_ACCESS
tristate "InfiniBand userspace access (verbs and CM)"
- select ANON_INODES
depends on MMU
---help---
Userspace InfiniBand access support. This enables the
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 9de5ed38da83..3798d77d131c 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -22,7 +22,6 @@ menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
- select ANON_INODES
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/vfio.txt for more details.
diff --git a/fs/Makefile b/fs/Makefile
index 427fec226fae..35945f8139e6 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -25,7 +25,7 @@ obj-$(CONFIG_PROC_FS) += proc_namespace.o
obj-y += notify/
obj-$(CONFIG_EPOLL) += eventpoll.o
-obj-$(CONFIG_ANON_INODES) += anon_inodes.o
+obj-y += anon_inodes.o
obj-$(CONFIG_SIGNALFD) += signalfd.o
obj-$(CONFIG_TIMERFD) += timerfd.o
obj-$(CONFIG_EVENTFD) += eventfd.o
diff --git a/fs/notify/fanotify/Kconfig b/fs/notify/fanotify/Kconfig
index 735bfb2e9190..521dc91d2cb5 100644
--- a/fs/notify/fanotify/Kconfig
+++ b/fs/notify/fanotify/Kconfig
@@ -1,7 +1,6 @@
config FANOTIFY
bool "Filesystem wide access notification"
select FSNOTIFY
- select ANON_INODES
select EXPORTFS
default n
---help---
diff --git a/fs/notify/inotify/Kconfig b/fs/notify/inotify/Kconfig
index b981fc0c8379..0161c74e76e2 100644
--- a/fs/notify/inotify/Kconfig
+++ b/fs/notify/inotify/Kconfig
@@ -1,6 +1,5 @@
config INOTIFY_USER
bool "Inotify support for userspace"
- select ANON_INODES
select FSNOTIFY
default y
---help---
diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..be8f97e37a76 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1171,9 +1171,6 @@ config LD_DEAD_CODE_DATA_ELIMINATION
config SYSCTL
bool
-config ANON_INODES
- bool
-
config HAVE_UID16
bool
@@ -1378,14 +1375,12 @@ config HAVE_FUTEX_CMPXCHG
config EPOLL
bool "Enable eventpoll support" if EXPERT
default y
- select ANON_INODES
help
Disabling this option will cause the kernel to be built without
support for epoll family of system calls.
config SIGNALFD
bool "Enable signalfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the signalfd() system call that allows to receive signals
@@ -1395,7 +1390,6 @@ config SIGNALFD
config TIMERFD
bool "Enable timerfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the timerfd() system call that allows to receive timer
@@ -1405,7 +1399,6 @@ config TIMERFD
config EVENTFD
bool "Enable eventfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the eventfd() system call that allows to receive both
@@ -1516,7 +1509,6 @@ config KALLSYMS_BASE_RELATIVE
# syscall, maps, verifier
config BPF_SYSCALL
bool "Enable bpf() system call"
- select ANON_INODES
select BPF
select IRQ_WORK
default n
@@ -1533,7 +1525,6 @@ config BPF_JIT_ALWAYS_ON
config USERFAULTFD
bool "Enable userfaultfd() system call"
- select ANON_INODES
depends on MMU
help
Enable the userfaultfd() system call that allows to intercept and
@@ -1600,7 +1591,6 @@ config PERF_EVENTS
bool "Kernel performance events and counters"
default y if PROFILING
depends on HAVE_PERF_EVENTS
- select ANON_INODES
select IRQ_WORK
select SRCU
help
--
2.21.0
^ permalink raw reply related
* [PATCH 0/4] pidfd_open()
From: Christian Brauner @ 2019-03-27 16:21 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
Hey,
After the discussion over the last days, this is a fresh approach to
getting pidfds independent of the translate_pid() patchset.
pidfd_open() allows to retrieve pidfds for processes and removes the
dependency of pidfd on procfs.
These pidfds are allocated using anon_inode_getfd(), are O_CLOEXEC by
default and can be used with the pidfd_send_signal() syscall. They are not
dirfds and as such have the advantage that we can make them pollable or
readable in the future if we see a need to do so. Currently they do not
support any advanced operations. The pidfds are not associated with a
specific pid namespaces but rather only reference struct pid of a given
process in their private_data member.
One of the oustanding issues has been how to get information about a given
process if pidfds are regular file descriptors and do not provide access to
the process /proc/<pid> directory.
Various solutions have been proposed. The one that most people prefer is to
be able to retrieve a file descriptor to /proc/<pid> based on a pidfd (and
the other way around).
IF PROCFD_TO_PIDFD is passed as a flag together with a file descriptor to a
/proc mount in a given pid namespace and a pidfd pidfd_open() will return a
file descriptor to the corresponding /proc/<pid> directory in procfs
mounts' pid namespace. pidfd_open() is very careful to verify that the pid
hasn't been recycled in between.
IF PIDFD_TO_PROCFD is passed as a flag together with a file descriptor
referencing a /proc/<pid> directory a pidfd referencing the struct pid
stashed in /proc/<pid> of the process will be returned.
The pidfd_open() syscalls in that manner resembles openat() as it uses a
flag argument to modify what type of file descriptor will be returned.
The pidfd_open() implementation together with the flags argument strikes me
as an elegant compromise between splitting this into multiple syscalls and
avoiding ioctls().
Note that this patchset also includes Al's and David's commit to make anon
inodes unconditional. The original intention is to make it possible to use
anon inodes in core vfs functions. pidctl() has the same requirement so
David suggested I sent this in alongside this patch. Both are informed of
this.
The syscall comes with appropriate basic testing.
/* Examples */
// Retrieve pidfd
int pidfd = pidfd_open(1234, -1, -1, 0);
// Retrieve /proc/<pid> handle for pidfd
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
// Retrieve pidfd for /proc/<pid>
int procpidfd = open("/proc/1234", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int pidfd = pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
Thanks!
Christian
Christian Brauner (3):
pid: add pidfd_open()
signal: support pidfd_open() with pidfd_send_signal()
tests: add pidfd_open() tests
David Howells (1):
Make anon_inodes unconditional
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
include/linux/pid.h | 2 +
include/linux/syscalls.h | 2 +
include/uapi/linux/wait.h | 3 +
init/Kconfig | 10 -
kernel/pid.c | 247 ++++++++++++++++++
kernel/signal.c | 14 +-
kernel/sys_ni.c | 3 -
tools/testing/selftests/pidfd/Makefile | 2 +-
.../testing/selftests/pidfd/pidfd_open_test.c | 201 ++++++++++++++
28 files changed, 469 insertions(+), 35 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
--
2.21.0
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Alexey Gladkov @ 2019-03-27 16:04 UTC (permalink / raw)
To: Jessica Yu
Cc: Masahiro Yamada, Michal Marek, Linux Kernel Mailing List,
Linux Kbuild mailing list, linux-api, Kirill A . Shutemov,
Gleb Fotengauer-Malinovskiy, Dmitry V. Levin, Dmitry Torokhov,
Rusty Russell, Lucas De Marchi
In-Reply-To: <20190327154025.GB23293@linux-8ccs>
On Wed, Mar 27, 2019 at 04:40:25PM +0100, Jessica Yu wrote:
> +++ Alexey Gladkov [26/03/19 18:24 +0100]:
> >On Fri, Mar 22, 2019 at 02:34:12PM +0900, Masahiro Yamada wrote:
> >> Hi.
> >>
> >> (added some people to CC)
>
> (Thanks Masahiro for the CC!)
>
> >>
> >> On Fri, Mar 15, 2019 at 7:10 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >> >
> >> > Problem:
> >> >
> >> > When a kernel module is compiled as a separate module, some important
> >> > information about the kernel module is available via .modinfo section of
> >> > the module. In contrast, when the kernel module is compiled into the
> >> > kernel, that information is not available.
> >>
> >>
> >> I might be missing something, but
> >> vmlinux provides info of builtin modules
> >> in /sys/module/.
> >
> >No. There are definitely not all modules. I have a builtin sha256_generic,
> >but I can't find him in the /sys/module.
>
> Yeah, you'll only find builtin modules under /sys/module/ if it has any module
> parameters, otherwise you won't find it there. As Masahiro already mentioned,
> if a builtin module has any parameters, they would be accessible under /sys/module/.
>
> >> (Looks like currently only module_param and MODULE_VERSION)
> >>
> >> This patch is not exactly the same, but I see a kind of overwrap.
> >> I'd like to be sure if we want this new scheme.
> >
> >The /sys/module is only for running kernel. One of my use cases is
> >to create an initrd for a new kernel.
> >
> >>
> >> > Information about built-in modules is necessary in the following cases:
> >> >
> >> > 1. When it is necessary to find out what additional parameters can be
> >> > passed to the kernel at boot time.
> >>
> >>
> >> Actually, /sys/module/<module>/parameters/
> >> exposes this information.
> >>
> >> Doesn't it work for your purpose?
> >
> >No, since creating an initrd needs to know all the modalias before
> >I get the sysfs for new kernel. Also there are no modalias at all.
> >
> >> > 2. When you need to know which module names and their aliases are in
> >> > the kernel. This is very useful for creating an initrd image.
> >> >
>
> Hm, I do see one possible additional use-case for preserving module alias
> information for built-in modules - modprobe will currently error (I think,
> correct me if I'm wrong) if we try invoking modprobe with an alias of a
> built-in module, simply because this information is not in modules.builtin or
> modules.alias.
Yes. Patch for modprobe in my todo list. The reason I didn’t do it was
because I wasn’t sure that the file format was final.
> Since kbuild already outputs modules.builtin, I would suggest outputting
> something like a modules.builtin.alias file (and I guess maybe a modules.builtin.param
> file too if that's deemed useful), in a format that is consumable by kmod/modprobe,
> so that modprobing an alias of a built-in module doesn't produce an error. I
> think this should be easy to do if we keep and parse the resulting .modinfo for
> builtin modules. This is just an idea, opinions welcome. I've added Lucas to CC
> in case he has any thoughts.
You don't like kernel.builtin.modinfo ?
It is much easier to create and it has almost the same format as the
modules. So I think it will be easier to parse in kmod.
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH V31 19/25] x86/mmiotrace: Lock down the testmmiotrace module
From: Steven Rostedt @ 2019-03-27 15:57 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, dhowells, linux-api,
luto, Thomas Gleixner, Matthew Garrett, Ingo Molnar,
H. Peter Anvin, x86
In-Reply-To: <20190326182742.16950-20-matthewgarrett@google.com>
On Tue, 26 Mar 2019 11:27:35 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> From: David Howells <dhowells@redhat.com>
>
> The testmmiotrace module shouldn't be permitted when the kernel is locked
> down as it can be used to arbitrarily read and write MMIO space. This is
> a runtime check rather than buildtime in order to allow configurations
> where the same kernel may be run in both locked down or permissive modes
> depending on local policy.
>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
I'm curious. Should there be a mode to lockdown the tracefs directory
too? As that can expose addresses.
-- Steve
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: David Howells <dhowells@redhat.com
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Steven Rostedt <rostedt@goodmis.org>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: "H. Peter Anvin" <hpa@zytor.com>
> cc: x86@kernel.org
> ---
> arch/x86/mm/testmmiotrace.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c
> index f6ae6830b341..9e8ad665f354 100644
> --- a/arch/x86/mm/testmmiotrace.c
> +++ b/arch/x86/mm/testmmiotrace.c
> @@ -115,6 +115,9 @@ static int __init init(void)
> {
> unsigned long size = (read_far) ? (8 << 20) : (16 << 10);
>
> + if (kernel_is_locked_down("MMIO trace testing", LOCKDOWN_INTEGRITY))
> + return -EPERM;
> +
> if (mmio_address == 0) {
> pr_err("you have to use the module argument mmio_address.\n");
> pr_err("DO NOT LOAD THIS MODULE UNLESS YOU REALLY KNOW WHAT YOU ARE DOING!\n");
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Jessica Yu @ 2019-03-27 15:40 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Masahiro Yamada, Michal Marek, Linux Kernel Mailing List,
Linux Kbuild mailing list, linux-api, Kirill A . Shutemov,
Gleb Fotengauer-Malinovskiy, Dmitry V. Levin, Dmitry Torokhov,
Rusty Russell, Lucas De Marchi
In-Reply-To: <20190326172411.GA15936@Legion-PC.fortress>
+++ Alexey Gladkov [26/03/19 18:24 +0100]:
>On Fri, Mar 22, 2019 at 02:34:12PM +0900, Masahiro Yamada wrote:
>> Hi.
>>
>> (added some people to CC)
(Thanks Masahiro for the CC!)
>>
>> On Fri, Mar 15, 2019 at 7:10 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>> >
>> > Problem:
>> >
>> > When a kernel module is compiled as a separate module, some important
>> > information about the kernel module is available via .modinfo section of
>> > the module. In contrast, when the kernel module is compiled into the
>> > kernel, that information is not available.
>>
>>
>> I might be missing something, but
>> vmlinux provides info of builtin modules
>> in /sys/module/.
>
>No. There are definitely not all modules. I have a builtin sha256_generic,
>but I can't find him in the /sys/module.
Yeah, you'll only find builtin modules under /sys/module/ if it has any module
parameters, otherwise you won't find it there. As Masahiro already mentioned,
if a builtin module has any parameters, they would be accessible under /sys/module/.
>> (Looks like currently only module_param and MODULE_VERSION)
>>
>> This patch is not exactly the same, but I see a kind of overwrap.
>> I'd like to be sure if we want this new scheme.
>
>The /sys/module is only for running kernel. One of my use cases is
>to create an initrd for a new kernel.
>
>>
>> > Information about built-in modules is necessary in the following cases:
>> >
>> > 1. When it is necessary to find out what additional parameters can be
>> > passed to the kernel at boot time.
>>
>>
>> Actually, /sys/module/<module>/parameters/
>> exposes this information.
>>
>> Doesn't it work for your purpose?
>
>No, since creating an initrd needs to know all the modalias before
>I get the sysfs for new kernel. Also there are no modalias at all.
>
>> > 2. When you need to know which module names and their aliases are in
>> > the kernel. This is very useful for creating an initrd image.
>> >
Hm, I do see one possible additional use-case for preserving module alias
information for built-in modules - modprobe will currently error (I think,
correct me if I'm wrong) if we try invoking modprobe with an alias of a
built-in module, simply because this information is not in modules.builtin or
modules.alias.
Since kbuild already outputs modules.builtin, I would suggest outputting
something like a modules.builtin.alias file (and I guess maybe a modules.builtin.param
file too if that's deemed useful), in a format that is consumable by kmod/modprobe,
so that modprobing an alias of a built-in module doesn't produce an error. I
think this should be easy to do if we keep and parse the resulting .modinfo for
builtin modules. This is just an idea, opinions welcome. I've added Lucas to CC
in case he has any thoughts.
Thanks,
Jessica
^ permalink raw reply
* Re: [PATCH 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v7)
From: Martin Schwidefsky @ 2019-03-27 9:16 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Carlos O'Donell, Paul Burton, Will Deacon, Boqun Feng,
Heiko Carstens, Vasily Gorbik, Russell King,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, carlos,
Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Dave Watson <davejwatson>
In-Reply-To: <1965431879.7576.1553529272844.JavaMail.zimbra@efficios.com>
On Mon, 25 Mar 2019 11:54:32 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >> +++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h
> [...]
> >> +
> >> +/* Signature required before each abort handler code. */
> >> +#define RSEQ_SIG 0x53053053
> >
> > Why not a s390 specific value here?
>
> s390 also has the abort handler in a __rseq_failure section:
>
> #define RSEQ_ASM_DEFINE_ABORT(label, teardown, abort_label) \
> ".pushsection __rseq_failure, \"ax\"\n\t" \
> ".long " __rseq_str(RSEQ_SIG) "\n\t" \
> __rseq_str(label) ":\n\t" \
> teardown \
> "j %l[" __rseq_str(abort_label) "]\n\t" \
> ".popsection\n\t"
>
> Same question applies as powerpc: since disassemblers will try to decode
> that instruction, would it be better to define it as a valid one ?
>
> [...]
A 4-byte sequence starting with 0x53 is decoded as a "diebr" instruction.
And please replace that "j %l[...]" with a "jg %l[...]", the branch target
range of the "j" instruction is 64K, not enough for the general case.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-03-27 7:10 UTC (permalink / raw)
To: Scott Wood
Cc: atull, mdf, linux-fpga, linux-kernel, linux-api, Ananda Ravuri,
Xu Yilun
In-Reply-To: <fd78636fb37cfcb725b22bb6a5f7576fa61a00a7.camel@redhat.com>
On Wed, Mar 27, 2019 at 01:19:29AM -0500, Scott Wood wrote:
> On Wed, 2019-03-27 at 13:10 +0800, Wu Hao wrote:
> > On Mon, Mar 25, 2019 at 05:58:36PM -0500, Scott Wood wrote:
> > > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > > In early partial reconfiguration private feature, it only
> > > > > supports 32bit data width when writing data to hardware for
> > > > > PR. 512bit data width PR support is an important optimization
> > > > > for some specific solutions (e.g. XEON with FPGA integrated),
> > > > > it allows driver to use AVX512 instruction to improve the
> > > > > performance of partial reconfiguration. e.g. programming one
> > > > > 100MB bitstream image via this 512bit data width PR hardware
> > > > > only takes ~300ms, but 32bit revision requires ~3s per test
> > > > > result.
> > > > >
> > > > > Please note now this optimization is only done on revision 2
> > > > > of this PR private feature which is only used in integrated
> > > > > solution that AVX512 is always supported.
> > > > >
> > > > > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > > ---
> > > > > drivers/fpga/dfl-fme-main.c | 3 ++
> > > > > drivers/fpga/dfl-fme-mgr.c | 75
> > > > > +++++++++++++++++++++++++++++++++++++-
> > > > > --
> > > > > -----
> > > > > drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> > > > > drivers/fpga/dfl-fme.h | 2 ++
> > > > > drivers/fpga/dfl.h | 5 +++
> > > > > 5 files changed, 99 insertions(+), 31 deletions(-)
> > > > >
> > > > > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-
> > > > > main.c
> > > > > index 086ad24..076d74f 100644
> > > > > --- a/drivers/fpga/dfl-fme-main.c
> > > > > +++ b/drivers/fpga/dfl-fme-main.c
> > > > > @@ -21,6 +21,8 @@
> > > > > #include "dfl.h"
> > > > > #include "dfl-fme.h"
> > > > >
> > > > > +#define DRV_VERSION "0.8"
> > > >
> > > > What is this going to be used for? Under what circumstances will the
> > > > driver version be bumped? What does it have to do with 512-bit
> > > > writes?
> >
> > This patchset adds more features to this driver, so i would like to add
> > a DRV_VERSION there as an initial one. In the future, if some new features
> > or extensions for existing features (e.g. new revision of a private
> > feature)
> > are added we need to bump this version.
>
> This doesn't seem like a good way of advertising API availability... Besides
> being awkward to query, what happens if a distro kernel has backported some
> features but not others that came before? What does it advertise?
DRV_VERSION here is not used for API availablity. :)
> I'd suggest some sort of feature flag mechanism that can be queried via
> ioctl (e.g. along the lines of KVM capabilities), if "try the API and fall
> back if it fails" is unsatisfactory.
>
> Plus, if it's about new APIs being exposed, this doesn't seem like the right
> patch for it to be in...
Actually this patch doesn't introduce new APIs, I am trying to make this
transparent to endusers. That means users don't need to know it's a 32bit
PR or a faster 512bit one, they still use the same IOCTL interface for PR.
the API_VERSION and CHECK_EXTENSION ioctls have been defined, but I think
at least we don't need to bump them for this change. How do you think?
>
> > > Sorry, I missed the comment about revision 2 only being on integrated
> > > devices -- but will that always be the case? Seems worthwhile to check
> > > for
> > > AVX512 support anyway. And there's still the possibility of being built
> > > with an old binutils such that CONFIG_AS_AVX512 is not set, or running
> > > on a
> > > kernel where avx512 was disabled via a boot option.
> > >
> > > What about future revisions >= 2? Currently the driver will treat them
> > > as
> > > if they were revision < 2. Is that intended?
> >
> > Yes, it's intended. Currently we don't have any hardware with revisions >
> > 2,
> > and support new revisions may need new code. :) e.g. currently revision
> > is
> > used to tell 32bit vs 512bit PR, but in future revisions, it may have new
> > capability registers for this purpose.
>
> The driver should refuse to bind to unrecognized revisions, if they're not
> expected to be compatible.
Yes, agree.
Thanks
Hao
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-03-27 6:19 UTC (permalink / raw)
To: Wu Hao
Cc: atull, mdf, linux-fpga, linux-kernel, linux-api, Ananda Ravuri,
Xu Yilun
In-Reply-To: <20190327051040.GB20968@hao-dev>
On Wed, 2019-03-27 at 13:10 +0800, Wu Hao wrote:
> On Mon, Mar 25, 2019 at 05:58:36PM -0500, Scott Wood wrote:
> > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > In early partial reconfiguration private feature, it only
> > > > supports 32bit data width when writing data to hardware for
> > > > PR. 512bit data width PR support is an important optimization
> > > > for some specific solutions (e.g. XEON with FPGA integrated),
> > > > it allows driver to use AVX512 instruction to improve the
> > > > performance of partial reconfiguration. e.g. programming one
> > > > 100MB bitstream image via this 512bit data width PR hardware
> > > > only takes ~300ms, but 32bit revision requires ~3s per test
> > > > result.
> > > >
> > > > Please note now this optimization is only done on revision 2
> > > > of this PR private feature which is only used in integrated
> > > > solution that AVX512 is always supported.
> > > >
> > > > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > ---
> > > > drivers/fpga/dfl-fme-main.c | 3 ++
> > > > drivers/fpga/dfl-fme-mgr.c | 75
> > > > +++++++++++++++++++++++++++++++++++++-
> > > > --
> > > > -----
> > > > drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> > > > drivers/fpga/dfl-fme.h | 2 ++
> > > > drivers/fpga/dfl.h | 5 +++
> > > > 5 files changed, 99 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-
> > > > main.c
> > > > index 086ad24..076d74f 100644
> > > > --- a/drivers/fpga/dfl-fme-main.c
> > > > +++ b/drivers/fpga/dfl-fme-main.c
> > > > @@ -21,6 +21,8 @@
> > > > #include "dfl.h"
> > > > #include "dfl-fme.h"
> > > >
> > > > +#define DRV_VERSION "0.8"
> > >
> > > What is this going to be used for? Under what circumstances will the
> > > driver version be bumped? What does it have to do with 512-bit
> > > writes?
>
> This patchset adds more features to this driver, so i would like to add
> a DRV_VERSION there as an initial one. In the future, if some new features
> or extensions for existing features (e.g. new revision of a private
> feature)
> are added we need to bump this version.
This doesn't seem like a good way of advertising API availability... Besides
being awkward to query, what happens if a distro kernel has backported some
features but not others that came before? What does it advertise?
I'd suggest some sort of feature flag mechanism that can be queried via
ioctl (e.g. along the lines of KVM capabilities), if "try the API and fall
back if it fails" is unsatisfactory.
Plus, if it's about new APIs being exposed, this doesn't seem like the right
patch for it to be in...
> > Sorry, I missed the comment about revision 2 only being on integrated
> > devices -- but will that always be the case? Seems worthwhile to check
> > for
> > AVX512 support anyway. And there's still the possibility of being built
> > with an old binutils such that CONFIG_AS_AVX512 is not set, or running
> > on a
> > kernel where avx512 was disabled via a boot option.
> >
> > What about future revisions >= 2? Currently the driver will treat them
> > as
> > if they were revision < 2. Is that intended?
>
> Yes, it's intended. Currently we don't have any hardware with revisions >
> 2,
> and support new revisions may need new code. :) e.g. currently revision
> is
> used to tell 32bit vs 512bit PR, but in future revisions, it may have new
> capability registers for this purpose.
The driver should refuse to bind to unrecognized revisions, if they're not
expected to be compatible.
-Scott
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Scott Wood @ 2019-03-27 6:10 UTC (permalink / raw)
To: Wu Hao
Cc: Alan Tull, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <20190327043743.GA20968@hao-dev>
On Wed, 2019-03-27 at 12:37 +0800, Wu Hao wrote:
> On Tue, Mar 26, 2019 at 04:22:34PM -0500, Scott Wood wrote:
> > On Tue, 2019-03-26 at 14:33 -0500, Alan Tull wrote:
> > > On Mon, Mar 25, 2019 at 5:58 PM Scott Wood <swood@redhat.com> wrote:
> > > > >
> > > Hi Scott,
> > >
> > > > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > > > +#else
> > > > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > > > +{
> > > > > > + WARN_ON_ONCE(1);
> > > > > > +}
> > > > > > +#endif
> > > > >
Likewise, this will be called if a revision 2 device is used on non-
> > > > > x86
> > > > > (or on x86 with an old binutils). The driver should fall back to
> > > > > 32-
> > > > > bit
> > > > > in such cases.
>
> Unfortunately revision 2 is only for integrated FPGA solution, and it
> doesn't
> support any fallback solution (original 32bit data partial reconfiguration
> is
> not supported any more), so driver has to WARN in such path.
>From the commit message it seemed like this was just an optimization, not
something necessary to support revision 2.
If there's no way to program the device without AVX512, then printing an
error message and returning an error to userspace would be better than
WARN_ON, since it's not actually a kernel bug.
-Scott
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-03-27 6:03 UTC (permalink / raw)
To: Scott Wood
Cc: Alan Tull, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <29827fcdcdd4fb46b680a6bd184b44fca638666e.camel@redhat.com>
On Wed, Mar 27, 2019 at 01:10:31AM -0500, Scott Wood wrote:
> On Wed, 2019-03-27 at 12:37 +0800, Wu Hao wrote:
> > On Tue, Mar 26, 2019 at 04:22:34PM -0500, Scott Wood wrote:
> > > On Tue, 2019-03-26 at 14:33 -0500, Alan Tull wrote:
> > > > On Mon, Mar 25, 2019 at 5:58 PM Scott Wood <swood@redhat.com> wrote:
> > > > > >
> > > > Hi Scott,
> > > >
> > > > > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > > > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > > > > +#else
> > > > > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > > > > +{
> > > > > > > + WARN_ON_ONCE(1);
> > > > > > > +}
> > > > > > > +#endif
> > > > > >
> Likewise, this will be called if a revision 2 device is used on non-
> > > > > > x86
> > > > > > (or on x86 with an old binutils). The driver should fall back to
> > > > > > 32-
> > > > > > bit
> > > > > > in such cases.
> >
> > Unfortunately revision 2 is only for integrated FPGA solution, and it
> > doesn't
> > support any fallback solution (original 32bit data partial reconfiguration
> > is
> > not supported any more), so driver has to WARN in such path.
>
> >From the commit message it seemed like this was just an optimization, not
> something necessary to support revision 2.
>
> If there's no way to program the device without AVX512, then printing an
> error message and returning an error to userspace would be better than
> WARN_ON, since it's not actually a kernel bug.
Fair enough. Will do. Thanks for the suggestion.
Hao
>
> -Scott
>
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-03-27 5:46 UTC (permalink / raw)
To: Scott Wood
Cc: atull, mdf, linux-fpga, linux-kernel, linux-api, Ananda Ravuri,
Xu Yilun
In-Reply-To: <127a9356a7bf597d35dd361f2b16bf80460f0370.camel@redhat.com>
On Mon, Mar 25, 2019 at 05:53:50PM -0500, Scott Wood wrote:
> On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > @@ -200,21 +228,32 @@ static int fme_mgr_write(struct fpga_manager *mgr,
> > pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT,
> > pr_status);
> > }
> >
> > - if (count < 4) {
> > + if (count < priv->pr_datawidth) {
> > dev_err(dev, "Invalid PR bitstream size\n");
> > return -EINVAL;
>
> Shouldn't this have become a WARN_ON in patch 2 given that the kernel
> already pads the buffer?
Thanks a lot for the review and comments.
I agree. it's better to use WARN_ON this place.
>
> > }
> >
> > - pr_data = 0;
> > - pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> > - *(((u32 *)buf) + i));
> > - writeq(pr_data, fme_pr + FME_PR_DATA);
> > - count -= 4;
> > + switch (priv->pr_datawidth) {
> > + case 4:
> > + pr_data = 0;
> > + pr_data |= FIELD_PREP(FME_PR_DATA_PR_DATA_RAW,
> > + *((u32 *)buf));
>
> I know it's not new, but why not just "pr_data = FIELD..."? Const should
> also be preserved in the cast, and you can drop one set of parentheses.
Yes, agree, will fix this.
>
> > + writeq(pr_data, fme_pr + FME_PR_DATA);
> > + break;
> > + case 64:
> > + copy512((void *)buf, fme_pr + FME_PR_512_DATA);
> > + break;
>
> Unnecessary cast.
Will fix this.
>
> > + default:
> > + ret = -EFAULT;
> > + goto done;
>
> How is it EFAULT? Any other value for pr_datawidth should be WARN_ON
> since it's set by kernel code.
Agree, will fix this in the next version.
>
> > @@ -159,13 +161,10 @@ static int fme_pr(struct platform_device *pdev,
> > unsigned long arg)
> > fpga_bridges_put(®ion->bridge_list);
> >
> > put_device(®ion->dev);
> > -unlock_exit:
> > - mutex_unlock(&pdata->lock);
> > free_exit:
> > vfree(buf);
> > - if (copy_to_user((void __user *)arg, &port_pr, minsz))
> > - return -EFAULT;
> > -
>
> Why is the copy_to_user being removed?
This code is not needed at all but added by mistake i think.
Sorry, i should move these code into a separated patch with proper comments
to avoid confusion.
Thanks
Hao
>
> -Scott
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 5:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <16124107-70D3-4CA0-9766-36FC6DC10128@amacapital.net>
On Tue, Mar 26, 2019 at 10:29:41PM -0700, Andy Lutomirski wrote:
>
>
> > On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> >> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> >>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>
> >>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> >>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> >>>> <matthewgarrett@google.com> wrote:
> >>>>>
> >>>>> From: Matthew Garrett <mjg59@google.com>
> >>>>>
> >>>>> debugfs has not been meaningfully audited in terms of ensuring that
> >>>>> userland cannot trample over the kernel. At Greg's request, disable
> >>>>> access to it entirely when the kernel is locked down. This is done at
> >>>>> open() time rather than init time as the kernel lockdown status may be
> >>>>> made stricter at runtime.
> >>>>
> >>>> Ugh. Some of those files are very useful. Could this perhaps still
> >>>> allow O_RDONLY if we're in INTEGRITY mode?
> >>>
> >>> Useful for what? Debugging, sure, but for "normal operation", no kernel
> >>> functionality should ever require debugfs. If it does, that's a bug and
> >>> should be fixed.
> >>>
> >>
> >> I semi-regularly read files in debugfs to diagnose things, and I think
> >> it would be good for this to work on distro kernels.
> >
> > Doing that for debugging is wonderful. People who want this type of
> > "lock down" are trading potential security for diagnositic ability.
> >
>
> I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
Reading a debugfs file can expose loads of things that can help take
over a kernel, or at least make it easier. Pointer addresses, internal
system state, loads of other fun things. And before 4.14 or so, it was
pretty trivial to use it to oops the kernel as well (not an issue here
anymore, but people are right to be nervous).
Personally, I think these are all just "confidentiality" type things,
but who really knows given the wild-west nature of debugfs (which is as
designed). And given that I think this patch series just crazy anyway,
I really don't care :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 5:29 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327050615.GA548@kroah.com>
> On Mar 26, 2019, at 10:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
>>> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>>> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
>>>> On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
>>>> <matthewgarrett@google.com> wrote:
>>>>>
>>>>> From: Matthew Garrett <mjg59@google.com>
>>>>>
>>>>> debugfs has not been meaningfully audited in terms of ensuring that
>>>>> userland cannot trample over the kernel. At Greg's request, disable
>>>>> access to it entirely when the kernel is locked down. This is done at
>>>>> open() time rather than init time as the kernel lockdown status may be
>>>>> made stricter at runtime.
>>>>
>>>> Ugh. Some of those files are very useful. Could this perhaps still
>>>> allow O_RDONLY if we're in INTEGRITY mode?
>>>
>>> Useful for what? Debugging, sure, but for "normal operation", no kernel
>>> functionality should ever require debugfs. If it does, that's a bug and
>>> should be fixed.
>>>
>>
>> I semi-regularly read files in debugfs to diagnose things, and I think
>> it would be good for this to work on distro kernels.
>
> Doing that for debugging is wonderful. People who want this type of
> "lock down" are trading potential security for diagnositic ability.
>
I think you may be missing the point of splitting lockdown to separate integrity and confidentiality. Can you actually think of a case where *reading* a debugfs file can take over a kernel?
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-03-27 5:10 UTC (permalink / raw)
To: Scott Wood
Cc: atull, mdf, linux-fpga, linux-kernel, linux-api, Ananda Ravuri,
Xu Yilun
In-Reply-To: <655bf2991a4f8bf6a473c91218d6dba7748520aa.camel@redhat.com>
On Mon, Mar 25, 2019 at 05:58:36PM -0500, Scott Wood wrote:
> On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > In early partial reconfiguration private feature, it only
> > > supports 32bit data width when writing data to hardware for
> > > PR. 512bit data width PR support is an important optimization
> > > for some specific solutions (e.g. XEON with FPGA integrated),
> > > it allows driver to use AVX512 instruction to improve the
> > > performance of partial reconfiguration. e.g. programming one
> > > 100MB bitstream image via this 512bit data width PR hardware
> > > only takes ~300ms, but 32bit revision requires ~3s per test
> > > result.
> > >
> > > Please note now this optimization is only done on revision 2
> > > of this PR private feature which is only used in integrated
> > > solution that AVX512 is always supported.
> > >
> > > Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > ---
> > > drivers/fpga/dfl-fme-main.c | 3 ++
> > > drivers/fpga/dfl-fme-mgr.c | 75 +++++++++++++++++++++++++++++++++++++-
> > > --
> > > -----
> > > drivers/fpga/dfl-fme-pr.c | 45 ++++++++++++++++-----------
> > > drivers/fpga/dfl-fme.h | 2 ++
> > > drivers/fpga/dfl.h | 5 +++
> > > 5 files changed, 99 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > > index 086ad24..076d74f 100644
> > > --- a/drivers/fpga/dfl-fme-main.c
> > > +++ b/drivers/fpga/dfl-fme-main.c
> > > @@ -21,6 +21,8 @@
> > > #include "dfl.h"
> > > #include "dfl-fme.h"
> > >
> > > +#define DRV_VERSION "0.8"
> >
> > What is this going to be used for? Under what circumstances will the
> > driver version be bumped? What does it have to do with 512-bit writes?
This patchset adds more features to this driver, so i would like to add
a DRV_VERSION there as an initial one. In the future, if some new features
or extensions for existing features (e.g. new revision of a private feature)
are added we need to bump this version.
> >
> > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > +
> > > +#include <asm/fpu/api.h>
> > > +
> > > +static inline void copy512(void *src, void __iomem *dst)
> > > +{
> > > + kernel_fpu_begin();
> > > +
> > > + asm volatile("vmovdqu64 (%0), %%zmm0;"
> > > + "vmovntdq %%zmm0, (%1);"
> > > + :
> > > + : "r"(src), "r"(dst));
> > > +
> > > + kernel_fpu_end();
> > > +}
> >
> > Shouldn't there be some sort of check that AVX512 is actually supported
> > on the running system?
> >
> > Also, src should be const, and the asm statement should have a memory
> > clobber.
> >
> > > +#else
> > > +static inline void copy512(void *src, void __iomem *dst)
> > > +{
> > > + WARN_ON_ONCE(1);
> > > +}
> > > +#endif
> >
> > Likewise, this will be called if a revision 2 device is used on non-x86
> > (or on x86 with an old binutils). The driver should fall back to 32-bit
> > in such cases.
>
> Sorry, I missed the comment about revision 2 only being on integrated
> devices -- but will that always be the case? Seems worthwhile to check for
> AVX512 support anyway. And there's still the possibility of being built
> with an old binutils such that CONFIG_AS_AVX512 is not set, or running on a
> kernel where avx512 was disabled via a boot option.
>
> What about future revisions >= 2? Currently the driver will treat them as
> if they were revision < 2. Is that intended?
Yes, it's intended. Currently we don't have any hardware with revisions > 2,
and support new revisions may need new code. :) e.g. currently revision is
used to tell 32bit vs 512bit PR, but in future revisions, it may have new
capability registers for this purpose.
Thanks
Hao
>
> -Scott
>
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Greg KH @ 2019-03-27 5:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Matthew Garrett, James Morris, LSM List, LKML, David Howells,
Linux API, Matthew Garrett
In-Reply-To: <CALCETrVDDf4dj3Md1Nxc5P_NCToXGnAbEO3bGfwgKSfeVmU4Kw@mail.gmail.com>
On Tue, Mar 26, 2019 at 09:29:14PM -0700, Andy Lutomirski wrote:
> On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > > On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > > <matthewgarrett@google.com> wrote:
> > > >
> > > > From: Matthew Garrett <mjg59@google.com>
> > > >
> > > > debugfs has not been meaningfully audited in terms of ensuring that
> > > > userland cannot trample over the kernel. At Greg's request, disable
> > > > access to it entirely when the kernel is locked down. This is done at
> > > > open() time rather than init time as the kernel lockdown status may be
> > > > made stricter at runtime.
> > >
> > > Ugh. Some of those files are very useful. Could this perhaps still
> > > allow O_RDONLY if we're in INTEGRITY mode?
> >
> > Useful for what? Debugging, sure, but for "normal operation", no kernel
> > functionality should ever require debugfs. If it does, that's a bug and
> > should be fixed.
> >
>
> I semi-regularly read files in debugfs to diagnose things, and I think
> it would be good for this to work on distro kernels.
Doing that for debugging is wonderful. People who want this type of
"lock down" are trading potential security for diagnositic ability.
good luck!
greg k-h
^ permalink raw reply
* Re: [PATCH 03/17] fpga: dfl: fme: support 512bit data width PR
From: Wu Hao @ 2019-03-27 4:37 UTC (permalink / raw)
To: Scott Wood
Cc: Alan Tull, Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <580c6c604d3915c105f076fc7f22ab5da98598fc.camel@redhat.com>
On Tue, Mar 26, 2019 at 04:22:34PM -0500, Scott Wood wrote:
> On Tue, 2019-03-26 at 14:33 -0500, Alan Tull wrote:
> > On Mon, Mar 25, 2019 at 5:58 PM Scott Wood <swood@redhat.com> wrote:
> >
> > Hi Scott,
> >
> > > On Mon, 2019-03-25 at 17:53 -0500, Scott Wood wrote:
> > > > On Mon, 2019-03-25 at 11:07 +0800, Wu Hao wrote:
> > > > > +#if defined(CONFIG_X86) && defined(CONFIG_AS_AVX512)
> > > > > +
> > > > > +#include <asm/fpu/api.h>
> > > > > +
> > > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > > +{
> > > > > + kernel_fpu_begin();
> > > > > +
> > > > > + asm volatile("vmovdqu64 (%0), %%zmm0;"
> > > > > + "vmovntdq %%zmm0, (%1);"
> > > > > + :
> > > > > + : "r"(src), "r"(dst));
> > > > > +
> > > > > + kernel_fpu_end();
> > > > > +}
> > > >
> > > > Shouldn't there be some sort of check that AVX512 is actually
> > > > supported
> > > > on the running system?
> > > >
> > > > Also, src should be const, and the asm statement should have a memory
> > > > clobber.
Yes, I will fix this in the next version.
> > > >
> > > > > +#else
> > > > > +static inline void copy512(void *src, void __iomem *dst)
> > > > > +{
> > > > > + WARN_ON_ONCE(1);
> > > > > +}
> > > > > +#endif
> > > >
> > > > Likewise, this will be called if a revision 2 device is used on non-
> > > > x86
> > > > (or on x86 with an old binutils). The driver should fall back to 32-
> > > > bit
> > > > in such cases.
Unfortunately revision 2 is only for integrated FPGA solution, and it doesn't
support any fallback solution (original 32bit data partial reconfiguration is
not supported any more), so driver has to WARN in such path.
> > >
> > > Sorry, I missed the comment about revision 2 only being on integrated
> > > devices -- but will that always be the case? Seems worthwhile to check
> > > for
> > > AVX512 support anyway. And there's still the possibility of being built
> > > with an old binutils such that CONFIG_AS_AVX512 is not set, or running
> > > on a
> > > kernel where avx512 was disabled via a boot option.
> >
> > The code checks for CONFIG_AS_AVX512 above.
>
> That just indicates that binutils supports it. Plus, the code does not
> check for CONFIG_AS_AVX512 when deciding whether to set pr_datawidth to 64
> (and thus call copy512), so you'll get a WARN_ON rather than falling back to
> 32-bit.
>
> > What boot option are you referring to?
>
> clearcpuid=304
Just tried it, my system was down after running above AVX512 with this option.
I agree that it needs to add some check code to make sure it's safe to run
such instructions. I will add some cpu_feature_enabled() check in the next
version.
Thanks a lot for the review and comments.
Hao
>
> -Scott
>
^ permalink raw reply
* Re: [PATCH V31 25/25] debugfs: Disable open() when kernel is locked down
From: Andy Lutomirski @ 2019-03-27 4:29 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, Matthew Garrett, James Morris, LSM List, LKML,
David Howells, Linux API, Matthew Garrett
In-Reply-To: <20190327003057.GA27311@kroah.com>
On Tue, Mar 26, 2019 at 5:31 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 26, 2019 at 12:20:24PM -0700, Andy Lutomirski wrote:
> > On Tue, Mar 26, 2019 at 11:28 AM Matthew Garrett
> > <matthewgarrett@google.com> wrote:
> > >
> > > From: Matthew Garrett <mjg59@google.com>
> > >
> > > debugfs has not been meaningfully audited in terms of ensuring that
> > > userland cannot trample over the kernel. At Greg's request, disable
> > > access to it entirely when the kernel is locked down. This is done at
> > > open() time rather than init time as the kernel lockdown status may be
> > > made stricter at runtime.
> >
> > Ugh. Some of those files are very useful. Could this perhaps still
> > allow O_RDONLY if we're in INTEGRITY mode?
>
> Useful for what? Debugging, sure, but for "normal operation", no kernel
> functionality should ever require debugfs. If it does, that's a bug and
> should be fixed.
>
I semi-regularly read files in debugfs to diagnose things, and I think
it would be good for this to work on distro kernels.
^ 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