LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Davidlohr Bueso <dave@stgolabs.net>
To: akpm@linux-foundation.org
Cc: viro@zeniv.linux.org.uk, gorcunov@openvz.org, oleg@redhat.com,
	koct9i@gmail.com, dave@stgolabs.net, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso <dbueso@suse.de>
Subject: [PATCH 3/4] prctl: move MMF_EXE_FILE_CHANGED into exe_file struct
Date: Sat, 14 Mar 2015 15:39:25 -0700	[thread overview]
Message-ID: <1426372766-3029-4-git-send-email-dave@stgolabs.net> (raw)
In-Reply-To: <1426372766-3029-1-git-send-email-dave@stgolabs.net>

Given the introduction of the exe_file structure, this
functionality should be associated with. Because this is
a very specific prctl property, it is easily to do so. As
of now, when the file has already changed, mmap_sem is not
taken at all (however we do need it of course to check the
old mapping, but this is now shared) and we maintain the
test-and-set logic ensuring nothing raced when we were not
holding the exe_file lock.

Now, the downside is that this patch makes MMF_EXE_FILE_CHANGED
functionality general, of course trivially enlarging the mm_struct
to users that don't care about this - which is the most usual case.
But I don't see this of any importance really. Similarly the funcs
that prctl makes use of are also global, in fork.c -- I preferred
leaving things generic for any(?) future user(s), but it could very
well be moved to sys.c.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Oleg Nesterov <oleg@redhat.com>
CC: Konstantin Khlebnikov <koct9i@gmail.com>
---
 include/linux/mm.h       |  5 +++--
 include/linux/mm_types.h |  1 +
 include/linux/sched.h    |  5 ++---
 kernel/fork.c            | 37 +++++++++++++++++++++++++++---
 kernel/sys.c             | 58 +++++++++++++++++++++++++-----------------------
 5 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0c0720d..90eae9f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1911,9 +1911,10 @@ extern void mm_drop_all_locks(struct mm_struct *mm);
 
 /* mm->exe_file handling */
 extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
-extern void set_mm_exe_file_locked(struct mm_struct *mm,
-				   struct file *new_exe_file);
 extern struct file *get_mm_exe_file(struct mm_struct *mm);
+extern bool test_and_set_mm_exe_file(struct mm_struct *mm,
+				     struct file *new_exe_file);
+extern bool mm_exe_file_changed(struct mm_struct *mm);
 
 extern int may_expand_vm(struct mm_struct *mm, unsigned long npages);
 extern struct vm_area_struct *_install_special_mapping(struct mm_struct *mm,
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 1fc994e..2d8b06b 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -328,6 +328,7 @@ struct core_state {
 
 struct exe_file {
 	rwlock_t lock;
+	bool changed; /* see prctl_set_mm_exe_file() */
 	struct file *file;
 };
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6d77432..0caf62e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -494,10 +494,9 @@ static inline int get_dumpable(struct mm_struct *mm)
 					/* leave room for more dump flags */
 #define MMF_VM_MERGEABLE	16	/* KSM may merge identical pages */
 #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
-#define MMF_EXE_FILE_CHANGED	18	/* see prctl_set_mm_exe_file() */
 
-#define MMF_HAS_UPROBES		19	/* has uprobes */
-#define MMF_RECALC_UPROBES	20	/* MMF_HAS_UPROBES can be wrong */
+#define MMF_HAS_UPROBES		18	/* has uprobes */
+#define MMF_RECALC_UPROBES	19	/* MMF_HAS_UPROBES can be wrong */
 
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
diff --git a/kernel/fork.c b/kernel/fork.c
index aa0332b..54b0b91 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -675,18 +675,50 @@ void mmput(struct mm_struct *mm)
 }
 EXPORT_SYMBOL_GPL(mmput);
 
-void set_mm_exe_file_locked(struct mm_struct *mm, struct file *new_exe_file)
+/*
+ * exe_file handling is differentiated by the caller's need to
+ * be aware of the file being changed -- which will always require
+ * holding the exe_file lock. As such, the following functions
+ * keep track of this are (currently only used by prctl):
+ *   - test_and_set_mm_exe_file()
+ *   - mm_exe_file_changed()
+ *
+ * The rest of the callers should only stick to:
+ *   - set_mm_exe_file()
+ *   - get_mm_exe_file()
+ */
+bool test_and_set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
 {
+	bool ret = false;
+
+	write_lock(&mm->exe_file.lock);
+	if (mm->exe_file.changed)
+		goto done;
+
 	if (new_exe_file)
 		get_file(new_exe_file);
 	if (mm->exe_file.file)
 		fput(mm->exe_file.file);
 
-	write_lock(&mm->exe_file.lock);
 	mm->exe_file.file = new_exe_file;
+	ret = mm->exe_file.changed = true;
+done:
 	write_unlock(&mm->exe_file.lock);
+	return ret;
 }
 
+bool mm_exe_file_changed(struct mm_struct *mm)
+{
+	bool ret;
+
+	read_lock(&mm->exe_file.lock);
+	ret = mm->exe_file.changed;
+	read_lock(&mm->exe_file.lock);
+
+	return ret;
+}
+
+/* lockless -- see each caller for justification */
 void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
 {
 	if (new_exe_file)
@@ -711,7 +743,6 @@ struct file *get_mm_exe_file(struct mm_struct *mm)
 }
 EXPORT_SYMBOL(get_mm_exe_file);
 
-
 static void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm)
 {
 	/*
diff --git a/kernel/sys.c b/kernel/sys.c
index a4d70f0..a82d0c4 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1649,17 +1649,27 @@ SYSCALL_DEFINE1(umask, int, mask)
 	return mask;
 }
 
-static int __prctl_set_mm_exe_file(struct mm_struct *mm, struct fd exe)
+static int __prctl_set_mm_exe_file(struct mm_struct *mm, struct fd exefd)
 {
-	int err;
 	struct file *exe_file;
 
 	/*
+	 * The symlink can be changed only once, just to disallow arbitrary
+	 * transitions malicious software might bring in. This means one
+	 * could make a snapshot over all processes running and monitor
+	 * /proc/pid/exe changes to notice unusual activity if needed.
+	 */
+	if (mm_exe_file_changed(mm))
+		return -EPERM;
+
+	/*
 	 * Forbid mm->exe_file change if old file still mapped.
 	 */
 	exe_file = get_mm_exe_file(mm);
-	err = -EBUSY;
-	down_write(&mm->mmap_sem);
+	if (!exe_file)
+		goto set_file;
+
+	down_read(&mm->mmap_sem);
 	if (exe_file) {
 		struct vm_area_struct *vma;
 
@@ -1669,44 +1679,36 @@ static int __prctl_set_mm_exe_file(struct mm_struct *mm, struct fd exe)
 			if (path_equal(&vma->vm_file->f_path,
 				       &exe_file->f_path)) {
 				fput(exe_file);
-				goto exit_err;
+				up_read(&mm->mmap_sem);
+				return -EBUSY;
 			}
 		}
 
 		fput(exe_file);
 	}
