Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Fri, 3 Apr 2020 09:23:52 -0700 | Subject | Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1 |
| |
On Fri, Apr 3, 2020 at 8:09 AM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > > On 4/2/20 9:04 PM, Linus Torvalds wrote: > > In fact, then you could drop the > > > > mutex_unlock(&tsk->signal->exec_update_mutex); > > > > in the error case of exec_mmap(), because now the error handling in > > free_bprm() would do the cleanup automatically. > > > > The error handling is sometimes called when the exec_update_mutex is > not taken, in fact even de_thread not called.
But that's the whole point of the flag. Make the flag be about "do I hold the mutex", and then the error handling does the right thing regardless.
> Can you say how you would suggest that to be done?
I think the easiest thing to do to explain is to just write the patch.
This is entirely untested, but see what the difference is? I make the flag be about exactly where I take the lock, not about some "I have called exec_mmap".
Which means that now exec_mmap() doesn't even need to unlock it in the error case, because the unlocking will happen properly in the bprm_exit regardless.
This makes that unconditional unlocking logic much more obvious.
That said, Eric says he can make it all properly static so that it doesn't need that kind of dynamic "if (x) unlock()" logic at all, which is much better.
So this patch is not for consumption, it's purely for "look, something like this"
Linus fs/exec.c | 15 +++++++-------- include/linux/binfmts.h | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..cdc7f1145662 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1041,8 +1041,9 @@ EXPORT_SYMBOL(read_code); * On success, this function returns with the mutex * exec_update_mutex locked. */ -static int exec_mmap(struct mm_struct *mm) +static int exec_mmap(struct linux_binprm *bprm) { + struct mm_struct *mm = bprm->mm; struct task_struct *tsk; struct mm_struct *old_mm, *active_mm; int ret; @@ -1055,6 +1056,7 @@ static int exec_mmap(struct mm_struct *mm) ret = mutex_lock_killable(&tsk->signal->exec_update_mutex); if (ret) return ret; + bprm->update_mutex_held = 1; if (old_mm) { sync_mm_rss(old_mm); @@ -1067,7 +1069,6 @@ static int exec_mmap(struct mm_struct *mm) down_read(&old_mm->mmap_sem); if (unlikely(old_mm->core_state)) { up_read(&old_mm->mmap_sem); - mutex_unlock(&tsk->signal->exec_update_mutex); return -EINTR; } } @@ -1321,17 +1322,15 @@ int flush_old_exec(struct linux_binprm * bprm) * Release all of the old mmap stuff */ acct_arg_size(bprm, 0); - retval = exec_mmap(bprm->mm); + retval = exec_mmap(bprm); if (retval) goto out; /* - * After setting bprm->called_exec_mmap (to mark that current is - * using the prepared mm now), we have nothing left of the original + * After setting bprm->mm to NULL, we have nothing left of the original * process. If anything from here on returns an error, the check * in search_binary_handler() will SEGV current. */ - bprm->called_exec_mmap = 1; bprm->mm = NULL; #ifdef CONFIG_POSIX_TIMERS @@ -1477,7 +1476,7 @@ static void free_bprm(struct linux_binprm *bprm) { free_arg_pages(bprm); if (bprm->cred) { - if (bprm->called_exec_mmap) + if (bprm->update_mutex_held) mutex_unlock(¤t->signal->exec_update_mutex); mutex_unlock(¤t->signal->cred_guard_mutex); abort_creds(bprm->cred); @@ -1720,7 +1719,7 @@ int search_binary_handler(struct linux_binprm *bprm) read_lock(&binfmt_lock); put_binfmt(fmt); - if (retval < 0 && bprm->called_exec_mmap) { + if (retval < 0 && !bprm->mm) { /* we got to flush_old_exec() and failed after it */ read_unlock(&binfmt_lock); force_sigsegv(SIGSEGV); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index a345d9fed3d8..b815783c8b2c 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -50,7 +50,7 @@ struct linux_binprm { * This is past the point of no return, when the * exec_update_mutex has been taken. */ - called_exec_mmap:1; + update_mutex_held:1; #ifdef __alpha__ unsigned int taso:1; #endif | |