[LTP] [PATCH v2] Refactor fork14 using new LTP API

[LTP] [PATCH v2] Refactor fork14 using new LTP API

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Fixed copyright
Raise TCONF on map failures

 testcases/kernel/syscalls/fork/fork14.c | 202 +++++++++++-------------
 1 file changed, 89 insertions(+), 113 deletions(-)

diff --git a/testcases/kernel/syscalls/fork/fork14.c b/testcases/kernel/syscalls/fork/fork14.c
index 93af2ebac..0ad573fe4 100644
--- a/testcases/kernel/syscalls/fork/fork14.c
+++ b/testcases/kernel/syscalls/fork/fork14.c
@@ -1,143 +1,119 @@
-/*********************************************************************
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
  * Copyright (C) 2014  Red Hat, Inc.
+ * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
- *
- * This test is a reporducer for this patch:
- *              https://lkml.org/lkml/2012/4/24/328
+ * This test is a reporducer for this patch: https://lkml.org/lkml/2012/4/24/328
  * Since vma length in dup_mmap is calculated and stored in a unsigned
  * int, it will overflow when length of mmaped memory > 16 TB. When
- * overflow occur, fork will  incorrectly succeed. The patch above
- * fixed it.
- ********************************************************************/
-
-#include <sys/mman.h>
-#include <sys/wait.h>
-#include <stdio.h>
-#include <unistd.h>
-#include "test.h"
-#include "safe_macros.h"
-#include "lapi/abisize.h"
+ * overflow occur, fork will incorrectly succeed. The patch above fixed it.
+ */
 
-char *TCID = "fork14";
-int TST_TOTAL = 1;
+#ifndef TST_ABI32
 
-#define GB		(1024 * 1024 * 1024L)
+#include <stdlib.h>
+#include <sys/wait.h>
+#include "tst_test.h"
 
-/* set mmap threshold to 16TB */
 #define LARGE		(16 * 1024)
 #define EXTENT		(16 * 1024 + 10)
 
-static char **pointer_vec;
-
-static void setup(void);
-static void cleanup(void);
-static int  fork_test(void);
+static char **memvec;
 
-int main(int ac, char **av)
+static void run(void)
 {
-	int lc, reproduced;
+	int i, j, ret;
+	pid_t pid;
+	void *mem;
+	int failures = 0;
+	int prev_failed = 0;
+	int passed = 1;
 
-	tst_parse_opts(ac, av, NULL, NULL);
-/*
- * Tested on ppc64/x86_64/i386/s390x. And only 64bit has this issue.
- * Since a 32bit program can't mmap so many memory.
- */
-#ifdef TST_ABI32
-	tst_brkm(TCONF, NULL, "This test is only for 64bit.");
-#endif
-	setup();
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
+	for (i = 0; i < EXTENT; i++) {
+		mem = mmap(NULL, 1 * TST_GB,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS,
+			0, 0);
 
-		reproduced = fork_test();
-		if (reproduced == 0)
-			tst_resm(TPASS, "fork failed as expected.");
-	}
-	cleanup();
-	tst_exit();
-}
+		if (mem == MAP_FAILED) {
+			tst_res(TINFO, "mmap() failed");
 
-static void setup(void)
-{
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	TEST_PAUSE;
+			++failures;
 
-	pointer_vec = SAFE_MALLOC(cleanup, EXTENT * sizeof(char *));
-}
+			if (failures > 10) {
+				tst_brk(TCONF, "mmap() fails too many "
+					"times, so it's almost impossible to "
+					"get a vm_area_struct sized 16TB.");
+			}
+		}
 
-static void cleanup(void)
-{
-	free(pointer_vec);
-}
+		memvec[i] = mem;
 
