Introduce the cloning with pids functionality

kernel/pid.c struct pid *alloc_pid(struct pid_namespace *ns) · line 286

Quoted 9 lines · Pavel Emelyanov

kernel/pid.c +1 -1

@@ -281,7 +281,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
 {
 	struct pid *pid;
 	enum pid_type type;
-	int i, nr;
+	int i, nr = -ENOMEM;
 	struct pid_namespace *tmp;
 	struct upid *upid;

This doesn't look right at first glance... I mean, if the first kmem_cache_alloc(ns->pid_cachep) fails, this -ENOMEM won't be returned as ERR_PTR().

kernel/pid.c out_free: · line 325

Quoted 7 lines

kernel/pid.c +1 -1

@@ -321,7 +321,7 @@ out_free:
 		free_pidmap(pid->numbers + i);
 
 	kmem_cache_free(ns->pid_cachep, pid);
-	pid = NULL;
+	pid = ERR_PTR(nr);
 	goto out;

Off-topic, but with or withoit this patch this "goto out" looks strange imho. Why not a simple

	-	pid = NULL;
	-	goto out;
	+	return ERR_PTR(nr);

instead? But this is minor and subjective, I won't insist.

Oleg.

.

The map's page allocation code is moved to separate function to make clone-with-pids patching simpler.

kernel/pid.c +7 -0

+static int alloc_pidmap_page(struct pidmap *map)
+{
+	void *page = kzalloc(PAGE_SIZE, GFP_KERNEL);
+
+	/*
+	 * Free the page if someone raced with us
+	 * installing it:

kernel/pid.c +5 -1

 	for (i = ns->level; i >= 0; i--) {
-		nr = alloc_pidmap(tmp);
+		nr = 0;
+		if (unlikely(want_pids != NULL)) {
+			if (get_user(nr, want_pids)) {
+				nr = -EFAULT;
+				goto out_free;

Hello, Pavel.

Quoted 12 lines

kernel/pid.c +10 -1

 	for (i = ns->level; i >= 0; i--) {
-		nr = alloc_pidmap(tmp);
+		nr = 0;
+		if (unlikely(want_pids != NULL)) {
+			if (get_user(nr, want_pids)) {
+				nr = -EFAULT;
+				goto out_free;
+			}
+
+			if (nr != 0) {
+				want_pids++;
+				nr = set_pidmap(tmp, nr);

kernel/pid.c struct pid *alloc_pid(struct pid_namespace *ns) · line 336

Quoted 5 lines

kernel/pid.c +5 -0

+			} else
+				want_pids = NULL; /* optimize above */
+		}
+		if (nr == 0)
+			nr = alloc_pidmap(tmp);

Hmmm... is it necessary to be able to replicate pids on all namespaces?

Also, isn't it a bit weird to be able to request PIDs in the namespaces which is beyond the task which requested cloning?

Thanks.

Quoted 2 lines

Hmmm... is it necessary to be able to replicate pids on all namespaces?

Not for us (I mean OpenVZ). But since we should (in theory) be able to recreate the nested set of namespaces it would be good if the API allows for this from the very beginning.

Quoted 2 lines

Also, isn't it a bit weird to be able to request PIDs in the namespaces which is beyond the task which requested cloning?

It is, but the last_pid != 0 check will abort this request with EPERM :) Do you think some other behavior would be better?

Hello,

Quoted 6 lines · Pavel Emelyanov

Quoted 2 lines

Hmmm... is it necessary to be able to replicate pids on all namespaces?

Not for us (I mean OpenVZ). But since we should (in theory) be able to recreate the nested set of namespaces it would be good if the API allows for this from the very beginning.

I see.

Quoted 3 lines

Quoted 2 lines

Also, isn't it a bit weird to be able to request PIDs in the namespaces which is beyond the task which requested cloning?

Quoted 2 lines

It is, but the last_pid != 0 check will abort this request with EPERM :) Do you think some other behavior would be better?

Right, missed that part. The only suggestion then is that clearing wants_pid to NULL isn't an optimization but should be part of API - ie. want_pids is 0 terminated; otherwise, the caller must know at which ns depth it is.

Thanks.

The child_tidptr points to an array of pids for current namespace and its ancestors. When 0 is met in this array the pid number for the corresponding namespace is generated, rather than set.

