Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Matt Helsley]

I don't see any mention in the changelog of the point brought up by Ingo:

http://lkml.org/lkml/2009/4/10/105

You also haven't responded to my comment about holding mmap
semaphore:

http://lkml.indiana.edu/hypermail/linux/kernel/0904.1/01417.html

Also, please consider combining with Ingo's point with mine: the mmap
semaphore will be held for the duration of that long VMA walk. Plus
any userspace task can trigger that walk as often as they like via
readlink.

It also appears you haven't responded to Eric and Andrew's suggestion
of a struct path in place of a file reference: 

http://lkml.indiana.edu/hypermail/linux/kernel/0904.1/01698.html

Lastly, please keep me on Cc for future revisions of this patch since
I am interested in following changes to the implementation of
/proc/*/exe.

Thanks,
	-Matt Helsley

----- Forwarded message from Alexey Dobriyan <adobriyan@gmail.com> -----

From: Alexey Dobriyan <adobriyan@gmail.com>
To: akpm@linux-foundation.org
Subject: [PATCH 14/38] Remove struct mm_struct::exe_file et al
Date: Fri, 22 May 2009 08:55:08 +0400
Cc: xemul@parallels.com, containers@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, dave@linux.vnet.ibm.com,
        mingo@elte.hu, torvalds@linux-foundation.org,
        Alexey Dobriyan <adobriyan@gmail.com>

Commit 925d1c401fa6cfd0df5d2e37da8981494ccdec07 aka "procfs task exe
symlink".
introduced struct mm_struct::exe_file and struct
mm_struct::num_exe_file_vmas.

The rationale is weak: unifying MMU and no-MMU version of /proc/*/exe
code.
For this a) struct mm_struct becomes bigger, b) mmap/munmap/exit become
slower,
c) patch adds more code than removes in fact.

After commit 8feae13110d60cc6287afabc2887366b0eb226c2 aka
"NOMMU: Make VMAs per MM as for MMU-mode linux" no-MMU kernels also
maintain list of VMAs in ->mmap, so we can switch back for MMU version
of /proc/*/exe.

This also helps C/R, no need to save and restore ->exe_file and to count
additional references to check if there is a leak of struct file outside
group of checkpointed resources.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 fs/exec.c                |    2 -
 fs/proc/base.c           |  105
+++++++++++++---------------------------------
 include/linux/mm.h       |   12 -----
 include/linux/mm_types.h |    6 ---
 include/linux/proc_fs.h  |   20 ---------
 kernel/fork.c            |    3 -
 mm/mmap.c                |   22 ++--------
 mm/nommu.c               |   16 +------
 8 files changed, 36 insertions(+), 150 deletions(-)

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Andrew Morton]

On Tue, 26 May 2009 04:36:18 -0700
Matt Helsley <matthltc@us.ibm.com> wrote:

> I don't see any mention in the changelog of the point brought up by Ingo:
> 
> http://lkml.org/lkml/2009/4/10/105

Nor of Eric's comments.

Alexey, pleeeze don't do this.  We (read: I) heavily depend upon patch
submitters to keep track of outstanding issues and review comments,
etc.

If the patch submitter simply blows these things off then it devolves
to me having to keep track of each patch's issue list as well as the
patch itself.  My workload goes up by a factor of N and the error rate
goes up by N^2 :(

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Alexey Dobriyan]

On Tue, May 26, 2009 at 04:24:15PM -0700, Andrew Morton wrote:
> On Tue, 26 May 2009 04:36:18 -0700
> Matt Helsley <matthltc@us.ibm.com> wrote:
> 
> > I don't see any mention in the changelog of the point brought up by Ingo:
> > 
> > http://lkml.org/lkml/2009/4/10/105
> 
> Nor of Eric's comments.
> 
> Alexey, pleeeze don't do this.  We (read: I) heavily depend upon patch
> submitters to keep track of outstanding issues and review comments,
> etc.
> 
> If the patch submitter simply blows these things off then it devolves
> to me having to keep track of each patch's issue list as well as the
> patch itself.  My workload goes up by a factor of N and the error rate
> goes up by N^2 :(

grmbh..

"Security" and "holding ->mmap_sem" were answered and dismissed.

You can't do readlink(2) on /proc/*/exe if you can't ptrace task.
So no new possible holes are created.

->mmap_sem was held since /proc/*/exe was added and nobody cared.
And, again, you can't readlink _any_ /proc/*/exe.

Patch simply restores code to year-back state.

I'll send removal and readddition of "struct path" as separate things
next time.


And BTW, there is something unnatural when executable path is attached
to mm_struct(!) not task_struct, so yet another argument to ->exe_file
removal.

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Andrew Morton]

On Mon, 1 Jun 2009 01:54:27 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> And BTW, there is something unnatural when executable path is attached
> to mm_struct(!) not task_struct,

mm_struct is the central object for a heavyweight process.  All threads
within that process share the same executable path (don't they?) so
attaching the executable path to the mm seems OK to me.

What I always find a bit weird is that an MM container is used as the
central point for a number of sched obects.  But it's logical, given
that the never-before-stated definition of a heavyweight process is
"thing which share a VM".

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Linus Torvalds]

On Sun, 31 May 2009, Andrew Morton wrote:
> 
> What I always find a bit weird is that an MM container is used as the
> central point for a number of sched obects.  But it's logical, given
> that the never-before-stated definition of a heavyweight process is
> "thing which share a VM".

It has nothing to do with "heavy-weight process" or anything else.

The thing is, from a scheduling standpoint, one of the primary performance 
concerns in the TLB switch.

And there's a 1:1 relationship between TLB switch and MM container, modulo 
the issue of kernel tasks (and those obviously "borrow" approproate MM 
structs to avoid the switch).

So it's not weird at all. It's very direct, and a very straightforward and 
obvious relationship.

			Linus

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Andrew Morton]

On Sun, 31 May 2009 16:15:50 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Sun, 31 May 2009, Andrew Morton wrote:
> > 
> > What I always find a bit weird is that an MM container is used as the
> > central point for a number of sched obects.  But it's logical, given
> > that the never-before-stated definition of a heavyweight process is
> > "thing which share a VM".
> 
> It has nothing to do with "heavy-weight process" or anything else.
> 
> The thing is, from a scheduling standpoint, one of the primary performance 
> concerns in the TLB switch.
> 
> And there's a 1:1 relationship between TLB switch and MM container, modulo 
> the issue of kernel tasks (and those obviously "borrow" approproate MM 
> structs to avoid the switch).

That's all an obscure performance-oriented internal implementation detail.

> So it's not weird at all. It's very direct, and a very straightforward and 
> obvious relationship.

It's arbitrary!  If we were to gain more performance benefit by
aggregating processes under, say, the fs_struct then that's the way the
kernel would have been implemented.

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Linus Torvalds]

On Sun, 31 May 2009, Andrew Morton wrote:
> 
> It's arbitrary!  If we were to gain more performance benefit by
> aggregating processes under, say, the fs_struct then that's the way the
> kernel would have been implemented.

What?

That's like saying that "if quarks were the size of soccer-balls, we'd all 
be pink".

It makes no sense.

When  you claim that it's arbitrary, you're totally ignoring reality, and 
CPU design.

The fact is, TLB's are very fundamental to task switching. filesystems are 
not. It's that simple. IOW, it's not at all arbitrary, it's a direct 
result of how the world works. The "world" for the kernel is the reality 
of CPU's, and in that reality, TLB switching is a huge factor.

There is nothing arbitrary about it at all.

			Linus

[PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Sun, May 31, 2009 at 03:19:53PM -0700, Andrew Morton wrote:
> On Mon, 1 Jun 2009 01:54:27 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > And BTW, there is something unnatural when executable path is attached
> > to mm_struct(!) not task_struct,
> 
> mm_struct is the central object for a heavyweight process.  All threads
> within that process share the same executable path (don't they?) so
> attaching the executable path to the mm seems OK to me.

OK, let's try this:


[PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

->exec_path marks executable which is associated with running task.
Binfmt loader decides which executable is such and can, in theory,
assign anything. Unlike current status quo when first VM_EXECUTABLE mapping is
sort of marks running executable.

If executable unmaps its all VM_EXECUTABLE mappings, /proc/*/exe ceases
to exists, ick! And userpsace can't even use MAP_EXECUTABLE.

Tasks which aren't created by running clone(2) and execve(2)
(read: kernel threads) get empty ->exec_path and

->exec_path is copied on clone(2) and put at do_exit() time.

->exec_path is going to replace struct mm_struct::exe_file et al
and allows to remove VM_EXECUTABLE flag while keeping readlink("/proc/*/exe")
without loop over all VMAs.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 fs/binfmt_aout.c      |    1 +
 fs/binfmt_elf.c       |    1 +
 fs/binfmt_elf_fdpic.c |    1 +
 fs/binfmt_flat.c      |    1 +
 fs/binfmt_som.c       |    1 +
 fs/proc/base.c        |   38 ++++++++++++++------------------------
 include/linux/sched.h |   25 +++++++++++++++++++++++++
 kernel/exit.c         |    1 +
 kernel/fork.c         |    2 ++
 9 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index b639dcf..a19b185 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -379,6 +379,7 @@ beyond_if:
 	regs->gp = ex.a_gpvalue;
 #endif
 	start_thread(regs, ex.a_entry, current->mm->start_stack);
+	set_task_exec_path(current, &bprm->file->f_path);
 	return 0;
 }
 
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 40381df..b815bfc 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -999,6 +999,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 #endif
 
 	start_thread(regs, elf_entry, bprm->p);
+	set_task_exec_path(current, &bprm->file->f_path);
 	retval = 0;
 out:
 	kfree(loc);
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index fdb66fa..f545504 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1185,6 +1185,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 		seg++;
 	}
 
+	set_task_exec_path(current, &file->f_path);
 	return 0;
 }
 
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 697f6b5..a16f977 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -798,6 +798,7 @@ static int load_flat_file(struct linux_binprm * bprm,
 			libinfo->lib_list[id].start_brk) +	/* start brk */
 			stack_len);
 
