Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

[Linus Torvalds]

On Fri, Apr 3, 2020 at 8:09 AM Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> 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(&current->signal->exec_update_mutex);
 		mutex_unlock(&current->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