[v2,3/5] exec: Move cleanup of posix timers on exec out of de_thread
diff mbox series

Message ID 87eeu25y14.fsf_-_@x220.int.ebiederm.org
State In Next
Commit ac2000b0ce6c5472ca0cd1811bcc400c7e21b00a
Headers show
Series
  • Infrastructure to allow fixing exec deadlocks
Related show

Commit Message

Eric W. Biederman March 8, 2020, 9:36 p.m. UTC
These functions have very little to do with de_thread move them out
of de_thread an into flush_old_exec proper so it can be more clearly
seen what flush_old_exec is doing.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Bernd Edlinger March 9, 2020, 7:30 p.m. UTC | #1
On 3/8/20 10:36 PM, Eric W. Biederman wrote:
> 
> These functions have very little to do with de_thread move them out
> of de_thread an into flush_old_exec proper so it can be more clearly
> seen what flush_old_exec is doing.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>


Bernd.
> ---
>  fs/exec.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index ff74b9a74d34..215d86f77b63 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>  	/* we have changed execution domain */
>  	tsk->exit_signal = SIGCHLD;
>  
> -#ifdef CONFIG_POSIX_TIMERS
> -	exit_itimers(sig);
> -	flush_itimer_signals();
> -#endif
> -
>  	BUG_ON(!thread_group_leader(tsk));
>  	return 0;
>  
> @@ -1277,6 +1272,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>  	if (retval)
>  		goto out;
>  
> +#ifdef CONFIG_POSIX_TIMERS
> +	exit_itimers(me->signal);
> +	flush_itimer_signals();
> +#endif
> +
>  	/*
>  	 * Make the signal table private.
>  	 */
>
Christian Brauner March 9, 2020, 7:59 p.m. UTC | #2
On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
> 
> These functions have very little to do with de_thread move them out
> of de_thread an into flush_old_exec proper so it can be more clearly
> seen what flush_old_exec is doing.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/exec.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index ff74b9a74d34..215d86f77b63 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)

While you're cleaning up de_thread() wouldn't it be good to also take
the opportunity and remove the task argument from de_thread(). It's only
ever used with current. Could be done in one of your patches or as a
separate patch.

diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..ee108707e4b0 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1061,8 +1061,9 @@ 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(void)
 {
+       struct task_struct *tsk = current;
        struct signal_struct *sig = tsk->signal;
        struct sighand_struct *oldsighand = tsk->sighand;
        spinlock_t *lock = &oldsighand->siglock;
@@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm)
         * Make sure we have a private signal table and that
         * we are unassociated from the previous thread group.
         */
-       retval = de_thread(current);
+       retval = de_thread();
        if (retval)
                goto out;
Eric W. Biederman March 9, 2020, 8:06 p.m. UTC | #3
Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
>> 
>> These functions have very little to do with de_thread move them out
>> of de_thread an into flush_old_exec proper so it can be more clearly
>> seen what flush_old_exec is doing.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/exec.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/exec.c b/fs/exec.c
>> index ff74b9a74d34..215d86f77b63 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>
> While you're cleaning up de_thread() wouldn't it be good to also take
> the opportunity and remove the task argument from de_thread(). It's only
> ever used with current. Could be done in one of your patches or as a
> separate patch.

How does that affect the code generation?

My sense is that computing current once in flush_old_exec is much
better than computing it in each function flush_old_exec calls.
I remember that computing current used to be not expensive but
noticable.

For clarity I can see renaming tsk to me.  So that it is clear we are
talking about the current process, and not some arbitrary process.

And for clarity my goal here is not to clean up de_thread.  Though
I don't mind that result.  My goal is to get the extra work out of
de_thread so we can do process tear down cleanups that are safe
according to the ordinary process rules, before taking a mutex that
protects exec mucking with all of the state in exec.

Eric


> diff --git a/fs/exec.c b/fs/exec.c
> index db17be51b112..ee108707e4b0 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1061,8 +1061,9 @@ 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(void)
>  {
> +       struct task_struct *tsk = current;
>         struct signal_struct *sig = tsk->signal;
>         struct sighand_struct *oldsighand = tsk->sighand;
>         spinlock_t *lock = &oldsighand->siglock;
> @@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm)
>          * Make sure we have a private signal table and that
>          * we are unassociated from the previous thread group.
>          */
> -       retval = de_thread(current);
> +       retval = de_thread();
>         if (retval)
>                 goto out;
Christian Brauner March 9, 2020, 8:17 p.m. UTC | #4
On Mon, Mar 09, 2020 at 03:06:46PM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
> > On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
> >> 
> >> These functions have very little to do with de_thread move them out
> >> of de_thread an into flush_old_exec proper so it can be more clearly
> >> seen what flush_old_exec is doing.
> >> 
> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> ---
> >>  fs/exec.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/fs/exec.c b/fs/exec.c
> >> index ff74b9a74d34..215d86f77b63 100644
> >> --- a/fs/exec.c
> >> +++ b/fs/exec.c
> >> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
> >
> > While you're cleaning up de_thread() wouldn't it be good to also take
> > the opportunity and remove the task argument from de_thread(). It's only
> > ever used with current. Could be done in one of your patches or as a
> > separate patch.
> 
> How does that affect the code generation?

The same way renaming "tsk" to "me" does.

> 
> My sense is that computing current once in flush_old_exec is much
> better than computing it in each function flush_old_exec calls.
> I remember that computing current used to be not expensive but
> noticable.
> 
> For clarity I can see renaming tsk to me.  So that it is clear we are
> talking about the current process, and not some arbitrary process.

For clarity since de_thread() uses "tsk" giving the impression that any
task can be dethreaded while it's only ever used with current. It's just
a suggestion since you're doing the rename tsk->me anyway it would fit
with the series. You do whatever you want though.
(I just remember that the same request was made once to changes I did:
Don't pass current as arg when it's the only task passed.)
Eric W. Biederman March 9, 2020, 8:48 p.m. UTC | #5
Christian Brauner <christian.brauner@ubuntu.com> writes:

> On Mon, Mar 09, 2020 at 03:06:46PM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brauner@ubuntu.com> writes:
>> 
>> > On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
>> >> 
>> >> These functions have very little to do with de_thread move them out
>> >> of de_thread an into flush_old_exec proper so it can be more clearly
>> >> seen what flush_old_exec is doing.
>> >> 
>> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> >> ---
>> >>  fs/exec.c | 10 +++++-----
>> >>  1 file changed, 5 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/fs/exec.c b/fs/exec.c
>> >> index ff74b9a74d34..215d86f77b63 100644
>> >> --- a/fs/exec.c
>> >> +++ b/fs/exec.c
>> >> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>> >
>> > While you're cleaning up de_thread() wouldn't it be good to also take
>> > the opportunity and remove the task argument from de_thread(). It's only
>> > ever used with current. Could be done in one of your patches or as a
>> > separate patch.
>> 
>> How does that affect the code generation?
>
> The same way renaming "tsk" to "me" does.
>
>> 
>> My sense is that computing current once in flush_old_exec is much
>> better than computing it in each function flush_old_exec calls.
>> I remember that computing current used to be not expensive but
>> noticable.
>> 
>> For clarity I can see renaming tsk to me.  So that it is clear we are
>> talking about the current process, and not some arbitrary process.
>
> For clarity since de_thread() uses "tsk" giving the impression that any
> task can be dethreaded while it's only ever used with current. It's just
> a suggestion since you're doing the rename tsk->me anyway it would fit
> with the series. You do whatever you want though.
> (I just remember that the same request was made once to changes I did:
> Don't pass current as arg when it's the only task passed.)

That's fair.

And I completely agree that we should at least rename tsk to me.
Just for clarity.

My apologies if I am a little short.  My little son has been an extra
handful lately.

Eric
Christian Brauner March 10, 2020, 8:55 a.m. UTC | #6
On Mon, Mar 09, 2020 at 03:48:55PM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@ubuntu.com> writes:
> 
> > On Mon, Mar 09, 2020 at 03:06:46PM -0500, Eric W. Biederman wrote:
> >> Christian Brauner <christian.brauner@ubuntu.com> writes:
> >> 
> >> > On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
> >> >> 
> >> >> These functions have very little to do with de_thread move them out
> >> >> of de_thread an into flush_old_exec proper so it can be more clearly
> >> >> seen what flush_old_exec is doing.
> >> >> 
> >> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> >> ---
> >> >>  fs/exec.c | 10 +++++-----
> >> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >> >> 
> >> >> diff --git a/fs/exec.c b/fs/exec.c
> >> >> index ff74b9a74d34..215d86f77b63 100644
> >> >> --- a/fs/exec.c
> >> >> +++ b/fs/exec.c
> >> >> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
> >> >
> >> > While you're cleaning up de_thread() wouldn't it be good to also take
> >> > the opportunity and remove the task argument from de_thread(). It's only
> >> > ever used with current. Could be done in one of your patches or as a
> >> > separate patch.
> >> 
> >> How does that affect the code generation?
> >
> > The same way renaming "tsk" to "me" does.
> >
> >> 
> >> My sense is that computing current once in flush_old_exec is much
> >> better than computing it in each function flush_old_exec calls.
> >> I remember that computing current used to be not expensive but
> >> noticable.
> >> 
> >> For clarity I can see renaming tsk to me.  So that it is clear we are
> >> talking about the current process, and not some arbitrary process.
> >
> > For clarity since de_thread() uses "tsk" giving the impression that any
> > task can be dethreaded while it's only ever used with current. It's just
> > a suggestion since you're doing the rename tsk->me anyway it would fit
> > with the series. You do whatever you want though.
> > (I just remember that the same request was made once to changes I did:
> > Don't pass current as arg when it's the only task passed.)
> 
> That's fair.
> 
> And I completely agree that we should at least rename tsk to me.
> Just for clarity.
> 
> My apologies if I am a little short.  My little son has been an extra
> handful lately.

No worries, stress is a thing most of us know too well.

