Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec
From: Kees Cook <hidden>
Date: 2020-03-10 20:44:15
Also in:
linux-api, linux-fsdevel, linux-mm, lkml, stable
Possibly related (same subject, not in this thread)
- 2020-03-10 · Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec · Eric W. Biederman <hidden>
- 2020-03-10 · Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec · Eric W. Biederman <hidden>
- 2020-03-09 · Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec · Eric W. Biederman <hidden>
- 2020-03-09 · Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec · Bernd Edlinger <hidden>
- 2020-03-09 · Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec · Eric W. Biederman <hidden>
On Sun, Mar 08, 2020 at 04:38:00PM -0500, Eric W. Biederman wrote:
I have read through the code in exec_mmap and I do not see anything that depends on sighand or the sighand lock, or on signals in anyway so this should be safe. This rearrangement of code has two siginficant benefits. It makes the determination of passing the point of no return by testing bprm->mm accurate. All failures prior to that point in flush_old_exec are either truly recoverable or they are fatal.
Agreed. Though I see a use of "current", which maybe you want to parameterize to a "me" argument in acct_arg_size(). (Though looking at the callers, perhaps there is no benefit?)
quoted hunk ↗ jump to hunk
Futher this consolidates all of the possible indefinite waits for userspace together at the top of flush_old_exec. The possible wait for a ptracer on PTRACE_EVENT_EXIT, the possible wait for a page fault to be resolved in clear_child_tid, and the possible wait for a page fault in exit_robust_list. This consolidation allows the creation of a mutex to replace cred_guard_mutex that is not held of possible indefinite userspace waits. Which will allow removing deadlock scenarios from the kernel. Signed-off-by: "Eric W. Biederman" <redacted> --- fs/exec.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)diff --git a/fs/exec.c b/fs/exec.c index 215d86f77b63..d820a7272a76 100644 --- a/fs/exec.c +++ b/fs/exec.c@@ -1272,18 +1272,6 @@ int flush_old_exec(struct linux_binprm * bprm) if (retval) goto out; -#ifdef CONFIG_POSIX_TIMERS - exit_itimers(me->signal); - flush_itimer_signals(); -#endif
I think this comment: /* * This is called by do_exit or de_thread, only when there are no more * references to the shared signal_struct. */ void exit_itimers(struct signal_struct *sig) Refers to there being other threads, yes? Not that the signal table is private yet?
quoted hunk ↗ jump to hunk
- - /* - * Make the signal table private. - */ - retval = unshare_sighand(me); - if (retval) - goto out; - /* * Must be called _before_ exec_mmap() as bprm->mm is * not visibile until then. This also enables the update@@ -1307,6 +1295,18 @@ int flush_old_exec(struct linux_binprm * bprm) */ bprm->mm = NULL; +#ifdef CONFIG_POSIX_TIMERS + exit_itimers(me->signal); + flush_itimer_signals(); +#endif
I've mostly convinced myself that there are no "side-effects" from having these timers expire as the mm is going away. I think some kind of comment of that intent should be explicitly stated here above the timer work. Beyond that: Reviewed-by: Kees Cook <redacted> -Kees
+ + /* + * Make the signal table private. + */ + retval = unshare_sighand(me); + if (retval) + goto out; + set_fs(USER_DS); me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_NO_SETAFFINITY); -- 2.25.0
-- Kees Cook