+	set_task_exec_path(current, &bprm->file->f_path);
 	return 0;
 err:
 	return ret;
diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
index eff74b9..6c56262 100644
--- a/fs/binfmt_som.c
+++ b/fs/binfmt_som.c
@@ -174,6 +174,7 @@ static int map_som_binary(struct file *file,
 	up_write(&current->mm->mmap_sem);
 	if (retval > 0 || retval < -1024)
 		retval = 0;
+	set_task_exec_path(current, &bprm->file->f_path);
 out:
 	set_fs(old_fs);
 	return retval;
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3326bbf..dc4ee6a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -201,6 +201,20 @@ static int proc_root_link(struct inode *inode, struct path *path)
 	return result;
 }
 
+static int proc_exe_link(struct inode *inode, struct path *path)
+{
+	struct task_struct *tsk;
+
+	tsk = get_proc_task(inode);
+	if (!tsk)
+		return -ENOENT;
+	get_task_exec_path(tsk, path);
+	put_task_struct(tsk);
+	if (!path->mnt || !path->dentry)
+		return -ENOENT;
+	return 0;
+}
+
 /*
  * Return zero if current may access user memory in @task, -error if not.
  */
@@ -1302,30 +1316,6 @@ void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm)
 	newmm->exe_file = get_mm_exe_file(oldmm);
 }
 
-static int proc_exe_link(struct inode *inode, struct path *exe_path)
-{
-	struct task_struct *task;
-	struct mm_struct *mm;
-	struct file *exe_file;
-
-	task = get_proc_task(inode);
-	if (!task)
-		return -ENOENT;
-	mm = get_task_mm(task);
-	put_task_struct(task);
-	if (!mm)
-		return -ENOENT;
-	exe_file = get_mm_exe_file(mm);
-	mmput(mm);
-	if (exe_file) {
-		*exe_path = exe_file->f_path;
-		path_get(&exe_file->f_path);
-		fput(exe_file);
-		return 0;
-	} else
-		return -ENOENT;
-}
-
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index b4c38bc..6b2dd01 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1265,6 +1265,12 @@ struct task_struct {
 #endif
 /* CPU-specific state of this task */
 	struct thread_struct thread;
+	/*
+	 * Executable, binfmt loader wants to associate with task
+	 * (read: execve(2) argument).
+	 * Empty, if concept isn't applicable, e. g. kernel thread.
+	 */
+	struct path exec_path;
 /* filesystem information */
 	struct fs_struct *fs;
 /* open file information */
@@ -2403,6 +2409,25 @@ static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 
 #define TASK_STATE_TO_CHAR_STR "RSDTtZX"
 
+static inline void get_task_exec_path(struct task_struct *tsk, struct path *path)
+{
+	task_lock(tsk);
+	path_get(&tsk->exec_path);
+	*path = tsk->exec_path;
+	task_unlock(tsk);
+}
+
+static inline void set_task_exec_path(struct task_struct *tsk, struct path *path)
+{
+	struct path old_path;
+
+	path_get(path);
+	task_lock(tsk);
+	old_path = tsk->exec_path;
+	tsk->exec_path = *path;
+	task_unlock(tsk);
+	path_put(&old_path);
+}
 #endif /* __KERNEL__ */
 
 #endif
diff --git a/kernel/exit.c b/kernel/exit.c
index abf9cf3..8e70b54 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -962,6 +962,7 @@ NORET_TYPE void do_exit(long code)
 
 	exit_sem(tsk);
 	exit_files(tsk);
+	set_task_exec_path(tsk, &(struct path){ .mnt = NULL, .dentry = NULL });
 	exit_fs(tsk);
 	check_stack_usage();
 	exit_thread();
diff --git a/kernel/fork.c b/kernel/fork.c
index b9e2edd..c0ee931 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1191,6 +1191,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	cgroup_fork_callbacks(p);
 	cgroup_callbacks_done = 1;
 
+	get_task_exec_path(current, &p->exec_path);
+
 	/* Need tasklist lock for parent etc handling! */
 	write_lock_irq(&tasklist_lock);
 

[PATCH 2/9] exec_path 2/9: switch audit to ->exec_path

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 kernel/auditsc.c |   23 +++++++----------------
 1 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 7d6ac7c..76829b6 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -53,6 +53,7 @@
 #include <linux/socket.h>
 #include <linux/mqueue.h>
 #include <linux/audit.h>
+#include <linux/path.h>
 #include <linux/personality.h>
 #include <linux/time.h>
 #include <linux/netlink.h>
@@ -949,8 +950,7 @@ EXPORT_SYMBOL(audit_log_task_context);
 static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 {
 	char name[sizeof(tsk->comm)];
-	struct mm_struct *mm = tsk->mm;
-	struct vm_area_struct *vma;
+	struct path exec_path;
 
 	/* tsk == current */
 
@@ -958,20 +958,11 @@ static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk
 	audit_log_format(ab, " comm=");
 	audit_log_untrustedstring(ab, name);
 
-	if (mm) {
-		down_read(&mm->mmap_sem);
-		vma = mm->mmap;
-		while (vma) {
-			if ((vma->vm_flags & VM_EXECUTABLE) &&
-			    vma->vm_file) {
-				audit_log_d_path(ab, "exe=",
-						 &vma->vm_file->f_path);
-				break;
-			}
-			vma = vma->vm_next;
-		}
-		up_read(&mm->mmap_sem);
-	}
+	get_task_exec_path(tsk, &exec_path);
+	if (exec_path.mnt && exec_path.dentry)
+		audit_log_d_path(ab, "exe=", &exec_path);
+	path_put(&exec_path);
+
 	audit_log_task_context(ab);
 }
 

[PATCH 3/9] exec_path 3/9: switch TOMOYO to ->exec_path

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 security/tomoyo/common.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index ddfb9cc..d26deea 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/uaccess.h>
+#include <linux/path.h>
 #include <linux/security.h>
 #include <linux/hardirq.h>
 #include "realpath.h"
@@ -700,20 +701,13 @@ bool tomoyo_io_printf(struct tomoyo_io_buffer *head, const char *fmt, ...)
  */
 static const char *tomoyo_get_exe(void)
 {
-	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma;
+	struct path exec_path;
 	const char *cp = NULL;
 
-	if (!mm)
-		return NULL;
-	down_read(&mm->mmap_sem);
-	for (vma = mm->mmap; vma; vma = vma->vm_next) {
-		if ((vma->vm_flags & VM_EXECUTABLE) && vma->vm_file) {
-			cp = tomoyo_realpath_from_path(&vma->vm_file->f_path);
-			break;
-		}
-	}
-	up_read(&mm->mmap_sem);
+	get_task_exec_path(current, &exec_path);
+	if (exec_path.mnt && exec_path.dentry)
+		cp = tomoyo_realpath_from_path(&exec_path);
+	path_put(&exec_path);
 	return cp;
 }
 

[PATCH 4/9] exec_path 4/9: switch oprofile to ->exec_path

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 drivers/oprofile/buffer_sync.c |   30 ++++++++----------------------
 1 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index 8574622..64c6d7d 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -25,6 +25,7 @@
 #include <linux/workqueue.h>
 #include <linux/notifier.h>
 #include <linux/dcookies.h>
+#include <linux/path.h>
 #include <linux/profile.h>
 #include <linux/module.h>
 #include <linux/fs.h>
@@ -213,30 +214,15 @@ static inline unsigned long fast_get_dcookie(struct path *path)
 	return cookie;
 }
 
-
-/* Look up the dcookie for the task's first VM_EXECUTABLE mapping,
- * which corresponds loosely to "application name". This is
- * not strictly necessary but allows oprofile to associate
- * shared-library samples with particular applications
- */
-static unsigned long get_exec_dcookie(struct mm_struct *mm)
+static unsigned long get_exec_dcookie(struct task_struct *tsk)
 {
 	unsigned long cookie = NO_COOKIE;
-	struct vm_area_struct *vma;
+	struct path exec_path;
 
-	if (!mm)
-		goto out;
-
-	for (vma = mm->mmap; vma; vma = vma->vm_next) {
-		if (!vma->vm_file)
-			continue;
-		if (!(vma->vm_flags & VM_EXECUTABLE))
-			continue;
-		cookie = fast_get_dcookie(&vma->vm_file->f_path);
-		break;
-	}
-
-out:
+	get_task_exec_path(tsk, &exec_path);
+	if (exec_path.mnt && exec_path.dentry)
+		cookie = fast_get_dcookie(&exec_path);
+	path_put(&exec_path);
 	return cookie;
 }
 
