lkml.org 
[lkml]   [2020]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/6] exec: Run sync_mm_rss before taking exec_update_mutex
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

\
 
 \ /
  Last update: 2020-05-09 07:17    [W:0.631 / U:8.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site