Christian
Kees Cook March 10, 2020, 8:16 p.m. UTC | #7
On Mon, Mar 09, 2020 at 03:48:55PM -0500, Eric W. Biederman wrote:
> And I completely agree that we should at least rename tsk to me.
> Just for clarity.

I think it wouldn't hurt to add comments to spell it out explicitly
in each of the tsk->me functions, something like:

/*
 * The "me" task_struct argument here must only ever refer to "current",
 * but it gets passed in to avoid re-calculating "current" in each helper.
 */

I've found that the exec code in its entirety would be better off with
more comments. :) Usually that's the bulk of what I find myself adding
when I make changes in this area. ;)

-Kees
Kees Cook March 10, 2020, 8:31 p.m. UTC | #8
On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
> 
> These functions have very little to do with de_thread move them out
> of de_thread an into flush_old_exec proper so it can be more clearly
> seen what flush_old_exec is doing.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/exec.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index ff74b9a74d34..215d86f77b63 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>  	/* we have changed execution domain */
>  	tsk->exit_signal = SIGCHLD;
>  
> -#ifdef CONFIG_POSIX_TIMERS
> -	exit_itimers(sig);
> -	flush_itimer_signals();
> -#endif
> -
>  	BUG_ON(!thread_group_leader(tsk));
>  	return 0;
>  
> @@ -1277,6 +1272,11 @@ 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 twitch at seeing #ifdefs in .c instead of hidden in the .h declarations
of these two functions, but as this is a copy/paste, I'll live. ;)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>  	/*
>  	 * Make the signal table private.
>  	 */
> -- 
> 2.25.0
>
Jann Horn March 10, 2020, 8:57 p.m. UTC | #9
On Sun, Mar 8, 2020 at 10:39 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> These functions have very little to do with de_thread move them out
> of de_thread an into flush_old_exec proper so it can be more clearly
> seen what flush_old_exec is doing.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/exec.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index ff74b9a74d34..215d86f77b63 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>         /* we have changed execution domain */
>         tsk->exit_signal = SIGCHLD;
>
> -#ifdef CONFIG_POSIX_TIMERS
> -       exit_itimers(sig);
> -       flush_itimer_signals();
> -#endif
> -
>         BUG_ON(!thread_group_leader(tsk));
>         return 0;
>
> @@ -1277,6 +1272,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>         if (retval)
>                 goto out;
>
> +#ifdef CONFIG_POSIX_TIMERS
> +       exit_itimers(me->signal);
> +       flush_itimer_signals();
> +#endif

nit: exit_itimers() has a comment referring to de_thread, that should
probably be updated
Eric W. Biederman March 10, 2020, 9:05 p.m. UTC | #10
Jann Horn <jannh@google.com> writes:

> On Sun, Mar 8, 2020 at 10:39 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> These functions have very little to do with de_thread move them out
>> of de_thread an into flush_old_exec proper so it can be more clearly
>> seen what flush_old_exec is doing.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/exec.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index ff74b9a74d34..215d86f77b63 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1189,11 +1189,6 @@ static int de_thread(struct task_struct *tsk)
>>         /* we have changed execution domain */
>>         tsk->exit_signal = SIGCHLD;
>>
>> -#ifdef CONFIG_POSIX_TIMERS
>> -       exit_itimers(sig);
>> -       flush_itimer_signals();
>> -#endif
>> -
>>         BUG_ON(!thread_group_leader(tsk));
>>         return 0;
>>
>> @@ -1277,6 +1272,11 @@ int flush_old_exec(struct linux_binprm * bprm)
>>         if (retval)
>>                 goto out;
>>
>> +#ifdef CONFIG_POSIX_TIMERS
>> +       exit_itimers(me->signal);
>> +       flush_itimer_signals();
>> +#endif
>
> nit: exit_itimers() has a comment referring to de_thread, that should
> probably be updated

Good point.

Eric
Christian Brauner March 10, 2020, 9:22 p.m. UTC | #11
On Sun, Mar 08, 2020 at 04:36:55PM -0500, Eric W. Biederman wrote:
> 
> These functions have very little to do with de_thread move them out
> of de_thread an into flush_old_exec proper so it can be more clearly
> seen what flush_old_exec is doing.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Patch
diff mbox series

diff --git a/fs/exec.c b/fs/exec.c
index ff74b9a74d34..215d86f77b63 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1189,11 +1189,6 @@  static int de_thread(struct task_struct *tsk)
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
 
-#ifdef CONFIG_POSIX_TIMERS
-	exit_itimers(sig);
-	flush_itimer_signals();
-#endif
-
 	BUG_ON(!thread_group_leader(tsk));
 	return 0;
 
@@ -1277,6 +1272,11 @@  int flush_old_exec(struct linux_binprm * bprm)
 	if (retval)
 		goto out;
 
+#ifdef CONFIG_POSIX_TIMERS
+	exit_itimers(me->signal);
+	flush_itimer_signals();
+#endif
+
 	/*
 	 * Make the signal table private.
 	 */