@@ -543,7 +529,7 @@ void sync_buffer(int cpu)
 				release_mm(oldmm);
 				mm = take_tasks_mm(new);
 				if (mm != oldmm)
-					cookie = get_exec_dcookie(mm);
+					cookie = get_exec_dcookie(new);
 				add_user_ctx_switch(new, cookie);
 			}
 			if (op_cpu_buffer_get_size(&entry))

[PATCH 5/9] exec_path 5/9: make struct spu_context::owner task_struct

[Alexey Dobriyan]

Cell SPU code is going to use ->exec_path to get dcookies and stuff
so it needs task_struct saved, not mm_struct.

Export __put_task_struct() to allow link.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/powerpc/platforms/cell/spufs/context.c |    9 +++++----
 arch/powerpc/platforms/cell/spufs/file.c    |    2 +-
 arch/powerpc/platforms/cell/spufs/sched.c   |    2 +-
 arch/powerpc/platforms/cell/spufs/spufs.h   |    2 +-
 kernel/fork.c                               |    1 +
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/cell/spufs/context.c b/arch/powerpc/platforms/cell/spufs/context.c
index db5398c..7ad45e1 100644
--- a/arch/powerpc/platforms/cell/spufs/context.c
+++ b/arch/powerpc/platforms/cell/spufs/context.c
@@ -57,7 +57,8 @@ struct spu_context *alloc_spu_context(struct spu_gang *gang)
 	init_waitqueue_head(&ctx->run_wq);
 	ctx->state = SPU_STATE_SAVED;
 	ctx->ops = &spu_backing_ops;
-	ctx->owner = get_task_mm(current);
+	get_task_struct(current);
+	ctx->owner = current;
 	INIT_LIST_HEAD(&ctx->rq);
 	INIT_LIST_HEAD(&ctx->aff_list);
 	if (gang)
@@ -111,7 +112,7 @@ int put_spu_context(struct spu_context *ctx)
 /* give up the mm reference when the context is about to be destroyed */
 void spu_forget(struct spu_context *ctx)
 {
-	struct mm_struct *mm;
+	struct task_struct *tsk;
 
 	/*
 	 * This is basically an open-coded spu_acquire_saved, except that
@@ -122,9 +123,9 @@ void spu_forget(struct spu_context *ctx)
 	if (ctx->state != SPU_STATE_SAVED)
 		spu_deactivate(ctx);
 
-	mm = ctx->owner;
+	tsk = ctx->owner;
 	ctx->owner = NULL;
-	mmput(mm);
+	put_task_struct(tsk);
 	spu_release(ctx);
 }
 
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index d6a519e..d781304 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -1545,7 +1545,7 @@ static int spufs_mfc_open(struct inode *inode, struct file *file)
 	struct spu_context *ctx = i->i_ctx;
 
 	/* we don't want to deal with DMA into other processes */
-	if (ctx->owner != current->mm)
+	if (ctx->owner != current)
 		return -EINVAL;
 
 	if (atomic_read(&inode->i_count) != 1)
diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c
index f085369..9fbd87a 100644
--- a/arch/powerpc/platforms/cell/spufs/sched.c
+++ b/arch/powerpc/platforms/cell/spufs/sched.c
@@ -230,7 +230,7 @@ static void spu_bind_context(struct spu *spu, struct spu_context *ctx)
 	ctx->stats.slb_flt_base = spu->stats.slb_flt;
 	ctx->stats.class2_intr_base = spu->stats.class2_intr;
 
-	spu_associate_mm(spu, ctx->owner);
+	spu_associate_mm(spu, ctx->owner->mm);
 
 	spin_lock_irq(&spu->register_lock);
 	spu->ctx = ctx;
diff --git a/arch/powerpc/platforms/cell/spufs/spufs.h b/arch/powerpc/platforms/cell/spufs/spufs.h
index ae31573..70a7813 100644
--- a/arch/powerpc/platforms/cell/spufs/spufs.h
+++ b/arch/powerpc/platforms/cell/spufs/spufs.h
@@ -95,7 +95,7 @@ struct spu_context {
 	struct mutex state_mutex;
 	struct mutex run_mutex;
 
-	struct mm_struct *owner;
+	struct task_struct *owner;
 
 	struct kref kref;
 	wait_queue_head_t ibox_wq;
diff --git a/kernel/fork.c b/kernel/fork.c
index c0ee931..b396c07 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -160,6 +160,7 @@ void __put_task_struct(struct task_struct *tsk)
 	if (!profile_handoff_task(tsk))
 		free_task(tsk);
 }
+EXPORT_SYMBOL(__put_task_struct);
 
 /*
  * macro override instead of weak attribute alias, to workaround

[PATCH 6/9] exec_path 6/9: add struct spu::tsk

[Alexey Dobriyan]

Pass down task_struct too.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/powerpc/include/asm/spu.h            |    3 ++-
 arch/powerpc/platforms/cell/spu_base.c    |   11 ++++++-----
 arch/powerpc/platforms/cell/spufs/sched.c |    4 ++--
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/spu.h b/arch/powerpc/include/asm/spu.h
index 0ab8d86..f9b1bcf 100644
--- a/arch/powerpc/include/asm/spu.h
+++ b/arch/powerpc/include/asm/spu.h
@@ -135,6 +135,7 @@ struct spu {
 	u64 class_1_dsisr;
 	size_t ls_size;
 	unsigned int slb_replace;
+	struct task_struct *tsk;
 	struct mm_struct *mm;
 	struct spu_context *ctx;
 	struct spu_runqueue *rq;
@@ -212,7 +213,7 @@ static inline void crash_register_spus(struct list_head *list)
 #endif
 
 extern void spu_invalidate_slbs(struct spu *spu);
-extern void spu_associate_mm(struct spu *spu, struct mm_struct *mm);
+extern void spu_associate_task(struct spu *spu, struct task_struct *tsk);
 int spu_64k_pages_available(void);
 
 /* Calls from the memory management to the SPU */
diff --git a/arch/powerpc/platforms/cell/spu_base.c b/arch/powerpc/platforms/cell/spu_base.c
index 9abd210..46f4a63 100644
--- a/arch/powerpc/platforms/cell/spu_base.c
+++ b/arch/powerpc/platforms/cell/spu_base.c
@@ -117,17 +117,18 @@ static inline void mm_needs_global_tlbie(struct mm_struct *mm)
 	bitmap_fill(cpumask_bits(mm_cpumask(mm)), nr);
 }
 
-void spu_associate_mm(struct spu *spu, struct mm_struct *mm)
+void spu_associate_task(struct spu *spu, struct task_struct *tsk)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&spu_full_list_lock, flags);
-	spu->mm = mm;
+	spu->tsk = tsk;
+	spu->mm = tsk ? tsk->mm : NULL;
 	spin_unlock_irqrestore(&spu_full_list_lock, flags);
-	if (mm)
-		mm_needs_global_tlbie(mm);
+	if (tsk && tsk->mm)
+		mm_needs_global_tlbie(tsk->mm);
 }
-EXPORT_SYMBOL_GPL(spu_associate_mm);
+EXPORT_SYMBOL_GPL(spu_associate_task);
 
 int spu_64k_pages_available(void)
 {
diff --git a/arch/powerpc/platforms/cell/spufs/sched.c b/arch/powerpc/platforms/cell/spufs/sched.c
index 9fbd87a..2f7e497 100644
--- a/arch/powerpc/platforms/cell/spufs/sched.c
+++ b/arch/powerpc/platforms/cell/spufs/sched.c
@@ -230,7 +230,7 @@ static void spu_bind_context(struct spu *spu, struct spu_context *ctx)
 	ctx->stats.slb_flt_base = spu->stats.slb_flt;
 	ctx->stats.class2_intr_base = spu->stats.class2_intr;
 
-	spu_associate_mm(spu, ctx->owner->mm);
+	spu_associate_task(spu, ctx->owner);
 
 	spin_lock_irq(&spu->register_lock);
 	spu->ctx = ctx;
@@ -470,7 +470,7 @@ static void spu_unbind_context(struct spu *spu, struct spu_context *ctx)
 	spu->ctx = NULL;
 	spin_unlock_irq(&spu->register_lock);
 
-	spu_associate_mm(spu, NULL);
+	spu_associate_task(spu, NULL);
 
 	ctx->stats.slb_flt +=
 		(spu->stats.slb_flt - ctx->stats.slb_flt_base);

[PATCH 7/9] exec_path 7/9: switch cell SPU thing to ->exec_path

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 arch/powerpc/oprofile/cell/spu_task_sync.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/oprofile/cell/spu_task_sync.c b/arch/powerpc/oprofile/cell/spu_task_sync.c
index 6b793ae..fe17a0a 100644
--- a/arch/powerpc/oprofile/cell/spu_task_sync.c
+++ b/arch/powerpc/oprofile/cell/spu_task_sync.c
@@ -26,6 +26,7 @@
 #include <linux/notifier.h>
 #include <linux/numa.h>
 #include <linux/oprofile.h>
+#include <linux/path.h>
 #include <linux/spinlock.h>
 #include "pr_util.h"
 
@@ -303,9 +304,8 @@ static inline unsigned long fast_get_dcookie(struct path *path)
 	return cookie;
 }
 