+	up_read(&mm->mmap_sem);
 
+set_file:
 	/*
-	 * The symlink can be changed only once, just to disallow arbitrary
-	 * transitions malicious software might bring in. This means one
-	 * could make a snapshot over all processes running and monitor
-	 * /proc/pid/exe changes to notice unusual activity if needed.
+	 * Recheck the file state again before setting.
+	 * This grabs a reference to exefd.file.
 	 */
-	err = -EPERM;
-	if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
-		goto exit_err;
-
-	up_write(&mm->mmap_sem);
-
-	/* this grabs a reference to exe.file */
-	set_mm_exe_file_locked(mm, exe.file);
-	return 0;
-exit_err:
-	up_write(&mm->mmap_sem);
-	return err;
+	if (test_and_set_mm_exe_file(mm, exefd.file))
+		return 0;
+	return -EPERM;
 }
 
 static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
 {
-	struct fd exe;
+	struct fd exefd;
 	struct inode *inode;
 	int err;
 
-	exe = fdget(fd);
-	if (!exe.file)
+	exefd = fdget(fd);
+	if (!exefd.file)
 		return -EBADF;
 
-	inode = file_inode(exe.file);
+	inode = file_inode(exefd.file);
 
 	/*
 	 * Because the original mm->exe_file points to executable file, make
@@ -1715,16 +1717,16 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
 	 */
 	err = -EACCES;
 	if (!S_ISREG(inode->i_mode)	||
-	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+	    exefd.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
 		goto exit;
 
 	err = inode_permission(inode, MAY_EXEC);
 	if (err)
 		goto exit;
 
-	err = __prctl_set_mm_exe_file(mm, exe);
+	err = __prctl_set_mm_exe_file(mm, exefd);
 exit:
-	fdput(exe);
+	fdput(exefd);
 	return err;
 }
 
-- 
2.1.4


  parent reply	other threads:[~2015-03-14 22:40 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-14 22:39 [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 1/4] " Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 2/4] mm: introduce struct exe_file Davidlohr Bueso
2015-03-14 22:39 ` Davidlohr Bueso [this message]
2015-03-15  2:13   ` [PATCH 3/4] prctl: move MMF_EXE_FILE_CHANGED into exe_file struct Davidlohr Bueso
2015-03-14 22:39 ` [PATCH 4/4] kernel/fork: use pr_alert() for rss counter bugs Davidlohr Bueso
2015-03-16 11:30   ` Konstantin Khlebnikov
2015-03-15 14:21 ` [PATCH -next v2 0/4] mm: replace mmap_sem for mm->exe_file serialization Oleg Nesterov
2015-03-15 14:54   ` Davidlohr Bueso
2015-03-15 15:26     ` Oleg Nesterov
2015-03-15 15:42       ` Davidlohr Bueso
2015-03-15 17:05         ` Cyrill Gorcunov
2015-03-15 17:34           ` Davidlohr Bueso
2015-03-16 22:08           ` Kees Cook
2015-03-20 16:09             ` Cyrill Gorcunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1426372766-3029-4-git-send-email-dave@stgolabs.net \
    --to=dave@stgolabs.net \
    --cc=akpm@linux-foundation.org \
    --cc=dbueso@suse.de \
    --cc=gorcunov@openvz.org \
    --cc=koct9i@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=oleg@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH 3/4] prctl: move MMF_EXE_FILE_CHANGED into exe_file struct' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).