fork: Add the ability to create tasks with given pids

kernel/pid.c static int alloc_pidmap(struct pid_namespace *pid_ns) · line 237

Quoted 12 lines · Pavel Emelyanov

kernel/pid.c +11 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	/*
+	 * When creating a new pid namespace we must make its init
+	 * have pid == 1 in it.
+	 */
+	if (pid_ns->child_reaper == NULL)
+		return 0;

Do we really need this check? please see below...

kernel/pid.c static int alloc_pidmap(struct pid_namespace *pid_ns) · line 245

Quoted 7 lines

kernel/pid.c +7 -0

+	/*
+	 * Don't allow to create a task with a pid which has recently
+	 * belonged to some other (dead already) task. Only init (of
+	 * a freshly created namespace) and his clones can do this.
+	 */
+	if (pid_ns->last_pid != 1)
+		return -EPERM;

->last_pid == 1. This means that pid_nr == 1 was already created in this namespace via CLONE_NEWPID, and the child with this pid must be ->child_reaper, no?

IOW. if copy_process() allocs the first pid in the new pid_ns, it always sets ->child_reaper.

Cough. I really think 45a68628 should be reverted ;) IMHO it complicates the understanding of CLONE_NEWPID logic.

Oleg.

.

Quoted 16 lines · Oleg Nesterov

kernel/pid.c static int alloc_pidmap(struct pid_namespace *pid_ns) · line 237

Quoted 12 lines · Pavel Emelyanov

kernel/pid.c +11 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	/*
+	 * When creating a new pid namespace we must make its init
+	 * have pid == 1 in it.
+	 */
+	if (pid_ns->child_reaper == NULL)
+		return 0;

Do we really need this check? please see below...

Quoted 11 lines

kernel/pid.c static int alloc_pidmap(struct pid_namespace *pid_ns) · line 245

Quoted 7 lines

kernel/pid.c +7 -0

+	/*
+	 * Don't allow to create a task with a pid which has recently
+	 * belonged to some other (dead already) task. Only init (of
+	 * a freshly created namespace) and his clones can do this.
+	 */
+	if (pid_ns->last_pid != 1)
+		return -EPERM;

->last_pid == 1. This means that pid_nr == 1 was already created in this namespace via CLONE_NEWPID, and the child with this pid must be ->child_reaper, no?

If you use the CLONE_NEWPID | CLONE_CHILD_USEPIDS

Quoted 2 lines

Cough. I really think 45a68628 should be reverted ;) IMHO it complicates the understanding of CLONE_NEWPID logic.

If we remove it, then it's OK to remove the check above, but in this case we make it possible to have an init with pid != 1. This is flexible, but ... strange.

When restoring a task (or a set of tasks) we need to recreate them with exactly the same pid(s) as they had before. [...]

Ok. Patches 1-2 look sane on their own. I think we can merge them regardless in the next cycle if Oleg & co agree. Oleg?

Patch 3 obviously is generating discussion and actually introduces new functionality, so this one is the contentious one..

Linus

Quoted 3 lines · Pavel Emelyanov

When restoring a task (or a set of tasks) we need to recreate them with exactly the same pid(s) as they had before. [...]

kernel/pid.c +11 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	/*
+	 * When creating a new pid namespace we must make its init
+	 * have pid == 1 in it.
+	 */
+	if (pid_ns->child_reaper == NULL)
+		return 0;

My previous objection was wrong, I should try to find something else ;)

Quoted 16 lines · Pavel Emelyanov

kernel/pid.c +15 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	/*
+	 * When creating a new pid namespace we must make its init
+	 * have pid == 1 in it.
+	 */
+	if (pid_ns->child_reaper == NULL)
+		return 0;
+
+	/*
+	 * Don't allow to create a task with a pid which has recently
+	 * belonged to some other (dead already) task. Only init (of

kernel/pid.c static int alloc_pidmap(struct pid_namespace *pid_ns) · line 247

Quoted 6 lines

kernel/pid.c +6 -0

+	 * a freshly created namespace) and his clones can do this.
+	 */
+	if (pid_ns->last_pid != 1)
+		return -EPERM;
+
+	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];

