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 <[email protected]>
---
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
[email protected] (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.
Andrew Morton <[email protected]> writes:
> [email protected] (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
>@@ -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
--
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.
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.
>> 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
--
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