2006-02-08 16:10:58

by Eric W. Biederman

Subject: [PATCH] fork: Allow init to become a session leader.

With the bug fixes from killing session == 0 and pgrp == 0 we
have essentially made pid == 1 a session leader. However reading
through the code I can see nothing, that sets the session->leader
flag. In fact we actively clear it in all cases during clone.
And setsid will fail to set it because the session == 1 and
process group == 1 already exist.

So this patch forces the session leader flag and for good measure
the pgrp, session and tty of init as well.

Signed-off-by: Eric W. Biederman <[email protected]>


---

kernel/fork.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

6bc9fa2aca38bf739e20ae6192a068310ff9739a
diff --git a/kernel/fork.c b/kernel/fork.c
index a08e5cf..ff10b11 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1179,9 +1179,16 @@ static task_t *copy_process(unsigned lon
attach_pid(p, PIDTYPE_PID, p->pid);
attach_pid(p, PIDTYPE_TGID, p->tgid);
if (thread_group_leader(p)) {
- p->signal->tty = current->signal->tty;
- p->signal->pgrp = process_group(current);
- p->signal->session = current->signal->session;
+ if (unlikely(p->pid == 1)) {
+ p->signal->tty = NULL;
+ p->signal->leader = 1;
+ p->signal->pgrp = 1;
+ p->signal->session = 1;
+ } else {
+ p->signal->tty = current->signal->tty;
+ p->signal->pgrp = process_group(current);
+ p->signal->session = current->signal->session;
+ }
attach_pid(p, PIDTYPE_PGID, process_group(p));
attach_pid(p, PIDTYPE_SID, p->signal->session);
__get_cpu_var(process_counts)++;
--
1.1.5.g3480

2006-02-08 17:11:43

by Oleg Nesterov

Subject: Re: [PATCH] fork: Allow init to become a session leader.

"Eric W. Biederman" wrote:
>
> With the bug fixes from killing session == 0 and pgrp == 0 we
> have essentially made pid == 1 a session leader. However reading
> through the code I can see nothing, that sets the session->leader
> flag. In fact we actively clear it in all cases during clone.
> And setsid will fail to set it because the session == 1 and
> process group == 1 already exist.
>
> So this patch forces the session leader flag and for good measure
> the pgrp, session and tty of init as well.
>
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1179,9 +1179,16 @@ static task_t *copy_process(unsigned lon
> attach_pid(p, PIDTYPE_PID, p->pid);
> attach_pid(p, PIDTYPE_TGID, p->tgid);
> if (thread_group_leader(p)) {
> - p->signal->tty = current->signal->tty;
> - p->signal->pgrp = process_group(current);
> - p->signal->session = current->signal->session;
> + if (unlikely(p->pid == 1)) {
> + p->signal->tty = NULL;
> + p->signal->leader = 1;
> + p->signal->pgrp = 1;
> + p->signal->session = 1;

Isn't it enough to just set current->signal->leader = 1
in init/main.c:init() ? This process was already forked
with (1,1) special pids and ->tty == NULL.

Oleg.

2006-02-08 17:30:41

by Eric W. Biederman

Subject: Re: [PATCH] fork: Allow init to become a session leader.

Oleg Nesterov <[email protected]> writes:

> "Eric W. Biederman" wrote:
>>
>> With the bug fixes from killing session == 0 and pgrp == 0 we
>> have essentially made pid == 1 a session leader. However reading
>> through the code I can see nothing, that sets the session->leader
>> flag. In fact we actively clear it in all cases during clone.
>> And setsid will fail to set it because the session == 1 and
>> process group == 1 already exist.
>>
>> So this patch forces the session leader flag and for good measure
>> the pgrp, session and tty of init as well.
>>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1179,9 +1179,16 @@ static task_t *copy_process(unsigned lon
>> attach_pid(p, PIDTYPE_PID, p->pid);
>> attach_pid(p, PIDTYPE_TGID, p->tgid);
>> if (thread_group_leader(p)) {
>> - p->signal->tty = current->signal->tty;
>> - p->signal->pgrp = process_group(current);
>> - p->signal->session = current->signal->session;
>> + if (unlikely(p->pid == 1)) {
>> + p->signal->tty = NULL;
>> + p->signal->leader = 1;
>> + p->signal->pgrp = 1;
>> + p->signal->session = 1;
>
> Isn't it enough to just set current->signal->leader = 1
> in init/main.c:init() ? This process was already forked
> with (1,1) special pids and ->tty == NULL.

At the moment yes. Being explicit can help readability.

This allows us the opportunity to leave the unhashed idle task as
(0,0) and more interesting to me, if we ever fork another process with
pid == 1 in a different process id namespace we do need to do all of
this.

Eric