[PATCH 2/3] pidhash: don't use zero pids

[PATCH 2/3] pidhash: don't use zero pids

[Oleg Nesterov]

daemonize() calls set_special_pids(1,1), while init and
kernel threads spawned from init/main.c:init() run with
0,0 special pids. This patch changes INIT_SIGNALS() so
that that they run with ->pgrp == ->session == 1 also.

This patch relies on fact that swapper's pid == 1.

Now we never use pid == 0 in kernel/pid.c.

[ Andrew, this patch obsoletes "[PATCH] pid: Don't hash pid 0.",
  filename pid-dont-hash-pid-0.patch ].

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- RC-1/include/linux/init_task.h~ZEROPID	2005-11-22 19:35:52.000000000 +0300
+++ RC-1/include/linux/init_task.h	2006-01-30 15:12:46.000000000 +0300
@@ -62,6 +62,8 @@
 	.posix_timers	 = LIST_HEAD_INIT(sig.posix_timers),		\
 	.cpu_timers	= INIT_CPU_TIMERS(sig.cpu_timers),		\
 	.rlim		= INIT_RLIMITS,					\
+	.pgrp		= 1,						\
+	.session	= 1,						\
 }
 
 #define INIT_SIGHAND(sighand) {						\

Re: [PATCH 2/3] pidhash: don't use zero pids

[Eric W. Biederman]

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

> daemonize() calls set_special_pids(1,1), while init and
> kernel threads spawned from init/main.c:init() run with
> 0,0 special pids. This patch changes INIT_SIGNALS() so
> that that they run with ->pgrp == ->session == 1 also.
>
> This patch relies on fact that swapper's pid == 1.
>
> Now we never use pid == 0 in kernel/pid.c.

This changes what is visible to user space, for the case
where we are not a member of a session of a process group.

By hashing the values these non-groups become available to
user space.  Which I find disturbing.  Before I can comment
further I need to see if there are any well defined semantics
for processes that are not part of a session or a process
group.  If there are well defined semantics we have just
broken user space.

Eric

Re: [PATCH 2/3] pidhash: don't use zero pids

[Kirill Korotaev]

Hello Oleg,

I had quite the same comment, but had no time to check it.
I can't understand what problem do you solve, or just making code 
cleaner (from your point of view)?
For me it was quite natural that pid=0 is used by idle, and I'm very 
suspicuos about such changes.

Kirill

> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
>> daemonize() calls set_special_pids(1,1), while init and
>> kernel threads spawned from init/main.c:init() run with
>> 0,0 special pids. This patch changes INIT_SIGNALS() so
>> that that they run with ->pgrp == ->session == 1 also.
>>
>> This patch relies on fact that swapper's pid == 1.
>>
>> Now we never use pid == 0 in kernel/pid.c.
> 
> This changes what is visible to user space, for the case
> where we are not a member of a session of a process group.
> 
> By hashing the values these non-groups become available to
> user space.  Which I find disturbing.  Before I can comment
> further I need to see if there are any well defined semantics
> for processes that are not part of a session or a process
> group.  If there are well defined semantics we have just
> broken user space.
> 
> Eric
> 

Re: [PATCH 2/3] pidhash: don't use zero pids

[Oleg Nesterov]

Hello Kirill,

Kirill Korotaev wrote:
> 
> Hello Oleg,
> 
> I had quite the same comment, but had no time to check it.
> I can't understand what problem do you solve, or just making code
> cleaner (from your point of view)?

Please look at http://marc.theaimsgroup.com/?t=113851660700001

> For me it was quite natural that pid=0 is used by idle, and I'm very
> suspicuos about such changes.

This patch does not change idle's pid, it is still 0. It changes ->pgrp
and ->session only from 0 to 1. Currently kernel threads run with 0,0
unless they call daemonize() which does set_special_pids(1, 1).

Oleg.

Re: [PATCH 2/3] pidhash: don't use zero pids

[Eric W. Biederman]

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

> Hello Kirill,
>
> Kirill Korotaev wrote:
>> 
>> Hello Oleg,
>> 
>> I had quite the same comment, but had no time to check it.
>> I can't understand what problem do you solve, or just making code
>> cleaner (from your point of view)?
>
> Please look at http://marc.theaimsgroup.com/?t=113851660700001
>
>> For me it was quite natural that pid=0 is used by idle, and I'm very
>> suspicuos about such changes.
>
> This patch does not change idle's pid, it is still 0. It changes ->pgrp
> and ->session only from 0 to 1. Currently kernel threads run with 0,0
> unless they call daemonize() which does set_special_pids(1, 1).


daemonize consuming pids (1,1) then consumes pgrp 1.  So that when
/sbin/init calls setsid() it thinks /sbin/init is a process group
leader and setsid() fails.  So /sbin/init wants pgrp 1 session 1
but doesn't get it.  I am pretty certain daemonize did not exist so
/sbin/init got pgrp 1 session 1 in 2.4.

That is the bug that is being fixed.

This patch takes things one step farther and essentially calls
setsid() for pid == 1 before init is execed.  That is new behavior
but it cleans up the kernel as we now do not need to support the
case of a process without a process group or a session.

The only process that could have possibly cared was /sbin/init
and it already calls setsid() because it doesn't want that.

If this was going to break anything noticeable the change in behavior
from 2.4 to 2.6 would have already done that.

Hopefully that is sufficiently comprehensible to everyone.

Eric

Re: [PATCH 2/3] pidhash: don't use zero pids

[Eric W. Biederman]

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

> daemonize() calls set_special_pids(1,1), while init and
> kernel threads spawned from init/main.c:init() run with
> 0,0 special pids. This patch changes INIT_SIGNALS() so
> that that they run with ->pgrp == ->session == 1 also.
>
> This patch relies on fact that swapper's pid == 1.

Looking more closely this appears to be a bug fix plain
and simple, and as the bug has existed for so long without
out anyone noticing it should not be a problem to fix it.

/sbin/init has been calling setsid() forever to ensure
it has a session and a process group of 1.  daemonize
using these ids and then being called before /sbin/init
called setsid, caused setsid to fail as the process group
already existed.

The only symptom I have noticed is that job control does
not work in 2.6 when you specify init=/bin/bash.   And I didn't
realized until I looked back at some older kernels that setsid
would have worked and they would not have had this problem.

If there ever was a reason we wanted to have processes that were
not part of process groups and sessions that reasons seems
lost in the mists of time and we can just forget about it.

/sbin/init does not test the return value of setsid() so
we should have no problems whatsoever.

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

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
> --- RC-1/include/linux/init_task.h~ZEROPID 2005-11-22 19:35:52.000000000 +0300
> +++ RC-1/include/linux/init_task.h	2006-01-30 15:12:46.000000000 +0300
> @@ -62,6 +62,8 @@
>  	.posix_timers	 = LIST_HEAD_INIT(sig.posix_timers),		\
>  	.cpu_timers	= INIT_CPU_TIMERS(sig.cpu_timers),		\
>  	.rlim		= INIT_RLIMITS,					\
> +	.pgrp		= 1,						\
> +	.session	= 1,						\
>  }
>  
>  #define INIT_SIGHAND(sighand) { \