lkml.org 
[lkml]   [2014]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Oleg Nesterov <oleg@redhat.com> writes:

> alloc_pid() does get_pid_ns() beforehand but forgets to put_pid_ns()
> if it fails because disable_pid_allocation() was called by the exiting
> child_reaper. We can move get_pid_ns() down to successful return.
>
> While at it, simplify/cleanup the "goto out" mess, no need to confuse
> the success/error return paths.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/pid.c | 7 +++----
> 1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 9b9a266..dfc2f3b 100644
> --- 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.

Moving get_pid_ns down does close a leak in the error handling path.

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 increment of ns->nr_hashed after the adding to the rcu list is only safe
because we always access ns->nr_hashed with the pidmap_lock held.

Eric


> return pid;
>
> out_unlock:
> @@ -346,8 +345,8 @@ out_free:
> free_pidmap(pid->numbers + i);
>
> kmem_cache_free(ns->pid_cachep, pid);
> - pid = NULL;
> - goto out;
> +out:
> + return NULL;
> }
>
> void disable_pid_allocation(struct pid_namespace *ns)


\
 
 \ /
  Last update: 2014-11-24 23:21    [W:0.198 / U:1.768 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site