-/* Look up the dcookie for the task's first VM_EXECUTABLE mapping,
- * which corresponds loosely to "application name". Also, determine
- * the offset for the SPU ELF object.  If computed offset is
+/*
+ * Determine the offset for the SPU ELF object.  If computed offset is
  * non-zero, it implies an embedded SPU object; otherwise, it's a
  * separate SPU binary, in which case we retrieve it's dcookie.
  * For the embedded case, we must determine if SPU ELF is embedded
@@ -320,26 +320,22 @@ get_exec_dcookie_and_offset(struct spu *spu, unsigned int *offsetp,
 {
 	unsigned long app_cookie = 0;
 	unsigned int my_offset = 0;
-	struct file *app = NULL;
 	struct vm_area_struct *vma;
+	struct task_struct *tsk = spu->tsk;
 	struct mm_struct *mm = spu->mm;
+	struct path exec_path;
 
 	if (!mm)
 		goto out;
 
-	down_read(&mm->mmap_sem);
-
-	for (vma = mm->mmap; vma; vma = vma->vm_next) {
-		if (!vma->vm_file)
-			continue;
-		if (!(vma->vm_flags & VM_EXECUTABLE))
-			continue;
-		app_cookie = fast_get_dcookie(&vma->vm_file->f_path);
-		pr_debug("got dcookie for %s\n",
-			 vma->vm_file->f_dentry->d_name.name);
-		app = vma->vm_file;
-		break;
+	get_task_exec_path(tsk, &exec_path);
+	if (exec_path.mnt && exec_path.dentry) {
+		app_cookie = fast_get_dcookie(&exec_path);
+		pr_debug("got dcookie for %s\n", exec_path.dentry->d_name.name);
 	}
+	path_put(&exec_path);
+
+	down_read(&mm->mmap_sem);
 
 	for (vma = mm->mmap; vma; vma = vma->vm_next) {
 		if (vma->vm_start > spu_ref || vma->vm_end <= spu_ref)

[PATCH 8/9] exec_path 8/9: remove ->exe_file et al

[Alexey Dobriyan]

struct mm_struct::exe_file was fully replaced by ->exec_path.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 fs/exec.c                |    2 -
 fs/proc/base.c           |   51 ----------------------------------------------
 include/linux/mm.h       |   12 ----------
 include/linux/mm_types.h |    6 -----
 include/linux/proc_fs.h  |   20 ------------------
 kernel/fork.c            |    3 --
 mm/mmap.c                |   22 +++----------------
 mm/nommu.c               |   16 +------------
 8 files changed, 6 insertions(+), 126 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 895823d..04fbe3e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -938,8 +938,6 @@ int flush_old_exec(struct linux_binprm * bprm)
 	if (retval)
 		goto out;
 
-	set_mm_exe_file(bprm->mm, bprm->file);
-
 	/*
 	 * Release all of the old mmap stuff
 	 */
diff --git a/fs/proc/base.c b/fs/proc/base.c
index dc4ee6a..94b4c48 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1265,57 +1265,6 @@ static const struct file_operations proc_pid_sched_operations = {
 
 #endif
 
-/*
- * We added or removed a vma mapping the executable. The vmas are only mapped
- * during exec and are not mapped with the mmap system call.
- * Callers must hold down_write() on the mm's mmap_sem for these
- */
-void added_exe_file_vma(struct mm_struct *mm)
-{
-	mm->num_exe_file_vmas++;
-}
-
-void removed_exe_file_vma(struct mm_struct *mm)
-{
-	mm->num_exe_file_vmas--;
-	if ((mm->num_exe_file_vmas == 0) && mm->exe_file){
-		fput(mm->exe_file);
-		mm->exe_file = NULL;
-	}
-
-}
-
-void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
-{
-	if (new_exe_file)
-		get_file(new_exe_file);
-	if (mm->exe_file)
-		fput(mm->exe_file);
-	mm->exe_file = new_exe_file;
-	mm->num_exe_file_vmas = 0;
-}
-
-struct file *get_mm_exe_file(struct mm_struct *mm)
-{
-	struct file *exe_file;
-
-	/* We need mmap_sem to protect against races with removal of
-	 * VM_EXECUTABLE vmas */
-	down_read(&mm->mmap_sem);
-	exe_file = mm->exe_file;
-	if (exe_file)
-		get_file(exe_file);
-	up_read(&mm->mmap_sem);
-	return exe_file;
-}
-
-void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm)
-{
-	/* It's safe to write the exe_file pointer without exe_file_lock because
-	 * this is called during fork when the task is not yet in /proc */
-	newmm->exe_file = get_mm_exe_file(oldmm);
-}
-
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index bff1f0d..b3b61a6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1121,18 +1121,6 @@ extern void exit_mmap(struct mm_struct *);
 extern int mm_take_all_locks(struct mm_struct *mm);
 extern void mm_drop_all_locks(struct mm_struct *mm);
 
-#ifdef CONFIG_PROC_FS
-/* From fs/proc/base.c. callers must _not_ hold the mm's exe_file_lock */
-extern void added_exe_file_vma(struct mm_struct *mm);
-extern void removed_exe_file_vma(struct mm_struct *mm);
-#else
-static inline void added_exe_file_vma(struct mm_struct *mm)
-{}
-
-static inline void removed_exe_file_vma(struct mm_struct *mm)
-{}
-#endif /* CONFIG_PROC_FS */
-
 extern int may_expand_vm(struct mm_struct *mm, unsigned long npages);
 extern int install_special_mapping(struct mm_struct *mm,
 				   unsigned long addr, unsigned long len,
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 0e80e26..90786ea 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -269,12 +269,6 @@ struct mm_struct {
 	 */
 	struct task_struct *owner;
 #endif
-
-#ifdef CONFIG_PROC_FS
-	/* store ref to file /proc/<pid>/exe symlink points to */
-	struct file *exe_file;
-	unsigned long num_exe_file_vmas;
-#endif
 #ifdef CONFIG_MMU_NOTIFIER
 	struct mmu_notifier_mm *mmu_notifier_mm;
 #endif
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index fbfa3d4..64ed076 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -188,12 +188,6 @@ extern void proc_net_remove(struct net *net, const char *name);
 extern struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
 	struct proc_dir_entry *parent);
 
-/* While the {get|set|dup}_mm_exe_file functions are for mm_structs, they are
- * only needed to implement /proc/<pid>|self/exe so we define them here. */
-extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
-extern struct file *get_mm_exe_file(struct mm_struct *mm);
-extern void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm);
-
 #else
 
 #define proc_net_fops_create(net, name, mode, fops)  ({ (void)(mode), NULL; })
@@ -240,20 +234,6 @@ static inline int pid_ns_prepare_proc(struct pid_namespace *ns)
 static inline void pid_ns_release_proc(struct pid_namespace *ns)
 {
 }
-
-static inline void set_mm_exe_file(struct mm_struct *mm,
-				   struct file *new_exe_file)
-{}
-
-static inline struct file *get_mm_exe_file(struct mm_struct *mm)
-{
-	return NULL;
-}
-
-static inline void dup_mm_exe_file(struct mm_struct *oldmm,
-	       			   struct mm_struct *newmm)
-{}
-
 #endif /* CONFIG_PROC_FS */
 
 #if !defined(CONFIG_PROC_KCORE)
