Messages in this thread | | | From | (Eric W. Biederman) | Date | Sun, 29 Apr 2012 19:59:36 -0700 | Subject | Re: [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
| |