| Message ID | 20220315122351.8556-9-andrea.cervesato@suse.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Rewrite userns testing suite using new LTP API | expand |
Hi Andrea, ./userns08 -i50 tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) userns08.c:36: TBROK: clone3 failed: ENOSPC (28) Something needs to be closed after each run. Kind regards, Petr
Hi Petr, this is really strange. Thanks for the review, I'm going to check where's the bug. Andrea On 3/23/22 10:36, Petr Vorel wrote: > Hi Andrea, > > ./userns08 -i50 > tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' > tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:36: TBROK: clone3 failed: ENOSPC (28) > > Something needs to be closed after each run. > > Kind regards, > Petr >
Hi! > ./userns08 -i50 > tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' > tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > userns08.c:36: TBROK: clone3 failed: ENOSPC (28) > > Something needs to be closed after each run. ENOSPC means that we created too many user namespaces. The problem is likely that we are creating the namespaces faster than they are being asynchronously cleaned up in the kernel. Adding sleep(1) to the clone_newuser() function gives kernel enough time to clean the namespaces and the test works with any -i. Also note that we get the exact same failure if we execute the test a few times in a row, i.e. for i in `seq 10`; do ./userns08 done The original test fails in the same way, so while it should be fixed, it's not really reason to block this patchset. And the only correct fix would be retrying the clone() on ENOSPC in the SAFE_CLONE().
> Hi! > > ./userns08 -i50 > > tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' > > tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > userns08.c:36: TBROK: clone3 failed: ENOSPC (28) > > Something needs to be closed after each run. > ENOSPC means that we created too many user namespaces. The problem is > likely that we are creating the namespaces faster than they are being > asynchronously cleaned up in the kernel. Adding sleep(1) to the > clone_newuser() function gives kernel enough time to clean the > namespaces and the test works with any -i. Also note that we get the > exact same failure if we execute the test a few times in a row, i.e. > for i in `seq 10`; do > ./userns08 > done +1 > The original test fails in the same way, so while it should be fixed, > it's not really reason to block this patchset. Agree (I forget to write I suspected the problem wasn't new in this patchset). > And the only correct fix would be retrying the clone() on ENOSPC in the > SAFE_CLONE(). +1. I suppose Andrea will have look into it (otherwise I'll do it). Kind regards, Petr
Hi Petr, I can check it, but it will probably take some time because I'm not familiar with that particular scenario. Can we go ahead with the patch-set anyway? Andrea On 3/23/22 13:54, Petr Vorel wrote: >> Hi! >>> ./userns08 -i50 >>> tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' >>> tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s >>> userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) >>> userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) >>> userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) >>> userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) >>> userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) >>> userns08.c:36: TBROK: clone3 failed: ENOSPC (28) >>> Something needs to be closed after each run. >> ENOSPC means that we created too many user namespaces. The problem is >> likely that we are creating the namespaces faster than they are being >> asynchronously cleaned up in the kernel. Adding sleep(1) to the >> clone_newuser() function gives kernel enough time to clean the >> namespaces and the test works with any -i. Also note that we get the >> exact same failure if we execute the test a few times in a row, i.e. >> for i in `seq 10`; do >> ./userns08 >> done > +1 > >> The original test fails in the same way, so while it should be fixed, >> it's not really reason to block this patchset. > Agree (I forget to write I suspected the problem wasn't new in this patchset). > >> And the only correct fix would be retrying the clone() on ENOSPC in the >> SAFE_CLONE(). > +1. I suppose Andrea will have look into it (otherwise I'll do it). > > Kind regards, > Petr >
Hi Andrea, > Hi Petr, > I can check it, but it will probably take some time because I'm not familiar > with that particular scenario. Can we go ahead with the patch-set anyway? OK, I'll implement the fix in the library. And for sure this is not a blocker for your patchset. I've just haven't finishing reviewing it. Kind regards, Petr > Andrea > On 3/23/22 13:54, Petr Vorel wrote: > > > Hi! > > > > ./userns08 -i50 > > > > tst_kconfig.c:82: TINFO: Parsing kernel config '/proc/config.gz' > > > > tst_test.c:1456: TINFO: Timeout per run is 0h 05m 00s > > > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > > > userns08.c:65: TPASS: Denied write access to ./restricted : EACCES (13) > > > > userns08.c:36: TBROK: clone3 failed: ENOSPC (28) > > > > Something needs to be closed after each run. > > > ENOSPC means that we created too many user namespaces. The problem is > > > likely that we are creating the namespaces faster than they are being > > > asynchronously cleaned up in the kernel. Adding sleep(1) to the > > > clone_newuser() function gives kernel enough time to clean the > > > namespaces and the test works with any -i. Also note that we get the > > > exact same failure if we execute the test a few times in a row, i.e. > > > for i in `seq 10`; do > > > ./userns08 > > > done > > +1 > > > The original test fails in the same way, so while it should be fixed, > > > it's not really reason to block this patchset. > > Agree (I forget to write I suspected the problem wasn't new in this patchset). > > > And the only correct fix would be retrying the clone() on ENOSPC in the > > > SAFE_CLONE(). > > +1. I suppose Andrea will have look into it (otherwise I'll do it). > > Kind regards, > > Petr
Hi Andrea, [Cc Richie] This subject: "Rewrite userns08.c using new LTP API" is misleading, userns08.c was already using new API. You're: * s/tst_res/tst_brk/ (that would deserve explanation why) * removing tst_reap_children() * changing formatting (some of them create too long lines) ... > static pid_t clone_newuser(void) > { > - const struct tst_clone_args cargs = { > - CLONE_NEWUSER, > - SIGCHLD > - }; > + const struct tst_clone_args cargs = { CLONE_NEWUSER, SIGCHLD }; > return SAFE_CLONE(&cargs); > } > -static void write_mapping(const pid_t proc_in_ns, > - const char *const id_mapping) > +static void write_mapping(const pid_t proc_in_ns, const char *const id_mapping) > { > char proc_path[PATH_MAX]; > int proc_dir; > @@ -61,11 +58,11 @@ static void write_mapping(const pid_t proc_in_ns, > static void ns_level2(void) > { > if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0)) > - tst_res(TINFO | TERRNO, "Failed to set dumpable flag"); > + tst_brk(TBROK | TTERRNO, "Failed to set dumpable flag"); Not sure which one is correct (whether tst_res or tst_brk). But TTERRNO is obviously wrong, that's for using TEST(). Here should remain TERRNO. > + > TST_CHECKPOINT_WAKE_AND_WAIT(1); > - TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, > - "Denied write access to ./restricted"); > + TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, "Denied write access to ./restricted"); I'd keep this one. > exit(0); > } > @@ -89,7 +86,6 @@ static void ns_level1(void) > write_mapping(level2_proc, map_over_5); > TST_CHECKPOINT_WAKE(1); > - tst_reap_children(); Well, test works without it, but not really sure if it can be removed. Kind regards, Petr > exit(0); > } > @@ -111,7 +107,6 @@ static void run(void) > write_mapping(level1_proc, "0 100000 1000"); > TST_CHECKPOINT_WAKE(0); > - tst_reap_children();
Hi Petr, I think this patch can be avoided at this point Andrea On 3/25/22 08:18, Petr Vorel wrote: > Hi Andrea, > > [Cc Richie] > > This subject: "Rewrite userns08.c using new LTP API" is misleading, > userns08.c was already using new API. > > You're: > * s/tst_res/tst_brk/ (that would deserve explanation why) > * removing tst_reap_children() > * changing formatting (some of them create too long lines) > > ... >> static pid_t clone_newuser(void) >> { >> - const struct tst_clone_args cargs = { >> - CLONE_NEWUSER, >> - SIGCHLD >> - }; >> + const struct tst_clone_args cargs = { CLONE_NEWUSER, SIGCHLD }; >> return SAFE_CLONE(&cargs); >> } >> -static void write_mapping(const pid_t proc_in_ns, >> - const char *const id_mapping) >> +static void write_mapping(const pid_t proc_in_ns, const char *const id_mapping) >> { >> char proc_path[PATH_MAX]; >> int proc_dir; >> @@ -61,11 +58,11 @@ static void write_mapping(const pid_t proc_in_ns, >> static void ns_level2(void) >> { >> if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0)) >> - tst_res(TINFO | TERRNO, "Failed to set dumpable flag"); >> + tst_brk(TBROK | TTERRNO, "Failed to set dumpable flag"); > Not sure which one is correct (whether tst_res or tst_brk). > But TTERRNO is obviously wrong, that's for using TEST(). Here should remain > TERRNO. > >> + >> TST_CHECKPOINT_WAKE_AND_WAIT(1); >> - TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, >> - "Denied write access to ./restricted"); >> + TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, "Denied write access to ./restricted"); > I'd keep this one. > >> exit(0); >> } >> @@ -89,7 +86,6 @@ static void ns_level1(void) >> write_mapping(level2_proc, map_over_5); >> TST_CHECKPOINT_WAKE(1); >> - tst_reap_children(); > Well, test works without it, but not really sure if it can be removed. > > Kind regards, > Petr > >> exit(0); >> } >> @@ -111,7 +107,6 @@ static void run(void) >> write_mapping(level1_proc, "0 100000 1000"); >> TST_CHECKPOINT_WAKE(0); >> - tst_reap_children();
diff --git a/testcases/kernel/containers/userns/userns08.c b/testcases/kernel/containers/userns/userns08.c index bde944f22..1180be494 100644 --- a/testcases/kernel/containers/userns/userns08.c +++ b/testcases/kernel/containers/userns/userns08.c @@ -17,6 +17,7 @@ * by (the real) root. So on the second level we reset dumpable to 1. * */ + #define _GNU_SOURCE #include <stdio.h> #include <stdlib.h> @@ -30,16 +31,12 @@ static pid_t clone_newuser(void) { - const struct tst_clone_args cargs = { - CLONE_NEWUSER, - SIGCHLD - }; + const struct tst_clone_args cargs = { CLONE_NEWUSER, SIGCHLD }; return SAFE_CLONE(&cargs); } -static void write_mapping(const pid_t proc_in_ns, - const char *const id_mapping) +static void write_mapping(const pid_t proc_in_ns, const char *const id_mapping) { char proc_path[PATH_MAX]; int proc_dir; @@ -61,11 +58,11 @@ static void write_mapping(const pid_t proc_in_ns, static void ns_level2(void) { if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0)) - tst_res(TINFO | TERRNO, "Failed to set dumpable flag"); + tst_brk(TBROK | TTERRNO, "Failed to set dumpable flag"); + TST_CHECKPOINT_WAKE_AND_WAIT(1); - TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, - "Denied write access to ./restricted"); + TST_EXP_FAIL(open("restricted", O_WRONLY), EACCES, "Denied write access to ./restricted"); exit(0); } @@ -89,7 +86,6 @@ static void ns_level1(void) write_mapping(level2_proc, map_over_5); TST_CHECKPOINT_WAKE(1); - tst_reap_children(); exit(0); } @@ -111,7 +107,6 @@ static void run(void) write_mapping(level1_proc, "0 100000 1000"); TST_CHECKPOINT_WAKE(0); - tst_reap_children(); } static void setup(void)
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de> --- testcases/kernel/containers/userns/userns08.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)