Re: [PATCH] pid: Don't hash pid 0.

Re: [PATCH] pid: Don't hash pid 0.

[Oleg Nesterov]

Eric W. Biederman wrote:
>
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -148,6 +148,9 @@ int fastcall attach_pid(task_t *task, en
>  {
>  	struct pid *pid, *task_pid;
>  
> +	if (!nr)
> +		goto out;
> +
>  	task_pid = &task->pids[type];
>  	pid = find_pid(type, nr);
>  	task_pid->nr = nr;

If nr == 0 then task_pid->nr is uninitialized, so

> @@ -169,6 +172,9 @@ static fastcall int __detach_pid(task_t 
>  	int nr = 0;
>  
>  	pid = &task->pids[type];
> +	if (!pid->nr)
> +		goto out;

this is unsafe.

Yes, INIT_TASK() sets pids[...].nr == 0, but this is fragile and at
least needs a comment.

Eric, Andrew, I think I have a better patch, will post in a minute.

Oleg.

[PATCH] pid: Don't hash pid 0.

[Eric W. Biederman]

pid 0 is never exported to userspace, so hashing it servers no useful
purpose.

Explicitly not hashing pid 0 allows struct pid to be marked as not
hashed, and it allows us to avoid checks if for pid 0 when searching
for processes to signal if pid 0 does not have a special meaning.

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


---

 kernel/pid.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

da30e3ccb4b506b79fe0e6439addbfc763e92e24
diff --git a/kernel/pid.c b/kernel/pid.c
index d2247dc..7890867 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -148,6 +148,9 @@ int fastcall attach_pid(task_t *task, en
 {
 	struct pid *pid, *task_pid;
 
+	if (!nr)
+		goto out;
+
 	task_pid = &task->pids[type];
 	pid = find_pid(type, nr);
 	task_pid->nr = nr;
@@ -159,7 +162,7 @@ int fastcall attach_pid(task_t *task, en
 		INIT_HLIST_NODE(&task_pid->pid_chain);
 		list_add_tail_rcu(&task_pid->pid_list, &pid->pid_list);
 	}
-
+ out:
 	return 0;
 }
 
@@ -169,6 +172,9 @@ static fastcall int __detach_pid(task_t 
 	int nr = 0;
 
 	pid = &task->pids[type];
+	if (!pid->nr)
+		goto out;
+
 	if (!hlist_unhashed(&pid->pid_chain)) {
 
 		if (list_empty(&pid->pid_list)) {
@@ -185,7 +191,7 @@ static fastcall int __detach_pid(task_t 
 
 	list_del_rcu(&pid->pid_list);
 	pid->nr = 0;
-
+ out:
 	return nr;
 }
 
-- 
1.1.5.g3480

Re: [PATCH] pid: Don't hash pid 0.

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> pid 0 is never exported to userspace, so hashing it servers no useful
>  purpose.

uh-huh.

>  Explicitly not hashing pid 0 allows struct pid to be marked as not
>  hashed, and it allows us to avoid checks if for pid 0 when searching
>  for processes to signal if pid 0 does not have a special meaning.

Were you planning on sending a patch to avoid those checks?  Because right
now, this patch looks like a net loss.

Re: [PATCH] pid: Don't hash pid 0.

[Eric W. Biederman]

Andrew Morton <akpm@osdl.org> writes:

> ebiederm@xmission.com (Eric W. Biederman) wrote:
>>
>> pid 0 is never exported to userspace, so hashing it servers no useful
>>  purpose.
>
> uh-huh.
>
>>  Explicitly not hashing pid 0 allows struct pid to be marked as not
>>  hashed, and it allows us to avoid checks if for pid 0 when searching
>>  for processes to signal if pid 0 does not have a special meaning.
>
> Were you planning on sending a patch to avoid those checks?  Because right
> now, this patch looks like a net loss.

Yes.

I just asked for comments on linux-kernel.

Before I go further I feel honor bound to explain my biases.

.....

I am stuck out in no man land on clusters without customary OS services.
Because of their distributed nature I have applications I cannot swap,
can barely suspend, and cannot migrate to other cpus.

To solve that problem it looks to me like I need multiple namespace support
in the OS so when I migrate an application I can capture all of the OS
state the application depends on.

With respect to namespaces I have done some proof of concept implementations
and now that I know that what I want is possible I am working on a production
quality implementation.

I am starting with working my way towards multiple instances of a PID
namespace in the kernel.

What you have seen are the first couple of cleanups to get the worst
of the offenders from being a hinderance.

The next step is to fix the problem of pid reference wrap around,
inside the kernel.  At least this looks to be a promising direction
to solve a rare case and lay the foundation that I will need for
multiple instances of the pid namespace.

I am doing my best to work with all interested parties and to provide
a good solution that works for everyone and not just me.  Taking
everything in small obviously correct patches as I go.

Eric

Re: [PATCH] pid: Don't hash pid 0.

[Jan Engelhardt]

>@@ -148,6 +148,9 @@ int fastcall attach_pid(task_t *task, en
> {
> 	struct pid *pid, *task_pid;
> 
>+	if (!nr)
>+		goto out;
>+

How about nr==0, it would make it more obvious.



Jan Engelhardt
-- 

Re: [PATCH] pid: Don't hash pid 0.

[Yuki Cuss]

Jan Engelhardt wrote:

>>@@ -148,6 +148,9 @@ int fastcall attach_pid(task_t *task, en
>>{
>>	struct pid *pid, *task_pid;
>>
>>+	if (!nr)
>>+		goto out;
>>+
>>    
>>
>
>How about nr==0, it would make it more obvious.
>
>
>
>Jan Engelhardt
>  
>

I am inclined to agree. `!nr' seems to imply some sort of an error 
condition; perhaps a comment could be placed in order to make why the 
case of (nr == 0) is being ignored.

 - Yuki.

Re: [PATCH] pid: Don't hash pid 0.

[Jan Engelhardt]

>> How about nr==0, it would make it more obvious.
>
> I am inclined to agree. `!nr' seems to imply some sort of an error condition;

! seems to imply a boolean usually. (If this was Java, this would even 
be enforced.)
However, !x (and x) is scattered all around the kernel where 
x==0,x!=0 (or x==NULL,x!=NULL) would be more readable.

> perhaps a comment could be placed in order to make why the case of (nr == 0) is
> being ignored.
>
> - Yuki.
>

Jan Engelhardt
-- 

Re: [PATCH] pid: Don't hash pid 0.

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

Hi.

On Monday 30 January 2006 19:49, Jan Engelhardt wrote:
> >> How about nr==0, it would make it more obvious.
> >
> > I am inclined to agree. `!nr' seems to imply some sort of an error
> > condition;
>
> ! seems to imply a boolean usually. (If this was Java, this would even
> be enforced.)

If this was Java! Thank goodness it's not :>

Nigel

> However, !x (and x) is scattered all around the kernel where
> x==0,x!=0 (or x==NULL,x!=NULL) would be more readable.
>
> > perhaps a comment could be placed in order to make why the case of (nr ==
> > 0) is being ignored.
> >
> > - Yuki.
>
> Jan Engelhardt

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]