probably we should check that map < PIDMAP_ENTRIES ?

Oleg.

Gentlemen, please, find some time for this, your ACK/NACK on the API proposal is required badly.

The proposal is to introduce the CLONE_CHILD_USEPIDS flag for clone() syscall and pass the pids values in the child_tidptr. In order not to introduce the hole for the pid-reuse attack, using this flag will result in EPERM in case the pid namespace we're trying to create pid in has at least one pid (except for the init's one) generated with regular fork()/clone().

Currently Tejun and Oleg are worrying only about the intrusiveness of this approach, although Oleg agrees, that it solves all the problems it should. The previous attempts to implement the similar stuff stopped, but no objections against this were expressed. So the decision of whether it's OK to go this way or not is required.

The API will be used like in the code below

	/* restore new pid namespace with an init in it */
	pid = clone(CLONE_NEWPID);

Quoted 3 lines · Pavel Emelyanov

Gentlemen, please, find some time for this, your ACK/NACK on the API proposal is required badly.

Please.

Quoted 11 lines

The proposal is to introduce the CLONE_CHILD_USEPIDS flag for clone() syscall and pass the pids values in the child_tidptr. In order not to introduce the hole for the pid-reuse attack, using this flag will result in EPERM in case the pid namespace we're trying to create pid in has at least one pid (except for the init's one) generated with regular fork()/clone().

Currently Tejun and Oleg are worrying only about the intrusiveness of this approach, although Oleg agrees, that it solves all the problems it should. The previous attempts to implement the similar stuff stopped, but no objections against this were expressed. So the decision of whether it's OK to go this way or not is required.

Yes, personally I'd prefer /proc/set_last_pid (or something similar) which simply writes to pid_ns->last_pid. Perhaps it is less convenient from the user-space pov (serialization, security) but it is much simpler.

OTOH, I do not pretend I understand the user-space needs, so I won't argue. This series seems correct, the bugs we discussed are fixed.

But. Speaking of API, it differs a bit compared to the previous version...

Quoted 4 lines

The API will be used like in the code below

	/* restore new pid namespace with an init in it */
	pid = clone(CLONE_NEWPID);

Yes, CLONE_NEWPID | CLONE_CHILD_USEPIDS is not possible.

Then how the array of pids in child_tidptr[] can be useful? If CLONE_NEWPID can't restore the pid_nr's in the parent namespaces, then probably this doesn't makes sense at all?

IOW. I think we should either allow CLONE_NEWPID | CLONE_CHILD_USEPIDS (with additional check in set_pidmap() to ensure that CLONE_NEWPID
comes with child_tidptr[0] == 1), or we should treat the "overloaded"
child_tidptr as a simple pid_t.

Again, I won't insist. Just I want to be sure we do not miss something adding the new API.

Oleg.

Quoted 3 lines · Oleg Nesterov

But. Speaking of API, it differs a bit compared to the previous version...

Quoted 6 lines

Quoted 4 lines

The API will be used like in the code below

	/* restore new pid namespace with an init in it */
	pid = clone(CLONE_NEWPID);

Yes, CLONE_NEWPID | CLONE_CHILD_USEPIDS is not possible.

It should be.

Quoted 3 lines

Yes, personally I'd prefer /proc/set_last_pid (or something similar) which simply writes to pid_ns->last_pid. Perhaps it is less convenient from the user-space pov (serialization, security) but it is much simpler.

Yes, this is also possible. I have a working prototype of /proc/sys/kernel/ns_last_pid with the security issue solved, but setting sysctl then cloning seems more obfuscating to me than just passing an array of pids to clone.

Hello,

Quoted 4 lines · Pavel Emelyanov

Quoted 3 lines

Yes, personally I'd prefer /proc/set_last_pid (or something similar) which simply writes to pid_ns->last_pid. Perhaps it is less convenient from the user-space pov (serialization, security) but it is much simpler.