diff --git a/kernel/fork.c b/kernel/fork.c
index b396c07..1cd1a14 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -489,7 +489,6 @@ void mmput(struct mm_struct *mm)
 	if (atomic_dec_and_test(&mm->mm_users)) {
 		exit_aio(mm);
 		exit_mmap(mm);
-		set_mm_exe_file(mm, NULL);
 		if (!list_empty(&mm->mmlist)) {
 			spin_lock(&mmlist_lock);
 			list_del(&mm->mmlist);
@@ -612,8 +611,6 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
 	if (init_new_context(tsk, mm))
 		goto fail_nocontext;
 
-	dup_mm_exe_file(oldmm, mm);
-
 	err = dup_mmap(mm, oldmm);
 	if (err)
 		goto free_pt;
diff --git a/mm/mmap.c b/mm/mmap.c
index 6b7b1a9..76faabc 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -232,11 +232,8 @@ static struct vm_area_struct *remove_vma(struct vm_area_struct *vma)
 	might_sleep();
 	if (vma->vm_ops && vma->vm_ops->close)
 		vma->vm_ops->close(vma);
-	if (vma->vm_file) {
+	if (vma->vm_file)
 		fput(vma->vm_file);
-		if (vma->vm_flags & VM_EXECUTABLE)
-			removed_exe_file_vma(vma->vm_mm);
-	}
 	mpol_put(vma_policy(vma));
 	kmem_cache_free(vm_area_cachep, vma);
 	return next;
@@ -633,11 +630,8 @@ again:			remove_next = 1 + (end > next->vm_end);
 		spin_unlock(&mapping->i_mmap_lock);
 
 	if (remove_next) {
-		if (file) {
+		if (file)
 			fput(file);
-			if (next->vm_flags & VM_EXECUTABLE)
-				removed_exe_file_vma(mm);
-		}
 		mm->map_count--;
 		mpol_put(vma_policy(next));
 		kmem_cache_free(vm_area_cachep, next);
@@ -1192,8 +1186,6 @@ munmap_back:
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
-		if (vm_flags & VM_EXECUTABLE)
-			added_exe_file_vma(mm);
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
 		if (error)
@@ -1849,11 +1841,8 @@ int split_vma(struct mm_struct * mm, struct vm_area_struct * vma,
 	}
 	vma_set_policy(new, pol);
 
-	if (new->vm_file) {
+	if (new->vm_file)
 		get_file(new->vm_file);
-		if (vma->vm_flags & VM_EXECUTABLE)
-			added_exe_file_vma(mm);
-	}
 
 	if (new->vm_ops && new->vm_ops->open)
 		new->vm_ops->open(new);
@@ -2200,11 +2189,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 			new_vma->vm_start = addr;
 			new_vma->vm_end = addr + len;
 			new_vma->vm_pgoff = pgoff;
-			if (new_vma->vm_file) {
+			if (new_vma->vm_file)
 				get_file(new_vma->vm_file);
-				if (vma->vm_flags & VM_EXECUTABLE)
-					added_exe_file_vma(mm);
-			}
 			if (new_vma->vm_ops && new_vma->vm_ops->open)
 				new_vma->vm_ops->open(new_vma);
 			vma_link(mm, new_vma, prev, rb_link, rb_parent);
diff --git a/mm/nommu.c b/mm/nommu.c
index b571ef7..352aac5 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -721,11 +721,8 @@ static void delete_vma(struct mm_struct *mm, struct vm_area_struct *vma)
 	kenter("%p", vma);
 	if (vma->vm_ops && vma->vm_ops->close)
 		vma->vm_ops->close(vma);
-	if (vma->vm_file) {
+	if (vma->vm_file)
 		fput(vma->vm_file);
-		if (vma->vm_flags & VM_EXECUTABLE)
-			removed_exe_file_vma(mm);
-	}
 	put_nommu_region(vma->vm_region);
 	kmem_cache_free(vm_area_cachep, vma);
 }
@@ -1218,10 +1215,6 @@ unsigned long do_mmap_pgoff(struct file *file,
 		get_file(file);
 		vma->vm_file = file;
 		get_file(file);
-		if (vm_flags & VM_EXECUTABLE) {
-			added_exe_file_vma(current->mm);
-			vma->vm_mm = current->mm;
-		}
 	}
 
 	down_write(&nommu_region_sem);
@@ -1359,11 +1352,8 @@ share:
 error_put_region:
 	__put_nommu_region(region);
 	if (vma) {
-		if (vma->vm_file) {
+		if (vma->vm_file)
 			fput(vma->vm_file);
-			if (vma->vm_flags & VM_EXECUTABLE)
-				removed_exe_file_vma(vma->vm_mm);
-		}
 		kmem_cache_free(vm_area_cachep, vma);
 	}
 	kleave(" = %d [pr]", ret);
@@ -1375,8 +1365,6 @@ error:
 	fput(region->vm_file);
 	kmem_cache_free(vm_region_jar, region);
 	fput(vma->vm_file);
-	if (vma->vm_flags & VM_EXECUTABLE)
-		removed_exe_file_vma(vma->vm_mm);
 	kmem_cache_free(vm_area_cachep, vma);
 	kleave(" = %d", ret);
 	return ret;

[PATCH 9/9] exec_path 9/9: remove VM_EXECUTABLE

[Alexey Dobriyan]

Noone uses VM_EXECUTABLE now, binfmt loaders set ->exec_path by hand
and MAP_EXECUTABLE is ignored from userland.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 include/linux/mm.h   |    1 -
 include/linux/mman.h |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b3b61a6..79854af 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -82,7 +82,6 @@ extern unsigned int kobjsize(const void *objp);
 #define VM_PFNMAP	0x00000400	/* Page-ranges managed without "struct page", just pure PFN */
 #define VM_DENYWRITE	0x00000800	/* ETXTBSY on write attempts.. */
 
-#define VM_EXECUTABLE	0x00001000
 #define VM_LOCKED	0x00002000
 #define VM_IO           0x00004000	/* Memory mapped I/O or similar */
 
diff --git a/include/linux/mman.h b/include/linux/mman.h
index 9872d6c..1a01871 100644
--- a/include/linux/mman.h
+++ b/include/linux/mman.h
@@ -86,7 +86,6 @@ calc_vm_flag_bits(unsigned long flags)
 {
 	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
 	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
-	       _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
 	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    );
 }
 #endif /* __KERNEL__ */

Re: [PATCH 9/9] exec_path 9/9: remove VM_EXECUTABLE

[Matt Helsley]

On Thu, Jun 04, 2009 at 03:08:10AM +0400, Alexey Dobriyan wrote:
> Noone uses VM_EXECUTABLE now, binfmt loaders set ->exec_path by hand
> and MAP_EXECUTABLE is ignored from userland.

nit: MAP_EXECUTABLE is still used in the binfmt loaders when they
supply flags to do_mmap(). That needs to be removed before getting
rid of VM_EXECUTABLE.

Cheers,
	-Matt Helsley

> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>  include/linux/mm.h   |    1 -
>  include/linux/mman.h |    1 -
>  2 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b3b61a6..79854af 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -82,7 +82,6 @@ extern unsigned int kobjsize(const void *objp);
>  #define VM_PFNMAP	0x00000400	/* Page-ranges managed without "struct page", just pure PFN */
>  #define VM_DENYWRITE	0x00000800	/* ETXTBSY on write attempts.. */
> 
> -#define VM_EXECUTABLE	0x00001000
>  #define VM_LOCKED	0x00002000
>  #define VM_IO           0x00004000	/* Memory mapped I/O or similar */
> 
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index 9872d6c..1a01871 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -86,7 +86,6 @@ calc_vm_flag_bits(unsigned long flags)
>  {
>  	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
>  	       _calc_vm_trans(flags, MAP_DENYWRITE,  VM_DENYWRITE ) |
> -	       _calc_vm_trans(flags, MAP_EXECUTABLE, VM_EXECUTABLE) |
>  	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    );
>  }
>  #endif /* __KERNEL__ */

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Linus Torvalds]

On Thu, 4 Jun 2009, Alexey Dobriyan wrote:
> 
> [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

Ok, looks reasonable to me, and seems to remove more code than it adds, 
and makes even that smaller remaining code more readable.

That said, I suspect there are user-space loaders etc that set 
VM_EXECUTABLE by hand when mmap'ing the main executable. Will this get 
things right? Or do we even allow that?

Can you also add more commentary about the fundamental reasons you want to 
do this, rather than talking just about the implementation issues.

			Linus

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Matt Helsley]

