Re: [RFC PATCH] namespaces: fix leak on fork() failure

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> On 04/28, Mike Galbraith wrote:
>>
>> Greetings,
>
> Hi,
>
> Add CC's. I never understood the proc/namespace interaction in details,
> and it seems to me I forgot everything.
>
>> SIGCHLD delivery during fork() may cause failure,
>
> Or any other reason to fail after copy_namespaces()
>
>> resulting in the aborted
>> child being cloned with CLONE_NEWPID leaking namespaces due to proc being
>> mounted during pid namespace creation, but not unmounted on fork() failure.
>
> Heh. Please look at http://marc.info/?l=linux-kernel&m=127687751003902
> and the whole thread, there are a lot more problems here.

I don't remember seeing a leak in that conversation.

> But this particular one looks simple iirc.
>
>> @@ -216,6 +216,14 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
>>  	rcu_assign_pointer(p->nsproxy, new);
>>
>>  	if (ns && atomic_dec_and_test(&ns->count)) {
>> +		/* Handle fork() failure, unmount proc before proceeding */
>> +		if (unlikely(!new && !((p->flags & PF_EXITING)))) {
>> +			struct pid_namespace *pid_ns = ns->pid_ns;
>> +
>> +			if (pid_ns && pid_ns != &init_pid_ns)
>> +				pid_ns_release_proc(pid_ns);
>> +		}
>> +
>>  		/*
>>  		 * wait for others to get what they want from this nsproxy.
>>  		 *
>
> At first glance this looks correct. But the PF_EXITING check doesn't
> look very nice imho. It is needed to detect the case when the caller
> is copy_process()->bad_fork_cleanup_namespaces and p is not current.

Mike's proposed change to switch_task_namespace is most definitely not
correct.  This will potentially get called on unshare and so we don't
limit ourselves to just an exit pid_namespace.  The result is that we
could free the proc mount long before it is safe.

At the same time the leak that Mike detected is most definitely real.

> Perhaps it would be more clean to add the explicit
>
> 	 bad_fork_cleanup_namespaces:
> 	+	if (unlikely(clone_flags & CLONE_NEWPID))
> 	+		pid_ns_release_proc(...);
> 		exit_task_namespaces(p);
> 		
> 		
> code into this error path in copy_process?

For now Oleg your minimal patch looks good. 

Part of me would like to call proc_flush_task instead of
pid_ns_release_proc but we have no assurance task_pid and task_tgid are
valid when we get here so proc_flush_task is out.

There are crazy code paths like daemonize() that also call
swith_task_namespaces and change the pid namespace that are still
potentially broken.

Breaking the loop between the pid namespace and the proc mount would
be good, and I will see about making the time to push those patches.
So we can have something much less magical going on.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/