[LTP] [PATCH v1] Refactor fork12 using new LTP API

[LTP] [PATCH v1] Refactor fork12 using new LTP API

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/syscalls/fork/fork12.c | 177 +++++++++---------------
 1 file changed, 62 insertions(+), 115 deletions(-)

diff --git a/testcases/kernel/syscalls/fork/fork12.c b/testcases/kernel/syscalls/fork/fork12.c
index 1c55c0c30..d8767859f 100644
--- a/testcases/kernel/syscalls/fork/fork12.c
+++ b/testcases/kernel/syscalls/fork/fork12.c
@@ -1,137 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   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.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- *
- *
- * NAME
- *	fork12.c
- *
- * DESCRIPTION
- *	Check that all children inherit parent's file descriptor
- *
- * ALGORITHM
- *	Parent forks processes until -1 is returned.$
- *
- * USAGE
- *	fork12
- *	** CAUTION ** Can hang your machine, esp prior to 2.4.19
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *	07/2002 Split from fork07 as a test case to exhaust available pids.
+ * Copyright (c) International Business Machines  Corp., 2001
+ *     07/2001 Ported by Wayne Boyer
+ * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+/*\
+ *[Description]
  *
- * RESTRICTIONS
- *	Should be run as root to avoid resource limits.$
- *	Should not be run with other test programs because it tries to
- *	  use all available pids.
+ * This test spawns multiple processes inside a child, until fork() returns -1.
+ * Then we check if EAGAIN is raised.
  */
 
-#include <stdio.h>
+#include <stdlib.h>
 #include <sys/wait.h>
-#include <errno.h>
-#include <string.h>
-#include "test.h"
-#include "safe_macros.h"
-
-char *TCID = "fork12";
-int TST_TOTAL = 1;
+#include "tst_test.h"
 
-static void setup(void);
-static void cleanup(void);
-static void fork12_sigs(int signum);
+static pid_t child_pid;
 