Quoted 3 lines

Yes, this is also possible. I have a working prototype of /proc/sys/kernel/ns_last_pid with the security issue solved, but setting sysctl then cloning seems more obfuscating to me than just passing an array of pids to clone.

Do you mind sharing the patch?

It doesn't have to be perfect. I'm just curious how it looks. IMHO the suggested pid array passing is good enough and not too intrusive but, if there's something simpler from kernel side, given that this is a very specialized interface, I think we definitely need to consider that.

Thank you.

Then I introduce the kernel.ns_last_pid sysctl that is allows for MAY_OPEN | MAP_WRITE for the namespace's init only and allows for MAY_WRITE for anyone else. Thus, if we want to write to this file from non-init task it must have the respective fd inherited from the init on fork. It works OK for checkpoint/restore.

The patch is:

kernel/pid_namespace.c +10 -0

index e9c9adc..3686a07 100644
@@ -15,6 +15,7 @@
 #include <linux/acct.h>
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
+#include <linux/sysctl.h>
 
 #define BITS_PER_PAGE		(PAGE_SIZE*8)
 

@@ -191,9 +192,54 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	return;
 }
 
+static int pid_ns_ctl_permissions(struct nsproxy *namespaces,
+			struct ctl_table *table, int op)
+{
+	int mode = 0644;
+
+	if ((op & MAY_OPEN) &&
+			current != namespaces->pid_ns->child_reaper)
+		/*
+		 * Writing to this sysctl is allowed only for init

Well, after a bit more thinking I found one more pros for this sysctl - when restoring a container we'll have the possibility to set the last_pid to what we want to prevent the pids reuse after the restore.

Quoted 9 lines

kernel/pid_namespace.c +9 -0

+static int pid_ns_ctl_permissions(struct nsproxy *namespaces,
+			struct ctl_table *table, int op)
+{
+	int mode = 0644;
+
+	if ((op & MAY_OPEN) &&
+			current != namespaces->pid_ns->child_reaper)
+		/*
+		 * Writing to this sysctl is allowed only for init

kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 213

Quoted 10 lines

kernel/pid_namespace.c +10 -0

+		 * and to whoever it grands the open file
+		 */
+		mode &= ~0222;
+
+	return sysctl_test_perm(mode, op);
+}
+
+static struct ctl_table_root pid_ns_root = {
+	.permissions = pid_ns_ctl_permissions,
+};

Hmmm... I hope this could be prettier. I'm having trouble following where the MAY_OPEN comes from. Can you please explain?

From this calltrace:

Can't we for now allow this for root and then later allow CAP_CHECKPOINT that Cyrill suggested? Or do we want to allow setting pids even w/o CR for NS creator?

kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 221

Quoted 7 lines

kernel/pid_namespace.c +7 -0

+static int pid_ns_ctl_handler(struct ctl_table *table, int write,
+		     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tmp = *table;
+	tmp.data = &current->nsproxy->pid_ns->last_pid;
+	return proc_dointvec(&tmp, write, buffer, lenp, ppos);
+}

Probably better to call set_last_pid() on write path instead?

Quoted 4 lines

Well, after a bit more thinking I found one more pros for this sysctl - when restoring a container we'll have the possibility to set the last_pid to what we want to prevent the pids reuse after the restore.

Hmmm... I personally like this one better. Restoring multilevel pids would be more tedious but should still be possible and I really like that it's staying out of clone path and all modifications are to ns and pid code. Oleg, what do you think?

Thank you.

Quoted 3 lines

Can't we for now allow this for root and then later allow CAP_CHECKPOINT that Cyrill suggested? Or do we want to allow setting pids even w/o CR for NS creator?

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

This whole userspace C/R stuff and being able to set the child's pid has potential of being very useful for GDB too, allowing a much better reimplementation of its old checkpointing feature [*], and allowing for a faster reverse debugging implementation, by being able to do faster rewinding -- restore snapshot and replay instructions up to N (by single stepping or running to breakpoint), rather than manually undoing the effects of each instruction, one by one.

