* [PATCH v4 3/6] shm: add memfd_create() syscall
From: David Herrmann @ 2014-07-20 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Michael Kerrisk, Ryan Lortie, Linus Torvalds, Andrew Morton,
linux-mm, linux-fsdevel, linux-api, Greg Kroah-Hartman,
john.stultz, Lennart Poettering, Daniel Mack, Kay Sievers,
Hugh Dickins, Andy Lutomirski, Alexander Viro, David Herrmann
In-Reply-To: <1405877680-999-1-git-send-email-dh.herrmann@gmail.com>
memfd_create() is similar to mmap(MAP_ANON), but returns a file-descriptor
that you can pass to mmap(). It can support sealing and avoids any
connection to user-visible mount-points. Thus, it's not subject to quotas
on mounted file-systems, but can be used like malloc()'ed memory, but
with a file-descriptor to it.
memfd_create() returns the raw shmem file, so calls like ftruncate() can
be used to modify the underlying inode. Also calls like fstat()
will return proper information and mark the file as regular file. If you
want sealing, you can specify MFD_ALLOW_SEALING. Otherwise, sealing is not
supported (like on all other regular files).
Compared to O_TMPFILE, it does not require a tmpfs mount-point and is not
subject to a filesystem size limit. It is still properly accounted to
memcg limits, though, and to the same overcommit or no-overcommit
accounting as all user memory.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
arch/x86/syscalls/syscall_32.tbl | 1 +
arch/x86/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 1 +
include/uapi/linux/memfd.h | 8 +++++
kernel/sys_ni.c | 1 +
mm/shmem.c | 73 ++++++++++++++++++++++++++++++++++++++++
6 files changed, 85 insertions(+)
create mode 100644 include/uapi/linux/memfd.h
diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index d6b8679..e7495b4 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -360,3 +360,4 @@
351 i386 sched_setattr sys_sched_setattr
352 i386 sched_getattr sys_sched_getattr
353 i386 renameat2 sys_renameat2
+354 i386 memfd_create sys_memfd_create
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index ec255a1..28be0e1 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -323,6 +323,7 @@
314 common sched_setattr sys_sched_setattr
315 common sched_getattr sys_sched_getattr
316 common renameat2 sys_renameat2
+317 common memfd_create sys_memfd_create
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b0881a0..de00585 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -802,6 +802,7 @@ asmlinkage long sys_timerfd_settime(int ufd, int flags,
asmlinkage long sys_timerfd_gettime(int ufd, struct itimerspec __user *otmr);
asmlinkage long sys_eventfd(unsigned int count);
asmlinkage long sys_eventfd2(unsigned int count, int flags);
+asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags);
asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
asmlinkage long sys_old_readdir(unsigned int, struct old_linux_dirent __user *, unsigned int);
asmlinkage long sys_pselect6(int, fd_set __user *, fd_set __user *,
diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
new file mode 100644
index 0000000..534e364
--- /dev/null
+++ b/include/uapi/linux/memfd.h
@@ -0,0 +1,8 @@
+#ifndef _UAPI_LINUX_MEMFD_H
+#define _UAPI_LINUX_MEMFD_H
+
+/* flags for memfd_create(2) (unsigned int) */
+#define MFD_CLOEXEC 0x0001U
+#define MFD_ALLOW_SEALING 0x0002U
+
+#endif /* _UAPI_LINUX_MEMFD_H */
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 36441b5..489a4e6 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -197,6 +197,7 @@ cond_syscall(compat_sys_timerfd_settime);
cond_syscall(compat_sys_timerfd_gettime);
cond_syscall(sys_eventfd);
cond_syscall(sys_eventfd2);
+cond_syscall(sys_memfd_create);
/* performance counters: */
cond_syscall(sys_perf_event_open);
diff --git a/mm/shmem.c b/mm/shmem.c
index 51dccd0..770e072 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -66,7 +66,9 @@ static struct vfsmount *shm_mnt;
#include <linux/highmem.h>
#include <linux/seq_file.h>
#include <linux/magic.h>
+#include <linux/syscalls.h>
#include <linux/fcntl.h>
+#include <uapi/linux/memfd.h>
#include <asm/uaccess.h>
#include <asm/pgtable.h>
@@ -2678,6 +2680,77 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
shmem_show_mpol(seq, sbinfo->mpol);
return 0;
}
+
+#define MFD_NAME_PREFIX "memfd:"
+#define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1)
+#define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
+
+#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING)
+
+SYSCALL_DEFINE2(memfd_create,
+ const char __user *, uname,
+ unsigned int, flags)
+{
+ struct shmem_inode_info *info;
+ struct file *file;
+ int fd, error;
+ char *name;
+ long len;
+
+ if (flags & ~(unsigned int)MFD_ALL_FLAGS)
+ return -EINVAL;
+
+ /* length includes terminating zero */
+ len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
+ if (len <= 0)
+ return -EFAULT;
+ if (len > MFD_NAME_MAX_LEN + 1)
+ return -EINVAL;
+
+ name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_TEMPORARY);
+ if (!name)
+ return -ENOMEM;
+
+ strcpy(name, MFD_NAME_PREFIX);
+ if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
+ error = -EFAULT;
+ goto err_name;
+ }
+
+ /* terminating-zero may have changed after strnlen_user() returned */
+ if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
+ error = -EFAULT;
+ goto err_name;
+ }
+
+ fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
+ if (fd < 0) {
+ error = fd;
+ goto err_name;
+ }
+
+ file = shmem_file_setup(name, 0, VM_NORESERVE);
+ if (IS_ERR(file)) {
+ error = PTR_ERR(file);
+ goto err_fd;
+ }
+ info = SHMEM_I(file_inode(file));
+ file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
+ file->f_flags |= O_RDWR | O_LARGEFILE;
+ if (flags & MFD_ALLOW_SEALING)
+ info->seals &= ~F_SEAL_SEAL;
+
+ fd_install(fd, file);
+ kfree(name);
+ return fd;
+
+err_fd:
+ put_unused_fd(fd);
+err_name:
+ kfree(name);
+ return error;
+}
+
#endif /* CONFIG_TMPFS */
static void shmem_put_super(struct super_block *sb)
--
2.0.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v4 2/6] shm: add sealing API
From: David Herrmann @ 2014-07-20 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Michael Kerrisk, Ryan Lortie, Linus Torvalds, Andrew Morton,
linux-mm, linux-fsdevel, linux-api, Greg Kroah-Hartman,
john.stultz, Lennart Poettering, Daniel Mack, Kay Sievers,
Hugh Dickins, Andy Lutomirski, Alexander Viro, David Herrmann
In-Reply-To: <1405877680-999-1-git-send-email-dh.herrmann@gmail.com>
If two processes share a common memory region, they usually want some
guarantees to allow safe access. This often includes:
- one side cannot overwrite data while the other reads it
- one side cannot shrink the buffer while the other accesses it
- one side cannot grow the buffer beyond previously set boundaries
If there is a trust-relationship between both parties, there is no need
for policy enforcement. However, if there's no trust relationship (eg.,
for general-purpose IPC) sharing memory-regions is highly fragile and
often not possible without local copies. Look at the following two
use-cases:
1) A graphics client wants to share its rendering-buffer with a
graphics-server. The memory-region is allocated by the client for
read/write access and a second FD is passed to the server. While
scanning out from the memory region, the server has no guarantee that
the client doesn't shrink the buffer at any time, requiring rather
cumbersome SIGBUS handling.
2) A process wants to perform an RPC on another process. To avoid huge
bandwidth consumption, zero-copy is preferred. After a message is
assembled in-memory and a FD is passed to the remote side, both sides
want to be sure that neither modifies this shared copy, anymore. The
source may have put sensible data into the message without a separate
copy and the target may want to parse the message inline, to avoid a
local copy.
While SIGBUS handling, POSIX mandatory locking and MAP_DENYWRITE provide
ways to achieve most of this, the first one is unproportionally ugly to
use in libraries and the latter two are broken/racy or even disabled due
to denial of service attacks.
This patch introduces the concept of SEALING. If you seal a file, a
specific set of operations is blocked on that file forever.
Unlike locks, seals can only be set, never removed. Hence, once you
verified a specific set of seals is set, you're guaranteed that no-one can
perform the blocked operations on this file, anymore.
An initial set of SEALS is introduced by this patch:
- SHRINK: If SEAL_SHRINK is set, the file in question cannot be reduced
in size. This affects ftruncate() and open(O_TRUNC).
- GROW: If SEAL_GROW is set, the file in question cannot be increased
in size. This affects ftruncate(), fallocate() and write().
- WRITE: If SEAL_WRITE is set, no write operations (besides resizing)
are possible. This affects fallocate(PUNCH_HOLE), mmap() and
write().
- SEAL: If SEAL_SEAL is set, no further seals can be added to a file.
This basically prevents the F_ADD_SEAL operation on a file and
can be set to prevent others from adding further seals that you
don't want.
The described use-cases can easily use these seals to provide safe use
without any trust-relationship:
1) The graphics server can verify that a passed file-descriptor has
SEAL_SHRINK set. This allows safe scanout, while the client is
allowed to increase buffer size for window-resizing on-the-fly.
Concurrent writes are explicitly allowed.
2) For general-purpose IPC, both processes can verify that SEAL_SHRINK,
SEAL_GROW and SEAL_WRITE are set. This guarantees that neither
process can modify the data while the other side parses it.
Furthermore, it guarantees that even with writable FDs passed to the
peer, it cannot increase the size to hit memory-limits of the source
process (in case the file-storage is accounted to the source).
The new API is an extension to fcntl(), adding two new commands:
F_GET_SEALS: Return a bitset describing the seals on the file. This
can be called on any FD if the underlying file supports
sealing.
F_ADD_SEALS: Change the seals of a given file. This requires WRITE
access to the file and F_SEAL_SEAL may not already be set.
Furthermore, the underlying file must support sealing and
there may not be any existing shared mapping of that file.
Otherwise, EBADF/EPERM is returned.
The given seals are _added_ to the existing set of seals
on the file. You cannot remove seals again.
The fcntl() handler is currently specific to shmem and disabled on all
files. A file needs to explicitly support sealing for this interface to
work. A separate syscall is added in a follow-up, which creates files that
support sealing. There is no intention to support this on other
file-systems. Semantics are unclear for non-volatile files and we lack any
use-case right now. Therefore, the implementation is specific to shmem.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
fs/fcntl.c | 5 ++
include/linux/shmem_fs.h | 17 ++++++
include/uapi/linux/fcntl.h | 15 +++++
mm/shmem.c | 143 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 180 insertions(+)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 72c82f6..22d1c3d 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -21,6 +21,7 @@
#include <linux/rcupdate.h>
#include <linux/pid_namespace.h>
#include <linux/user_namespace.h>
+#include <linux/shmem_fs.h>
#include <asm/poll.h>
#include <asm/siginfo.h>
@@ -336,6 +337,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
case F_GETPIPE_SZ:
err = pipe_fcntl(filp, cmd, arg);
break;
+ case F_ADD_SEALS:
+ case F_GET_SEALS:
+ err = shmem_fcntl(filp, cmd, arg);
+ break;
default:
break;
}
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index 4d1771c..50777b5 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -1,6 +1,7 @@
#ifndef __SHMEM_FS_H
#define __SHMEM_FS_H
+#include <linux/file.h>
#include <linux/swap.h>
#include <linux/mempolicy.h>
#include <linux/pagemap.h>
@@ -11,6 +12,7 @@
struct shmem_inode_info {
spinlock_t lock;
+ unsigned int seals; /* shmem seals */
unsigned long flags;
unsigned long alloced; /* data pages alloced to file */
union {
@@ -65,4 +67,19 @@ static inline struct page *shmem_read_mapping_page(
mapping_gfp_mask(mapping));
}
+#ifdef CONFIG_TMPFS
+
+extern int shmem_add_seals(struct file *file, unsigned int seals);
+extern int shmem_get_seals(struct file *file);
+extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
+
+#else
+
+static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
+{
+ return -EINVAL;
+}
+
+#endif
+
#endif
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 074b886..beed138 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -28,6 +28,21 @@
#define F_GETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 8)
/*
+ * Set/Get seals
+ */
+#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
+#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
+
+/*
+ * Types of seals
+ */
+#define F_SEAL_SEAL 0x0001 /* prevent further seals from being set */
+#define F_SEAL_SHRINK 0x0002 /* prevent file from shrinking */
+#define F_SEAL_GROW 0x0004 /* prevent file from growing */
+#define F_SEAL_WRITE 0x0008 /* prevent writes */
+/* (1U << 31) is reserved for signed error codes */
+
+/*
* Types of directory notifications that may be requested.
*/
#define DN_ACCESS 0x00000001 /* File accessed */
diff --git a/mm/shmem.c b/mm/shmem.c
index 1140f49..51dccd0 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -66,6 +66,7 @@ static struct vfsmount *shm_mnt;
#include <linux/highmem.h>
#include <linux/seq_file.h>
#include <linux/magic.h>
+#include <linux/fcntl.h>
#include <asm/uaccess.h>
#include <asm/pgtable.h>
@@ -532,6 +533,7 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
{
struct inode *inode = dentry->d_inode;
+ struct shmem_inode_info *info = SHMEM_I(inode);
int error;
error = inode_change_ok(inode, attr);
@@ -542,6 +544,11 @@ static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
loff_t oldsize = inode->i_size;
loff_t newsize = attr->ia_size;
+ /* protected by i_mutex */
+ if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
+ (newsize > oldsize && (info->seals & F_SEAL_GROW)))
+ return -EPERM;
+
if (newsize != oldsize) {
i_size_write(inode, newsize);
inode->i_ctime = inode->i_mtime = CURRENT_TIME;
@@ -1364,6 +1371,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
info = SHMEM_I(inode);
memset(info, 0, (char *)inode - (char *)info);
spin_lock_init(&info->lock);
+ info->seals = F_SEAL_SEAL;
info->flags = flags & VM_NORESERVE;
INIT_LIST_HEAD(&info->swaplist);
simple_xattrs_init(&info->xattrs);
@@ -1422,7 +1430,17 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
struct page **pagep, void **fsdata)
{
struct inode *inode = mapping->host;
+ struct shmem_inode_info *info = SHMEM_I(inode);
pgoff_t index = pos >> PAGE_CACHE_SHIFT;
+
+ /* i_mutex is held by caller */
+ if (unlikely(info->seals)) {
+ if (info->seals & F_SEAL_WRITE)
+ return -EPERM;
+ if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
+ return -EPERM;
+ }
+
return shmem_getpage(inode, index, pagep, SGP_WRITE, NULL);
}
@@ -1760,11 +1778,125 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
return offset;
}
+static int shmem_wait_for_pins(struct address_space *mapping)
+{
+ return 0;
+}
+
+#define F_ALL_SEALS (F_SEAL_SEAL | \
+ F_SEAL_SHRINK | \
+ F_SEAL_GROW | \
+ F_SEAL_WRITE)
+
+int shmem_add_seals(struct file *file, unsigned int seals)
+{
+ struct inode *inode = file_inode(file);
+ struct shmem_inode_info *info = SHMEM_I(inode);
+ int error;
+
+ /*
+ * SEALING
+ * Sealing allows multiple parties to share a shmem-file but restrict
+ * access to a specific subset of file operations. Seals can only be
+ * added, but never removed. This way, mutually untrusted parties can
+ * share common memory regions with a well-defined policy. A malicious
+ * peer can thus never perform unwanted operations on a shared object.
+ *
+ * Seals are only supported on special shmem-files and always affect
+ * the whole underlying inode. Once a seal is set, it may prevent some
+ * kinds of access to the file. Currently, the following seals are
+ * defined:
+ * SEAL_SEAL: Prevent further seals from being set on this file
+ * SEAL_SHRINK: Prevent the file from shrinking
+ * SEAL_GROW: Prevent the file from growing
+ * SEAL_WRITE: Prevent write access to the file
+ *
+ * As we don't require any trust relationship between two parties, we
+ * must prevent seals from being removed. Therefore, sealing a file
+ * only adds a given set of seals to the file, it never touches
+ * existing seals. Furthermore, the "setting seals"-operation can be
+ * sealed itself, which basically prevents any further seal from being
+ * added.
+ *
+ * Semantics of sealing are only defined on volatile files. Only
+ * anonymous shmem files support sealing. More importantly, seals are
+ * never written to disk. Therefore, there's no plan to support it on
+ * other file types.
+ */
+
+ if (file->f_op != &shmem_file_operations)
+ return -EINVAL;
+ if (!(file->f_mode & FMODE_WRITE))
+ return -EPERM;
+ if (seals & ~(unsigned int)F_ALL_SEALS)
+ return -EINVAL;
+
+ mutex_lock(&inode->i_mutex);
+
+ if (info->seals & F_SEAL_SEAL) {
+ error = -EPERM;
+ goto unlock;
+ }
+
+ if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
+ error = mapping_deny_writable(file->f_mapping);
+ if (error)
+ goto unlock;
+
+ error = shmem_wait_for_pins(file->f_mapping);
+ if (error) {
+ mapping_allow_writable(file->f_mapping);
+ goto unlock;
+ }
+ }
+
+ info->seals |= seals;
+ error = 0;
+
+unlock:
+ mutex_unlock(&inode->i_mutex);
+ return error;
+}
+EXPORT_SYMBOL_GPL(shmem_add_seals);
+
+int shmem_get_seals(struct file *file)
+{
+ if (file->f_op != &shmem_file_operations)
+ return -EINVAL;
+
+ return SHMEM_I(file_inode(file))->seals;
+}
+EXPORT_SYMBOL_GPL(shmem_get_seals);
+
+long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ long error;
+
+ switch (cmd) {
+ case F_ADD_SEALS:
+ /* disallow upper 32bit */
+ if (arg > UINT_MAX)
+ return -EINVAL;
+
+ error = shmem_add_seals(file, arg);
+ break;
+ case F_GET_SEALS:
+ error = shmem_get_seals(file);
+ break;
+ default:
+ error = -EINVAL;
+ break;
+ }
+
+ return error;
+}
+
static long shmem_fallocate(struct file *file, int mode, loff_t offset,
loff_t len)
{
struct inode *inode = file_inode(file);
struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
+ struct shmem_inode_info *info = SHMEM_I(inode);
struct shmem_falloc shmem_falloc;
pgoff_t start, index, end;
int error;
@@ -1781,6 +1913,12 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
loff_t unmap_start = round_up(offset, PAGE_SIZE);
loff_t unmap_end = round_down(offset + len, PAGE_SIZE) - 1;
+ /* protected by i_mutex */
+ if (info->seals & F_SEAL_WRITE) {
+ error = -EPERM;
+ goto out;
+ }
+
shmem_falloc.start = unmap_start >> PAGE_SHIFT;
shmem_falloc.next = (unmap_end + 1) >> PAGE_SHIFT;
spin_lock(&inode->i_lock);
@@ -1801,6 +1939,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
if (error)
goto out;
+ if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
+ error = -EPERM;
+ goto out;
+ }
+
start = offset >> PAGE_CACHE_SHIFT;
end = (offset + len + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
/* Try to avoid a swapstorm if len is impossible to satisfy */
--
2.0.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v4 1/6] mm: allow drivers to prevent new writable mappings
From: David Herrmann @ 2014-07-20 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Michael Kerrisk, Ryan Lortie, Linus Torvalds, Andrew Morton,
linux-mm, linux-fsdevel, linux-api, Greg Kroah-Hartman,
john.stultz, Lennart Poettering, Daniel Mack, Kay Sievers,
Hugh Dickins, Andy Lutomirski, Alexander Viro, David Herrmann
In-Reply-To: <1405877680-999-1-git-send-email-dh.herrmann@gmail.com>
The i_mmap_writable field counts existing writable mappings of an
address_space. To allow drivers to prevent new writable mappings, make
this counter signed and prevent new writable mappings if it is negative.
This is modelled after i_writecount and DENYWRITE.
This will be required by the shmem-sealing infrastructure to prevent any
new writable mappings after the WRITE seal has been set. In case there
exists a writable mapping, this operation will fail with EBUSY.
Note that we rely on the fact that iff you already own a writable mapping,
you can increase the counter without using the helpers. This is the same
that we do for i_writecount.
Acked-by: Hugh Dickins <hughd@google.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
fs/inode.c | 1 +
include/linux/fs.h | 29 +++++++++++++++++++++++++++--
kernel/fork.c | 2 +-
mm/mmap.c | 30 ++++++++++++++++++++++++------
mm/swap_state.c | 1 +
5 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 6eecb7f..9945b11 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -165,6 +165,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
mapping->a_ops = &empty_aops;
mapping->host = inode;
mapping->flags = 0;
+ atomic_set(&mapping->i_mmap_writable, 0);
mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
mapping->private_data = NULL;
mapping->backing_dev_info = &default_backing_dev_info;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e11d60c..152b254 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -387,7 +387,7 @@ struct address_space {
struct inode *host; /* owner: inode, block_device */
struct radix_tree_root page_tree; /* radix tree of all pages */
spinlock_t tree_lock; /* and lock protecting it */
- unsigned int i_mmap_writable;/* count VM_SHARED mappings */
+ atomic_t i_mmap_writable;/* count VM_SHARED mappings */
struct rb_root i_mmap; /* tree of private and shared mappings */
struct list_head i_mmap_nonlinear;/*list VM_NONLINEAR mappings */
struct mutex i_mmap_mutex; /* protect tree, count, list */
@@ -470,10 +470,35 @@ static inline int mapping_mapped(struct address_space *mapping)
* Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap_pgoff
* marks vma as VM_SHARED if it is shared, and the file was opened for
* writing i.e. vma may be mprotected writable even if now readonly.
+ *
+ * If i_mmap_writable is negative, no new writable mappings are allowed. You
+ * can only deny writable mappings, if none exists right now.
*/
static inline int mapping_writably_mapped(struct address_space *mapping)
{
- return mapping->i_mmap_writable != 0;
+ return atomic_read(&mapping->i_mmap_writable) > 0;
+}
+
+static inline int mapping_map_writable(struct address_space *mapping)
+{
+ return atomic_inc_unless_negative(&mapping->i_mmap_writable) ?
+ 0 : -EPERM;
+}
+
+static inline void mapping_unmap_writable(struct address_space *mapping)
+{
+ atomic_dec(&mapping->i_mmap_writable);
+}
+
+static inline int mapping_deny_writable(struct address_space *mapping)
+{
+ return atomic_dec_unless_positive(&mapping->i_mmap_writable) ?
+ 0 : -EBUSY;
+}
+
+static inline void mapping_allow_writable(struct address_space *mapping)
+{
+ atomic_inc(&mapping->i_mmap_writable);
}
/*
diff --git a/kernel/fork.c b/kernel/fork.c
index 6a13c46..9d37dfd 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -421,7 +421,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
atomic_dec(&inode->i_writecount);
mutex_lock(&mapping->i_mmap_mutex);
if (tmp->vm_flags & VM_SHARED)
- mapping->i_mmap_writable++;
+ atomic_inc(&mapping->i_mmap_writable);
flush_dcache_mmap_lock(mapping);
/* insert tmp into the share list, just after mpnt */
if (unlikely(tmp->vm_flags & VM_NONLINEAR))
diff --git a/mm/mmap.c b/mm/mmap.c
index 129b847..7f548e4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -216,7 +216,7 @@ static void __remove_shared_vm_struct(struct vm_area_struct *vma,
if (vma->vm_flags & VM_DENYWRITE)
atomic_inc(&file_inode(file)->i_writecount);
if (vma->vm_flags & VM_SHARED)
- mapping->i_mmap_writable--;
+ mapping_unmap_writable(mapping);
flush_dcache_mmap_lock(mapping);
if (unlikely(vma->vm_flags & VM_NONLINEAR))
@@ -617,7 +617,7 @@ static void __vma_link_file(struct vm_area_struct *vma)
if (vma->vm_flags & VM_DENYWRITE)
atomic_dec(&file_inode(file)->i_writecount);
if (vma->vm_flags & VM_SHARED)
- mapping->i_mmap_writable++;
+ atomic_inc(&mapping->i_mmap_writable);
flush_dcache_mmap_lock(mapping);
if (unlikely(vma->vm_flags & VM_NONLINEAR))
@@ -1572,6 +1572,17 @@ munmap_back:
if (error)
goto free_vma;
}
+ if (vm_flags & VM_SHARED) {
+ error = mapping_map_writable(file->f_mapping);
+ if (error)
+ goto allow_write_and_free_vma;
+ }
+
+ /* ->mmap() can change vma->vm_file, but must guarantee that
+ * vma_link() below can deny write-access if VM_DENYWRITE is set
+ * and map writably if VM_SHARED is set. This usually means the
+ * new file must not have been exposed to user-space, yet.
+ */
vma->vm_file = get_file(file);
error = file->f_op->mmap(file, vma);
if (error)
@@ -1611,8 +1622,12 @@ munmap_back:
vma_link(mm, vma, prev, rb_link, rb_parent);
/* Once vma denies write, undo our temporary denial count */
- if (vm_flags & VM_DENYWRITE)
- allow_write_access(file);
+ if (file) {
+ if (vm_flags & VM_SHARED)
+ mapping_unmap_writable(file->f_mapping);
+ if (vm_flags & VM_DENYWRITE)
+ allow_write_access(file);
+ }
file = vma->vm_file;
out:
perf_event_mmap(vma);
@@ -1641,14 +1656,17 @@ out:
return addr;
unmap_and_free_vma:
- if (vm_flags & VM_DENYWRITE)
- allow_write_access(file);
vma->vm_file = NULL;
fput(file);
/* Undo any partial mapping done by a device driver. */
unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
charged = 0;
+ if (vm_flags & VM_SHARED)
+ mapping_unmap_writable(file->f_mapping);
+allow_write_and_free_vma:
+ if (vm_flags & VM_DENYWRITE)
+ allow_write_access(file);
free_vma:
kmem_cache_free(vm_area_cachep, vma);
unacct_error:
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 2972eee..31321fa 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -39,6 +39,7 @@ static struct backing_dev_info swap_backing_dev_info = {
struct address_space swapper_spaces[MAX_SWAPFILES] = {
[0 ... MAX_SWAPFILES - 1] = {
.page_tree = RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
+ .i_mmap_writable = ATOMIC_INIT(0),
.a_ops = &swap_aops,
.backing_dev_info = &swap_backing_dev_info,
}
--
2.0.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [PATCH v4 0/6] File Sealing & memfd_create()
From: David Herrmann @ 2014-07-20 17:34 UTC (permalink / raw)
To: linux-kernel
Cc: Michael Kerrisk, Ryan Lortie, Linus Torvalds, Andrew Morton,
linux-mm, linux-fsdevel, linux-api, Greg Kroah-Hartman,
john.stultz, Lennart Poettering, Daniel Mack, Kay Sievers,
Hugh Dickins, Andy Lutomirski, Alexander Viro, David Herrmann
Hi
This is v4 of the File-Sealing and memfd_create() patches. You can find v1 with
a longer introduction at gmane [1], there's also v2 [2] and v3 [3] available.
See also the article about sealing on LWN [4], and a high-level introduction on
the new API in my blog [5]. Last but not least, man-page proposals are
available in my private repository [6].
This series introduces two new APIs:
memfd_create(): Think of this syscall as malloc() but it returns a
file-descriptor instead of a pointer. That file-descriptor is
backed by anon-memory and can be memory-mapped for access.
sealing: The sealing API can be used to prevent a specific set of operations
on a file-descriptor. You 'seal' the file and give thus the
guarantee, that those operations will be rejected from now on.
This series adds the memfd_create(2) syscall only to x86 and x86-64. Patches for
most other architectures are available in my private repository [7]. Missing
architectures are:
alpha, avr32, blackfin, cris, frv, m32r, microblaze, mn10300, sh, sparc,
um, xtensa
These architectures lack several newer syscalls, so those should be added first
before adding memfd_create(2). I can provide patches for those, if required.
However, I think it should be kept separate from this series.
Changes in v4:
- drop page-isolation in favor of shmem_wait_for_pins()
- add unlikely(info->seals) to write_begin hot-path
- return EPERM for F_ADD_SEALS if file is not writable
- moved shmem_wait_for_pins() entirely into it's own commit
- make O_LARGEFILE mandatory part of memfd_create() ABI
- add lru_add_drain() to shmem_tag_pins() hot-path
- minor coding-style changes
Thanks
David
[1] memfd v1: http://thread.gmane.org/gmane.comp.video.dri.devel/102241
[2] memfd v2: http://thread.gmane.org/gmane.linux.kernel.mm/115713
[3] memfd v3: http://thread.gmane.org/gmane.linux.kernel.mm/118721
[4] LWN article: https://lwn.net/Articles/593918/
[5] API Intro: http://dvdhrm.wordpress.com/2014/06/10/memfd_create2/
[6] Man-pages: http://cgit.freedesktop.org/~dvdhrm/man-pages/log/?h=memfd
[7] Dev-repo: http://cgit.freedesktop.org/~dvdhrm/linux/log/?h=memfd
David Herrmann (6):
mm: allow drivers to prevent new writable mappings
shm: add sealing API
shm: add memfd_create() syscall
selftests: add memfd_create() + sealing tests
selftests: add memfd/sealing page-pinning tests
shm: wait for pins to be released when sealing
arch/x86/syscalls/syscall_32.tbl | 1 +
arch/x86/syscalls/syscall_64.tbl | 1 +
fs/fcntl.c | 5 +
fs/inode.c | 1 +
include/linux/fs.h | 29 +-
include/linux/shmem_fs.h | 17 +
include/linux/syscalls.h | 1 +
include/uapi/linux/fcntl.h | 15 +
include/uapi/linux/memfd.h | 8 +
kernel/fork.c | 2 +-
kernel/sys_ni.c | 1 +
mm/mmap.c | 30 +-
mm/shmem.c | 324 +++++++++
mm/swap_state.c | 1 +
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/memfd/.gitignore | 4 +
tools/testing/selftests/memfd/Makefile | 41 ++
tools/testing/selftests/memfd/fuse_mnt.c | 110 +++
tools/testing/selftests/memfd/fuse_test.c | 311 +++++++++
tools/testing/selftests/memfd/memfd_test.c | 913 +++++++++++++++++++++++++
tools/testing/selftests/memfd/run_fuse_test.sh | 14 +
21 files changed, 1821 insertions(+), 9 deletions(-)
create mode 100644 include/uapi/linux/memfd.h
create mode 100644 tools/testing/selftests/memfd/.gitignore
create mode 100644 tools/testing/selftests/memfd/Makefile
create mode 100755 tools/testing/selftests/memfd/fuse_mnt.c
create mode 100644 tools/testing/selftests/memfd/fuse_test.c
create mode 100644 tools/testing/selftests/memfd/memfd_test.c
create mode 100755 tools/testing/selftests/memfd/run_fuse_test.sh
--
2.0.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 08/25] amdkfd: Add IOCTL set definitions of amdkfd
From: Jerome Glisse @ 2014-07-20 16:54 UTC (permalink / raw)
To: Oded Gabbay
Cc: David Airlie, Alex Deucher, Andrew Morton, John Bridgman,
Joerg Roedel, Andrew Lewycky, Christian König,
Michel Dänzer, Ben Goz, Alexey Skidanov, Evgeny Pinchuk,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1405603773-32688-9-git-send-email-oded.gabbay-5C7GfCeVMHo@public.gmane.org>
On Thu, Jul 17, 2014 at 04:29:15PM +0300, Oded Gabbay wrote:
> - KFD_IOC_GET_VERSION:
> Retrieves the interface version of amdkfd
>
> - KFD_IOC_CREATE_QUEUE:
> Creates a usermode queue that runs on a specific GPU device
>
> - KFD_IOC_DESTROY_QUEUE:
> Destroys an existing usermode queue
>
> - KFD_IOC_SET_MEMORY_POLICY:
> Sets the memory policy of the default and alternate aperture of the calling process
>
> - KFD_IOC_GET_CLOCK_COUNTERS:
> Retrieves counters (timestamps) of CPU and GPU
>
> - KFD_IOC_GET_PROCESS_APERTURES:
> Retrieves information about process apertures that were initialized
> during the open() call of the amdkfd device
>
> - KFD_IOC_UPDATE_QUEUE:
> Updates configuration of an existing usermode queue
>
> - KFD_IOC_PMC_ACQUIRE_ACCESS:
> Acquires exclusive access (from other HSA processes) to the performance
> counters of the GPU
>
> - KFD_IOC_PMC_RELEASE_ACCESS:
> Releases exclusive access of the GPU's performance counters
Exclusive userspace access is recipie for failure. This must go and you must
come up with better model. Which in my mind involve an ioctl for each command
buffer submission.
>
> Signed-off-by: Oded Gabbay <oded.gabbay-5C7GfCeVMHo@public.gmane.org>
> ---
> include/uapi/linux/kfd_ioctl.h | 133 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 133 insertions(+)
> create mode 100644 include/uapi/linux/kfd_ioctl.h
>
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> new file mode 100644
> index 0000000..3cedd1a
> --- /dev/null
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -0,0 +1,133 @@
> +/*
> + * Copyright 2014 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef KFD_IOCTL_H_INCLUDED
> +#define KFD_IOCTL_H_INCLUDED
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define KFD_IOCTL_CURRENT_VERSION 1
> +
> +/* The 64-bit ABI is the authoritative version. */
> +#pragma pack(push, 1)
pagram pack must be remove do not use that.
> +
> +struct kfd_ioctl_get_version_args {
> + uint32_t min_supported_version; /* from KFD */
> + uint32_t max_supported_version; /* from KFD */
> +};
> +
> +/* For kfd_ioctl_create_queue_args.queue_type. */
> +#define KFD_IOC_QUEUE_TYPE_COMPUTE 0
> +#define KFD_IOC_QUEUE_TYPE_SDMA 1
> +
> +struct kfd_ioctl_create_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> + uint64_t write_pointer_address; /* from KFD */
> + uint64_t read_pointer_address; /* from KFD */
> + uint64_t doorbell_address; /* from KFD */
> +
> + uint32_t ring_size; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> + uint32_t queue_type; /* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority; /* to KFD */
> + uint32_t queue_id; /* from KFD */
> +};
> +
> +struct kfd_ioctl_destroy_queue_args {
> + uint32_t queue_id; /* to KFD */
> +};
> +
> +struct kfd_ioctl_update_queue_args {
> + uint64_t ring_base_address; /* to KFD */
> +
> + uint32_t queue_id; /* to KFD */
> + uint32_t ring_size; /* to KFD */
> + uint32_t queue_percentage; /* to KFD */
> + uint32_t queue_priority; /* to KFD */
> +};
The queue_percentage and queue_priority really needs some explanations. I guess
userspace would shed some light on those but still this should be properly explain.
> +
> +/* For kfd_ioctl_set_memory_policy_args.default_policy and alternate_policy */
> +#define KFD_IOC_CACHE_POLICY_COHERENT 0
> +#define KFD_IOC_CACHE_POLICY_NONCOHERENT 1
> +
> +struct kfd_ioctl_set_memory_policy_args {
> + uint64_t alternate_aperture_base; /* to KFD */
> + uint64_t alternate_aperture_size; /* to KFD */
> +
> + uint32_t gpu_id; /* to KFD */
> + uint32_t default_policy; /* to KFD */
> + uint32_t alternate_policy; /* to KFD */
> +};
Same what is aperture in this context. I know about all this stuff but i have no
idea what aperture is in this context.
> +
> +struct kfd_ioctl_get_clock_counters_args {
> + uint64_t gpu_clock_counter; /* from KFD */
> + uint64_t cpu_clock_counter; /* from KFD */
> + uint64_t system_clock_counter; /* from KFD */
> + uint64_t system_clock_freq; /* from KFD */
> +
> + uint32_t gpu_id; /* to KFD */
> +};
Again comment about what kind of counter this is, monotonic, ...
> +
> +#define NUM_OF_SUPPORTED_GPUS 7
> +
> +struct kfd_process_device_apertures {
> + uint64_t lds_base; /* from KFD */
> + uint64_t lds_limit; /* from KFD */
> + uint64_t scratch_base; /* from KFD */
> + uint64_t scratch_limit; /* from KFD */
> + uint64_t gpuvm_base; /* from KFD */
> + uint64_t gpuvm_limit; /* from KFD */
> + uint32_t gpu_id; /* from KFD */
> +};
> +
> +struct kfd_ioctl_get_process_apertures_args {
> + struct kfd_process_device_apertures process_apertures[NUM_OF_SUPPORTED_GPUS];/* from KFD */
> + uint8_t num_of_nodes; /* from KFD, should be in the range [1 - NUM_OF_SUPPORTED_GPUS]*/
> +};
I would rather see userspace provide a pointer to an array and the size of that
array and possibly size of individual element. This would allow you to grow the
kfd_process_device_apertures if needs be. Thought as i understand it this is a
temporary driver.
> +
> +struct kfd_ioctl_pmc_acquire_access_args {
> + uint64_t trace_id; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> +};
> +
> +struct kfd_ioctl_pmc_release_access_args {
> + uint64_t trace_id; /* to KFD */
> + uint32_t gpu_id; /* to KFD */
> +};
As said above no ioctl to have some exclusive access you can not trust userspace
that's rules NUMBER ONE.
> +
> +#define KFD_IOC_MAGIC 'K'
> +
> +#define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> +#define KFD_IOC_CREATE_QUEUE _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
> +#define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> +#define KFD_IOC_SET_MEMORY_POLICY _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
> +#define KFD_IOC_GET_CLOCK_COUNTERS _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
> +#define KFD_IOC_GET_PROCESS_APERTURES _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
> +#define KFD_IOC_UPDATE_QUEUE _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
> +#define KFD_IOC_PMC_ACQUIRE_ACCESS _IOW(KFD_IOC_MAGIC, 12, struct kfd_ioctl_pmc_acquire_access_args)
> +#define KFD_IOC_PMC_RELEASE_ACCESS _IOW(KFD_IOC_MAGIC, 13, struct kfd_ioctl_pmc_release_access_args)
> +
> +#pragma pack(pop)
No pragma packing.
> +
> +#endif
> --
> 1.9.1
>
^ permalink raw reply
* Re: [RFC v3 7/7] shm: isolate pinned pages when sealing files
From: David Herrmann @ 2014-07-19 16:40 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407090155250.7841@eggly.anvils>
Hi
On Wed, Jul 9, 2014 at 10:57 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> When setting SEAL_WRITE, we must make sure nobody has a writable reference
>> to the pages (via GUP or similar). We currently check references and wait
>> some time for them to be dropped. This, however, might fail for several
>> reasons, including:
>> - the page is pinned for longer than we wait
>> - while we wait, someone takes an already pinned page for read-access
>>
>> Therefore, this patch introduces page-isolation. When sealing a file with
>> SEAL_WRITE, we copy all pages that have an elevated ref-count. The newpage
>> is put in place atomically, the old page is detached and left alone. It
>> will get reclaimed once the last external user dropped it.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> I've not checked it line by line, but this seems to be very good work;
> and I'm glad you have posted it, where we can refer back to it in future.
>
> However, I'm NAKing this patch, at least for now.
>
> The reason is simple and twofold.
>
> I absolutely do not want to be maintaining an alternative form of
> page migration in mm/shmem.c. Shmem has its own peculiar problems
> (mostly because of swap): adding a new dimension of very rarely
> exercised complication, and dependence on the rest mm, is not wise.
>
> And sealing just does not need this. It is clearly technically
> superior to, and more satisfying than, the "wait-a-while-then-give-up"
> technique which it would replace. But in practice, the wait-a-while
> technique is quite good enough (and much better self-contained than this).
>
> I've heard no requirement to support sealing of objects pinned for I/O,
> and the sealer just would not have given the object out for that; the
> requirement is to give the recipient of a sealed object confidence
> that it cannot be susceptible to modification in that way.
>
> I doubt there will ever be an actual need for sealing to use this
> migration technique; but I can imagine us referring back to your work in
> future, when/if implementing revoke on regular files. And integrating
> this into mm/migrate.c's unmap_and_move() as an extra-force mode
> (proceed even when the page count is raised).
>
> I think the concerns I had, when Tony first proposed this migration copy
> technique, were in fact unfounded - I was worried by the new inverse COW.
> On reflection, I don't think this introduces any new risks, which are
> not already present in page migration, truncation and orphaned pages.
>
> I didn't begin to test it at all, but the only defects that stood out
> in your code were in the areas of memcg and mlock. I think that if we
> go down the road of duplicating pinned pages, then we do have to make
> a memcg charge on the new page in addition to the old page. And if
> any pages happen to be mlock'ed into an address space, then we ought
> to map in the replacement pages afterwards (as page migration does,
> whether mlock'ed or not).
>
> (You were perfectly correct to use unmap_mapping_range(), rather than
> try_to_unmap() as page migration does: because unmap_mapping_range()
> manages the VM_NONLINEAR case. But our intention, under way, is to
> scrap all VM_NONLINEAR code, and just emulate it with multiple vmas,
> in which case try_to_unmap() should do.)
Dropping VM_NONLINEAR would make a lot of stuff so much easier.
I'm fine with dropping this patch again. The mlock and memcg issues
you raised are valid and should get fixed. And indeed, my testing
never triggered any real evelated page-refs except if I pinned them
via FUSE. Therefore, the wait-for-pins function should be sufficient,
indeed.
Thanks for the reviews! I will send v4 shortly.
David
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC v3 6/7] shm: wait for pins to be released when sealing
From: David Herrmann @ 2014-07-19 16:36 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407160308550.1775@eggly.anvils>
Hi
On Wed, Jul 16, 2014 at 12:09 PM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> We currently fail setting SEAL_WRITE in case there're pending page
>> references. This patch extends the pin-tests to wait up to 150ms for all
>> references to be dropped. This is still not perfect in that it doesn't
>> account for harmless read-only pins, but it's much better than a hard
>> failure.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Right, I didn't look through the patch itself, just compared the result
> with what I sent. Okay, you prefer to separate out shmem_tag_pins().
The main reason why I split both is to avoid goto-label "restart" and
"restart2".
> Yes, it looks fine. There's just one change I'd like at this stage,
> something I realized shortly after sending the code fragment: please
> add a call to lru_add_drain() at the head of shmem_tag_pins(). The
> reason being that lru_add_drain() is local to the cpu, so cheap, and
> in many cases will bring down all the raised refcounts right then.
>
> Whereas lru_add_drain_all() in the first scan of shmem_wait_for_pins()
> is much more expensive, involving inter-processor interrupts to do
> that on all cpus: it is appropriate to call it at that point, but we
> really ought to try the cheaper lru_add_drain() at the earlier stage.
I added an lru_add_drain_all() to my shmem_test_pins() function in
Patch 2/7. This patch dropped it again as your wait_for_pins() already
included it and it's quite expensive. But yes, the local
lru_add_drain() makes perfect sense. Fixed!
Thanks
David
> I would also like never to embark on this scan of the radix_tree
> and wait for pins, if the pages were never given out in a VM_SHARED
> mapping - or is that unrealistic, because every memfd is read-write,
> and typical initialization expected to be by mmap() rather than write()?
> But anyway, you're quite right not to get into that at this stage:
> it's best left as an optimization once the basics are safely in.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 5/7] selftests: add memfd/sealing page-pinning tests
From: David Herrmann @ 2014-07-19 16:32 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407160307460.1775@eggly.anvils>
Hi
On Wed, Jul 16, 2014 at 12:08 PM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> Setting SEAL_WRITE is not possible if there're pending GUP users. This
>> commit adds selftests for memfd+sealing that use FUSE to create pending
>> page-references. FUSE is very helpful here in that it allows us to delay
>> direct-IO operations for an arbitrary amount of time. This way, we can
>> force the kernel to pin pages and then run our normal selftests.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> I had a number of problems in getting this working (on openSUSE 13.1):
> rpm told me I had fuse installed, yet I had to download and install
> the tarball to get header files needed; then "make fuse_mnt" told me
> to add -D_FILE_OFFSET_BITS=64 to the compile flags; after which I
> got "undefined reference to `fuse_main_real'"; but then I tried
> "make run_fuse" as root, and it seemed to sort these issues out
> for itself, aside from "./run_fuse_test.sh: Permission denied" -
> which was within my bounds of comprehension unlike the rest!
>
> No complaint, thanks for providing the test (though I didn't check
> the source to convince myself that "DONE" has done what's claimed):
> some rainy day someone can get the Makefile working more smoothly,
> no need to delay the patchset for this.
_FILE_OFFSET_BITS=64 makes sense. I added it. The "undefined ref"
thing doesn't make sense to me and I cannot reproduce it. I will see
what I can do.
The "Permission denied" obviously just requires access to /dev/fuse,
as you figured out yourself.
Thanks
David
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 4/7] selftests: add memfd_create() + sealing tests
From: David Herrmann @ 2014-07-19 16:31 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407160307160.1775@eggly.anvils>
Hi
On Wed, Jul 16, 2014 at 12:07 PM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> Some basic tests to verify sealing on memfds works as expected and
>> guarantees the advertised semantics.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks for whatever the fix was, I didn't hit any problem running
> this version repeatedly, on 64-bit and on 32-bit.
glibc does pid-caching so getpid() can be skipped once called. fork()
and clone() have to update it, though. Therefore, you shouldn't mix
fork() and clone() in the same process. I replaced my fork() call with
a simple clone() and the bug was gone.
Thanks
David
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 3/7] shm: add memfd_create() syscall
From: David Herrmann @ 2014-07-19 16:29 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407160306420.1775@eggly.anvils>
Hi
On Wed, Jul 16, 2014 at 12:07 PM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> memfd_create() is similar to mmap(MAP_ANON), but returns a file-descriptor
>> that you can pass to mmap(). It can support sealing and avoids any
>> connection to user-visible mount-points. Thus, it's not subject to quotas
>> on mounted file-systems, but can be used like malloc()'ed memory, but
>> with a file-descriptor to it.
>>
>> memfd_create() returns the raw shmem file, so calls like ftruncate() can
>> be used to modify the underlying inode. Also calls like fstat()
>> will return proper information and mark the file as regular file. If you
>> want sealing, you can specify MFD_ALLOW_SEALING. Otherwise, sealing is not
>> supported (like on all other regular files).
>>
>> Compared to O_TMPFILE, it does not require a tmpfs mount-point and is not
>> subject to quotas and alike. It is still properly accounted to memcg
>> limits, though.
>
> It's an important point, but unclear quite what "quotas and alike" means.
> There's never been any quota support in shmem/tmpfs, but filesystem size
> can be limited. Maybe say "and is not subject to a filesystem size limit.
> It is still properly accounted to memcg limits, though, and to the same
> overcommit or no-overcommit accounting as all user memory."
Yes, makes sense. Fixed.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> A comment or two below, but this is okay by me. I'm not wildly excited
> to be getting a new system call in mm/shmem.c. I do like it much better
> now that you've dropped the size arg, thank you, but I still find it an
> odd system call: if it were not for the name, that you want so much for
> debugging, I think we would just implement this with a /dev/sealable
> alongside /dev/zero, which gave you your own object on opening (in the
> way that /dev/zero gives you your own object on mmap'ing).
mmap() supports replacing the file by a new file. Therefore, /dev/zero
works just fine. open() doesn't allow that and it looks non-trivial to
make it work. "non-trivial" is not really a counter-argument, but the
object-name is worth a new syscall, in my opinion. And it's a really
nice feature to debug complex systems.
> I haven't checked the manpage, I hope it's made very clear that
> there's no uniqueness imposed on the name, that it's merely a tag
> attached to the object.
Yes, the man-page clearly states that names are for debugging purposes
only and exposed via /proc/self/fd/ symlink-targets. They're not
subject to conflict-tests nor do two memfd's with the same name behave
any different.
> But from a shmem point of view this seems fine: if everyone else
> is happy with memfd_create(), it's fine by me.
>
>> ---
>> arch/x86/syscalls/syscall_32.tbl | 1 +
>> arch/x86/syscalls/syscall_64.tbl | 1 +
>> include/linux/syscalls.h | 1 +
>> include/uapi/linux/memfd.h | 8 +++++
>> kernel/sys_ni.c | 1 +
>> mm/shmem.c | 72 ++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 84 insertions(+)
>> create mode 100644 include/uapi/linux/memfd.h
>>
>> diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
>> index d6b8679..e7495b4 100644
>> --- a/arch/x86/syscalls/syscall_32.tbl
>> +++ b/arch/x86/syscalls/syscall_32.tbl
>> @@ -360,3 +360,4 @@
>> 351 i386 sched_setattr sys_sched_setattr
>> 352 i386 sched_getattr sys_sched_getattr
>> 353 i386 renameat2 sys_renameat2
>> +354 i386 memfd_create sys_memfd_create
>> diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
>> index ec255a1..28be0e1 100644
>> --- a/arch/x86/syscalls/syscall_64.tbl
>> +++ b/arch/x86/syscalls/syscall_64.tbl
>> @@ -323,6 +323,7 @@
>> 314 common sched_setattr sys_sched_setattr
>> 315 common sched_getattr sys_sched_getattr
>> 316 common renameat2 sys_renameat2
>> +317 common memfd_create sys_memfd_create
>>
>> #
>> # x32-specific system call numbers start at 512 to avoid cache impact
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index b0881a0..0be5d4d 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -802,6 +802,7 @@ asmlinkage long sys_timerfd_settime(int ufd, int flags,
>> asmlinkage long sys_timerfd_gettime(int ufd, struct itimerspec __user *otmr);
>> asmlinkage long sys_eventfd(unsigned int count);
>> asmlinkage long sys_eventfd2(unsigned int count, int flags);
>> +asmlinkage long sys_memfd_create(const char *uname_ptr, unsigned int flags);
>> asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
>> asmlinkage long sys_old_readdir(unsigned int, struct old_linux_dirent __user *, unsigned int);
>> asmlinkage long sys_pselect6(int, fd_set __user *, fd_set __user *,
>> diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
>> new file mode 100644
>> index 0000000..534e364
>> --- /dev/null
>> +++ b/include/uapi/linux/memfd.h
>> @@ -0,0 +1,8 @@
>> +#ifndef _UAPI_LINUX_MEMFD_H
>> +#define _UAPI_LINUX_MEMFD_H
>> +
>> +/* flags for memfd_create(2) (unsigned int) */
>> +#define MFD_CLOEXEC 0x0001U
>> +#define MFD_ALLOW_SEALING 0x0002U
>> +
>> +#endif /* _UAPI_LINUX_MEMFD_H */
>> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
>> index 36441b5..489a4e6 100644
>> --- a/kernel/sys_ni.c
>> +++ b/kernel/sys_ni.c
>> @@ -197,6 +197,7 @@ cond_syscall(compat_sys_timerfd_settime);
>> cond_syscall(compat_sys_timerfd_gettime);
>> cond_syscall(sys_eventfd);
>> cond_syscall(sys_eventfd2);
>> +cond_syscall(sys_memfd_create);
>>
>> /* performance counters: */
>> cond_syscall(sys_perf_event_open);
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 1438b3e..e7c5fe1 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -66,7 +66,9 @@ static struct vfsmount *shm_mnt;
>> #include <linux/highmem.h>
>> #include <linux/seq_file.h>
>> #include <linux/magic.h>
>> +#include <linux/syscalls.h>
>> #include <linux/fcntl.h>
>> +#include <uapi/linux/memfd.h>
>>
>> #include <asm/uaccess.h>
>> #include <asm/pgtable.h>
>> @@ -2662,6 +2664,76 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>> shmem_show_mpol(seq, sbinfo->mpol);
>> return 0;
>> }
>> +
>> +#define MFD_NAME_PREFIX "memfd:"
>> +#define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1)
>> +#define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
>> +
>> +#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING)
>> +
>> +SYSCALL_DEFINE2(memfd_create,
>> + const char*, uname,
>
> Jann Horn suggested "const char __user *" rather than "const char *",
> here and in syscalls.h, I think that's right (for sparse: compare
> with sys_open, for example).
Both fixed already. Sorry, I forgot to reply to Jann Horn. Thanks to
both of you!
>> + unsigned int, flags)
>> +{
>> + struct shmem_inode_info *info;
>> + struct file *file;
>> + int fd, error;
>> + char *name;
>> + long len;
>> +
>> + if (flags & ~(unsigned int)MFD_ALL_FLAGS)
>> + return -EINVAL;
>> +
>> + /* length includes terminating zero */
>> + len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
>> + if (len <= 0)
>> + return -EFAULT;
>> + if (len > MFD_NAME_MAX_LEN + 1)
>> + return -EINVAL;
>> +
>> + name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_TEMPORARY);
>> + if (!name)
>> + return -ENOMEM;
>> +
>> + strcpy(name, MFD_NAME_PREFIX);
>> + if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
>> + error = -EFAULT;
>> + goto err_name;
>> + }
>> +
>> + /* terminating-zero may have changed after strnlen_user() returned */
>> + if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
>> + error = -EFAULT;
>> + goto err_name;
>> + }
>> +
>> + fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
>
> Perhaps we should throw O_LARGEFILE in there too? So 32-bit is not
> surprised when it accesses beyond MAX_NON_LFS. I guess it's almost
> a non-issue, since the file is in memory, so not expected to be very
> large; but I seem to recall being caught out by a missing O_LARGEFILE
> in the past, and a new interface like this might do better to force it.
>
> But I'm not very sure of my ground here: please ask around, an fsdevel
> person will have a better idea than me, whether it's best included.
get_unused_fd_flags() doesn't take other flags than O_CLOEXEC, we need
to set it directly like we already do for f_mode.
On 64bit O_LARGEFILE is already forced for many syscalls. I added it
now as it makes perfect sense. It's part of the memfd ABI now.
man-page is fixed, too.
Thanks
David
>> + if (fd < 0) {
>> + error = fd;
>> + goto err_name;
>> + }
>> +
>> + file = shmem_file_setup(name, 0, VM_NORESERVE);
>> + if (IS_ERR(file)) {
>> + error = PTR_ERR(file);
>> + goto err_fd;
>> + }
>> + info = SHMEM_I(file_inode(file));
>> + file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
>> + if (flags & MFD_ALLOW_SEALING)
>> + info->seals &= ~F_SEAL_SEAL;
>> +
>> + fd_install(fd, file);
>> + kfree(name);
>> + return fd;
>> +
>> +err_fd:
>> + put_unused_fd(fd);
>> +err_name:
>> + kfree(name);
>> + return error;
>> +}
>> +
>> #endif /* CONFIG_TMPFS */
>>
>> static void shmem_put_super(struct super_block *sb)
>> --
>> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 2/7] shm: add sealing API
From: David Herrmann @ 2014-07-19 16:17 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407160304570.1775@eggly.anvils>
Hi Hugh
On Wed, Jul 16, 2014 at 12:06 PM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> If two processes share a common memory region, they usually want some
>> guarantees to allow safe access. This often includes:
>> - one side cannot overwrite data while the other reads it
>> - one side cannot shrink the buffer while the other accesses it
>> - one side cannot grow the buffer beyond previously set boundaries
>>
>> If there is a trust-relationship between both parties, there is no need
>> for policy enforcement. However, if there's no trust relationship (eg.,
>> for general-purpose IPC) sharing memory-regions is highly fragile and
>> often not possible without local copies. Look at the following two
>> use-cases:
>> 1) A graphics client wants to share its rendering-buffer with a
>> graphics-server. The memory-region is allocated by the client for
>> read/write access and a second FD is passed to the server. While
>> scanning out from the memory region, the server has no guarantee that
>> the client doesn't shrink the buffer at any time, requiring rather
>> cumbersome SIGBUS handling.
>> 2) A process wants to perform an RPC on another process. To avoid huge
>> bandwidth consumption, zero-copy is preferred. After a message is
>> assembled in-memory and a FD is passed to the remote side, both sides
>> want to be sure that neither modifies this shared copy, anymore. The
>> source may have put sensible data into the message without a separate
>> copy and the target may want to parse the message inline, to avoid a
>> local copy.
>>
>> While SIGBUS handling, POSIX mandatory locking and MAP_DENYWRITE provide
>> ways to achieve most of this, the first one is unproportionally ugly to
>> use in libraries and the latter two are broken/racy or even disabled due
>> to denial of service attacks.
>>
>> This patch introduces the concept of SEALING. If you seal a file, a
>> specific set of operations is blocked on that file forever.
>> Unlike locks, seals can only be set, never removed. Hence, once you
>> verified a specific set of seals is set, you're guaranteed that no-one can
>> perform the blocked operations on this file, anymore.
>>
>> An initial set of SEALS is introduced by this patch:
>> - SHRINK: If SEAL_SHRINK is set, the file in question cannot be reduced
>> in size. This affects ftruncate() and open(O_TRUNC).
>> - GROW: If SEAL_GROW is set, the file in question cannot be increased
>> in size. This affects ftruncate(), fallocate() and write().
>> - WRITE: If SEAL_WRITE is set, no write operations (besides resizing)
>> are possible. This affects fallocate(PUNCH_HOLE), mmap() and
>> write().
>> - SEAL: If SEAL_SEAL is set, no further seals can be added to a file.
>> This basically prevents the F_ADD_SEAL operation on a file and
>> can be set to prevent others from adding further seals that you
>> don't want.
>>
>> The described use-cases can easily use these seals to provide safe use
>> without any trust-relationship:
>> 1) The graphics server can verify that a passed file-descriptor has
>> SEAL_SHRINK set. This allows safe scanout, while the client is
>> allowed to increase buffer size for window-resizing on-the-fly.
>> Concurrent writes are explicitly allowed.
>> 2) For general-purpose IPC, both processes can verify that SEAL_SHRINK,
>> SEAL_GROW and SEAL_WRITE are set. This guarantees that neither
>> process can modify the data while the other side parses it.
>> Furthermore, it guarantees that even with writable FDs passed to the
>> peer, it cannot increase the size to hit memory-limits of the source
>> process (in case the file-storage is accounted to the source).
>>
>> The new API is an extension to fcntl(), adding two new commands:
>> F_GET_SEALS: Return a bitset describing the seals on the file. This
>> can be called on any FD if the underlying file supports
>> sealing.
>> F_ADD_SEALS: Change the seals of a given file. This requires WRITE
>> access to the file and F_SEAL_SEAL may not already be set.
>> Furthermore, the underlying file must support sealing and
>> there may not be any existing shared mapping of that file.
>> Otherwise, EBADF/EPERM is returned.
>> The given seals are _added_ to the existing set of seals
>> on the file. You cannot remove seals again.
>>
>> The fcntl() handler is currently specific to shmem and disabled on all
>> files. A file needs to explicitly support sealing for this interface to
>> work. A separate syscall is added in a follow-up, which creates files that
>> support sealing. There is no intention to support this on other
>> file-systems. Semantics are unclear for non-volatile files and we lack any
>> use-case right now. Therefore, the implementation is specific to shmem.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Looks pretty good to me, minor comments below.
>
>> ---
>> fs/fcntl.c | 5 ++
>> include/linux/shmem_fs.h | 17 ++++
>> include/uapi/linux/fcntl.h | 15 ++++
>> mm/shmem.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 223 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>> index 72c82f6..22d1c3d 100644
>> --- a/fs/fcntl.c
>> +++ b/fs/fcntl.c
>> @@ -21,6 +21,7 @@
>> #include <linux/rcupdate.h>
>> #include <linux/pid_namespace.h>
>> #include <linux/user_namespace.h>
>> +#include <linux/shmem_fs.h>
>>
>> #include <asm/poll.h>
>> #include <asm/siginfo.h>
>> @@ -336,6 +337,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
>> case F_GETPIPE_SZ:
>> err = pipe_fcntl(filp, cmd, arg);
>> break;
>> + case F_ADD_SEALS:
>> + case F_GET_SEALS:
>> + err = shmem_fcntl(filp, cmd, arg);
>> + break;
>> default:
>> break;
>> }
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 4d1771c..50777b5 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -1,6 +1,7 @@
>> #ifndef __SHMEM_FS_H
>> #define __SHMEM_FS_H
>>
>> +#include <linux/file.h>
>> #include <linux/swap.h>
>> #include <linux/mempolicy.h>
>> #include <linux/pagemap.h>
>> @@ -11,6 +12,7 @@
>>
>> struct shmem_inode_info {
>> spinlock_t lock;
>> + unsigned int seals; /* shmem seals */
>> unsigned long flags;
>> unsigned long alloced; /* data pages alloced to file */
>> union {
>> @@ -65,4 +67,19 @@ static inline struct page *shmem_read_mapping_page(
>> mapping_gfp_mask(mapping));
>> }
>>
>> +#ifdef CONFIG_TMPFS
>> +
>> +extern int shmem_add_seals(struct file *file, unsigned int seals);
>> +extern int shmem_get_seals(struct file *file);
>> +extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
>> +
>> +#else
>> +
>> +static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +#endif
>> +
>> #endif
>> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
>> index 074b886..beed138 100644
>> --- a/include/uapi/linux/fcntl.h
>> +++ b/include/uapi/linux/fcntl.h
>> @@ -28,6 +28,21 @@
>> #define F_GETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 8)
>>
>> /*
>> + * Set/Get seals
>> + */
>> +#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
>> +#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
>> +
>> +/*
>> + * Types of seals
>> + */
>> +#define F_SEAL_SEAL 0x0001 /* prevent further seals from being set */
>> +#define F_SEAL_SHRINK 0x0002 /* prevent file from shrinking */
>> +#define F_SEAL_GROW 0x0004 /* prevent file from growing */
>> +#define F_SEAL_WRITE 0x0008 /* prevent writes */
>> +/* (1U << 31) is reserved for signed error codes */
>> +
>> +/*
>> * Types of directory notifications that may be requested.
>> */
>> #define DN_ACCESS 0x00000001 /* File accessed */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index f484c27..1438b3e 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -66,6 +66,7 @@ static struct vfsmount *shm_mnt;
>> #include <linux/highmem.h>
>> #include <linux/seq_file.h>
>> #include <linux/magic.h>
>> +#include <linux/fcntl.h>
>>
>> #include <asm/uaccess.h>
>> #include <asm/pgtable.h>
>> @@ -531,16 +532,23 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
>> static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
>> {
>> struct inode *inode = dentry->d_inode;
>> + struct shmem_inode_info *info = SHMEM_I(inode);
>> + loff_t oldsize = inode->i_size;
>> + loff_t newsize = attr->ia_size;
>> int error;
>>
>> error = inode_change_ok(inode, attr);
>> if (error)
>> return error;
>>
>> - if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
>> - loff_t oldsize = inode->i_size;
>> - loff_t newsize = attr->ia_size;
>> + /* protected by i_mutex */
>> + if (attr->ia_valid & ATTR_SIZE) {
>> + if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
>> + (newsize > oldsize && (info->seals & F_SEAL_GROW)))
>> + return -EPERM;
>> + }
>
> Not important but...
> I'd have thought all that was better inside the S_ISREG(inode->i_mode) &&
> (attr->ia_valid & ATTR_SIZE) block. Less unnecessary change above, and
> more efficient for non-size attrs. You cannot seal anything but a regular
> file anyway, right?
You're right. Fixed.
>>
>> + if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
>> if (newsize != oldsize) {
>> i_size_write(inode, newsize);
>> inode->i_ctime = inode->i_mtime = CURRENT_TIME;
>> @@ -1315,6 +1323,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>> info = SHMEM_I(inode);
>> memset(info, 0, (char *)inode - (char *)info);
>> spin_lock_init(&info->lock);
>> + info->seals = F_SEAL_SEAL;
>> info->flags = flags & VM_NORESERVE;
>> INIT_LIST_HEAD(&info->swaplist);
>> simple_xattrs_init(&info->xattrs);
>> @@ -1374,7 +1383,15 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
>> {
>> int ret;
>> struct inode *inode = mapping->host;
>> + struct shmem_inode_info *info = SHMEM_I(inode);
>> pgoff_t index = pos >> PAGE_CACHE_SHIFT;
>> +
>> + /* i_mutex is held by caller */
>> + if (info->seals & F_SEAL_WRITE)
>> + return -EPERM;
>> + if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
>> + return -EPERM;
>
> I think this is your only addition which comes in a hot path.
> Mel has been shaving nanoseconds off this path recently: you're not
> introducing any atomic ops here, good, but I wonder if it would make any
> measurable difference to include this pair of tests inside a single
> "if (unlikely(info->seals)) {". Maybe not, but it wouldn't hurt.
It doesn't hurt adding the unlikely() protection, either. Fixed.
>> +
>> ret = shmem_getpage(inode, index, pagep, SGP_WRITE, NULL);
>> if (ret == 0 && *pagep)
>> init_page_accessed(*pagep);
>> @@ -1715,11 +1732,166 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
>> return offset;
>> }
>>
>> +/*
>> + * Setting SEAL_WRITE requires us to verify there's no pending writer. However,
>> + * via get_user_pages(), drivers might have some pending I/O without any active
>> + * user-space mappings (eg., direct-IO, AIO). Therefore, we look at all pages
>> + * and see whether it has an elevated ref-count. If so, we abort.
>> + * The caller must guarantee that no new user will acquire writable references
>> + * to those pages to avoid races.
>> + */
>> +static int shmem_test_for_pins(struct address_space *mapping)
>> +{
>> + struct radix_tree_iter iter;
>> + void **slot;
>> + pgoff_t start;
>> + struct page *page;
>> + int error;
>> +
>> + /* flush additional refs in lru_add early */
>> + lru_add_drain_all();
>> +
>> + error = 0;
>> + start = 0;
>> + rcu_read_lock();
>> +
>> +restart:
>> + radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
>> + page = radix_tree_deref_slot(slot);
>> + if (!page || radix_tree_exception(page)) {
>> + if (radix_tree_deref_retry(page))
>> + goto restart;
>> + } else if (page_count(page) - page_mapcount(page) > 1) {
>> + error = -EBUSY;
>> + break;
>> + }
>> +
>> + if (need_resched()) {
>> + cond_resched_rcu();
>> + start = iter.index + 1;
>> + goto restart;
>> + }
>> + }
>> + rcu_read_unlock();
>> +
>> + return error;
>> +}
>
> Please leave shmem_test_for_pins() (and the comment above it)
> out of this particular patch.
>
> The implementation here satisfies none of us, make it harder to
> figure out the patch improving it later, and distracts from the basic
> sealing interface and functionality that you introduce in this patch.
>
> A brief comment on the issue instead - "But what if a page of the
> object is pinned for pending I/O? See later patch" - maybe, but on the
> whole I think it's better to raise and settle the issue in later patch.
Removed and replaced by a dummy:
int shmem_wait_for_pins(struct address_space *mapping)
{
return 0;
}
>> +
>> +#define F_ALL_SEALS (F_SEAL_SEAL | \
>> + F_SEAL_SHRINK | \
>> + F_SEAL_GROW | \
>> + F_SEAL_WRITE)
>> +
>> +int shmem_add_seals(struct file *file, unsigned int seals)
>> +{
>> + struct dentry *dentry = file->f_path.dentry;
>> + struct inode *inode = dentry->d_inode;
>
> struct inode *inode = file_inode(file), and forget about dentry?
Fixed.
>> + struct shmem_inode_info *info = SHMEM_I(inode);
>> + int error;
>> +
>> + /*
>> + * SEALING
>> + * Sealing allows multiple parties to share a shmem-file but restrict
>> + * access to a specific subset of file operations. Seals can only be
>> + * added, but never removed. This way, mutually untrusted parties can
>> + * share common memory regions with a well-defined policy. A malicious
>> + * peer can thus never perform unwanted operations on a shared object.
>> + *
>> + * Seals are only supported on special shmem-files and always affect
>> + * the whole underlying inode. Once a seal is set, it may prevent some
>> + * kinds of access to the file. Currently, the following seals are
>> + * defined:
>> + * SEAL_SEAL: Prevent further seals from being set on this file
>> + * SEAL_SHRINK: Prevent the file from shrinking
>> + * SEAL_GROW: Prevent the file from growing
>> + * SEAL_WRITE: Prevent write access to the file
>> + *
>> + * As we don't require any trust relationship between two parties, we
>> + * must prevent seals from being removed. Therefore, sealing a file
>> + * only adds a given set of seals to the file, it never touches
>> + * existing seals. Furthermore, the "setting seals"-operation can be
>> + * sealed itself, which basically prevents any further seal from being
>> + * added.
>> + *
>> + * Semantics of sealing are only defined on volatile files. Only
>> + * anonymous shmem files support sealing. More importantly, seals are
>> + * never written to disk. Therefore, there's no plan to support it on
>> + * other file types.
>> + */
>> +
>> + if (file->f_op != &shmem_file_operations)
>> + return -EINVAL;
>> + if (!(file->f_mode & FMODE_WRITE))
>> + return -EINVAL;
>
> I would expect -EBADF there (like when you write to read-only fd).
> Though I was okay with the -EPERM you had the previous version.
Nice catch. Replaced by EPERM again.
>> + if (seals & ~(unsigned int)F_ALL_SEALS)
>> + return -EINVAL;
>> +
>> + mutex_lock(&inode->i_mutex);
>> +
>> + if (info->seals & F_SEAL_SEAL) {
>
> I notice this is inconsistent with F_SEAL_WRITE just below:
> we're allowed to SEAL_WRITE what's already SEAL_WRITEd,
> but not to SEAL_SEAL what's already SEAL_SEALed.
> Oh, never mind, I can see that makes some sense.
>
>> + error = -EPERM;
>> + goto unlock;
>> + }
>> +
>> + if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
>> + error = mapping_deny_writable(file->f_mapping);
>> + if (error)
>
> Which would be -EBUSY, yes, that seems okay.
>
> And with your atomic i_mmap_writable changes in 1/7, and the i_mutex
> here, the locking is now solid, and accomplished simply: nice.
>
>> + goto unlock;
>> +
>> + error = shmem_test_for_pins(file->f_mapping);
>> + if (error) {
>> + mapping_allow_writable(file->f_mapping);
>> + goto unlock;
>> + }
>
> Right, although I ask you to remove shmem_test_for_pins() from this
> patch, I can see that you might want to include a "return 0" stub for
> shmem_wait_for_pins() in this patch, just so that this can appear here
> now, and we consider the non-atomicity of it. Yes, I agree this is
> how it should proceed: first deny, then re-allow if waiting fails.
I replaced shmem_test_for_pins() with shmem_wait_for_pins() and kept
the code as is.
>> + }
>> +
>> + info->seals |= seals;
>> + error = 0;
>> +
>> +unlock:
>> + mutex_unlock(&inode->i_mutex);
>> + return error;
>> +}
>> +EXPORT_SYMBOL_GPL(shmem_add_seals);
>> +
>> +int shmem_get_seals(struct file *file)
>> +{
>> + if (file->f_op != &shmem_file_operations)
>> + return -EINVAL;
>
> That's fine, though it is worth considering whether return 0
> might be preferable. No, I suppose this is easier, fits with
> shmem_fcntl() just returning -EINVAL when !TMPFS or !SHMEM.
Agreed.
>> +
>> + return SHMEM_I(file_inode(file))->seals & F_ALL_SEALS;
>
> & F_ALL_SEALS? Okay, that may be some kind of future proofing that you
> have in mind; but it may just be a leftover from when you were using bit
> 31 for internal use.
Nice catch. It's indeed a left-over. I removed it.
>> +}
>> +EXPORT_SYMBOL_GPL(shmem_get_seals);
>> +
>> +long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>> +{
>> + long error;
>> +
>> + switch (cmd) {
>> + case F_ADD_SEALS:
>> + /* disallow upper 32bit */
>> + if (arg >> 32)
>> + return -EINVAL;
>
> That is worth checking, but gives
> mm/shmem.c:1948:3: warning: right shift count >= width of type
> on a 32-bit build. I expect there's an accepted way to do it;
> I've used "arg > UINT_MAX" myself in some places.
The zero-test bot already reported this and I fixed it with a u64
cast. Your UINT_MAX test is definitely nicer so I changed it again.
Thanks!
David
>> +
>> + error = shmem_add_seals(file, arg);
>> + break;
>> + case F_GET_SEALS:
>> + error = shmem_get_seals(file);
>> + break;
>> + default:
>> + error = -EINVAL;
>> + break;
>> + }
>> +
>> + return error;
>> +}
>> +
>> static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>> loff_t len)
>> {
>> struct inode *inode = file_inode(file);
>> struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
>> + struct shmem_inode_info *info = SHMEM_I(inode);
>> struct shmem_falloc shmem_falloc;
>> pgoff_t start, index, end;
>> int error;
>> @@ -1731,6 +1903,12 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>> loff_t unmap_start = round_up(offset, PAGE_SIZE);
>> loff_t unmap_end = round_down(offset + len, PAGE_SIZE) - 1;
>>
>> + /* protected by i_mutex */
>> + if (info->seals & F_SEAL_WRITE) {
>> + error = -EPERM;
>> + goto out;
>> + }
>> +
>> if ((u64)unmap_end > (u64)unmap_start)
>> unmap_mapping_range(mapping, unmap_start,
>> 1 + unmap_end - unmap_start, 0);
>> @@ -1745,6 +1923,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>> if (error)
>> goto out;
>>
>> + if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
>> + error = -EPERM;
>> + goto out;
>> + }
>> +
>> start = offset >> PAGE_CACHE_SHIFT;
>> end = (offset + len + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
>> /* Try to avoid a swapstorm if len is impossible to satisfy */
>> --
>> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 1/7] mm: allow drivers to prevent new writable mappings
From: David Herrmann @ 2014-07-19 16:12 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <alpine.LSU.2.11.1407090154070.7841@eggly.anvils>
Hi Hugh
On Wed, Jul 9, 2014 at 10:55 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 13 Jun 2014, David Herrmann wrote:
>
>> The i_mmap_writable field counts existing writable mappings of an
>> address_space. To allow drivers to prevent new writable mappings, make
>> this counter signed and prevent new writable mappings if it is negative.
>> This is modelled after i_writecount and DENYWRITE.
>>
>> This will be required by the shmem-sealing infrastructure to prevent any
>> new writable mappings after the WRITE seal has been set. In case there
>> exists a writable mapping, this operation will fail with EBUSY.
>>
>> Note that we rely on the fact that iff you already own a writable mapping,
>> you can increase the counter without using the helpers. This is the same
>> that we do for i_writecount.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> I'm very pleased with the way you chose to do this in the end, following
> the way VM_DENYWRITE does it: it works out nicer than I had imagined.
>
> I do have some comments below, but the only one where I end up asking
> for a change is to remove the void "return "; but please read through
> in case any of my wonderings lead you to change your mind on something.
>
> I am very tempted to suggest that you add VM_WRITE into those VM_SHARED
> tests, and put the then necessary additional tests into mprotect.c: so
> that you would no longer have the odd case that a VM_SHARED !VM_WRITE
> area has to be unmapped before sealing the fd.
>
> But that would involve an audit of flush_dcache_page() architectures,
> and perhaps some mods to keep them safe across the mprotect(): let's
> put it down as a desirable future enhancement, just to remove an odd
> restriction.
>
> With the "return " gone,
> Acked-by: Hugh Dickins <hughd@google.com>
Thanks for the review! Yes, allowing VM_SHARED without VM_WRITE would
be nice, but I think it would make this patchset more complex than it
already is. I agree that it's better done separately.
>> ---
>> fs/inode.c | 1 +
>> include/linux/fs.h | 29 +++++++++++++++++++++++++++--
>> kernel/fork.c | 2 +-
>> mm/mmap.c | 24 ++++++++++++++++++------
>> mm/swap_state.c | 1 +
>> 5 files changed, 48 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 6eecb7f..9945b11 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -165,6 +165,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>> mapping->a_ops = &empty_aops;
>> mapping->host = inode;
>> mapping->flags = 0;
>> + atomic_set(&mapping->i_mmap_writable, 0);
>> mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>> mapping->private_data = NULL;
>> mapping->backing_dev_info = &default_backing_dev_info;
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 338e6f7..71d17c9 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -387,7 +387,7 @@ struct address_space {
>> struct inode *host; /* owner: inode, block_device */
>> struct radix_tree_root page_tree; /* radix tree of all pages */
>> spinlock_t tree_lock; /* and lock protecting it */
>> - unsigned int i_mmap_writable;/* count VM_SHARED mappings */
>> + atomic_t i_mmap_writable;/* count VM_SHARED mappings */
>> struct rb_root i_mmap; /* tree of private and shared mappings */
>> struct list_head i_mmap_nonlinear;/*list VM_NONLINEAR mappings */
>> struct mutex i_mmap_mutex; /* protect tree, count, list */
>> @@ -470,10 +470,35 @@ static inline int mapping_mapped(struct address_space *mapping)
>> * Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap_pgoff
>> * marks vma as VM_SHARED if it is shared, and the file was opened for
>> * writing i.e. vma may be mprotected writable even if now readonly.
>> + *
>> + * If i_mmap_writable is negative, no new writable mappings are allowed. You
>> + * can only deny writable mappings, if none exists right now.
>> */
>> static inline int mapping_writably_mapped(struct address_space *mapping)
>> {
>> - return mapping->i_mmap_writable != 0;
>> + return atomic_read(&mapping->i_mmap_writable) > 0;
>
> I have a slight anxiety that you're halving the writable range here:
> but I think that if we can overflow with this change, we could overflow
> before; and there are int counts elsewhere which would overflow easier.
Currently you need a new vm_area_struct to increase that counter.
vm_area_struct is about 144 bytes, times INT_MAX is something around
300GB. This does indeed look like a limit that can be reached not far
from now on common machines. I somehow doubt it's the only atomic that
might overflow soon, though. And there's always RLIMIT_AS you can set
to avoid such attacks.
I wouldn't oppose to using atomic_long_t, though. Many VM counters
already use atomic_long_t instead of atomic_t.
>> +}
>> +
>> +static inline int mapping_map_writable(struct address_space *mapping)
>> +{
>> + return atomic_inc_unless_negative(&mapping->i_mmap_writable) ?
>> + 0 : -EPERM;
>> +}
>> +
>> +static inline void mapping_unmap_writable(struct address_space *mapping)
>> +{
>> + return atomic_dec(&mapping->i_mmap_writable);
>
> Better just
>
> atomic_dec(&mapping->i_mmap_writable);
>
> I didn't realize before that the compiler allows you to return a void
> function from a void function without warning, but better not rely on that.
Fixed, thanks.
>> +}
>> +
>> +static inline int mapping_deny_writable(struct address_space *mapping)
>> +{
>> + return atomic_dec_unless_positive(&mapping->i_mmap_writable) ?
>> + 0 : -EBUSY;
>> +}
>> +
>> +static inline void mapping_allow_writable(struct address_space *mapping)
>> +{
>> + atomic_inc(&mapping->i_mmap_writable);
>> }
>>
>> /*
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index d2799d1..f1f127e 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -421,7 +421,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>> atomic_dec(&inode->i_writecount);
>> mutex_lock(&mapping->i_mmap_mutex);
>> if (tmp->vm_flags & VM_SHARED)
>> - mapping->i_mmap_writable++;
>> + atomic_inc(&mapping->i_mmap_writable);
>> flush_dcache_mmap_lock(mapping);
>> /* insert tmp into the share list, just after mpnt */
>> if (unlikely(tmp->vm_flags & VM_NONLINEAR))
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 129b847..19b6562 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -216,7 +216,7 @@ static void __remove_shared_vm_struct(struct vm_area_struct *vma,
>> if (vma->vm_flags & VM_DENYWRITE)
>> atomic_inc(&file_inode(file)->i_writecount);
>> if (vma->vm_flags & VM_SHARED)
>> - mapping->i_mmap_writable--;
>> + mapping_unmap_writable(mapping);
>
> Okay. I waste ridiculous time trying to decide whether it's better to say
>
> mapping_unmap_writable(mapping)
> or
> atomic_dec(&mapping->i_mmap_writable);
>
> here: there are consistency arguments both ways. I usually solve this
> problem by not giving wrapper names to such operations, but in this case
> I think the answer is just to accept whatever you decided was best.
>
>>
>> flush_dcache_mmap_lock(mapping);
>> if (unlikely(vma->vm_flags & VM_NONLINEAR))
>> @@ -617,7 +617,7 @@ static void __vma_link_file(struct vm_area_struct *vma)
>> if (vma->vm_flags & VM_DENYWRITE)
>> atomic_dec(&file_inode(file)->i_writecount);
>> if (vma->vm_flags & VM_SHARED)
>> - mapping->i_mmap_writable++;
>> + atomic_inc(&mapping->i_mmap_writable);
>>
>> flush_dcache_mmap_lock(mapping);
>> if (unlikely(vma->vm_flags & VM_NONLINEAR))
>> @@ -1572,6 +1572,11 @@ munmap_back:
>> if (error)
>> goto free_vma;
>> }
>> + if (vm_flags & VM_SHARED) {
>> + error = mapping_map_writable(file->f_mapping);
>> + if (error)
>> + goto allow_write_and_free_vma;
>> + }
>> vma->vm_file = get_file(file);
>> error = file->f_op->mmap(file, vma);
>> if (error)
>> @@ -1611,8 +1616,12 @@ munmap_back:
>>
>> vma_link(mm, vma, prev, rb_link, rb_parent);
>> /* Once vma denies write, undo our temporary denial count */
>> - if (vm_flags & VM_DENYWRITE)
>> - allow_write_access(file);
>> + if (file) {
>> + if (vm_flags & VM_SHARED)
>> + mapping_unmap_writable(file->f_mapping);
>> + if (vm_flags & VM_DENYWRITE)
>> + allow_write_access(file);
>> + }
>> file = vma->vm_file;
>
> Very good. A bit subtle, and for a while I was preparing to warn you
> of the odd shmem_zero_setup() case (which materializes a file when
> none came in), and the danger of the count going wrong on that.
>
> But you have it handled, with the "if (file)" block immediately before
> the new assignment of file, which should force anyone to think about it.
> And the ordering here, with respect to file and error exits, has always
> been tricky - I don't think you are making it any worse.
I tried to keep it as similar to VM_DENYWRITE as possible. It's not
really obvious and I had to read it several times, too. But looks all
fine to me now.
> Oh, more subtle than I realized above: I've just gone through a last
> minute "hey, it's wrong; oh, no, it's okay" waver here, remembering
> how mmap /dev/zero (and some others, I suppose) gives you a new file.
>
> That is okay: we don't even have to assume that sealing is limited
> to your memfd_create(): it's safe to assume that any device which
> forks you a new file, is safe to assert mapping_map_writable() upon
> temporarily; and the new file given could never come already sealed.
>
> But maybe a brief comment to reassure us in future?
I added a comment explaining that ->mmap has to make sure vma_link()
succeeds in raising these counters in case it modified vma->vm_file.
All users I found return files that haven't been exposed to user-space
at that point, therefore they're safe. They also don't do initial
sealing or VM_DENYWRITE, so they are safe in both regards.
>> out:
>> perf_event_mmap(vma);
>> @@ -1641,14 +1650,17 @@ out:
>> return addr;
>>
>> unmap_and_free_vma:
>> - if (vm_flags & VM_DENYWRITE)
>> - allow_write_access(file);
>> vma->vm_file = NULL;
>> fput(file);
>>
>> /* Undo any partial mapping done by a device driver. */
>> unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
>> charged = 0;
>> + if (vm_flags & VM_SHARED)
>> + mapping_unmap_writable(file->f_mapping);
>> +allow_write_and_free_vma:
>> + if (vm_flags & VM_DENYWRITE)
>> + allow_write_access(file);
>
> Okay. I don't enjoy that rearrangement, such that those two uses of
> file come after the fput(file); but I believe there are good reasons
> why it can never be the final fput(file), so I think you're okay.
Actually, the fput(file) should rather be "put(vma->vm_file). The
reference we free here is not the reference that we got passed by the
syscall-context, but it's the reference we took for vma->vm_file.
Therefore, I think it's fine to access "file" afterwards, as this is
an independent reference. The vma->vm_file reference is also taken
_after_ doing VM_DENYWRITE and VM_SHARED accounting. Therefore, I put
the cleanup of vma->vm_file before reverting the counters for
symmetry's sake.
Note that we cannot turn fput(file) into fput(vma->vm_file), though.
The vma->vm_file field might be left set to something else after
->mmap() failed.
Thanks
David
>> free_vma:
>> kmem_cache_free(vm_area_cachep, vma);
>> unacct_error:
>> diff --git a/mm/swap_state.c b/mm/swap_state.c
>> index 2972eee..31321fa 100644
>> --- a/mm/swap_state.c
>> +++ b/mm/swap_state.c
>> @@ -39,6 +39,7 @@ static struct backing_dev_info swap_backing_dev_info = {
>> struct address_space swapper_spaces[MAX_SWAPFILES] = {
>> [0 ... MAX_SWAPFILES - 1] = {
>> .page_tree = RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
>> + .i_mmap_writable = ATOMIC_INIT(0),
>> .a_ops = &swap_aops,
>> .backing_dev_info = &swap_backing_dev_info,
>> }
>> --
>> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH -v4] random: introduce getrandom(2) system call
From: Theodore Ts'o @ 2014-07-18 21:15 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-api, linux-crypto, beck, deraadt, Theodore Ts'o
The getrandom(2) system call was requested by the LibreSSL Portable
developers. It is analoguous to the getentropy(2) system call in
OpenBSD.
The rationale of this system call is to provide resiliance against
file descriptor exhaustion attacks, where the attacker consumes all
available file descriptors, forcing the use of the fallback code where
/dev/[u]random is not available. Since the fallback code is often not
well-tested, it is better to eliminate this potential failure mode
entirely.
The other feature provided by this new system call is the ability to
request randomness from the /dev/urandom entropy pool, but to block
until at least 128 bits of entropy has been accumulated in the
/dev/urandom entropy pool. Historically, the emphasis in the
/dev/urandom development has been to ensure that urandom pool is
initialized as quickly as possible after system boot, and preferably
before the init scripts start execution.
This is because changing /dev/urandom reads to block represents an
interface change that could potentially break userspace which is not
acceptable. In practice, on most x86 desktop and server systems, in
general the entropy pool can be initialized before it is needed (and
in modern kernels, we will printk a warning message if not). However,
on an embedded system, this may not be the case. And so with this new
interface, we can provide the functionality of blocking until the
urandom pool has been initialized. Any userspace program which uses
this new functionality must take care to assure that if it is used
during the boot process, that it will not cause the init scripts or
other portions of the system startup to hang indefinitely.
SYNOPSIS
#include <linux/random.h>
int getrandom(void *buf, size_t buflen, unsigned int flags);
DESCRIPTION
The system call getrandom() fills the buffer pointed to by buf
with up to buflen random bytes which can be used to seed user
space random number generators (i.e., DRBG's) or for other
cryptographic uses. It should not be used for Monte Carlo
simulations or other programs/algorithms which are doing
probabilistic sampling.
If the GRND_RANDOM flags bit is set, then draw from the
/dev/random pool instead of the /dev/urandom pool. The
/dev/random pool is limited based on the entropy that can be
obtained from environmental noise, so if there is insufficient
entropy, the requested number of bytes may not be returned.
If there is no entropy available at all, getrandom(2) will
either block, or return an error with errno set to EAGAIN if
the GRND_NONBLOCK bit is set in flags.
If the GRND_RANDOM bit is not set, then the /dev/urandom pool
will be used. Unlike using read(2) to fetch data from
/dev/urandom, if the urandom pool has not been sufficiently
initialized, getrandom(2) will block or return -1 with the
errno set to EGAIN if the GRND_NONBLOCK bit is set in flags.
The getentropy(2) system call in OpenBSD can be emulated using
the following function:
int getentropy(void *buf, size_t buflen)
{
int ret;
if (buflen > 256)
goto failure;
ret = getrandom(buf, buflen, 0);
if (ret < 0)
return ret;
if (ret == buflen)
return 0;
failure:
errno = EIO;
return -1;
}
RETURN VALUE
On success, the number of bytes that was filled in the buf is
returned. This may not be all the bytes requested by the
caller via buflen if insufficient entropy was present in the
/dev/random pool, or if the system call was interrupted by a
signal.
On error, -1 is returned, and errno is set appropriately.
ERRORS
EINVAL An invalid flag was passed to getrandom(2)
EFAULT buf is outside the accessible address space.
EAGAIN The requested entropy was not available, and the
getentropy(2) would have blocked if GRND_BLOCK flag
was set.
EINTR While blocked waiting for entropy, the call was
interrupted by a signal handler; see the description
of how interrupted read(2) calls on "slow" devices
are handled with and without the SA_RESTART flag
in the signal(7) man page.
NOTES
For small requests (buflen <= 256) getrandom(2) will not
return EINTR when reading from the urandom pool once the
entropy pool has been initialized, and it will return all of
the bytes that have been requested. This is the recommended
way to use getrandom(2), and is designed for compatibility
with OpenBSD's getentropy() system call.
However, if you are using GRND_RANDOM, then getrandom(2) may
block until the entropy accounting determines that sufficient
environmental noise has been gathered such that getrandom(2)
will be operating as a NRBG instead of a DRBG for those people
who are working in the NIST SP 800-90 regime. Since it may
block for a long time, these guarantees do *not* apply. The
user may want to interrupt a hanging process using a signal,
so blocking until all of the requested bytes are returned
would be unfriendly.
For this reason, the user of getrandom(2) MUST always check
the return value, in case it returns some error, or if fewer
bytes than requested was returned. In the case of
!GRND_RANDOM and small request, the latter should never
happen, but the careful userspace code (and all crypto code
should be careful) should check for this anyway!
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Zach Brown <zab@zabbo.net>
---
Changelog:
v4: Use a wait queue and wait_event() instead of a completion handler
More fine tuning of the commit description and man page
v3: Eliminate potential short reads and EINTR returns when requesting
small amounts of entropy from urandom (once the urandom pool is
initialized). See the NOTES section in the suggested man page for
a more in-depth discussion of the issues involved.
v2: Statically declare the completion structure
Check for and reject unknown flags
arch/x86/syscalls/syscall_32.tbl | 1 +
arch/x86/syscalls/syscall_64.tbl | 1 +
drivers/char/random.c | 42 ++++++++++++++++++++++++++++++++++++---
include/linux/syscalls.h | 3 +++
include/uapi/asm-generic/unistd.h | 4 +++-
include/uapi/linux/random.h | 9 +++++++++
6 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index d6b8679..f484e39 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -360,3 +360,4 @@
351 i386 sched_setattr sys_sched_setattr
352 i386 sched_getattr sys_sched_getattr
353 i386 renameat2 sys_renameat2
+354 i386 getrandom sys_getrandom
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index ec255a1..6705032 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -323,6 +323,7 @@
314 common sched_setattr sys_sched_setattr
315 common sched_getattr sys_sched_getattr
316 common renameat2 sys_renameat2
+317 common getrandom sys_getrandom
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/drivers/char/random.c b/drivers/char/random.c
index aa22fe5..c5335a2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -258,6 +258,8 @@
#include <linux/kmemcheck.h>
#include <linux/workqueue.h>
#include <linux/irq.h>
+#include <linux/syscalls.h>
+#include <linux/completion.h>
#include <asm/processor.h>
#include <asm/uaccess.h>
@@ -404,6 +406,7 @@ static struct poolinfo {
*/
static DECLARE_WAIT_QUEUE_HEAD(random_read_wait);
static DECLARE_WAIT_QUEUE_HEAD(random_write_wait);
+static DECLARE_WAIT_QUEUE_HEAD(urandom_init_wait);
static struct fasync_struct *fasync;
/**********************************************************************
@@ -657,6 +660,7 @@ retry:
r->entropy_total = 0;
if (r == &nonblocking_pool) {
prandom_reseed_late();
+ wake_up_interruptible(&urandom_init_wait);
pr_notice("random: %s pool is initialized\n", r->name);
}
}
@@ -1174,13 +1178,14 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
{
ssize_t ret = 0, i;
__u8 tmp[EXTRACT_SIZE];
+ int large_request = (nbytes > 256);
trace_extract_entropy_user(r->name, nbytes, ENTROPY_BITS(r), _RET_IP_);
xfer_secondary_pool(r, nbytes);
nbytes = account(r, nbytes, 0, 0);
while (nbytes) {
- if (need_resched()) {
+ if (large_request && need_resched()) {
if (signal_pending(current)) {
if (ret == 0)
ret = -ERESTARTSYS;
@@ -1355,7 +1360,7 @@ static int arch_random_refill(void)
}
static ssize_t
-random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
+_random_read(int nonblock, char __user *buf, size_t nbytes)
{
ssize_t n;
@@ -1379,7 +1384,7 @@ random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
if (arch_random_refill())
continue;
- if (file->f_flags & O_NONBLOCK)
+ if (nonblock)
return -EAGAIN;
wait_event_interruptible(random_read_wait,
@@ -1391,6 +1396,12 @@ random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
}
static ssize_t
+random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
+{
+ return _random_read(file->f_flags & O_NONBLOCK, buf, nbytes);
+}
+
+static ssize_t
urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
int ret;
@@ -1533,6 +1544,31 @@ const struct file_operations urandom_fops = {
.llseek = noop_llseek,
};
+SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
+ unsigned int, flags)
+{
+ int r;
+
+ if (flags & ~(GRND_NONBLOCK|GRND_RANDOM))
+ return -EINVAL;
+
+ if (count > INT_MAX)
+ count = INT_MAX;
+
+ if (flags & GRND_RANDOM)
+ return _random_read(flags & GRND_NONBLOCK, buf, count);
+
+ if (unlikely(nonblocking_pool.initialized == 0)) {
+ if (flags & GRND_NONBLOCK)
+ return -EAGAIN;
+ wait_event_interruptible(urandom_init_wait,
+ nonblocking_pool.initialized);
+ if (signal_pending(current))
+ return -ERESTARTSYS;
+ }
+ return urandom_read(NULL, buf, count, NULL);
+}
+
/***************************************************************
* Random UUID interface
*
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b0881a0..cd82f72f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -866,4 +866,7 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
unsigned long idx1, unsigned long idx2);
asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
+asmlinkage long sys_getrandom(char __user * buf, size_t count,
+ unsigned int flags);
+
#endif
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 3336406..2926b1d 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -699,9 +699,11 @@ __SYSCALL(__NR_sched_setattr, sys_sched_setattr)
__SYSCALL(__NR_sched_getattr, sys_sched_getattr)
#define __NR_renameat2 276
__SYSCALL(__NR_renameat2, sys_renameat2)
+#define __NR_getrandom 277
+__SYSCALL(__NR_getrandom, sys_getrandom)
#undef __NR_syscalls
-#define __NR_syscalls 277
+#define __NR_syscalls 278
/*
* All syscalls below here should go away really,
diff --git a/include/uapi/linux/random.h b/include/uapi/linux/random.h
index fff3528..3f93d16 100644
--- a/include/uapi/linux/random.h
+++ b/include/uapi/linux/random.h
@@ -40,4 +40,13 @@ struct rand_pool_info {
__u32 buf[0];
};
+/*
+ * Flags for getrandom(2)
+ *
+ * GRND_NONBLOCK Don't block and return EAGAIN instead
+ * GRND_RANDOM Use the /dev/random pool instead of /dev/urandom
+ */
+#define GRND_NONBLOCK 0x0001
+#define GRND_RANDOM 0x0002
+
#endif /* _UAPI_LINUX_RANDOM_H */
--
2.0.0
^ permalink raw reply related
* Re: [PATCH v12 11/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-18 18:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, linux-kernel@vger.kernel.org, James Morris,
Oleg Nesterov, David Drysdale, Michael Kerrisk (man-pages),
Will Drewry, Julien Tinnes, Linux API, X86 ML,
linux-arm-kernel@lists.infradead.org, Linux MIPS Mailing List,
linux-arch, LSM List
In-Reply-To: <CALCETrWj-NJeeKe6+qTZd3QsvLNMV_K=2swoNXza7Ef2xHkXoQ@mail.gmail.com>
On Fri, Jul 18, 2014 at 11:51 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Jul 18, 2014 at 11:13 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Jul 18, 2014 at 10:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Thu, Jul 17, 2014 at 8:26 PM, James Morris <jmorris@namei.org> wrote:
>>>> On Thu, 17 Jul 2014, Kees Cook wrote:
>>>>
>>>>> Twelfth time's the charm! :)
>>>>
>>>> Btw, there doesn't seem to be an official seccomp maintainer. Kees, would
>>>> you like to volunteer for this? If so, send in a patch for MAINTAINERS,
>>>> and set up a git tree for me to pull from.
>>
>> Sure thing, I'll get this set up now. I wonder if I should include the
>> glob "arch/*/kernel/ptrace.c" in the MAINTAINERS entry. :P
>
> You might want to convince some arch maintainers first, and that's
> like herding cats. Or Oleg, at least. Also, do you really want to be
> asked to deal with the never-ending stream of bugs that people find in
> files matching that glob?
Yeah, I've opted for "K: \bsecure_computing" Seemed like a decent middle-ground.
>>> *snicker* :)
>>>
>>> Kees, if you take on this awesome responsibility, should I send you a
>>> rebased version of the fastpath stuff? If so, I think that the
>>> arch-neutral part should go in through your shiny new tree (once it's
>>> reviewed to your satisfaction), and I'll ask hpa to pick up the x86
>>> part.
>>
>> Yeah, that sounds perfect.
>>
>>> I'd volunteer to be a "R:eviewer", but I don't think that the R tag
>>> has made it into MAINTAINERS yet.
>>
>> I'd love to have you listed. :)
>
> Feel free to pester me once "R" appears in case I forget.
I will indeed! :)
-Kees
>
> You might be able to convince me to co-maintain some day. Hmm.
>
> --Andy
>
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Chrome OS Security
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Andy Lutomirski @ 2014-07-18 18:57 UTC (permalink / raw)
To: Aditya Kali
Cc: Linux API, Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo,
cgroups-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar
In-Reply-To: <CAGr1F2GwZvZLPGLWKPPOt3vREwwVNbVPrgE6YJ01bACKejbc4Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jul 18, 2014 at 11:51 AM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Jul 18, 2014 at 9:51 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> > What happens if someone moves a task in a cgroup namespace outside of
>>> > the namespace root cgroup?
>>> >
>>>
>>> Attempt to move a task outside of cgroupns root will fail with EPERM.
>>> This is true irrespective of the privileges of the process attempting
>>> this. Once cgroupns is created, the task will be confined to the
>>> cgroup hierarchy under its cgroupns root until it dies.
>>
>> Can a task in a non-init userns create a cgroupns? If not, that's
>> unusual. If so, is it problematic if they can prevent themselves from
>> being moved?
>>
>
> Currently, only a task with CAP_SYS_ADMIN in the init-userns can
> create cgroupns. It is stricter than for other namespaces, yes.
I'm slightly hesitant to have unshare(CLONE_NEWUSER |
CLONE_NEWCGROUPNS | ...) start having weird side effects that are
visible outside the namespace, especially when those side effects
don't happen (because the call fails entirely) if
unshare(CLONE_NEWUSER) happens first. I don't see a real problem with
it, but it's weird.
>
>> I hate to say it, but it might be worth requiring explicit permission
>> from the cgroup manager for this. For example, there could be a new
>> cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
>> will fail with -EPERM unless the caller is in a may_share=1 cgroup.
>> may_unshare in a parent cgroup would not give child cgroups the
>> ability to unshare.
>>
>
> What you suggest can be done. The current patch-set punts the problem
> of permission checking by only allowing unshare from a
> capable(CAP_SYS_ADMIN) process. This can be implemented as a follow-up
> improvement to cgroupns feature if we want to open it to non-init
> userns.
>
> Being said that, I would argue that even if we don't have this
> explicit permission and relax the check to non-init userns, it should
> be 'OK' to let ns_capable(current_user_ns(), CAP_SYS_ADMIN) tasks to
> unshare cgroupns (basically, if you can "create" a cgroup hierarchy,
> you should probably be allowed to unshare() it).
But non-init-userns tasks can't create cgroup hierarchies, unless I
misunderstand the current code. And, if they can, I bet I can find
three or four serious security issues in an hour or two. :)
> By unsharing
> cgroupns, the tasks can only confine themselves further under its
> cgroupns-root. As long as it cannot escape that hierarchy, it should
> be fine.
But they can also *lock* their hierarchy.
> In my experience, there is seldom a need to move tasks out of their
> cgroup. At most, we create a sub-cgroup and move the task there (which
> is allowed in their cgroupns). Even for a cgroup manager, I can't
> think of a case where it will be useful to move a task from one cgroup
> hierarchy to another. Such move seems overly complicated (even without
> cgroup namespaces). The cgroup manager can just modify the settings of
> the task's cgroup as needed or simply kill & restart the task in a new
> container.
>
I do this all the time. Maybe my new systemd overlords will make me
stop doing it, at which point my current production setup will blow
up.
--Andy
^ permalink raw reply
* Re: [PATCH v12 11/11] seccomp: add thread sync ability
From: Andy Lutomirski @ 2014-07-18 18:51 UTC (permalink / raw)
To: Kees Cook
Cc: James Morris, linux-kernel@vger.kernel.org, James Morris,
Oleg Nesterov, David Drysdale, Michael Kerrisk (man-pages),
Will Drewry, Julien Tinnes, Linux API, X86 ML,
linux-arm-kernel@lists.infradead.org, Linux MIPS Mailing List,
linux-arch, LSM List
In-Reply-To: <CAGXu5jL9wQ4ZJ1kDRx_Y_cyGQEjO_Ugmcc--Q71x32_x1q9ScA@mail.gmail.com>
On Fri, Jul 18, 2014 at 11:13 AM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jul 18, 2014 at 10:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Jul 17, 2014 at 8:26 PM, James Morris <jmorris@namei.org> wrote:
>>> On Thu, 17 Jul 2014, Kees Cook wrote:
>>>
>>>> Twelfth time's the charm! :)
>>>
>>> Btw, there doesn't seem to be an official seccomp maintainer. Kees, would
>>> you like to volunteer for this? If so, send in a patch for MAINTAINERS,
>>> and set up a git tree for me to pull from.
>
> Sure thing, I'll get this set up now. I wonder if I should include the
> glob "arch/*/kernel/ptrace.c" in the MAINTAINERS entry. :P
You might want to convince some arch maintainers first, and that's
like herding cats. Or Oleg, at least. Also, do you really want to be
asked to deal with the never-ending stream of bugs that people find in
files matching that glob?
>
>> *snicker* :)
>>
>> Kees, if you take on this awesome responsibility, should I send you a
>> rebased version of the fastpath stuff? If so, I think that the
>> arch-neutral part should go in through your shiny new tree (once it's
>> reviewed to your satisfaction), and I'll ask hpa to pick up the x86
>> part.
>
> Yeah, that sounds perfect.
>
>> I'd volunteer to be a "R:eviewer", but I don't think that the R tag
>> has made it into MAINTAINERS yet.
>
> I'd love to have you listed. :)
Feel free to pester me once "R" appears in case I forget.
You might be able to convince me to co-maintain some day. Hmm.
--Andy
>
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Aditya Kali @ 2014-07-18 18:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux API, Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo,
cgroups-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar
In-Reply-To: <CALCETrW6YpyJBmr3sZC6KL03GP4dcGYavQF5DFZfys6Cok-vpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jul 18, 2014 at 9:51 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> > What happens if someone moves a task in a cgroup namespace outside of
>> > the namespace root cgroup?
>> >
>>
>> Attempt to move a task outside of cgroupns root will fail with EPERM.
>> This is true irrespective of the privileges of the process attempting
>> this. Once cgroupns is created, the task will be confined to the
>> cgroup hierarchy under its cgroupns root until it dies.
>
> Can a task in a non-init userns create a cgroupns? If not, that's
> unusual. If so, is it problematic if they can prevent themselves from
> being moved?
>
Currently, only a task with CAP_SYS_ADMIN in the init-userns can
create cgroupns. It is stricter than for other namespaces, yes.
> I hate to say it, but it might be worth requiring explicit permission
> from the cgroup manager for this. For example, there could be a new
> cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
> will fail with -EPERM unless the caller is in a may_share=1 cgroup.
> may_unshare in a parent cgroup would not give child cgroups the
> ability to unshare.
>
What you suggest can be done. The current patch-set punts the problem
of permission checking by only allowing unshare from a
capable(CAP_SYS_ADMIN) process. This can be implemented as a follow-up
improvement to cgroupns feature if we want to open it to non-init
userns.
Being said that, I would argue that even if we don't have this
explicit permission and relax the check to non-init userns, it should
be 'OK' to let ns_capable(current_user_ns(), CAP_SYS_ADMIN) tasks to
unshare cgroupns (basically, if you can "create" a cgroup hierarchy,
you should probably be allowed to unshare() it). By unsharing
cgroupns, the tasks can only confine themselves further under its
cgroupns-root. As long as it cannot escape that hierarchy, it should
be fine.
In my experience, there is seldom a need to move tasks out of their
cgroup. At most, we create a sub-cgroup and move the task there (which
is allowed in their cgroupns). Even for a cgroup manager, I can't
think of a case where it will be useful to move a task from one cgroup
hierarchy to another. Such move seems overly complicated (even without
cgroup namespaces). The cgroup manager can just modify the settings of
the task's cgroup as needed or simply kill & restart the task in a new
container.
> --Andy
Thanks,
--
Aditya
^ permalink raw reply
* Re: [PATCH v12 11/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-18 18:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, linux-kernel@vger.kernel.org, James Morris,
Oleg Nesterov, David Drysdale, Michael Kerrisk (man-pages),
Will Drewry, Julien Tinnes, Linux API, X86 ML,
linux-arm-kernel@lists.infradead.org, Linux MIPS Mailing List,
linux-arch, LSM List
In-Reply-To: <CALCETrUv3G+C3PTOD1FJGOG8AQCA30hNkUiusCnQDGq6Kt_Feg@mail.gmail.com>
On Fri, Jul 18, 2014 at 10:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jul 17, 2014 at 8:26 PM, James Morris <jmorris@namei.org> wrote:
>> On Thu, 17 Jul 2014, Kees Cook wrote:
>>
>>> Twelfth time's the charm! :)
>>
>> Btw, there doesn't seem to be an official seccomp maintainer. Kees, would
>> you like to volunteer for this? If so, send in a patch for MAINTAINERS,
>> and set up a git tree for me to pull from.
Sure thing, I'll get this set up now. I wonder if I should include the
glob "arch/*/kernel/ptrace.c" in the MAINTAINERS entry. :P
> *snicker* :)
>
> Kees, if you take on this awesome responsibility, should I send you a
> rebased version of the fastpath stuff? If so, I think that the
> arch-neutral part should go in through your shiny new tree (once it's
> reviewed to your satisfaction), and I'll ask hpa to pick up the x86
> part.
Yeah, that sounds perfect.
> I'd volunteer to be a "R:eviewer", but I don't think that the R tag
> has made it into MAINTAINERS yet.
I'd love to have you listed. :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v12 11/11] seccomp: add thread sync ability
From: Andy Lutomirski @ 2014-07-18 17:17 UTC (permalink / raw)
To: James Morris
Cc: Kees Cook, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
James Morris, Oleg Nesterov, David Drysdale,
Michael Kerrisk (man-pages), Will Drewry, Julien Tinnes,
Linux API, X86 ML,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Linux MIPS Mailing List, linux-arch, LSM List
In-Reply-To: <alpine.LRH.2.11.1407181325200.15709-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
On Thu, Jul 17, 2014 at 8:26 PM, James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 17 Jul 2014, Kees Cook wrote:
>
>> Twelfth time's the charm! :)
>
> Btw, there doesn't seem to be an official seccomp maintainer. Kees, would
> you like to volunteer for this? If so, send in a patch for MAINTAINERS,
> and set up a git tree for me to pull from.
*snicker* :)
Kees, if you take on this awesome responsibility, should I send you a
rebased version of the fastpath stuff? If so, I think that the
arch-neutral part should go in through your shiny new tree (once it's
reviewed to your satisfaction), and I'll ask hpa to pick up the x86
part.
I'd volunteer to be a "R:eviewer", but I don't think that the R tag
has made it into MAINTAINERS yet.
--Andy
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Andy Lutomirski @ 2014-07-18 16:51 UTC (permalink / raw)
To: Aditya Kali
Cc: Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, Linux API, Tejun Heo,
Ingo Molnar
In-Reply-To: <CAGr1F2Ht1q_nYGJwmQvEEyj8r3R1stgD=g3s8_5zYOTogjz-UQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> > On Thu, Jul 17, 2014 at 12:52 PM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >> Introduce the ability to create new cgroup namespace. The newly created
> >> cgroup namespace remembers the 'struct cgroup *root_cgrp' at the point
> >> of creation of the cgroup namespace. The task that creates the new
> >> cgroup namespace and all its future children will now be restricted only
> >> to the cgroup hierarchy under this root_cgrp. In the first version,
> >> setns() is not supported for cgroup namespaces.
> >> The main purpose of cgroup namespace is to virtualize the contents
> >> of /proc/self/cgroup file. Processes inside a cgroup namespace
> >> are only able to see paths relative to their namespace root.
> >> This allows container-tools (like libcontainer, lxc, lmctfy, etc.)
> >> to create completely virtualized containers without leaking system
> >> level cgroup hierarchy to the task.
> >
> > What happens if someone moves a task in a cgroup namespace outside of
> > the namespace root cgroup?
> >
>
> Attempt to move a task outside of cgroupns root will fail with EPERM.
> This is true irrespective of the privileges of the process attempting
> this. Once cgroupns is created, the task will be confined to the
> cgroup hierarchy under its cgroupns root until it dies.
Can a task in a non-init userns create a cgroupns? If not, that's
unusual. If so, is it problematic if they can prevent themselves from
being moved?
I hate to say it, but it might be worth requiring explicit permission
from the cgroup manager for this. For example, there could be a new
cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
will fail with -EPERM unless the caller is in a may_share=1 cgroup.
may_unshare in a parent cgroup would not give child cgroups the
ability to unshare.
--Andy
^ permalink raw reply
* Re: [PATCH 0/5] RFC: CGroup Namespaces
From: Serge Hallyn @ 2014-07-18 16:00 UTC (permalink / raw)
To: Aditya Kali
Cc: tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, mingo-H+wXaHxf7aLQT0dZR+AlfA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <1405626731-12220-1-git-send-email-adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Quoting Aditya Kali (adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org):
> Background
> Cgroups and Namespaces are used together to create “virtual”
> containers that isolates the host environment from the processes
> running in container. But since cgroups themselves are not
> “virtualized”, the task is always able to see global cgroups view
> through cgroupfs mount and via /proc/self/cgroup file.
>
> $ cat /proc/self/cgroup
> 0:cpuset,cpu,cpuacct,memory,devices,freezer,hugetlb:/batchjobs/c_job_id1
>
> This exposure of cgroup names to the processes running inside a
> container results in some problems:
> (1) The container names are typically host-container-management-agent
> (systemd, docker/libcontainer, etc.) data and leaking its name (or
> leaking the hierarchy) reveals too much information about the host
> system.
> (2) It makes the container migration across machines (CRIU) more
> difficult as the container names need to be unique across the
> machines in the migration domain.
> (3) It makes it difficult to run container management tools (like
> docker/libcontainer, lmctfy, etc.) within virtual containers
> without adding dependency on some state/agent present outside the
> container.
>
> Note that the feature proposed here is completely different than the
> “ns cgroup” feature which existed in the linux kernel until recently.
> The ns cgroup also attempted to connect cgroups and namespaces by
> creating a new cgroup every time a new namespace was created. It did
> not solve any of the above mentioned problems and was later dropped
> from the kernel.
>
> Introducing CGroup Namespaces
> With unified cgroup hierarchy
> (Documentation/cgroups/unified-hierarchy.txt), the containers can now
> have a much more coherent cgroup view and its easy to associate a
> container with a single cgroup. This also allows us to virtualize the
> cgroup view for tasks inside the container.
Hi,
So right now we basically do this in userspace using cgmanager. Each
container/chroot/whatever that has a cgproxy is 'locked' under that
proxy's cgroup. So if root in a container asks the cgproxy for the
cgroup of pid 2000, and cgproxy is in /lxc/u1 while pid 2000 in the
container is in /lxc/u1/service1, then the response will be '/service1'.
Same happens with creating cgroups, moving pids into cgroups, etc.
(Hoping to take a close look at this set early next week)
-serge
^ permalink raw reply
* Re: [PATCH v13 7/8] arm64: add pmd_[dirty|mkclean] for THP
From: Steve Capper @ 2014-07-18 14:55 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-kernel, linux-mm, Michael Kerrisk, Linux API,
Hugh Dickins, Johannes Weiner, Rik van Riel, KOSAKI Motohiro,
Mel Gorman, Jason Evans, Zhang Yanfei, Kirill A. Shutemov,
Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel
In-Reply-To: <1405666386-15095-8-git-send-email-minchan@kernel.org>
On Fri, Jul 18, 2014 at 03:53:05PM +0900, Minchan Kim wrote:
> MADV_FREE needs pmd_dirty and pmd_mkclean for detecting recent
> overwrite of the contents since MADV_FREE syscall is called for
> THP page.
>
> This patch adds pmd_dirty and pmd_mkclean for THP page MADV_FREE
> support.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Steve Capper <steve.capper@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Acked-by: Steve Capper <steve.capper@linaro.org>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v13 6/8] arm: add pmd_[dirty|mkclean] for THP
From: Steve Capper @ 2014-07-18 14:54 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Michael Kerrisk, Linux API,
Hugh Dickins, Johannes Weiner, Rik van Riel, KOSAKI Motohiro,
Mel Gorman, Jason Evans, Zhang Yanfei, Kirill A. Shutemov,
Catalin Marinas, Will Deacon, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1405666386-15095-7-git-send-email-minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Fri, Jul 18, 2014 at 03:53:04PM +0900, Minchan Kim wrote:
> MADV_FREE needs pmd_dirty and pmd_mkclean for detecting recent
> overwrite of the contents since MADV_FREE syscall is called for
> THP page.
>
> This patch adds pmd_dirty and pmd_mkclean for THP page MADV_FREE
> support.
>
> Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: Steve Capper <steve.capper-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Signed-off-by: Minchan Kim <minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
This patch looks good to me:
Acked-by: Steve Capper <steve.capper-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
There is another patch that introduces a helper function to test for
pmd bits, please see below.
> ---
> arch/arm/include/asm/pgtable-3level.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
> index 85c60adc8b60..830f84f2d277 100644
> --- a/arch/arm/include/asm/pgtable-3level.h
> +++ b/arch/arm/include/asm/pgtable-3level.h
> @@ -220,6 +220,8 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr)
> #define pmd_trans_splitting(pmd) (pmd_val(pmd) & PMD_SECT_SPLITTING)
> #endif
>
> +#define pmd_dirty(pmd) (pmd_val(pmd) & PMD_SECT_DIRTY)
Russell,
Should this be folded into my {pte|pmd}_isset patch?
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268979.html
Cheers,
--
Steve
> +
> #define PMD_BIT_FUNC(fn,op) \
> static inline pmd_t pmd_##fn(pmd_t pmd) { pmd_val(pmd) op; return pmd; }
>
> @@ -228,6 +230,7 @@ PMD_BIT_FUNC(mkold, &= ~PMD_SECT_AF);
> PMD_BIT_FUNC(mksplitting, |= PMD_SECT_SPLITTING);
> PMD_BIT_FUNC(mkwrite, &= ~PMD_SECT_RDONLY);
> PMD_BIT_FUNC(mkdirty, |= PMD_SECT_DIRTY);
> +PMD_BIT_FUNC(mkclean, &= ~PMD_SECT_DIRTY);
> PMD_BIT_FUNC(mkyoung, |= PMD_SECT_AF);
>
> #define pmd_mkhuge(pmd) (__pmd(pmd_val(pmd) & ~PMD_TABLE_BIT))
> --
> 2.0.0
>
^ permalink raw reply
* Re: [tytso-DPNOqEs/LNQ@public.gmane.org: [PATCH, RFC -v3] random: introduce getrandom(2) system call]
From: Theodore Ts'o @ 2014-07-18 14:04 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5293846.HEIrYe5Drj@wuerfel>
On Fri, Jul 18, 2014 at 03:16:18PM +0200, Arnd Bergmann wrote:
> On Friday 18 July 2014 08:56:06 Theodore Ts'o wrote:
> >
> > The change in the v3 version of the commit was to eliminate potential
> > short reads and EINTR returns when reading from urandom (once the
> > urandom pool is initialized). This was based on comments and requests
> > from Theo de Raadt. See the NOTES section in the suggested man page for
> > a more in-depth discussion of the issues involved.
>
> I think there is a problem with the completion...
>
> However, here you can get called an arbitrary number of times.
> It seems entirely possible than an attacker can manage to call
> this function 2 billion times. Assuming a latency of 1 microsecond
> per syscall, that would take about half an hour. After that, you
> never again get any urandom data out of the syscall.
>
> I think you are better off using a plain wait_event() here.
Nice catch, thanks!!
I'll rework the patch to use wait_event().
- Ted
^ permalink raw reply
* Re: [tytso-DPNOqEs/LNQ@public.gmane.org: [PATCH, RFC -v3] random: introduce getrandom(2) system call]
From: Arnd Bergmann @ 2014-07-18 13:16 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140718125606.GH1491-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org>
On Friday 18 July 2014 08:56:06 Theodore Ts'o wrote:
>
> The change in the v3 version of the commit was to eliminate potential
> short reads and EINTR returns when reading from urandom (once the
> urandom pool is initialized). This was based on comments and requests
> from Theo de Raadt. See the NOTES section in the suggested man page for
> a more in-depth discussion of the issues involved.
I think there is a problem with the completion:
> @@ -657,6 +661,7 @@ retry:
> r->entropy_total = 0;
> if (r == &nonblocking_pool) {
> prandom_reseed_late();
> + complete_all(&urandom_initialized);
> pr_notice("random: %s pool is initialized\n", r->name);
> }
> }
complete_all() sets the 'done' part of the completion to 'UINT_MAX/2',
which is enough to ensure that everyone waiting for the completion
at a given time will wake up, but a completion should not
be reused without calling init_completion again after a call to
complete_all().
> +SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
> + unsigned int, flags)
> +{
> + int r;
> +
> + if (flags & ~(GRND_NONBLOCK|GRND_RANDOM))
> + return -EINVAL;
> +
> + if (count > INT_MAX)
> + count = INT_MAX;
> +
> + if (flags & GRND_RANDOM)
> + return _random_read(flags & GRND_NONBLOCK, buf, count);
> + if (flags & GRND_NONBLOCK) {
> + if (!completion_done(&urandom_initialized))
> + return -EAGAIN;
> + } else {
> + r = wait_for_completion_interruptible(&urandom_initialized);
> + if (r)
> + return r;
> + }
> + return urandom_read(NULL, buf, count, NULL);
> +}
> +
However, here you can get called an arbitrary number of times.
It seems entirely possible than an attacker can manage to call
this function 2 billion times. Assuming a latency of 1 microsecond
per syscall, that would take about half an hour. After that, you
never again get any urandom data out of the syscall.
I think you are better off using a plain wait_event() here.
Arnd
^ 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