For security reasons after a regular clone/fork is done in a namespace further cloning with predefined pid is not allowed.

kernel/pid.c +4 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;

Quoted 4 lines · Pavel Emelyanov

The child_tidptr points to an array of pids for current namespace and its ancestors. When 0 is met in this array the pid number for the corresponding namespace is generated, rather than set.

I must have missed something, but I can't unserstand how this works.

Quoted 2 lines

For security reasons after a regular clone/fork is done in a namespace further cloning with predefined pid is not allowed.

I guess, this is pid_ns->last_pid != 0 check in set_pidmap(), right ?

Quoted 4 lines

kernel/pid.c +4 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;

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

Quoted 10 lines

kernel/pid.c +10 -0

+
+	offset = pid & BITS_PER_PAGE_MASK;
+	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
+
+	if (unlikely(!map->page))
+		if (alloc_pidmap_page(map))
+			return -ENOMEM;
+
+	if (pid_ns->last_pid != 0)
+		return -EPERM;

OK, but it should be always true, no? IOW, set_pidmap() should always fail?

Unless: you are using CLONE_NEWPID along with CLONE_CHILD_USEPIDS and this child_tidptr array has only one pid (before zero pid).

So, could you please explain what I have missed?

Quoted 10 lines

Quoted 3 lines

The child_tidptr points to an array of pids for current namespace and its ancestors. When 0 is met in this array the pid number for the corresponding namespace is generated, rather than set.

I must have missed something, but I can't unserstand how this works.

Quoted 2 lines

For security reasons after a regular clone/fork is done in a namespace further cloning with predefined pid is not allowed.

I guess, this is pid_ns->last_pid != 0 check in set_pidmap(), right ?

Quoted 18 lines

Quoted 4 lines

kernel/pid.c +4 -0

+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;

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

Quoted 10 lines

kernel/pid.c +10 -0

+
+	offset = pid & BITS_PER_PAGE_MASK;
+	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
+
+	if (unlikely(!map->page))
+		if (alloc_pidmap_page(map))
+			return -ENOMEM;
+
+	if (pid_ns->last_pid != 0)
+		return -EPERM;

OK, but it should be always true, no? IOW, set_pidmap() should always fail?

Quoted 2 lines

Unless: you are using CLONE_NEWPID along with CLONE_CHILD_USEPIDS and this child_tidptr array has only one pid (before zero pid).

And, if you do clone(CLONE_NEWPID | CLONE_CHILD_USEPIDS), then new_ns->child_reaper == NULL (unless you pass "1" in child_tidptr[]) ?

Quoted 1 line

So, could you please explain what I have missed?

please ;) I guess I misread this patch completely. Help!

Oleg.

Quoted 10 lines

Quoted 3 lines

Unless: you are using CLONE_NEWPID along with CLONE_CHILD_USEPIDS and this child_tidptr array has only one pid (before zero pid).

And, if you do clone(CLONE_NEWPID | CLONE_CHILD_USEPIDS), then new_ns->child_reaper == NULL (unless you pass "1" in child_tidptr[]) ?

Quoted 1 line

So, could you please explain what I have missed?

please ;) I guess I misread this patch completely. Help!

This is how I plan to use this functionality.

When creating an init of a container being restored I call

   pids[0] = 1;
   pids[1] = 0;

clone(CLONE_NEWPID | CLONE_CHILD_USEPIDS, &pids)

Then this created "init" task will have to read pids from image files and call

pids[0] = <pid>
pids[1] = 0

clone(CLONE_CHILD_USEPIDS, &pids);

one by one. At this point the last_pid is still 0

Quoted 18 lines · Pavel Emelyanov

Quoted 10 lines

Quoted 3 lines

Unless: you are using CLONE_NEWPID along with CLONE_CHILD_USEPIDS and this child_tidptr array has only one pid (before zero pid).

And, if you do clone(CLONE_NEWPID | CLONE_CHILD_USEPIDS), then new_ns->child_reaper == NULL (unless you pass "1" in child_tidptr[]) ?

Quoted 1 line

So, could you please explain what I have missed?

please ;) I guess I misread this patch completely. Help!

This is how I plan to use this functionality.

When creating an init of a container being restored I call

   pids[0] = 1;
   pids[1] = 0;

