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 <[email protected]>
---
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(¤t->tasks);
list_add_tail(¤t->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
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.
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.
Oleg Nesterov <[email protected]> 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
Oleg Nesterov <[email protected]> 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
"Eric W. Biederman" wrote:
>
> Oleg Nesterov <[email protected]> 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.
Oleg Nesterov <[email protected]> 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