Re: [PATCH v7 1/2] fork: extend clone3() to support setting a PID

[Oleg Nesterov <oleg@redhat.com>]

On 11/11, Adrian Reber wrote:
>
> v7:
>  - changed set_tid to be an array to set the PID of a process
>    in multiple nested PID namespaces at the same time as discussed
>    at LPC 2019 (container MC)

cough... iirc you convinced me this is not needed when we discussed
the previous version ;) Nevermind, probably my memory fools me.

So far I only have some cosmetic nits,

> @@ -175,6 +187,18 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>
>  	for (i = ns->level; i >= 0; i--) {
>  		int pid_min = 1;
> +		int t_pos = 0;
                    ^^^^^

I won't insist, but I'd suggest to cache set_tid[t_pos] instead to make
the code a bit more simple.

> @@ -186,12 +210,24 @@ struct pid *alloc_pid(struct pid_namespace *ns)
>  		if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
>  			pid_min = RESERVED_PIDS;

You can probably move this code into the "else" branch below.

IOW, something like


	for (i = ns->level; i >= 0; i--) {
		int xxx = 0;

		if (set_tid_size) {
			int pos = ns->level - i;

			xxx = set_tid[pos];
			if (xxx < 1 || xxx >= pid_max)
				return ERR_PTR(-EINVAL);
			/* Also fail if a PID != 1 is requested and no PID 1 exists */
			if (xxx != 1 && !tmp->child_reaper)
				return ERR_PTR(-EINVAL);
			if (!ns_capable(tmp->user_ns, CAP_SYS_ADMIN))
				return ERR_PTR(-EPERM);
			set_tid_size--;
		}

		idr_preload(GFP_KERNEL);
		spin_lock_irq(&pidmap_lock);

		if (xxx) {
			nr = idr_alloc(&tmp->idr, NULL, xxx, xxx + 1,
					GFP_ATOMIC);
			/*
			 * If ENOSPC is returned it means that the PID is
			 * alreay in use. Return EEXIST in that case.
			 */
			if (nr == -ENOSPC)
				nr = -EEXIST;
		} else {
			int pid_min = 1;
			/*
			 * init really needs pid 1, but after reaching the
			 * maximum wrap back to RESERVED_PIDS
			 */
			if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
				pid_min = RESERVED_PIDS;
			/*
			 * Store a null pointer so find_pid_ns does not find
			 * a partially initialized PID (see below).
			 */
			nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min,
					      pid_max, GFP_ATOMIC);
		}

		...

This way only the "if (set_tid_size)" block has to play with set_tid_size/set_tid.

note also that this way we can easily allow set_tid[some_level] == 0, we can
simply do

	-	if (xxx < 1 || xxx >= pid_max)
	+	if (xxx < 0 || xxx >= pid_max)

although I don't think this is really useful.

Oleg.