* Patch "fs/proc: Stop trying to report thread stacks" has been added to the 3.18-stable tree
From: gregkh @ 2018-04-06 7:51 UTC (permalink / raw)
To: luto, akpm, bp, brgerst, gregkh, hannes, jann, keescook,
linux-api, mingo, peterz, tglx, torvalds, tycho.andersen, viro
Cc: stable, stable-commits
This is a note to let you know that I've just added the patch titled
fs/proc: Stop trying to report thread stacks
to the 3.18-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
fs-proc-stop-trying-to-report-thread-stacks.patch
and it can be found in the queue-3.18 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From b18cb64ead400c01bf1580eeba330ace51f8087d Mon Sep 17 00:00:00 2001
From: Andy Lutomirski <luto@kernel.org>
Date: Fri, 30 Sep 2016 10:58:57 -0700
Subject: fs/proc: Stop trying to report thread stacks
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/proc.txt | 26 --------------------------
fs/proc/task_mmu.c | 29 ++++++++++-------------------
fs/proc/task_nommu.c | 26 +++++++++-----------------
3 files changed, 19 insertions(+), 62 deletions(-)
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -371,32 +371,6 @@ is not associated with a file:
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. Hence, for the example above, the
-task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -248,24 +248,15 @@ static int do_maps_open(struct inode *in
* /proc/PID/maps that is the stack of the main task.
*/
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
- int stack = 0;
-
- if (is_pid) {
- stack = vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
}
static void
@@ -332,7 +323,7 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- if (is_stack(priv, vma, is_pid))
+ if (is_stack(priv, vma))
name = "[stack]";
}
@@ -1501,7 +1492,7 @@ static int show_numa_map(struct seq_file
seq_path(m, &file->f_path, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else if (is_stack(proc_priv, vma, is_pid)) {
+ } else if (is_stack(proc_priv, vma)) {
seq_puts(m, " stack");
}
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -124,25 +124,17 @@ unsigned long task_statm(struct mm_struc
}
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
struct mm_struct *mm = vma->vm_mm;
- int stack = 0;
- if (is_pid) {
- stack = vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
}
/*
@@ -184,7 +176,7 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_path(m, &file->f_path, "");
- } else if (mm && is_stack(priv, vma, is_pid)) {
+ } else if (mm && is_stack(priv, vma)) {
seq_pad(m, ' ');
seq_printf(m, "[stack]");
}
Patches currently in stable-queue which might be from luto@kernel.org are
queue-3.18/perf-hwbp-simplify-the-perf-hwbp-code-fix-documentation.patch
queue-3.18/fs-proc-stop-trying-to-report-thread-stacks.patch
queue-3.18/kvm-x86-fix-icebp-instruction-handling.patch
^ permalink raw reply
* Patch "fs/proc: Stop trying to report thread stacks" has been added to the 4.4-stable tree
From: gregkh @ 2018-04-06 7:54 UTC (permalink / raw)
To: luto, akpm, bp, brgerst, gregkh, hannes, jann, keescook,
linux-api, mingo, peterz, tglx, torvalds, tycho.andersen, viro
Cc: stable, stable-commits
This is a note to let you know that I've just added the patch titled
fs/proc: Stop trying to report thread stacks
to the 4.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
fs-proc-stop-trying-to-report-thread-stacks.patch
and it can be found in the queue-4.4 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
>From b18cb64ead400c01bf1580eeba330ace51f8087d Mon Sep 17 00:00:00 2001
From: Andy Lutomirski <luto@kernel.org>
Date: Fri, 30 Sep 2016 10:58:57 -0700
Subject: fs/proc: Stop trying to report thread stacks
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/proc.txt | 26 --------------------------
fs/proc/task_mmu.c | 29 ++++++++++-------------------
fs/proc/task_nommu.c | 26 +++++++++-----------------
3 files changed, 19 insertions(+), 62 deletions(-)
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -383,32 +383,6 @@ is not associated with a file:
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. Hence, for the example above, the
-task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -253,24 +253,15 @@ static int do_maps_open(struct inode *in
* /proc/PID/maps that is the stack of the main task.
*/
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
- int stack = 0;
-
- if (is_pid) {
- stack = vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
}
static void
@@ -337,7 +328,7 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- if (is_stack(priv, vma, is_pid))
+ if (is_stack(priv, vma))
name = "[stack]";
}
@@ -1560,7 +1551,7 @@ static int show_numa_map(struct seq_file
seq_file_path(m, file, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else if (is_stack(proc_priv, vma, is_pid)) {
+ } else if (is_stack(proc_priv, vma)) {
seq_puts(m, " stack");
}
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -124,25 +124,17 @@ unsigned long task_statm(struct mm_struc
}
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
struct mm_struct *mm = vma->vm_mm;
- int stack = 0;
- if (is_pid) {
- stack = vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
}
/*
@@ -184,7 +176,7 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_file_path(m, file, "");
- } else if (mm && is_stack(priv, vma, is_pid)) {
+ } else if (mm && is_stack(priv, vma)) {
seq_pad(m, ' ');
seq_printf(m, "[stack]");
}
Patches currently in stable-queue which might be from luto@kernel.org are
queue-4.4/perf-hwbp-simplify-the-perf-hwbp-code-fix-documentation.patch
queue-4.4/fs-proc-stop-trying-to-report-thread-stacks.patch
queue-4.4/nospec-kill-array_index_nospec_mask_check.patch
^ permalink raw reply
* Re: [PATCH v4 04/24] fpga: add device feature list support
From: Wu Hao @ 2018-04-06 11:01 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXTRNWH6d+RstjPenYyOjktyAhA35-K_OM_3JZ_T3Ai0gA@mail.gmail.com>
On Wed, Apr 04, 2018 at 03:06:57PM -0500, Alan Tull wrote:
> On Mon, Apr 2, 2018 at 8:36 PM, Wu Hao <hao.wu@intel.com> wrote:
> > On Mon, Apr 02, 2018 at 02:06:56PM -0500, Alan Tull wrote:
> >> On Sun, Apr 1, 2018 at 11:22 PM, Wu Hao <hao.wu@intel.com> wrote:
> >> > On Thu, Mar 29, 2018 at 04:57:22PM -0500, Alan Tull wrote:
> >> >> On Mon, Mar 26, 2018 at 9:35 PM, Wu Hao <hao.wu@intel.com> wrote:
> >> >>
> >> >> Hi Hao,
> >> >>
> >> >> Currently there is one set of functions that handles port enable,
> >> >> disable, and reset and it's in dfl.c and dfl.h, so that's not in any
> >> >> driver module that can be switched out if necessary for a different
> >> >> implementation of the port. Finding a way for this patchset to be
> >> >> structured for DFL to control what low level manager/port drivers are
> >> >> used is the current challenge that I've got a lot of my attention on.
> >> >>
> >> >> Thanks for the explanations on how virtualization affects how this can
> >> >> be implemented.
> >> >>
> >> >> > On Mon, Mar 26, 2018 at 12:21:23PM -0500, Alan Tull wrote:
> >> >> >> On Thu, Mar 22, 2018 at 11:33 PM, Wu Hao <hao.wu@intel.com> wrote:
> >> >> >>
> >> >> >> >> > +
> >> >> >> >> > +/*
> >> >> >> >> > + * This function resets the FPGA Port and its accelerator (AFU) by function
> >> >> >> >> > + * __fpga_port_disable and __fpga_port_enable (set port soft reset bit and
> >> >> >> >> > + * then clear it). Userspace can do Port reset at any time, e.g during DMA
> >> >> >> >> > + * or Partial Reconfiguration. But it should never cause any system level
> >> >> >> >> > + * issue, only functional failure (e.g DMA or PR operation failure) and be
> >> >> >> >> > + * recoverable from the failure.
> >> >> >> >> > + *
> >> >> >> >> > + * Note: the accelerator (AFU) is not accessible when its port is in reset
> >> >> >> >> > + * (disabled). Any attempts on MMIO access to AFU while in reset, will
> >> >> >> >> > + * result errors reported via port error reporting sub feature (if present).
> >> >> >> >> > + */
> >> >> >> >> > +static inline int __fpga_port_reset(struct platform_device *pdev)
> >> >> >> >> > +{
> >> >> >> >> > + int ret;
> >> >> >> >> > +
> >> >> >> >> > + ret = __fpga_port_disable(pdev);
> >> >> >> >> > + if (ret)
> >> >> >> >> > + return ret;
> >> >> >> >> > +
> >> >> >> >> > + __fpga_port_enable(pdev);
> >> >> >> >> > +
> >> >> >> >> > + return 0;
> >> >> >> >> > +}
> >> >> >> >> > +
> >> >> >> >> > +static inline int fpga_port_reset(struct platform_device *pdev)
> >> >> >> >> > +{
> >> >> >> >> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >> >> >> >> > + int ret;
> >> >> >> >> > +
> >> >> >> >> > + mutex_lock(&pdata->lock);
> >> >> >> >> > + ret = __fpga_port_reset(pdev);
> >> >> >> >> > + mutex_unlock(&pdata->lock);
> >> >> >> >> > +
> >> >> >> >> > + return ret;
> >> >> >> >> > +}
> >> >> >> >>
> >> >> >> >> I'm still scratching my head about how the enumeration code also has
> >> >> >> >> code that handles resetting the PL in a FPGA region and
> >> >> >> >> enabling/disabling the bridge. We've discussed this before [1] and I
> >> >> >> >> know you've looked into it, I'm still trying to figure out how this
> >> >> >> >> can be made modular, so when someone needs to support a different port
> >> >> >> >> in the future, it isn't a complete rewrite.
> >> >> >> >>
> >> >> >> >> Speaking of resets, one way forward would be to create a reset
> >> >> >> >> controller for the port (and if possible move the port code to the
> >> >> >> >> bridge platform driver). The current linux-next repo adds support for
> >> >> >> >> reset lookups, so that reset controllers are supported for non-DT
> >> >> >> >> platforms [2].
> >> >> >> >>
> >> >> >> >> So the bridge driver would implement the enable/disable functions and
> >> >> >> >> create a reset controller, the fpga-region (or whoever else needs it)
> >> >> >> >> could look the reset controller and use the reset. By using the
> >> >> >> >> kernel reset framework, we don't have to have that piece of code
> >> >> >> >> shared around by having a reset function in a .h file. And it avoids
> >> >> >> >> adding extra dependencies between modules. Also, where necessary, I'd
> >> >> >> >> rather add functionality to the existing bridge/mgr/region frameworks,
> >> >> >> >> adding common interfaces at that level to allow reuse (like adding
> >> >> >> >> status to fpga-mgr). Ideally, this DFL framework would sit on top of
> >> >> >> >> mgr and bridge and allow those to be swapped out for reuse of the DFL
> >> >> >> >> framework on other devices. Also it will save future headaches as mgr
> >> >> >> >> or port implementations evolve.
> >> >> >> >
> >> >> >> > Thanks a lot for the suggestion. I really really appreciate this.
> >> >> >>
> >> >> >> Yes, this is a good discussion, thanks.
> >> >> >>
> >> >> >> >
> >> >> >> > Actually if we consider the virutalization case as I mentioned in [1] below,
> >> >> >> > that means AFU and its Port will be turned into a PCI VF and assigned (passed
> >> >> >> > through) to a virtual machine. There is no FME block on that PCI VF device,
> >> >> >> > (the FME is always kept in PCI PF device in the host) and currently the bridge
> >> >> >> > is created by FME module for PR functionatily. So in the guest virtual machine,
> >> >> >> > nobody creates the reset controller actually.
> >> >> >> >
> >> >> >> > As I mentioned in [1], one possible method is, put these port reset functions to
> >> >> >> > AFU (Port) module, and share those functions with FME bridge module.
> >> >> >>
> >> >> >> Yes, the port reset functions could move into an AFU driver, and then
> >> >> >> also the AFU driver could also create a reset controller and register
> >> >> >> a lookup [2] for the reset. That would be just a few lines of code.
> >> >> >> The reset controller would control enabling/disabling the port. The
> >> >> >> bridge driver could get the reset controller to use during FPGA
> >> >> >> programming. That is instead of sharing a reset function with the
> >> >> >> bridge driver. It decouples the FPGA bridge driver and simplifies it
> >> >> >> to be something that just needs to control a reset instead of needing
> >> >> >> to include a specific .h file that makes a port reset function
> >> >> >> available.
> >> >> >
> >> >> > Hi Alan
> >> >> >
> >> >> > Thanks a lot for the feedback. :)
> >> >> >
> >> >> > The major concern here is, for virtualization case, after we enable the SRIOV
> >> >> > to create VFs, AFUs(and ports) are turned into VFs from PF. Once AFUs are moved
> >> >> > from PF to VFs, then we should remove all related user interfaces exported by
> >> >> > the afu platform device under PF by unregistering these platform devices from
> >> >> > the system. So in this case the reset controller created by the AFU platform
> >> >> > driver, should be removed when the AFU platform devices are deleted from the
> >> >> > system in this case, but we still have FME and FME bridge present on PF, then
> >> >> > FME bridge can't find the reset controller any longer to do port enable/disable.
> >> >>
> >> >> OK
> >> >>
> >> >> >
> >> >> > Sorry, I found my previous description is not accurate.
> >> >> >
> >> >> > VFs could be passed through to a virtual machine, if we let AFU/Port create
> >> >> > reset controller, then the reset controllers are created in the virtual machine.
> >> >> > And FME is always in PF in the host, so FME bridge in host have no access to the
> >> >> > reset controllers in the virtual machine.
> >> >>
> >> >> Thanks for the explanation. Does the current implementation allows
> >> >> the port's PORT_CTRL_SFTRST reset bit to be controlled by PF and VF at
> >> >> the same time?
> >> >
> >> > Yes, it allows it to be accessed by PF and VF at the same time, only for Port
> >> > Registers, not for AFU registers (any access will cause errors reported by HW).
> >>
> >> OK that explains a lot.
> >>
> >> >
> >> >> Or is the idea that the VF has to be given up in order to allow the FME PF to
> >> >> be able to reprogram?
> >> >
> >> > Without any notification mechanism between PF and VFs, the safe way of doing the
> >> > PR to AFUs (accelerators) on VFs is, 1) hot-unplug the AFU (VF) from the VM,
> >> > 2) turn the AFU back to PF from VF, 3) PR to the AFU on PF, 4) turn that AFU to
> >> > VF again, 5) hot-plug the AFU (VF) to target VM again. We tested this flow, it
> >> > works and doesn't need to shutdown the VM.
> >>
> >> That sounds like a lot of trouble.
> >
> > Without notification mechanism, this is the safe way of doing PR. We can't PR a
> > AFU (assigned to VM) directly without any notification to VM, as VM owns this
> > device (AFU). SW works on guest may get troubles if PR is done at unexpected
> > time. For above flow, it's just the pcie hot plug function, and we tested it
> > works fine as we expected.
> >
> >>
> >> > But once we have implemented some
> >> > methods to notification between PF and VFs, we don't have to do these steps.
> >>
> >> See below. Let's get aligned with what we're trying to architect first.
> >
> > Sure.
> >
> >>
> >> >
> >> >> After the AFU and port is turned into a VF, is the port's memory range is
> >> >> mapped in both the PF and the VF?
> >> >
> >> > Yes.
> >> >
> >> >>
> >> >> >
> >> >> >>
> >> >> >> > I think
> >> >> >> > that will make the code in the common DFL framework a little more clean,
> >> >> >>
> >> >> >> Yes, IIUC that may also make it easier as the port/AFU gets added
> >> >> >> functionality that is intended to be controlled by the VF anyway
> >> >> >> (while the only port-related thing that is needed by the FME is port
> >> >> >> enable/disable).
> >> >> >>
> >> >> >> > but it
> >> >> >> > will introduce some module dependency here for sure, (e.g FME modules can't
> >> >> >> > finish PR without AFU (Port) Module loaded).
> >> >> >>
> >> >> >> That sounds like an OK type of dependency, i.e. if the modules are not
> >> >> >> all loaded, it doesn't work. :-)
> >> >> >
> >> >> > Find a reset controller by lookup, if not found, return error code. It seems
> >> >> > not a really hard module dependency between port/afu and FME bridge modules.
> >> >>
> >> >> That was what I was hoping would work here. But if the module isn't
> >> >> loaded because it failed due to the reset controller in the AFU driver
> >> >> went away, then, yes, that won't work.
> >> >>
> >> >> > But if in FME bridge, it uses functions exposed by port/afu module, that's a
> >> >> > hard dependency. : )
> >> >>
> >> >> Yes I'm trying to find ways to get away from that kind of hard
> >> >> dependency. So when someone uses this with a different port, it won't
> >> >> be a huge rewrite of dfl.c and dfl.h. I understand that the port is
> >> >> used by both the AFU and the PR code, that's why it's in a file that
> >> >> is included by both of them. That's going to be a problem as soon as
> >> >> this is used with a different port.
> >> >
> >> > or we could add some callbacks, and let port driver register its own function
> >> > for enable/disable operation? But then dfl.c / dfl.h will still see some common
> >> > port code there.
> >> >
> >> >>
> >> >> >
> >> >> > I can try to move related code to afu/port driver instead in the next version
> >> >> > for sure, but I can't create the reset controller per the reason above. Please
> >> >> > let me know if more thoughts on this. : )
> >> >>
> >> >> Maybe that is the way forward. I'm still thinking about this. So the
> >> >> DFL will create a AFU driver that includes the port. If someone
> >> >> implements a different port, there would be a different id to cause
> >> >> that AFU driver to be loaded instead. It seems a shame that more of
> >> >> the AFU code couldn't be reused. That was the original idea of
> >> >> fpga-bridge. Unfortunately it seems that the bridge is needed by both
> >> >> the VF and PF so it's complicated by that.
> >> >>
> >> >> >
> >> >> >>
> >> >> >> > But anyway it may be still
> >> >> >> > acceptable for users as all these modules could be loaded automatically. How do
> >> >> >> > you think? :)
> >> >> >>
> >> >> >> The other thing I want to get right now is if there is a different
> >> >> >> AFU/port that needs a different driver. Can the DFL be changed to
> >> >> >> specify what AFU/port to load? I really really want to avoid large
> >> >> >> code rewrites in the future that we can anticipate now. Such as
> >> >> >> someone implements their own static image, it has DFL, but the port is
> >> >> >> somewhat different. Instead of seeing features as just something that
> >> >> >> gets added, the DFL also specifies what port driver and mgr driver to
> >> >> >> load. The stuff we discussed above is a good step towards that, but
> >> >> >> not all of it.
> >> >> >
> >> >> > I'm not sure if any vendor
> >> >>
> >> >> Since this is open source, it's important to remember that vendors
> >> >> aren't the only ones driving development of Linux. Any user of FPGA
> >> >> under Linux can (and has) come along and add to this subsystem. This
> >> >> code should not discourage that.
> >> >
> >> > Agree.
> >> >
> >> >>
> >> >> > wants to create a totally different port here, if
> >> >> > yes, then it could have a different feature id in Device Feature Header (DFH).
> >> >> > I think it's possible to use that feature id to decide which driver to load
> >> >> > (or which platform device to create).
> >> >>
> >> >> I think it's what we need.
> >> >
> >> > Yes.
> >> >
> >> >>
> >> >> > But vendors don't have to do that, as it
> >> >> > could reuse current port driver and private features added already, or even
> >> >> > add some new vendor specific private feature under the port to save cost.
> >> >>
> >> >> They would have to implement a static image with port registers that
> >> >> function the same way for at least port enable/disable/reset. If they
> >> >> need to tweak the driver implementation for their hardware then that's
> >> >> not possible or it's ugly at least.
> >> >
> >> > Agree, in that case, it's better to use a new feature id with a different
> >> > port implementation.
> >> >
> >> >> This is also the case if you have some newer version of you port while
> >> >> keeping legacy support for your original port.
> >> >
> >> > In Device Feature Header, there is a field to indicate the revision of
> >> > this Port (private feature has revision bit in DFH too). But we should
> >> > not use this field to indicate a totally different implementation.
> >> >
> >> >>
> >> >> I understand that virtualization is making this hard. Thanks for
> >> >> thinking about how this can move forward on this issue.
> >> >
> >> > Yes, I will try to move the actual port related code into AFU/port driver
> >> > in the next version, thanks for the comments and suggestions.
> >>
> >> I have some serious doubts that's the direction to go in. Before you
> >> do a lot of work in that direction, let me explain again the larger
> >> context and what's motivating my comments.
> >>
> >> The point of having a FPGA framework (fpga-bridge.c, fpga-manager.c,
> >> and fpga-region.c) is to separate the layers above the framework
> >> (enumeration and interfaces) from the layer below the framework (low
> >> level FPGA bridge/manager/region drivers). The layer above and the
> >> layer below shouldn't share code or talk directly to each other. That
> >> kind of workaround defeats the purpose of having a framework and
> >> prevents reuse. If you need a workaround like that, it's probably a
> >> case where the framework needs some added functionality that's
> >> generally usable. For example, we've run into that before, in v1,
> >> your FPGA manager driver was returning status via its private data.
> >> We discussed it and added status to the fpga-mgr framework so you
> >> wouldn't need to do that.
> >>
> >> So implementing fpga_port_enable in the enumeration code and then
> >> accessing that code in both the AFU code (upper layer) and the FME
> >> bridge driver (lower layer) beaks the model. When an implementation
> >> works around a framework to do what it wants, that mean that none of
> >> that implementation is reusable. A lot of this review has been me
> >> trying to understand and untangle that. I'm trying to guide the
> >> development of the FPGA framework to have reusibility.
> >>
> >> The 'port' is really what the existing FPGA framework calls an FPGA
> >> bridge (with added functionality). The port code should go into the
> >> bridge driver dfl-fme-br.c. There will need to be some new
> >> functionality needed for fpga-bridge.c to be able to do what you want
> >> - some way of making a reset function available for VF while
> >> enable/disable is still available as PF for the fpga-region.c to
> >> control.
> >
> > Understand, actually I have considered to move bridge creation from FME
> > to Port, but it's facing the same problem as reset controller, as port
> > platform device should be unregisted from system in virtualization case.
> > Let me consider further to see if any better approach on this implementation.
> > May be back to this thread later for discussion.
>
> It appears that a lot of effort is going into working around the
> current BBS limitations with regards to virtualization. One direct
> way for this all to move forward would be to defer virtualization
> support for later. That would involve moving the port code to
> dfl-fme-br.c and adding resets to the fpga-bridge framework for the
> AFU. (Also add selection of low level mgr and br driver to DFL).
> That gets something working that is modular, just doesn't include
> virtualization for the moment.
Hi Alan
Understand, but as you know, the current Intel FPGA devices supported by
this patchset are targeted to be used for cloud, data center and etc, so
virutalization support is a very important feature to the end user for
the FPGA devices. This is the reason that We have to support virtualization
with our driver code (please refer to [1] for some introduction for
these Intel FPGA devices).
[1] https://www.altera.com/solutions/acceleration-hub/overview.html
Thanks
Hao
>
> Alan
>
> >
> > Thanks
> > Hao
> >
> >>
> >> Thanks,
> >> Alan
> >>
> >> >
> >> > Thanks
> >> > Hao
> >> >
> >> >>
> >> >> Alan
> >> >>
> >> >> >
> >> >> > Thanks
> >> >> > Hao
> >> >> >
> >> >> >>
> >> >> >> Alan
> >> >> >>
> >> >> >> >
> >> >> >> > Thanks
> >> >> >> > Hao
> >> >> >> >
> >> >> >> >
> >> >> >> >>
> >> >> >> >> Alan
> >> >> >> >>
> >> >> >> >> [1] https://lkml.org/lkml/2017/12/22/398
> >> >> >> >> [2] https://patchwork.kernel.org/patch/10247475/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 10/24] fpga: dfl: add FPGA Management Engine driver basic framework
From: Wu Hao @ 2018-04-06 11:04 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXQidwWhHfHd-usQVdK3jZbJuB3y8=NK7y5K_h_8eJhjAg@mail.gmail.com>
On Thu, Apr 05, 2018 at 01:35:24PM -0500, Alan Tull wrote:
> On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> > From: Kang Luwei <luwei.kang@intel.com>
> >
> > The FPGA Management Engine (FME) provides power, thermal management,
> > performance counters, partial reconfiguration and other functions. For each
> > function, it is packaged into a private feature linked to the FME feature
> > device in the 'Device Feature List'. It's a platform device created by
> > DFL framework.
> >
> > This patch adds the basic framework of FME platform driver. It defines
> > sub feature drivers to handle the different sub features, including init,
> > uinit and ioctl. It also registers the file operations for the device file.
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > v3: rename driver from intel-fpga-fme to dfl-fme
> > rename Kconfig from INTEL_FPGA_FME to FPGA_DFL_FME
> > v4: fix SPDX license issue, use dfl-fme as module name
> > ---
> > drivers/fpga/Kconfig | 10 +++
> > drivers/fpga/Makefile | 3 +
> > drivers/fpga/dfl-fme-main.c | 158 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 171 insertions(+)
> > create mode 100644 drivers/fpga/dfl-fme-main.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 87f3d44..103d5e2 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -140,6 +140,16 @@ config FPGA_DFL
> > Gate Array (FPGA) solutions which implement Device Feature List.
> > It provides enumeration APIs, and feature device infrastructure.
> >
> > +config FPGA_DFL_FME
> > + tristate "FPGA DFL FME Driver"
> > + depends on FPGA_DFL
> > + help
> > + The FPGA Management Engine (FME) is a feature device implemented
> > + under Device Feature List (DFL) framework. Select this option to
> > + enable the platform device driver for FME which implements all
> > + FPGA platform level management features. There shall be 1 FME
> > + per DFL based FPGA device.
> > +
> > config FPGA_DFL_PCI
> > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> > depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 4375630..fbd1c85 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -30,6 +30,9 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> >
> > # FPGA Device Feature List Support
> > obj-$(CONFIG_FPGA_DFL) += dfl.o
> > +obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> > +
> > +dfl-fme-objs := dfl-fme-main.o
> >
> > # Drivers for FPGAs which implement DFL
> > obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
> > new file mode 100644
> > index 0000000..ebe6b52
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-fme-main.c
> > @@ -0,0 +1,158 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for FPGA Management Engine (FME)
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Kang Luwei <luwei.kang@intel.com>
> > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > + * Joseph Grecco <joe.grecco@intel.com>
> > + * Enno Luebbers <enno.luebbers@intel.com>
> > + * Tim Whisonant <tim.whisonant@intel.com>
> > + * Ananda Ravuri <ananda.ravuri@intel.com>
> > + * Henry Mitchel <henry.mitchel@intel.com>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include "dfl.h"
> > +
> > +static int fme_hdr_init(struct platform_device *pdev, struct feature *feature)
> > +{
> > + dev_dbg(&pdev->dev, "FME HDR Init.\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void fme_hdr_uinit(struct platform_device *pdev, struct feature *feature)
> > +{
> > + dev_dbg(&pdev->dev, "FME HDR UInit.\n");
> > +}
> > +
> > +static const struct feature_ops fme_hdr_ops = {
> > + .init = fme_hdr_init,
> > + .uinit = fme_hdr_uinit,
> > +};
> > +
> > +static struct feature_driver fme_feature_drvs[] = {
> > + {
> > + .id = FME_FEATURE_ID_HEADER,
> > + .ops = &fme_hdr_ops,
> > + },
> > + {
> > + .ops = NULL,
> > + },
> > +};
> > +
> > +static int fme_open(struct inode *inode, struct file *filp)
> > +{
> > + struct platform_device *fdev = fpga_inode_to_feature_dev(inode);
> > + struct feature_platform_data *pdata = dev_get_platdata(&fdev->dev);
> > + int ret;
> > +
> > + if (WARN_ON(!pdata))
> > + return -ENODEV;
> > +
> > + ret = feature_dev_use_begin(pdata);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(&fdev->dev, "Device File Open\n");
> > + filp->private_data = pdata;
> > +
> > + return 0;
> > +}
> > +
> > +static int fme_release(struct inode *inode, struct file *filp)
> > +{
> > + struct feature_platform_data *pdata = filp->private_data;
> > + struct platform_device *pdev = pdata->dev;
> > +
> > + dev_dbg(&pdev->dev, "Device File Release\n");
> > + feature_dev_use_end(pdata);
> > +
> > + return 0;
> > +}
> > +
> > +static long fme_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > +{
> > + struct feature_platform_data *pdata = filp->private_data;
> > + struct platform_device *pdev = pdata->dev;
> > + struct feature *f;
> > + long ret;
> > +
> > + dev_dbg(&pdev->dev, "%s cmd 0x%x\n", __func__, cmd);
> > +
> > + switch (cmd) {
> > + default:
> > + /*
> > + * Let sub-feature's ioctl function to handle the cmd
> > + * Sub-feature's ioctl returns -ENODEV when cmd is not
> > + * handled in this sub feature, and returns 0 and other
> > + * error code if cmd is handled.
> > + */
> > + fpga_dev_for_each_feature(pdata, f) {
> > + if (f->ops && f->ops->ioctl) {
> > + ret = f->ops->ioctl(pdev, f, cmd, arg);
> > + if (ret == -ENODEV)
> > + continue;
> > + else
> > + return ret;
>
> continue and else aren't needed. Could be
>
> if (ret != -ENODEV)
> return ret;
Hi Alan
Thanks for the review and comments.
I agree with you, will fix this in the next version.
Thanks
Hao
>
> Alan
>
> > + }
> > + }
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static const struct file_operations fme_fops = {
> > + .owner = THIS_MODULE,
> > + .open = fme_open,
> > + .release = fme_release,
> > + .unlocked_ioctl = fme_ioctl,
> > +};
> > +
> > +static int fme_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > +
> > + ret = fpga_dev_feature_init(pdev, fme_feature_drvs);
> > + if (ret)
> > + goto exit;
> > +
> > + ret = fpga_register_dev_ops(pdev, &fme_fops, THIS_MODULE);
> > + if (ret)
> > + goto feature_uinit;
> > +
> > + return 0;
> > +
> > +feature_uinit:
> > + fpga_dev_feature_uinit(pdev);
> > +exit:
> > + return ret;
> > +}
> > +
> > +static int fme_remove(struct platform_device *pdev)
> > +{
> > + fpga_dev_feature_uinit(pdev);
> > + fpga_unregister_dev_ops(pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver fme_driver = {
> > + .driver = {
> > + .name = FPGA_FEATURE_DEV_FME,
> > + },
> > + .probe = fme_probe,
> > + .remove = fme_remove,
> > +};
> > +
> > +module_platform_driver(fme_driver);
> > +
> > +MODULE_DESCRIPTION("FPGA Management Engine driver");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:dfl-fme");
> > --
> > 2.7.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 20/24] fpga: dfl: add FPGA Accelerated Function Unit driver basic framework
From: Wu Hao @ 2018-04-06 11:05 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXSgd4ySjCqiTyMA4vaOq4jByhLOot02GCLMmgoQSye49Q@mail.gmail.com>
On Thu, Apr 05, 2018 at 01:26:57PM -0500, Alan Tull wrote:
> On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> One minor thing below.
>
> > On DFL FPGA devices, the Accelerated Function Unit (AFU), can be
> > reprogrammed for different functions. It connects to the FPGA
> > infrastructure("blue bistream") via a Port. Port CSRs are implemented
> > separately from the AFU CSRs to provide control and status of the Port.
> > Once valid green bitstream is programmed into the AFU, it allows access
> > to the AFU CSRs in the AFU MMIO space.
> >
> > This patch only implements basic driver framework for AFU, including
> > device file operation framework.
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > ---
> > v3: rename driver to dfl-afu-main.c
> > v4: rename to dfl-port and fix SPDX license issue.
> > ---
> > drivers/fpga/Kconfig | 9 +++
> > drivers/fpga/Makefile | 2 +
> > drivers/fpga/dfl-afu-main.c | 159 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 170 insertions(+)
> > create mode 100644 drivers/fpga/dfl-afu-main.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 65d54a4..4c6b45f 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -168,6 +168,15 @@ config FPGA_DFL_FME_REGION
> > help
> > Say Y to enable FPGA Region driver for FPGA Management Engine.
> >
> > +config FPGA_DFL_AFU
> > + tristate "FPGA DFL AFU Driver"
> > + depends on FPGA_DFL
> > + help
> > + This is the driver for FPGA Accelerated Function Unit (AFU) which
> > + implements AFU and Port management features. A User AFU connects
> > + to the FPGA infrastructure via a Port. There may be more than 1
> > + Port/AFU per DFL based FPGA device.
> > +
> > config FPGA_DFL_PCI
> > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> > depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 163894e..5c9607b 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -34,8 +34,10 @@ obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> > obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> > obj-$(CONFIG_FPGA_DFL_FME_BRIDGE) += dfl-fme-br.o
> > obj-$(CONFIG_FPGA_DFL_FME_REGION) += dfl-fme-region.o
> > +obj-$(CONFIG_FPGA_DFL_AFU) += dfl-afu.o
> >
> > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> > +dfl-afu-objs := dfl-afu-main.o
> >
> > # Drivers for FPGAs which implement DFL
> > obj-$(CONFIG_FPGA_DFL_PCI) += dfl-pci.o
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > new file mode 100644
> > index 0000000..70db28c
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for FPGA Accelerated Function Unit (AFU)
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Wu Hao <hao.wu@intel.com>
> > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > + * Joseph Grecco <joe.grecco@intel.com>
> > + * Enno Luebbers <enno.luebbers@intel.com>
> > + * Tim Whisonant <tim.whisonant@intel.com>
> > + * Ananda Ravuri <ananda.ravuri@intel.com>
> > + * Henry Mitchel <henry.mitchel@intel.com>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +
> > +#include "dfl.h"
> > +
> > +static int port_hdr_init(struct platform_device *pdev, struct feature *feature)
> > +{
> > + dev_dbg(&pdev->dev, "PORT HDR Init.\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void port_hdr_uinit(struct platform_device *pdev,
> > + struct feature *feature)
> > +{
> > + dev_dbg(&pdev->dev, "PORT HDR UInit.\n");
> > +}
> > +
> > +static const struct feature_ops port_hdr_ops = {
> > + .init = port_hdr_init,
> > + .uinit = port_hdr_uinit,
> > +};
> > +
> > +static struct feature_driver port_feature_drvs[] = {
> > + {
> > + .id = PORT_FEATURE_ID_HEADER,
> > + .ops = &port_hdr_ops,
> > + },
> > + {
> > + .ops = NULL,
> > + }
> > +};
> > +
> > +static int afu_open(struct inode *inode, struct file *filp)
> > +{
> > + struct platform_device *fdev = fpga_inode_to_feature_dev(inode);
> > + struct feature_platform_data *pdata;
> > + int ret;
> > +
> > + pdata = dev_get_platdata(&fdev->dev);
> > + if (WARN_ON(!pdata))
> > + return -ENODEV;
> > +
> > + ret = feature_dev_use_begin(pdata);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(&fdev->dev, "Device File Open\n");
> > + filp->private_data = fdev;
> > +
> > + return 0;
> > +}
> > +
> > +static int afu_release(struct inode *inode, struct file *filp)
> > +{
> > + struct platform_device *pdev = filp->private_data;
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +
> > + dev_dbg(&pdev->dev, "Device File Release\n");
> > +
> > + feature_dev_use_end(pdata);
> > +
> > + return 0;
> > +}
> > +
> > +static long afu_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > +{
> > + struct platform_device *pdev = filp->private_data;
> > + struct feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > + struct feature *f;
> > + long ret;
> > +
> > + dev_dbg(&pdev->dev, "%s cmd 0x%x\n", __func__, cmd);
> > +
> > + switch (cmd) {
> > + default:
> > + /*
> > + * Let sub-feature's ioctl function to handle the cmd
> > + * Sub-feature's ioctl returns -ENODEV when cmd is not
> > + * handled in this sub feature, and returns 0 and other
> > + * error code if cmd is handled.
> > + */
> > + fpga_dev_for_each_feature(pdata, f)
> > + if (f->ops && f->ops->ioctl) {
> > + ret = f->ops->ioctl(pdev, f, cmd, arg);
> > + if (ret == -ENODEV)
> > + continue;
> > + else
> > + return ret;
>
> The continue..else isn't needed. Could just be
> if (ret != -ENODEV)
> return ret;
Agree, will fix this in the next version. Thanks.
Hao
>
> Thanks,
> Alan
>
> > + }
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static const struct file_operations afu_fops = {
> > + .owner = THIS_MODULE,
> > + .open = afu_open,
> > + .release = afu_release,
> > + .unlocked_ioctl = afu_ioctl,
> > +};
> > +
> > +static int afu_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > +
> > + dev_dbg(&pdev->dev, "%s\n", __func__);
> > +
> > + ret = fpga_dev_feature_init(pdev, port_feature_drvs);
> > + if (ret)
> > + return ret;
> > +
> > + ret = fpga_register_dev_ops(pdev, &afu_fops, THIS_MODULE);
> > + if (ret)
> > + fpga_dev_feature_uinit(pdev);
> > +
> > + return ret;
> > +}
> > +
> > +static int afu_remove(struct platform_device *pdev)
> > +{
> > + dev_dbg(&pdev->dev, "%s\n", __func__);
> > +
> > + fpga_dev_feature_uinit(pdev);
> > + fpga_unregister_dev_ops(pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver afu_driver = {
> > + .driver = {
> > + .name = FPGA_FEATURE_DEV_PORT,
> > + },
> > + .probe = afu_probe,
> > + .remove = afu_remove,
> > +};
> > +
> > +module_platform_driver(afu_driver);
> > +
> > +MODULE_DESCRIPTION("FPGA Accelerated Function Unit driver");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:dfl-port");
> > --
> > 2.7.4
> >
^ permalink raw reply
* Re: [PATCH v4 16/24] fpga: dfl: add fpga manager platform driver for FME
From: Wu Hao @ 2018-04-06 11:11 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer, Xiao Guangrong
In-Reply-To: <CANk1AXRbYZDyguNfdE+n6Y2CwJFgqzZmQ0x-PiAa+kjqUUrB6g@mail.gmail.com>
On Thu, Apr 05, 2018 at 01:45:07PM -0500, Alan Tull wrote:
> On Thu, Mar 22, 2018 at 1:07 AM, Wu Hao <hao.wu@intel.com> wrote:
> > On Wed, Mar 21, 2018 at 09:55:52AM -0700, Moritz Fischer wrote:
> >> On Wed, Mar 21, 2018 at 10:50:01AM +0800, Wu Hao wrote:
> >> > On Tue, Mar 20, 2018 at 03:32:34PM -0500, Alan Tull wrote:
> >> > > On Tue, Feb 13, 2018 at 3:24 AM, Wu Hao <hao.wu@intel.com> wrote:
> >> > >
> >> > > Hi Hao,
> >> > >
> >> > > Elsewhere we discussed moving #defines used only in this driver either
> >> > > to this .c file or to a similarly named .h file. A couple minor
> >> > > things below.
> >> >
> >> > Hi Alan,
> >> >
> >> > Yes, I will move those #defines into a similarly named .h file.
> >> >
> >> > >
> >> > > > This patch adds fpga manager driver for FPGA Management Engine (FME). It
> >> > > > implements fpga_manager_ops for FPGA Partial Reconfiguration function.
> >> > > >
> >> > > > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> >> > > > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> >> > > > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> >> > > > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> >> > > > Signed-off-by: Kang Luwei <luwei.kang@intel.com>
> >> > > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> >> > > > ---
> >> > > > v3: rename driver to dfl-fpga-fme-mgr
> >> > > > implemented status callback for fpga manager
> >> > > > rebased due to fpga api changes
> >> > > > v4: rename to dfl-fme-mgr, and fix SPDX license issue
> >> > > > add pr_credit comments and improve dev_err message
> >> > > > remove interface_id sysfs interface
> >> > > > include dfl-fme-pr.h instead of dfl.h
> >> > > > ---
> >> > > > drivers/fpga/Kconfig | 6 +
> >> > > > drivers/fpga/Makefile | 1 +
> >> > > > drivers/fpga/dfl-fme-mgr.c | 290 +++++++++++++++++++++++++++++++++++++++++++++
> >> > > > 3 files changed, 297 insertions(+)
> >> > > > create mode 100644 drivers/fpga/dfl-fme-mgr.c
> >> > > >
> >> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> >> > > > index 103d5e2..89f76e8 100644
> >> > > > --- a/drivers/fpga/Kconfig
> >> > > > +++ b/drivers/fpga/Kconfig
> >> > > > @@ -150,6 +150,12 @@ config FPGA_DFL_FME
> >> > > > FPGA platform level management features. There shall be 1 FME
> >> > > > per DFL based FPGA device.
> >> > > >
> >> > > > +config FPGA_DFL_FME_MGR
> >> > > > + tristate "FPGA DFL FME Manager Driver"
> >> > > > + depends on FPGA_DFL_FME
> >> > > > + help
> >> > > > + Say Y to enable FPGA Manager driver for FPGA Management Engine.
> >> > > > +
> >> > > > config FPGA_DFL_PCI
> >> > > > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> >> > > > depends on PCI && FPGA_DFL
> >> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> >> > > > index 3c44fc9..f82814a 100644
> >> > > > --- a/drivers/fpga/Makefile
> >> > > > +++ b/drivers/fpga/Makefile
> >> > > > @@ -31,6 +31,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> >> > > > # FPGA Device Feature List Support
> >> > > > obj-$(CONFIG_FPGA_DFL) += dfl.o
> >> > > > obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> >> > > > +obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> >> > > >
> >> > > > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> >> > > >
> >> > > > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> >> > > > new file mode 100644
> >> > > > index 0000000..2f92c29
> >> > > > --- /dev/null
> >> > > > +++ b/drivers/fpga/dfl-fme-mgr.c
> >> > > > @@ -0,0 +1,290 @@
> >> > > > +// SPDX-License-Identifier: GPL-2.0
> >> > > > +/*
> >> > > > + * FPGA Manager Driver for FPGA Management Engine (FME)
> >> > > > + *
> >> > > > + * Copyright (C) 2017 Intel Corporation, Inc.
> >> > > > + *
> >> > > > + * Authors:
> >> > > > + * Kang Luwei <luwei.kang@intel.com>
> >> > > > + * Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >> > > > + * Wu Hao <hao.wu@intel.com>
> >> > > > + * Joseph Grecco <joe.grecco@intel.com>
> >> > > > + * Enno Luebbers <enno.luebbers@intel.com>
> >> > > > + * Tim Whisonant <tim.whisonant@intel.com>
> >> > > > + * Ananda Ravuri <ananda.ravuri@intel.com>
> >> > > > + * Christopher Rauer <christopher.rauer@intel.com>
> >> > > > + * Henry Mitchel <henry.mitchel@intel.com>
> >> > > > + */
> >> > > > +
> >> > > > +#include <linux/bitfield.h>
> >> > > > +#include <linux/module.h>
> >> > > > +#include <linux/iopoll.h>
> >> > > > +#include <linux/fpga/fpga-mgr.h>
> >> > > > +
> >> > > > +#include "dfl-fme-pr.h"
> >> > > > +
> >> > > > +#define PR_WAIT_TIMEOUT 8000000
> >> > > > +#define PR_HOST_STATUS_IDLE 0
> >> > > > +
> >> > > > +struct fme_mgr_priv {
> >> > > > + void __iomem *ioaddr;
> >> > > > + u64 pr_error;
> >> > > > +};
> >> > > > +
> >> > > > +static u64 pr_error_to_mgr_status(u64 err)
> >> > > > +{
> >> > > > + u64 status = 0;
> >> > > > +
> >> > > > + if (err & FME_PR_ERR_OPERATION_ERR)
> >> > > > + status |= FPGA_MGR_STATUS_OPERATION_ERR;
> >> > > > + if (err & FME_PR_ERR_CRC_ERR)
> >> > > > + status |= FPGA_MGR_STATUS_CRC_ERR;
> >> > > > + if (err & FME_PR_ERR_INCOMPATIBLE_BS)
> >> > > > + status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
> >> > > > + if (err & FME_PR_ERR_PROTOCOL_ERR)
> >> > > > + status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
> >> > > > + if (err & FME_PR_ERR_FIFO_OVERFLOW)
> >> > > > + status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
> >> > > > +
> >> > > > + return status;
> >> > > > +}
> >> > > > +
> >> > > > +static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
> >> > > > +{
> >> > > > + u64 pr_status, pr_error;
> >> > > > +
> >> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> >> > > > + if (!(pr_status & FME_PR_STS_PR_STS))
> >> > > > + return 0;
> >> > > > +
> >> > > > + pr_error = readq(fme_pr + FME_PR_ERR);
> >> > > > + writeq(pr_error, fme_pr + FME_PR_ERR);
> >> > > > +
> >> > > > + return pr_error;
> >> > > > +}
> >> > > > +
> >> > > > +static int fme_mgr_write_init(struct fpga_manager *mgr,
> >> > > > + struct fpga_image_info *info,
> >> > > > + const char *buf, size_t count)
> >> > > > +{
> >> > > > + struct device *dev = &mgr->dev;
> >> > > > + struct fme_mgr_priv *priv = mgr->priv;
> >> > > > + void __iomem *fme_pr = priv->ioaddr;
> >> > > > + u64 pr_ctrl, pr_status;
> >> > > > +
> >> > > > + if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> >> > > > + dev_err(dev, "only supports partial reconfiguration.\n");
> >> > > > + return -EINVAL;
> >> > > > + }
> >> > > > +
> >> > > > + dev_dbg(dev, "resetting PR before initiated PR\n");
> >> > > > +
> >> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> >> > > > + pr_ctrl |= FME_PR_CTRL_PR_RST;
> >> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> >> > > > +
> >> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> >> > > > + pr_ctrl & FME_PR_CTRL_PR_RSTACK, 1,
> >> > > > + PR_WAIT_TIMEOUT)) {
> >> > > > + dev_err(dev, "PR Reset ACK timeout\n");
> >> > > > + return -ETIMEDOUT;
> >> > > > + }
> >> > > > +
> >> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> >> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RST;
> >> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> >> > > > +
> >> > > > + dev_dbg(dev,
> >> > > > + "waiting for PR resource in HW to be initialized and ready\n");
> >> > > > +
> >> > > > + if (readq_poll_timeout(fme_pr + FME_PR_STS, pr_status,
> >> > > > + (pr_status & FME_PR_STS_PR_STS) ==
> >> > > > + FME_PR_STS_PR_STS_IDLE, 1, PR_WAIT_TIMEOUT)) {
> >> > > > + dev_err(dev, "PR Status timeout\n");
> >> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> >> > > > + return -ETIMEDOUT;
> >> > > > + }
> >> > > > +
> >> > > > + dev_dbg(dev, "check and clear previous PR error\n");
> >> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> >> > > > + if (priv->pr_error)
> >> > > > + dev_dbg(dev, "previous PR error detected %llx\n",
> >> > > > + (unsigned long long)priv->pr_error);
> >> > > > +
> >> > > > + dev_dbg(dev, "set PR port ID\n");
> >> > > > +
> >> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> >> > > > + pr_ctrl &= ~FME_PR_CTRL_PR_RGN_ID;
> >> > > > + pr_ctrl |= FIELD_PREP(FME_PR_CTRL_PR_RGN_ID, info->region_id);
> >> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> >> > > > +
> >> > > > + return 0;
> >> > > > +}
> >> > > > +
> >> > > > +static int fme_mgr_write(struct fpga_manager *mgr,
> >> > > > + const char *buf, size_t count)
> >> > > > +{
> >> > > > + struct device *dev = &mgr->dev;
> >> > > > + struct fme_mgr_priv *priv = mgr->priv;
> >> > > > + void __iomem *fme_pr = priv->ioaddr;
> >> > > > + u64 pr_ctrl, pr_status, pr_data;
> >> > > > + int delay = 0, pr_credit, i = 0;
> >> > > > +
> >> > > > + dev_dbg(dev, "start request\n");
> >> > > > +
> >> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> >> > > > + pr_ctrl |= FME_PR_CTRL_PR_START;
> >> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> >> > > > +
> >> > > > + dev_dbg(dev, "pushing data from bitstream to HW\n");
> >> > > > +
> >> > > > + /*
> >> > > > + * driver can push data to PR hardware using PR_DATA register once HW
> >> > > > + * has enough pr_credit (> 1), pr_credit reduces one for every 32bit
> >> > > > + * pr data write to PR_DATA register. If pr_credit <= 1, driver needs
> >> > > > + * to wait for enough pr_credit from hardware by polling.
> >> > > > + */
> >> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> >> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> >> > > > +
> >> > > > + while (count > 0) {
> >> > > > + while (pr_credit <= 1) {
> >> > > > + if (delay++ > PR_WAIT_TIMEOUT) {
> >> > > > + dev_err(dev, "PR_CREDIT timeout\n");
> >> > > > + return -ETIMEDOUT;
> >> > > > + }
> >> > > > + udelay(1);
> >> > > > +
> >> > > > + pr_status = readq(fme_pr + FME_PR_STS);
> >> > > > + pr_credit = FIELD_GET(FME_PR_STS_PR_CREDIT, pr_status);
> >> > > > + }
> >> > > > +
> >> > > > + if (count >= 4) {
> >> > > > + 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;
> >> > > > + pr_credit--;
> >> > > > + i++;
> >> > > > + } else {
> >> > > > + WARN_ON(1);
> >> > > > + return -EINVAL;
> >> > > > + }
>
> Where there's an if..else that has a 'return' on one of its branch, it
> can become an 'if' without the 'else'. This is strongly preferred in
> the kernel. Trying to avoid indention where possible. So this
> becomes
>
> if (count < 4) {
> WARN_ON(1);
> return -EINVAL;
> }
>
> pr_data = 0;
> so on...
Agree, code looks better with your suggestion.
I will fix this problem in the next verion. Thanks for the comments.
Hao
>
> Alan
>
> >> > > > + }
> >> > > > +
> >> > > > + return 0;
> >> > > > +}
> >> > > > +
> >> > > > +static int fme_mgr_write_complete(struct fpga_manager *mgr,
> >> > > > + struct fpga_image_info *info)
> >> > > > +{
> >> > > > + struct device *dev = &mgr->dev;
> >> > > > + struct fme_mgr_priv *priv = mgr->priv;
> >> > > > + void __iomem *fme_pr = priv->ioaddr;
> >> > > > + u64 pr_ctrl;
> >> > > > +
> >> > > > + pr_ctrl = readq(fme_pr + FME_PR_CTRL);
> >> > > > + pr_ctrl |= FME_PR_CTRL_PR_COMPLETE;
> >> > > > + writeq(pr_ctrl, fme_pr + FME_PR_CTRL);
> >> > > > +
> >> > > > + dev_dbg(dev, "green bitstream push complete\n");
> >> > > > + dev_dbg(dev, "waiting for HW to release PR resource\n");
> >> > > > +
> >> > > > + if (readq_poll_timeout(fme_pr + FME_PR_CTRL, pr_ctrl,
> >> > > > + !(pr_ctrl & FME_PR_CTRL_PR_START), 1,
> >> > > > + PR_WAIT_TIMEOUT)) {
> >> > > > + dev_err(dev, "PR Completion ACK timeout.\n");
> >> > > > + return -ETIMEDOUT;
> >> > > > + }
> >> > > > +
> >> > > > + dev_dbg(dev, "PR operation complete, checking status\n");
> >> > > > + priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> >> > > > + if (priv->pr_error) {
> >> > > > + dev_dbg(dev, "PR error detected %llx\n",
> >> > > > + (unsigned long long)priv->pr_error);
> >> > > > + return -EIO;
> >> > > > + }
> >> > > > +
> >> > > > + dev_dbg(dev, "PR done successfully\n");
> >> > > > +
> >> > > > + return 0;
> >> > > > +}
> >> > > > +
> >> > > > +static enum fpga_mgr_states fme_mgr_state(struct fpga_manager *mgr)
> >> > > > +{
> >> > > > + return FPGA_MGR_STATE_UNKNOWN;
> >> > > > +}
> >> > > > +
> >> > > > +static u64 fme_mgr_status(struct fpga_manager *mgr)
> >> > > > +{
> >> > > > + struct fme_mgr_priv *priv = mgr->priv;
> >> > > > +
> >> > > > + return pr_error_to_mgr_status(priv->pr_error);
> >> > > > +}
> >> > > > +
> >> > > > +static const struct fpga_manager_ops fme_mgr_ops = {
> >> > > > + .write_init = fme_mgr_write_init,
> >> > > > + .write = fme_mgr_write,
> >> > > > + .write_complete = fme_mgr_write_complete,
> >> > > > + .state = fme_mgr_state,
> >> > > > + .status = fme_mgr_status,
> >> > > > +};
> >> > > > +
> >> > > > +static int fme_mgr_probe(struct platform_device *pdev)
> >> > > > +{
> >> > > > + struct device *dev = &pdev->dev;
> >> > > > + struct fme_mgr_priv *priv;
> >> > > > + struct fpga_manager *mgr;
> >> > > > + struct resource *res;
> >> > > > + int ret;
> >> > > > +
> >> > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> > > > + if (!priv)
> >> > > > + return -ENOMEM;
> >> > > > +
> >> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > > > + priv->ioaddr = devm_ioremap(dev, res->start, resource_size(res));
> >> > >
> >> > > How about using devm_ioremap_resourc(dev, res) here instead?
> >> >
> >> > Actually the register region has already been mapped in lower level driver
> >> > (e.g pci) so I think we don't have to map the second time here. I plan to
> >> > add some code to pass the ioaddr via the platform data, and check if valid
> >> > ioaddr from the platform data firstly in this probe function. If no pdata
> >> > or no valid ioaddr, then go with devm_ioremap_resource. :)
> >>
> >> If you end up sharing register spaces between drivers is regmap / syscon
> >> maybe a good idea?
> >
> > Hi Moritz,
> >
> > Thanks for the comments. I tried the regmap_mmio before but it failed to use
> > regmap_read and regmap_write for register access. It requires to use writeq /
> > readq for 64bit registers on the hardware, but regmap_read and regmap_write
> > only accepts unsigned int value as input parameter. So I only replaced the
> > bitfields struct/union with macro.
> >
> > Thanks
> > Hao
> >
> >> >
> >> > >
> >> > > > + if (IS_ERR(priv->ioaddr))
> >> > > > + return PTR_ERR(priv->ioaddr);
> >> > > > +
> >> > > > + mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
> >> > > > + if (!mgr)
> >> > > > + return -ENOMEM;
> >> > > > +
> >> > > > + mgr->name = "DFL FPGA Manager";
> >> > > > + mgr->mops = &fme_mgr_ops;
> >> > > > + mgr->priv = priv;
> >> > > > + mgr->parent = dev;
> >> > > > + platform_set_drvdata(pdev, mgr);
> >> > > > +
> >> > > > + ret = fpga_mgr_register(mgr);
> >> > > > + if (ret)
> >> > > > + dev_err(dev, "unable to register FPGA manager\n");
> >> > > > +
> >> > > > + return ret;
> >> > >
> >> > > You can probably just do "return fpga_mgr_register(mgr);" here.
> >> >
> >> > Yes, it looks better, I will fix it. Thanks a lot for the review.
> >> >
> >> > Hao
> >> >
> >> > >
> >> > > Thanks,
> >> > > Alan
> >> > >
> >> > > > +}
> >> > > > +
> >> > > > +static int fme_mgr_remove(struct platform_device *pdev)
> >> > > > +{
> >> > > > + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> >> > > > +
> >> > > > + fpga_mgr_unregister(mgr);
> >> > > > +
> >> > > > + return 0;
> >> > > > +}
> >> > > > +
> >> > > > +static struct platform_driver fme_mgr_driver = {
> >> > > > + .driver = {
> >> > > > + .name = FPGA_DFL_FME_MGR,
> >> > > > + },
> >> > > > + .probe = fme_mgr_probe,
> >> > > > + .remove = fme_mgr_remove,
> >> > > > +};
> >> > > > +
> >> > > > +module_platform_driver(fme_mgr_driver);
> >> > > > +
> >> > > > +MODULE_DESCRIPTION("FPGA Manager for DFL FPGA Management Engine");
> >> > > > +MODULE_AUTHOR("Intel Corporation");
> >> > > > +MODULE_LICENSE("GPL v2");
> >> > > > +MODULE_ALIAS("platform:dfl-fme-mgr");
> >> > > > --
> >> > > > 2.7.4
> >> > > >
> >>
> >> Cheers,
> >> Moritz
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/6] aio: refactor read/write iocb setup
From: Al Viro @ 2018-04-06 12:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180406071011.GA21308@lst.de>
On Fri, Apr 06, 2018 at 09:10:11AM +0200, Christoph Hellwig wrote:
> On Fri, Apr 06, 2018 at 04:21:46AM +0100, Al Viro wrote:
> > On Wed, Mar 28, 2018 at 09:26:36AM +0200, Christoph Hellwig wrote:
> > > + struct inode *inode = file_inode(file);
> > > +
> > > req->ki_flags |= IOCB_WRITE;
> > > file_start_write(file);
> > > - ret = aio_ret(req, call_write_iter(file, req, &iter));
> > > + ret = aio_rw_ret(req, call_write_iter(file, req, &iter));
> > > /*
> > > - * We release freeze protection in aio_complete(). Fool lockdep
> > > - * by telling it the lock got released so that it doesn't
> > > - * complain about held lock when we return to userspace.
> > > + * We release freeze protection in aio_complete_rw(). Fool
> > > + * lockdep by telling it the lock got released so that it
> > > + * doesn't complain about held lock when we return to userspace.
> > > */
> > > - if (S_ISREG(file_inode(file)->i_mode))
> > > - __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> > > + if (S_ISREG(inode->i_mode))
> >
> > ... and that's another use-after-free, since we might've already done fput() of
> > that sucker by that point.
>
> Indeed. Not in any way new in this patch, this is an existing issue
> dating way back that needs to be fixed, which will be rather annoying
> without taking an extra reference to the inode or at least sb.
New, actually - mainline has get_file()/fput() around the equivalent area.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: io_pgetevents & aio fsync V2
From: Jeff Moyer @ 2018-04-06 12:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Al Viro, Avi Kivity, linux-aio, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <20180406062700.GA20714@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> On Fri, Apr 06, 2018 at 04:16:30AM +0100, Al Viro wrote:
>> BTW, this is only tangentially related, but... does *anything* call
>> io_submit() for huge amounts of iocb?
I don't know. If an application did that, as many I/Os as could fit
into the ring buffer would be submitted, and that's what gets returned
from the system call (the number of submitted iocbs).
>> Check in do_io_submit() is insane - "no more than MAX_LONG total of
>> _pointers_". Compat variant goes for "no more than a page worth of
>> pointers" and there's a hard limit in ioctx_alloc() - we can't ever
>> get more than 8M slots in ring buffer...
>
> Logical upper bound for io_submit is nr_events passed to io_setup(),
> which is bound by aio_max_nr. Except that we never actually check
> against nr_events (or max_reqs as it is known in kernel) in io_submit.
> Sigh..
io_submit_one calls aio_get_req which calls get_reqs_available, which is
what does the checking for an available ring buffer entry.
-Jeff
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* [PATCH 3.18 84/93] fs/proc: Stop trying to report thread stacks
From: Greg Kroah-Hartman @ 2018-04-06 13:23 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Jann Horn, Andy Lutomirski,
Thomas Gleixner, Al Viro, Andrew Morton, Borislav Petkov,
Brian Gerst, Johannes Weiner, Kees Cook, Linus Torvalds,
Linux API, Peter Zijlstra, Tycho Andersen, Ingo Molnar
In-Reply-To: <20180406084224.918716300@linuxfoundation.org>
3.18-stable review patch. If anyone has any objections, please let me know.
------------------
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/proc.txt | 26 --------------------------
fs/proc/task_mmu.c | 29 ++++++++++-------------------
fs/proc/task_nommu.c | 26 +++++++++-----------------
3 files changed, 19 insertions(+), 62 deletions(-)
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -371,32 +371,6 @@ is not associated with a file:
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. Hence, for the example above, the
-task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -248,24 +248,15 @@ static int do_maps_open(struct inode *in
* /proc/PID/maps that is the stack of the main task.
*/
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
- int stack = 0;
-
- if (is_pid) {
- stack = vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
}
static void
@@ -332,7 +323,7 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- if (is_stack(priv, vma, is_pid))
+ if (is_stack(priv, vma))
name = "[stack]";
}
@@ -1501,7 +1492,7 @@ static int show_numa_map(struct seq_file
seq_path(m, &file->f_path, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else if (is_stack(proc_priv, vma, is_pid)) {
+ } else if (is_stack(proc_priv, vma)) {
seq_puts(m, " stack");
}
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -124,25 +124,17 @@ unsigned long task_statm(struct mm_struc
}
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
struct mm_struct *mm = vma->vm_mm;
- int stack = 0;
- if (is_pid) {
- stack = vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
}
/*
@@ -184,7 +176,7 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_path(m, &file->f_path, "");
- } else if (mm && is_stack(priv, vma, is_pid)) {
+ } else if (mm && is_stack(priv, vma)) {
seq_pad(m, ' ');
seq_printf(m, "[stack]");
}
^ permalink raw reply
* [PATCH 4.4 53/72] fs/proc: Stop trying to report thread stacks
From: Greg Kroah-Hartman @ 2018-04-06 13:23 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Jann Horn, Andy Lutomirski,
Thomas Gleixner, Al Viro, Andrew Morton, Borislav Petkov,
Brian Gerst, Johannes Weiner, Kees Cook, Linus Torvalds,
Linux API, Peter Zijlstra, Tycho Andersen, Ingo Molnar
In-Reply-To: <20180406084305.210085169@linuxfoundation.org>
4.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Andy Lutomirski <luto@kernel.org>
commit b18cb64ead400c01bf1580eeba330ace51f8087d upstream.
This reverts more of:
b76437579d13 ("procfs: mark thread stack correctly in proc/<pid>/maps")
... which was partially reverted by:
65376df58217 ("proc: revert /proc/<pid>/maps [stack:TID] annotation")
Originally, /proc/PID/task/TID/maps was the same as /proc/TID/maps.
In current kernels, /proc/PID/maps (or /proc/TID/maps even for
threads) shows "[stack]" for VMAs in the mm's stack address range.
In contrast, /proc/PID/task/TID/maps uses KSTK_ESP to guess the
target thread's stack's VMA. This is racy, probably returns garbage
and, on arches with CONFIG_TASK_INFO_IN_THREAD=y, is also crash-prone:
KSTK_ESP is not safe to use on tasks that aren't known to be running
ordinary process-context kernel code.
This patch removes the difference and just shows "[stack]" for VMAs
in the mm's stack range. This is IMO much more sensible -- the
actual "stack" address really is treated specially by the VM code,
and the current thread stack isn't even well-defined for programs
that frequently switch stacks on their own.
Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Link: http://lkml.kernel.org/r/3e678474ec14e0a0ec34c611016753eea2e1b8ba.1475257877.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/proc.txt | 26 --------------------------
fs/proc/task_mmu.c | 29 ++++++++++-------------------
fs/proc/task_nommu.c | 26 +++++++++-----------------
3 files changed, 19 insertions(+), 62 deletions(-)
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -383,32 +383,6 @@ is not associated with a file:
or if empty, the mapping is anonymous.
-The /proc/PID/task/TID/maps is a view of the virtual memory from the viewpoint
-of the individual tasks of a process. In this file you will see a mapping marked
-as [stack] if that task sees it as a stack. Hence, for the example above, the
-task-level map, i.e. /proc/PID/task/TID/maps for thread 1001 will look like this:
-
-08048000-08049000 r-xp 00000000 03:00 8312 /opt/test
-08049000-0804a000 rw-p 00001000 03:00 8312 /opt/test
-0804a000-0806b000 rw-p 00000000 00:00 0 [heap]
-a7cb1000-a7cb2000 ---p 00000000 00:00 0
-a7cb2000-a7eb2000 rw-p 00000000 00:00 0
-a7eb2000-a7eb3000 ---p 00000000 00:00 0
-a7eb3000-a7ed5000 rw-p 00000000 00:00 0 [stack]
-a7ed5000-a8008000 r-xp 00000000 03:00 4222 /lib/libc.so.6
-a8008000-a800a000 r--p 00133000 03:00 4222 /lib/libc.so.6
-a800a000-a800b000 rw-p 00135000 03:00 4222 /lib/libc.so.6
-a800b000-a800e000 rw-p 00000000 00:00 0
-a800e000-a8022000 r-xp 00000000 03:00 14462 /lib/libpthread.so.0
-a8022000-a8023000 r--p 00013000 03:00 14462 /lib/libpthread.so.0
-a8023000-a8024000 rw-p 00014000 03:00 14462 /lib/libpthread.so.0
-a8024000-a8027000 rw-p 00000000 00:00 0
-a8027000-a8043000 r-xp 00000000 03:00 8317 /lib/ld-linux.so.2
-a8043000-a8044000 r--p 0001b000 03:00 8317 /lib/ld-linux.so.2
-a8044000-a8045000 rw-p 0001c000 03:00 8317 /lib/ld-linux.so.2
-aff35000-aff4a000 rw-p 00000000 00:00 0
-ffffe000-fffff000 r-xp 00000000 00:00 0 [vdso]
-
The /proc/PID/smaps is an extension based on maps, showing the memory
consumption for each of the process's mappings. For each of mappings there
is a series of lines such as the following:
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -253,24 +253,15 @@ static int do_maps_open(struct inode *in
* /proc/PID/maps that is the stack of the main task.
*/
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
- int stack = 0;
-
- if (is_pid) {
- stack = vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= vma->vm_mm->start_stack &&
+ vma->vm_end >= vma->vm_mm->start_stack;
}
static void
@@ -337,7 +328,7 @@ show_map_vma(struct seq_file *m, struct
goto done;
}
- if (is_stack(priv, vma, is_pid))
+ if (is_stack(priv, vma))
name = "[stack]";
}
@@ -1560,7 +1551,7 @@ static int show_numa_map(struct seq_file
seq_file_path(m, file, "\n\t= ");
} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
seq_puts(m, " heap");
- } else if (is_stack(proc_priv, vma, is_pid)) {
+ } else if (is_stack(proc_priv, vma)) {
seq_puts(m, " stack");
}
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -124,25 +124,17 @@ unsigned long task_statm(struct mm_struc
}
static int is_stack(struct proc_maps_private *priv,
- struct vm_area_struct *vma, int is_pid)
+ struct vm_area_struct *vma)
{
struct mm_struct *mm = vma->vm_mm;
- int stack = 0;
- if (is_pid) {
- stack = vma->vm_start <= mm->start_stack &&
- vma->vm_end >= mm->start_stack;
- } else {
- struct inode *inode = priv->inode;
- struct task_struct *task;
-
- rcu_read_lock();
- task = pid_task(proc_pid(inode), PIDTYPE_PID);
- if (task)
- stack = vma_is_stack_for_task(vma, task);
- rcu_read_unlock();
- }
- return stack;
+ /*
+ * We make no effort to guess what a given thread considers to be
+ * its "stack". It's not even well-defined for programs written
+ * languages like Go.
+ */
+ return vma->vm_start <= mm->start_stack &&
+ vma->vm_end >= mm->start_stack;
}
/*
@@ -184,7 +176,7 @@ static int nommu_vma_show(struct seq_fil
if (file) {
seq_pad(m, ' ');
seq_file_path(m, file, "");
- } else if (mm && is_stack(priv, vma, is_pid)) {
+ } else if (mm && is_stack(priv, vma)) {
seq_pad(m, ' ');
seq_printf(m, "[stack]");
}
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: joeyli @ 2018-04-08 7:44 UTC (permalink / raw)
To: Matthew Garrett
Cc: alexei.starovoitov, luto, David Howells, Ard Biesheuvel, jmorris,
Alan Cox, Linus Torvalds, Greg Kroah-Hartman,
Linux Kernel Mailing List, jforbes, linux-man, LSM List,
linux-api, Kees Cook, linux-efi
In-Reply-To: <CACdnJuv8t3kS5BCowWQ7rMjL2K73913wX2-bb5HF0ddY3iaBHg@mail.gmail.com>
On Wed, Apr 04, 2018 at 04:31:46AM +0000, Matthew Garrett wrote:
> On Tue, Apr 3, 2018 at 7:34 PM Alexei Starovoitov <
> alexei.starovoitov@gmail.com> wrote:
> > If the only thing that folks are paranoid about is reading
> > arbitrary kernel memory with bpf_probe_read() helper
> > then preferred patch would be to disable it during verification
> > when in lockdown mode.
> > No run-time overhead and android folks will be happy
> > that lockdown doesn't break their work.
> > They converted out-of-tree networking accounting
> > module and corresponding user daemon to use bpf:
>
> https://www.linuxplumbersconf.org/2017/ocw/system/presentations/4791/original/eBPF%20cgroup%20filters%20for%20data%20usage%20accounting%20on%20Android.pdf
>
> An alternative would be to only disable kernel reads if the kernel contains
> secrets that aren't supposed to be readable by root. If the keyring is
> configured such that root can read everything, it seems like less of a
> concern?
Currently the KMK (kernel master key) can be a trusted key (TPM sealed) or a
user key (plaintext). The EVM keeps a key (plaintext) in memory that
it is decrypted from a KMK encrypted key. Those kernel reads functions should
be disabled when the KMK be loaded to keyring.
You idea is good that kernel can keep those reads functions enabled until KMK
be loaded. Which means those functions are still available before user enables
KMK and EVM.
There have another idea is using a tree to register all sensitive data
then blanking them when reading. Here is a very early developing version:
https://github.com/joeyli/linux-sensitive_data/commits/sensitive-data-tree-v0.1-v4.15
But this approach causes runtime overhead and all sensitive data address must
be found and registered (e.g. plaintext in encryption module)
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: joeyli @ 2018-04-08 8:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, David Howells, Ard Biesheuvel, James Morris,
One Thousand Gnomes, Linus Torvalds, Matthew Garrett, Greg KH,
LKML, Justin Forbes, linux-man, LSM List, Linux API, Kees Cook,
linux-efi
In-Reply-To: <CAADnVQLu5EZLJ1C9-dd4=K+sed6+38oy047+nqCMfuPFB=wttw@mail.gmail.com>
On Tue, Apr 03, 2018 at 07:34:25PM -0700, Alexei Starovoitov wrote:
> On Tue, Apr 3, 2018 at 9:26 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > On Tue, Apr 3, 2018 at 8:41 AM, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >> On Tue, Apr 03, 2018 at 08:11:07AM -0700, Andy Lutomirski wrote:
> >>> >
> >>> >> "bpf: Restrict kernel image access functions when the kernel is locked down":
> >>> >> This patch just sucks in general.
> >>> >
> >>> > Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since
> >>> > it gives you unrestricted access to the kernel.
> >>>
> >>> bpf, in certain contexts, gives you unrestricted access to *reading*
> >>> kernel memory. bpf should, under no circumstances, let you write to
> >>> the kernel unless you're using fault injection or similar.
> >>>
> >>> I'm surprised that Alexei acked this patch. If something like XDP or
> >>> bpfilter starts becoming widely used, this patch will require a lot of
> >>> reworking to avoid breaking standard distros.
> >>
> >> my understanding was that this lockdown set attemps to disallow _reads_
> >> of kernel memory from anything, so first version of patch was adding
> >> run-time checks for bpf_probe_read() which is no-go
> >> and without this helper the bpf for tracing is losing a lot of its power,
> >> so the easiest is to disable it all.
> >
> > Fair enough.
>
> Actually looking at the patch again:
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=efi-lock-down&id=78bb0059c3b8304a8d124b55feebc780fb3e0500
>
The bpf is totally disabled in this patch.
> If the only thing that folks are paranoid about is reading
> arbitrary kernel memory with bpf_probe_read() helper
> then preferred patch would be to disable it during verification
> when in lockdown mode
Sorry for I didn't fully understand your idea...
Do you mean that using bpf verifier to filter out bpf program that
uses bpf_probe_read()?
.
> No run-time overhead and android folks will be happy
> that lockdown doesn't break their work.
> They converted out-of-tree networking accounting
> module and corresponding user daemon to use bpf:
> https://www.linuxplumbersconf.org/2017/ocw/system/presentations/4791/original/eBPF%20cgroup%20filters%20for%20data%20usage%20accounting%20on%20Android.pdf
Thanks
Joey Lee
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Pavel Machek @ 2018-04-08 8:10 UTC (permalink / raw)
To: Matthew Garrett
Cc: Linus Torvalds, luto, David Howells, Ard Biesheuvel, jmorris,
Alan Cox, Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes,
linux-man, jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <CACdnJuvheCbuvTLyDWMQWcUytc4EvuSUaEKywfEX8Kp6qYL9dg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]
On Tue 2018-04-03 21:08:54, Matthew Garrett wrote:
> On Tue, Apr 3, 2018 at 2:01 PM Linus Torvalds
> <torvalds@linux-foundation.org>
> wrote:
>
> > On Tue, Apr 3, 2018 at 1:54 PM, Matthew Garrett <mjg59@google.com> wrote:
> > >
> > >> .. maybe you don't *want* secure boot, but it's been pushed in your
> > >> face by people with an agenda?
> > >
> > > Then turn it off, or build a self-signed kernel that doesn't do this?
>
> > Umm. So you asked a question, and then when you got an answer you said
> > "don't do that then".
>
> > The fact is, some hardware pushes secure boot pretty hard. That has
> > *nothing* to do with some "lockdown" mode.
>
> Secure Boot ensures that the firmware will only load signed bootloaders. If
> a signed bootloader loads a kernel that's effectively an unsigned
> bootloader, there's no point in using Secure Boot - you should just turn it
> off instead, because it's not giving you any meaningful
> security. Andy's
Not true.
I have kernel with printk() enabled. Yes, once userland is started,
you can boot another kernel, maybe.
Maybe my kernel is locked down with exception of kexec, and it does
printk(KERN_CRIT "kexecing") followed by mdelay(5000). That's pretty
good security.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Pavel Machek @ 2018-04-08 8:23 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Andy Lutomirski, Matthew Garrett, Ard Biesheuvel,
James Morris, Alan Cox, Greg Kroah-Hartman,
Linux Kernel Mailing List, Justin Forbes, linux-man, joeyli,
LSM List, Linux API, Kees Cook, linux-efi
In-Reply-To: <10718.1522798745@warthog.procyon.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]
On Wed 2018-04-04 00:39:05, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > The same thing is true of some lockdown patch. Maybe it's a good thing
> > in general. But whether it's a good thing is _entirely_ independent of
> > any secure boot issue. I can see using secure boot without it, but I
> > can very much also see using lockdown without secure boot.
> >
> > The two things are simply entirely orthogonal. They have _zero_
> > overlap. I'm not seeing why they'd be linked at all in any way.
>
> I'm not sure I agree. Here's my reasoning:
>
> (1) Lockdown mode really needs to activated during kernel boot, before
> userspace has a chance to run, otherwise there's a window of opportunity
> in which the kernel *isn't* locked down.
>
> (2) If the kernel isn't booted in secure boot mode, then there's the
> opportunity to tamper before the kernel even starts booting.
>
> (3) There doesn't seem any point in booting in secure boot mode if you don't
> protect the running kernel image against tampering. What does it mean to
> be in "secure boot mode" in that case? If the kernel can be tampered
> with, it would seem to be, by definition, insecure.
This one is not true, either.
If kernel does "printk(KERN_CRIT "loading unsigned module");
mdelay(10000);", it is useful for secure boot and provides way to
owner to play.
Nokia N9 / N950 uses this kind of "security" for example. It is rather
annoying but better than not being able to run custom kernels at all.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
From: Mickaël Salaün @ 2018-04-08 13:13 UTC (permalink / raw)
To: Andy Lutomirski, Alexei Starovoitov, Daniel Borkmann
Cc: LKML, Alexei Starovoitov, Arnaldo Carvalho de Melo,
Casey Schaufler, David Drysdale, David S . Miller,
Eric W . Biederman, Jann Horn, Jonathan Corbet, Michael Kerrisk,
Kees Cook, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Tycho Andersen, Will Drewry,
Kernel Hardening, Linux API
In-Reply-To: <ab8dda73-4a6e-4e10-cda0-3e91c5019a63@digikod.net>
[-- Attachment #1.1: Type: text/plain, Size: 18229 bytes --]
On 02/27/2018 10:48 PM, Mickaël Salaün wrote:
>
> On 27/02/2018 17:39, Andy Lutomirski wrote:
>> On Tue, Feb 27, 2018 at 5:32 AM, Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Tue, Feb 27, 2018 at 05:20:55AM +0000, Andy Lutomirski wrote:
>>>> On Tue, Feb 27, 2018 at 4:54 AM, Alexei Starovoitov
>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>> On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
>>>>>> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>> On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
>>>>>>>> The seccomp(2) syscall can be used by a task to apply a Landlock program
>>>>>>>> to itself. As a seccomp filter, a Landlock program is enforced for the
>>>>>>>> current task and all its future children. A program is immutable and a
>>>>>>>> task can only add new restricting programs to itself, forming a list of
>>>>>>>> programss.
>>>>>>>>
>>>>>>>> A Landlock program is tied to a Landlock hook. If the action on a kernel
>>>>>>>> object is allowed by the other Linux security mechanisms (e.g. DAC,
>>>>>>>> capabilities, other LSM), then a Landlock hook related to this kind of
>>>>>>>> object is triggered. The list of programs for this hook is then
>>>>>>>> evaluated. Each program return a 32-bit value which can deny the action
>>>>>>>> on a kernel object with a non-zero value. If every programs of the list
>>>>>>>> return zero, then the action on the object is allowed.
>>>>>>>>
>>>>>>>> Multiple Landlock programs can be chained to share a 64-bits value for a
>>>>>>>> call chain (e.g. evaluating multiple elements of a file path). This
>>>>>>>> chaining is restricted when a process construct this chain by loading a
>>>>>>>> program, but additional checks are performed when it requests to apply
>>>>>>>> this chain of programs to itself. The restrictions ensure that it is
>>>>>>>> not possible to call multiple programs in a way that would imply to
>>>>>>>> handle multiple shared values (i.e. cookies) for one chain. For now,
>>>>>>>> only a fs_pick program can be chained to the same type of program,
>>>>>>>> because it may make sense if they have different triggers (cf. next
>>>>>>>> commits). This restrictions still allows to reuse Landlock programs in
>>>>>>>> a safe way (e.g. use the same loaded fs_walk program with multiple
>>>>>>>> chains of fs_pick programs).
>>>>>>>>
>>>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>> +struct landlock_prog_set *landlock_prepend_prog(
>>>>>>>> + struct landlock_prog_set *current_prog_set,
>>>>>>>> + struct bpf_prog *prog)
>>>>>>>> +{
>>>>>>>> + struct landlock_prog_set *new_prog_set = current_prog_set;
>>>>>>>> + unsigned long pages;
>>>>>>>> + int err;
>>>>>>>> + size_t i;
>>>>>>>> + struct landlock_prog_set tmp_prog_set = {};
>>>>>>>> +
>>>>>>>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>> +
>>>>>>>> + /* validate memory size allocation */
>>>>>>>> + pages = prog->pages;
>>>>>>>> + if (current_prog_set) {
>>>>>>>> + size_t i;
>>>>>>>> +
>>>>>>>> + for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
>>>>>>>> + struct landlock_prog_list *walker_p;
>>>>>>>> +
>>>>>>>> + for (walker_p = current_prog_set->programs[i];
>>>>>>>> + walker_p; walker_p = walker_p->prev)
>>>>>>>> + pages += walker_p->prog->pages;
>>>>>>>> + }
>>>>>>>> + /* count a struct landlock_prog_set if we need to allocate one */
>>>>>>>> + if (refcount_read(¤t_prog_set->usage) != 1)
>>>>>>>> + pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
>>>>>>>> + / PAGE_SIZE;
>>>>>>>> + }
>>>>>>>> + if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
>>>>>>>> + return ERR_PTR(-E2BIG);
>>>>>>>> +
>>>>>>>> + /* ensure early that we can allocate enough memory for the new
>>>>>>>> + * prog_lists */
>>>>>>>> + err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
>>>>>>>> + if (err)
>>>>>>>> + return ERR_PTR(err);
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * Each task_struct points to an array of prog list pointers. These
>>>>>>>> + * tables are duplicated when additions are made (which means each
>>>>>>>> + * table needs to be refcounted for the processes using it). When a new
>>>>>>>> + * table is created, all the refcounters on the prog_list are bumped (to
>>>>>>>> + * track each table that references the prog). When a new prog is
>>>>>>>> + * added, it's just prepended to the list for the new table to point
>>>>>>>> + * at.
>>>>>>>> + *
>>>>>>>> + * Manage all the possible errors before this step to not uselessly
>>>>>>>> + * duplicate current_prog_set and avoid a rollback.
>>>>>>>> + */
>>>>>>>> + if (!new_prog_set) {
>>>>>>>> + /*
>>>>>>>> + * If there is no Landlock program set used by the current task,
>>>>>>>> + * then create a new one.
>>>>>>>> + */
>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>> + goto put_tmp_lists;
>>>>>>>> + } else if (refcount_read(¤t_prog_set->usage) > 1) {
>>>>>>>> + /*
>>>>>>>> + * If the current task is not the sole user of its Landlock
>>>>>>>> + * program set, then duplicate them.
>>>>>>>> + */
>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>> + goto put_tmp_lists;
>>>>>>>> + for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
>>>>>>>> + new_prog_set->programs[i] =
>>>>>>>> + READ_ONCE(current_prog_set->programs[i]);
>>>>>>>> + if (new_prog_set->programs[i])
>>>>>>>> + refcount_inc(&new_prog_set->programs[i]->usage);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * Landlock program set from the current task will not be freed
>>>>>>>> + * here because the usage is strictly greater than 1. It is
>>>>>>>> + * only prevented to be freed by another task thanks to the
>>>>>>>> + * caller of landlock_prepend_prog() which should be locked if
>>>>>>>> + * needed.
>>>>>>>> + */
>>>>>>>> + landlock_put_prog_set(current_prog_set);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + /* prepend tmp_prog_set to new_prog_set */
>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
>>>>>>>> + /* get the last new list */
>>>>>>>> + struct landlock_prog_list *last_list =
>>>>>>>> + tmp_prog_set.programs[i];
>>>>>>>> +
>>>>>>>> + if (last_list) {
>>>>>>>> + while (last_list->prev)
>>>>>>>> + last_list = last_list->prev;
>>>>>>>> + /* no need to increment usage (pointer replacement) */
>>>>>>>> + last_list->prev = new_prog_set->programs[i];
>>>>>>>> + new_prog_set->programs[i] = tmp_prog_set.programs[i];
>>>>>>>> + }
>>>>>>>> + }
>>>>>>>> + new_prog_set->chain_last = tmp_prog_set.chain_last;
>>>>>>>> + return new_prog_set;
>>>>>>>> +
>>>>>>>> +put_tmp_lists:
>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
>>>>>>>> + put_landlock_prog_list(tmp_prog_set.programs[i]);
>>>>>>>> + return new_prog_set;
>>>>>>>> +}
>>>>>>>
>>>>>>> Nack on the chaining concept.
>>>>>>> Please do not reinvent the wheel.
>>>>>>> There is an existing mechanism for attaching/detaching/quering multiple
>>>>>>> programs attached to cgroup and tracing hooks that are also
>>>>>>> efficiently executed via BPF_PROG_RUN_ARRAY.
>>>>>>> Please use that instead.
>>>>>>>
>>>>>>
>>>>>> I don't see how that would help. Suppose you add a filter, then
>>>>>> fork(), and then the child adds another filter. Do you want to
>>>>>> duplicate the entire array? You certainly can't *modify* the array
>>>>>> because you'll affect processes that shouldn't be affected.
>>>>>>
>>>>>> In contrast, doing this through seccomp like the earlier patches
>>>>>> seemed just fine to me, and seccomp already had the right logic.
>>>>>
>>>>> it doesn't look to me that existing seccomp side of managing fork
>>>>> situation can be reused. Here there is an attempt to add 'chaining'
>>>>> concept which sort of an extension of existing seccomp style,
>>>>> but somehow heavily done on bpf side and contradicts cgroup/tracing.
>>>>>
>>>>
>>>> I don't see why the seccomp way can't be used. I agree with you that
>>>> the seccomp *style* shouldn't be used in bpf code like this, but I
>>>> think that Landlock programs can and should just live in the existing
>>>> seccomp chain. If the existing seccomp code needs some modification
>>>> to make this work, then so be it.
>>>
>>> +1
>>> if that was the case...
>>> but that's not my reading of the patch set.
>>
>> An earlier version of the patch set used the seccomp filter chain.
>> Mickaël, what exactly was wrong with that approach other than that the
>> seccomp() syscall was awkward for you to use? You could add a
>> seccomp_add_landlock_rule() syscall if you needed to.
>
> Nothing was wrong about about that, this part did not changed (see my
> next comment).
>
>>
>> As a side comment, why is this an LSM at all, let alone a non-stacking
>> LSM? It would make a lot more sense to me to make Landlock depend on
>> having LSMs configured in but to call the landlock hooks directly from
>> the security_xyz() hooks.
>
> See Casey's answer and his patch series: https://lwn.net/Articles/741963/
>
>>
>>>
>>>> In other words, the kernel already has two kinds of chaining:
>>>> seccomp's and bpf's. bpf's doesn't work right for this type of usage
>>>> across fork(), whereas seccomp's already handles that case correctly.
>>>> (In contrast, seccomp's is totally wrong for cgroup-attached filters.)
>>>> So IMO Landlock should use the seccomp core code and call into bpf
>>>> for the actual filtering.
>>>
>>> +1
>>> in cgroup we had to invent this new BPF_PROG_RUN_ARRAY mechanism,
>>> since cgroup hierarchy can be complicated with bpf progs attached
>>> at different levels with different override/multiprog properties,
>>> so walking link list and checking all flags at run-time would have
>>> been too slow. That's why we added compute_effective_progs().
>>
>> If we start adding override flags to Landlock, I think we're doing it
>> wrong. With cgroup bpf programs, the whole mess is set up by the
>> administrator. With seccomp, and with Landlock if done correctly, it
>> *won't* be set up by the administrator, so the chance that everyone
>> gets all the flags right is about zero. All attached filters should
>> run unconditionally.
>
>
> There is a misunderstanding about this chaining mechanism. This should
> not be confused with the list of seccomp filters nor the cgroup
> hierarchies. Landlock programs can be stacked the same way seccomp's
> filters can (cf. struct landlock_prog_set, the "chain_last" field is an
> optimization which is not used for this struct handling). This stackable
> property did not changed from the previous patch series. The chaining
> mechanism is for another use case, which does not make sense for seccomp
> filters nor other eBPF program types, at least for now, from what I can
> tell.
>
> You may want to get a look at my talk at FOSDEM
> (https://landlock.io/talks/2018-02-04_landlock-fosdem.pdf), especially
> slides 11 and 12.
>
> Let me explain my reasoning about this program chaining thing.
>
> To check if an action on a file is allowed, we first need to identify
> this file and match it to the security policy. In a previous
> (non-public) patch series, I tried to use one type of eBPF program to
> check every kind of access to a file. To be able to identify a file, I
> relied on an eBPF map, similar to the current inode map. This map store
> a set of references to file descriptors. I then created a function
> bpf_is_file_beneath() to check if the requested file was beneath a file
> in the map. This way, no chaining, only one eBPF program type to check
> an access to a file... but some issues then emerged. First, this design
> create a side-channel which help an attacker using such a program to
> infer some information not normally available, for example to get a hint
> on where a file descriptor (received from a UNIX socket) come from.
> Another issue is that this type of program would be called for each
> component of a path. Indeed, when the kernel check if an access to a
> file is allowed, it walk through all of the directories in its path
> (checking if the current process is allowed to execute them). That first
> attempt led me to rethink the way we could filter an access to a file
> *path*.
>
> To minimize the number of called to an eBPF program dedicated to
> validate an access to a file path, I decided to create three subtype of
> eBPF programs. The FS_WALK type is called when walking through every
> directory of a file path (except the last one if it is the target). We
> can then restrict this type of program to the minimum set of functions
> it is allowed to call and the minimum set of data available from its
> context. The first implicit chaining is for this type of program. To be
> able to evaluate a path while being called for all its components, this
> program need to store a state (to remember what was the parent directory
> of this path). There is no "previous" field in the subtype for this
> program because it is chained with itself, for each directories. This
> enable to create a FS_WALK program to evaluate a file hierarchy, thank
> to the inode map which can be used to check if a directory of this
> hierarchy is part of an allowed (or denied) list of directories. This
> design enables to express a file hierarchy in a programmatic way,
> without requiring an eBPF helper to do the job (unlike my first experiment).
>
> The explicit chaining is used to tied a path evaluation (with a FS_WALK
> program) to an access to the actual file being requested (the last
> component of a file path), with a FS_PICK program. It is only at this
> time that the kernel check for the requested action (e.g. read, write,
> chdir, append...). To be able to filter such access request we can have
> one call to the same program for every action and let this program check
> for which action it was called. However, this design does not allow the
> kernel to know if the current action is indeed handled by this program.
> Hence, it is not possible to implement a cache mechanism to only call
> this program if it knows how to handle this action.
>
> The approach I took for this FS_PICK type of program is to add to its
> subtype which action it can handle (with the "triggers" bitfield, seen
> as ORed actions). This way, the kernel knows if a call to a FS_PICK
> program is necessary. If the user wants to enforce a different security
> policy according to the action requested on a file, then it needs
> multiple FS_PICK programs. However, to reduce the number of such
> programs, this patch series allow a FS_PICK program to be chained with
> another, the same way a FS_WALK is chained with itself. This way, if the
> user want to check if the action is a for example an "open" and a "read"
> and not a "map" and a "read", then it can chain multiple FS_PICK
> programs with different triggers actions. The OR check performed by the
> kernel is not a limitation then, only a way to know if a call to an eBPF
> program is needed.
>
> The last type of program is FS_GET. This one is called when a process
> get a struct file or change its working directory. This is the only
> program type able (and allowed) to tag a file. This restriction is
> important to not being subject to resource exhaustion attacks (i.e.
> tagging every inode accessible to an attacker, which would allocate too
> much kernel memory).
>
> This design gives room for improvements to create a cache of eBPF
> context (input data, including maps if any), with the result of an eBPF
> program. This would help limit the number of call to an eBPF program the
> same way SELinux or other kernel components do to limit costly checks.
>
> The eBPF maps of progs are useful to call the same type of eBPF
> program. It does not fit with this use case because we may want multiple
> eBPF program according to the action requested on a kernel object (e.g.
> FS_GET). The other reason is because the eBPF program does not know what
> will be the next (type of) access check performed by the kernel.
>
> To say it another way, this chaining mechanism is a way to split a
> kernel object evaluation with multiple specialized programs, each of
> them being able to deal with data tied to their type. Using a monolithic
> eBPF program to check everything does not scale and does not fit with
> unprivileged use either.
>
> As a side note, the cookie value is only an ephemeral value to keep a
> state between multiple programs call. It can be used to create a state
> machine for an object evaluation.
>
> I don't see a way to do an efficient and programmatic path evaluation,
> with different access checks, with the current eBPF features. Please let
> me know if you know how to do it another way.
>
Andy, Alexei, Daniel, what do you think about this Landlock program
chaining and cookie?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
From: Andy Lutomirski @ 2018-04-08 21:06 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Andy Lutomirski, Alexei Starovoitov, Daniel Borkmann, LKML,
Alexei Starovoitov, Arnaldo Carvalho de Melo, Casey Schaufler,
David Drysdale, David S . Miller, Eric W . Biederman, Jann Horn,
Jonathan Corbet, Michael Kerrisk, Kees Cook, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Tejun Heo,
Thomas Graf, Tycho Andersen, Will Drewry, Kernel Hardening, Linux
In-Reply-To: <498f8193-c909-78b2-e4ca-c1dd05605255@digikod.net>
On Sun, Apr 8, 2018 at 6:13 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 02/27/2018 10:48 PM, Mickaël Salaün wrote:
>>
>> On 27/02/2018 17:39, Andy Lutomirski wrote:
>>> On Tue, Feb 27, 2018 at 5:32 AM, Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>> On Tue, Feb 27, 2018 at 05:20:55AM +0000, Andy Lutomirski wrote:
>>>>> On Tue, Feb 27, 2018 at 4:54 AM, Alexei Starovoitov
>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>> On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
>>>>>>> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>>> On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
>>>>>>>>> The seccomp(2) syscall can be used by a task to apply a Landlock program
>>>>>>>>> to itself. As a seccomp filter, a Landlock program is enforced for the
>>>>>>>>> current task and all its future children. A program is immutable and a
>>>>>>>>> task can only add new restricting programs to itself, forming a list of
>>>>>>>>> programss.
>>>>>>>>>
>>>>>>>>> A Landlock program is tied to a Landlock hook. If the action on a kernel
>>>>>>>>> object is allowed by the other Linux security mechanisms (e.g. DAC,
>>>>>>>>> capabilities, other LSM), then a Landlock hook related to this kind of
>>>>>>>>> object is triggered. The list of programs for this hook is then
>>>>>>>>> evaluated. Each program return a 32-bit value which can deny the action
>>>>>>>>> on a kernel object with a non-zero value. If every programs of the list
>>>>>>>>> return zero, then the action on the object is allowed.
>>>>>>>>>
>>>>>>>>> Multiple Landlock programs can be chained to share a 64-bits value for a
>>>>>>>>> call chain (e.g. evaluating multiple elements of a file path). This
>>>>>>>>> chaining is restricted when a process construct this chain by loading a
>>>>>>>>> program, but additional checks are performed when it requests to apply
>>>>>>>>> this chain of programs to itself. The restrictions ensure that it is
>>>>>>>>> not possible to call multiple programs in a way that would imply to
>>>>>>>>> handle multiple shared values (i.e. cookies) for one chain. For now,
>>>>>>>>> only a fs_pick program can be chained to the same type of program,
>>>>>>>>> because it may make sense if they have different triggers (cf. next
>>>>>>>>> commits). This restrictions still allows to reuse Landlock programs in
>>>>>>>>> a safe way (e.g. use the same loaded fs_walk program with multiple
>>>>>>>>> chains of fs_pick programs).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>>> +struct landlock_prog_set *landlock_prepend_prog(
>>>>>>>>> + struct landlock_prog_set *current_prog_set,
>>>>>>>>> + struct bpf_prog *prog)
>>>>>>>>> +{
>>>>>>>>> + struct landlock_prog_set *new_prog_set = current_prog_set;
>>>>>>>>> + unsigned long pages;
>>>>>>>>> + int err;
>>>>>>>>> + size_t i;
>>>>>>>>> + struct landlock_prog_set tmp_prog_set = {};
>>>>>>>>> +
>>>>>>>>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
>>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>>> +
>>>>>>>>> + /* validate memory size allocation */
>>>>>>>>> + pages = prog->pages;
>>>>>>>>> + if (current_prog_set) {
>>>>>>>>> + size_t i;
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
>>>>>>>>> + struct landlock_prog_list *walker_p;
>>>>>>>>> +
>>>>>>>>> + for (walker_p = current_prog_set->programs[i];
>>>>>>>>> + walker_p; walker_p = walker_p->prev)
>>>>>>>>> + pages += walker_p->prog->pages;
>>>>>>>>> + }
>>>>>>>>> + /* count a struct landlock_prog_set if we need to allocate one */
>>>>>>>>> + if (refcount_read(¤t_prog_set->usage) != 1)
>>>>>>>>> + pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
>>>>>>>>> + / PAGE_SIZE;
>>>>>>>>> + }
>>>>>>>>> + if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
>>>>>>>>> + return ERR_PTR(-E2BIG);
>>>>>>>>> +
>>>>>>>>> + /* ensure early that we can allocate enough memory for the new
>>>>>>>>> + * prog_lists */
>>>>>>>>> + err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
>>>>>>>>> + if (err)
>>>>>>>>> + return ERR_PTR(err);
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * Each task_struct points to an array of prog list pointers. These
>>>>>>>>> + * tables are duplicated when additions are made (which means each
>>>>>>>>> + * table needs to be refcounted for the processes using it). When a new
>>>>>>>>> + * table is created, all the refcounters on the prog_list are bumped (to
>>>>>>>>> + * track each table that references the prog). When a new prog is
>>>>>>>>> + * added, it's just prepended to the list for the new table to point
>>>>>>>>> + * at.
>>>>>>>>> + *
>>>>>>>>> + * Manage all the possible errors before this step to not uselessly
>>>>>>>>> + * duplicate current_prog_set and avoid a rollback.
>>>>>>>>> + */
>>>>>>>>> + if (!new_prog_set) {
>>>>>>>>> + /*
>>>>>>>>> + * If there is no Landlock program set used by the current task,
>>>>>>>>> + * then create a new one.
>>>>>>>>> + */
>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>> + } else if (refcount_read(¤t_prog_set->usage) > 1) {
>>>>>>>>> + /*
>>>>>>>>> + * If the current task is not the sole user of its Landlock
>>>>>>>>> + * program set, then duplicate them.
>>>>>>>>> + */
>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
>>>>>>>>> + new_prog_set->programs[i] =
>>>>>>>>> + READ_ONCE(current_prog_set->programs[i]);
>>>>>>>>> + if (new_prog_set->programs[i])
>>>>>>>>> + refcount_inc(&new_prog_set->programs[i]->usage);
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + /*
>>>>>>>>> + * Landlock program set from the current task will not be freed
>>>>>>>>> + * here because the usage is strictly greater than 1. It is
>>>>>>>>> + * only prevented to be freed by another task thanks to the
>>>>>>>>> + * caller of landlock_prepend_prog() which should be locked if
>>>>>>>>> + * needed.
>>>>>>>>> + */
>>>>>>>>> + landlock_put_prog_set(current_prog_set);
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + /* prepend tmp_prog_set to new_prog_set */
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
>>>>>>>>> + /* get the last new list */
>>>>>>>>> + struct landlock_prog_list *last_list =
>>>>>>>>> + tmp_prog_set.programs[i];
>>>>>>>>> +
>>>>>>>>> + if (last_list) {
>>>>>>>>> + while (last_list->prev)
>>>>>>>>> + last_list = last_list->prev;
>>>>>>>>> + /* no need to increment usage (pointer replacement) */
>>>>>>>>> + last_list->prev = new_prog_set->programs[i];
>>>>>>>>> + new_prog_set->programs[i] = tmp_prog_set.programs[i];
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> + new_prog_set->chain_last = tmp_prog_set.chain_last;
>>>>>>>>> + return new_prog_set;
>>>>>>>>> +
>>>>>>>>> +put_tmp_lists:
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
>>>>>>>>> + put_landlock_prog_list(tmp_prog_set.programs[i]);
>>>>>>>>> + return new_prog_set;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>> Nack on the chaining concept.
>>>>>>>> Please do not reinvent the wheel.
>>>>>>>> There is an existing mechanism for attaching/detaching/quering multiple
>>>>>>>> programs attached to cgroup and tracing hooks that are also
>>>>>>>> efficiently executed via BPF_PROG_RUN_ARRAY.
>>>>>>>> Please use that instead.
>>>>>>>>
>>>>>>>
>>>>>>> I don't see how that would help. Suppose you add a filter, then
>>>>>>> fork(), and then the child adds another filter. Do you want to
>>>>>>> duplicate the entire array? You certainly can't *modify* the array
>>>>>>> because you'll affect processes that shouldn't be affected.
>>>>>>>
>>>>>>> In contrast, doing this through seccomp like the earlier patches
>>>>>>> seemed just fine to me, and seccomp already had the right logic.
>>>>>>
>>>>>> it doesn't look to me that existing seccomp side of managing fork
>>>>>> situation can be reused. Here there is an attempt to add 'chaining'
>>>>>> concept which sort of an extension of existing seccomp style,
>>>>>> but somehow heavily done on bpf side and contradicts cgroup/tracing.
>>>>>>
>>>>>
>>>>> I don't see why the seccomp way can't be used. I agree with you that
>>>>> the seccomp *style* shouldn't be used in bpf code like this, but I
>>>>> think that Landlock programs can and should just live in the existing
>>>>> seccomp chain. If the existing seccomp code needs some modification
>>>>> to make this work, then so be it.
>>>>
>>>> +1
>>>> if that was the case...
>>>> but that's not my reading of the patch set.
>>>
>>> An earlier version of the patch set used the seccomp filter chain.
>>> Mickaël, what exactly was wrong with that approach other than that the
>>> seccomp() syscall was awkward for you to use? You could add a
>>> seccomp_add_landlock_rule() syscall if you needed to.
>>
>> Nothing was wrong about about that, this part did not changed (see my
>> next comment).
>>
>>>
>>> As a side comment, why is this an LSM at all, let alone a non-stacking
>>> LSM? It would make a lot more sense to me to make Landlock depend on
>>> having LSMs configured in but to call the landlock hooks directly from
>>> the security_xyz() hooks.
>>
>> See Casey's answer and his patch series: https://lwn.net/Articles/741963/
>>
>>>
>>>>
>>>>> In other words, the kernel already has two kinds of chaining:
>>>>> seccomp's and bpf's. bpf's doesn't work right for this type of usage
>>>>> across fork(), whereas seccomp's already handles that case correctly.
>>>>> (In contrast, seccomp's is totally wrong for cgroup-attached filters.)
>>>>> So IMO Landlock should use the seccomp core code and call into bpf
>>>>> for the actual filtering.
>>>>
>>>> +1
>>>> in cgroup we had to invent this new BPF_PROG_RUN_ARRAY mechanism,
>>>> since cgroup hierarchy can be complicated with bpf progs attached
>>>> at different levels with different override/multiprog properties,
>>>> so walking link list and checking all flags at run-time would have
>>>> been too slow. That's why we added compute_effective_progs().
>>>
>>> If we start adding override flags to Landlock, I think we're doing it
>>> wrong. With cgroup bpf programs, the whole mess is set up by the
>>> administrator. With seccomp, and with Landlock if done correctly, it
>>> *won't* be set up by the administrator, so the chance that everyone
>>> gets all the flags right is about zero. All attached filters should
>>> run unconditionally.
>>
>>
>> There is a misunderstanding about this chaining mechanism. This should
>> not be confused with the list of seccomp filters nor the cgroup
>> hierarchies. Landlock programs can be stacked the same way seccomp's
>> filters can (cf. struct landlock_prog_set, the "chain_last" field is an
>> optimization which is not used for this struct handling). This stackable
>> property did not changed from the previous patch series. The chaining
>> mechanism is for another use case, which does not make sense for seccomp
>> filters nor other eBPF program types, at least for now, from what I can
>> tell.
>>
>> You may want to get a look at my talk at FOSDEM
>> (https://landlock.io/talks/2018-02-04_landlock-fosdem.pdf), especially
>> slides 11 and 12.
>>
>> Let me explain my reasoning about this program chaining thing.
>>
>> To check if an action on a file is allowed, we first need to identify
>> this file and match it to the security policy. In a previous
>> (non-public) patch series, I tried to use one type of eBPF program to
>> check every kind of access to a file. To be able to identify a file, I
>> relied on an eBPF map, similar to the current inode map. This map store
>> a set of references to file descriptors. I then created a function
>> bpf_is_file_beneath() to check if the requested file was beneath a file
>> in the map. This way, no chaining, only one eBPF program type to check
>> an access to a file... but some issues then emerged. First, this design
>> create a side-channel which help an attacker using such a program to
>> infer some information not normally available, for example to get a hint
>> on where a file descriptor (received from a UNIX socket) come from.
>> Another issue is that this type of program would be called for each
>> component of a path. Indeed, when the kernel check if an access to a
>> file is allowed, it walk through all of the directories in its path
>> (checking if the current process is allowed to execute them). That first
>> attempt led me to rethink the way we could filter an access to a file
>> *path*.
>>
>> To minimize the number of called to an eBPF program dedicated to
>> validate an access to a file path, I decided to create three subtype of
>> eBPF programs. The FS_WALK type is called when walking through every
>> directory of a file path (except the last one if it is the target). We
>> can then restrict this type of program to the minimum set of functions
>> it is allowed to call and the minimum set of data available from its
>> context. The first implicit chaining is for this type of program. To be
>> able to evaluate a path while being called for all its components, this
>> program need to store a state (to remember what was the parent directory
>> of this path). There is no "previous" field in the subtype for this
>> program because it is chained with itself, for each directories. This
>> enable to create a FS_WALK program to evaluate a file hierarchy, thank
>> to the inode map which can be used to check if a directory of this
>> hierarchy is part of an allowed (or denied) list of directories. This
>> design enables to express a file hierarchy in a programmatic way,
>> without requiring an eBPF helper to do the job (unlike my first experiment).
>>
>> The explicit chaining is used to tied a path evaluation (with a FS_WALK
>> program) to an access to the actual file being requested (the last
>> component of a file path), with a FS_PICK program. It is only at this
>> time that the kernel check for the requested action (e.g. read, write,
>> chdir, append...). To be able to filter such access request we can have
>> one call to the same program for every action and let this program check
>> for which action it was called. However, this design does not allow the
>> kernel to know if the current action is indeed handled by this program.
>> Hence, it is not possible to implement a cache mechanism to only call
>> this program if it knows how to handle this action.
>>
>> The approach I took for this FS_PICK type of program is to add to its
>> subtype which action it can handle (with the "triggers" bitfield, seen
>> as ORed actions). This way, the kernel knows if a call to a FS_PICK
>> program is necessary. If the user wants to enforce a different security
>> policy according to the action requested on a file, then it needs
>> multiple FS_PICK programs. However, to reduce the number of such
>> programs, this patch series allow a FS_PICK program to be chained with
>> another, the same way a FS_WALK is chained with itself. This way, if the
>> user want to check if the action is a for example an "open" and a "read"
>> and not a "map" and a "read", then it can chain multiple FS_PICK
>> programs with different triggers actions. The OR check performed by the
>> kernel is not a limitation then, only a way to know if a call to an eBPF
>> program is needed.
>>
>> The last type of program is FS_GET. This one is called when a process
>> get a struct file or change its working directory. This is the only
>> program type able (and allowed) to tag a file. This restriction is
>> important to not being subject to resource exhaustion attacks (i.e.
>> tagging every inode accessible to an attacker, which would allocate too
>> much kernel memory).
>>
>> This design gives room for improvements to create a cache of eBPF
>> context (input data, including maps if any), with the result of an eBPF
>> program. This would help limit the number of call to an eBPF program the
>> same way SELinux or other kernel components do to limit costly checks.
>>
>> The eBPF maps of progs are useful to call the same type of eBPF
>> program. It does not fit with this use case because we may want multiple
>> eBPF program according to the action requested on a kernel object (e.g.
>> FS_GET). The other reason is because the eBPF program does not know what
>> will be the next (type of) access check performed by the kernel.
>>
>> To say it another way, this chaining mechanism is a way to split a
>> kernel object evaluation with multiple specialized programs, each of
>> them being able to deal with data tied to their type. Using a monolithic
>> eBPF program to check everything does not scale and does not fit with
>> unprivileged use either.
>>
>> As a side note, the cookie value is only an ephemeral value to keep a
>> state between multiple programs call. It can be used to create a state
>> machine for an object evaluation.
>>
>> I don't see a way to do an efficient and programmatic path evaluation,
>> with different access checks, with the current eBPF features. Please let
>> me know if you know how to do it another way.
>>
>
> Andy, Alexei, Daniel, what do you think about this Landlock program
> chaining and cookie?
>
Can you give a small pseudocode real world example that acutally needs
chaining? The mechanism is quite complicated and I'd like to
understand how it'll be used.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Pavel Machek @ 2018-04-08 22:00 UTC (permalink / raw)
To: Theodore Y. Ts'o, Matthew Garrett, Linus Torvalds, luto,
David Howells, Ard Biesheuvel, jmorris, Alan Cox,
Greg Kroah-Hartman, Linux Kernel Mailing List, jforbes, linux-man,
jlee, LSM List, linux-api, Kees Cook, linux-efi
In-Reply-To: <20180404125743.GB16242@thunk.org>
[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]
Hi!
> > What I'm afraid of is this turning into a "security" feature that ends up
> > being circumvented in most scenarios where it's currently deployed - eg,
> > module signatures are mostly worthless in the non-lockdown case because you
> > can just grab the sig_enforce symbol address and then kexec a preamble that
> > flips it back to N regardless of the kernel config.
>
> Whoa. Why doesn't lockdown prevent kexec? Put another away, why
> isn't this a problem for people who are fearful that Linux could be
> used as part of a Windows boot virus in a Secure UEFI context?
>
> If lockdown simply included a requirement for a signed kernel for
> kexec --- and if kernel signing aren't available, to simply not alow
> kexec, wouldn't that take care of this case?
>
> This wouldn't even be all that much of a burden for non-distro users
> with lockdown enabled, since in my experience outside of enterprise
> and data center use cases, kexec isn't used --- and in fact, very
> often kexec doesn't even work outside of a very carefully selected and
> bug-fixed set of device drivers. (It often doesn't work in non-distro
> kernels because very few upstream developers really care about kexec.)
I do have Motorola Droid 4 here (cellphone). It uses safestrap.. and
than it turn kexec's a lot (so that you can select Android vs. Jolla
vs. ... during boot).
So yes, kexec shows even in unexpected places.
And BTW.. the cellphone thingie is a situation where manufacturer
works against it users. Motorola does _not_ want me to run my own
kernels here.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
From: Mickaël Salaün @ 2018-04-08 22:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Alexei Starovoitov, Daniel Borkmann, LKML, Alexei Starovoitov,
Arnaldo Carvalho de Melo, Casey Schaufler, David Drysdale,
David S . Miller, Eric W . Biederman, Jann Horn, Jonathan Corbet,
Michael Kerrisk, Kees Cook, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Tejun Heo, Thomas Graf,
Tycho Andersen, Will Drewry
In-Reply-To: <CALCETrViaXEx1iQ6q8bEEWSLchj=FH6LjcRY6+hjMx8A+rtgDQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 22278 bytes --]
On 04/08/2018 11:06 PM, Andy Lutomirski wrote:
> On Sun, Apr 8, 2018 at 6:13 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 02/27/2018 10:48 PM, Mickaël Salaün wrote:
>>>
>>> On 27/02/2018 17:39, Andy Lutomirski wrote:
>>>> On Tue, Feb 27, 2018 at 5:32 AM, Alexei Starovoitov
>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>> On Tue, Feb 27, 2018 at 05:20:55AM +0000, Andy Lutomirski wrote:
>>>>>> On Tue, Feb 27, 2018 at 4:54 AM, Alexei Starovoitov
>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>> On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
>>>>>>>> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
>>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>> On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
>>>>>>>>>> The seccomp(2) syscall can be used by a task to apply a Landlock program
>>>>>>>>>> to itself. As a seccomp filter, a Landlock program is enforced for the
>>>>>>>>>> current task and all its future children. A program is immutable and a
>>>>>>>>>> task can only add new restricting programs to itself, forming a list of
>>>>>>>>>> programss.
>>>>>>>>>>
>>>>>>>>>> A Landlock program is tied to a Landlock hook. If the action on a kernel
>>>>>>>>>> object is allowed by the other Linux security mechanisms (e.g. DAC,
>>>>>>>>>> capabilities, other LSM), then a Landlock hook related to this kind of
>>>>>>>>>> object is triggered. The list of programs for this hook is then
>>>>>>>>>> evaluated. Each program return a 32-bit value which can deny the action
>>>>>>>>>> on a kernel object with a non-zero value. If every programs of the list
>>>>>>>>>> return zero, then the action on the object is allowed.
>>>>>>>>>>
>>>>>>>>>> Multiple Landlock programs can be chained to share a 64-bits value for a
>>>>>>>>>> call chain (e.g. evaluating multiple elements of a file path). This
>>>>>>>>>> chaining is restricted when a process construct this chain by loading a
>>>>>>>>>> program, but additional checks are performed when it requests to apply
>>>>>>>>>> this chain of programs to itself. The restrictions ensure that it is
>>>>>>>>>> not possible to call multiple programs in a way that would imply to
>>>>>>>>>> handle multiple shared values (i.e. cookies) for one chain. For now,
>>>>>>>>>> only a fs_pick program can be chained to the same type of program,
>>>>>>>>>> because it may make sense if they have different triggers (cf. next
>>>>>>>>>> commits). This restrictions still allows to reuse Landlock programs in
>>>>>>>>>> a safe way (e.g. use the same loaded fs_walk program with multiple
>>>>>>>>>> chains of fs_pick programs).
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>>> +struct landlock_prog_set *landlock_prepend_prog(
>>>>>>>>>> + struct landlock_prog_set *current_prog_set,
>>>>>>>>>> + struct bpf_prog *prog)
>>>>>>>>>> +{
>>>>>>>>>> + struct landlock_prog_set *new_prog_set = current_prog_set;
>>>>>>>>>> + unsigned long pages;
>>>>>>>>>> + int err;
>>>>>>>>>> + size_t i;
>>>>>>>>>> + struct landlock_prog_set tmp_prog_set = {};
>>>>>>>>>> +
>>>>>>>>>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
>>>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>>>> +
>>>>>>>>>> + /* validate memory size allocation */
>>>>>>>>>> + pages = prog->pages;
>>>>>>>>>> + if (current_prog_set) {
>>>>>>>>>> + size_t i;
>>>>>>>>>> +
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
>>>>>>>>>> + struct landlock_prog_list *walker_p;
>>>>>>>>>> +
>>>>>>>>>> + for (walker_p = current_prog_set->programs[i];
>>>>>>>>>> + walker_p; walker_p = walker_p->prev)
>>>>>>>>>> + pages += walker_p->prog->pages;
>>>>>>>>>> + }
>>>>>>>>>> + /* count a struct landlock_prog_set if we need to allocate one */
>>>>>>>>>> + if (refcount_read(¤t_prog_set->usage) != 1)
>>>>>>>>>> + pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
>>>>>>>>>> + / PAGE_SIZE;
>>>>>>>>>> + }
>>>>>>>>>> + if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
>>>>>>>>>> + return ERR_PTR(-E2BIG);
>>>>>>>>>> +
>>>>>>>>>> + /* ensure early that we can allocate enough memory for the new
>>>>>>>>>> + * prog_lists */
>>>>>>>>>> + err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
>>>>>>>>>> + if (err)
>>>>>>>>>> + return ERR_PTR(err);
>>>>>>>>>> +
>>>>>>>>>> + /*
>>>>>>>>>> + * Each task_struct points to an array of prog list pointers. These
>>>>>>>>>> + * tables are duplicated when additions are made (which means each
>>>>>>>>>> + * table needs to be refcounted for the processes using it). When a new
>>>>>>>>>> + * table is created, all the refcounters on the prog_list are bumped (to
>>>>>>>>>> + * track each table that references the prog). When a new prog is
>>>>>>>>>> + * added, it's just prepended to the list for the new table to point
>>>>>>>>>> + * at.
>>>>>>>>>> + *
>>>>>>>>>> + * Manage all the possible errors before this step to not uselessly
>>>>>>>>>> + * duplicate current_prog_set and avoid a rollback.
>>>>>>>>>> + */
>>>>>>>>>> + if (!new_prog_set) {
>>>>>>>>>> + /*
>>>>>>>>>> + * If there is no Landlock program set used by the current task,
>>>>>>>>>> + * then create a new one.
>>>>>>>>>> + */
>>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>>> + } else if (refcount_read(¤t_prog_set->usage) > 1) {
>>>>>>>>>> + /*
>>>>>>>>>> + * If the current task is not the sole user of its Landlock
>>>>>>>>>> + * program set, then duplicate them.
>>>>>>>>>> + */
>>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
>>>>>>>>>> + new_prog_set->programs[i] =
>>>>>>>>>> + READ_ONCE(current_prog_set->programs[i]);
>>>>>>>>>> + if (new_prog_set->programs[i])
>>>>>>>>>> + refcount_inc(&new_prog_set->programs[i]->usage);
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + /*
>>>>>>>>>> + * Landlock program set from the current task will not be freed
>>>>>>>>>> + * here because the usage is strictly greater than 1. It is
>>>>>>>>>> + * only prevented to be freed by another task thanks to the
>>>>>>>>>> + * caller of landlock_prepend_prog() which should be locked if
>>>>>>>>>> + * needed.
>>>>>>>>>> + */
>>>>>>>>>> + landlock_put_prog_set(current_prog_set);
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + /* prepend tmp_prog_set to new_prog_set */
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
>>>>>>>>>> + /* get the last new list */
>>>>>>>>>> + struct landlock_prog_list *last_list =
>>>>>>>>>> + tmp_prog_set.programs[i];
>>>>>>>>>> +
>>>>>>>>>> + if (last_list) {
>>>>>>>>>> + while (last_list->prev)
>>>>>>>>>> + last_list = last_list->prev;
>>>>>>>>>> + /* no need to increment usage (pointer replacement) */
>>>>>>>>>> + last_list->prev = new_prog_set->programs[i];
>>>>>>>>>> + new_prog_set->programs[i] = tmp_prog_set.programs[i];
>>>>>>>>>> + }
>>>>>>>>>> + }
>>>>>>>>>> + new_prog_set->chain_last = tmp_prog_set.chain_last;
>>>>>>>>>> + return new_prog_set;
>>>>>>>>>> +
>>>>>>>>>> +put_tmp_lists:
>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
>>>>>>>>>> + put_landlock_prog_list(tmp_prog_set.programs[i]);
>>>>>>>>>> + return new_prog_set;
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> Nack on the chaining concept.
>>>>>>>>> Please do not reinvent the wheel.
>>>>>>>>> There is an existing mechanism for attaching/detaching/quering multiple
>>>>>>>>> programs attached to cgroup and tracing hooks that are also
>>>>>>>>> efficiently executed via BPF_PROG_RUN_ARRAY.
>>>>>>>>> Please use that instead.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I don't see how that would help. Suppose you add a filter, then
>>>>>>>> fork(), and then the child adds another filter. Do you want to
>>>>>>>> duplicate the entire array? You certainly can't *modify* the array
>>>>>>>> because you'll affect processes that shouldn't be affected.
>>>>>>>>
>>>>>>>> In contrast, doing this through seccomp like the earlier patches
>>>>>>>> seemed just fine to me, and seccomp already had the right logic.
>>>>>>>
>>>>>>> it doesn't look to me that existing seccomp side of managing fork
>>>>>>> situation can be reused. Here there is an attempt to add 'chaining'
>>>>>>> concept which sort of an extension of existing seccomp style,
>>>>>>> but somehow heavily done on bpf side and contradicts cgroup/tracing.
>>>>>>>
>>>>>>
>>>>>> I don't see why the seccomp way can't be used. I agree with you that
>>>>>> the seccomp *style* shouldn't be used in bpf code like this, but I
>>>>>> think that Landlock programs can and should just live in the existing
>>>>>> seccomp chain. If the existing seccomp code needs some modification
>>>>>> to make this work, then so be it.
>>>>>
>>>>> +1
>>>>> if that was the case...
>>>>> but that's not my reading of the patch set.
>>>>
>>>> An earlier version of the patch set used the seccomp filter chain.
>>>> Mickaël, what exactly was wrong with that approach other than that the
>>>> seccomp() syscall was awkward for you to use? You could add a
>>>> seccomp_add_landlock_rule() syscall if you needed to.
>>>
>>> Nothing was wrong about about that, this part did not changed (see my
>>> next comment).
>>>
>>>>
>>>> As a side comment, why is this an LSM at all, let alone a non-stacking
>>>> LSM? It would make a lot more sense to me to make Landlock depend on
>>>> having LSMs configured in but to call the landlock hooks directly from
>>>> the security_xyz() hooks.
>>>
>>> See Casey's answer and his patch series: https://lwn.net/Articles/741963/
>>>
>>>>
>>>>>
>>>>>> In other words, the kernel already has two kinds of chaining:
>>>>>> seccomp's and bpf's. bpf's doesn't work right for this type of usage
>>>>>> across fork(), whereas seccomp's already handles that case correctly.
>>>>>> (In contrast, seccomp's is totally wrong for cgroup-attached filters.)
>>>>>> So IMO Landlock should use the seccomp core code and call into bpf
>>>>>> for the actual filtering.
>>>>>
>>>>> +1
>>>>> in cgroup we had to invent this new BPF_PROG_RUN_ARRAY mechanism,
>>>>> since cgroup hierarchy can be complicated with bpf progs attached
>>>>> at different levels with different override/multiprog properties,
>>>>> so walking link list and checking all flags at run-time would have
>>>>> been too slow. That's why we added compute_effective_progs().
>>>>
>>>> If we start adding override flags to Landlock, I think we're doing it
>>>> wrong. With cgroup bpf programs, the whole mess is set up by the
>>>> administrator. With seccomp, and with Landlock if done correctly, it
>>>> *won't* be set up by the administrator, so the chance that everyone
>>>> gets all the flags right is about zero. All attached filters should
>>>> run unconditionally.
>>>
>>>
>>> There is a misunderstanding about this chaining mechanism. This should
>>> not be confused with the list of seccomp filters nor the cgroup
>>> hierarchies. Landlock programs can be stacked the same way seccomp's
>>> filters can (cf. struct landlock_prog_set, the "chain_last" field is an
>>> optimization which is not used for this struct handling). This stackable
>>> property did not changed from the previous patch series. The chaining
>>> mechanism is for another use case, which does not make sense for seccomp
>>> filters nor other eBPF program types, at least for now, from what I can
>>> tell.
>>>
>>> You may want to get a look at my talk at FOSDEM
>>> (https://landlock.io/talks/2018-02-04_landlock-fosdem.pdf), especially
>>> slides 11 and 12.
>>>
>>> Let me explain my reasoning about this program chaining thing.
>>>
>>> To check if an action on a file is allowed, we first need to identify
>>> this file and match it to the security policy. In a previous
>>> (non-public) patch series, I tried to use one type of eBPF program to
>>> check every kind of access to a file. To be able to identify a file, I
>>> relied on an eBPF map, similar to the current inode map. This map store
>>> a set of references to file descriptors. I then created a function
>>> bpf_is_file_beneath() to check if the requested file was beneath a file
>>> in the map. This way, no chaining, only one eBPF program type to check
>>> an access to a file... but some issues then emerged. First, this design
>>> create a side-channel which help an attacker using such a program to
>>> infer some information not normally available, for example to get a hint
>>> on where a file descriptor (received from a UNIX socket) come from.
>>> Another issue is that this type of program would be called for each
>>> component of a path. Indeed, when the kernel check if an access to a
>>> file is allowed, it walk through all of the directories in its path
>>> (checking if the current process is allowed to execute them). That first
>>> attempt led me to rethink the way we could filter an access to a file
>>> *path*.
>>>
>>> To minimize the number of called to an eBPF program dedicated to
>>> validate an access to a file path, I decided to create three subtype of
>>> eBPF programs. The FS_WALK type is called when walking through every
>>> directory of a file path (except the last one if it is the target). We
>>> can then restrict this type of program to the minimum set of functions
>>> it is allowed to call and the minimum set of data available from its
>>> context. The first implicit chaining is for this type of program. To be
>>> able to evaluate a path while being called for all its components, this
>>> program need to store a state (to remember what was the parent directory
>>> of this path). There is no "previous" field in the subtype for this
>>> program because it is chained with itself, for each directories. This
>>> enable to create a FS_WALK program to evaluate a file hierarchy, thank
>>> to the inode map which can be used to check if a directory of this
>>> hierarchy is part of an allowed (or denied) list of directories. This
>>> design enables to express a file hierarchy in a programmatic way,
>>> without requiring an eBPF helper to do the job (unlike my first experiment).
>>>
>>> The explicit chaining is used to tied a path evaluation (with a FS_WALK
>>> program) to an access to the actual file being requested (the last
>>> component of a file path), with a FS_PICK program. It is only at this
>>> time that the kernel check for the requested action (e.g. read, write,
>>> chdir, append...). To be able to filter such access request we can have
>>> one call to the same program for every action and let this program check
>>> for which action it was called. However, this design does not allow the
>>> kernel to know if the current action is indeed handled by this program.
>>> Hence, it is not possible to implement a cache mechanism to only call
>>> this program if it knows how to handle this action.
>>>
>>> The approach I took for this FS_PICK type of program is to add to its
>>> subtype which action it can handle (with the "triggers" bitfield, seen
>>> as ORed actions). This way, the kernel knows if a call to a FS_PICK
>>> program is necessary. If the user wants to enforce a different security
>>> policy according to the action requested on a file, then it needs
>>> multiple FS_PICK programs. However, to reduce the number of such
>>> programs, this patch series allow a FS_PICK program to be chained with
>>> another, the same way a FS_WALK is chained with itself. This way, if the
>>> user want to check if the action is a for example an "open" and a "read"
>>> and not a "map" and a "read", then it can chain multiple FS_PICK
>>> programs with different triggers actions. The OR check performed by the
>>> kernel is not a limitation then, only a way to know if a call to an eBPF
>>> program is needed.
>>>
>>> The last type of program is FS_GET. This one is called when a process
>>> get a struct file or change its working directory. This is the only
>>> program type able (and allowed) to tag a file. This restriction is
>>> important to not being subject to resource exhaustion attacks (i.e.
>>> tagging every inode accessible to an attacker, which would allocate too
>>> much kernel memory).
>>>
>>> This design gives room for improvements to create a cache of eBPF
>>> context (input data, including maps if any), with the result of an eBPF
>>> program. This would help limit the number of call to an eBPF program the
>>> same way SELinux or other kernel components do to limit costly checks.
>>>
>>> The eBPF maps of progs are useful to call the same type of eBPF
>>> program. It does not fit with this use case because we may want multiple
>>> eBPF program according to the action requested on a kernel object (e.g.
>>> FS_GET). The other reason is because the eBPF program does not know what
>>> will be the next (type of) access check performed by the kernel.
>>>
>>> To say it another way, this chaining mechanism is a way to split a
>>> kernel object evaluation with multiple specialized programs, each of
>>> them being able to deal with data tied to their type. Using a monolithic
>>> eBPF program to check everything does not scale and does not fit with
>>> unprivileged use either.
>>>
>>> As a side note, the cookie value is only an ephemeral value to keep a
>>> state between multiple programs call. It can be used to create a state
>>> machine for an object evaluation.
>>>
>>> I don't see a way to do an efficient and programmatic path evaluation,
>>> with different access checks, with the current eBPF features. Please let
>>> me know if you know how to do it another way.
>>>
>>
>> Andy, Alexei, Daniel, what do you think about this Landlock program
>> chaining and cookie?
>>
>
> Can you give a small pseudocode real world example that acutally needs
> chaining? The mechanism is quite complicated and I'd like to
> understand how it'll be used.
>
Here is the interesting part from the example (patch 09/11):
+SEC("maps")
+struct bpf_map_def inode_map = {
+ .type = BPF_MAP_TYPE_INODE,
+ .key_size = sizeof(u32),
+ .value_size = sizeof(u64),
+ .max_entries = 20,
+};
+
+SEC("subtype/landlock1")
+static union bpf_prog_subtype _subtype1 = {
+ .landlock_hook = {
+ .type = LANDLOCK_HOOK_FS_WALK,
+ }
+};
+
+static __always_inline __u64 update_cookie(__u64 cookie, __u8 lookup,
+ void *inode, void *chain, bool freeze)
+{
+ __u64 map_allow = 0;
+
+ if (cookie == 0) {
+ cookie = bpf_inode_get_tag(inode, chain);
+ if (cookie)
+ return cookie;
+ /* only look for the first match in the map, ignore nested
+ * paths in this example */
+ map_allow = bpf_inode_map_lookup(&inode_map, inode);
+ if (map_allow)
+ cookie = 1 | map_allow;
+ } else {
+ if (cookie & COOKIE_VALUE_FREEZED)
+ return cookie;
+ map_allow = cookie & _MAP_MARK_MASK;
+ cookie &= ~_MAP_MARK_MASK;
+ switch (lookup) {
+ case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOTDOT:
+ cookie--;
+ break;
+ case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOT:
+ break;
+ default:
+ /* ignore _MAP_MARK_MASK overflow in this example */
+ cookie++;
+ break;
+ }
+ if (cookie >= 1)
+ cookie |= map_allow;
+ }
+ /* do not modify the cookie for each fs_pick */
+ if (freeze && cookie)
+ cookie |= COOKIE_VALUE_FREEZED;
+ return cookie;
+}
+
+SEC("landlock1")
+int fs_walk(struct landlock_ctx_fs_walk *ctx)
+{
+ ctx->cookie = update_cookie(ctx->cookie, ctx->inode_lookup,
+ (void *)ctx->inode, (void *)ctx->chain, false);
+ return LANDLOCK_RET_ALLOW;
+}
The program "landlock1" is called for every directory execution (except
the last one if it is the leaf of a path). This enables to identify a
file hierarchy with only a (one dimension) list of file descriptors
(i.e. inode_map).
Underneath, the Landlock LSM part looks if there is an associated path
walk (nameidata) with each inode access request. If there is one, then
the cookie associated with the path walk (if any) is made available
through the eBPF program context. This enables to develop a state
machine with an eBPF program to "evaluate" a file path (without string
parsing).
The goal with this chaining mechanism is to be able to express a complex
kernel object like a file, with multiple run of one or more eBPF
programs, as a multilayer evaluation. This semantic may only make sense
for the user/developer and his security policy. We must keep in mind
that this object identification should be available to unprivileged
processes. This means that we must be very careful to what kind of
information are available to an eBPF program because this can then leak
to a process (e.g. through a map). With this mechanism, only information
already available to user space is available to the eBPF program.
In this example, the complexity of the path evaluation is in the eBPF
program. We can then keep the kernel code more simple and generic. This
enables more flexibility for a security policy definition.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Alexei Starovoitov @ 2018-04-09 3:40 UTC (permalink / raw)
To: joeyli
Cc: Andy Lutomirski, David Howells, Ard Biesheuvel, James Morris,
One Thousand Gnomes, Linus Torvalds, Matthew Garrett, Greg KH,
LKML, Justin Forbes, linux-man, LSM List, Linux API, Kees Cook,
linux-efi, Daniel Borkmann
In-Reply-To: <20180408080742.GE7362@linux-l9pv.suse>
On Sun, Apr 08, 2018 at 04:07:42PM +0800, joeyli wrote:
>
> > If the only thing that folks are paranoid about is reading
> > arbitrary kernel memory with bpf_probe_read() helper
> > then preferred patch would be to disable it during verification
> > when in lockdown mode
>
> Sorry for I didn't fully understand your idea...
> Do you mean that using bpf verifier to filter out bpf program that
> uses bpf_probe_read()?
Take a look bpf_get_trace_printk_proto().
Similarly we can add bpf_get_probe_read_proto() that
will return NULL if lockdown is on.
Then programs with bpf_probe_read() will be rejected by the verifier.
^ permalink raw reply
* Re: [GIT PULL] Kernel lockdown for secure boot
From: Daniel Borkmann @ 2018-04-09 8:14 UTC (permalink / raw)
To: Alexei Starovoitov, joeyli
Cc: Andy Lutomirski, David Howells, Ard Biesheuvel, James Morris,
One Thousand Gnomes, Linus Torvalds, Matthew Garrett, Greg KH,
LKML, Justin Forbes, linux-man, LSM List, Linux API, Kees Cook,
linux-efi
In-Reply-To: <20180409034008.dyte7k5kgkbjh5is@ast-mbp.dhcp.thefacebook.com>
On 04/09/2018 05:40 AM, Alexei Starovoitov wrote:
> On Sun, Apr 08, 2018 at 04:07:42PM +0800, joeyli wrote:
[...]
>>> If the only thing that folks are paranoid about is reading
>>> arbitrary kernel memory with bpf_probe_read() helper
>>> then preferred patch would be to disable it during verification
>>> when in lockdown mode
>>
>> Sorry for I didn't fully understand your idea...
>> Do you mean that using bpf verifier to filter out bpf program that
>> uses bpf_probe_read()?
>
> Take a look bpf_get_trace_printk_proto().
> Similarly we can add bpf_get_probe_read_proto() that
> will return NULL if lockdown is on.
> Then programs with bpf_probe_read() will be rejected by the verifier.
Fully agree with the above. For the two helpers, something like the below
would be sufficient to reject progs at verification time.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d88e96d..51a6c2e 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -117,6 +117,11 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
.arg3_type = ARG_ANYTHING,
};
+static const struct bpf_func_proto *bpf_get_probe_read_proto(void)
+{
+ return kernel_is_locked_down("BPF") ? NULL : &bpf_probe_read_proto;
+}
+
BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
u32, size)
{
@@ -282,6 +287,9 @@ static const struct bpf_func_proto bpf_trace_printk_proto = {
const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
{
+ if (kernel_is_locked_down("BPF"))
+ return NULL;
+
/*
* this program might be calling bpf_trace_printk,
* so allocate per-cpu printk buffers
@@ -535,7 +543,7 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_map_delete_elem:
return &bpf_map_delete_elem_proto;
case BPF_FUNC_probe_read:
- return &bpf_probe_read_proto;
+ return bpf_get_probe_read_proto();
case BPF_FUNC_ktime_get_ns:
return &bpf_ktime_get_ns_proto;
case BPF_FUNC_tail_call:
^ permalink raw reply related
* Re: [GIT PULL] Kernel lockdown for secure boot
From: joeyli @ 2018-04-09 13:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, David Howells, Ard Biesheuvel, James Morris,
One Thousand Gnomes, Linus Torvalds, Matthew Garrett, Greg KH,
LKML, Justin Forbes, linux-man, LSM List, Linux API, Kees Cook,
linux-efi, Daniel Borkmann
In-Reply-To: <20180409034008.dyte7k5kgkbjh5is@ast-mbp.dhcp.thefacebook.com>
On Sun, Apr 08, 2018 at 08:40:10PM -0700, Alexei Starovoitov wrote:
> On Sun, Apr 08, 2018 at 04:07:42PM +0800, joeyli wrote:
> >
> > > If the only thing that folks are paranoid about is reading
> > > arbitrary kernel memory with bpf_probe_read() helper
> > > then preferred patch would be to disable it during verification
> > > when in lockdown mode
> >
> > Sorry for I didn't fully understand your idea...
> > Do you mean that using bpf verifier to filter out bpf program that
> > uses bpf_probe_read()?
>
> Take a look bpf_get_trace_printk_proto().
> Similarly we can add bpf_get_probe_read_proto() that
> will return NULL if lockdown is on.
> Then programs with bpf_probe_read() will be rejected by the verifier.
>
OK, I saw check_helper_call(). Thank you for point it out.
it's good idea!
Joey Lee
^ permalink raw reply
* Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
From: Alexei Starovoitov @ 2018-04-10 4:48 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Andy Lutomirski, Daniel Borkmann, LKML, Alexei Starovoitov,
Arnaldo Carvalho de Melo, Casey Schaufler, David Drysdale,
David S . Miller, Eric W . Biederman, Jann Horn, Jonathan Corbet,
Michael Kerrisk, Kees Cook, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Tejun Heo, Thomas Graf,
Tycho Andersen, Will Drewry
In-Reply-To: <c6621359-e8f8-dc14-d449-f8e5d7149a97@digikod.net>
On Mon, Apr 09, 2018 at 12:01:59AM +0200, Mickaël Salaün wrote:
>
> On 04/08/2018 11:06 PM, Andy Lutomirski wrote:
> > On Sun, Apr 8, 2018 at 6:13 AM, Mickaël Salaün <mic@digikod.net> wrote:
> >>
> >> On 02/27/2018 10:48 PM, Mickaël Salaün wrote:
> >>>
> >>> On 27/02/2018 17:39, Andy Lutomirski wrote:
> >>>> On Tue, Feb 27, 2018 at 5:32 AM, Alexei Starovoitov
> >>>> <alexei.starovoitov@gmail.com> wrote:
> >>>>> On Tue, Feb 27, 2018 at 05:20:55AM +0000, Andy Lutomirski wrote:
> >>>>>> On Tue, Feb 27, 2018 at 4:54 AM, Alexei Starovoitov
> >>>>>> <alexei.starovoitov@gmail.com> wrote:
> >>>>>>> On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
> >>>>>>>> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
> >>>>>>>> <alexei.starovoitov@gmail.com> wrote:
> >>>>>>>>> On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
> >>>>>>>>>> The seccomp(2) syscall can be used by a task to apply a Landlock program
> >>>>>>>>>> to itself. As a seccomp filter, a Landlock program is enforced for the
> >>>>>>>>>> current task and all its future children. A program is immutable and a
> >>>>>>>>>> task can only add new restricting programs to itself, forming a list of
> >>>>>>>>>> programss.
> >>>>>>>>>>
> >>>>>>>>>> A Landlock program is tied to a Landlock hook. If the action on a kernel
> >>>>>>>>>> object is allowed by the other Linux security mechanisms (e.g. DAC,
> >>>>>>>>>> capabilities, other LSM), then a Landlock hook related to this kind of
> >>>>>>>>>> object is triggered. The list of programs for this hook is then
> >>>>>>>>>> evaluated. Each program return a 32-bit value which can deny the action
> >>>>>>>>>> on a kernel object with a non-zero value. If every programs of the list
> >>>>>>>>>> return zero, then the action on the object is allowed.
> >>>>>>>>>>
> >>>>>>>>>> Multiple Landlock programs can be chained to share a 64-bits value for a
> >>>>>>>>>> call chain (e.g. evaluating multiple elements of a file path). This
> >>>>>>>>>> chaining is restricted when a process construct this chain by loading a
> >>>>>>>>>> program, but additional checks are performed when it requests to apply
> >>>>>>>>>> this chain of programs to itself. The restrictions ensure that it is
> >>>>>>>>>> not possible to call multiple programs in a way that would imply to
> >>>>>>>>>> handle multiple shared values (i.e. cookies) for one chain. For now,
> >>>>>>>>>> only a fs_pick program can be chained to the same type of program,
> >>>>>>>>>> because it may make sense if they have different triggers (cf. next
> >>>>>>>>>> commits). This restrictions still allows to reuse Landlock programs in
> >>>>>>>>>> a safe way (e.g. use the same loaded fs_walk program with multiple
> >>>>>>>>>> chains of fs_pick programs).
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> >>>>>>>>>
> >>>>>>>>> ...
> >>>>>>>>>
> >>>>>>>>>> +struct landlock_prog_set *landlock_prepend_prog(
> >>>>>>>>>> + struct landlock_prog_set *current_prog_set,
> >>>>>>>>>> + struct bpf_prog *prog)
> >>>>>>>>>> +{
> >>>>>>>>>> + struct landlock_prog_set *new_prog_set = current_prog_set;
> >>>>>>>>>> + unsigned long pages;
> >>>>>>>>>> + int err;
> >>>>>>>>>> + size_t i;
> >>>>>>>>>> + struct landlock_prog_set tmp_prog_set = {};
> >>>>>>>>>> +
> >>>>>>>>>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
> >>>>>>>>>> + return ERR_PTR(-EINVAL);
> >>>>>>>>>> +
> >>>>>>>>>> + /* validate memory size allocation */
> >>>>>>>>>> + pages = prog->pages;
> >>>>>>>>>> + if (current_prog_set) {
> >>>>>>>>>> + size_t i;
> >>>>>>>>>> +
> >>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
> >>>>>>>>>> + struct landlock_prog_list *walker_p;
> >>>>>>>>>> +
> >>>>>>>>>> + for (walker_p = current_prog_set->programs[i];
> >>>>>>>>>> + walker_p; walker_p = walker_p->prev)
> >>>>>>>>>> + pages += walker_p->prog->pages;
> >>>>>>>>>> + }
> >>>>>>>>>> + /* count a struct landlock_prog_set if we need to allocate one */
> >>>>>>>>>> + if (refcount_read(¤t_prog_set->usage) != 1)
> >>>>>>>>>> + pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
> >>>>>>>>>> + / PAGE_SIZE;
> >>>>>>>>>> + }
> >>>>>>>>>> + if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
> >>>>>>>>>> + return ERR_PTR(-E2BIG);
> >>>>>>>>>> +
> >>>>>>>>>> + /* ensure early that we can allocate enough memory for the new
> >>>>>>>>>> + * prog_lists */
> >>>>>>>>>> + err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
> >>>>>>>>>> + if (err)
> >>>>>>>>>> + return ERR_PTR(err);
> >>>>>>>>>> +
> >>>>>>>>>> + /*
> >>>>>>>>>> + * Each task_struct points to an array of prog list pointers. These
> >>>>>>>>>> + * tables are duplicated when additions are made (which means each
> >>>>>>>>>> + * table needs to be refcounted for the processes using it). When a new
> >>>>>>>>>> + * table is created, all the refcounters on the prog_list are bumped (to
> >>>>>>>>>> + * track each table that references the prog). When a new prog is
> >>>>>>>>>> + * added, it's just prepended to the list for the new table to point
> >>>>>>>>>> + * at.
> >>>>>>>>>> + *
> >>>>>>>>>> + * Manage all the possible errors before this step to not uselessly
> >>>>>>>>>> + * duplicate current_prog_set and avoid a rollback.
> >>>>>>>>>> + */
> >>>>>>>>>> + if (!new_prog_set) {
> >>>>>>>>>> + /*
> >>>>>>>>>> + * If there is no Landlock program set used by the current task,
> >>>>>>>>>> + * then create a new one.
> >>>>>>>>>> + */
> >>>>>>>>>> + new_prog_set = new_landlock_prog_set();
> >>>>>>>>>> + if (IS_ERR(new_prog_set))
> >>>>>>>>>> + goto put_tmp_lists;
> >>>>>>>>>> + } else if (refcount_read(¤t_prog_set->usage) > 1) {
> >>>>>>>>>> + /*
> >>>>>>>>>> + * If the current task is not the sole user of its Landlock
> >>>>>>>>>> + * program set, then duplicate them.
> >>>>>>>>>> + */
> >>>>>>>>>> + new_prog_set = new_landlock_prog_set();
> >>>>>>>>>> + if (IS_ERR(new_prog_set))
> >>>>>>>>>> + goto put_tmp_lists;
> >>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
> >>>>>>>>>> + new_prog_set->programs[i] =
> >>>>>>>>>> + READ_ONCE(current_prog_set->programs[i]);
> >>>>>>>>>> + if (new_prog_set->programs[i])
> >>>>>>>>>> + refcount_inc(&new_prog_set->programs[i]->usage);
> >>>>>>>>>> + }
> >>>>>>>>>> +
> >>>>>>>>>> + /*
> >>>>>>>>>> + * Landlock program set from the current task will not be freed
> >>>>>>>>>> + * here because the usage is strictly greater than 1. It is
> >>>>>>>>>> + * only prevented to be freed by another task thanks to the
> >>>>>>>>>> + * caller of landlock_prepend_prog() which should be locked if
> >>>>>>>>>> + * needed.
> >>>>>>>>>> + */
> >>>>>>>>>> + landlock_put_prog_set(current_prog_set);
> >>>>>>>>>> + }
> >>>>>>>>>> +
> >>>>>>>>>> + /* prepend tmp_prog_set to new_prog_set */
> >>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
> >>>>>>>>>> + /* get the last new list */
> >>>>>>>>>> + struct landlock_prog_list *last_list =
> >>>>>>>>>> + tmp_prog_set.programs[i];
> >>>>>>>>>> +
> >>>>>>>>>> + if (last_list) {
> >>>>>>>>>> + while (last_list->prev)
> >>>>>>>>>> + last_list = last_list->prev;
> >>>>>>>>>> + /* no need to increment usage (pointer replacement) */
> >>>>>>>>>> + last_list->prev = new_prog_set->programs[i];
> >>>>>>>>>> + new_prog_set->programs[i] = tmp_prog_set.programs[i];
> >>>>>>>>>> + }
> >>>>>>>>>> + }
> >>>>>>>>>> + new_prog_set->chain_last = tmp_prog_set.chain_last;
> >>>>>>>>>> + return new_prog_set;
> >>>>>>>>>> +
> >>>>>>>>>> +put_tmp_lists:
> >>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
> >>>>>>>>>> + put_landlock_prog_list(tmp_prog_set.programs[i]);
> >>>>>>>>>> + return new_prog_set;
> >>>>>>>>>> +}
> >>>>>>>>>
> >>>>>>>>> Nack on the chaining concept.
> >>>>>>>>> Please do not reinvent the wheel.
> >>>>>>>>> There is an existing mechanism for attaching/detaching/quering multiple
> >>>>>>>>> programs attached to cgroup and tracing hooks that are also
> >>>>>>>>> efficiently executed via BPF_PROG_RUN_ARRAY.
> >>>>>>>>> Please use that instead.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I don't see how that would help. Suppose you add a filter, then
> >>>>>>>> fork(), and then the child adds another filter. Do you want to
> >>>>>>>> duplicate the entire array? You certainly can't *modify* the array
> >>>>>>>> because you'll affect processes that shouldn't be affected.
> >>>>>>>>
> >>>>>>>> In contrast, doing this through seccomp like the earlier patches
> >>>>>>>> seemed just fine to me, and seccomp already had the right logic.
> >>>>>>>
> >>>>>>> it doesn't look to me that existing seccomp side of managing fork
> >>>>>>> situation can be reused. Here there is an attempt to add 'chaining'
> >>>>>>> concept which sort of an extension of existing seccomp style,
> >>>>>>> but somehow heavily done on bpf side and contradicts cgroup/tracing.
> >>>>>>>
> >>>>>>
> >>>>>> I don't see why the seccomp way can't be used. I agree with you that
> >>>>>> the seccomp *style* shouldn't be used in bpf code like this, but I
> >>>>>> think that Landlock programs can and should just live in the existing
> >>>>>> seccomp chain. If the existing seccomp code needs some modification
> >>>>>> to make this work, then so be it.
> >>>>>
> >>>>> +1
> >>>>> if that was the case...
> >>>>> but that's not my reading of the patch set.
> >>>>
> >>>> An earlier version of the patch set used the seccomp filter chain.
> >>>> Mickaël, what exactly was wrong with that approach other than that the
> >>>> seccomp() syscall was awkward for you to use? You could add a
> >>>> seccomp_add_landlock_rule() syscall if you needed to.
> >>>
> >>> Nothing was wrong about about that, this part did not changed (see my
> >>> next comment).
> >>>
> >>>>
> >>>> As a side comment, why is this an LSM at all, let alone a non-stacking
> >>>> LSM? It would make a lot more sense to me to make Landlock depend on
> >>>> having LSMs configured in but to call the landlock hooks directly from
> >>>> the security_xyz() hooks.
> >>>
> >>> See Casey's answer and his patch series: https://lwn.net/Articles/741963/
> >>>
> >>>>
> >>>>>
> >>>>>> In other words, the kernel already has two kinds of chaining:
> >>>>>> seccomp's and bpf's. bpf's doesn't work right for this type of usage
> >>>>>> across fork(), whereas seccomp's already handles that case correctly.
> >>>>>> (In contrast, seccomp's is totally wrong for cgroup-attached filters.)
> >>>>>> So IMO Landlock should use the seccomp core code and call into bpf
> >>>>>> for the actual filtering.
> >>>>>
> >>>>> +1
> >>>>> in cgroup we had to invent this new BPF_PROG_RUN_ARRAY mechanism,
> >>>>> since cgroup hierarchy can be complicated with bpf progs attached
> >>>>> at different levels with different override/multiprog properties,
> >>>>> so walking link list and checking all flags at run-time would have
> >>>>> been too slow. That's why we added compute_effective_progs().
> >>>>
> >>>> If we start adding override flags to Landlock, I think we're doing it
> >>>> wrong. With cgroup bpf programs, the whole mess is set up by the
> >>>> administrator. With seccomp, and with Landlock if done correctly, it
> >>>> *won't* be set up by the administrator, so the chance that everyone
> >>>> gets all the flags right is about zero. All attached filters should
> >>>> run unconditionally.
> >>>
> >>>
> >>> There is a misunderstanding about this chaining mechanism. This should
> >>> not be confused with the list of seccomp filters nor the cgroup
> >>> hierarchies. Landlock programs can be stacked the same way seccomp's
> >>> filters can (cf. struct landlock_prog_set, the "chain_last" field is an
> >>> optimization which is not used for this struct handling). This stackable
> >>> property did not changed from the previous patch series. The chaining
> >>> mechanism is for another use case, which does not make sense for seccomp
> >>> filters nor other eBPF program types, at least for now, from what I can
> >>> tell.
> >>>
> >>> You may want to get a look at my talk at FOSDEM
> >>> (https://landlock.io/talks/2018-02-04_landlock-fosdem.pdf), especially
> >>> slides 11 and 12.
> >>>
> >>> Let me explain my reasoning about this program chaining thing.
> >>>
> >>> To check if an action on a file is allowed, we first need to identify
> >>> this file and match it to the security policy. In a previous
> >>> (non-public) patch series, I tried to use one type of eBPF program to
> >>> check every kind of access to a file. To be able to identify a file, I
> >>> relied on an eBPF map, similar to the current inode map. This map store
> >>> a set of references to file descriptors. I then created a function
> >>> bpf_is_file_beneath() to check if the requested file was beneath a file
> >>> in the map. This way, no chaining, only one eBPF program type to check
> >>> an access to a file... but some issues then emerged. First, this design
> >>> create a side-channel which help an attacker using such a program to
> >>> infer some information not normally available, for example to get a hint
> >>> on where a file descriptor (received from a UNIX socket) come from.
> >>> Another issue is that this type of program would be called for each
> >>> component of a path. Indeed, when the kernel check if an access to a
> >>> file is allowed, it walk through all of the directories in its path
> >>> (checking if the current process is allowed to execute them). That first
> >>> attempt led me to rethink the way we could filter an access to a file
> >>> *path*.
> >>>
> >>> To minimize the number of called to an eBPF program dedicated to
> >>> validate an access to a file path, I decided to create three subtype of
> >>> eBPF programs. The FS_WALK type is called when walking through every
> >>> directory of a file path (except the last one if it is the target). We
> >>> can then restrict this type of program to the minimum set of functions
> >>> it is allowed to call and the minimum set of data available from its
> >>> context. The first implicit chaining is for this type of program. To be
> >>> able to evaluate a path while being called for all its components, this
> >>> program need to store a state (to remember what was the parent directory
> >>> of this path). There is no "previous" field in the subtype for this
> >>> program because it is chained with itself, for each directories. This
> >>> enable to create a FS_WALK program to evaluate a file hierarchy, thank
> >>> to the inode map which can be used to check if a directory of this
> >>> hierarchy is part of an allowed (or denied) list of directories. This
> >>> design enables to express a file hierarchy in a programmatic way,
> >>> without requiring an eBPF helper to do the job (unlike my first experiment).
> >>>
> >>> The explicit chaining is used to tied a path evaluation (with a FS_WALK
> >>> program) to an access to the actual file being requested (the last
> >>> component of a file path), with a FS_PICK program. It is only at this
> >>> time that the kernel check for the requested action (e.g. read, write,
> >>> chdir, append...). To be able to filter such access request we can have
> >>> one call to the same program for every action and let this program check
> >>> for which action it was called. However, this design does not allow the
> >>> kernel to know if the current action is indeed handled by this program.
> >>> Hence, it is not possible to implement a cache mechanism to only call
> >>> this program if it knows how to handle this action.
> >>>
> >>> The approach I took for this FS_PICK type of program is to add to its
> >>> subtype which action it can handle (with the "triggers" bitfield, seen
> >>> as ORed actions). This way, the kernel knows if a call to a FS_PICK
> >>> program is necessary. If the user wants to enforce a different security
> >>> policy according to the action requested on a file, then it needs
> >>> multiple FS_PICK programs. However, to reduce the number of such
> >>> programs, this patch series allow a FS_PICK program to be chained with
> >>> another, the same way a FS_WALK is chained with itself. This way, if the
> >>> user want to check if the action is a for example an "open" and a "read"
> >>> and not a "map" and a "read", then it can chain multiple FS_PICK
> >>> programs with different triggers actions. The OR check performed by the
> >>> kernel is not a limitation then, only a way to know if a call to an eBPF
> >>> program is needed.
> >>>
> >>> The last type of program is FS_GET. This one is called when a process
> >>> get a struct file or change its working directory. This is the only
> >>> program type able (and allowed) to tag a file. This restriction is
> >>> important to not being subject to resource exhaustion attacks (i.e.
> >>> tagging every inode accessible to an attacker, which would allocate too
> >>> much kernel memory).
> >>>
> >>> This design gives room for improvements to create a cache of eBPF
> >>> context (input data, including maps if any), with the result of an eBPF
> >>> program. This would help limit the number of call to an eBPF program the
> >>> same way SELinux or other kernel components do to limit costly checks.
> >>>
> >>> The eBPF maps of progs are useful to call the same type of eBPF
> >>> program. It does not fit with this use case because we may want multiple
> >>> eBPF program according to the action requested on a kernel object (e.g.
> >>> FS_GET). The other reason is because the eBPF program does not know what
> >>> will be the next (type of) access check performed by the kernel.
> >>>
> >>> To say it another way, this chaining mechanism is a way to split a
> >>> kernel object evaluation with multiple specialized programs, each of
> >>> them being able to deal with data tied to their type. Using a monolithic
> >>> eBPF program to check everything does not scale and does not fit with
> >>> unprivileged use either.
> >>>
> >>> As a side note, the cookie value is only an ephemeral value to keep a
> >>> state between multiple programs call. It can be used to create a state
> >>> machine for an object evaluation.
> >>>
> >>> I don't see a way to do an efficient and programmatic path evaluation,
> >>> with different access checks, with the current eBPF features. Please let
> >>> me know if you know how to do it another way.
> >>>
> >>
> >> Andy, Alexei, Daniel, what do you think about this Landlock program
> >> chaining and cookie?
> >>
> >
> > Can you give a small pseudocode real world example that acutally needs
> > chaining? The mechanism is quite complicated and I'd like to
> > understand how it'll be used.
> >
>
> Here is the interesting part from the example (patch 09/11):
>
> +SEC("maps")
> +struct bpf_map_def inode_map = {
> + .type = BPF_MAP_TYPE_INODE,
> + .key_size = sizeof(u32),
> + .value_size = sizeof(u64),
> + .max_entries = 20,
> +};
> +
> +SEC("subtype/landlock1")
> +static union bpf_prog_subtype _subtype1 = {
> + .landlock_hook = {
> + .type = LANDLOCK_HOOK_FS_WALK,
> + }
> +};
> +
> +static __always_inline __u64 update_cookie(__u64 cookie, __u8 lookup,
> + void *inode, void *chain, bool freeze)
> +{
> + __u64 map_allow = 0;
> +
> + if (cookie == 0) {
> + cookie = bpf_inode_get_tag(inode, chain);
> + if (cookie)
> + return cookie;
> + /* only look for the first match in the map, ignore nested
> + * paths in this example */
> + map_allow = bpf_inode_map_lookup(&inode_map, inode);
> + if (map_allow)
> + cookie = 1 | map_allow;
> + } else {
> + if (cookie & COOKIE_VALUE_FREEZED)
> + return cookie;
> + map_allow = cookie & _MAP_MARK_MASK;
> + cookie &= ~_MAP_MARK_MASK;
> + switch (lookup) {
> + case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOTDOT:
> + cookie--;
> + break;
> + case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOT:
> + break;
> + default:
> + /* ignore _MAP_MARK_MASK overflow in this example */
> + cookie++;
> + break;
> + }
> + if (cookie >= 1)
> + cookie |= map_allow;
> + }
> + /* do not modify the cookie for each fs_pick */
> + if (freeze && cookie)
> + cookie |= COOKIE_VALUE_FREEZED;
> + return cookie;
> +}
> +
> +SEC("landlock1")
> +int fs_walk(struct landlock_ctx_fs_walk *ctx)
> +{
> + ctx->cookie = update_cookie(ctx->cookie, ctx->inode_lookup,
> + (void *)ctx->inode, (void *)ctx->chain, false);
> + return LANDLOCK_RET_ALLOW;
> +}
>
> The program "landlock1" is called for every directory execution (except
> the last one if it is the leaf of a path). This enables to identify a
> file hierarchy with only a (one dimension) list of file descriptors
> (i.e. inode_map).
>
> Underneath, the Landlock LSM part looks if there is an associated path
> walk (nameidata) with each inode access request. If there is one, then
> the cookie associated with the path walk (if any) is made available
> through the eBPF program context. This enables to develop a state
> machine with an eBPF program to "evaluate" a file path (without string
> parsing).
>
> The goal with this chaining mechanism is to be able to express a complex
> kernel object like a file, with multiple run of one or more eBPF
> programs, as a multilayer evaluation. This semantic may only make sense
> for the user/developer and his security policy. We must keep in mind
> that this object identification should be available to unprivileged
> processes. This means that we must be very careful to what kind of
> information are available to an eBPF program because this can then leak
> to a process (e.g. through a map). With this mechanism, only information
> already available to user space is available to the eBPF program.
>
> In this example, the complexity of the path evaluation is in the eBPF
> program. We can then keep the kernel code more simple and generic. This
> enables more flexibility for a security policy definition.
it all sounds correct on paper, but it's pretty novel
approach and I'm not sure I see all the details in the patch.
When people say "inode" they most of the time mean inode integer number,
whereas in this patch do you mean a raw pointer to in-kernel
'struct inode' ?
To avoid confusion it should probably be called differently.
If you meant inode as a number then why inode only?
where is superblock, device, mount point?
How bpf side can compare inodes without this additional info?
How bpf side will know what inode to compare to?
What if inode number is reused?
This approach is an optimization to compare inodes
instead of strings passed into sys_open ?
If you meant inode as a pointer how bpf side will
know the pointer before the walk begins?
What guarantees that it's not a stale pointer?
^ permalink raw reply
* [PATCH] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: mhocko @ 2018-04-11 12:04 UTC (permalink / raw)
To: Michael Kerrisk
Cc: John Hubbard, Andrew Morton, linux-mm, LKML, linux-api,
Michal Hocko
From: Michal Hocko <mhocko@suse.com>
4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
atomicaly probe for a given address range.
[wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
Andrew's sent the MAP_FIXED_NOREPLACE to Linus for the upcoming merge
window. So here we go with the man page update.
man2/mmap.2 | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/man2/mmap.2 b/man2/mmap.2
index ea64eb8f0dcc..f702f3e4eba2 100644
--- a/man2/mmap.2
+++ b/man2/mmap.2
@@ -261,6 +261,27 @@ Examples include
and the PAM libraries
.UR http://www.linux-pam.org
.UE .
+Newer kernels
+(Linux 4.17 and later) have a
+.B MAP_FIXED_NOREPLACE
+option that avoids the corruption problem; if available, MAP_FIXED_NOREPLACE
+should be preferred over MAP_FIXED.
+.TP
+.BR MAP_FIXED_NOREPLACE " (since Linux 4.17)"
+Similar to MAP_FIXED with respect to the
+.I
+addr
+enforcement, but different in that MAP_FIXED_NOREPLACE never clobbers a pre-existing
+mapped range. If the requested range would collide with an existing
+mapping, then this call fails with
+.B EEXIST.
+This flag can therefore be used as a way to atomically (with respect to other
+threads) attempt to map an address range: one thread will succeed; all others
+will report failure. Please note that older kernels which do not recognize this
+flag will typically (upon detecting a collision with a pre-existing mapping)
+fall back to a "non-MAP_FIXED" type of behavior: they will return an address that
+is different than the requested one. Therefore, backward-compatible software
+should check the returned address against the requested address.
.TP
.B MAP_GROWSDOWN
This flag is used for stacks.
@@ -487,6 +508,12 @@ is not a valid file descriptor (and
.B MAP_ANONYMOUS
was not set).
.TP
+.B EEXIST
+range covered by
+.IR addr ,
+.IR length
+is clashing with an existing mapping.
+.TP
.B EINVAL
We don't like
.IR addr ,
--
2.16.3
^ permalink raw reply related
* Re: [PATCH 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Vlastimil Babka @ 2018-04-11 13:16 UTC (permalink / raw)
To: Roman Gushchin, linux-mm
Cc: Andrew Morton, Alexander Viro, Michal Hocko, Johannes Weiner,
linux-fsdevel, linux-kernel, kernel-team, Linux API
In-Reply-To: <20180305133743.12746-2-guro@fb.com>
[+CC linux-api]
On 03/05/2018 02:37 PM, Roman Gushchin wrote:
> This patch introduces a concept of indirectly reclaimable memory
> and adds the corresponding memory counter and /proc/vmstat item.
>
> Indirectly reclaimable memory is any sort of memory, used by
> the kernel (except of reclaimable slabs), which is actually
> reclaimable, i.e. will be released under memory pressure.
>
> The counter is in bytes, as it's not always possible to
> count such objects in pages. The name contains BYTES
> by analogy to NR_KERNEL_STACK_KB.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: kernel-team@fb.com
Hmm, looks like I'm late and this user-visible API change was just
merged. But it's for rc1, so we can still change it, hopefully?
One problem I see with the counter is that it's in bytes, but among
counters that use pages, and the name doesn't indicate it. Then, I don't
see why users should care about the "indirectly" part, as that's just an
implementation detail. It is reclaimable and that's what matters, right?
(I also wanted to complain about lack of Documentation/... update, but
looks like there's no general file about vmstat, ugh)
I also kind of liked the idea from v1 rfc posting that there would be a
separate set of reclaimable kmalloc-X caches for these kind of
allocations. Besides accounting, it should also help reduce memory
fragmentation. The right variant of cache would be detected via
__GFP_RECLAIMABLE.
With that in mind, can we at least for now put the (manually maintained)
byte counter in a variable that's not directly exposed via /proc/vmstat,
and then when printing nr_slab_reclaimable, simply add the value
(divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
subtract the same value. This way we would be simply making the existing
counters more precise, in line with their semantics.
Thoughts?
Vlastimil
> ---
> include/linux/mmzone.h | 1 +
> mm/vmstat.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index e09fe563d5dc..15e783f29e21 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -180,6 +180,7 @@ enum node_stat_item {
> NR_VMSCAN_IMMEDIATE, /* Prioritise for reclaim when writeback ends */
> NR_DIRTIED, /* page dirtyings since bootup */
> NR_WRITTEN, /* page writings since bootup */
> + NR_INDIRECTLY_RECLAIMABLE_BYTES, /* measured in bytes */
> NR_VM_NODE_STAT_ITEMS
> };
>
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 40b2db6db6b1..b6b5684f31fe 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1161,6 +1161,7 @@ const char * const vmstat_text[] = {
> "nr_vmscan_immediate_reclaim",
> "nr_dirtied",
> "nr_written",
> + "nr_indirectly_reclaimable",
>
> /* enum writeback_stat_item counters */
> "nr_dirty_threshold",
>
^ permalink raw reply
* Re: [PATCH 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Roman Gushchin @ 2018-04-11 13:56 UTC (permalink / raw)
To: Vlastimil Babka
Cc: linux-mm, Andrew Morton, Alexander Viro, Michal Hocko,
Johannes Weiner, linux-fsdevel, linux-kernel, kernel-team,
Linux API
In-Reply-To: <08524819-14ef-81d0-fa90-d7af13c6b9d5@suse.cz>
On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote:
> [+CC linux-api]
>
> On 03/05/2018 02:37 PM, Roman Gushchin wrote:
> > This patch introduces a concept of indirectly reclaimable memory
> > and adds the corresponding memory counter and /proc/vmstat item.
> >
> > Indirectly reclaimable memory is any sort of memory, used by
> > the kernel (except of reclaimable slabs), which is actually
> > reclaimable, i.e. will be released under memory pressure.
> >
> > The counter is in bytes, as it's not always possible to
> > count such objects in pages. The name contains BYTES
> > by analogy to NR_KERNEL_STACK_KB.
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-mm@kvack.org
> > Cc: kernel-team@fb.com
>
> Hmm, looks like I'm late and this user-visible API change was just
> merged. But it's for rc1, so we can still change it, hopefully?
>
> One problem I see with the counter is that it's in bytes, but among
> counters that use pages, and the name doesn't indicate it.
Here I just followed "nr_kernel_stack" path, which is measured in kB,
but this is not mentioned in the field name.
> Then, I don't
> see why users should care about the "indirectly" part, as that's just an
> implementation detail. It is reclaimable and that's what matters, right?
> (I also wanted to complain about lack of Documentation/... update, but
> looks like there's no general file about vmstat, ugh)
I agree, that it's a bit weird, and it's probably better to not expose
it at all; but this is how all vm counters work. We do expose them all
in /proc/vmstat. A good number of them is useless until you are not a
mm developer, so it's arguable more "debug info" rather than "api".
It's definitely not a reason to make them messy.
Does "nr_indirectly_reclaimable_bytes" look better to you?
>
> I also kind of liked the idea from v1 rfc posting that there would be a
> separate set of reclaimable kmalloc-X caches for these kind of
> allocations. Besides accounting, it should also help reduce memory
> fragmentation. The right variant of cache would be detected via
> __GFP_RECLAIMABLE.
Well, the downside is that we have to introduce X new caches
just for this particular problem. I'm not strictly against the idea,
but not convinced that it's much better.
>
> With that in mind, can we at least for now put the (manually maintained)
> byte counter in a variable that's not directly exposed via /proc/vmstat,
> and then when printing nr_slab_reclaimable, simply add the value
> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
> subtract the same value. This way we would be simply making the existing
> counters more precise, in line with their semantics.
Idk, I don't like the idea of adding a counter outside of the vm counters
infrastructure, and I definitely wouldn't touch the exposed
nr_slab_reclaimable and nr_slab_unreclaimable fields.
We do have some stats in /proc/slabinfo, /proc/meminfo and /sys/kernel/slab
and I think that we should keep it consistent.
Thanks!
>
> Thoughts?
> Vlastimil
>
> > ---
> > include/linux/mmzone.h | 1 +
> > mm/vmstat.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index e09fe563d5dc..15e783f29e21 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -180,6 +180,7 @@ enum node_stat_item {
> > NR_VMSCAN_IMMEDIATE, /* Prioritise for reclaim when writeback ends */
> > NR_DIRTIED, /* page dirtyings since bootup */
> > NR_WRITTEN, /* page writings since bootup */
> > + NR_INDIRECTLY_RECLAIMABLE_BYTES, /* measured in bytes */
> > NR_VM_NODE_STAT_ITEMS
> > };
> >
> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > index 40b2db6db6b1..b6b5684f31fe 100644
> > --- a/mm/vmstat.c
> > +++ b/mm/vmstat.c
> > @@ -1161,6 +1161,7 @@ const char * const vmstat_text[] = {
> > "nr_vmscan_immediate_reclaim",
> > "nr_dirtied",
> > "nr_written",
> > + "nr_indirectly_reclaimable",
> >
> > /* enum writeback_stat_item counters */
> > "nr_dirty_threshold",
> >
>
^ 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