Quoted 1 line

clone(CLONE_NEWPID | CLONE_CHILD_USEPIDS, &pids)

Yep, this is clear. In this case everything works because the pid_ns has no pids (and thus ->last_pid == 0).

But. Let me repeat the question, what if you do the same with pids[0] = 2 /* anything != 1 */ ? In this case we create the new pid_ns, but its ->child_reaper is NULL. Unless I missed something.

Quoted 6 lines

Then this created "init" task will have to read pids from image files and call

pids[0] = <pid>
pids[1] = 0

Quoted 3 lines

clone(CLONE_CHILD_USEPIDS, &pids);

one by one. At this point the last_pid is still 0

Yes, understood. set_pidmap() bypasses the last_pid logic.

Clever hack^Wtrick ;)

May be this deserves a comment above "if (pid_ns->last_pid != 0)", and perhaps it would be more clean to do this check before anything else.

Hmm. It seems, we can make a simpler patch to achieve the (roughly) same effect. Without touching copy_process/alloc_pid paths. What if we simply add PR_SET_LAST_PID? (or something else).

In this case the new init (created normally) read the pids from image file and does prcrl(PR_SET_LAST_PID, pid-1) before the next fork.

What do you think?

Oleg.

Quoted 8 lines

Hmm. It seems, we can make a simpler patch to achieve the (roughly) same effect. Without touching copy_process/alloc_pid paths. What if we simply add PR_SET_LAST_PID? (or something else).

In this case the new init (created normally) read the pids from image file and does prcrl(PR_SET_LAST_PID, pid-1) before the next fork.

What do you think?

This will make it impossible to fork() children on restore in parallel. And I don't want to lose this ability :(

It's highly unlikely that the ability to fork in parallel would contribute to any meaningful speedup. That is not the critical path by *far*

Quoted 3 lines

It's highly unlikely that the ability to fork in parallel would contribute to any meaningful speedup. That is not the critical path by *far*

To add a bit, it's also one of the more *serialized* path. Doing it in parallel might actually hurt,

so if losing the ability to do it makes it less intrusive, that's the better way.

Quoted 2 lines

To add a bit, it's also one of the more *serialized* path. Doing it in parallel might actually hurt,

Can you elaborate on this, please?

Quoted 4 lines · Pavel Emelyanov

Quoted 2 lines

To add a bit, it's also one of the more *serialized* path. Doing it in parallel might actually hurt,

Can you elaborate on this, please?

It goes through a lot of locks - tasklist_lock, mmlist_lock, other subsys locks depending on CLONE_* options. If you try to do that on multiple CPUs in parallel, depending on the level of concurrency, contention and extra cacheline bounces may dominate the overhead.

Thanks.

1 cpu 500k forks - 37s

2 cpus on different cores 500k forks on each in parallel - 39s 4 cpus on different cores 500k forks on each in parallel - 41s

8 cpus 500k forks on each in parallel - 1m5s

So the fork() scaling seems quite good to me.

Hello,

Quoted 1 line · Pavel Emelyanov

1 cpu 500k forks - 37s

That's ~14k forks per sec. Do you still think you need parallel forking?

Quoted 6 lines

2 cpus on different cores 500k forks on each in parallel - 39s 4 cpus on different cores 500k forks on each in parallel - 41s

8 cpus 500k forks on each in parallel - 1m5s

So the fork() scaling seems quite good to me.

Yeah, looks pretty good actually. Hmmm, this is on a single socket w/ shared cache where cacheline bouncing is quite cheap, right? Also, how are those forking processes related? On multiple sockets, it's gonna scale worse. Dunno how much tho.

At any rate, if you do the rest in paralllel, whether forking is parallel or not is immaterial. Let's just do something least intrusive.

Thanks.

Hm, so intrusiveness is your main concern here, I see.

OK, let's assume we go with sysctl setting the last_pid.

One of the major concerns with previous attempts have been - someone creates a process with a pid that was in use by some app recently and screws things up with pid reuse.

My approach solves this,

how can sysctl handle it? Allowing
the last_pid change by the CAP_SYA_ADMIN

only is not an option, since people
are looking forward to non-root restore.

Quoted 1 line

Hm, so intrusiveness is your main concern here, I see.

To me, this is important.