-static int fork_test(void)
-{
-	int i, j, prev_failed = 0, fails = 0, cnt = 0;
-	int reproduced = 0;
-	void *addr;
+		pid = fork();
 
-	for (i = 0; i < EXTENT; i++) {
-		addr = mmap(NULL, 1 * GB, PROT_READ | PROT_WRITE,
-			    MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
-		if (addr == MAP_FAILED) {
-			pointer_vec[i] = NULL;
-			fails++;
-			/*
-			 * EXTENT is "16*1024+10", if fails count exceeds 10,
-			 * we are almost impossible to get an vm_area_struct
-			 * sized 16TB
+		if (pid == -1) {
+			/* keep track of the failed fork() and verify that next one
+			 * is failing as well.
 			 */
-			if (fails == 11) {
-				tst_brkm(TCONF, cleanup, "mmap() fails too many"
-					 "times, so we are almost impossible to"
-					 " get an vm_area_struct sized 16TB.");
-			}
-		} else {
-			pointer_vec[i] = addr;
+			prev_failed = 1;
+			continue;
 		}
-		cnt++;
 
-		switch (tst_fork()) {
-		case -1:
-			prev_failed = 1;
-		break;
-		case 0:
+		if (!pid)
 			exit(0);
-		default:
-			SAFE_WAITPID(cleanup, -1, NULL, 0);
 
-			if (prev_failed > 0 && i >= LARGE) {
-				tst_resm(TFAIL, "Fork succeeds incorrectly");
-				reproduced = 1;
-				goto clear_memory_map;
-			}
+		ret = waitpid(pid, NULL, 0);
+		if (ret == -1 && errno != ECHILD)
+			tst_brk(TBROK | TERRNO, "waitpid() error");
+
+		if (prev_failed && i >= LARGE) {
+			passed = 0;
+			break;
 		}
+
+		prev_failed = 0;
+
+		tst_res(TINFO, "fork() passed at %d attempt", i);
+	}
+
+	for (j = 0; j < i; j++) {
+		if (memvec[j])
+			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
 	}
 
-clear_memory_map:
-	for (j = 0; j < cnt; j++) {
-		if (pointer_vec[j])
-			SAFE_MUNMAP(cleanup, pointer_vec[j], 1 * GB);
+	if (passed)
+		tst_res(TPASS, "fork() failed as expected");
+	else
+		tst_res(TFAIL, "fork() succeeded incorrectly");
+}
+
+static void setup(void)
+{
+	memvec = SAFE_MALLOC(EXTENT * sizeof(char *));
+	memset(memvec, 0, EXTENT);
+}
+
+static void cleanup(void)
+{
+	for (long i = 0; i < EXTENT; i++) {
+		if (memvec[i])
+			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
 	}
 
-	return reproduced;
+	free(memvec);
 }
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.forks_child = 1,
+};
+
+#else /* TST_ABI32 */
+	TST_TEST_TCONF("Test doesn't supports 32bits architecture");
+#endif
-- 
2.35.3


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

Re: [LTP] [PATCH v2] Refactor fork14 using new LTP API

[Petr Vorel]

Hi Andrea,

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

> +#ifndef TST_ABI32

> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include "tst_test.h"
This will fail on 32bit arch, where TST_TEST_TCONF() is used. You need to
include tst_test.h first.

...
> +static void run(void)
{
...
> +	for (i = 0; i < EXTENT; i++) {
> +		mem = mmap(NULL, 1 * TST_GB,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS,
> +			0, 0);
We have SAFE_MMAP(), is there a reason not to use it?

> -		reproduced = fork_test();
> -		if (reproduced == 0)
> -			tst_resm(TPASS, "fork failed as expected.");
> -	}
> -	cleanup();
> -	tst_exit();
> -}
> +		if (mem == MAP_FAILED) {
> +			tst_res(TINFO, "mmap() failed");
...
> +			if (failures > 10) {
> +				tst_brk(TCONF, "mmap() fails too many "
> +					"times, so it's almost impossible to "
> +					"get a vm_area_struct sized 16TB.");
I would join this into single string.
> +			}
> +		}

> -		case -1:
> -			prev_failed = 1;
> -		break;
> -		case 0:
> +		if (!pid)
>  			exit(0);
> -		default:
> -			SAFE_WAITPID(cleanup, -1, NULL, 0);

> -			if (prev_failed > 0 && i >= LARGE) {
> -				tst_resm(TFAIL, "Fork succeeds incorrectly");
> -				reproduced = 1;
> -				goto clear_memory_map;
> -			}
> +		ret = waitpid(pid, NULL, 0);
> +		if (ret == -1 && errno != ECHILD)
> +			tst_brk(TBROK | TERRNO, "waitpid() error");
Why not use SAFE_WAITPID(). It was even used before.

> +
> +		if (prev_failed && i >= LARGE) {
> +			passed = 0;
> +			break;
>  		}
> +
> +		prev_failed = 0;
> +
> +		tst_res(TINFO, "fork() passed at %d attempt", i);
> +	}
> +
> +	for (j = 0; j < i; j++) {
> +		if (memvec[j])
> +			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
>  	}

...
> +static void setup(void)
> +{
> +	memvec = SAFE_MALLOC(EXTENT * sizeof(char *));
> +	memset(memvec, 0, EXTENT);
> +}
> +
> +static void cleanup(void)
> +{
> +	for (long i = 0; i < EXTENT; i++) {
> +		if (memvec[i])
If malloc() in setup() fails (rare case, I know) bad thing happen here,
you should check for (memvec && memvec[i]) here
> +			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
>  	}

Also here should be check if (memvec)
> +	free(memvec);

Kind regards,
Petr

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

Re: [LTP] [PATCH v2] Refactor fork14 using new LTP API

[Andrea Cervesato via ltp]

Hi!

On 12/18/23 12:58, Petr Vorel wrote:
> Hi Andrea,
>
>> From: Andrea Cervesato <andrea.cervesato@suse.com>
>> +#ifndef TST_ABI32
>> +#include <stdlib.h>
>> +#include <sys/wait.h>
>> +#include "tst_test.h"
> This will fail on 32bit arch, where TST_TEST_TCONF() is used. You need to
> include tst_test.h first.
>
> ...
>> +static void run(void)
> {
> ...
>> +	for (i = 0; i < EXTENT; i++) {
>> +		mem = mmap(NULL, 1 * TST_GB,
>> +			PROT_READ | PROT_WRITE,
>> +			MAP_PRIVATE | MAP_ANONYMOUS,
>> +			0, 0);
> We have SAFE_MMAP(), is there a reason not to use it?
I think the original idea was to wait some memory was released by the 
system, but now I see that the algorithm is just broken, since we are 
not skipping any cycle if mmap fails. So I will just use SAFE_MMAP()
>
>> -		reproduced = fork_test();
>> -		if (reproduced == 0)
>> -			tst_resm(TPASS, "fork failed as expected.");
>> -	}
>> -	cleanup();
>> -	tst_exit();
>> -}
>> +		if (mem == MAP_FAILED) {
>> +			tst_res(TINFO, "mmap() failed");
> ...
>> +			if (failures > 10) {
>> +				tst_brk(TCONF, "mmap() fails too many "
>> +					"times, so it's almost impossible to "
>> +					"get a vm_area_struct sized 16TB.");
> I would join this into single string.
This will go away if we use SAFE_MMAP()
>> +			}
>> +		}
>> -		case -1:
>> -			prev_failed = 1;
>> -		break;
>> -		case 0:
>> +		if (!pid)
>>   			exit(0);
>> -		default:
>> -			SAFE_WAITPID(cleanup, -1, NULL, 0);
>> -			if (prev_failed > 0 && i >= LARGE) {
>> -				tst_resm(TFAIL, "Fork succeeds incorrectly");
>> -				reproduced = 1;
>> -				goto clear_memory_map;
>> -			}
>> +		ret = waitpid(pid, NULL, 0);
>> +		if (ret == -1 && errno != ECHILD)
>> +			tst_brk(TBROK | TERRNO, "waitpid() error");
> Why not use SAFE_WAITPID(). It was even used before.
Because child might end before calling waitpid().
>
>> +
>> +		if (prev_failed && i >= LARGE) {
>> +			passed = 0;
>> +			break;
>>   		}
>> +
>> +		prev_failed = 0;
>> +
>> +		tst_res(TINFO, "fork() passed at %d attempt", i);
>> +	}
>> +
>> +	for (j = 0; j < i; j++) {
>> +		if (memvec[j])
>> +			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
>>   	}
> ...
>> +static void setup(void)
>> +{
>> +	memvec = SAFE_MALLOC(EXTENT * sizeof(char *));
>> +	memset(memvec, 0, EXTENT);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	for (long i = 0; i < EXTENT; i++) {
>> +		if (memvec[i])
> If malloc() in setup() fails (rare case, I know) bad thing happen here,
> you should check for (memvec && memvec[i]) here
>> +			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
>>   	}
> Also here should be check if (memvec)
>> +	free(memvec);
> Kind regards,
> Petr

Andrea


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