Re: [LTP] [PATCH V2] container: new testcase pidns32

[Yuan Sun <sunyuan3@huawei.com>]

Hi Jan,
     See in-line comments.
On 2015/8/11 16:05, Jan Stancek wrote:
>
> ----- Original Message -----
>> From: "Yuan Sun" <sunyuan3@huawei.com>
>> To: jstancek@redhat.com
>> Cc: ltp-list@lists.sourceforge.net
>> Sent: Friday, 7 August, 2015 6:55:08 AM
>> Subject: [PATCH V2] container: new testcase pidns32
>>
>> Kernel imposes a limit of 32 nested levels of pid namespaces.
>>
>> Signed-off-by: Yuan Sun <sunyuan3@huawei.com>
>> ---
>>   runtest/containers                           |  1 +
>>   testcases/kernel/containers/pidns/.gitignore |  1 +
>>   testcases/kernel/containers/pidns/pidns32.c  | 96
>>   ++++++++++++++++++++++++++++
>>   3 files changed, 98 insertions(+)
>>   create mode 100644 testcases/kernel/containers/pidns/pidns32.c
>>
>> diff --git a/runtest/containers b/runtest/containers
>> index 9dc44bc..ac37ab2 100644
>> --- a/runtest/containers
>> +++ b/runtest/containers
>> @@ -13,6 +13,7 @@ pidns17 pidns17
>>   pidns20 pidns20
>>   pidns30 pidns30
>>   pidns31 pidns31
>> +pidns32 pidns32
>>   
>>   mqns_01 mqns_01
>>   mqns_01_clone mqns_01 -clone
>> diff --git a/testcases/kernel/containers/pidns/.gitignore
>> b/testcases/kernel/containers/pidns/.gitignore
>> index e56c1f9..488b045 100644
>> --- a/testcases/kernel/containers/pidns/.gitignore
>> +++ b/testcases/kernel/containers/pidns/.gitignore
>> @@ -12,3 +12,4 @@
>>   /pidns20
>>   /pidns30
>>   /pidns31
>> +/pidns32
>> diff --git a/testcases/kernel/containers/pidns/pidns32.c
>> b/testcases/kernel/containers/pidns/pidns32.c
>> new file mode 100644
>> index 0000000..d0d3e1e
>> --- /dev/null
>> +++ b/testcases/kernel/containers/pidns/pidns32.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright (c) Huawei Technologies Co., Ltd., 2015
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
>> + * the GNU General Public License for more details.
>> + */
>> +
>> +/*
> Hi,
>
> I'm still not entirely convinced about this test.
>
>> + * Verify that:
>> + * The kernel imposes a limit of 32 nested levels of pid namespaces.
> If it doesn't, is that a bad thing? Are there some adverse effects, when
> such limitation is lacking?
It is a kernel feature. The following information is from
http://www.serverphorums.com/read.php?12,580615

'struct pid' is a "variable sized struct" - a header with an array
of upids at the end.

A size of the array depends on a level (depth) of pid namespaces. Now
a level of pidns is not limited, so 'struct pid' can be more than one
page.

Looks reasonable, that it should be less than a page. MAX_PIS_NS_LEVEL
is not calculated from PAGE_SIZE, because in this case it depends on
architectures, config options and it will be reduced, if someone adds a
new fields in struct pid or struct upid.

I suggest to set MAX_PID_NS_LEVEL = 32, because it saves ability to
expand "struct pid" and it's more than enough for all known for me
use-cases. 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.
>
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <sys/wait.h>
>> +#include <assert.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include "test.h"
>> +#include "pidns_helper.h"
>> +
>> +#define MAXNEST 32
>> +
>> +char *TCID = "pidns32";
>> +int TST_TOTAL = 1;
>> +
>> +static void setup(void)
>> +{
>> +	if (tst_kvercmp(3, 7, 0) < 0)
>> +		tst_brkm(TCONF, NULL, "nest depth limitation not supported");
>> +	tst_require_root();
>> +	check_newpid();
>> +	tst_tmpdir();
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	tst_rmdir();
>> +}
>> +
>> +static int child_fn1(void *arg)
>> +{
>> +	pid_t cpid1;
>> +	long level = (long)arg;
>> +
>> +	cpid1 = ltp_clone_quick(CLONE_NEWPID | SIGCHLD,
>> +		(void *)child_fn1, (void *)(level + 1));
> You are relying on fact that clone will fail at MAXNEST levels.
> If it doesn't then this keeps making child processes presumably
> until you hit OOM.
>
> First action of new child should be to check current level. If it's
> over, test can end there.
Yes, you are right. I will update the code.
>
>> +	if (cpid1 < 0) {
>> +		if (level == MAXNEST)
>> +			return 0;
>> +		return 1;
>> +	}
>> +	if (waitpid(cpid1, NULL, 0) == -1)
>> +		return 1;
> If you ignore status in waitpid() call, how do you know that child at
> MAXNEXT level failed test condition?
  79 /* MAX_PID_NS_LEVEL is needed for limiting size of 'struct pid' */
  80 #define MAX_PID_NS_LEVEL 32
  81
  82 static struct pid_namespace *create_pid_namespace(struct 
user_namespace *user_ns,
  83         struct pid_namespace *parent_pid_ns)
  84 {
  85         struct pid_namespace *ns;
  86         unsigned int level = parent_pid_ns->level + 1;
  87         int i;
  88         int err;
  89
  90         if (level > MAX_PID_NS_LEVEL) {
  91                 err = -EINVAL;
  92                 goto out;
  93         }
http://lxr.free-electrons.com/source/kernel/pid_namespace.c

I need to update the code to judge if the error is EINVAL.
>
> Regards,
> Jan
>
>> +	if (level > MAXNEST) {
>> +		printf("MAX_PIS_NS_LEVEL doestn't take effect\n");
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void test_max_nest(void)
>> +{
>> +	pid_t cpid1;
>> +
>> +	cpid1 = ltp_clone_quick(CLONE_NEWPID | SIGCHLD,
>> +		(void *)child_fn1, (void *)1);
>> +	if (cpid1 < 0)
>> +		tst_brkm(TBROK | TERRNO, cleanup, "clone failed");
>> +
>> +	tst_record_childstatus(cleanup, cpid1);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	int lc;
>> +
>> +	setup();
>> +	tst_parse_opts(argc, argv, NULL, NULL);
>> +
>> +	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> +		tst_count = 0;
>> +		test_max_nest();
>> +	}
>> +
>> +	cleanup();
>> +	tst_exit();
>> +}
>> --
>> 1.9.1
>>
>>
> .
>


------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list