Re: [PATCH 1/2] exec: Properly mark the point of no return

Bernd Edlinger [off-list ref] writes:

On 3/5/20 10:15 PM, Eric W. Biederman wrote:

quoted

Add a flag binfmt->unrecoverable to mark when execution has gotten to
the point where it is impossible to return to userspace with the
calling process unchanged.

While techinically this state starts as soon as de_thread starts
killing threads, the only return path at that point is if there is a
fatal signal pending.  I have choosen instead to set unrecoverable
when the killing stops, and there are possibilities of failures other
than fatal signals.  In particular it is possible for the allocation
of a new sighand structure to fail.

Setting unrecoverable at this point has the benefit that other actions
can be taken after the other threads are all dead, and the
unrecoverable flag can double as a flag that those actions have been
taken.

Signed-off-by: "Eric W. Biederman" <redacted>
---
 fs/exec.c               | 7 ++++---
 include/linux/binfmts.h | 7 ++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..c243f9660d46 100644
--- a/fs/exec.c
+++ b/fs/exec.c

@@ -1061,7 +1061,7 @@ static int exec_mmap(struct mm_struct *mm)
  * disturbing other processes.  (Other processes might share the signal
  * table via the CLONE_SIGHAND option to clone().)
  */
-static int de_thread(struct task_struct *tsk)
+static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk)
 {
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *oldsighand = tsk->sighand;

@@ -1182,6 +1182,7 @@ static int de_thread(struct task_struct *tsk)
 		release_task(leader);
 	}
 
+	bprm->unrecoverable = true;
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
 

ah, sorry, 
        if (thread_group_empty(tsk))
                goto no_thread_group;
will skip this:

        sig->group_exit_task = NULL;
        sig->notify_count = 0;

no_thread_group:
        /* we have changed execution domain */
        tsk->exit_signal = SIGCHLD;

so I think the bprm->unrecoverable = true; should be here?

Absolutely.  Thank you very much.

This is why I try and keep things to one clear simple thing per patch so
silly thinkos like that can be caught.

Eric