lkml.org 
[lkml]   [2012]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH] namespaces: fix leak on fork() failure
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





\
 
 \ /
  Last update: 2012-04-30 05:01    [W:0.054 / U:0.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site