On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> On Sun, May 31, 2009 at 03:19:53PM -0700, Andrew Morton wrote:
> > On Mon, 1 Jun 2009 01:54:27 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > > And BTW, there is something unnatural when executable path is attached
> > > to mm_struct(!) not task_struct,
> > 
> > mm_struct is the central object for a heavyweight process.  All threads
> > within that process share the same executable path (don't they?) so
> > attaching the executable path to the mm seems OK to me.
> 
> OK, let's try this:
> 
> 
> [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe
> 
> ->exec_path marks executable which is associated with running task.
> Binfmt loader decides which executable is such and can, in theory,
> assign anything. Unlike current status quo when first VM_EXECUTABLE mapping is
> sort of marks running executable.
> 
> If executable unmaps its all VM_EXECUTABLE mappings, /proc/*/exe ceases
> to exists, ick! And userpsace can't even use MAP_EXECUTABLE.

Suprising but intentional and unavoidable. More below..

> 
> Tasks which aren't created by running clone(2) and execve(2)
> (read: kernel threads) get empty ->exec_path and
> 
> ->exec_path is copied on clone(2) and put at do_exit() time.

Doesn't this pin the vfs mount of the executable for the lifetime of
the task?
 
That was one of Al Viro's objections to early revisions of the exe_file
patches. It's the reason the exe_file patches kept track of the number of
VM_EXECUTABLE mappings with num_exe_file_vmas.

I've cc'd Al so he can confirm/deny my recollection of this. Basically
some programs need to be able to umount the filesystem that back their
executables. Being able to unmap these regions was effectively a
userspace API for unpinning these mounts. I needed to preserve that API,
hence the VMA ugliness of exe_file that you object to with the exe_file
patches.

I think patches 2-7 look great and could be adapted to use exe_file instead
of ->exec_path.

Cheers,
	-Matt Helsley

> 
> ->exec_path is going to replace struct mm_struct::exe_file et al
> and allows to remove VM_EXECUTABLE flag while keeping readlink("/proc/*/exe")
> without loop over all VMAs.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>  fs/binfmt_aout.c      |    1 +
>  fs/binfmt_elf.c       |    1 +
>  fs/binfmt_elf_fdpic.c |    1 +
>  fs/binfmt_flat.c      |    1 +
>  fs/binfmt_som.c       |    1 +
>  fs/proc/base.c        |   38 ++++++++++++++------------------------
>  include/linux/sched.h |   25 +++++++++++++++++++++++++
>  kernel/exit.c         |    1 +
>  kernel/fork.c         |    2 ++
>  9 files changed, 47 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
> index b639dcf..a19b185 100644
> --- a/fs/binfmt_aout.c
> +++ b/fs/binfmt_aout.c
> @@ -379,6 +379,7 @@ beyond_if:
>  	regs->gp = ex.a_gpvalue;
>  #endif
>  	start_thread(regs, ex.a_entry, current->mm->start_stack);
> +	set_task_exec_path(current, &bprm->file->f_path);
>  	return 0;
>  }
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 40381df..b815bfc 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -999,6 +999,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>  #endif
> 
>  	start_thread(regs, elf_entry, bprm->p);
> +	set_task_exec_path(current, &bprm->file->f_path);
>  	retval = 0;
>  out:
>  	kfree(loc);
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index fdb66fa..f545504 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -1185,6 +1185,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>  		seg++;
>  	}
> 
> +	set_task_exec_path(current, &file->f_path);
>  	return 0;
>  }
> 
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 697f6b5..a16f977 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -798,6 +798,7 @@ static int load_flat_file(struct linux_binprm * bprm,
>  			libinfo->lib_list[id].start_brk) +	/* start brk */
>  			stack_len);
> 
> +	set_task_exec_path(current, &bprm->file->f_path);
>  	return 0;
>  err:
>  	return ret;
> diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
> index eff74b9..6c56262 100644
> --- a/fs/binfmt_som.c
> +++ b/fs/binfmt_som.c
> @@ -174,6 +174,7 @@ static int map_som_binary(struct file *file,
>  	up_write(&current->mm->mmap_sem);
>  	if (retval > 0 || retval < -1024)
>  		retval = 0;
> +	set_task_exec_path(current, &bprm->file->f_path);
>  out:
>  	set_fs(old_fs);
>  	return retval;
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 3326bbf..dc4ee6a 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -201,6 +201,20 @@ static int proc_root_link(struct inode *inode, struct path *path)
>  	return result;
>  }
> 
> +static int proc_exe_link(struct inode *inode, struct path *path)
> +{
> +	struct task_struct *tsk;
> +
> +	tsk = get_proc_task(inode);
> +	if (!tsk)
> +		return -ENOENT;
> +	get_task_exec_path(tsk, path);
> +	put_task_struct(tsk);
> +	if (!path->mnt || !path->dentry)
> +		return -ENOENT;
> +	return 0;
> +}
> +
>  /*
>   * Return zero if current may access user memory in @task, -error if not.
>   */
> @@ -1302,30 +1316,6 @@ void dup_mm_exe_file(struct mm_struct *oldmm, struct mm_struct *newmm)
>  	newmm->exe_file = get_mm_exe_file(oldmm);
>  }
> 
> -static int proc_exe_link(struct inode *inode, struct path *exe_path)
> -{
> -	struct task_struct *task;
> -	struct mm_struct *mm;
> -	struct file *exe_file;
> -
> -	task = get_proc_task(inode);
> -	if (!task)
> -		return -ENOENT;
> -	mm = get_task_mm(task);
> -	put_task_struct(task);
> -	if (!mm)
> -		return -ENOENT;
> -	exe_file = get_mm_exe_file(mm);
> -	mmput(mm);
> -	if (exe_file) {
> -		*exe_path = exe_file->f_path;
> -		path_get(&exe_file->f_path);
> -		fput(exe_file);
> -		return 0;
> -	} else
> -		return -ENOENT;
> -}
> -
>  static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
>  {
>  	struct inode *inode = dentry->d_inode;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b4c38bc..6b2dd01 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1265,6 +1265,12 @@ struct task_struct {
>  #endif
>  /* CPU-specific state of this task */
>  	struct thread_struct thread;
> +	/*
> +	 * Executable, binfmt loader wants to associate with task
> +	 * (read: execve(2) argument).
> +	 * Empty, if concept isn't applicable, e. g. kernel thread.
> +	 */
> +	struct path exec_path;
>  /* filesystem information */
>  	struct fs_struct *fs;
>  /* open file information */
> @@ -2403,6 +2409,25 @@ static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
> 
>  #define TASK_STATE_TO_CHAR_STR "RSDTtZX"
> 
> +static inline void get_task_exec_path(struct task_struct *tsk, struct path *path)
> +{
> +	task_lock(tsk);
> +	path_get(&tsk->exec_path);
> +	*path = tsk->exec_path;
> +	task_unlock(tsk);
> +}
> +
> +static inline void set_task_exec_path(struct task_struct *tsk, struct path *path)
> +{
> +	struct path old_path;
> +
> +	path_get(path);
> +	task_lock(tsk);
> +	old_path = tsk->exec_path;
> +	tsk->exec_path = *path;
> +	task_unlock(tsk);
> +	path_put(&old_path);
> +}
>  #endif /* __KERNEL__ */
> 
>  #endif
> diff --git a/kernel/exit.c b/kernel/exit.c
> index abf9cf3..8e70b54 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -962,6 +962,7 @@ NORET_TYPE void do_exit(long code)
> 
>  	exit_sem(tsk);
>  	exit_files(tsk);
> +	set_task_exec_path(tsk, &(struct path){ .mnt = NULL, .dentry = NULL });
>  	exit_fs(tsk);
>  	check_stack_usage();
>  	exit_thread();
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b9e2edd..c0ee931 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1191,6 +1191,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	cgroup_fork_callbacks(p);
>  	cgroup_callbacks_done = 1;
> 
> +	get_task_exec_path(current, &p->exec_path);
> +
>  	/* Need tasklist lock for parent etc handling! */
>  	write_lock_irq(&tasklist_lock);
> 

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Matt Helsley]

On Thu, Jun 04, 2009 at 12:55:32AM -0700, Matt Helsley wrote:
> On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> > On Sun, May 31, 2009 at 03:19:53PM -0700, Andrew Morton wrote:
> > > On Mon, 1 Jun 2009 01:54:27 +0400 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > And BTW, there is something unnatural when executable path is attached
> > > > to mm_struct(!) not task_struct,
> > > 
> > > mm_struct is the central object for a heavyweight process.  All threads
> > > within that process share the same executable path (don't they?) so
> > > attaching the executable path to the mm seems OK to me.
> > 
> > OK, let's try this:
> > 
> > 
> > [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe
> > 
> > ->exec_path marks executable which is associated with running task.
> > Binfmt loader decides which executable is such and can, in theory,
> > assign anything. Unlike current status quo when first VM_EXECUTABLE mapping is
> > sort of marks running executable.
> > 
> > If executable unmaps its all VM_EXECUTABLE mappings, /proc/*/exe ceases
> > to exists, ick! And userpsace can't even use MAP_EXECUTABLE.
> 
> Suprising but intentional and unavoidable. More below..
> 
> > 
> > Tasks which aren't created by running clone(2) and execve(2)
> > (read: kernel threads) get empty ->exec_path and
> > 
> > ->exec_path is copied on clone(2) and put at do_exit() time.
> 
> Doesn't this pin the vfs mount of the executable for the lifetime of
> the task?
> 
> That was one of Al Viro's objections to early revisions of the exe_file
> patches. It's the reason the exe_file patches kept track of the number of
> VM_EXECUTABLE mappings with num_exe_file_vmas.
> 
> I've cc'd Al so he can confirm/deny my recollection of this. Basically
> some programs need to be able to umount the filesystem that back their
> executables. Being able to unmap these regions was effectively a
> userspace API for unpinning these mounts. I needed to preserve that API,
> hence the VMA ugliness of exe_file that you object to with the exe_file
> patches.
> 
> I think patches 2-7 look great and could be adapted to use exe_file instead
> of ->exec_path.

Well, except 5 and 6.

Alternately, I think you could use the same VMA code with ->exec_path to avoid
pinning the vfs mount. However, then it would probably be best to move
it into the mm just like exe_file...

> 
> Cheers,
> 	-Matt Helsley

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Linus Torvalds]

On Thu, 4 Jun 2009, Matt Helsley wrote:
> 
> Doesn't this pin the vfs mount of the executable for the lifetime of
> the task?

Well, yes, but so does the current code. 

Sure, in _theory_ it can be a non-mmap executable (maybe people still have 
those old OMAGIC a.out executables), and in _theory_ you could unmap the 
executable even if it was originally mmap'ed, but neither of those is 
exactly common, are they?

So in practice, nothing has changed wrt lifetime of the executable.

			Linus

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Matt Helsley]

On Thu, Jun 04, 2009 at 08:07:23AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 4 Jun 2009, Matt Helsley wrote:
> > 
> > Doesn't this pin the vfs mount of the executable for the lifetime of
> > the task?
> 
> Well, yes, but so does the current code.

Not quite. The current code pins it as long as the corresponding VMAs
are mapped -- not for the lifetime of the task.

> Sure, in _theory_ it can be a non-mmap executable (maybe people still have 
> those old OMAGIC a.out executables), and in _theory_ you could unmap the 
> executable even if it was originally mmap'ed, but neither of those is 
> exactly common, are they?

Not common to my knowledge, no.

> 
> So in practice, nothing has changed wrt lifetime of the executable.

Almost all of the time, yes.

Cheers,
	-Matt Helsley

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Thu, Jun 04, 2009 at 02:30:33PM -0700, Matt Helsley wrote:
> On Thu, Jun 04, 2009 at 08:07:23AM -0700, Linus Torvalds wrote:
> > On Thu, 4 Jun 2009, Matt Helsley wrote:
> > > 
> > > Doesn't this pin the vfs mount of the executable for the lifetime of
> > > the task?
> > 
> > Well, yes, but so does the current code.
> 
> Not quite. The current code pins it as long as the corresponding VMAs
> are mapped -- not for the lifetime of the task.
> 
> > Sure, in _theory_ it can be a non-mmap executable (maybe people still have 
> > those old OMAGIC a.out executables), and in _theory_ you could unmap the 
> > executable even if it was originally mmap'ed, but neither of those is 
> > exactly common, are they?
> 
> Not common to my knowledge, no.
> 
> > 
> > So in practice, nothing has changed wrt lifetime of the executable.
> 
> Almost all of the time, yes.

And year ago executable wasn't pinned at all, so if you're opposing
widening of time executable is pinned, you should revert your own patch
which introduced it in first place.

->exec_path merely makes /proc/*/exe very unheuristical (binfmt loader
decides, nothing else) and suitable for other code as demonstrated.

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Matt Helsley]

On Thu, Jun 04, 2009, Alexey Dobriyan wrote:
> On Thu, Jun 04, 2009 at 02:30:33PM -0700, Matt Helsley wrote:
> > On Thu, Jun 04, 2009 at 08:07:23AM -0700, Linus Torvalds wrote:
> > > On Thu, 4 Jun 2009, Matt Helsley wrote:
> > > > 
> > > > Doesn't this pin the vfs mount of the executable for the lifetime
> > > > of
> > > > the task?
> > > 
> > > Well, yes, but so does the current code.
> > 
> > Not quite. The current code pins it as long as the corresponding VMAs
> > are mapped -- not for the lifetime of the task.
> > 
> > > Sure, in _theory_ it can be a non-mmap executable (maybe people
> > > still have 
> > > those old OMAGIC a.out executables), and in _theory_ you could unmap
> > > the 
> > > executable even if it was originally mmap'ed, but neither of those
> > > is 
> > > exactly common, are they?
> > 
> > Not common to my knowledge, no.
> > 
> > > 
> > > So in practice, nothing has changed wrt lifetime of the executable.
> > 
> > Almost all of the time, yes.
> 
> And year ago executable wasn't pinned at all, so if you're opposing
> widening of time executable is pinned, you should revert your own patch
> which introduced it in first place.

You're wrong -- my patch did not widen the time the executable is pinned.
Read the exe_file changelog and give the code a more thorough read:

	[The exe_file] reference would prevent the filesystem holding the executable
	file from being unmounted even after unmapping the VMAs. So we track the
	number of VM_EXECUTABLE VMAs and drop the new reference when the last one is
	unmapped. This avoids pinning the mounted filesystem.

So long as the VMAs are mapped the filesystem is pinned. Until these VMAs are
unmapped exe_file does not extend the time that the filesystem is pinned. So
dropping the exe_file reference when the last VMA goes away should suffice in
preserving this property of the old VMA walk. The code does this by:

When exe_file is set from the bprm during exec we also set num_exe_file_vmas to 0.

When a VMA with VM_EXECUTABLE is mapped we call added_exe_file_vma() since
MAP_EXECUTABLE is only used for this (binfmt_* do_mmap() calls pass MAP_EXECUTABLE).
added_exe_file_vma() increments num_exe_file_vmas.

When a VM_EXECUTABLE VMA is split in two we call added_exe_file_vma() since there
is one more VMA.

When two VM_EXECUTABLE VMAs are merged we call removed_exe_file_vma() since there
is one less VMA. See removed_exe_file_vma() below.

When a VM_EXECUTABLE VMA is unmapped we call removed_exe_file_vma() since there
is one less VMA. When the last VM_EXECUTABLE VMA is unmapped num_exe_file_vmas 
drops to 0 and we trigger fput(exe_file):

void removed_exe_file_vma(struct mm_struct *mm)
{
       mm->num_exe_file_vmas--;
       if ((mm->num_exe_file_vmas == 0) && mm->exe_file){
               fput(mm->exe_file);
               mm->exe_file = NULL;
       }
}

which avoids pinning the VFS mount after the last VM_EXECUTABLE VMA is unmapped.

This is what used to happen with the old VMA-walk code too -- when the last
VM_EXECUTABLE VMA was unmapped the /proc/*/exe link disappeared  (as you've noted)
and the vfs mount was no longer pinned by the /proc/*/exe code.

In case you're curious here's what Al said when my original patch lacked this code
and hence ignored mount pinning:

	http://lkml.org/lkml/2007/7/12/398

So I added the VMA tracking code.

Since Linus seems to think vfs mount pinning is inconsequential
I don't mind dropping this objection. Please feel free to add my:

Acked-by: Matt Helsley <matthltc@us.ibm.com>

to this entire series.

> 
> ->exec_path merely makes /proc/*/exe very unheuristical (binfmt loader
> decides, nothing else) and suitable for other code as demonstrated.

Yes, the VMA walk that exe_file replaced was a bit of a heuristic.

Cheers,
	-Matt Helsley

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Christoph Hellwig]

On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> ->exec_path is copied on clone(2) and put at do_exit() time.
> 
> ->exec_path is going to replace struct mm_struct::exe_file et al
> and allows to remove VM_EXECUTABLE flag while keeping readlink("/proc/*/exe")
> without loop over all VMAs.

Why don't you leave it in mm_struct?  That'll avoid having to mess with
it in clone, and given that exec always replaces the whole VM it's a
more natural fit anyway.

But yeah, a path seems much more suitable than keeping a file.

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Linus Torvalds]

On Fri, 5 Jun 2009, Christoph Hellwig wrote:
>
> On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> > ->exec_path is copied on clone(2) and put at do_exit() time.
> > 
> > ->exec_path is going to replace struct mm_struct::exe_file et al
> > and allows to remove VM_EXECUTABLE flag while keeping readlink("/proc/*/exe")
> > without loop over all VMAs.
> 
> Why don't you leave it in mm_struct?  That'll avoid having to mess with
> it in clone, and given that exec always replaces the whole VM it's a
> more natural fit anyway.

Oh, I didn't even notice that, because I just assumed it was in mm_struct 
already due to the earlier discussion.

So I concur with Christoph - that field should be in the mm_struct. The 
executable is a "mapping" issue, and goes along with task->mm, and should 
be there, not in task_struct.

So it should be copied at fork() time when you do the dup_mm(), not 
anywhere else.

		Linus

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Fri, Jun 05, 2009 at 08:10:50AM -0700, Linus Torvalds wrote:
> On Fri, 5 Jun 2009, Christoph Hellwig wrote:
> >
> > On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> > > ->exec_path is copied on clone(2) and put at do_exit() time.
> > > 
> > > ->exec_path is going to replace struct mm_struct::exe_file et al
> > > and allows to remove VM_EXECUTABLE flag while keeping readlink("/proc/*/exe")
> > > without loop over all VMAs.
> > 
> > Why don't you leave it in mm_struct?  That'll avoid having to mess with
> > it in clone,

There is no mess in clone:

	+       get_task_exec_path(current, &p->exec_path);

and there is no mess in exit:

	+       set_task_exec_path(tsk, &(struct path){ .mnt = NULL, .dentry = NULL });

> > and given that exec always replaces the whole VM it's a more natural fit anyway.
> 
> Oh, I didn't even notice that, because I just assumed it was in mm_struct 
> already due to the earlier discussion.
> 
> So I concur with Christoph - that field should be in the mm_struct. The 
> executable is a "mapping" issue,

This mapping issue is what created VM_EXECUTABLE/MAP_EXECUTABLE in
the first place, I assume? Never mind it's cheap hack.

> and goes along with task->mm, and should be there, not in task_struct.

Because ->mm can be borrowed to unrelated task and user has to check
for it.

> So it should be copied at fork() time when you do the dup_mm(), not 
> anywhere else.

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Linus Torvalds]

