[Devel] Re: [RFC][PATCH 7/7][v2] Define clone_with_pids syscall

[Sukadev Bhattiprolu]

Oren Laadan [orenl at cs.columbia.edu] wrote:
| 
| 
| Sukadev Bhattiprolu wrote:
| > Oren Laadan [orenl at cs.columbia.edu] wrote:
| > 
| > | > +
| > | > +	if (num_pids < 0 || num_pids > task_pid(current)->level + 1)
| > | > +		return ERR_PTR(-EINVAL);
| > | 
| > | What happens if (num_pids < task_pid(current->level) + 1) ?
| > | Unless I missed something elsewhere, I suspect it will oops,
| > 
| > Yep. the kzalloc() below this check should use:
| > 
| > 	task_pid(current)->pid_ns->level+1 * sizeof(pid_t)
| > 
| > | because in patch #4, you had:
| > | 
| > |  	for (i = ns->level; i >= 0; i--) {
| > | -		nr = alloc_pidmap(tmp, 0);
| > | +		tpid = 0;
| > | +		if (target_pids)
| > | +			tpid = target_pids[i];
| > | +
| > | +		nr = alloc_pidmap(tmp, tpid);
| > |  		if (nr < 0)
| > |  			goto out_free;
| > | 
| > | In general, can a task figure out it's depth in the pid-ns hierarchy ?
| > | 
| > | I'm thinking of a case in which a checkpoint was taken of a (flat)
| > | container which is in depth 2 of the global hierarchy, and then it
| > | is restarted in depth 3, or in depth 1.
| > | 
| > | Perhaps the semantics of the syscall should be that target_pids will
| > | indicate the desired pids _from the bottom up_. A value of "0" means
| > 
| > Hmm, I thought bottom up would be confusing, but I guess that would
| > work better :-) Will fix and resend.
| 
| (Repeating...)
| 
| Perhaps bottom-up is a misleading: I don't care about the order...
| I meant that a N size array should affect the last N levels of
| the hierarchy (if it is deeper than N).

Agree.

| 
| You could actually keep everything else as is, and change the code
| that copies from user to always allocate a ns->level+1 size array,
| and copy the user data to the end of it, zeroing the beginning.

Yep, good idea.

Sukadev
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers