| Message ID | 875zd66za3.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Trivial cleanups for exec | expand |
On Fri, May 08, 2020 at 01:45:56PM -0500, Eric W. Biederman wrote: > Like exec_mm_release sync_mm_rss is about flushing out the state of > the old_mm, which does not need to happen under exec_update_mutex. > > Make this explicit by moving sync_mm_rss outside of exec_update_mutex. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Reviewed-by: Kees Cook <keescook@chromium.org> Additional thoughts below... > --- > fs/exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 11a5c073aa35..15682a1dfee9 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1051,13 +1051,14 @@ static int exec_mmap(struct mm_struct *mm) > tsk = current; > old_mm = current->mm; > exec_mm_release(tsk, old_mm); > + if (old_mm) > + sync_mm_rss(old_mm); > > ret = mutex_lock_killable(&tsk->signal->exec_update_mutex); > if (ret) > return ret; > > if (old_mm) { > - sync_mm_rss(old_mm); > /* > * Make sure that if there is a core dump in progress > * for the old mm, we get out and die instead of going $ git grep exec_mm_release fs/exec.c: exec_mm_release(tsk, old_mm); include/linux/sched/mm.h:extern void exec_mm_release(struct task_struct *, struct mm_struct *); kernel/fork.c:void exec_mm_release(struct task_struct *tsk, struct mm_struct *mm) kernel/fork.c: void exit_mm_release(struct task_struct *tsk, struct mm_struct *mm) { futex_exit_release(tsk); mm_release(tsk, mm); } void exec_mm_release(struct task_struct *tsk, struct mm_struct *mm) { futex_exec_release(tsk); mm_release(tsk, mm); } $ git grep exit_mm_release include/linux/sched/mm.h:extern void exit_mm_release(struct task_struct *, struct mm_struct *); kernel/exit.c: exit_mm_release(current, mm); kernel/fork.c:void exit_mm_release(struct task_struct *tsk, struct mm_struct *mm) kernel/exit.c: exit_mm_release(current, mm); if (!mm) return; sync_mm_rss(mm); It looks to me like both exec_mm_release() and exit_mm_release() could easily have the sync_mm_rss(...) folded into their function bodies and removed from the callers. *shrug*
Kees Cook <keescook@chromium.org> writes: > $ git grep exec_mm_release > fs/exec.c: exec_mm_release(tsk, old_mm); > include/linux/sched/mm.h:extern void exec_mm_release(struct task_struct *, struct mm_struct *); > kernel/fork.c:void exec_mm_release(struct task_struct *tsk, struct mm_struct *mm) > > kernel/fork.c: > > void exit_mm_release(struct task_struct *tsk, struct mm_struct *mm) > { > futex_exit_release(tsk); > mm_release(tsk, mm); > } > > void exec_mm_release(struct task_struct *tsk, struct mm_struct *mm) > { > futex_exec_release(tsk); > mm_release(tsk, mm); > } > > $ git grep exit_mm_release > include/linux/sched/mm.h:extern void exit_mm_release(struct task_struct *, struct mm_struct *); > kernel/exit.c: exit_mm_release(current, mm); > kernel/fork.c:void exit_mm_release(struct task_struct *tsk, struct mm_struct *mm) > > kernel/exit.c: > > exit_mm_release(current, mm); > if (!mm) > return; > sync_mm_rss(mm); > > It looks to me like both exec_mm_release() and exit_mm_release() could > easily have the sync_mm_rss(...) folded into their function bodies and > removed from the callers. *shrug* Well it would have to be all of: if (mm) sync_mm_rss(mm); I remember reading through exit_mm_release and seeing that nothing actually depended upon a non-NULL mm. Unless you have clear_child_tid set. I am not up to speed on that part of the mm layer right now to know if it is a good idea to put sync_mm_rss in exit_mm_release but at a quick look it feels like it. Eric
diff --git a/fs/exec.c b/fs/exec.c index 11a5c073aa35..15682a1dfee9 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1051,13 +1051,14 @@ static int exec_mmap(struct mm_struct *mm) tsk = current; old_mm = current->mm; exec_mm_release(tsk, old_mm); + if (old_mm) + sync_mm_rss(old_mm); ret = mutex_lock_killable(&tsk->signal->exec_update_mutex); if (ret) return ret; if (old_mm) { - sync_mm_rss(old_mm); /* * Make sure that if there is a core dump in progress * for the old mm, we get out and die instead of going
Like exec_mm_release sync_mm_rss is about flushing out the state of the old_mm, which does not need to happen under exec_update_mutex. Make this explicit by moving sync_mm_rss outside of exec_update_mutex. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)