[PATCH] pidns: limit a size of pid to one page

[PATCH] pidns: limit a size of pid to one page

[Andrew Vagin]

A size of pid depends on a level of pidns and now a level of pidns
is not limited, so it can be more than one page.

Looks reasonable, that it should be limited to a page size.  On x86_64
it will allow to create 125 nested pid namespaces.  I don't know a
usecase for which, it will be not enough. When someone finds a
reasonable use case, we can add a config option or a sysctl parameter.

In addition it will reduce effect of another problem, when we have many
nested namespaces and the oldest one starts dying.  zap_pid_ns_processe
will be called for each namespace and find_vpid will be called for each
process in a namespace. find_vpid will be called minimum max_level^2 / 2
times. The reason of that is that when we found a bit in pidmap, we
can't determine this pidns is top for this process or it isn't.

vpid is a heavy operation, so a fork bomb, which create many nested
namespace, can do a system inaccessible for a long time.

For example my system becomes inaccessible for a few minutes with
4000 processes.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Andrew Vagin <avagin@openvz.org>
---
 kernel/pid_namespace.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index b051fa6..bc822e6 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -70,12 +70,18 @@ err_alloc:
 	return NULL;
 }
 
+/* Limit a size of pid to one page */
+#define MAX_PID_NS_LEVEL ((PAGE_SIZE - sizeof(struct pid)) / sizeof(struct upid))
+
 static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_pid_ns)
 {
 	struct pid_namespace *ns;
 	unsigned int level = parent_pid_ns->level + 1;
 	int i, err = -ENOMEM;
 
+	if (level > MAX_PID_NS_LEVEL)
+		goto out;
+
 	ns = kmem_cache_zalloc(pid_ns_cachep, GFP_KERNEL);
 	if (ns == NULL)
 		goto out;
-- 
1.7.1

Re: [PATCH] pidns: limit a size of pid to one page

[Oleg Nesterov]

On 10/10, Andrew Vagin wrote:
>
> A size of pid depends on a level of pidns and now a level of pidns
> is not limited, so it can be more than one page.
>
> Looks reasonable, that it should be limited to a page size.  On x86_64
> it will allow to create 125 nested pid namespaces.  I don't know a
> usecase for which, it will be not enough. When someone finds a
> reasonable use case, we can add a config option or a sysctl parameter.

I think this makes sense.

Acked-by: Oleg Nesterov <oleg@redhat.com>

> In addition it will reduce effect of another problem, when we have many
> nested namespaces and the oldest one starts dying.  zap_pid_ns_processe
> will be called for each namespace and find_vpid will be called for each
> process in a namespace. find_vpid will be called minimum max_level^2 / 2
> times. The reason of that is that when we found a bit in pidmap, we
> can't determine this pidns is top for this process or it isn't.
>
> vpid is a heavy operation, so a fork bomb, which create many nested
> namespace, can do a system inaccessible for a long time.
>
> For example my system becomes inaccessible for a few minutes with
> 4000 processes.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Signed-off-by: Andrew Vagin <avagin@openvz.org>
> ---
>  kernel/pid_namespace.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index b051fa6..bc822e6 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -70,12 +70,18 @@ err_alloc:
>  	return NULL;
>  }
>
> +/* Limit a size of pid to one page */
> +#define MAX_PID_NS_LEVEL ((PAGE_SIZE - sizeof(struct pid)) / sizeof(struct upid))
> +
>  static struct pid_namespace *create_pid_namespace(struct pid_namespace *parent_pid_ns)
>  {
>  	struct pid_namespace *ns;
>  	unsigned int level = parent_pid_ns->level + 1;
>  	int i, err = -ENOMEM;
>
> +	if (level > MAX_PID_NS_LEVEL)
> +		goto out;
> +
>  	ns = kmem_cache_zalloc(pid_ns_cachep, GFP_KERNEL);
>  	if (ns == NULL)
>  		goto out;
> --
> 1.7.1
>

Re: [PATCH] pidns: limit a size of pid to one page

[Andrew Morton]

On Wed, 10 Oct 2012 14:24:14 +0400
Andrew Vagin <avagin@openvz.org> wrote:

> A size of pid depends on a level of pidns and now a level of pidns
> is not limited, so it can be more than one page.
> 
> Looks reasonable, that it should be limited to a page size.  On x86_64
> it will allow to create 125 nested pid namespaces.  I don't know a
> usecase for which, it will be not enough. When someone finds a
> reasonable use case, we can add a config option or a sysctl parameter.
> 
> In addition it will reduce effect of another problem, when we have many
> nested namespaces and the oldest one starts dying.  zap_pid_ns_processe
> will be called for each namespace and find_vpid will be called for each
> process in a namespace. find_vpid will be called minimum max_level^2 / 2
> times. The reason of that is that when we found a bit in pidmap, we
> can't determine this pidns is top for this process or it isn't.
> 
> vpid is a heavy operation, so a fork bomb, which create many nested
> namespace, can do a system inaccessible for a long time.
> 
> For example my system becomes inaccessible for a few minutes with
> 4000 processes.

This changelog is a bit hard to follow.  After reading the code, I see
that `struct pid' is a "variable sized struct" - a header with an array
of upids at the end.  I suggest this be explained in the changelog so
others don't scratch their heads in the way I did.

Really, what the patch actually does is to limit the nesting depth of
pid namespaces, yes?  If so I think that is the way the patch should be
presented - the restriction of the storage size of a struct pid is an
implementation detail.

> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -70,12 +70,18 @@ err_alloc:
>  	return NULL;
>  }
>  
> +/* Limit a size of pid to one page */
> +#define MAX_PID_NS_LEVEL ((PAGE_SIZE - sizeof(struct pid)) / sizeof(struct upid))

I don't it was a good choice to use PAGE_SIZE here.  PAGE_SIZE can vary
from 4k to 64k, which is a large range.  This choice introduces the
risk that an application will pass testing on one architecture/config
but will later fail when deployed on other archs/configs.  And it means
that large PAGE_SIZE setups might still experience the problems which
you have identified.

And the use of sizeof(struct upid) and sizeof(struct pid) mean that if
we later change the size of those structures, the kernel's userspace
interface will be accidentally changed in subtle and surprising ways.

So I suggest you do

#define MAX_PID_NS_LEVEL 128

and leave it at that?

And the comment for MAX_PID_NS_LEVEL should tell the reader *why* we did
this, not what we did (which is pretty obvious from reading the definition).