Skip to contentSkip to conversation

turn mm->exe_file into mm->exe_path

PATCH In review Oleg Nesterov 3 participants 9 messages Mar 5, 2012 – Mar 6, 2012

Subsystems (5)
VFS MM core Scheduler exec and binfmt +1 more subsystems
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

cover turn mm->exe_file into mm->exe_path

I think the patch is simple and self-explanatory, it simply does s/mm->exe_file/mm->exe_path/.

Why do we need mm->exe_file? IIUC, there are 2 reasons:

1. we do not want O(n) proc/pid/exe looking for the 1st VM_EXECUTABLE vma.

2. we do not want to rely on vma->vm_file->f_path, bprm->file->f_op->mmap can change ->vm_file.

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

And can't we also remove added_exe_file_vma/removed_exe_file_vma? Why do we need mm->num_exe_file_vmas? Afaics it is only needed to "free" mm->exe_file if the application unmaps all these vmas. Say, to allow to unmount fs.

Can't we simply add PR_CLEAR_MM_EXE_PATH instead? Of course it is not enough if ->vm_file still has a reference. But c/r people want PR_SET_MM_EXE_FILE anyway, see http://marc.info/?t=133052865500016
So perhaps we can add PR_SET_MM_EXE_PATH which accepts NULL as well and kill this counter?

Oleg.

author
#1 lore raw

PATCH 1/1 turn mm->exe_file into mm->exe_path

Turn mm->exe_file into mm->exe_path and fix the compilation errors. We only need this member to get the path, additional reference to bprm->file makes no sense.

The patch doesn't rename added_exe_file_vma/removed_exe_file_vma and mm->num_exe_file_vmas, and perhaps we can remove them later.

Also remove the stale comment in include/linux/mm.h.

Signed-off-by Oleg Nesterov <[email protected]>
---
 fs/exec.c                |   20 ++++++++++----------
 fs/proc/base.c           |   17 ++++++++---------
 include/linux/mm.h       |    5 ++---
 include/linux/mm_types.h |    2 +-
 kernel/fork.c            |   41 +++++++++++++++++++++--------------------
 5 files changed, 42 insertions(+), 43 deletions(-)
fs/exec.c +10 -10
index 92ce83a..cf16922 100644
@@ -1098,7 +1098,7 @@ int flush_old_exec(struct linux_binprm * bprm)
 	if (retval)
 		goto out;
 
-	set_mm_exe_file(bprm->mm, bprm->file);
+	set_mm_exe_path(bprm->mm, &bprm->file->f_path);
 
 	filename_to_taskname(bprm->tcomm, bprm->filename, sizeof(bprm->tcomm));
 	/*

@@ -1671,14 +1671,14 @@ static void cn_escape(char *str)
 			*str = '!';
 }
 
-static int cn_print_exe_file(struct core_name *cn)
+static int cn_print_exe_path(struct core_name *cn)
 {
-	struct file *exe_file;
+	struct path *exe_path;
 	char *pathbuf, *path;
 	int ret;
 
-	exe_file = get_mm_exe_file(current->mm);
-	if (!exe_file) {
+	exe_path = get_mm_exe_path(current->mm);
+	if (!exe_path) {
 		char *commstart = cn->corename + cn->used;
 		ret = cn_printf(cn, "%s (path unknown)", current->comm);
 		cn_escape(commstart);

@@ -1688,10 +1688,10 @@ static int cn_print_exe_file(struct core_name *cn)
 	pathbuf = kmalloc(PATH_MAX, GFP_TEMPORARY);
 	if (!pathbuf) {
 		ret = -ENOMEM;
-		goto put_exe_file;
+		goto put_exe_path;
 	}
 
-	path = d_path(&exe_file->f_path, pathbuf, PATH_MAX);
+	path = d_path(exe_path, pathbuf, PATH_MAX);
 	if (IS_ERR(path)) {
 		ret = PTR_ERR(path);
 		goto free_buf;

@@ -1703,8 +1703,8 @@ static int cn_print_exe_file(struct core_name *cn)
 
 free_buf:
 	kfree(pathbuf);
-put_exe_file:
-	fput(exe_file);
+put_exe_path:
+	path_put(exe_path);
 	return ret;
 }
 

@@ -1786,7 +1786,7 @@ static int format_corename(struct core_name *cn, long signr)
 				break;
 			}
 			case 'E':
-				err = cn_print_exe_file(cn);
+				err = cn_print_exe_path(cn);
 				break;
 			/* core limit size */
 			case 'c':
fs/proc/base.c +8 -9
index d4548dd..0d7dbd6 100644
@@ -1401,11 +1401,11 @@ static const struct file_operations proc_pid_set_comm_operations = {
 	.release	= single_release,
 };
 