Let me repeat. It is not that I strongly against CLONE_CHILD_USEPIDS (although yes, I can't say personally I like it very much ;). Just I am trying to ensure we can't make something more clear/clean/simple, at least from the kernel pov. Especially because you are trying to establish the new user-visible API.

Oleg.

Hello,

Quoted 9 lines · Pavel Emelyanov

Quoted 8 lines

Hmm. It seems, we can make a simpler patch to achieve the (roughly) same effect. Without touching copy_process/alloc_pid paths. What if we simply add PR_SET_LAST_PID? (or something else).

In this case the new init (created normally) read the pids from image file and does prcrl(PR_SET_LAST_PID, pid-1) before the next fork.

What do you think?

Quoted 2 lines

This will make it impossible to fork() children on restore in parallel. And I don't want to lose this ability :(

It's highly unlikely that the ability to fork in parallel would contribute to any meaningful speedup. That is not the critical path by *far* and I don't think it's worth optimizing for. Forking in serial and restoring the rest of states in parallel should be enough.

Thanks.

First of all - let's imagine that we just want to clone a set of tasks. Then each of them will have to fork its kids, then report first of them that he's OK to fork and wait for it to report back, that forking is done. Then do the same for the rest of them. This is not impossible, but painful.

Next - let's consider we have some tasks sharing various resources, e.g. mm-s or fd-tables. This means, that these tasks should be cloned in the carefully calculated sequence with CLONE_XXX flags set. In this case the described above scheme with fork() serialization simply won't work and we'll have to invent some fancy messaging with "now X fork with Y pid" and "X done with forking, please go on" messages.

Quoted 4 lines · Oleg Nesterov

But. Let me repeat the question, what if you do the same with pids[0] = 2 /* anything != 1 */ ? In this case we create the new pid_ns, but its ->child_reaper is NULL. Unless I missed something.

Hm... You're right here. I've missed the fact, then in recent kernels child_reaper is set under pid == 1 condition (was clone_flags & CLONE_NEWPID).

How about if I fix it by disabling the simultaneous use of CLONE_NEWPID and CLONE_CHILD_USEPIDS and checking for last_pid != 1 in the set_pidmap?

Quoted 8 lines

Hmm. It seems, we can make a simpler patch to achieve the (roughly) same effect. Without touching copy_process/alloc_pid paths. What if we simply add PR_SET_LAST_PID? (or something else).

In this case the new init (created normally) read the pids from image file and does prcrl(PR_SET_LAST_PID, pid-1) before the next fork.

What do you think?

This will make it impossible to fork() children on restore in parallel. And I don't want to lose this ability :(

Quoted 2 lines

This will make it impossible to fork() children on restore in parallel. And I don't want to lose this ability :(

Yes, this is true. You need some form of synchronization in user-space. But, otoh, prctl/sysctl/whatever is much simpler. Both from implementation pov and from understanding/using. You can even do, say, pthread_create() to make a thread with the desired tid. And of course I like the fact we do not add the new hacks into copy_process's paths.

And. If you want to restore the process tree, then these new children have to cooperate anyway. Say, nobody can clone() without CLONE_CHILD_USEPIDS before we restore all pids.

Yes, sysctl+clone should be "atomic", but that is all. Does it really hurt? OK, if nothing else, can't you do somthing like

	int fork_with_pid(int pid)
	{
		int ret;
		int pipefd[2];

		pipe(pipefd);

	retry:
		prcrl(PR_SET_LAST_PID, pid-1);
		ret = fork();

		if (ret == 0) {
			/* child, wait from parent's ACK */
			read(pipefd[0], 1, NULL);
			return 0;
		}

		/* raced with another user of PR_SET_LAST_PID */
		if (unlikely(ret != pid) {
			kill(ret, SIGKILL);
			waitpid(ret);
			goto retry;
		}

		close(pipefd[1]);
		return pid;
	}

?

Oleg.

Quoted 3 lines

Yes, sysctl+clone should be "atomic", but that is all. Does it really hurt? OK, if nothing else, can't you do somthing like

...
Nope, as I said to Tejun, we will most likely not forks children in the depth-first order,

But I don't insist.

If the CLONE_CHILD_USEPIDS has absolutely no way in the kernel we'll have to go the uglier path.