IOW, root only would be a shame from GDB's perspective.

Hello,

Quoted 7 lines · Pedro Alves

This whole userspace C/R stuff and being able to set the child's pid has potential of being very useful for GDB too, allowing a much better reimplementation of its old checkpointing feature [*], and allowing for a faster reverse debugging implementation, by being able to do faster rewinding -- restore snapshot and replay instructions up to N (by single stepping or running to breakpoint), rather than manually undoing the effects of each instruction, one by one.

Quoted 1 line

IOW, root only would be a shame from GDB's perspective.

Would CAP_CHECKPOINT be a shame too?

I'm reluctant about priviledge
through fd inheritance mostly because of its unusualness. I don't think priv management is a good problem space for small creative solutions. We're much better off with mundane mechanisms which people are already familiar with and is easy to account for.

Quoted 1 line

Would CAP_CHECKPOINT be a shame too?

I think CAP_CHECKPOINT (or something through some LSM) would be definitely better.

Quoted 5 lines

I'm reluctant about priviledge
through fd inheritance mostly because of its unusualness. I don't think priv management is a good problem space for small creative solutions. We're much better off with mundane mechanisms which people are already familiar with and is easy to account for.

fd inheritance wouldn't work for gdb; a user spawned gdb wouldn't inherit an open fd to kernel.ns_last_pid from anywhere.

I see. So, let's do it for root for now and later add finer grained CAP as necessary/viable.

Pavel, what do you think?

Quoted 3 lines · Tejun Heo

I see. So, let's do it for root for now and later add finer grained CAP as necessary/viable.

Agreed.

Stupid off-topic question, I am just curious... Why /proc/sys dislikes chmod ? I do not understand this code, but it seems that it would be simple to change proc_sys_setattr() to respect ATTR_MODE and update PROC_I(inode)->sysctl_entry->mode, no?

Hello,

Quoted 11 lines · Pedro Alves

Quoted 1 line

Would CAP_CHECKPOINT be a shame too?

I think CAP_CHECKPOINT (or something through some LSM) would be definitely better.

Quoted 5 lines

I'm reluctant about priviledge
through fd inheritance mostly because of its unusualness. I don't think priv management is a good problem space for small creative solutions. We're much better off with mundane mechanisms which people are already familiar with and is easy to account for.

Quoted 2 lines

fd inheritance wouldn't work for gdb; a user spawned gdb wouldn't inherit an open fd to kernel.ns_last_pid from anywhere.

I see. So, let's do it for root for now and later add finer grained CAP as necessary/viable. Pavel, what do you think?

Thanks.

Quoted 14 lines · Tejun Heo

Hello,

Quoted 11 lines · Pedro Alves

Quoted 1 line

Would CAP_CHECKPOINT be a shame too?

I think CAP_CHECKPOINT (or something through some LSM) would be definitely better.

Quoted 5 lines

I'm reluctant about priviledge
through fd inheritance mostly because of its unusualness. I don't think priv management is a good problem space for small creative solutions. We're much better off with mundane mechanisms which people are already familiar with and is easy to account for.

Quoted 5 lines

Quoted 2 lines

fd inheritance wouldn't work for gdb; a user spawned gdb wouldn't inherit an open fd to kernel.ns_last_pid from anywhere.

I see. So, let's do it for root for now and later add finer grained CAP as necessary/viable. Pavel, what do you think?

OK, I'll send the respective patches soon.

Quoted 2 lines

Thanks.

Hm... Started testing this stuff and thought about Pedro's wish to use this in gdb one more time :(

The thing is, that we (in checkpoint/restore) are going to use this sysctl when creating a pid namespace from scratch, thus having full control over all the forks happening in this namespace.

But when it comes to the ability for gdb to create a task with a given pid in a _living_ namespace this solution simply won't work! It doesn't guarantee, that after setting the last_pid via sysctl this last_pid stays the same at the time we do call fork()/clone(). Because there are other tasks that can call fork themselves ignoring any lask_pid locking we can play with.