On Fri, 5 Jun 2009, Alexey Dobriyan wrote:
> 
> This mapping issue is what created VM_EXECUTABLE/MAP_EXECUTABLE in
> the first place, I assume? Never mind it's cheap hack.
> 
> > and goes along with task->mm, and should be there, not in task_struct.
> 
> Because ->mm can be borrowed to unrelated task and user has to check
> for it.

Not it can't.

You're confusing ->mm with ->active_mm.

The latter can be borrowed. The former can not.

The fact is, the executable is _inherently_ tied to the mm. It's what it 
is mapped into. It makes no sense to tie it to anything else. It's simply 
fundamentally not a "per-thread" thing.

		Linus

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Fri, Jun 05, 2009 at 08:49:56AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 5 Jun 2009, Alexey Dobriyan wrote:
> > 
> > This mapping issue is what created VM_EXECUTABLE/MAP_EXECUTABLE in
> > the first place, I assume? Never mind it's cheap hack.
> > 
> > > and goes along with task->mm, and should be there, not in task_struct.
> > 
> > Because ->mm can be borrowed to unrelated task and user has to check
> > for it.
> 
> Not it can't.
> 
> You're confusing ->mm with ->active_mm.
> 
> The latter can be borrowed. The former can not.

Not permanently, but it can:

	static void aio_kick_handler(struct work_struct *work)
	{
	        struct kioctx *ctx = container_of(work, struct kioctx, wq.work);
	        mm_segment_t oldfs = get_fs();
	        struct mm_struct *mm;
	        int requeue;
	
	        set_fs(USER_DS);
	        use_mm(ctx->mm);
	        spin_lock_irq(&ctx->ctx_lock);
	        requeue =__aio_run_iocbs(ctx);
	        mm = ctx->mm;
	        spin_unlock_irq(&ctx->ctx_lock);
	        unuse_mm(mm);
	        set_fs(oldfs);
	        /*
	         * we're in a worker thread already, don't use
	         * queue_delayed_work,
	         */
	        if (requeue)
	                queue_delayed_work(aio_wq, &ctx->wq, 0);
	}

