Re: RFC [patch 13/34] PID Virtualization Define new task_pid api

[ebiederm@xmission.com (Eric W. Biederman)]

Hubertus Franke <frankeh@watson.ibm.com> writes:

> 2nd:
> ====	
> Issue: we don't need pid virtualization, instead simply use <container,pid>
> pair.
>
> This requires a bit more thought. Essentially that's what I was doing, but I
> mangled them into the same pid and using masking to add/remove the
> container for internal use.  As pointed out by Alan(?), we can
> indeed reused the same pid internally many times as long as we can
> distinguish during the pid-to-task_struct lookup. This is easily
> done because, the caller provides the context hence the container for the
> lookup.
>
> Actions: The vpid_to_pid will disappear and the check for whether we
> are in the same container needs to be pushed down into the task
> lookup. question remains to  figure out  whether the context of the
> task lookup (will always remain the caller ?). 

Any place the kernel saves a pid and then proceeds to signal it later.
At that later point in time it is possibly you will be in the wrong
context.

This probably justifies having a kpid_t that has both the process
space id and the pid in it.  For when the kernel is storing pids to
use as weak references, for signal purposes etc.

At least tty_io.c and fcntl.c have examples where you the caller
may not have the proper context.

> Doing so has an implication, namely that we are moving over to "system
> containers". The current implementation requires the vpid/pid only
> for the boundary condition at the top of the container (to rewrite
> pid=1) and its parent and the fact that we wanted a global look
> through container=0. If said boundary would be eliminated and we
> simply make a container a child of the initproc (pid=1), this would
> be unnecessary. 
>
> all together this would provide private namespaces (as just suggested by Eric).
>
> The feeling would be that large parts of patch could be reduce by
> this.

Simplified, and made easier to understand.  I don't know if the number
of lines affected can or should be reduced.  

One of my problems with your current approach is that it doesn't help
identify where you have problems.

I have found a specific example that your current patches get wrong,
because you make assumptions about which context is valid.

>From function kernel/khtread.c
> static void keventd_create_kthread(void *_create)
> {
> 	struct kthread_create_info *create = _create;
> 	int pid;
> 
> 	/* We want our own signal handler (we take no signals by default). */
> 	pid = kernel_thread(kthread, create, CLONE_FS | CLONE_FILES | SIGCHLD);
> 	if (pid < 0) {
> 		create->result = ERR_PTR(pid);
> 	} else {
> 		wait_for_completion(&create->started);
> 		create->result = find_task_by_pid(pid);
> 	}
> 	complete(&create->done);
> }

kernel_thread() is a light wrapper around do_fork().
do_fork returns a virtual pid.
find_task_by_pid takes a pid with the upper bits holding the process
   space id.

Therefore if this function or a cousin of it was ever triggered
by a userspace application in a virtual context find_task_by_pid
would fail to find the task structure.

The only way I know to make this change safely is to make compilation
of all functions that manipulate pids in possibly dangerous ways fail.
And then to manually and slowly fix them up.

That way if something is missed.  You get a compile error instead
of incorrect execution.

Eric