Quoted 8 lines · Pavel Emelyanov

Hm... Started testing this stuff and thought about Pedro's wish to use this in gdb one more time :(

The thing is, that we (in checkpoint/restore) are going to use this sysctl when creating a pid namespace from scratch, thus having full control over all the forks happening in this namespace.

Quoted 5 lines

But when it comes to the ability for gdb to create a task with a given pid in a _living_ namespace this solution simply won't work! It doesn't guarantee, that after setting the last_pid via sysctl this last_pid stays the same at the time we do call fork()/clone(). Because there are other tasks that can call fork themselves ignoring any lask_pid locking we can play with.

Sure. ->last_pid is a hint, nothing more.

But nothing can work reliably on the running system. Even if we can't race with another fork(), the required pid_nr can be already in use.

Oleg.

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid,

* one more field on struct pid is OK, since it size doesn't change (32 bit level is anyway not required, it's OK to reduce on down to 16 bits)

* yes, we have +1 member on task_struct :(

Oleg, Tejun, do you agree with such an approach?

Quoted 3 lines · Pavel Emelyanov

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid,

Oh. This is subjective, yes, but this doesn't clean to me.

Amd why?? On the running system PR_RESERVE_PID can obviously fail anyway. It only helps to avoid the race with another fork.

Quoted 2 lines

* one more field on struct pid is OK, since it size doesn't change (32 bit level is anyway not required, it's OK to reduce on down to 16 bits)

Even if sizeof is the same, the new member and the code which plays with ->flags doesn't make the things better ;)

Quoted 1 line

* yes, we have +1 member on task_struct :(

Yes, and this task_struct->rsv_pid acts as implicit parameter for the next clone(). Doesn't look very nice to me. Plus the code complications.

Quoted 1 line

Oleg, Tejun, do you agree with such an approach?

If set_last_pid doesn't work, I'd prefer CLONE_CHILD_USEPIDS.

Oleg.

.

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Pavel Emelyanov

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid,

Quoted 4 lines

Oh. This is subjective, yes, but this doesn't clean to me.

Amd why?? On the running system PR_RESERVE_PID can obviously fail anyway. It only helps to avoid the race with another fork.

No. It can fail if you try to allocate a pid with given number. The API allows for pid generation. AFAIU this can help with Pedro's requirements to resurrect task with the same pid value it used to have before.

Quoted 2 lines

Yes, and this task_struct->rsv_pid acts as implicit parameter for the next clone(). Doesn't look very nice to me. Plus the code complications.

Well, the last_pid is also an implicit parameter for the next clone() with sysctl approach :)

Quoted 7 lines · Pavel Emelyanov

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Pavel Emelyanov

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid,

Quoted 8 lines

Quoted 4 lines

Oh. This is subjective, yes, but this doesn't clean to me.

Amd why?? On the running system PR_RESERVE_PID can obviously fail anyway. It only helps to avoid the race with another fork.

No. It can fail if you try to allocate a pid with given number. The API allows for pid generation. AFAIU this can help with Pedro's requirements to resurrect task with the same pid value it used to have before.

Yes gdb can do fork() in a row (until it unreserves the pid) and the pid will be the same.

OK, I misunderstood. I thought you insist that PR_RESERVE_PID itself is reliable.

But this can only work in the simplest case.

How you can restore the multithread tracee?

You need to unreserve/reserve the previous pid, and we have the same problems again, no?

Quoted 5 lines

Quoted 2 lines

Yes, and this task_struct->rsv_pid acts as implicit parameter for the next clone(). Doesn't look very nice to me. Plus the code complications.

Well, the last_pid is also an implicit parameter for the next clone() with sysctl approach :)

Yes. but it is already here ;)

Oleg.

Quoted 1 line · Oleg Nesterov

How you can restore the multithread tracee?

Don't know :) But if this approach sounds promising (I see, that now it's not, but...) I can think more on it.

Quoted 1 line

You need to unreserve/reserve the previous pid, and we have the same problems again, no?

With the existing patch - yes, but as I said above - we need to decide which direction to go and then I'll think further.

By now your opinion is to better stay where we are ;) but if moving is unavoidable, then it's better to take the CLONE_CHILD_USEPIDS route. That's my position as well.

From the perspective of a client that is

From what I read from their papers, in their approach, from userspace, they

From the perspective of a client that is
going to use this on a live system, CLONE_CHILD_USEPIDS seems a little better, in that the pid race is only against another task reusing the same pid,

while with setting last_pid, you have a try/whoops-not-the-pid-I-want/kill/retry/rinse/repeat/ loop racing against all fork/clone's in the system,

OK, here's another proposal that seem to suit all of us:

1. me wants to clone tasks with pids set
2. Pedro wants to fork task with not changing pids and w/o root perms
3. Oleg and Tejun want to have little intrusion into fork() path

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid, this pid survives and keeps its bit in the pidmap after detach. The 2nd fork() after the 1st task death thus can reuse the same pid again. This basic thing doesn't require root perms at all and safe against pid reuse problems. When requesting for pid reservation task may specify a pid number it wants to have, but this requires root perms (CAP_SYS_ADMIN).

Pedro, I suppose this will work for your checkpoint feature in gdb, am I right?

Few comments about intrusion:

* the common path - if (pid !=&init_struct_pid) - on fork is just modified
* we have -1 argument to copy_process
* one more field on struct pid is OK, since it size doesn't change (32 bit level is anyway not required, it's OK to reduce on down to 16 bits)
* no clone flags extension
* no new locking - the reserved pid manipulations happen under tasklist_lock and existing common paths do not require more of it
* yes, we have +1 member on task_struct :(

Current API problems:

* Only one fork() with pid at a time. Next call to PR_RESERVE_PID will kill the previous reservation (don't know how to fix)
* No way to fork() an init of a pid sub-namespace with desired pid in current (can be fixed for a flag for PR_RESERVE_PID saying that we need a pid for a namespace of a next level)

* No way to grab existing pid for reserve (can be fixed, if someone wants this)

We can add flag to sys_wait4(), and stash pid in wait_task_zombie(), right before release_task() code will looks something like this:

(quoted diff) +8 -1

-       if (p != NULL)
+       if (p != NULL) {
+               if ((wo->wo_flags & WCATCHPID) && !current->pid_stash) {
+                       struct pid *pid = task_pid(p);
+
+                       pid->flags |= PID_STASHED;
+                       current->pid_stash = get_pid(pid);
+               }
                release_task(p);
+       }

And next fork() creates child with the same pid.
So, struct pid will work like boomerang =)

OK, here's another proposal that seem to suit all of us:

1. me wants to clone tasks with pids set
2. Pedro wants to fork task with not changing pids and w/o root perms
3. Oleg and Tejun want to have little intrusion into fork() path

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid, this pid survives and keeps its bit in the pidmap after detach. The 2nd fork() after the 1st task death thus can reuse the same pid again. This basic thing doesn't require root perms at all and safe against pid reuse problems. When requesting for pid reservation task may specify a pid number it wants to have, but this requires root perms (CAP_SYS_ADMIN).

Pedro, I suppose this will work for your checkpoint feature in gdb, am I right?

Few comments about intrusion:

* the common path - if (pid != &init_struct_pid) - on fork is just modified
* we have -1 argument to copy_process
* one more field on struct pid is OK, since it size doesn't change (32 bit level is anyway not required, it's OK to reduce on down to 16 bits)
* no clone flags extension
* no new locking - the reserved pid manipulations happen under tasklist_lock and existing common paths do not require more of it
* yes, we have +1 member on task_struct :(

Current API problems:

* Only one fork() with pid at a time. Next call to PR_RESERVE_PID will kill the previous reservation (don't know how to fix)
* No way to fork() an init of a pid sub-namespace with desired pid in current (can be fixed for a flag for PR_RESERVE_PID saying that we need a pid for a namespace of a next level)
* No way to grab existing pid for reserve (can be fixed, if someone wants this)

Oleg, Tejun, do you agree with such an approach?

Hello, Pavel.

Quoted 35 lines · Pavel Emelyanov

OK, here's another proposal that seem to suit all of us:

1. me wants to clone tasks with pids set
2. Pedro wants to fork task with not changing pids and w/o root perms
3. Oleg and Tejun want to have little intrusion into fork() path

The proposal is to implement the PR_RESERVE_PID prctl which allocates and puts a pid on the current. The subsequent fork() uses this pid, this pid survives and keeps its bit in the pidmap after detach. The 2nd fork() after the 1st task death thus can reuse the same pid again. This basic thing doesn't require root perms at all and safe against pid reuse problems. When requesting for pid reservation task may specify a pid number it wants to have, but this requires root perms (CAP_SYS_ADMIN).

Pedro, I suppose this will work for your checkpoint feature in gdb, am I right?

Few comments about intrusion:

* the common path - if (pid != &init_struct_pid) - on fork is just modified
* we have -1 argument to copy_process
* one more field on struct pid is OK, since it size doesn't change (32 bit level is anyway not required, it's OK to reduce on down to 16 bits)
* no clone flags extension
* no new locking - the reserved pid manipulations happen under tasklist_lock and existing common paths do not require more of it
* yes, we have +1 member on task_struct :(

Current API problems:

* Only one fork() with pid at a time. Next call to PR_RESERVE_PID will kill the previous reservation (don't know how to fix)
* No way to fork() an init of a pid sub-namespace with desired pid in current (can be fixed for a flag for PR_RESERVE_PID saying that we need a pid for a namespace of a next level)
* No way to grab existing pid for reserve (can be fixed, if someone wants this)

Quoted 1 line

Oleg, Tejun, do you agree with such an approach?

Hmmm... Any attempt to reserve PIDs without full control over the namespace is futile. It can never be complete / reliable.

Let's just
forget about it. If anyone, including gdb, wants to have fun with CR, let them manage namespace too; otherwise, it's never gonna be reliable.

If you take the above out, setting last_pid is as simple as it gets and good enough. It's essentially few tens of lines of code to add userland interface for setting one pid_t value. Let's restrict manipulation to root for now and see whether finer grained CAP_* makes sense as we go along.

Thanks.

Quoted 2 lines

Hmmm... Any attempt to reserve PIDs without full control over the namespace is futile. It can never be complete / reliable.

Why? What's the _real_ problem with the

pid = prctl(PR_RESERVE_PID, 0); /* let the kernel _generate_ a pid for us */

while (1) {
	real_pid = fork();
	BUG_ON(pid != real_pid);
	if (real_pid == 0)
		return do_child();

	wait();
}

model? Let's temporarily forget about the single reserved pid implementation limitation and concentrate on the approach itself.

Quoted 2 lines

Hmmm... I hope this could be prettier. I'm having trouble following where the MAY_OPEN comes from. Can you please explain?

From this calltrace:

pid_ns_ctl_permissions
sysctl_perm
proc_sys_permission
inode_permission
do_last <<<<< MAY_OPEN appears here
path_openat
do_filp_open
do_sys_open
sys_open

Quoted 3 lines

Can't we for now allow this for root and then later allow CAP_CHECKPOINT that Cyrill suggested? Or do we want to allow setting pids even w/o CR for NS creator?

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

Quoted 9 lines

kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 221

Quoted 7 lines

kernel/pid_namespace.c +7 -0

+static int pid_ns_ctl_handler(struct ctl_table *table, int write,
+		     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tmp = *table;
+	tmp.data = &current->nsproxy->pid_ns->last_pid;
+	return proc_dointvec(&tmp, write, buffer, lenp, ppos);
+}

Probably better to call set_last_pid() on write path instead?

Why? The usage of this sysctl is going to be synchronized by external locks, so why should we care?

I think the question should usually be the other way around. Why deviate when the deviation doesn't earn any tangible benefit? If you think setting it explicitly is justified, explain why in the comment of the setter and places where those explicit settings are.

Hello,

Quoted 5 lines · Pavel Emelyanov

Quoted 2 lines

Hmmm... I hope this could be prettier. I'm having trouble following where the MAY_OPEN comes from. Can you please explain?

From this calltrace:

Quoted 9 lines

pid_ns_ctl_permissions
sysctl_perm
proc_sys_permission
inode_permission
do_last <<<<< MAY_OPEN appears here
path_openat
do_filp_open
do_sys_open
sys_open

Thanks a lot. :)

Quoted 6 lines

Quoted 3 lines

Can't we for now allow this for root and then later allow CAP_CHECKPOINT that Cyrill suggested? Or do we want to allow setting pids even w/o CR for NS creator?

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

But wouldn't CAP_CHECKPOINT be enough for systemd?

Quoted 10 lines

Quoted 9 lines

kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 221

Quoted 7 lines

kernel/pid_namespace.c +7 -0

+static int pid_ns_ctl_handler(struct ctl_table *table, int write,
+		     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tmp = *table;
+	tmp.data = &current->nsproxy->pid_ns->last_pid;
+	return proc_dointvec(&tmp, write, buffer, lenp, ppos);
+}

Probably better to call set_last_pid() on write path instead?

Quoted 2 lines

Why? The usage of this sysctl is going to be synchronized by external locks, so why should we care?

I think the question should usually be the other way around. Why deviate when the deviation doesn't earn any tangible benefit? If you think setting it explicitly is justified, explain why in the comment of the setter and places where those explicit settings are.

Thanks.

Quoted 5 lines

Quoted 3 lines

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

But wouldn't CAP_CHECKPOINT be enough for systemd?

It would, but what's the point in granting to a systemd (which can be a container's init by the way) the ability to use the _whole_ checkpoint/restore engine?

Quoted 8 lines · Pavel Emelyanov

Quoted 5 lines

Quoted 3 lines

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

But wouldn't CAP_CHECKPOINT be enough for systemd?

It would, but what's the point in granting to a systemd (which can be a container's init by the way) the ability to use the _whole_ checkpoint/restore engine?

