* [PATCH v7 02/11] proc: add proc_fs_info struct to store proc information
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This is a preparation patch that adds proc_fs_info to be able to store
different procfs options and informations. Right now some mount options
are stored inside the pid namespace which makes it hard to change or
modernize procfs without affecting pid namespaces. Plus we do want to
treat proc as more of a real mount point and filesystem. procfs is part
of Linux API where it offers some features using filesystem syscalls and
in order to support some features where we are able to have multiple
instances of procfs, each one with its mount options inside the same pid
namespace, we have to separate these procfs instances.
This is the same feature that was also added to other Linux interfaces
like devpts in order to support containers, sandboxes, and to have
multiple instances of devpts filesystem [1].
[1] https://elixir.bootlin.com/linux/v3.4/source/Documentation/filesystems/devpts.txt
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/locks.c | 6 +++--
fs/proc/base.c | 8 +++++--
fs/proc/inode.c | 4 ++--
fs/proc/root.c | 49 +++++++++++++++++++++++++++--------------
include/linux/proc_fs.h | 11 ++++++++-
5 files changed, 54 insertions(+), 24 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 6970f55daf54..21200e3005e4 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2795,7 +2795,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
{
struct inode *inode = NULL;
unsigned int fl_pid;
- struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(file_inode(f->file)->i_sb);
+ struct pid_namespace *proc_pidns = fs_info->pid_ns;
fl_pid = locks_translate_pid(fl, proc_pidns);
/*
@@ -2873,7 +2874,8 @@ static int locks_show(struct seq_file *f, void *v)
{
struct locks_iterator *iter = f->private;
struct file_lock *fl, *bfl;
- struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(file_inode(f->file)->i_sb);
+ struct pid_namespace *proc_pidns = fs_info->pid_ns;
fl = hlist_entry(v, struct file_lock, fl_link);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..672e71c52dbd 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3243,6 +3243,7 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
{
struct task_struct *task;
unsigned tgid;
+ struct proc_fs_info *fs_info;
struct pid_namespace *ns;
struct dentry *result = ERR_PTR(-ENOENT);
@@ -3250,7 +3251,8 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
if (tgid == ~0U)
goto out;
- ns = dentry->d_sb->s_fs_info;
+ fs_info = proc_sb_info(dentry->d_sb);
+ ns = fs_info->pid_ns;
rcu_read_lock();
task = find_task_by_pid_ns(tgid, ns);
if (task)
@@ -3538,6 +3540,7 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry
struct task_struct *task;
struct task_struct *leader = get_proc_task(dir);
unsigned tid;
+ struct proc_fs_info *fs_info;
struct pid_namespace *ns;
struct dentry *result = ERR_PTR(-ENOENT);
@@ -3548,7 +3551,8 @@ static struct dentry *proc_task_lookup(struct inode *dir, struct dentry * dentry
if (tid == ~0U)
goto out;
- ns = dentry->d_sb->s_fs_info;
+ fs_info = proc_sb_info(dentry->d_sb);
+ ns = fs_info->pid_ns;
rcu_read_lock();
task = find_task_by_pid_ns(tid, ns);
if (task)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index dbe43a50caf2..b631608dfbcc 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -104,8 +104,8 @@ void __init proc_init_kmemcache(void)
static int proc_show_options(struct seq_file *seq, struct dentry *root)
{
- struct super_block *sb = root->d_sb;
- struct pid_namespace *pid = sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(root->d_sb);
+ struct pid_namespace *pid = fs_info->pid_ns;
if (!gid_eq(pid->pid_gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, pid->pid_gid));
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 0b7c8dffc9ae..d449f095f0f7 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -30,7 +30,7 @@
#include "internal.h"
struct proc_fs_context {
- struct pid_namespace *pid_ns;
+ struct proc_fs_info *fs_info;
unsigned int mask;
int hidepid;
int gid;
@@ -97,7 +97,8 @@ static void proc_apply_options(struct super_block *s,
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
{
- struct pid_namespace *pid_ns = get_pid_ns(s->s_fs_info);
+ struct proc_fs_context *ctx = fc->fs_private;
+ struct pid_namespace *pid_ns = get_pid_ns(ctx->fs_info->pid_ns);
struct inode *root_inode;
int ret;
@@ -145,7 +146,8 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
static int proc_reconfigure(struct fs_context *fc)
{
struct super_block *sb = fc->root->d_sb;
- struct pid_namespace *pid = sb->s_fs_info;
+ struct proc_fs_info *fs_info = proc_sb_info(sb);
+ struct pid_namespace *pid = fs_info->pid_ns;
sync_filesystem(sb);
@@ -157,14 +159,14 @@ static int proc_get_tree(struct fs_context *fc)
{
struct proc_fs_context *ctx = fc->fs_private;
- return get_tree_keyed(fc, proc_fill_super, ctx->pid_ns);
+ return get_tree_keyed(fc, proc_fill_super, ctx->fs_info);
}
static void proc_fs_context_free(struct fs_context *fc)
{
struct proc_fs_context *ctx = fc->fs_private;
- put_pid_ns(ctx->pid_ns);
+ put_pid_ns(ctx->fs_info->pid_ns);
kfree(ctx);
}
@@ -178,14 +180,27 @@ static const struct fs_context_operations proc_fs_context_ops = {
static int proc_init_fs_context(struct fs_context *fc)
{
struct proc_fs_context *ctx;
+ struct pid_namespace *pid_ns;
ctx = kzalloc(sizeof(struct proc_fs_context), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
- ctx->pid_ns = get_pid_ns(task_active_pid_ns(current));
+ pid_ns = get_pid_ns(task_active_pid_ns(current));
+
+ if (!pid_ns->proc_mnt) {
+ ctx->fs_info = kzalloc(sizeof(struct proc_fs_info), GFP_KERNEL);
+ if (!ctx->fs_info) {
+ kfree(ctx);
+ return -ENOMEM;
+ }
+ ctx->fs_info->pid_ns = pid_ns;
+ } else {
+ ctx->fs_info = proc_sb_info(pid_ns->proc_mnt->mnt_sb);
+ }
+
put_user_ns(fc->user_ns);
- fc->user_ns = get_user_ns(ctx->pid_ns->user_ns);
+ fc->user_ns = get_user_ns(ctx->fs_info->pid_ns->user_ns);
fc->fs_private = ctx;
fc->ops = &proc_fs_context_ops;
return 0;
@@ -193,15 +208,15 @@ static int proc_init_fs_context(struct fs_context *fc)
static void proc_kill_sb(struct super_block *sb)
{
- struct pid_namespace *ns;
+ struct proc_fs_info *fs_info = proc_sb_info(sb);
- ns = (struct pid_namespace *)sb->s_fs_info;
- if (ns->proc_self)
- dput(ns->proc_self);
- if (ns->proc_thread_self)
- dput(ns->proc_thread_self);
+ if (fs_info->pid_ns->proc_self)
+ dput(fs_info->pid_ns->proc_self);
+ if (fs_info->pid_ns->proc_thread_self)
+ dput(fs_info->pid_ns->proc_thread_self);
kill_anon_super(sb);
- put_pid_ns(ns);
+ put_pid_ns(fs_info->pid_ns);
+ kfree(fs_info);
}
static struct file_system_type proc_fs_type = {
@@ -314,10 +329,10 @@ int pid_ns_prepare_proc(struct pid_namespace *ns)
}
ctx = fc->fs_private;
- if (ctx->pid_ns != ns) {
- put_pid_ns(ctx->pid_ns);
+ if (ctx->fs_info->pid_ns != ns) {
+ put_pid_ns(ctx->fs_info->pid_ns);
get_pid_ns(ns);
- ctx->pid_ns = ns;
+ ctx->fs_info->pid_ns = ns;
}
mnt = fc_mount(fc);
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index a705aa2d03f9..2d79489e55aa 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -12,6 +12,15 @@ struct proc_dir_entry;
struct seq_file;
struct seq_operations;
+struct proc_fs_info {
+ struct pid_namespace *pid_ns;
+};
+
+static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
+{
+ return sb->s_fs_info;
+}
+
#ifdef CONFIG_PROC_FS
typedef int (*proc_write_t)(struct file *, char *, size_t);
@@ -146,7 +155,7 @@ int open_related_ns(struct ns_common *ns,
/* get the associated pid namespace for a file in procfs */
static inline struct pid_namespace *proc_pid_ns(const struct inode *inode)
{
- return inode->i_sb->s_fs_info;
+ return proc_sb_info(inode->i_sb)->pid_ns;
}
#endif /* _LINUX_PROC_FS_H */
--
2.24.1
^ permalink raw reply related
* [PATCH v7 01/11] proc: Rename struct proc_fs_info to proc_fs_opts
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200125130541.450409-1-gladkov.alexey-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Alexey Gladkov <gladkov.alexey-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
fs/proc_namespace.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 273ee82d8aa9..9a8b624bc3db 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -37,23 +37,23 @@ static __poll_t mounts_poll(struct file *file, poll_table *wait)
return res;
}
-struct proc_fs_info {
+struct proc_fs_opts {
int flag;
const char *str;
};
static int show_sb_opts(struct seq_file *m, struct super_block *sb)
{
- static const struct proc_fs_info fs_info[] = {
+ static const struct proc_fs_opts fs_opts[] = {
{ SB_SYNCHRONOUS, ",sync" },
{ SB_DIRSYNC, ",dirsync" },
{ SB_MANDLOCK, ",mand" },
{ SB_LAZYTIME, ",lazytime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = fs_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = fs_opts; fs_infop->flag; fs_infop++) {
if (sb->s_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
@@ -63,7 +63,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb)
static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{
- static const struct proc_fs_info mnt_info[] = {
+ static const struct proc_fs_opts mnt_opts[] = {
{ MNT_NOSUID, ",nosuid" },
{ MNT_NODEV, ",nodev" },
{ MNT_NOEXEC, ",noexec" },
@@ -72,9 +72,9 @@ static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
{ MNT_RELATIME, ",relatime" },
{ 0, NULL }
};
- const struct proc_fs_info *fs_infop;
+ const struct proc_fs_opts *fs_infop;
- for (fs_infop = mnt_info; fs_infop->flag; fs_infop++) {
+ for (fs_infop = mnt_opts; fs_infop->flag; fs_infop++) {
if (mnt->mnt_flags & fs_infop->flag)
seq_puts(m, fs_infop->str);
}
--
2.24.1
^ permalink raw reply related
* [PATCH v7 00/11] proc: modernize proc to support multiple private instances
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer
Greetings!
Preface:
--------
This is patchset v7 to modernize procfs and make it able to support multiple
private instances per the same pid namespace.
This patchset can be applied on top of v5.4-rc7-49-g0e3f1ad80fc8
Procfs modernization:
---------------------
Historically procfs was always tied to pid namespaces, during pid
namespace creation we internally create a procfs mount for it. However,
this has the effect that all new procfs mounts are just a mirror of the
internal one, any change, any mount option update, any new future
introduction will propagate to all other procfs mounts that are in the
same pid namespace.
This may have solved several use cases in that time. However today we
face new requirements, and making procfs able to support new private
instances inside same pid namespace seems a major point. If we want to
to introduce new features and security mechanisms we have to make sure
first that we do not break existing usecases. Supporting private procfs
instances will allow to support new features and behaviour without
propagating it to all other procfs mounts.
Today procfs is more of a burden especially to some Embedded, IoT,
sandbox, container use cases. In user space we are over-mounting null
or inaccessible files on top to hide files and information. If we want
to hide pids we have to create PID namespaces otherwise mount options
propagate to all other proc mounts, changing a mount option value in one
mount will propagate to all other proc mounts. If we want to introduce
new features, then they will propagate to all other mounts too, resulting
either maybe new useful functionality or maybe breaking stuff. We have
also to note that userspace should not workaround procfs, the kernel
should just provide a sane simple interface.
In this regard several developers and maintainers pointed out that
there are problems with procfs and it has to be modernized:
"Here's another one: split up and modernize /proc." by Andy Lutomirski [1]
Discussion about kernel pointer leaks:
"And yes, as Kees and Daniel mentioned, it's definitely not just dmesg.
In fact, the primary things tend to be /proc and /sys, not dmesg
itself." By Linus Torvalds [2]
Lot of other areas in the kernel and filesystems have been updated to be
able to support private instances, devpts is one major example [3].
Which will be used for:
1) Embedded systems and IoT: usually we have one supervisor for
apps, we have some lightweight sandbox support, however if we create
pid namespaces we have to manage all the processes inside too,
where our goal is to be able to run a bunch of apps each one inside
its own mount namespace, maybe use network namespaces for vlans
setups, but right now we only want mount namespaces, without all the
other complexity. We want procfs to behave more like a real file system,
and block access to inodes that belong to other users. The 'hidepid=' will
not work since it is a shared mount option.
2) Containers, sandboxes and Private instances of file systems - devpts case
Historically, lot of file systems inside Linux kernel view when instantiated
were just a mirror of an already created and mounted filesystem. This was the
case of devpts filesystem, it seems at that time the requirements were to
optimize things and reuse the same memory, etc. This design used to work but not
anymore with today's containers, IoT, hostile environments and all the privacy
challenges that Linux faces.
In that regards, devpts was updated so that each new mounts is a total
independent file system by the following patches:
"devpts: Make each mount of devpts an independent filesystem" by
Eric W. Biederman [3] [4]
3) Linux Security Modules have multiple ptrace paths inside some
subsystems, however inside procfs, the implementation does not guarantee
that the ptrace() check which triggers the security_ptrace_check() hook
will always run. We have the 'hidepid' mount option that can be used to
force the ptrace_may_access() check inside has_pid_permissions() to run.
The problem is that 'hidepid' is per pid namespace and not attached to
the mount point, any remount or modification of 'hidepid' will propagate
to all other procfs mounts.
This also does not allow to support Yama LSM easily in desktop and user
sessions. Yama ptrace scope which restricts ptrace and some other
syscalls to be allowed only on inferiors, can be updated to have a
per-task context, where the context will be inherited during fork(),
clone() and preserved across execve(). If we support multiple private
procfs instances, then we may force the ptrace_may_access() on
/proc/<pids>/ to always run inside that new procfs instances. This will
allow to specifiy on user sessions if we should populate procfs with
pids that the user can ptrace or not.
By using Yama ptrace scope, some restricted users will only be able to see
inferiors inside /proc, they won't even be able to see their other
processes. Some software like Chromium, Firefox's crash handler, Wine
and others are already using Yama to restrict which processes can be
ptracable. With this change this will give the possibility to restrict
/proc/<pids>/ but more importantly this will give desktop users a
generic and usuable way to specifiy which users should see all processes
and which user can not.
Side notes:
* This covers the lack of seccomp where it is not able to parse
arguments, it is easy to install a seccomp filter on direct syscalls
that operate on pids, however /proc/<pid>/ is a Linux ABI using
filesystem syscalls. With this change all LSMs should be able to analyze
open/read/write/close... on /proc/<pid>/
4) This will allow to implement new features either in kernel or
userspace without having to worry about procfs.
In containers, sandboxes, etc we have workarounds to hide some /proc
inodes, this should be supported natively without doing extra complex
work, the kernel should be able to support sane options that work with
today and future Linux use cases.
5) Creation of new superblock with all procfs options for each procfs
mount will fix the ignoring of mount options. The problem is that the
second mount of procfs in the same pid namespace ignores the mount
options. The mount options are ignored without error until procfs is
remounted.
Before:
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
# strace -e mount mount -o hidepid=1 -t proc proc /tmp/proc
mount("proc", "/tmp/proc", "proc", 0, "hidepid=1") = 0
+++ exited with 0 +++
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
proc /tmp/proc proc rw,relatime,hidepid=2 0 0
# mount -o remount,hidepid=1 -t proc proc /tmp/proc
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=1 0 0
proc /tmp/proc proc rw,relatime,hidepid=1 0 0
After:
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
# mount -o hidepid=1 -t proc proc /tmp/proc
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
proc /tmp/proc proc rw,relatime,hidepid=1 0 0
Introduced changes:
-------------------
Each mount of procfs creates a separate procfs instance with its own
mount options.
This series adds few new mount options:
* New 'hidepid=4' mount option to show only ptraceable processes in the procfs.
This allows to support lightweight sandboxes in Embedded Linux, also
solves the case for LSM where now with this mount option, we make sure
that they have a ptrace path in procfs.
* 'subset=pidfs' that allows to hide non-pid inodes from procfs. It can be used
in containers and sandboxes, as these are already trying to hide and block
access to procfs inodes anyway.
ChangeLog:
----------
# v7:
* 'pidonly=1' renamed to 'subset=pidfs' as Alexey Dobriyan suggested.
* HIDEPID_* moved to uapi/ as they are user interface to mount().
Suggested-by Alexey Dobriyan <adobriyan@gmail.com>
# v6:
* 'hidepid=' and 'gid=' mount options are moved from pid namespace to superblock.
* 'newinstance' mount option removed as Eric W. Biederman suggested.
Mount of procfs always creates a new instance.
* 'limit_pids' renamed to 'hidepid=3'.
* I took into account the comment of Linus Torvalds [7].
* Documentation added.
# v5:
* Fixed a bug that caused a problem with the Fedora boot.
* The 'pidonly' option is visible among the mount options.
# v2:
* Renamed mount options to 'newinstance' and 'pids='
Suggested-by: Andy Lutomirski <luto@kernel.org>
* Fixed order of commit, Suggested-by: Andy Lutomirski <luto@kernel.org>
* Many bug fixes.
# v1:
* Removed 'unshared' mount option and replaced it with 'limit_pids'
which is attached to the current procfs mount.
Suggested-by Andy Lutomirski <luto@kernel.org>
* Do not fill dcache with pid entries that we can not ptrace.
* Many bug fixes.
References:
-----------
[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-January/004215.html
[2] http://www.openwall.com/lists/kernel-hardening/2017/10/05/5
[3] https://lwn.net/Articles/689539/
[4] http://lxr.free-electrons.com/source/Documentation/filesystems/devpts.txt?v=3.14
[5] https://lkml.org/lkml/2017/5/2/407
[6] https://lkml.org/lkml/2017/5/3/357
[7] https://lkml.org/lkml/2018/5/11/505
Alexey Gladkov (11):
proc: Rename struct proc_fs_info to proc_fs_opts
proc: add proc_fs_info struct to store proc information
proc: move /proc/{self|thread-self} dentries to proc_fs_info
proc: move hide_pid, pid_gid from pid_namespace to proc_fs_info
proc: add helpers to set and get proc hidepid and gid mount options
proc: support mounting procfs instances inside same pid namespace
proc: flush task dcache entries from all procfs instances
proc: instantiate only pids that we can ptrace on 'hidepid=4' mount
option
proc: add option to mount only a pids subset
docs: proc: add documentation for "hidepid=4" and "subset=pidfs"
options and new mount behavior
proc: Move hidepid values to uapi as they are user interface to mount
Documentation/filesystems/proc.txt | 53 ++++++++++++
fs/locks.c | 6 +-
fs/proc/base.c | 69 ++++++++++++----
fs/proc/generic.c | 9 ++
fs/proc/inode.c | 21 +++--
fs/proc/internal.h | 30 +++++++
fs/proc/root.c | 128 +++++++++++++++++++++++------
fs/proc/self.c | 4 +-
fs/proc/thread_self.c | 6 +-
fs/proc_namespace.c | 14 ++--
include/linux/pid_namespace.h | 54 +++++++++---
include/linux/proc_fs.h | 25 +++++-
include/uapi/linux/proc_fs.h | 13 +++
13 files changed, 355 insertions(+), 77 deletions(-)
create mode 100644 include/uapi/linux/proc_fs.h
--
2.24.1
^ permalink raw reply
* Re: [PATCH] Add prctl support for controlling mem reclaim V4
From: Darrick J. Wong @ 2020-01-24 22:09 UTC (permalink / raw)
To: Mike Christie
Cc: linux-api, idryomov, mhocko, david, linux-mm, linux-kernel,
linux-scsi, linux-fsdevel, linux-block, martin, Damien.LeMoal,
Michal Hocko, Masato Suzuki
In-Reply-To: <20191112001900.9206-1-mchristi@redhat.com>
On Mon, Nov 11, 2019 at 06:19:00PM -0600, Mike Christie wrote:
> There are several storage drivers like dm-multipath, iscsi, tcmu-runner,
> amd nbd that have userspace components that can run in the IO path. For
> example, iscsi and nbd's userspace deamons may need to recreate a socket
> and/or send IO on it, and dm-multipath's daemon multipathd may need to
> send SG IO or read/write IO to figure out the state of paths and re-set
> them up.
>
> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the
> memalloc_*_save/restore functions to control the allocation behavior,
> but for userspace we would end up hitting an allocation that ended up
> writing data back to the same device we are trying to allocate for.
> The device is then in a state of deadlock, because to execute IO the
> device needs to allocate memory, but to allocate memory the memory
> layers want execute IO to the device.
>
> Here is an example with nbd using a local userspace daemon that performs
> network IO to a remote server. We are using XFS on top of the nbd device,
> but it can happen with any FS or other modules layered on top of the nbd
> device that can write out data to free memory. Here a nbd daemon helper
> thread, msgr-worker-1, is performing a write/sendmsg on a socket to execute
> a request. This kicks off a reclaim operation which results in a WRITE to
> the nbd device and the nbd thread calling back into the mm layer.
>
/me would like to see the documentation update to prctl(2).
Assuming that "set takes 0 or 1, get returns 0 or 1" is more or less how
this interface is supposed to work,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> [ 1626.609191] msgr-worker-1 D 0 1026 1 0x00004000
> [ 1626.609193] Call Trace:
> [ 1626.609195] ? __schedule+0x29b/0x630
> [ 1626.609197] ? wait_for_completion+0xe0/0x170
> [ 1626.609198] schedule+0x30/0xb0
> [ 1626.609200] schedule_timeout+0x1f6/0x2f0
> [ 1626.609202] ? blk_finish_plug+0x21/0x2e
> [ 1626.609204] ? _xfs_buf_ioapply+0x2e6/0x410
> [ 1626.609206] ? wait_for_completion+0xe0/0x170
> [ 1626.609208] wait_for_completion+0x108/0x170
> [ 1626.609210] ? wake_up_q+0x70/0x70
> [ 1626.609212] ? __xfs_buf_submit+0x12e/0x250
> [ 1626.609214] ? xfs_bwrite+0x25/0x60
> [ 1626.609215] xfs_buf_iowait+0x22/0xf0
> [ 1626.609218] __xfs_buf_submit+0x12e/0x250
> [ 1626.609220] xfs_bwrite+0x25/0x60
> [ 1626.609222] xfs_reclaim_inode+0x2e8/0x310
> [ 1626.609224] xfs_reclaim_inodes_ag+0x1b6/0x300
> [ 1626.609227] xfs_reclaim_inodes_nr+0x31/0x40
> [ 1626.609228] super_cache_scan+0x152/0x1a0
> [ 1626.609231] do_shrink_slab+0x12c/0x2d0
> [ 1626.609233] shrink_slab+0x9c/0x2a0
> [ 1626.609235] shrink_node+0xd7/0x470
> [ 1626.609237] do_try_to_free_pages+0xbf/0x380
> [ 1626.609240] try_to_free_pages+0xd9/0x1f0
> [ 1626.609245] __alloc_pages_slowpath+0x3a4/0xd30
> [ 1626.609251] ? ___slab_alloc+0x238/0x560
> [ 1626.609254] __alloc_pages_nodemask+0x30c/0x350
> [ 1626.609259] skb_page_frag_refill+0x97/0xd0
> [ 1626.609274] sk_page_frag_refill+0x1d/0x80
> [ 1626.609279] tcp_sendmsg_locked+0x2bb/0xdd0
> [ 1626.609304] tcp_sendmsg+0x27/0x40
> [ 1626.609307] sock_sendmsg+0x54/0x60
> [ 1626.609308] ___sys_sendmsg+0x29f/0x320
> [ 1626.609313] ? sock_poll+0x66/0xb0
> [ 1626.609318] ? ep_item_poll.isra.15+0x40/0xc0
> [ 1626.609320] ? ep_send_events_proc+0xe6/0x230
> [ 1626.609322] ? hrtimer_try_to_cancel+0x54/0xf0
> [ 1626.609324] ? ep_read_events_proc+0xc0/0xc0
> [ 1626.609326] ? _raw_write_unlock_irq+0xa/0x20
> [ 1626.609327] ? ep_scan_ready_list.constprop.19+0x218/0x230
> [ 1626.609329] ? __hrtimer_init+0xb0/0xb0
> [ 1626.609331] ? _raw_spin_unlock_irq+0xa/0x20
> [ 1626.609334] ? ep_poll+0x26c/0x4a0
> [ 1626.609337] ? tcp_tsq_write.part.54+0xa0/0xa0
> [ 1626.609339] ? release_sock+0x43/0x90
> [ 1626.609341] ? _raw_spin_unlock_bh+0xa/0x20
> [ 1626.609342] __sys_sendmsg+0x47/0x80
> [ 1626.609347] do_syscall_64+0x5f/0x1c0
> [ 1626.609349] ? prepare_exit_to_usermode+0x75/0xa0
> [ 1626.609351] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This patch adds a new prctl command that daemons can use after they have
> done their initial setup, and before they start to do allocations that
> are in the IO path. It sets the PF_MEMALLOC_NOIO and PF_LESS_THROTTLE
> flags so both userspace block and FS threads can use it to avoid the
> allocation recursion and try to prevent from being throttled while
> writing out data to free up memory.
>
> Signed-off-by: Mike Christie <mchristi@redhat.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Tested-by: Masato Suzuki <masato.suzuki@wdc.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
>
> ---
>
> V4:
> - Fix PR_GET_IO_FLUSHER check to match SET.
>
> V3:
> - Drop NOFS, set PF_LESS_THROTTLE and rename prctl flag to reflect it
> is more general and can support both FS and block devices. Both fuse
> and block device daemons, nbd and tcmu-runner, have been tested to
> confirm the more restrictive PF_MEMALLOC_NOIO also works for fuse.
>
> - Use CAP_SYS_RESOURCE instead of admin.
>
> V2:
> - Use prctl instead of procfs.
> - Add support for NOFS for fuse.
> - Check permissions.
>
>
> include/uapi/linux/capability.h | 1 +
> include/uapi/linux/prctl.h | 4 ++++
> kernel/sys.c | 25 +++++++++++++++++++++++++
> 3 files changed, 30 insertions(+)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 240fdb9a60f6..272dc69fa080 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -301,6 +301,7 @@ struct vfs_ns_cap_data {
> /* Allow more than 64hz interrupts from the real-time clock */
> /* Override max number of consoles on console allocation */
> /* Override max number of keymaps */
> +/* Control memory reclaim behavior */
>
> #define CAP_SYS_RESOURCE 24
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 7da1b37b27aa..07b4f8131e36 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -234,4 +234,8 @@ struct prctl_mm_map {
> #define PR_GET_TAGGED_ADDR_CTRL 56
> # define PR_TAGGED_ADDR_ENABLE (1UL << 0)
>
> +/* Control reclaim behavior when allocating memory */
> +#define PR_SET_IO_FLUSHER 57
> +#define PR_GET_IO_FLUSHER 58
> +
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index a611d1d58c7d..c1a360370d09 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2259,6 +2259,8 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which,
> return -EINVAL;
> }
>
> +#define PR_IO_FLUSHER (PF_MEMALLOC_NOIO | PF_LESS_THROTTLE)
> +
> SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -2486,6 +2488,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> error = GET_TAGGED_ADDR_CTRL();
> break;
> + case PR_SET_IO_FLUSHER:
> + if (!capable(CAP_SYS_RESOURCE))
> + return -EPERM;
> +
> + if (arg3 || arg4 || arg5)
> + return -EINVAL;
> +
> + if (arg2 == 1)
> + current->flags |= PR_IO_FLUSHER;
> + else if (!arg2)
> + current->flags &= ~PR_IO_FLUSHER;
> + else
> + return -EINVAL;
> + break;
> + case PR_GET_IO_FLUSHER:
> + if (!capable(CAP_SYS_RESOURCE))
> + return -EPERM;
> +
> + if (arg2 || arg3 || arg4 || arg5)
> + return -EINVAL;
> +
> + error = (current->flags & PR_IO_FLUSHER) == PR_IO_FLUSHER;
> + break;
> default:
> error = -EINVAL;
> break;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] Add prctl support for controlling mem reclaim V4
From: Dave Chinner @ 2020-01-24 21:16 UTC (permalink / raw)
To: Mike Christie
Cc: Shakeel Butt, Andrew Morton, linux-api, idryomov, Michal Hocko,
Linux MM, LKML, linux-scsi, linux-fsdevel, linux-block, martin,
Damien.LeMoal, Michal Hocko, Masato Suzuki
In-Reply-To: <5E2B19C9.6080907@redhat.com>
On Fri, Jan 24, 2020 at 10:22:33AM -0600, Mike Christie wrote:
> On 12/05/2019 04:43 PM, Shakeel Butt wrote:
> > On Mon, Nov 11, 2019 at 4:19 PM Mike Christie <mchristi@redhat.com> wrote:
> >> This patch adds a new prctl command that daemons can use after they have
> >> done their initial setup, and before they start to do allocations that
> >> are in the IO path. It sets the PF_MEMALLOC_NOIO and PF_LESS_THROTTLE
> >> flags so both userspace block and FS threads can use it to avoid the
> >> allocation recursion and try to prevent from being throttled while
> >> writing out data to free up memory.
> >>
> >> Signed-off-by: Mike Christie <mchristi@redhat.com>
> >> Acked-by: Michal Hocko <mhocko@suse.com>
> >> Tested-by: Masato Suzuki <masato.suzuki@wdc.com>
> >> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> >
> > I suppose this patch should be routed through MM tree, so, CCing Andrew.
> >
>
> Andrew and other mm/storage developers,
>
> Do I need to handle anything else for this patch, or are there any other
> concerns? Is this maybe something we want to talk about at a quick LSF
> session?
>
> I have retested it with Linus's current tree. It still applies cleanly
> (just some offsets), and fixes the problem described above we have been
> hitting.
I must have missed this version being posted (just looked it up on
lore.kernel.org). As far as I'm concerned this is good to go and it
is absolutely necessary for userspace IO stacks to function
correctly.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
If no manintainer picks it up before the next merge window, then I
recommend resending the latest version to Linus asking him to merge
it.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap
From: Sargun Dhillon @ 2020-01-24 20:09 UTC (permalink / raw)
To: Tycho Andersen
Cc: LKML, Linux Containers, Linux API, Linux FS-devel Mailing List,
Christian Brauner
In-Reply-To: <20200124180332.GA4151@cisco>
On Fri, Jan 24, 2020 at 10:03 AM Tycho Andersen <tycho-E0fblnxP3wo@public.gmane.org> wrote:
>
> On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> > Currently, this just opens the group leader of the thread that triggere
> > the event, as pidfds (currently) are limited to group leaders.
>
> I don't love the semantics of this; when they're not limited to thread
> group leaders any more, we won't be able to change this. Is that work
> far off?
>
> Tycho
We would be able to change this in the future if we introduced a flag like
SECCOMP_USER_NOTIF_FLAG_PIDFD_THREAD which would send a
pidfd that's for the thread, and not just the group leader. The flag could
either be XOR with SECCOMP_USER_NOTIF_FLAG_PIDFD, or
could require both. Alternatively, we can rename
SECCOMP_USER_NOTIF_FLAG_PIDFD to
SECCOMP_USER_NOTIF_FLAG_GROUP_LEADER_PIDFD.
^ permalink raw reply
* [PATCH v2] mm: Add MREMAP_DONTUNMAP to mremap().
From: Brian Geffon @ 2020-01-24 19:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Michael S . Tsirkin, Brian Geffon, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
Andy Lutomirski, Andrea Arcangeli, Sonny Rao, Minchan Kim,
Joel Fernandes, Yu Zhao, Jesse Barnes
In-Reply-To: <20200123014627.71720-1-bgeffon-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
When remapping an anonymous, private mapping, if MREMAP_DONTUNMAP is
set, the source mapping will not be removed. Instead it will be
cleared as if a brand new anonymous, private mapping had been created
atomically as part of the mremap() call. If a userfaultfd was watching
the source, it will continue to watch the new mapping. For a mapping
that is shared or not anonymous, MREMAP_DONTUNMAP will cause the
mremap() call to fail. MREMAP_DONTUNMAP implies that MREMAP_FIXED is
also used. The final result is two equally sized VMAs where the
destination contains the PTEs of the source.
We hope to use this in Chrome OS where with userfaultfd we could write
an anonymous mapping to disk without having to STOP the process or worry
about VMA permission changes.
This feature also has a use case in Android, Lokesh Gidra has said
that "As part of using userfaultfd for GC, We'll have to move the physical
pages of the java heap to a separate location. For this purpose mremap
will be used. Without the MREMAP_DONTUNMAP flag, when I mremap the java
heap, its virtual mapping will be removed as well. Therefore, we'll
require performing mmap immediately after. This is not only time consuming
but also opens a time window where a native thread may call mmap and
reserve the java heap's address range for its own usage. This flag
solves the problem."
Signed-off-by: Brian Geffon <bgeffon-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
include/uapi/linux/mman.h | 5 +++--
mm/mremap.c | 37 ++++++++++++++++++++++++++++++-------
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
index fc1a64c3447b..923cc162609c 100644
--- a/include/uapi/linux/mman.h
+++ b/include/uapi/linux/mman.h
@@ -5,8 +5,9 @@
#include <asm/mman.h>
#include <asm-generic/hugetlb_encode.h>
-#define MREMAP_MAYMOVE 1
-#define MREMAP_FIXED 2
+#define MREMAP_MAYMOVE 1
+#define MREMAP_FIXED 2
+#define MREMAP_DONTUNMAP 4
#define OVERCOMMIT_GUESS 0
#define OVERCOMMIT_ALWAYS 1
diff --git a/mm/mremap.c b/mm/mremap.c
index 122938dcec15..bf97c3eb538b 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -318,8 +318,8 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
static unsigned long move_vma(struct vm_area_struct *vma,
unsigned long old_addr, unsigned long old_len,
unsigned long new_len, unsigned long new_addr,
- bool *locked, struct vm_userfaultfd_ctx *uf,
- struct list_head *uf_unmap)
+ bool *locked, unsigned long flags,
+ struct vm_userfaultfd_ctx *uf, struct list_head *uf_unmap)
{
struct mm_struct *mm = vma->vm_mm;
struct vm_area_struct *new_vma;
@@ -408,6 +408,13 @@ static unsigned long move_vma(struct vm_area_struct *vma,
if (unlikely(vma->vm_flags & VM_PFNMAP))
untrack_pfn_moved(vma);
+ if (unlikely(!err && (flags & MREMAP_DONTUNMAP))) {
+ if (vm_flags & VM_ACCOUNT)
+ vma->vm_flags |= VM_ACCOUNT;
+
+ goto out;
+ }
+
if (do_munmap(mm, old_addr, old_len, uf_unmap) < 0) {
/* OOM: unable to split vma, just get accounts right */
vm_unacct_memory(excess >> PAGE_SHIFT);
@@ -422,6 +429,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
vma->vm_next->vm_flags |= VM_ACCOUNT;
}
+out:
if (vm_flags & VM_LOCKED) {
mm->locked_vm += new_len >> PAGE_SHIFT;
*locked = true;
@@ -497,7 +505,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
unsigned long new_addr, unsigned long new_len, bool *locked,
- struct vm_userfaultfd_ctx *uf,
+ unsigned long flags, struct vm_userfaultfd_ctx *uf,
struct list_head *uf_unmap_early,
struct list_head *uf_unmap)
{
@@ -545,6 +553,17 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
old_len = new_len;
}
+ /*
+ * MREMAP_DONTUNMAP expands by old_len + (new_len - old_len), we will
+ * check that we can expand by old_len and vma_to_resize will handle
+ * the vma growing.
+ */
+ if (unlikely(flags & MREMAP_DONTUNMAP && !may_expand_vm(mm,
+ vma->vm_flags, old_len >> PAGE_SHIFT))) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
vma = vma_to_resize(addr, old_len, new_len, &charged);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
@@ -561,7 +580,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
if (IS_ERR_VALUE(ret))
goto out1;
- ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, uf,
+ ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
uf_unmap);
if (!(offset_in_page(ret)))
goto out;
@@ -609,12 +628,15 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
addr = untagged_addr(addr);
new_addr = untagged_addr(new_addr);
- if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
+ if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE | MREMAP_DONTUNMAP))
return ret;
if (flags & MREMAP_FIXED && !(flags & MREMAP_MAYMOVE))
return ret;
+ if (flags & MREMAP_DONTUNMAP && !(flags & MREMAP_FIXED))
+ return ret;
+
if (offset_in_page(addr))
return ret;
@@ -634,7 +656,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
if (flags & MREMAP_FIXED) {
ret = mremap_to(addr, old_len, new_addr, new_len,
- &locked, &uf, &uf_unmap_early, &uf_unmap);
+ &locked, flags, &uf, &uf_unmap_early,
+ &uf_unmap);
goto out;
}
@@ -712,7 +735,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
}
ret = move_vma(vma, addr, old_len, new_len, new_addr,
- &locked, &uf, &uf_unmap);
+ &locked, flags, &uf, &uf_unmap);
}
out:
if (offset_in_page(ret)) {
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* Re: [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap
From: Tycho Andersen @ 2020-01-24 18:03 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel,
christian.brauner
In-Reply-To: <20200124091743.3357-4-sargun@sargun.me>
On Fri, Jan 24, 2020 at 01:17:42AM -0800, Sargun Dhillon wrote:
> Currently, this just opens the group leader of the thread that triggere
> the event, as pidfds (currently) are limited to group leaders.
I don't love the semantics of this; when they're not limited to thread
group leaders any more, we won't be able to change this. Is that work
far off?
Tycho
^ permalink raw reply
* Re: [PATCH] Add prctl support for controlling mem reclaim V4
From: Mike Christie @ 2020-01-24 16:22 UTC (permalink / raw)
To: Shakeel Butt, Andrew Morton
Cc: linux-api, idryomov, Michal Hocko, david, Linux MM, LKML,
linux-scsi, linux-fsdevel, linux-block, martin, Damien.LeMoal,
Michal Hocko, Masato Suzuki
In-Reply-To: <CALvZod47XyD2x8TuZcb9PgeVY14JBwNhsUpN3RAeAt+RJJC=hg@mail.gmail.com>
On 12/05/2019 04:43 PM, Shakeel Butt wrote:
> On Mon, Nov 11, 2019 at 4:19 PM Mike Christie <mchristi@redhat.com> wrote:
>>
>> There are several storage drivers like dm-multipath, iscsi, tcmu-runner,
>> amd nbd that have userspace components that can run in the IO path. For
>> example, iscsi and nbd's userspace deamons may need to recreate a socket
>> and/or send IO on it, and dm-multipath's daemon multipathd may need to
>> send SG IO or read/write IO to figure out the state of paths and re-set
>> them up.
>>
>> In the kernel these drivers have access to GFP_NOIO/GFP_NOFS and the
>> memalloc_*_save/restore functions to control the allocation behavior,
>> but for userspace we would end up hitting an allocation that ended up
>> writing data back to the same device we are trying to allocate for.
>> The device is then in a state of deadlock, because to execute IO the
>> device needs to allocate memory, but to allocate memory the memory
>> layers want execute IO to the device.
>>
>> Here is an example with nbd using a local userspace daemon that performs
>> network IO to a remote server. We are using XFS on top of the nbd device,
>> but it can happen with any FS or other modules layered on top of the nbd
>> device that can write out data to free memory. Here a nbd daemon helper
>> thread, msgr-worker-1, is performing a write/sendmsg on a socket to execute
>> a request. This kicks off a reclaim operation which results in a WRITE to
>> the nbd device and the nbd thread calling back into the mm layer.
>>
>> [ 1626.609191] msgr-worker-1 D 0 1026 1 0x00004000
>> [ 1626.609193] Call Trace:
>> [ 1626.609195] ? __schedule+0x29b/0x630
>> [ 1626.609197] ? wait_for_completion+0xe0/0x170
>> [ 1626.609198] schedule+0x30/0xb0
>> [ 1626.609200] schedule_timeout+0x1f6/0x2f0
>> [ 1626.609202] ? blk_finish_plug+0x21/0x2e
>> [ 1626.609204] ? _xfs_buf_ioapply+0x2e6/0x410
>> [ 1626.609206] ? wait_for_completion+0xe0/0x170
>> [ 1626.609208] wait_for_completion+0x108/0x170
>> [ 1626.609210] ? wake_up_q+0x70/0x70
>> [ 1626.609212] ? __xfs_buf_submit+0x12e/0x250
>> [ 1626.609214] ? xfs_bwrite+0x25/0x60
>> [ 1626.609215] xfs_buf_iowait+0x22/0xf0
>> [ 1626.609218] __xfs_buf_submit+0x12e/0x250
>> [ 1626.609220] xfs_bwrite+0x25/0x60
>> [ 1626.609222] xfs_reclaim_inode+0x2e8/0x310
>> [ 1626.609224] xfs_reclaim_inodes_ag+0x1b6/0x300
>> [ 1626.609227] xfs_reclaim_inodes_nr+0x31/0x40
>> [ 1626.609228] super_cache_scan+0x152/0x1a0
>> [ 1626.609231] do_shrink_slab+0x12c/0x2d0
>> [ 1626.609233] shrink_slab+0x9c/0x2a0
>> [ 1626.609235] shrink_node+0xd7/0x470
>> [ 1626.609237] do_try_to_free_pages+0xbf/0x380
>> [ 1626.609240] try_to_free_pages+0xd9/0x1f0
>> [ 1626.609245] __alloc_pages_slowpath+0x3a4/0xd30
>> [ 1626.609251] ? ___slab_alloc+0x238/0x560
>> [ 1626.609254] __alloc_pages_nodemask+0x30c/0x350
>> [ 1626.609259] skb_page_frag_refill+0x97/0xd0
>> [ 1626.609274] sk_page_frag_refill+0x1d/0x80
>> [ 1626.609279] tcp_sendmsg_locked+0x2bb/0xdd0
>> [ 1626.609304] tcp_sendmsg+0x27/0x40
>> [ 1626.609307] sock_sendmsg+0x54/0x60
>> [ 1626.609308] ___sys_sendmsg+0x29f/0x320
>> [ 1626.609313] ? sock_poll+0x66/0xb0
>> [ 1626.609318] ? ep_item_poll.isra.15+0x40/0xc0
>> [ 1626.609320] ? ep_send_events_proc+0xe6/0x230
>> [ 1626.609322] ? hrtimer_try_to_cancel+0x54/0xf0
>> [ 1626.609324] ? ep_read_events_proc+0xc0/0xc0
>> [ 1626.609326] ? _raw_write_unlock_irq+0xa/0x20
>> [ 1626.609327] ? ep_scan_ready_list.constprop.19+0x218/0x230
>> [ 1626.609329] ? __hrtimer_init+0xb0/0xb0
>> [ 1626.609331] ? _raw_spin_unlock_irq+0xa/0x20
>> [ 1626.609334] ? ep_poll+0x26c/0x4a0
>> [ 1626.609337] ? tcp_tsq_write.part.54+0xa0/0xa0
>> [ 1626.609339] ? release_sock+0x43/0x90
>> [ 1626.609341] ? _raw_spin_unlock_bh+0xa/0x20
>> [ 1626.609342] __sys_sendmsg+0x47/0x80
>> [ 1626.609347] do_syscall_64+0x5f/0x1c0
>> [ 1626.609349] ? prepare_exit_to_usermode+0x75/0xa0
>> [ 1626.609351] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> This patch adds a new prctl command that daemons can use after they have
>> done their initial setup, and before they start to do allocations that
>> are in the IO path. It sets the PF_MEMALLOC_NOIO and PF_LESS_THROTTLE
>> flags so both userspace block and FS threads can use it to avoid the
>> allocation recursion and try to prevent from being throttled while
>> writing out data to free up memory.
>>
>> Signed-off-by: Mike Christie <mchristi@redhat.com>
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> Tested-by: Masato Suzuki <masato.suzuki@wdc.com>
>> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
>
> I suppose this patch should be routed through MM tree, so, CCing Andrew.
>
Andrew and other mm/storage developers,
Do I need to handle anything else for this patch, or are there any other
concerns? Is this maybe something we want to talk about at a quick LSF
session?
I have retested it with Linus's current tree. It still applies cleanly
(just some offsets), and fixes the problem described above we have been
hitting.
^ permalink raw reply
* [PATCH AUTOSEL 5.4 077/107] selftests: mlxsw: qos_mc_aware: Fix mausezahn invocation
From: Sasha Levin @ 2020-01-24 14:17 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Petr Machata, Amit Cohen, Ido Schimmel, David S . Miller,
Sasha Levin, linux-api
In-Reply-To: <20200124141817.28793-1-sashal@kernel.org>
From: Petr Machata <petrm@mellanox.com>
[ Upstream commit fef6d6704944c7be72fd2b77c021f1aed3d5df0d ]
Mausezahn does not recognize "own" as a keyword on source IP address. As a
result, the MC stream is not running at all, and therefore no UC
degradation can be observed even in principle.
Fix the invocation, and tighten the test: due to the minimum shaper
configured at the MC TCs, we always expect about 20% degradation. Fail the
test if it is lower.
Fixes: 573363a68f27 ("selftests: mlxsw: Add qos_lib.sh")
Signed-off-by: Petr Machata <petrm@mellanox.com>
Reported-by: Amit Cohen <amitc@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh b/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
index 47315fe48d5af..24dd8ed485802 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_mc_aware.sh
@@ -232,7 +232,7 @@ test_mc_aware()
stop_traffic
local ucth1=${uc_rate[1]}
- start_traffic $h1 own bc bc
+ start_traffic $h1 192.0.2.65 bc bc
local d0=$(date +%s)
local t0=$(ethtool_stats_get $h3 rx_octets_prio_0)
@@ -254,7 +254,11 @@ test_mc_aware()
ret = 100 * ($ucth1 - $ucth2) / $ucth1
if (ret > 0) { ret } else { 0 }
")
- check_err $(bc <<< "$deg > 25")
+
+ # Minimum shaper of 200Mbps on MC TCs should cause about 20% of
+ # degradation on 1Gbps link.
+ check_err $(bc <<< "$deg < 15") "Minimum shaper not in effect"
+ check_err $(bc <<< "$deg > 25") "MC traffic degrades UC performance too much"
local interval=$((d1 - d0))
local mc_ir=$(rate $u0 $u1 $interval)
--
2.20.1
^ permalink raw reply related
* Re: Proposal to fix pwrite with O_APPEND via pwritev2 flag
From: Rich Felker @ 2020-01-24 14:07 UTC (permalink / raw)
To: Florian Weimer; +Cc: linux-fsdevel, linux-kernel, linux-api, Alexander Viro
In-Reply-To: <87d0b942lp.fsf@oldenburg2.str.redhat.com>
On Fri, Jan 24, 2020 at 10:37:22AM +0100, Florian Weimer wrote:
> * Rich Felker:
>
> > There's a longstanding unfixable (due to API stability) bug in the
> > pwrite syscall:
> >
> > http://man7.org/linux/man-pages/man2/pwrite.2.html#BUGS
> >
> > whereby it wrongly honors O_APPEND if set, ignoring the caller-passed
> > offset. Now that there's a pwritev2 syscall that takes a flags
> > argument, it's possible to fix this without breaking stability by
> > adding a new RWF_NOAPPEND flag, which callers that want the fixed
> > behavior can then pass.
> >
> > I have a completely untested patch to add such a flag, but would like
> > to get a feel for whether the concept is acceptable before putting
> > time into testing it. If so, I'll submit this as a proper patch with
> > detailed commit message etc. Draft is below.
>
> Has this come up before?
I'm not sure if there's an open glibc bug for it or not, but it's come
up in musl community before that the kernel is non-conforming here for
historical reasons (preserving the original bug in case any software
is depending on it) and we've always wanted to have a fix, but
couldn't find one short of just erroring out if O_APPEND is set when
pwrite is called. That's what the fallback will do (rather than
silently write data at the wrong place) if pwritev2+RWF_NOAPPEND is
not supported on the system at runtime.
> I had already written a test case and it turns out that an O_APPEND
> descriptor does not protect the previously written data in the file:
>
> openat(AT_FDCWD, "/tmp/append-truncateuoRexJ", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
> write(3, "@", 1) = 1
> close(3) = 0
> openat(AT_FDCWD, "/tmp/append-truncateuoRexJ", O_WRONLY|O_APPEND) = 3
> ftruncate(3, 0) = 0
>
> So at least it looks like there is no security issue in adding a
> RWF_NOAPPEND flag.
Indeed, if you have the file open you can just use fcntl to remove
O_APPEND (but of course using that in an emulation would be racy), so
it's not a security boundary. Someone could try to "make it into one"
with seccomp, blocking fcntl that would remove O_APPEND and blocking
ftruncate, mmap, and all other ways you could modify the existing part
of the file, but that sounds fragile, and if they really want to do
that they can block pwritev2 as well (or at least block it with
RWF_NOAPPEND or future/unknown flags).
Rich
^ permalink raw reply
* Re: Proposal to fix pwrite with O_APPEND via pwritev2 flag
From: Florian Weimer @ 2020-01-24 9:37 UTC (permalink / raw)
To: Rich Felker; +Cc: linux-fsdevel, linux-kernel, linux-api, Alexander Viro
In-Reply-To: <20200124000243.GA12112@brightrain.aerifal.cx>
* Rich Felker:
> There's a longstanding unfixable (due to API stability) bug in the
> pwrite syscall:
>
> http://man7.org/linux/man-pages/man2/pwrite.2.html#BUGS
>
> whereby it wrongly honors O_APPEND if set, ignoring the caller-passed
> offset. Now that there's a pwritev2 syscall that takes a flags
> argument, it's possible to fix this without breaking stability by
> adding a new RWF_NOAPPEND flag, which callers that want the fixed
> behavior can then pass.
>
> I have a completely untested patch to add such a flag, but would like
> to get a feel for whether the concept is acceptable before putting
> time into testing it. If so, I'll submit this as a proper patch with
> detailed commit message etc. Draft is below.
Has this come up before?
I had already written a test case and it turns out that an O_APPEND
descriptor does not protect the previously written data in the file:
openat(AT_FDCWD, "/tmp/append-truncateuoRexJ", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
write(3, "@", 1) = 1
close(3) = 0
openat(AT_FDCWD, "/tmp/append-truncateuoRexJ", O_WRONLY|O_APPEND) = 3
ftruncate(3, 0) = 0
So at least it looks like there is no security issue in adding a
RWF_NOAPPEND flag.
Thanks,
Florian
^ permalink raw reply
* [PATCH 4/4] selftests/seccomp: test SECCOMP_USER_NOTIF_FLAG_PIDFD
From: Sargun Dhillon @ 2020-01-24 9:17 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: Sargun Dhillon, tycho-E0fblnxP3wo,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA
In-Reply-To: <20200124091743.3357-1-sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org>
This adds a test which uses the SECCOMP_USER_NOTIF_FLAG_PIDFD flag. It
does this by using sys_pidfd_send_signal to signal the process, and
then relies on traditional waitpid to ensure that the specific
signal was delivered.
Additionally, it verifies the case where the copy of the notification
to userspace fails, and the pidfd file is required to be freed.
Signed-off-by: Sargun Dhillon <sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 110 ++++++++++++++++++
1 file changed, 110 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index ee1b727ede04..ae9167ffbda9 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -187,6 +187,7 @@ struct seccomp_notif {
__u32 pid;
__u32 flags;
struct seccomp_data data;
+ __u32 pidfd;
};
struct seccomp_notif_resp {
@@ -212,6 +213,10 @@ struct seccomp_notif_sizes {
#define SECCOMP_USER_NOTIF_FLAG_CONTINUE 0x00000001
#endif
+#ifndef SECCOMP_USER_NOTIF_FLAG_PIDFD
+#define SECCOMP_USER_NOTIF_FLAG_PIDFD (1UL << 0)
+#endif
+
#ifndef seccomp
int seccomp(unsigned int op, unsigned int flags, void *args)
{
@@ -1871,6 +1876,7 @@ FIXTURE_TEARDOWN(TRACE_syscall)
free(self->prog.filter);
}
+
TEST_F(TRACE_syscall, ptrace_syscall_redirected)
{
/* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */
@@ -3612,6 +3618,110 @@ TEST(user_notification_continue)
}
}
+static int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
+ unsigned int flags)
+{
+#ifdef __NR_pidfd_send_signal
+ return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
+#else
+ errno = ENOSYS;
+ return -1;
+#endif
+}
+
+TEST(user_notification_pidfd)
+{
+ struct seccomp_notif req = {
+ .flags = SECCOMP_USER_NOTIF_FLAG_PIDFD,
+ };
+ struct seccomp_notif_resp resp = {};
+ int ret, listener, status;
+ pid_t pid;
+
+
+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+ ASSERT_EQ(0, ret) {
+ TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+ }
+
+ listener = user_trap_syscall(__NR_getppid,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ /* the process should be killed during this syscall */
+ syscall(__NR_getppid);
+ exit(0);
+ }
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+ ASSERT_GE(req.pidfd, 0);
+
+ ASSERT_EQ(sys_pidfd_send_signal(req.pidfd, SIGKILL, NULL, 0), 0) {
+ XFAIL(goto out,
+ "Kernel does not support pidfd_send_signal() syscall");
+ goto out;
+ }
+ EXPECT_EQ(req.pid, pid);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFSIGNALED(status));
+ EXPECT_EQ(SIGKILL, WTERMSIG(status));
+
+out:
+ close(req.pidfd);
+ close(listener);
+}
+
+TEST(user_notification_pidfd_fault)
+{
+ struct seccomp_notif req = {
+ .flags = SECCOMP_USER_NOTIF_FLAG_PIDFD,
+ };
+ struct seccomp_notif_resp resp = {};
+ int ret, listener, status;
+ pid_t pid;
+
+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+ ASSERT_EQ(0, ret) {
+ TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+ }
+
+ listener = user_trap_syscall(__NR_getppid,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0)
+ exit(syscall(__NR_getppid) != USER_NOTIF_MAGIC);
+
+ /* trigger an EFAULT */
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, NULL), -1);
+ EXPECT_EQ(errno, EFAULT);
+
+ /* Check that we can still fetch it. */
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+ EXPECT_EQ(req.pid, pid);
+
+ resp.id = req.id;
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0);
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+
+ close(req.pidfd);
+ close(listener);
+}
+
/*
* TODO:
* - add microbenchmarks
--
2.20.1
^ permalink raw reply related
* [PATCH 3/4] seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener trap
From: Sargun Dhillon @ 2020-01-24 9:17 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, christian.brauner
In-Reply-To: <20200124091743.3357-1-sargun@sargun.me>
This introduces the capability for users of seccomp's listener behaviour
to be able to receive the pidfd of the process that triggered the event.
Currently, this just opens the group leader of the thread that triggere
the event, as pidfds (currently) are limited to group leaders.
For actions which do not act on the process outside of the pidfd, there
is then no need to check the cookie to ensure validity of the request
throughout the listener's handling of it.
This can be extended later on as well when pidfd capabilities are added
to be able to have the listener imbue the pidfd with certain capabilities
when it is delivered to userspace.
It is the responsibility of the user to close the pidfd.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
include/uapi/linux/seccomp.h | 4 +++
kernel/seccomp.c | 68 ++++++++++++++++++++++++++++++++----
2 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index be84d87f1f46..64f6fc5c95f1 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -69,11 +69,15 @@ struct seccomp_notif_sizes {
__u16 seccomp_data;
};
+/* Valid flags for struct seccomp_notif */
+#define SECCOMP_USER_NOTIF_FLAG_PIDFD (1UL << 0) /* populate pidfd */
+
struct seccomp_notif {
__u64 id;
__u32 pid;
__u32 flags;
struct seccomp_data data;
+ __u32 pidfd;
};
/*
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index b6ea3dcb57bf..93f9cf45ce07 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1019,21 +1019,61 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
return 0;
}
+
+static long __seccomp_notify_recv_pidfd(void __user *buf,
+ struct seccomp_notif *unotif,
+ struct task_struct *group_leader)
+{
+ struct file *pidfd_file;
+ struct pid *pid;
+ int fd;
+
+ pid = get_task_pid(group_leader, PIDTYPE_PID);
+ pidfd_file = pidfd_create_file(pid);
+ put_pid(pid);
+ if (IS_ERR(pidfd_file))
+ return PTR_ERR(pidfd_file);
+
+ fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
+ if (fd < 0) {
+ fput(pidfd_file);
+ return fd;
+ }
+
+ unotif->pidfd = fd;
+
+ if (copy_to_user(buf, unotif, sizeof(*unotif))) {
+ put_unused_fd(fd);
+ fput(pidfd_file);
+ return -EFAULT;
+ }
+
+ fd_install(fd, pidfd_file);
+
+ return 0;
+}
+
static long seccomp_notify_recv(struct seccomp_filter *filter,
void __user *buf)
{
struct seccomp_knotif *knotif = NULL, *cur;
struct seccomp_notif unotif;
+ struct task_struct *group_leader;
+ bool send_pidfd;
ssize_t ret;
+ if (copy_from_user(&unotif, buf, sizeof(unotif)))
+ return -EFAULT;
/* Verify that we're not given garbage to keep struct extensible. */
- ret = check_zeroed_user(buf, sizeof(unotif));
- if (ret < 0)
- return ret;
- if (!ret)
+ if (unotif.id ||
+ unotif.pid ||
+ memchr_inv(&unotif.data, 0, sizeof(unotif.data)) ||
+ unotif.pidfd)
+ return -EINVAL;
+ if (unotif.flags & ~(SECCOMP_USER_NOTIF_FLAG_PIDFD))
return -EINVAL;
- memset(&unotif, 0, sizeof(unotif));
+ send_pidfd = unotif.flags & SECCOMP_USER_NOTIF_FLAG_PIDFD;
ret = down_interruptible(&filter->notif->request);
if (ret < 0)
@@ -1057,9 +1097,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
goto out;
}
+ memset(&unotif, 0, sizeof(unotif));
+
unotif.id = knotif->id;
unotif.pid = task_pid_vnr(knotif->task);
unotif.data = *(knotif->data);
+ if (send_pidfd)
+ group_leader = get_task_struct(knotif->task->group_leader);
knotif->state = SECCOMP_NOTIFY_SENT;
wake_up_poll(&filter->notif->wqh, EPOLLOUT | EPOLLWRNORM);
@@ -1067,9 +1111,21 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
out:
mutex_unlock(&filter->notify_lock);
- if (ret == 0 && copy_to_user(buf, &unotif, sizeof(unotif))) {
+ if (ret)
+ return ret;
+
+ /*
+ * We've successfully received a notification, let's try to copy it to
+ * userspace.
+ */
+ if (send_pidfd) {
+ ret = __seccomp_notify_recv_pidfd(buf, &unotif, group_leader);
+ put_task_struct(group_leader);
+ } else if (copy_to_user(buf, &unotif, sizeof(unotif))) {
ret = -EFAULT;
+ }
+ if (ret) {
/*
* Userspace screwed up. To make sure that we keep this
* notification alive, let's reset it back to INIT. It
--
2.20.1
^ permalink raw reply related
* [PATCH 2/4] fork: Use newly created pidfd_create_file helper
From: Sargun Dhillon @ 2020-01-24 9:17 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, christian.brauner
In-Reply-To: <20200124091743.3357-1-sargun@sargun.me>
Rather than duplicating the code to create a pidfd_file in kernel/fork.c,
use the helper in kernel/pid.c.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
kernel/fork.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 080809560072..181ab2958cad 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2106,14 +2106,12 @@ static __latent_entropy struct task_struct *copy_process(
pidfd = retval;
- pidfile = anon_inode_getfile("[pidfd]", &pidfd_fops, pid,
- O_RDWR | O_CLOEXEC);
+ pidfile = pidfd_create_file(pid);
if (IS_ERR(pidfile)) {
put_unused_fd(pidfd);
retval = PTR_ERR(pidfile);
goto bad_fork_free_pid;
}
- get_pid(pid); /* held by pidfile now */
retval = put_user(pidfd, args->pidfd);
if (retval)
--
2.20.1
^ permalink raw reply related
* [PATCH 1/4] pid: Add pidfd_create_file helper
From: Sargun Dhillon @ 2020-01-24 9:17 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, christian.brauner
In-Reply-To: <20200124091743.3357-1-sargun@sargun.me>
This helper allow for creation of pidfd files. The existing helper
(pidfd_create) creates file descriptors directly, which cannot
be used without race conditions when there is an intermediate
step between creation, and informing userspace the fd has been
created.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
include/linux/pid.h | 1 +
kernel/pid.c | 22 ++++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 998ae7d24450..70d4725cf8da 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
struct file;
extern struct pid *pidfd_pid(const struct file *file);
+extern struct file *pidfd_create_file(struct pid *pid);
static inline struct pid *get_pid(struct pid *pid)
{
diff --git a/kernel/pid.c b/kernel/pid.c
index 2278e249141d..2a34db290128 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -521,6 +521,28 @@ static int pidfd_create(struct pid *pid)
return fd;
}
+/**
+ * pidfd_create_file() - Create a new pidfd file.
+ *
+ * @pid: struct pid that the pidfd will reference
+ *
+ * This creates a new pidfd file.
+ *
+ * Return: On success, a cloexec pidfd file is returned
+ * On error, an err ptr will be returned.
+ */
+struct file *pidfd_create_file(struct pid *pid)
+{
+ struct file *f;
+
+ f = anon_inode_getfile("[pidfd]", &pidfd_fops, get_pid(pid),
+ O_RDWR | O_CLOEXEC);
+ if (IS_ERR(f))
+ put_pid(pid);
+
+ return f;
+}
+
/**
* pidfd_open() - Open new pid file descriptor.
*
--
2.20.1
^ permalink raw reply related
* [PATCH 0/4] Add the ability to get a pidfd on seccomp user notifications
From: Sargun Dhillon @ 2020-01-24 9:17 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: Sargun Dhillon, tycho-E0fblnxP3wo,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA
This patchset adds the ability for users of the seccomp user notification API
to receive the pidfd of the process which triggered the notification. It is
an optional feature that users must opt into by setting a flag when they
call the ioctl. With enhancements to other APIs, it should decrease
the need for the cookie-checking mechanism.
Sargun Dhillon (4):
pid: Add pidfd_create_file helper
fork: Use newly created pidfd_create_file helper
seccomp: Add SECCOMP_USER_NOTIF_FLAG_PIDFD to get pidfd on listener
trap
selftests/seccomp: test SECCOMP_USER_NOTIF_FLAG_PIDFD
include/linux/pid.h | 1 +
include/uapi/linux/seccomp.h | 4 +
kernel/fork.c | 4 +-
kernel/pid.c | 22 ++++
kernel/seccomp.c | 68 ++++++++++-
tools/testing/selftests/seccomp/seccomp_bpf.c | 110 ++++++++++++++++++
6 files changed, 200 insertions(+), 9 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH AUTOSEL 5.4 04/33] libbpf: Fix BTF-defined map's __type macro handling of arrays
From: Sasha Levin @ 2020-01-24 1:16 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Andrii Nakryiko, Alexei Starovoitov, Sasha Levin, linux-api
In-Reply-To: <20200124011708.18232-1-sashal@kernel.org>
From: Andrii Nakryiko <andriin@fb.com>
[ Upstream commit a53ba15d81995868651dd28a85d8045aef3d4e20 ]
Due to a quirky C syntax of declaring pointers to array or function
prototype, existing __type() macro doesn't work with map key/value types
that are array or function prototype. One has to create a typedef first
and use it to specify key/value type for a BPF map. By using typeof(),
pointer to type is now handled uniformly for all kinds of types. Convert
one of self-tests as a demonstration.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20191004040211.2434033-1-andriin@fb.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
tools/testing/selftests/bpf/bpf_helpers.h | 2 +-
tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 54a50699bbfda..9f77cbaac01c1 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -3,7 +3,7 @@
#define __BPF_HELPERS__
#define __uint(name, val) int (*name)[val]
-#define __type(name, val) val *name
+#define __type(name, val) typeof(val) *name
/* helper macro to print out debug messages */
#define bpf_printk(fmt, ...) \
diff --git a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
index f8ffa3f3d44bb..6cc4479ac9df6 100644
--- a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
+++ b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
@@ -47,12 +47,11 @@ struct {
* issue and avoid complicated C programming massaging.
* This is an acceptable workaround since there is one entry here.
*/
-typedef __u64 raw_stack_trace_t[2 * MAX_STACK_RAWTP];
struct {
__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
__uint(max_entries, 1);
__type(key, __u32);
- __type(value, raw_stack_trace_t);
+ __type(value, __u64[2 * MAX_STACK_RAWTP]);
} rawdata_map SEC(".maps");
SEC("raw_tracepoint/sys_enter")
--
2.20.1
^ permalink raw reply related
* Proposal to fix pwrite with O_APPEND via pwritev2 flag
From: Rich Felker @ 2020-01-24 0:02 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, linux-api; +Cc: Alexander Viro
There's a longstanding unfixable (due to API stability) bug in the
pwrite syscall:
http://man7.org/linux/man-pages/man2/pwrite.2.html#BUGS
whereby it wrongly honors O_APPEND if set, ignoring the caller-passed
offset. Now that there's a pwritev2 syscall that takes a flags
argument, it's possible to fix this without breaking stability by
adding a new RWF_NOAPPEND flag, which callers that want the fixed
behavior can then pass.
I have a completely untested patch to add such a flag, but would like
to get a feel for whether the concept is acceptable before putting
time into testing it. If so, I'll submit this as a proper patch with
detailed commit message etc. Draft is below.
Rich
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..3a769a972f79 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3397,6 +3397,8 @@ static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags)
{
if (unlikely(flags & ~RWF_SUPPORTED))
return -EOPNOTSUPP;
+ if (unlikely((flags & RWF_APPEND) && (flags & RWF_NOAPPEND)))
+ return -EINVAL;
if (flags & RWF_NOWAIT) {
if (!(ki->ki_filp->f_mode & FMODE_NOWAIT))
@@ -3411,6 +3413,8 @@ static inline int kiocb_set_rw_flags(struct kiocb *ki, rwf_t flags)
ki->ki_flags |= (IOCB_DSYNC | IOCB_SYNC);
if (flags & RWF_APPEND)
ki->ki_flags |= IOCB_APPEND;
+ if (flags & RWF_NOAPPEND)
+ ki->ki_flags &= ~IOCB_APPEND;
return 0;
}
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 379a612f8f1d..591357d9b3c9 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -299,8 +299,11 @@ typedef int __bitwise __kernel_rwf_t;
/* per-IO O_APPEND */
#define RWF_APPEND ((__force __kernel_rwf_t)0x00000010)
+/* per-IO negation of O_APPEND */
+#define RWF_NOAPPEND ((__force __kernel_rwf_t)0x00000020)
+
/* mask of flags supported by the kernel */
#define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\
- RWF_APPEND)
+ RWF_APPEND | RWF_NOAPPEND)
#endif /* _UAPI_LINUX_FS_H */
^ permalink raw reply related
* Re: [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting
From: Paul Moore @ 2020-01-23 21:47 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <20200123210240.sq64tptjm3ds7xss@madcap2.tricolour.ca>
On Thu, Jan 23, 2020 at 4:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-01-22 16:29, Paul Moore wrote:
> > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > Require the target task to be a descendant of the container
> > > orchestrator/engine.
> > >
> > > You would only change the audit container ID from one set or inherited
> > > value to another if you were nesting containers.
> > >
> > > If changing the contid, the container orchestrator/engine must be a
> > > descendant and not same orchestrator as the one that set it so it is not
> > > possible to change the contid of another orchestrator's container.
> > >
> > > Since the task_is_descendant() function is used in YAMA and in audit,
> > > remove the duplication and pull the function into kernel/core/sched.c
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/sched.h | 3 +++
> > > kernel/audit.c | 44 ++++++++++++++++++++++++++++++++++++--------
> > > kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++
> > > security/yama/yama_lsm.c | 33 ---------------------------------
> > > 4 files changed, 72 insertions(+), 41 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index f7a8d3288ca0..ef8e07524c46 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -2603,22 +2610,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > > oldcontid = audit_get_contid(task);
> > > read_lock(&tasklist_lock);
> > > /* Don't allow the contid to be unset */
> > > - if (!audit_contid_valid(contid))
> > > + if (!audit_contid_valid(contid)) {
> > > rc = -EINVAL;
> > > + goto unlock;
> > > + }
> > > /* Don't allow the contid to be set to the same value again */
> > > - else if (contid == oldcontid) {
> > > + if (contid == oldcontid) {
> > > rc = -EADDRINUSE;
> > > + goto unlock;
> > > + }
> > > /* if we don't have caps, reject */
> > > - else if (!capable(CAP_AUDIT_CONTROL))
> > > + if (!capable(CAP_AUDIT_CONTROL)) {
> > > rc = -EPERM;
> > > - /* if task has children or is not single-threaded, deny */
> > > - else if (!list_empty(&task->children))
> > > + goto unlock;
> > > + }
> > > + /* if task has children, deny */
> > > + if (!list_empty(&task->children)) {
> > > rc = -EBUSY;
> > > - else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > > + goto unlock;
> > > + }
> > > + /* if task is not single-threaded, deny */
> > > + if (!(thread_group_leader(task) && thread_group_empty(task))) {
> > > rc = -EALREADY;
> > > - /* if contid is already set, deny */
> > > - else if (audit_contid_set(task))
> > > + goto unlock;
> > > + }
> >
> > It seems like the if/else-if conversion above should be part of an
> > earlier patchset.
>
> I had considered that, but it wasn't obvious where that conversion
> should happen since it wasn't necessary earlier and is now. I can move
> it earlier if you feel strongly about it.
Not particularly.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Paul Moore @ 2020-01-23 21:35 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, mpatel, Serge Hallyn
In-Reply-To: <20200123200412.j2aucdp3cvk57prw@madcap2.tricolour.ca>
On Thu, Jan 23, 2020 at 3:04 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-01-23 12:09, Paul Moore wrote:
> > On Thu, Jan 23, 2020 at 11:29 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-01-22 16:28, Paul Moore wrote:
> > > > On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > >
> > > > > Add audit container identifier support to the action of signalling the
> > > > > audit daemon.
> > > > >
> > > > > Since this would need to add an element to the audit_sig_info struct,
> > > > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > > > audit_sig_info2 struct. Corresponding support is required in the
> > > > > userspace code to reflect the new record request and reply type.
> > > > > An older userspace won't break since it won't know to request this
> > > > > record type.
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > include/linux/audit.h | 7 +++++++
> > > > > include/uapi/linux/audit.h | 1 +
> > > > > kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> > > > > kernel/audit.h | 1 +
> > > > > security/selinux/nlmsgtab.c | 1 +
> > > > > 5 files changed, 45 insertions(+)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 0871c3e5d6df..51159c94041c 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -126,6 +126,14 @@ struct auditd_connection {
> > > > > kuid_t audit_sig_uid = INVALID_UID;
> > > > > pid_t audit_sig_pid = -1;
> > > > > u32 audit_sig_sid = 0;
> > > > > +/* Since the signal information is stored in the record buffer at the
> > > > > + * time of the signal, but not retrieved until later, there is a chance
> > > > > + * that the last process in the container could terminate before the
> > > > > + * signal record is delivered. In this circumstance, there is a chance
> > > > > + * the orchestrator could reuse the audit container identifier, causing
> > > > > + * an overlap of audit records that refer to the same audit container
> > > > > + * identifier, but a different container instance. */
> > > > > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> > > >
> > > > I believe we could prevent the case mentioned above by taking an
> > > > additional reference to the audit container ID object when the signal
> > > > information is collected, dropping it only after the signal
> > > > information is collected by userspace or another process signals the
> > > > audit daemon. Yes, it would block that audit container ID from being
> > > > reused immediately, but since we are talking about one number out of
> > > > 2^64 that seems like a reasonable tradeoff.
> > >
> > > I had thought that through and should have been more explicit about that
> > > situation when I documented it. We could do that, but then the syscall
> > > records would be connected with the call from auditd on shutdown to
> > > request that signal information, rather than the exit of that last
> > > process that was using that container. This strikes me as misleading.
> > > Is that really what we want?
> >
> > ???
> >
> > I think one of us is not understanding the other; maybe it's me, maybe
> > it's you, maybe it's both of us.
> >
> > Anyway, here is what I was trying to convey with my original comment
> > ... When we record the audit container ID in audit_signal_info() we
> > take an extra reference to the audit container ID object so that it
> > will not disappear (and get reused) until after we respond with an
> > AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the
> > AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in
> > audit_signal_info(). Unless I'm missing some other change you made,
> > this *shouldn't* affect the syscall records, all it does is preserve
> > the audit container ID object in the kernel's ACID store so it doesn't
> > get reused.
>
> This is exactly what I had understood. I hadn't considered the extra
> details below in detail due to my original syscall concern, but they
> make sense.
>
> The syscall I refer to is the one connected with the drop of the
> audit container identifier by the last process that was in that
> container in patch 5/16. The production of this record is contingent on
> the last ref in a contobj being dropped. So if it is due to that ref
> being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2
> record it fetched, then it will appear that the fetch action closed the
> container rather than the last process in the container to exit.
>
> Does this make sense?
More so than your original reply, at least to me anyway.
It makes sense that the audit container ID wouldn't be marked as
"dead" since it would still be very much alive and available for use
by the orchestrator, the question is if that is desirable or not. I
think the answer to this comes down the preserving the correctness of
the audit log.
If the audit container ID reported by AUDIT_SIGNAL_INFO2 has been
reused then I think there is a legitimate concern that the audit log
is not correct, and could be misleading. If we solve that by grabbing
an extra reference, then there could also be some confusion as
userspace considers a container to be "dead" while the audit container
ID still exists in the kernel, and the kernel generated audit
container ID death record will not be generated until much later (and
possibly be associated with a different event, but that could be
solved by unassociating the container death record). Of the two
approaches, I think the latter is safer in that it preserves the
correctness of the audit log, even though it could result in a delay
of the container death record.
Neither way is perfect, so if you have any other ideas I'm all ears.
> > (We do need to do some extra housekeeping in audit_signal_info() to
> > deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 -
> > basically if audit_sig_cid is not NULL we should drop a reference
> > before assigning it a new object pointer, and of course we would need
> > to set audit_sig_cid to NULL in audit_receive_msg() after sending it
> > up to userspace and dropping the extra ref.)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting
From: Richard Guy Briggs @ 2020-01-23 21:02 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <CAHC9VhT1+mx_tVzyXD=UBqagqYgAFjZ=X1A6oBiMvjVCn8=V-w@mail.gmail.com>
On 2020-01-22 16:29, Paul Moore wrote:
> On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Require the target task to be a descendant of the container
> > orchestrator/engine.
> >
> > You would only change the audit container ID from one set or inherited
> > value to another if you were nesting containers.
> >
> > If changing the contid, the container orchestrator/engine must be a
> > descendant and not same orchestrator as the one that set it so it is not
> > possible to change the contid of another orchestrator's container.
> >
> > Since the task_is_descendant() function is used in YAMA and in audit,
> > remove the duplication and pull the function into kernel/core/sched.c
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/sched.h | 3 +++
> > kernel/audit.c | 44 ++++++++++++++++++++++++++++++++++++--------
> > kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++
> > security/yama/yama_lsm.c | 33 ---------------------------------
> > 4 files changed, 72 insertions(+), 41 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f7a8d3288ca0..ef8e07524c46 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2603,22 +2610,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > oldcontid = audit_get_contid(task);
> > read_lock(&tasklist_lock);
> > /* Don't allow the contid to be unset */
> > - if (!audit_contid_valid(contid))
> > + if (!audit_contid_valid(contid)) {
> > rc = -EINVAL;
> > + goto unlock;
> > + }
> > /* Don't allow the contid to be set to the same value again */
> > - else if (contid == oldcontid) {
> > + if (contid == oldcontid) {
> > rc = -EADDRINUSE;
> > + goto unlock;
> > + }
> > /* if we don't have caps, reject */
> > - else if (!capable(CAP_AUDIT_CONTROL))
> > + if (!capable(CAP_AUDIT_CONTROL)) {
> > rc = -EPERM;
> > - /* if task has children or is not single-threaded, deny */
> > - else if (!list_empty(&task->children))
> > + goto unlock;
> > + }
> > + /* if task has children, deny */
> > + if (!list_empty(&task->children)) {
> > rc = -EBUSY;
> > - else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > + goto unlock;
> > + }
> > + /* if task is not single-threaded, deny */
> > + if (!(thread_group_leader(task) && thread_group_empty(task))) {
> > rc = -EALREADY;
> > - /* if contid is already set, deny */
> > - else if (audit_contid_set(task))
> > + goto unlock;
> > + }
>
> It seems like the if/else-if conversion above should be part of an
> earlier patchset.
I had considered that, but it wasn't obvious where that conversion
should happen since it wasn't necessary earlier and is now. I can move
it earlier if you feel strongly about it.
> > + /* if task is not descendant, block */
> > + if (task == current) {
> > + rc = -EBADSLT;
> > + goto unlock;
> > + }
> > + if (!task_is_descendant(current, task)) {
> > + rc = -EXDEV;
> > + goto unlock;
> > + }
>
> I understand you are trying to provide a unique error code for each
> failure case, but this is getting silly. Let's group the descendent
> checks under the same error code.
Ok. I was trying to provide more information for debugging for me and
for users.
> > + /* only allow contid setting again if nesting */
> > + if (audit_contid_set(task) && audit_contid_isowner(task))
> > rc = -ECHILD;
>
> Should that be "!audit_contid_isowner()"?
No. If the contid is already set on this task and if it is the same
orchestrator that already owns this one, then block it since the same
orchestrator is not allowed to set it again. Another orchestrator that
has been shown by previous tests to be a descendant of the orchestrator
that already owns this one would be permitted.
Now that I say this explicitly, it appears I need another test to check:
/* only allow contid setting again if nesting */
if (audit_contid_set(task) && ( audit_contid_isowner(task) || !task_is_descendant(_audit_contobj(task)->owner, current) ))
rc = -ECHILD;
So we're back to audit_contobj_owner() like in the previous patchset
that would make this cleaner.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Richard Guy Briggs @ 2020-01-23 20:04 UTC (permalink / raw)
To: Paul Moore
Cc: nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, mpatel, Serge Hallyn
In-Reply-To: <CAHC9VhTusiQoudB8G5jjDFyM9WxBUAjZ6_X35ywJ063Jb75dQA@mail.gmail.com>
On 2020-01-23 12:09, Paul Moore wrote:
> On Thu, Jan 23, 2020 at 11:29 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-01-22 16:28, Paul Moore wrote:
> > > On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > Add audit container identifier support to the action of signalling the
> > > > audit daemon.
> > > >
> > > > Since this would need to add an element to the audit_sig_info struct,
> > > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > > audit_sig_info2 struct. Corresponding support is required in the
> > > > userspace code to reflect the new record request and reply type.
> > > > An older userspace won't break since it won't know to request this
> > > > record type.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 7 +++++++
> > > > include/uapi/linux/audit.h | 1 +
> > > > kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> > > > kernel/audit.h | 1 +
> > > > security/selinux/nlmsgtab.c | 1 +
> > > > 5 files changed, 45 insertions(+)
> > >
> > > ...
> > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 0871c3e5d6df..51159c94041c 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -126,6 +126,14 @@ struct auditd_connection {
> > > > kuid_t audit_sig_uid = INVALID_UID;
> > > > pid_t audit_sig_pid = -1;
> > > > u32 audit_sig_sid = 0;
> > > > +/* Since the signal information is stored in the record buffer at the
> > > > + * time of the signal, but not retrieved until later, there is a chance
> > > > + * that the last process in the container could terminate before the
> > > > + * signal record is delivered. In this circumstance, there is a chance
> > > > + * the orchestrator could reuse the audit container identifier, causing
> > > > + * an overlap of audit records that refer to the same audit container
> > > > + * identifier, but a different container instance. */
> > > > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> > >
> > > I believe we could prevent the case mentioned above by taking an
> > > additional reference to the audit container ID object when the signal
> > > information is collected, dropping it only after the signal
> > > information is collected by userspace or another process signals the
> > > audit daemon. Yes, it would block that audit container ID from being
> > > reused immediately, but since we are talking about one number out of
> > > 2^64 that seems like a reasonable tradeoff.
> >
> > I had thought that through and should have been more explicit about that
> > situation when I documented it. We could do that, but then the syscall
> > records would be connected with the call from auditd on shutdown to
> > request that signal information, rather than the exit of that last
> > process that was using that container. This strikes me as misleading.
> > Is that really what we want?
>
> ???
>
> I think one of us is not understanding the other; maybe it's me, maybe
> it's you, maybe it's both of us.
>
> Anyway, here is what I was trying to convey with my original comment
> ... When we record the audit container ID in audit_signal_info() we
> take an extra reference to the audit container ID object so that it
> will not disappear (and get reused) until after we respond with an
> AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the
> AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in
> audit_signal_info(). Unless I'm missing some other change you made,
> this *shouldn't* affect the syscall records, all it does is preserve
> the audit container ID object in the kernel's ACID store so it doesn't
> get reused.
This is exactly what I had understood. I hadn't considered the extra
details below in detail due to my original syscall concern, but they
make sense.
The syscall I refer to is the one connected with the drop of the
audit container identifier by the last process that was in that
container in patch 5/16. The production of this record is contingent on
the last ref in a contobj being dropped. So if it is due to that ref
being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2
record it fetched, then it will appear that the fetch action closed the
container rather than the last process in the container to exit.
Does this make sense?
> (We do need to do some extra housekeeping in audit_signal_info() to
> deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 -
> basically if audit_sig_cid is not NULL we should drop a reference
> before assigning it a new object pointer, and of course we would need
> to set audit_sig_cid to NULL in audit_receive_msg() after sending it
> up to userspace and dropping the extra ref.)
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] mm: Add MREMAP_DONTUNMAP to mremap().
From: Brian Geffon @ 2020-01-23 19:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-mm, Andrew Morton, Michael S . Tsirkin, Arnd Bergmann,
Sonny Rao, Minchan Kim, Joel Fernandes, Lokesh Gidra, LKML,
linux-api, Yu Zhao, Jesse Barnes
In-Reply-To: <CD5EA896-7364-40E2-8709-A014973FFBC8@amacapital.net>
Andy,
Thanks, yes, that's a much clearer description of the feature. I'll
make sure to update the description with subsequent patches and with
later man page updates.
Brian
On Wed, Jan 22, 2020 at 7:02 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Jan 22, 2020, at 5:46 PM, Brian Geffon <bgeffon@google.com> wrote:
> >
> > MREMAP_DONTUNMAP is an additional flag that can be used with
> > MREMAP_FIXED to move a mapping to a new address. Normally, mremap(2)
> > would then tear down the old vma so subsequent accesses to the vma
> > cause a segfault. However, with this new flag it will keep the old
> > vma with zapping PTEs so any access to the old VMA after that point
> > will result in a pagefault.
>
> This needs a vastly better description. Perhaps:
>
> When remapping an anonymous, private mapping, if MREMAP_DONTUNMAP is set, the source mapping will not be removed. Instead it will be cleared as if a brand new anonymous, private mapping had been created atomically as part of the mremap() call. If a userfaultfd was watching the source, it will continue to watch the new mapping. For a mapping that is shared or not anonymous, MREMAP_DONTUNMAP will cause the mremap() call to fail.
>
> Or is it something else?
>
> >
> > This feature will find a use in ChromeOS along with userfaultfd.
> > Specifically we will want to register a VMA with userfaultfd and then
> > pull it out from under a running process. By using MREMAP_DONTUNMAP we
> > don't have to worry about mprotecting and then potentially racing with
> > VMA permission changes from a running process.
>
> Does this mean you yank it out but you want to replace it simultaneously?
>
> >
> > This feature also has a use case in Android, Lokesh Gidra has said
> > that "As part of using userfaultfd for GC, We'll have to move the physical
> > pages of the java heap to a separate location. For this purpose mremap
> > will be used. Without the MREMAP_DONTUNMAP flag, when I mremap the java
> > heap, its virtual mapping will be removed as well. Therefore, we'll
> > require performing mmap immediately after. This is not only time consuming
> > but also opens a time window where a native thread may call mmap and
> > reserve the java heap's address range for its own usage. This flag
> > solves the problem."
>
> Cute.
^ permalink raw reply
* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Paul Moore @ 2020-01-23 17:09 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <20200123162918.b3jbed7tbvr2sf2p@madcap2.tricolour.ca>
On Thu, Jan 23, 2020 at 11:29 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-01-22 16:28, Paul Moore wrote:
> > On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > Add audit container identifier support to the action of signalling the
> > > audit daemon.
> > >
> > > Since this would need to add an element to the audit_sig_info struct,
> > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > audit_sig_info2 struct. Corresponding support is required in the
> > > userspace code to reflect the new record request and reply type.
> > > An older userspace won't break since it won't know to request this
> > > record type.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/audit.h | 7 +++++++
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> > > kernel/audit.h | 1 +
> > > security/selinux/nlmsgtab.c | 1 +
> > > 5 files changed, 45 insertions(+)
> >
> > ...
> >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 0871c3e5d6df..51159c94041c 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -126,6 +126,14 @@ struct auditd_connection {
> > > kuid_t audit_sig_uid = INVALID_UID;
> > > pid_t audit_sig_pid = -1;
> > > u32 audit_sig_sid = 0;
> > > +/* Since the signal information is stored in the record buffer at the
> > > + * time of the signal, but not retrieved until later, there is a chance
> > > + * that the last process in the container could terminate before the
> > > + * signal record is delivered. In this circumstance, there is a chance
> > > + * the orchestrator could reuse the audit container identifier, causing
> > > + * an overlap of audit records that refer to the same audit container
> > > + * identifier, but a different container instance. */
> > > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> >
> > I believe we could prevent the case mentioned above by taking an
> > additional reference to the audit container ID object when the signal
> > information is collected, dropping it only after the signal
> > information is collected by userspace or another process signals the
> > audit daemon. Yes, it would block that audit container ID from being
> > reused immediately, but since we are talking about one number out of
> > 2^64 that seems like a reasonable tradeoff.
>
> I had thought that through and should have been more explicit about that
> situation when I documented it. We could do that, but then the syscall
> records would be connected with the call from auditd on shutdown to
> request that signal information, rather than the exit of that last
> process that was using that container. This strikes me as misleading.
> Is that really what we want?
???
I think one of us is not understanding the other; maybe it's me, maybe
it's you, maybe it's both of us.
Anyway, here is what I was trying to convey with my original comment
... When we record the audit container ID in audit_signal_info() we
take an extra reference to the audit container ID object so that it
will not disappear (and get reused) until after we respond with an
AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the
AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in
audit_signal_info(). Unless I'm missing some other change you made,
this *shouldn't* affect the syscall records, all it does is preserve
the audit container ID object in the kernel's ACID store so it doesn't
get reused.
(We do need to do some extra housekeeping in audit_signal_info() to
deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 -
basically if audit_sig_cid is not NULL we should drop a reference
before assigning it a new object pointer, and of course we would need
to set audit_sig_cid to NULL in audit_receive_msg() after sending it
up to userspace and dropping the extra ref.)
--
paul moore
www.paul-moore.com
^ 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