Messages in this thread | | | Date | Tue, 25 Nov 2014 18:07:18 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting |
| |
On 11/24, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@redhat.com> writes: > > > --- a/kernel/pid.c > > +++ b/kernel/pid.c > > @@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > goto out_free; > > } > > > > - get_pid_ns(ns); > > atomic_set(&pid->count, 1); > > for (type = 0; type < PIDTYPE_MAX; ++type) > > INIT_HLIST_HEAD(&pid->tasks[type]); > > @@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > } > > spin_unlock_irq(&pidmap_lock); > > > > -out: > > + get_pid_ns(ns); > > Moving the label and changing the goto out logic is gratuitous confusing > and I think it probably even generates worse code. > > Furthermore multiple exits make adding debugging code more difficult.
Oh, I strongly disagree but I am not going to argue ;) cleanups are always subjective, and I do believe in "maintainer is always right" mantra. I can make v2 without this change.
> Moving get_pid_ns down does close a leak in the error handling path.
OK, good.
> However at the moment my I can't figure out if it is safe to move > get_pid_ns elow hlist_add_head_rcu. Because once we are on the rcu list > the pid is findable, and being publicly visible with a bad refcount could cause > problems.
The caller has a reference, this ns can't go away. Obviously, otherwise get_pid_ns(ns) is not safe.
We need this get_pid_ns() to balance put_pid()->put_pid_ns() which obviously won't be called until we return this pid, otherwise everything is wrong.
So I think this should be safe?
Oleg.
| |