From: Eric W. Biederman <hidden> Date: 2009-10-23 01:03:53
Sukadev Bhattiprolu [off-list ref] writes:
Eric W. Biederman [ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org] wrote:
| +static int set_pidmap(struct pid_namespace *pid_ns, int target)
| +{
| + if (target >= pid_max)
| + return -1;
I am changing this and the next return to 'return -EINVAL', to match
an earlier patch in my patchset.
| + if (target < RESERVED_PIDS)
Should we replace RESERVED_PIDS with 0 ? We currently allow new
containers to have pids 1..32K in the first pass and in subsequent
passes assign starting at RESERVED_PIDS.
If it is a preexisting namespace pid namespace removing the RESERVED_PIDS
check removes most if not all of the point of RESERVED_PIDS.
In a new fresh pid namespace I have no problem with not performing
the RESERVED_PIDS check.
So I guess that makes the check.
if ((target < RESERVED_PIDS) && pid_ns->last_pid >= RESERVED_PIDS)
return -EINVAL;
Eric
Eric W. Biederman [ebiederm@xmission.com] wrote:
| > | + if (target < RESERVED_PIDS)
| >
| > Should we replace RESERVED_PIDS with 0 ? We currently allow new
| > containers to have pids 1..32K in the first pass and in subsequent
| > passes assign starting at RESERVED_PIDS.
|
| If it is a preexisting namespace pid namespace removing the RESERVED_PIDS
| check removes most if not all of the point of RESERVED_PIDS.
|
| In a new fresh pid namespace I have no problem with not performing
| the RESERVED_PIDS check.
In that case can we do this
if (target_pid < RESERVED_PIDS && !pid_ns->level)
return -EINVAL;
instead ?
|
| So I guess that makes the check.
|
| if ((target < RESERVED_PIDS) && pid_ns->last_pid >= RESERVED_PIDS)
| return -EINVAL;
I am just wondering if there is a small corner case where C/R would randomly
fail because of this sequence:
- C/R code calls clone() or clone3() say about RESERVED_PIDS-1
times and ->last_pid == RESERVED_PIDS-1.
- C/R code calls normal fork()/alloc_pidmap() for a short-lived
child - its pid == ->last_pid == RESERVED_PIDS
- C/R code then calls clone3()/set_pidmap() to set the pid of
a new child to RESERVED_PID but fails (i.e it fails to restore
a pid even when the pid is not in use).
We could argue that mixing alloc_pidmap() and set_pidmap() during restart
is bad since set_pidmap() may fail.
The C/R developer could argue that we are forcing them to specify a pid
even for a short lived process that they wait()s on and thus ensure that
pid is not in use.
Anyway, is RESERVED_PIDS meant for initial kernel-threads/daemons - if so
would it be ok enforce it only in init_pid_ns ?
Sukadev
Eric W. Biederman [ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org] wrote:
| > | + if (target < RESERVED_PIDS)
| >
| > Should we replace RESERVED_PIDS with 0 ? We currently allow new
| > containers to have pids 1..32K in the first pass and in subsequent
| > passes assign starting at RESERVED_PIDS.
|
| If it is a preexisting namespace pid namespace removing the RESERVED_PIDS
| check removes most if not all of the point of RESERVED_PIDS.
|
| In a new fresh pid namespace I have no problem with not performing
| the RESERVED_PIDS check.
In that case can we do this
if (target_pid < RESERVED_PIDS && !pid_ns->level)
return -EINVAL;
instead ?
|
| So I guess that makes the check.
|
| if ((target < RESERVED_PIDS) && pid_ns->last_pid >= RESERVED_PIDS)
| return -EINVAL;
I am just wondering if there is a small corner case where C/R would randomly
fail because of this sequence:
- C/R code calls clone() or clone3() say about RESERVED_PIDS-1
times and ->last_pid == RESERVED_PIDS-1.
- C/R code calls normal fork()/alloc_pidmap() for a short-lived
child - its pid == ->last_pid == RESERVED_PIDS
- C/R code then calls clone3()/set_pidmap() to set the pid of
a new child to RESERVED_PID but fails (i.e it fails to restore
a pid even when the pid is not in use).
Not only for short-lived children. The problem is restart will succeed
or fail depending on the order in which tasks were checkpointed. If
task with pid 290 is restarted after pid 305, restart will fail.
And because chekcpoint scans the task tree in a DFS manner, this is
more likely to happen than not.
I wonder why you'd like to restrict a pid-specific clone like that ?
It is already a privileged syscall, so it could be exempt. I suggest
that only regular clones will be constrained.
Oren.
Eric W. Biederman [ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org] wrote:
| > | + if (target < RESERVED_PIDS)
| >
| > Should we replace RESERVED_PIDS with 0 ? We currently allow new
| > containers to have pids 1..32K in the first pass and in subsequent
| > passes assign starting at RESERVED_PIDS.
|
| If it is a preexisting namespace pid namespace removing the RESERVED_PIDS
| check removes most if not all of the point of RESERVED_PIDS.
|
| In a new fresh pid namespace I have no problem with not performing
| the RESERVED_PIDS check.
In that case can we do this
if (target_pid < RESERVED_PIDS && !pid_ns->level)
return -EINVAL;
instead ?
|
| So I guess that makes the check.
|
| if ((target < RESERVED_PIDS) && pid_ns->last_pid >= RESERVED_PIDS)
| return -EINVAL;
I am just wondering if there is a small corner case where C/R would randomly
fail because of this sequence:
- C/R code calls clone() or clone3() say about RESERVED_PIDS-1
times and ->last_pid == RESERVED_PIDS-1.
- C/R code calls normal fork()/alloc_pidmap() for a short-lived
child - its pid == ->last_pid == RESERVED_PIDS
- C/R code then calls clone3()/set_pidmap() to set the pid of
a new child to RESERVED_PID but fails (i.e it fails to restore
a pid even when the pid is not in use).
Not only for short-lived children. The problem is restart will succeed
or fail depending on the order in which tasks were checkpointed. If
task with pid 290 is restarted after pid 305, restart will fail.
And because chekcpoint scans the task tree in a DFS manner, this is
more likely to happen than not.
I wonder why you'd like to restrict a pid-specific clone like that ?
It is already a privileged syscall, so it could be exempt. I suggest
that only regular clones will be constrained.
I stand corrected by Suka: a pid-specific clone does not change
last_pid. Therefore, given that 'restart' only creates tasks with
pid-specific clone, this should be safe for c/r.
Oren.