-static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
+static int proc_exe_link(struct dentry *dentry, struct path *path)
 {
 	struct task_struct *task;
 	struct mm_struct *mm;
-	struct file *exe_file;
+	struct path *exe_path;
 
 	task = get_proc_task(dentry->d_inode);
 	if (!task)

@@ -1414,15 +1414,14 @@ static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
 	put_task_struct(task);
 	if (!mm)
 		return -ENOENT;
-	exe_file = get_mm_exe_file(mm);
+	exe_path = get_mm_exe_path(mm);
 	mmput(mm);
-	if (exe_file) {
-		*exe_path = exe_file->f_path;
-		path_get(&exe_file->f_path);
-		fput(exe_file);
-		return 0;
-	} else
+
+	if (!exe_path)
 		return -ENOENT;
+
+	*path = *exe_path;
+	return 0;
 }
 
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
include/linux/mm.h +2 -3
index 17b27cd..d18fe93 100644
@@ -1375,11 +1375,10 @@ 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);
 
-/* 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);
-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 set_mm_exe_path(struct mm_struct *mm, struct path *exe_path);
+extern struct path *get_mm_exe_path(struct mm_struct *mm);
 
 extern int may_expand_vm(struct mm_struct *mm, unsigned long npages);
 extern int install_special_mapping(struct mm_struct *mm,
include/linux/mm_types.h +1 -1
index 3cc3062..e734984 100644
@@ -377,7 +377,7 @@ struct mm_struct {
 #endif
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
-	struct file *exe_file;
+	struct path *exe_path;
 	unsigned long num_exe_file_vmas;
 #ifdef CONFIG_MMU_NOTIFIER
 	struct mmu_notifier_mm *mmu_notifier_mm;
kernel/fork.c +21 -20

77 lines not on this page · raw message

Signature · 1 line
1.5.5.1
#2 lore raw
In reply to Oleg Nesterov
Quoted 19 lines · Oleg Nesterov

I think the patch is simple and self-explanatory, it simply does s/mm->exe_file/mm->exe_path/.

Why do we need mm->exe_file? IIUC, there are 2 reasons:

1. we do not want O(n) proc/pid/exe looking for the 1st VM_EXECUTABLE vma.

2. we do not want to rely on vma->vm_file->f_path, bprm->file->f_op->mmap can change ->vm_file.

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

And can't we also remove added_exe_file_vma/removed_exe_file_vma? Why do we need mm->num_exe_file_vmas? Afaics it is only needed to "free" mm->exe_file if the application unmaps all these vmas. Say, to allow to unmount fs.

Quoted 6 lines

Can't we simply add PR_CLEAR_MM_EXE_PATH instead? Of course it is not enough if ->vm_file still has a reference. But c/r people want PR_SET_MM_EXE_FILE anyway, see http://marc.info/?t=133052865500016
So perhaps we can add PR_SET_MM_EXE_PATH which accepts NULL as well and kill this counter?

So, if I understand you correctly, if there is exe_path, I would simply put() it and get() and assign new one. Looks cool for me.

Not sure where we may use PR_SET_MM_EXE_PATH with NULL to kill num_exe_file_vmas from user space though (or to kill this exe_path in case if num_exe_file_vmas is removed from code).

Cyrill

author
#3 lore raw
Quoted 28 lines · Cyrill Gorcunov
Quoted 19 lines · Oleg Nesterov

I think the patch is simple and self-explanatory, it simply does s/mm->exe_file/mm->exe_path/.

Why do we need mm->exe_file? IIUC, there are 2 reasons:

1. we do not want O(n) proc/pid/exe looking for the 1st VM_EXECUTABLE vma.

2. we do not want to rely on vma->vm_file->f_path, bprm->file->f_op->mmap can change ->vm_file.

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

And can't we also remove added_exe_file_vma/removed_exe_file_vma? Why do we need mm->num_exe_file_vmas? Afaics it is only needed to "free" mm->exe_file if the application unmaps all these vmas. Say, to allow to unmount fs.

Quoted 6 lines

Can't we simply add PR_CLEAR_MM_EXE_PATH instead? Of course it is not enough if ->vm_file still has a reference. But c/r people want PR_SET_MM_EXE_FILE anyway, see http://marc.info/?t=133052865500016
So perhaps we can add PR_SET_MM_EXE_PATH which accepts NULL as well and kill this counter?

Quoted 2 lines

So, if I understand you correctly, if there is exe_path, I would simply put() it and get() and assign new one. Looks cool for me.

Yes,

Quoted 2 lines

Not sure where we may use PR_SET_MM_EXE_PATH with NULL to kill num_exe_file_vmas from user space though

Please see above:

	Why do we need mm->num_exe_file_vmas? Afaics it is only needed to
	"free" mm->exe_file if the application unmaps all these vmas. Say,
	to allow to unmount fs.

If we remove this counter, the application can't do the final put. But if we add PR_SET_MM_EXE_PATH which accepts NULL we solve this problem and this works for c/r too.

Of course, this is orthogonal to the patch I sent.

Oleg.

#4 lore raw
Quoted 10 lines · Oleg Nesterov
Quoted 3 lines

Not sure where we may use PR_SET_MM_EXE_PATH with NULL to kill num_exe_file_vmas from user space though

Please see above:

	Why do we need mm->num_exe_file_vmas? Afaics it is only needed to
	"free" mm->exe_file if the application unmaps all these vmas. Say,
	to allow to unmount fs.
Quoted 6 lines

If we remove this counter, the application can't do the final put. But if we add PR_SET_MM_EXE_PATH which accepts NULL we solve this problem and this works for c/r too.

Of course, this is orthogonal to the patch I sent.

I see. Thanks, Oleg!

Cyrill

#5 lore raw
In reply to Oleg Nesterov
Quoted 7 lines · Oleg Nesterov

I think the patch is simple and self-explanatory, it simply does s/mm->exe_file/mm->exe_path/.

Why do we need mm->exe_file? IIUC, there are 2 reasons:

1. we do not want O(n) proc/pid/exe looking for the 1st VM_EXECUTABLE vma.

(Frankly this always seemed like a bit of a hack to me. If someone got creative with text layout then it could break this hack...)

Quoted 6 lines
	2. we do not want to rely on vma->vm_file->f_path,
	   bprm->file->f_op->mmap can change ->vm_file.

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

PATCH 1/1 looks fine. I think Alexey Dobriyan was working on a similar patch years ago.

Quoted 4 lines

And can't we also remove added_exe_file_vma/removed_exe_file_vma? Why do we need mm->num_exe_file_vmas? Afaics it is only needed to "free" mm->exe_file if the application unmaps all these vmas. Say, to allow to unmount fs.

Yup. I know it's not pretty to have to track the exe file refs this way but I couldn't see any other way to keep a reference to the file (or path) and avoid pinning the mounted filesystem the exectuable is on.

Quoted 2 lines

Can't we simply add PR_CLEAR_MM_EXE_PATH instead? Of course it is not enough if ->vm_file still has a reference. But c/r people want

Relying solely on this prctl would break existing programs. I believe Al Viro's example was a program that copies its text to a new executable area, unmaps the original, performs a pivot_root(), and finally umounts the old root. Removing the counter would cause the mount to be pinned for these programs and the umount would fail.

Quoted 5 lines

PR_SET_MM_EXE_FILE anyway, see http://marc.info/?t=133052865500016
So perhaps we can add PR_SET_MM_EXE_PATH which accepts NULL as well and kill this counter?

Oleg.

Cheers,
-Matt Helsley

author
#6 lore raw
Quoted 8 lines · Matt Helsley
Quoted 3 lines · Oleg Nesterov

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

PATCH 1/1 looks fine. I think Alexey Dobriyan was working on a similar patch years ago.

Good, can I take the above as your Acked-by/Reviewed-by ?

Quoted 5 lines
Quoted 4 lines

And can't we also remove added_exe_file_vma/removed_exe_file_vma? Why do we need mm->num_exe_file_vmas? Afaics it is only needed to "free" mm->exe_file if the application unmaps all these vmas. Say, to allow to unmount fs.

Quoted 8 lines

Yup. I know it's not pretty to have to track the exe file refs this way but I couldn't see any other way to keep a reference to the file (or path) and avoid pinning the mounted filesystem the exectuable is on.

Quoted 2 lines

Can't we simply add PR_CLEAR_MM_EXE_PATH instead? Of course it is not enough if ->vm_file still has a reference. But c/r people want

Relying solely on this prctl would break existing programs.

so we can't :/ I expected this answer.

Quoted 5 lines

I believe Al
Viro's example was a program that copies its text to a new executable area, unmaps the original, performs a pivot_root(), and finally umounts the old root. Removing the counter would cause the mount to be pinned for these programs and the umount would fail.

Yes, sure, the application should do PR_SET_MM_EXE_PATH(NULL) after unmap. I am wondering if there is any apllication which actually does this.

But OK, it is hardly possible to argue with "break existing program".

Thanks,

Oleg.

#7 lore raw
Quoted 4 lines · Oleg Nesterov
Quoted 3 lines · Matt Helsley
Quoted 1 line · Oleg Nesterov
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

PATCH 1/1 looks fine. I think Alexey Dobriyan was working on a similar patch years ago.

Good, can I take the above as your Acked-by/Reviewed-by ?

Yes, this would be great, so thenI would re-make
my patch on top of a new concept.

Cyrill

Acked-by
#8 lore raw
In reply to Oleg Nesterov
Quoted 11 lines · Oleg Nesterov
Quoted 8 lines · Matt Helsley
Quoted 3 lines · Oleg Nesterov

Unless there was another subtle reason, "struct path *exe_path" can equally work but it looks more clear.

PATCH 1/1 looks fine. I think Alexey Dobriyan was working on a similar patch years ago.

Good, can I take the above as your Acked-by/Reviewed-by ?

Sure:

Acked-by Matt Helsley <[email protected]>

Cheers,
-Matt Helsley