Christ, stop making it sound like we would *want* systemd to do even more odd things.

Quite frankly, any feature that is sold with ".. and systemd can use this fox Xyz", is a *misfeature* in my opinion. Core infrastructure like systemd should use a *minimal* interface, not some random extended features.

I'm starting to really dislike this whole feature discussion.

Linus
.

Quoted 8 lines · Pavel Emelyanov

Quoted 5 lines

Quoted 3 lines

I think that systemd guys can play with it. E.g. respawning daemons with predefined pids sounds like an interesting thing to play with.

But wouldn't CAP_CHECKPOINT be enough for systemd?

It would, but what's the point in granting to a systemd (which can be a container's init by the way) the ability to use the _whole_ checkpoint/restore engine?

Christ, stop making it sound like we would *want* systemd to do even more odd things.

Quite frankly, any feature that is sold with ".. and systemd can use this fox Xyz", is a *misfeature* in my opinion.  Core infrastructure like systemd should use a *minimal* interface, not some random extended features.

kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 221

Quoted 8 lines

kernel/pid_namespace.c +7 -0

+static int pid_ns_ctl_handler(struct ctl_table *table, int write,
+		     void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table tmp = *table;
+	tmp.data = &current->nsproxy->pid_ns->last_pid;
+	return proc_dointvec(&tmp, write, buffer, lenp, ppos);
+}

Probably better to call set_last_pid() on write path instead?

Quoted 4 lines

Well, after a bit more thinking I found one more pros for this sysctl - when restoring a container we'll have the possibility to set the last_pid to what we want to prevent the pids reuse after the restore.

Hmmm... I personally like this one better. Restoring multilevel pid would be more tedious but should still be possible and I really like that it's staying out of clone path and all modifications are to ns and pid code. Oleg, what do you think?