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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20121011131024.719863b4.akpm@linux-foundation.org>
Date:	Thu, 11 Oct 2012 13:10:24 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Andrew Vagin <avagin@...nvz.org>
Cc:	linux-kernel@...r.kernel.org, Oleg Nesterov <oleg@...hat.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Pavel Emelyanov <xemul@...allels.com>
Subject: Re: [PATCH] pidns: limit a size of pid to one page

On Wed, 10 Oct 2012 14:24:14 +0400
Andrew Vagin <avagin@...nvz.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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/