It's borrowed by kernel thread of course, not userspace task.

> The fact is, the executable is _inherently_ tied to the mm. It's what it 
> is mapped into. It makes no sense to tie it to anything else. It's simply 
> fundamentally not a "per-thread" thing.

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Linus Torvalds]

On Fri, 5 Jun 2009, Alexey Dobriyan wrote:
> 
> It's borrowed by kernel thread of course, not userspace task.

.. and even if it is, what's the problem? 

That kernel thread has borroed whe VM for a while. It effectively _is_ a 
thread of the process now. So it's technically not even wrong to 
explicitly allow things like /proc/*/exe to see it as such.

But you can hide it by just checking some flag in the thread structure if 
you really want to.

But when creating a regular thread, you should _not_ need to take a 
spinlock and duplicate the executable path!

		Linus

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Fri, Jun 05, 2009 at 09:48:02AM -0700, Linus Torvalds wrote:
> On Fri, 5 Jun 2009, Alexey Dobriyan wrote:
> > 
> > It's borrowed by kernel thread of course, not userspace task.
> 
> .. and even if it is, what's the problem? 

The problem is task_struct <=> mm_struct relationship is muzzled
for various reasons, and to not depend on subtleties of who-owns-mm-struct
of the day, ->exec_path is placed onto task_struct.

Done this, ->exec_path simply stop depend on other current and future
hacks, and only defined by what binfmt loader did.

> That kernel thread has borroed whe VM for a while. It effectively _is_ a 
> thread of the process now.

See? Thread is what you get via CLONE_THREAD.

> So it's technically not even wrong to 
> explicitly allow things like /proc/*/exe to see it as such.

This knowledge is some implementation detail, how AIO is implemented and
how kernel threads access userpace VM. But you're going to expose it
by not checking for kernel-threadness or something.

> But you can hide it by just checking some flag in the thread structure if 
> you really want to.
> 
> But when creating a regular thread, you should _not_ need to take a 
> spinlock and duplicate the executable path!

Since task_struct is literally copied during clone, _that_ line,
can be easily changed to just

	path_get(&p->exec_path);

But for consistency and to not give bad example it was written
with honest locking.

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Al Viro]

On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
> index eff74b9..6c56262 100644
> --- a/fs/binfmt_som.c
> +++ b/fs/binfmt_som.c
> @@ -174,6 +174,7 @@ static int map_som_binary(struct file *file,
>  	up_write(&current->mm->mmap_sem);
>  	if (retval > 0 || retval < -1024)
>  		retval = 0;
> +	set_task_exec_path(current, &bprm->file->f_path);

Oh?  Even on failure exits?
> +	if (!path->mnt || !path->dentry)
> +		return -ENOENT;

Umm...  I really don't like that.  Note that path with NULL vfsmount
and non-NULL dentry should never happen.  If anything, we ought
to add path_empty(path) (!(path)->mnt) and convert such places to it.

> +static inline void set_task_exec_path(struct task_struct *tsk, struct path *path)
> +{
> +	struct path old_path;
> +
> +	path_get(path);
> +	task_lock(tsk);
> +	old_path = tsk->exec_path;
> +	tsk->exec_path = *path;
> +	task_unlock(tsk);
> +	path_put(&old_path);
> +}

Do we ever have a right to do that to anything other than current?  Note
that fork() is a special case anyway...

> +	set_task_exec_path(tsk, &(struct path){ .mnt = NULL, .dentry = NULL });

Ew...

> +	get_task_exec_path(current, &p->exec_path);
> +

We already have that value sitting there, so why not get_path(&p->exec_path)?

The real problem I have with that we *really* can't umount the filesystem
that used to host the binary anymore.  At all.

Frankly, I'm almost tempted to add explicit way to switch the damn thing
via /proc/self/something - e.g. allow a binary to write a pathname to
/proc/self/set_exec and have that switch the sucker.  The interesting
part, of course, is figuring out the security implications of that...

Re: [PATCH 1/9] exec_path 1/9: introduce ->exec_path and switch /proc/*/exe

[Alexey Dobriyan]

On Sat, Jun 06, 2009 at 08:22:44AM +0100, Al Viro wrote:
> On Thu, Jun 04, 2009 at 03:04:22AM +0400, Alexey Dobriyan wrote:
> > diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
> > index eff74b9..6c56262 100644
> > --- a/fs/binfmt_som.c
> > +++ b/fs/binfmt_som.c
> > @@ -174,6 +174,7 @@ static int map_som_binary(struct file *file,
> >  	up_write(&current->mm->mmap_sem);
> >  	if (retval > 0 || retval < -1024)
> >  		retval = 0;
> > +	set_task_exec_path(current, &bprm->file->f_path);
> 
> Oh?  Even on failure exits?

OK.

> > +	if (!path->mnt || !path->dentry)
> > +		return -ENOENT;
> 
> Umm...  I really don't like that.  Note that path with NULL vfsmount
> and non-NULL dentry should never happen.  If anything, we ought
> to add path_empty(path) (!(path)->mnt) and convert such places to it.

Why mntget/mntput handle NULL vfsmount?

> > +static inline void set_task_exec_path(struct task_struct *tsk, struct path *path)
> > +{
> > +	struct path old_path;
> > +
> > +	path_get(path);
> > +	task_lock(tsk);
> > +	old_path = tsk->exec_path;
> > +	tsk->exec_path = *path;
> > +	task_unlock(tsk);
> > +	path_put(&old_path);
> > +}
> 
> Do we ever have a right to do that to anything other than current?  Note
> that fork() is a special case anyway...

Locking wise? Yes, why not.
 
> > +	set_task_exec_path(tsk, &(struct path){ .mnt = NULL, .dentry = NULL });
> 
> Ew...

:^)

> > +	get_task_exec_path(current, &p->exec_path);
> > +
> 
> We already have that value sitting there, so why not get_path(&p->exec_path)?
> 
> The real problem I have with that we *really* can't umount the filesystem
> that used to host the binary anymore.  At all.

OTOH, you can always answer the question what is executing unless task is
sufficiently dead.

Now, I dont' think anyone unmaps old executable except malicious stuff.

> Frankly, I'm almost tempted to add explicit way to switch the damn thing
> via /proc/self/something - e.g. allow a binary to write a pathname to
> /proc/self/set_exec and have that switch the sucker.  The interesting
> part, of course, is figuring out the security implications of that...

I think nobody will use it.

I think /proc/*/exe should stay informational without task being able
to mangle it at will.

Re: [PATCH 14/38] Remove struct mm_struct::exe_file et al

[Matt Helsley]

On Mon, Jun 01, 2009 at 01:54:27AM +0400, Alexey Dobriyan wrote:
> On Tue, May 26, 2009 at 04:24:15PM -0700, Andrew Morton wrote:
> > On Tue, 26 May 2009 04:36:18 -0700
> > Matt Helsley <matthltc@us.ibm.com> wrote:
> > 
> > > I don't see any mention in the changelog of the point brought up by Ingo:
> > > 
> > > http://lkml.org/lkml/2009/4/10/105
> > 
> > Nor of Eric's comments.
> > 
> > Alexey, pleeeze don't do this.  We (read: I) heavily depend upon patch
> > submitters to keep track of outstanding issues and review comments,
> > etc.
> > 
> > If the patch submitter simply blows these things off then it devolves
> > to me having to keep track of each patch's issue list as well as the
> > patch itself.  My workload goes up by a factor of N and the error rate
> > goes up by N^2 :(
> 
> grmbh..
> 
> "Security" and "holding ->mmap_sem" were answered and dismissed.
> 
> You can't do readlink(2) on /proc/*/exe if you can't ptrace task.
> So no new possible holes are created.

Yes, I messed up: you answered the security question.

I still like that exe_file avoids the VMA walk with mmap_sem held. Holding
mmap_sem seems like a bigger performance difference than adding a branch
based on vm_flags in the VMA add/remove/split/merge code. Wouldn't vm_flags
be cache-hot? If so the typical cost added is an && and the branch
itself. Performance-related side effects of those are extra instruction fetches
and, perhaps most of all, typical branch costs like pipeline stalls and
flushes.

I suspect you could avoid most of the hypothesized performance difference
introduced by exe_file (if it's even measurable) by marking the branch
unlikely(). I guess that flag is unlikely since it's only applied
during exec by the kernel, the splits or removals of these areas are
probably near-constant in number, and they usually take place shortly after
exec anyway.

Of course if branch prediction hardware is very good then it may be
even better to leave these alone.

> 
> ->mmap_sem was held since /proc/*/exe was added and nobody cared.

"nobody cared" isn't a good reason to put it back. That kind of argument just
as easily supports exe_file: "I added code to the VMA paths and nobody cared."
Of course now you care about these changes to the VMA paths and I care
about holding mmap_sem...

> And, again, you can't readlink _any_ /proc/*/exe.

Yup. Sorry about that again.

In summary: I think the performance benefits of this patch have yet to be
established and really the only benefit I agree with is the nice reduction
in code.

Cheers,
	-Matt Helsley