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

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

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/29, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>
>> > 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
>
> Yes, but please note that this change also checks "new == NULL", so I
> still think the patch is correct.

Sort of.  It is correct in the sense that it performs magic checks on
it's arguments to see that it's caller is exit_task_namespaces called
from the fork failure path.

It is incorrect in the case that it doesn't handle weird cases like
daemonize() which also call switch_namespaces.  So it is no better and
much more confusing and much less maintainable than your two line patch
below.

> But,
>
>> > 	 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.
>
> Good.
>
> Mike, could you please re-send the patch to akpm? Feel free to add my ack.
> I guess Eric will ack this fix too.

I will.

>> There are crazy code paths like daemonize()
>
> Forget. It has no callers anymore, should be killed. A user-space process
> should never use kernel_thread() and thus daemonize() is not needed.

Good point.  Oleg do you think you can send in the patches to kill
daemonize.  I make it a lot easier to sleep at night and review patches
if I did not have to think about that scary code path.

Eric