mm: rcu-protected get_mm_exe_file()

PATCH In review Konstantin Khlebnikov 3 participants 7 messages Mar 16, 2015

Subsystems (5)

Memory management VFS MM core Scheduler +1 more subsystems

lore

Share & tools

lore

Konstantin Khlebnikov

author

2015-03-16 13:13 UTC #0 lore raw

PATCH 1/1 mm: rcu-protected get_mm_exe_file()

This patch removes mm->mmap_sem from mm->exe_file read side. Also it kills dup_mm_exe_file() and moves exe_file duplication into dup_mmap() where both mmap_sems are locked.

Signed-off-by Konstantin Khlebnikov <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Al Viro <[email protected]>

---
 fs/file.c                |    3 +-
 include/linux/fs.h       |    1 +
 include/linux/mm_types.h |    2 +-
 kernel/fork.c            |   56 ++++++++++++++++++++++++++++++----------------
 4 files changed, 40 insertions(+), 22 deletions(-)

fs/file.c +1 -2

index ee738ea028fa..93c5f89c248b 100644
@@ -638,8 +638,7 @@ static struct file *__fget(unsigned int fd, fmode_t mask)
 	file = fcheck_files(files, fd);
 	if (file) {
 		/* File object ref couldn't be taken */
-		if ((file->f_mode & mask) ||
-		    !atomic_long_inc_not_zero(&file->f_count))
+		if ((file->f_mode & mask) || !get_file_rcu(file))
 			file = NULL;
 	}
 	rcu_read_unlock();

include/linux/fs.h +1 -0

index 1abc7f2a5730..29bc94cfa273 100644
@@ -847,6 +847,7 @@ static inline struct file *get_file(struct file *f)
 	atomic_long_inc(&f->f_count);
 	return f;
 }
+#define get_file_rcu(x) atomic_long_inc_not_zero(&(x)->f_count)
 #define fput_atomic(x)	atomic_long_add_unless(&(x)->f_count, -1, 1)
 #define file_count(x)	atomic_long_read(&(x)->f_count)

include/linux/mm_types.h +1 -1

index 590630eb59ba..8d37e26a1007 100644
@@ -429,7 +429,7 @@ struct mm_struct {
 #endif
 
 	/* store ref to file /proc/<pid>/exe symlink points to */
-	struct file *exe_file;
+	struct file __rcu *exe_file;
 #ifdef CONFIG_MMU_NOTIFIER
 	struct mmu_notifier_mm *mmu_notifier_mm;
 #endif

kernel/fork.c +37 ~~-19~~

94 lines not on this page · raw message

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oleg Nesterov

Acked-by on v1 1/1

2015-03-16 14:09 UTC #1 lore raw

Quoted 15 lines · Konstantin Khlebnikov

kernel/fork.c +12 -0

+/**
+ * set_mm_exe_file - change a reference to the mm's executable file
+ *
+ * This changes mm's executale file (shown as symlink /proc/[pid]/exe).
+ *
+ * Main users are mmput(), sys_execve() and sys_prctl(PR_SET_MM_MAP/EXE_FILE).
+ * Callers prevent concurrent invocations: in mmput() nobody alive left,
+ * in execve task is single-threaded, prctl holds mmap_sem exclusively.
+ */
 void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
 {
+	struct file *old_exe_file = rcu_dereference_protected(mm->exe_file,
+			!atomic_read(&mm->mm_users) || current->in_execve ||
+			lock_is_held(&mm->mmap_sem));

kernel/fork.c void mmput(struct mm_struct *mm) · line 706

Quoted 10 lines

kernel/fork.c +4 -3

+
 	if (new_exe_file)
 		get_file(new_exe_file);
-	if (mm->exe_file)
-		fput(mm->exe_file);
-	mm->exe_file = new_exe_file;
+	rcu_assign_pointer(mm->exe_file, new_exe_file);
+	if (old_exe_file)
+		fput(old_exe_file);
 }

Yes, I think this is correct, __fput() does call_rcu(file_free_rcu). And much better than the new lock ;)

Acked-by Oleg Nesterov <[email protected]>

So I think the patch is fine, but personally I dislike the "prctl holds mmap_sem exclusively" and rcu_dereference_protected().

I mean, I think we can do another cleanup on top of this change.

1. set_mm_exe_file() should be called by exit/exec only, so it should use

rcu_dereference_protected(mm->exe_file,
atomic_read(&mm->mm_users) <= 1);

2. prctl() should not use it, it can do

get_file(new_exe);
old_exe = xchg(&mm->exe_file);
if (old_exe)
fput(old_exe);

3. and we can remove down_write(mmap_sem) from prctl paths.

Actually we can do this even without xchg() above, but we might want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check.

What do you think?

Oleg.

Davidlohr Bueso

2015-03-16 14:50 UTC #2 lore raw

Quoted 4 lines · Oleg Nesterov

3. and we can remove down_write(mmap_sem) from prctl paths.
Actually we can do this even without xchg() above, but we might want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check.

Yeah I was waiting for security folks input about this, otherwise this still doesn't do it for me as we still have to deal with mmap_sem.

Konstantin Khlebnikov

author

2015-03-16 16:18 UTC #3 lore raw

Quoted 9 lines · Davidlohr Bueso

Quoted 4 lines · Oleg Nesterov
3. and we can remove down_write(mmap_sem) from prctl paths.
Actually we can do this even without xchg() above, but we might want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check.
Yeah I was waiting for security folks input about this, otherwise this still doesn't do it for me as we still have to deal with mmap_sem.

Why? mm->flags are updated atomically. mmap_sem isn't required here.

Signature · 7 lines

Konstantin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Davidlohr Bueso

2015-03-16 17:04 UTC #4 lore raw

Quoted 4 lines · Konstantin Khlebnikov

Quoted 3 lines · Davidlohr Bueso
Quoted 2 lines · Oleg Nesterov
3. and we can remove down_write(mmap_sem) from prctl paths.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Actually we can do this even without xchg() above, but we might want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check.
Yeah I was waiting for security folks input about this, otherwise this still doesn't do it for me as we still have to deal with mmap_sem.
Why? mm->flags are updated atomically. mmap_sem isn't required here.

Right, nm I was thinking of the set but Oleg's xchg() idea is obviously correct and a much better approach to mine. The only thing I'd suggest is explicitly commenting the lockless get_mm_exe_file callers, if useful, you can take it from my patch 1.

Konstantin Khlebnikov

author

2015-03-16 16:16 UTC #5 lore raw

In reply to Oleg Nesterov

Quoted 39 lines · Oleg Nesterov

Quoted 15 lines · Konstantin Khlebnikov
kernel/fork.c +12 -0
+/**
+ * set_mm_exe_file - change a reference to the mm's executable file
+ *
+ * This changes mm's executale file (shown as symlink /proc/[pid]/exe).
+ *
+ * Main users are mmput(), sys_execve() and sys_prctl(PR_SET_MM_MAP/EXE_FILE).
+ * Callers prevent concurrent invocations: in mmput() nobody alive left,
+ * in execve task is single-threaded, prctl holds mmap_sem exclusively.
+ */
  void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
  {
+	struct file *old_exe_file = rcu_dereference_protected(mm->exe_file,
+			!atomic_read(&mm->mm_users) || current->in_execve ||
+			lock_is_held(&mm->mmap_sem));
kernel/fork.c void mmput(struct mm_struct *mm) · line 706
Quoted 10 lines
kernel/fork.c +4 -3
+
  	if (new_exe_file)
  		get_file(new_exe_file);
-	if (mm->exe_file)
-		fput(mm->exe_file);
-	mm->exe_file = new_exe_file;
+	rcu_assign_pointer(mm->exe_file, new_exe_file);
+	if (old_exe_file)
+		fput(old_exe_file);
  }
Yes, I think this is correct, __fput() does call_rcu(file_free_rcu). And much better than the new lock ;)
Acked-by Oleg Nesterov <[email protected]>
So I think the patch is fine, but personally I dislike the "prctl holds mmap_sem exclusively" and rcu_dereference_protected().
I mean, I think we can do another cleanup on top of this change.

Quoted 12 lines

	1. set_mm_exe_file() should be called by exit/exec only, so
	   it should use

		rcu_dereference_protected(mm->exe_file,
					atomic_read(&mm->mm_users) <= 1);

	2. prctl() should not use it, it can do

	   get_file(new_exe);
	   old_exe = xchg(&mm->exe_file);
	   if (old_exe)
	   	fput(old_exe);

I think smp_mb() is required before xchg() or
probably this stuff should be hidden inside yet another magic RCU macro ( with two screens of comments =)

Quoted 10 lines

3. and we can remove down_write(mmap_sem) from prctl paths.
Actually we can do this even without xchg() above, but we might want to kill MMF_EXE_FILE_CHANGED and test_and_set_bit() check.
What do you think?
Oleg.

Signature · 7 lines

Konstantin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oleg Nesterov

2015-03-16 16:37 UTC #6 lore raw

Quoted 12 lines · Konstantin Khlebnikov

Quoted 10 lines · Oleg Nesterov
I mean, I think we can do another cleanup on top of this change.
1. set_mm_exe_file() should be called by exit/exec only, so it should use
rcu_dereference_protected(mm->exe_file,
atomic_read(&mm->mm_users) <= 1);
2. prctl() should not use it, it can do

Quoted 8 lines

Quoted 4 lines
	   get_file(new_exe);
	   old_exe = xchg(&mm->exe_file);
	   if (old_exe)
	   	fput(old_exe);
I think smp_mb() is required before xchg() or
probably this stuff should be hidden inside yet another magic RCU macro ( with two screens of comments =)

Not really, xchg() implies mb's on both sides. As any other atomic operation which returns the result.

(And in fact we do not even need rcu_assign_pointer() in set_mm_exe_file(),
get_mm_exe_file() could do READ_ONCE() + inc_not_zero(). But this is off- topic, and of course rcu_* helpers look better)

Oleg.