[PATCH] exec: Cleanup exec from a non thread group leader.

[PATCH] exec: Cleanup exec from a non thread group leader.

[Eric W. Biederman]

This patch modifies switch_exec_pids so that it uses the
normal attach_pid/detach_pid functions.  This makes
the code more maintainable and it removes a race
where find_pid could fail to find a thread group or
a process id that currently exists.

We also now preserve the exit_signal of our thread group
leader when we call exec (when we take over the thread
group leaders identity).

And for good measure we set the thread group leaders
exit_signal to -1 so it will self reap.  We are actually
past the point where that matters but it can't hurt, and
it might help someday.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/exec.c    |    3 ++-
 kernel/pid.c |   33 +++++++++++++--------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

dab45943cf60c11f4432d6fdd26d68eb7092b8dd
diff --git a/fs/exec.c b/fs/exec.c
index c9d8e31..922dbee 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -721,10 +721,11 @@ static int de_thread(struct task_struct 
 
 		list_del(&current->tasks);
 		list_add_tail(&current->tasks, &init_task.tasks);
-		current->exit_signal = SIGCHLD;
+		current->exit_signal = leader->exit_signal;
 
 		BUG_ON(leader->exit_state != EXIT_ZOMBIE);
 		leader->exit_state = EXIT_DEAD;
+		leader->exit_signal = -1;
 
 		write_unlock_irq(&tasklist_lock);
 		spin_unlock(&leader->proc_lock);
diff --git a/kernel/pid.c b/kernel/pid.c
index 1acc072..d2247dc 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -220,31 +220,24 @@ EXPORT_SYMBOL(find_task_by_pid_type);
 /*
  * This function switches the PIDs if a non-leader thread calls
  * sys_execve() - this must be done without releasing the PID.
- * (which a detach_pid() would eventually do.)
+ *
+ * The attach and detach operations have been carefully
+ * ordered so there is never an instant that pids that will
+ * survive are absent from the hash table.  This ensures
+ * that we don't release pids we mean to keep.
  */
 void switch_exec_pids(task_t *leader, task_t *thread)
 {
-	__detach_pid(leader, PIDTYPE_PID);
-	__detach_pid(leader, PIDTYPE_TGID);
-	__detach_pid(leader, PIDTYPE_PGID);
-	__detach_pid(leader, PIDTYPE_SID);
-
-	__detach_pid(thread, PIDTYPE_PID);
-	__detach_pid(thread, PIDTYPE_TGID);
-
-	leader->pid = leader->tgid = thread->pid;
-	thread->pid = thread->tgid;
-
-	attach_pid(thread, PIDTYPE_PID, thread->pid);
-	attach_pid(thread, PIDTYPE_TGID, thread->tgid);
+	detach_pid(thread, PIDTYPE_PID);
+	thread->pid = leader->pid;
+	attach_pid(thread, PIDTYPE_PID,  thread->pid);
 	attach_pid(thread, PIDTYPE_PGID, thread->signal->pgrp);
-	attach_pid(thread, PIDTYPE_SID, thread->signal->session);
-	list_add_tail(&thread->tasks, &init_task.tasks);
+	attach_pid(thread, PIDTYPE_SID,  thread->signal->session);
 
-	attach_pid(leader, PIDTYPE_PID, leader->pid);
-	attach_pid(leader, PIDTYPE_TGID, leader->tgid);
-	attach_pid(leader, PIDTYPE_PGID, leader->signal->pgrp);
-	attach_pid(leader, PIDTYPE_SID, leader->signal->session);
+	detach_pid(leader, PIDTYPE_PID);
+	detach_pid(leader, PIDTYPE_TGID);
+	detach_pid(leader, PIDTYPE_PGID);
+	detach_pid(leader, PIDTYPE_SID);
 }
 
 /*
-- 
1.1.5.g3480

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Oleg Nesterov]

Eric W. Biederman wrote:
>
> And for good measure we set the thread group leaders
> exit_signal to -1 so it will self reap.  We are actually
> past the point where that matters but it can't hurt, and
> it might help someday.
> ...
>               leader->exit_state = EXIT_DEAD;
> +             leader->exit_signal = -1;

I disagree. The leader is already practically reaped, it is EXIT_DEAD.
I think this change will confuse the reader who will try to understand
why do we need this subtle assignment.

>  void switch_exec_pids(task_t *leader, task_t *thread)
>  {
> -	__detach_pid(leader, PIDTYPE_PID);
> -	__detach_pid(leader, PIDTYPE_TGID);
> -	__detach_pid(leader, PIDTYPE_PGID);
> -	__detach_pid(leader, PIDTYPE_SID);
> -
> -	__detach_pid(thread, PIDTYPE_PID);
> -	__detach_pid(thread, PIDTYPE_TGID);
> -
> -	leader->pid = leader->tgid = thread->pid;
> -	thread->pid = thread->tgid;
> -
> -	attach_pid(thread, PIDTYPE_PID, thread->pid);
> -	attach_pid(thread, PIDTYPE_TGID, thread->tgid);
> +	detach_pid(thread, PIDTYPE_PID);
> +	thread->pid = leader->pid;
> +	attach_pid(thread, PIDTYPE_PID,  thread->pid);
>  	attach_pid(thread, PIDTYPE_PGID, thread->signal->pgrp);
> -	attach_pid(thread, PIDTYPE_SID, thread->signal->session);
> -	list_add_tail(&thread->tasks, &init_task.tasks);

The last deletion is wrong, I beleive.

> +	attach_pid(thread, PIDTYPE_SID,  thread->signal->session);
>  
> -	attach_pid(leader, PIDTYPE_PID, leader->pid);
> -	attach_pid(leader, PIDTYPE_TGID, leader->tgid);
> -	attach_pid(leader, PIDTYPE_PGID, leader->signal->pgrp);
> -	attach_pid(leader, PIDTYPE_SID, leader->signal->session);
> +	detach_pid(leader, PIDTYPE_PID);
> +	detach_pid(leader, PIDTYPE_TGID);
> +	detach_pid(leader, PIDTYPE_PGID);
> +	detach_pid(leader, PIDTYPE_SID);
>  }

I think most of detach_pid()s could be replaced with __detach_pid(),
this will save unneccesary pid_hash scanning

Oleg.

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Oleg Nesterov]

Oleg Nesterov wrote:
> 
> Eric W. Biederman wrote:
> >
> > -     list_add_tail(&thread->tasks, &init_task.tasks);
> 
> The last deletion is wrong, I beleive.

Just to clarify, it looks like we can kill this line because
de_thread() also does list_add_tail(current, &init_task.tasks).

But please note that it (and probably __ptrace_link() above)
does list_del(current->task) first, and current->task may have
very stale values after old leader called dup_task_struct().
SET_LINKS() in copy_process() does nothing with ->tasks in a
CLONE_THREAD case.

Oleg.

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> Eric W. Biederman wrote:
>>
>> And for good measure we set the thread group leaders
>> exit_signal to -1 so it will self reap.  We are actually
>> past the point where that matters but it can't hurt, and
>> it might help someday.
>> ...
>>               leader->exit_state = EXIT_DEAD;
>> +             leader->exit_signal = -1;
>
> I disagree. The leader is already practically reaped, it is EXIT_DEAD.
> I think this change will confuse the reader who will try to understand
> why do we need this subtle assignment.

Six of one half dozen of the other.  It doesn't matter so I don't
care.

>>  void switch_exec_pids(task_t *leader, task_t *thread)
>>  {
>> -	__detach_pid(leader, PIDTYPE_PID);
>> -	__detach_pid(leader, PIDTYPE_TGID);
>> -	__detach_pid(leader, PIDTYPE_PGID);
>> -	__detach_pid(leader, PIDTYPE_SID);
>> -
>> -	__detach_pid(thread, PIDTYPE_PID);
>> -	__detach_pid(thread, PIDTYPE_TGID);
>> -
>> -	leader->pid = leader->tgid = thread->pid;
>> -	thread->pid = thread->tgid;
>> -
>> -	attach_pid(thread, PIDTYPE_PID, thread->pid);
>> -	attach_pid(thread, PIDTYPE_TGID, thread->tgid);
>> +	detach_pid(thread, PIDTYPE_PID);
>> +	thread->pid = leader->pid;
>> +	attach_pid(thread, PIDTYPE_PID,  thread->pid);
>>  	attach_pid(thread, PIDTYPE_PGID, thread->signal->pgrp);
>> -	attach_pid(thread, PIDTYPE_SID, thread->signal->session);
>> -	list_add_tail(&thread->tasks, &init_task.tasks);
>
> The last deletion is wrong, I beleive.

list_add_tail is duplicate code.  It is already present in the caller.
So it is noise and confusing to leave it here.
But you already noted that in the following email.


>> +	attach_pid(thread, PIDTYPE_SID,  thread->signal->session);
>>  
>> -	attach_pid(leader, PIDTYPE_PID, leader->pid);
>> -	attach_pid(leader, PIDTYPE_TGID, leader->tgid);
>> -	attach_pid(leader, PIDTYPE_PGID, leader->signal->pgrp);
>> -	attach_pid(leader, PIDTYPE_SID, leader->signal->session);
>> +	detach_pid(leader, PIDTYPE_PID);
>> +	detach_pid(leader, PIDTYPE_TGID);
>> +	detach_pid(leader, PIDTYPE_PGID);
>> +	detach_pid(leader, PIDTYPE_SID);
>>  }
>
> I think most of detach_pid()s could be replaced with __detach_pid(),
> this will save unneccesary pid_hash scanning

Actually 90% of the point was to remove the need for __detach_pid.
But you are right __detach_pid would be safe and we know that because
of the ordering.  At the same time because we are not the last reference
the code will never do that.

I need to relook at this.  To not conflict with your code some of
the detach_pids need to be removed so we don't unhash things twice.

Eric

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> Oleg Nesterov wrote:
>> 
>> Eric W. Biederman wrote:
>> >
>> > -     list_add_tail(&thread->tasks, &init_task.tasks);
>> 
>> The last deletion is wrong, I beleive.
>
> Just to clarify, it looks like we can kill this line because
> de_thread() also does list_add_tail(current, &init_task.tasks).
>
> But please note that it (and probably __ptrace_link() above)
> does list_del(current->task) first, and current->task may have
> very stale values after old leader called dup_task_struct().
> SET_LINKS() in copy_process() does nothing with ->tasks in a
> CLONE_THREAD case.

Good point in that instance we need to remove the list_del
as well.

As for the other stale data that bears looking at.

Eric

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Oleg Nesterov]

"Eric W. Biederman" wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > Oleg Nesterov wrote:
> >>
> >> Eric W. Biederman wrote:
> >> >
> >> > -     list_add_tail(&thread->tasks, &init_task.tasks);
> >>
> >> The last deletion is wrong, I beleive.
> >
> > Just to clarify, it looks like we can kill this line because
> > de_thread() also does list_add_tail(current, &init_task.tasks).
> >
> > But please note that it (and probably __ptrace_link() above)
> > does list_del(current->task) first, and current->task may have
> > very stale values after old leader called dup_task_struct().
> > SET_LINKS() in copy_process() does nothing with ->tasks in a
> > CLONE_THREAD case.
> 
> Good point in that instance we need to remove the list_del
> as well.

We can't just remove this list_del, note __ptrace_link() above.
So if we remove list_add from switch_exec_pids() (like you did
in your patch) we should also place list_add before ptrace_link()
in de_thread(), otherwise I beleive it is a bug.

I agree, we should cleanup this. I just noticed that I forgot
to add you on CC: list while sending this patch:

	http://marc.theaimsgroup.com/?l=linux-kernel&m=113862839924746

Btw, I don't understand why __ptrace_link() use REMOVE_LINKS/SET_LINKS
instead of remove_parent/add_parent.

Oleg.

Re: [PATCH] exec: Cleanup exec from a non thread group leader.

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> We can't just remove this list_del, note __ptrace_link() above.
> So if we remove list_add from switch_exec_pids() (like you did
> in your patch) we should also place list_add before ptrace_link()
> in de_thread(), otherwise I beleive it is a bug.

Ok.  I see it now.  The REMOVE_LINKS/SET_LINKS deep in __ptrace_link()
touching the task list is sneaky.

> I agree, we should cleanup this. I just noticed that I forgot
> to add you on CC: list while sending this patch:
>
> 	http://marc.theaimsgroup.com/?l=linux-kernel&m=113862839924746
>
> Btw, I don't understand why __ptrace_link() use REMOVE_LINKS/SET_LINKS
> instead of remove_parent/add_parent.

I see one of two possibilities.
- Either there is a magic invariant that is supposed to be preserved
  about always being on the task list with a parent.
  (And the code in this part of exec is already broken).
- Or the code is just being inefficient.

A corollary is why is any of this code safe to run without holding
the tasklist_lock?

Eric