-int main(int ac, char **av)
+static void start_forking(void)
 {
-	int forks, pid1, fork_errno, waitstatus;
-	int ret, status;
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		tst_resm(TINFO, "Forking as many kids as possible");
-		forks = 0;
-		while ((pid1 = fork()) != -1) {
-			if (pid1 == 0) {	/* child */
-				/*
-				 * Taunt the OOM killer so that it doesn't
-				 * kill system processes
-				 */
-				SAFE_FILE_PRINTF(NULL,
-					"/proc/self/oom_score_adj", "500");
-				pause();
-				exit(0);
-			}
-			forks++;
-			ret = SAFE_WAITPID(cleanup, -1, &status, WNOHANG);
-			if (ret > 0) {
-				/* a child may be killed by OOM killer */
-				if (WTERMSIG(status) == SIGKILL)
-					break;
-				tst_brkm(TBROK, cleanup,
-					 "child exit with error code %d or signal %d",
-					 WEXITSTATUS(status), WTERMSIG(status));
-			}
+	pid_t pid;
+	int status;
+	int forks = 1;
+
+	tst_res(TINFO, "Forking as many children as possible");
+
+	while (1) {
+		TEST(fork());
+		pid = TST_RET;
+
+		if (pid == -1)
+			break;
+
+		if (!pid) {
+			/*
+			 * Taunt the OOM killer so that it doesn't
+			 * kill system processes
+			 */
+			SAFE_FILE_PRINTF("/proc/self/oom_score_adj", "500");
+			pause();
+			exit(0);
 		}
-		fork_errno = errno;
-
-		/* parent */
-		tst_resm(TINFO, "Number of processes forked is %d", forks);
-		tst_resm(TPASS, "fork() eventually failed with %d: %s",
-			 fork_errno, strerror(fork_errno));
-		/* collect our kids */
-		/*
-		 * Introducing a sleep(3) to make sure all children are
-		 * at pause() when SIGQUIT is sent to them
-		 */
-		sleep(3);
-		kill(0, SIGQUIT);
-		while (wait(&waitstatus) > 0) ;
 
+		forks++;
+
+		if (SAFE_WAITPID(-1, &status, WNOHANG) > 0) {
+			tst_brk(TBROK, "child exit with error code %d or signal %d",
+					WEXITSTATUS(status),
+					WTERMSIG(status));
+		}
 	}
 
-	cleanup();
-	tst_exit();
+	tst_res(TINFO, "Number of processes forked is %d", forks);
+	TST_EXP_EXPR(TST_ERR == EAGAIN, "last fork() failed with EAGAIN");
 }
 
-static void setup(void)
+static void run(void)
 {
-	tst_sig(FORK, fork12_sigs, cleanup);
-	TEST_PAUSE;
+	int status;
+
+	child_pid = SAFE_FORK();
+	if (!child_pid) {
+		start_forking();
+		return;
+	}
+
+	SAFE_WAIT(&status);
 }
 
 static void cleanup(void)
 {
-	int waitstatus;
+	kill(child_pid, 0);
 
-	/* collect our kids */
-	kill(0, SIGQUIT);
-	while (wait(&waitstatus) > 0) ;
+	if (errno != ESRCH)
+		SAFE_KILL(child_pid, SIGKILL);
 }
 
-static void fork12_sigs(int signum)
-{
-	if (signum == SIGQUIT) {
-		/* Children will continue, parent will ignore */
-	} else {
-		tst_brkm(TBROK, cleanup,
-			 "Unexpected signal %d received.", signum);
-	}
-}
+static struct tst_test test = {
+	.test_all = run,
+	.cleanup = cleanup,
+	.forks_child = 1,
+	.max_runtime = 600,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor fork12 using new LTP API

[Richard Palethorpe]

Hello,

Andrea Cervesato <andrea.cervesato@suse.de> writes:

> From: Andrea Cervesato <andrea.cervesato@suse.com>
>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  testcases/kernel/syscalls/fork/fork12.c | 177 +++++++++---------------
>  1 file changed, 62 insertions(+), 115 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/fork/fork12.c b/testcases/kernel/syscalls/fork/fork12.c
> index 1c55c0c30..d8767859f 100644
> --- a/testcases/kernel/syscalls/fork/fork12.c
> +++ b/testcases/kernel/syscalls/fork/fork12.c
> @@ -1,137 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - *   Copyright (c) International Business Machines  Corp., 2001
> - *
> - *   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.
> - *
> - *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - *
> - *
> - * NAME
> - *	fork12.c
> - *
> - * DESCRIPTION
> - *	Check that all children inherit parent's file descriptor
> - *
> - * ALGORITHM
> - *	Parent forks processes until -1 is returned.$
> - *
> - * USAGE
> - *	fork12
> - *	** CAUTION ** Can hang your machine, esp prior to 2.4.19
> - *
> - * HISTORY
> - *	07/2001 Ported by Wayne Boyer
> - *	07/2002 Split from fork07 as a test case to exhaust available pids.
> + * Copyright (c) International Business Machines  Corp., 2001
> + *     07/2001 Ported by Wayne Boyer
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +/*\
> + *[Description]
>   *
> - * RESTRICTIONS
> - *	Should be run as root to avoid resource limits.$
> - *	Should not be run with other test programs because it tries to
> - *	  use all available pids.
> + * This test spawns multiple processes inside a child, until fork() returns -1.
> + * Then we check if EAGAIN is raised.
>   */
>  
> -#include <stdio.h>
> +#include <stdlib.h>
>  #include <sys/wait.h>
> -#include <errno.h>
> -#include <string.h>
> -#include "test.h"
> -#include "safe_macros.h"
> -
> -char *TCID = "fork12";
> -int TST_TOTAL = 1;
> +#include "tst_test.h"
>  
> -static void setup(void);
> -static void cleanup(void);
> -static void fork12_sigs(int signum);
> +static pid_t child_pid;
>  
> -int main(int ac, char **av)
> +static void start_forking(void)
>  {
> -	int forks, pid1, fork_errno, waitstatus;
> -	int ret, status;
> -	int lc;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		tst_resm(TINFO, "Forking as many kids as possible");
> -		forks = 0;
> -		while ((pid1 = fork()) != -1) {
> -			if (pid1 == 0) {	/* child */
> -				/*
> -				 * Taunt the OOM killer so that it doesn't
> -				 * kill system processes
> -				 */
> -				SAFE_FILE_PRINTF(NULL,
> -					"/proc/self/oom_score_adj", "500");
> -				pause();
> -				exit(0);
> -			}
> -			forks++;
> -			ret = SAFE_WAITPID(cleanup, -1, &status, WNOHANG);
> -			if (ret > 0) {
> -				/* a child may be killed by OOM killer */
> -				if (WTERMSIG(status) == SIGKILL)
> -					break;
> -				tst_brkm(TBROK, cleanup,
> -					 "child exit with error code %d or signal %d",
> -					 WEXITSTATUS(status), WTERMSIG(status));
> -			}
> +	pid_t pid;
> +	int status;
> +	int forks = 1;
> +
> +	tst_res(TINFO, "Forking as many children as possible");
> +
> +	while (1) {
> +		TEST(fork());
> +		pid = TST_RET;
> +
> +		if (pid == -1)
> +			break;
> +
> +		if (!pid) {
> +			/*
> +			 * Taunt the OOM killer so that it doesn't
> +			 * kill system processes
> +			 */
> +			SAFE_FILE_PRINTF("/proc/self/oom_score_adj", "500");
> +			pause();
> +			exit(0);
>  		}
> -		fork_errno = errno;
> -
> -		/* parent */
> -		tst_resm(TINFO, "Number of processes forked is %d", forks);
> -		tst_resm(TPASS, "fork() eventually failed with %d: %s",
> -			 fork_errno, strerror(fork_errno));
> -		/* collect our kids */
> -		/*
> -		 * Introducing a sleep(3) to make sure all children are
> -		 * at pause() when SIGQUIT is sent to them
> -		 */
> -		sleep(3);
> -		kill(0, SIGQUIT);
> -		while (wait(&waitstatus) > 0) ;
>  
> +		forks++;
> +
> +		if (SAFE_WAITPID(-1, &status, WNOHANG) > 0) {
> +			tst_brk(TBROK, "child exit with error code %d or signal %d",
> +					WEXITSTATUS(status),
> +					WTERMSIG(status));
> +		}
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	tst_res(TINFO, "Number of processes forked is %d", forks);
> +	TST_EXP_EXPR(TST_ERR == EAGAIN, "last fork() failed with
> EAGAIN");

I think it should at least accept ENOMEM as well.

>  }
>  
> -static void setup(void)
> +static void run(void)
>  {
> -	tst_sig(FORK, fork12_sigs, cleanup);
> -	TEST_PAUSE;
> +	int status;
> +
> +	child_pid = SAFE_FORK();
> +	if (!child_pid) {

Maybe here we should start a new process group which we can kill later
in cleanup.

I guess you would call SAFE_SETPGID(0, 0) here.

> +		start_forking();
> +		return;
> +	}
> +
> +	SAFE_WAIT(&status);
>  }
>  
>  static void cleanup(void)
>  {
> -	int waitstatus;
> +	kill(child_pid, 0);
>  
> -	/* collect our kids */
> -	kill(0, SIGQUIT);
> -	while (wait(&waitstatus) > 0) ;
> +	if (errno != ESRCH)

Possibly errno is undefined if the call to kill was successful. Also
there is a race here. You probably should just do an unconditional
SIGKILL to all the children.

If you have a process group as mentioned above, then you should be able
to call kill(-child_pid, SIGKILL).

> +		SAFE_KILL(child_pid, SIGKILL);
>  }
>  
> -static void fork12_sigs(int signum)
> -{
> -	if (signum == SIGQUIT) {
> -		/* Children will continue, parent will ignore */
> -	} else {
> -		tst_brkm(TBROK, cleanup,
> -			 "Unexpected signal %d received.", signum);
> -	}
> -}
> +static struct tst_test test = {
> +	.test_all = run,
> +	.cleanup = cleanup,
> +	.forks_child = 1,
> +	.max_runtime = 600,
> +};
> -- 
> 2.35.3

BTW, tests like this seem to cause containers to become unresponsive. If
Kirk or LTX are running in the same container without any further
isolation then we could use all the resources assigned to a container on
forking. We can lock LTX in memory, but that still leaves CPU.

Perhaps the test could reduce its own process and memory limit? e.g. with prlimit


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor fork12 using new LTP API

[Richard Palethorpe]

Hello,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Andrea Cervesato <andrea.cervesato@suse.de> writes:
>
>> -	cleanup();
>> -	tst_exit();
>> +	tst_res(TINFO, "Number of processes forked is %d", forks);
>> +	TST_EXP_EXPR(TST_ERR == EAGAIN, "last fork() failed with
>> EAGAIN");
>
> I think it should at least accept ENOMEM as well.
>
>>  }
>>  
>> -static void setup(void)
>> +static void run(void)
>>  {
>> -	tst_sig(FORK, fork12_sigs, cleanup);
>> -	TEST_PAUSE;
>> +	int status;
>> +
>> +	child_pid = SAFE_FORK();
>> +	if (!child_pid) {
>
> Maybe here we should start a new process group which we can kill later
> in cleanup.
>
> I guess you would call SAFE_SETPGID(0, 0) here.
>
>> +		start_forking();
>> +		return;
>> +	}
>> +
>> +	SAFE_WAIT(&status);
>>  }
>>  
>>  static void cleanup(void)
>>  {
>> -	int waitstatus;
>> +	kill(child_pid, 0);
>>  
>> -	/* collect our kids */
>> -	kill(0, SIGQUIT);
>> -	while (wait(&waitstatus) > 0) ;
>> +	if (errno != ESRCH)
>
> Possibly errno is undefined if the call to kill was successful. Also
> there is a race here. You probably should just do an unconditional
> SIGKILL to all the children.
>
> If you have a process group as mentioned above, then you should be able
> to call kill(-child_pid, SIGKILL).
>
>> +		SAFE_KILL(child_pid, SIGKILL);
>>  }
>>  
>> -static void fork12_sigs(int signum)
>> -{
>> -	if (signum == SIGQUIT) {
>> -		/* Children will continue, parent will ignore */
>> -	} else {
>> -		tst_brkm(TBROK, cleanup,
>> -			 "Unexpected signal %d received.", signum);
>> -	}
>> -}
>> +static struct tst_test test = {
>> +	.test_all = run,
>> +	.cleanup = cleanup,
>> +	.forks_child = 1,
>> +	.max_runtime = 600,
>> +};
>> -- 
>> 2.35.3
>
> BTW, tests like this seem to cause containers to become unresponsive. If
> Kirk or LTX are running in the same container without any further
> isolation then we could use all the resources assigned to a container on
> forking. We can lock LTX in memory, but that still leaves CPU.
>
> Perhaps the test could reduce its own process and memory limit? e.g. with prlimit

This test also randomly fails outside of a container. Also other tests
that are testing the limits. This makes me think more that setting lower
prlimits is needed. Also this rewrite gets higher priority.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor fork12 using new LTP API

[Cyril Hrubis]

Hi!
> This test also randomly fails outside of a container. Also other tests
> that are testing the limits. This makes me think more that setting lower
> prlimits is needed. Also this rewrite gets higher priority.

Just note that this test is not in syscalls runtest file but in the
crashme runtest file, which contains highly questionable stuff.

I guess that the original test does not really take things like
overcommit and OMM into an account, so shifting the test goals by
setting the RLIMIT_NPROC so that we effectively check that the
limits are enforced is probably reasonable way how to fix the test.
Either we do that or we remove fork12.c.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor fork12 using new LTP API

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> This test also randomly fails outside of a container. Also other tests
>> that are testing the limits. This makes me think more that setting lower
>> prlimits is needed. Also this rewrite gets higher priority.
>
> Just note that this test is not in syscalls runtest file but in the
> crashme runtest file, which contains highly questionable stuff.
>
> I guess that the original test does not really take things like
> overcommit and OMM into an account, so shifting the test goals by
> setting the RLIMIT_NPROC so that we effectively check that the
> limits are enforced is probably reasonable way how to fix the test.
> Either we do that or we remove fork12.c.

Looking at the setrlimit tests we already do this in setrlimit01 as
well.

I guess someone might want to test a fork bomb. However I don't see how
it could be a reliable or meaningful test unless you set reasonable
limits for the particular system that the test is running on.

Just a thought; IMO stress tests are better handled by a tool like
stress-ng and some bespoke scripts for a particular system. Or else we
have to create a framework inside LTP for deciding on and implementing
reasonable limits.


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] Refactor fork12 using new LTP API

[Andrea Cervesato via ltp]

Hi!

On 11/1/23 09:11, Richard Palethorpe wrote:
> Hello,
>
> Cyril Hrubis <chrubis@suse.cz> writes:
>
>> Hi!
>>> This test also randomly fails outside of a container. Also other tests
>>> that are testing the limits. This makes me think more that setting lower
>>> prlimits is needed. Also this rewrite gets higher priority.
>> Just note that this test is not in syscalls runtest file but in the
>> crashme runtest file, which contains highly questionable stuff.
>>
>> I guess that the original test does not really take things like
>> overcommit and OMM into an account, so shifting the test goals by
>> setting the RLIMIT_NPROC so that we effectively check that the
>> limits are enforced is probably reasonable way how to fix the test.
>> Either we do that or we remove fork12.c.
> Looking at the setrlimit tests we already do this in setrlimit01 as
> well.
>
> I guess someone might want to test a fork bomb. However I don't see how
> it could be a reliable or meaningful test unless you set reasonable
> limits for the particular system that the test is running on.
>
> Just a thought; IMO stress tests are better handled by a tool like
> stress-ng and some bespoke scripts for a particular system. Or else we
> have to create a framework inside LTP for deciding on and implementing
> reasonable limits.
>
>
I agree with Richard. We can just remove fork12 and use setrlimit01 
instead (test that needs to be refactoring anyway).

Andrea Cervesato


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp