Hi Jan,
See in-line comments.
On 2015/8/11 16:05, Jan Stancek wrote:
>
> ----- Original Message -----
>> From: "Yuan Sun" <sunyu...@huawei.com>
>> To: jstan...@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 <sunyu...@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
