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

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

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Reset memvec array before each loop
TDEBUG info for "fork() passed attempt" message

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

diff --git a/testcases/kernel/syscalls/fork/fork14.c b/testcases/kernel/syscalls/fork/fork14.c
index 93af2ebac..213c99d36 100644
--- a/testcases/kernel/syscalls/fork/fork14.c
+++ b/testcases/kernel/syscalls/fork/fork14.c
@@ -1,143 +1,123 @@
-/*********************************************************************
+// SPDX-License-Identifier: GPL-2.0-only
+/*
  * Copyright (C) 2014  Red Hat, Inc.
- *
- * 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.
+ * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
  *
  * This test is a reporducer for this patch:
- *              https://lkml.org/lkml/2012/4/24/328
+ * https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
  * 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.
- ********************************************************************/
+ * overflow occurs, fork will incorrectly succeed. The patch above fixed it.
+ */
 
-#include <sys/mman.h>
+#include "tst_test.h"
+#include <stdlib.h>
 #include <sys/wait.h>
-#include <stdio.h>
-#include <unistd.h>
-#include "test.h"
-#include "safe_macros.h"
-#include "lapi/abisize.h"
-
-char *TCID = "fork14";
-int TST_TOTAL = 1;
 
-#define GB		(1024 * 1024 * 1024L)
-
-/* 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;
-
-	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;
-
-		reproduced = fork_test();
-		if (reproduced == 0)
-			tst_resm(TPASS, "fork failed as expected.");
-	}
-	cleanup();
-	tst_exit();
-}
+	int i, j, ret;
+	pid_t pid;
+	void *mem;
+	int prev_failed = 0;
+	int passed = 1;
+	int failures = 0;
 
-static void setup(void)
-{
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	TEST_PAUSE;
+	memset(memvec, 0, EXTENT);
 
-	pointer_vec = SAFE_MALLOC(cleanup, EXTENT * sizeof(char *));
-}
+	for (i = 0; i < EXTENT; i++) {
+		mem = mmap(NULL, 1 * TST_GB,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS,
+			0, 0);
 
-static void cleanup(void)
-{
-	free(pointer_vec);
-}
+		if (mem == MAP_FAILED) {
+			failures++;
 
-static int fork_test(void)
-{
-	int i, j, prev_failed = 0, fails = 0, cnt = 0;
-	int reproduced = 0;
-	void *addr;
+			tst_res(TINFO, "mmap() failed");
 
-	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 (fails == 11) {
-				tst_brkm(TCONF, cleanup, "mmap() fails too many"
-					 "times, so we are almost impossible to"
-					 " get an vm_area_struct sized 16TB.");
+			if (failures > 10) {
+				tst_brk(TCONF, "mmap() fails too many "
+					"times, so it's almost impossible to "
+					"get a vm_area_struct sized 16TB.");
 			}
-		} else {
-			pointer_vec[i] = addr;
+
+			continue;
 		}
-		cnt++;
 
-		switch (tst_fork()) {
-		case -1:
+		memvec[i] = mem;
+
+		pid = fork();
+
+		if (pid == -1) {
+			/* keep track of the failed fork() and verify that next one
+			 * is failing as well.
+			 */
 			prev_failed = 1;
-		break;
-		case 0:
+			continue;
+		}
+
+		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(TDEBUG, "fork() passed at %d attempt", i);
 	}
 
-clear_memory_map:
-	for (j = 0; j < cnt; j++) {
-		if (pointer_vec[j])
-			SAFE_MUNMAP(cleanup, pointer_vec[j], 1 * GB);
+	for (j = 0; j < i; j++) {
+		if (memvec[j])
+			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
 	}
 
-	return reproduced;
+	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 *));
+}
+
+static void cleanup(void)
+{
+	for (long i = 0; i < EXTENT; i++) {
+		if (memvec && memvec[i])
+			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
+	}
+
+	if (memvec)
+		free(memvec);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.forks_child = 1,
+	.skip_in_compat = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "7edc8b0ac16c"},
+		{}
+	}
+};
-- 
2.35.3


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

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

[Martin Doucha]

Hi,
Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 03. 05. 24 10:16, Andrea Cervesato wrote:
> From: Andrea Cervesato <andrea.cervesato@suse.com>
> 
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> Reset memvec array before each loop
> TDEBUG info for "fork() passed attempt" message
> 
>   testcases/kernel/syscalls/fork/fork14.c | 206 +++++++++++-------------
>   1 file changed, 93 insertions(+), 113 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fork/fork14.c b/testcases/kernel/syscalls/fork/fork14.c
> index 93af2ebac..213c99d36 100644
> --- a/testcases/kernel/syscalls/fork/fork14.c
> +++ b/testcases/kernel/syscalls/fork/fork14.c
> @@ -1,143 +1,123 @@
> -/*********************************************************************
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
>    * Copyright (C) 2014  Red Hat, Inc.
> - *
> - * 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.
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
>    *
>    * This test is a reporducer for this patch:
> - *              https://lkml.org/lkml/2012/4/24/328
> + * https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
>    * 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.
> - ********************************************************************/
> + * overflow occurs, fork will incorrectly succeed. The patch above fixed it.
> + */
>   
> -#include <sys/mman.h>
> +#include "tst_test.h"
> +#include <stdlib.h>
>   #include <sys/wait.h>
> -#include <stdio.h>
> -#include <unistd.h>
> -#include "test.h"
> -#include "safe_macros.h"
> -#include "lapi/abisize.h"
> -
> -char *TCID = "fork14";
> -int TST_TOTAL = 1;
>   
> -#define GB		(1024 * 1024 * 1024L)
> -
> -/* 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;
> -
> -	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;
> -
> -		reproduced = fork_test();
> -		if (reproduced == 0)
> -			tst_resm(TPASS, "fork failed as expected.");
> -	}
> -	cleanup();
> -	tst_exit();
> -}
> +	int i, j, ret;
> +	pid_t pid;
> +	void *mem;
> +	int prev_failed = 0;
> +	int passed = 1;
> +	int failures = 0;
>   
> -static void setup(void)
> -{
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	TEST_PAUSE;
> +	memset(memvec, 0, EXTENT);
>   
> -	pointer_vec = SAFE_MALLOC(cleanup, EXTENT * sizeof(char *));
> -}
> +	for (i = 0; i < EXTENT; i++) {
> +		mem = mmap(NULL, 1 * TST_GB,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS,
> +			0, 0);
>   
> -static void cleanup(void)
> -{
> -	free(pointer_vec);
> -}
> +		if (mem == MAP_FAILED) {
> +			failures++;
>   
> -static int fork_test(void)
> -{
> -	int i, j, prev_failed = 0, fails = 0, cnt = 0;
> -	int reproduced = 0;
> -	void *addr;
> +			tst_res(TINFO, "mmap() failed");
>   
> -	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 (fails == 11) {
> -				tst_brkm(TCONF, cleanup, "mmap() fails too many"
> -					 "times, so we are almost impossible to"
> -					 " get an vm_area_struct sized 16TB.");
> +			if (failures > 10) {
> +				tst_brk(TCONF, "mmap() fails too many "
> +					"times, so it's almost impossible to "
> +					"get a vm_area_struct sized 16TB.");
>   			}
> -		} else {
> -			pointer_vec[i] = addr;
> +
> +			continue;
>   		}
> -		cnt++;
>   
> -		switch (tst_fork()) {
> -		case -1:
> +		memvec[i] = mem;
> +
> +		pid = fork();
> +
> +		if (pid == -1) {
> +			/* keep track of the failed fork() and verify that next one
> +			 * is failing as well.
> +			 */
>   			prev_failed = 1;
> -		break;
> -		case 0:
> +			continue;
> +		}
> +
> +		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(TDEBUG, "fork() passed at %d attempt", i);
>   	}
>   
> -clear_memory_map:
> -	for (j = 0; j < cnt; j++) {
> -		if (pointer_vec[j])
> -			SAFE_MUNMAP(cleanup, pointer_vec[j], 1 * GB);
> +	for (j = 0; j < i; j++) {
> +		if (memvec[j])
> +			SAFE_MUNMAP(memvec[j], 1 * TST_GB);
>   	}
>   
> -	return reproduced;
> +	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 *));
> +}
> +
> +static void cleanup(void)
> +{
> +	for (long i = 0; i < EXTENT; i++) {
> +		if (memvec && memvec[i])
> +			SAFE_MUNMAP(memvec[i], 1 * TST_GB);
> +	}
> +
> +	if (memvec)
> +		free(memvec);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.forks_child = 1,
> +	.skip_in_compat = 1,
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "7edc8b0ac16c"},
> +		{}
> +	}
> +};

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

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

[Petr Vorel]

Hi Andrea, Martin,

> Hi,
> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

+1

...

Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > +
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.forks_child = 1,
> > +	.skip_in_compat = 1,

BTW test on x86 (i.e. true 64 bit) behaves exactly the same as on compat mode:

tst_test.c:1614: TINFO: Timeout per run is 0h 00m 30s
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:46: TINFO: mmap() failed
fork14.c:49: TCONF: mmap() fails too many times, so it's almost impossible to get a vm_area_struct sized 16TB.

IMHO we should whitelist it as well, I can change before merge with diff below.

(More elegant way would be to add .skip_in_32bit.)

Kind regards,
Petr

+++ testcases/kernel/syscalls/fork/fork14.c
@@ -18,6 +18,8 @@
 #include <stdlib.h>
 #include <sys/wait.h>
 
+#ifndef __i386__
+
 #define LARGE		(16 * 1024)
 #define EXTENT		(16 * 1024 + 10)
 
@@ -121,3 +123,6 @@ static struct tst_test test = {
 		{}
 	}
 };
+#else
+TST_TEST_TCONF("Not supported on x86 in 32-bit");
+#endif

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

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

[Andrea Cervesato via ltp]

Hi!

On 5/6/24 22:26, Petr Vorel wrote:
> Hi Andrea, Martin,
>
>> Hi,
>> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
> +1
>
> ...
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>> +
>>> +static struct tst_test test = {
>>> +	.test_all = run,
>>> +	.setup = setup,
>>> +	.cleanup = cleanup,
>>> +	.forks_child = 1,
>>> +	.skip_in_compat = 1,
> BTW test on x86 (i.e. true 64 bit) behaves exactly the same as on compat mode:
>
> tst_test.c:1614: TINFO: Timeout per run is 0h 00m 30s
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:46: TINFO: mmap() failed
> fork14.c:49: TCONF: mmap() fails too many times, so it's almost impossible to get a vm_area_struct sized 16TB.
>
> IMHO we should whitelist it as well, I can change before merge with diff below.
>
> (More elegant way would be to add .skip_in_32bit.)
>
> Kind regards,
> Petr
>
> +++ testcases/kernel/syscalls/fork/fork14.c
> @@ -18,6 +18,8 @@
>   #include <stdlib.h>
>   #include <sys/wait.h>
>   
> +#ifndef __i386__
TST_ABI32 is not enough? What about ".skip_in_compat"?
> +
>   #define LARGE		(16 * 1024)
>   #define EXTENT		(16 * 1024 + 10)
>   
> @@ -121,3 +123,6 @@ static struct tst_test test = {
>   		{}
>   	}
>   };
> +#else
> +TST_TEST_TCONF("Not supported on x86 in 32-bit");
> +#endif

Andrea


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

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

[Petr Vorel]

Hi Andrea,

...
> +/*\
> + * [Description]
>   *
>   * This test is a reporducer for this patch:
s/reporducer/reproducer/

> + * https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
>   * 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 occurs, fork will incorrectly succeed. The patch above fixed it.
> + */

nit: this has been merged to mainline long time ago (kernel 3.5). I'll change before merge to:

/*\
 * [Description]
 *
 * This test is a reproducer for kernel 3.5:
 * 7edc8b0ac16c ("mm/fork: fix overflow in vma length when copying mmap on clone")
 *
 * 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 occurs, fork will incorrectly succeed. The patch above fixed it.
 */

Kind regards,
Petr

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

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

[Petr Vorel]

Hi Andrea,

> Hi!

> On 5/6/24 22:26, Petr Vorel wrote:
> > Hi Andrea, Martin,

> >> Hi,
> >> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
> > +1

> > ...

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> >>> +
> >>> +static struct tst_test test = {
> >>> +	.test_all = run,
> >>> +	.setup = setup,
> >>> +	.cleanup = cleanup,
> >>> +	.forks_child = 1,
> >>> +	.skip_in_compat = 1,
> > BTW test on x86 (i.e. true 64 bit) behaves exactly the same as on compat mode:

> > tst_test.c:1614: TINFO: Timeout per run is 0h 00m 30s
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:46: TINFO: mmap() failed
> > fork14.c:49: TCONF: mmap() fails too many times, so it's almost impossible to get a vm_area_struct sized 16TB.

> > IMHO we should whitelist it as well, I can change before merge with diff below.

> > (More elegant way would be to add .skip_in_32bit.)

> > Kind regards,
> > Petr

> > +++ testcases/kernel/syscalls/fork/fork14.c
> > @@ -18,6 +18,8 @@
> >   #include <stdlib.h>
> >   #include <sys/wait.h>

> > +#ifndef __i386__
> TST_ABI32 is not enough? What about ".skip_in_compat"?

Yes, TST_ABI32 is actually better than limit to __i386__.
Do you give ack for these changes below? Or feel free to send new version?
IMHO the problem is 16TB is too much for 32 bit kernel (4 GiB limit).

.skip_in_compat would not be enough, because the problem is on pure 32 bit
distro e.g. with 32bit kernel (yes, there are people who use them), but
.skip_in_compat is for 64 bit kernel with 32bit userspace (compiled with -m32 in
CFLAGS and LDFLAGS).

Also, I suggested in the patch below to remove .skip_in_compat, but for info
version it would be good to keep it. I suppose it's not worth to add new flag
.skip_in_32bit for this single case (it'd be good for doc purposes).
@Cyril WDYT?

+++ testcases/kernel/syscalls/fork/fork14.c
@@ -7,17 +7,21 @@
 /*\
  * [Description]
  *
- * This test is a reporducer for this patch:
- * https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
- * Since vma length in dup_mmap is calculated and stored in a unsigned
+ * This test is a reproducer for kernel 3.5:
+ * 7edc8b0ac16c ("mm/fork: fix overflow in vma length when copying mmap on clone")
+ *
+ * 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 occurs, fork will incorrectly succeed. The patch above fixed it.
  */
 
+#include "lapi/abisize.h"
 #include "tst_test.h"
 #include <stdlib.h>
 #include <sys/wait.h>
 
+#ifndef TST_ABI32
+
 #define LARGE		(16 * 1024)
 #define EXTENT		(16 * 1024 + 10)
 
@@ -48,7 +52,7 @@ static void run(void)
 			if (failures > 10) {
 				tst_brk(TCONF, "mmap() fails too many "
 					"times, so it's almost impossible to "
-					"get a vm_area_struct sized 16TB.");
+					"get a vm_area_struct sized 16TB");
 			}
 
 			continue;
@@ -115,9 +119,11 @@ static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
 	.forks_child = 1,
-	.skip_in_compat = 1,
 	.tags = (const struct tst_tag[]) {
 		{"linux-git", "7edc8b0ac16c"},
 		{}
 	}
 };
+#else
+TST_TEST_TCONF("Not supported on x86 in 32-bit");
+#endif

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

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

[Cyril Hrubis]

Hi!
> +#ifndef TST_ABI32
> +
>  #define LARGE		(16 * 1024)
>  #define EXTENT		(16 * 1024 + 10)
>  
> @@ -48,7 +52,7 @@ static void run(void)
>  			if (failures > 10) {
>  				tst_brk(TCONF, "mmap() fails too many "
>  					"times, so it's almost impossible to "
> -					"get a vm_area_struct sized 16TB.");
> +					"get a vm_area_struct sized 16TB");
>  			}
>  
>  			continue;
> @@ -115,9 +119,11 @@ static struct tst_test test = {
>  	.setup = setup,
>  	.cleanup = cleanup,
>  	.forks_child = 1,
> -	.skip_in_compat = 1,
>  	.tags = (const struct tst_tag[]) {
>  		{"linux-git", "7edc8b0ac16c"},
>  		{}
>  	}
>  };
> +#else
> +TST_TEST_TCONF("Not supported on x86 in 32-bit");
> +#endif

I think that adding ifdefs and removing metadata is actually a step
back. I suppose that we need .skip_on_32bit or something similar.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Andrea Cervesato via ltp]

Hi Petr,

On 5/15/24 16:22, Petr Vorel wrote:
> Hi Andrea,
>
>> Hi!
>> On 5/6/24 22:26, Petr Vorel wrote:
>>> Hi Andrea, Martin,
>>>> Hi,
>>>> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
>>> +1
>>> ...
>>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>>>> +
>>>>> +static struct tst_test test = {
>>>>> +	.test_all = run,
>>>>> +	.setup = setup,
>>>>> +	.cleanup = cleanup,
>>>>> +	.forks_child = 1,
>>>>> +	.skip_in_compat = 1,
>>> BTW test on x86 (i.e. true 64 bit) behaves exactly the same as on compat mode:
>>> tst_test.c:1614: TINFO: Timeout per run is 0h 00m 30s
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:49: TCONF: mmap() fails too many times, so it's almost impossible to get a vm_area_struct sized 16TB.
>>> IMHO we should whitelist it as well, I can change before merge with diff below.
>>> (More elegant way would be to add .skip_in_32bit.)
>>> Kind regards,
>>> Petr
>>> +++ testcases/kernel/syscalls/fork/fork14.c
>>> @@ -18,6 +18,8 @@
>>>    #include <stdlib.h>
>>>    #include <sys/wait.h>
>>> +#ifndef __i386__
>> TST_ABI32 is not enough? What about ".skip_in_compat"?
> Yes, TST_ABI32 is actually better than limit to __i386__.
> Do you give ack for these changes below? Or feel free to send new version?
Sure feel free to use TST_API32
> IMHO the problem is 16TB is too much for 32 bit kernel (4 GiB limit).
This is wanted. It's explained in the bug description
>
> .skip_in_compat would not be enough, because the problem is on pure 32 bit
> distro e.g. with 32bit kernel (yes, there are people who use them), but
> .skip_in_compat is for 64 bit kernel with 32bit userspace (compiled with -m32 in
> CFLAGS and LDFLAGS).
>
> Also, I suggested in the patch below to remove .skip_in_compat, but for info
> version it would be good to keep it. I suppose it's not worth to add new flag
> .skip_in_32bit for this single case (it'd be good for doc purposes).
> @Cyril WDYT?
>
> +++ testcases/kernel/syscalls/fork/fork14.c
> @@ -7,17 +7,21 @@
>   /*\
>    * [Description]
>    *
> - * This test is a reporducer for this patch:
> - * https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
> - * Since vma length in dup_mmap is calculated and stored in a unsigned
> + * This test is a reproducer for kernel 3.5:
> + * 7edc8b0ac16c ("mm/fork: fix overflow in vma length when copying mmap on clone")
> + *
> + * 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 occurs, fork will incorrectly succeed. The patch above fixed it.
>    */
>   
> +#include "lapi/abisize.h"
>   #include "tst_test.h"
>   #include <stdlib.h>
>   #include <sys/wait.h>
>   
> +#ifndef TST_ABI32
> +
>   #define LARGE		(16 * 1024)
>   #define EXTENT		(16 * 1024 + 10)
>   
> @@ -48,7 +52,7 @@ static void run(void)
>   			if (failures > 10) {
>   				tst_brk(TCONF, "mmap() fails too many "
>   					"times, so it's almost impossible to "
> -					"get a vm_area_struct sized 16TB.");
> +					"get a vm_area_struct sized 16TB");
>   			}
>   
>   			continue;
> @@ -115,9 +119,11 @@ static struct tst_test test = {
>   	.setup = setup,
>   	.cleanup = cleanup,
>   	.forks_child = 1,
> -	.skip_in_compat = 1,
>   	.tags = (const struct tst_tag[]) {
>   		{"linux-git", "7edc8b0ac16c"},
>   		{}
>   	}
>   };
> +#else
> +TST_TEST_TCONF("Not supported on x86 in 32-bit");
> +#endif

Andrea


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

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

[Petr Vorel]

> Hi!
> > +#ifndef TST_ABI32
> > +
> >  #define LARGE		(16 * 1024)
> >  #define EXTENT		(16 * 1024 + 10)

> > @@ -48,7 +52,7 @@ static void run(void)
> >  			if (failures > 10) {
> >  				tst_brk(TCONF, "mmap() fails too many "
> >  					"times, so it's almost impossible to "
> > -					"get a vm_area_struct sized 16TB.");
> > +					"get a vm_area_struct sized 16TB");
> >  			}

> >  			continue;
> > @@ -115,9 +119,11 @@ static struct tst_test test = {
> >  	.setup = setup,
> >  	.cleanup = cleanup,
> >  	.forks_child = 1,
> > -	.skip_in_compat = 1,
> >  	.tags = (const struct tst_tag[]) {
> >  		{"linux-git", "7edc8b0ac16c"},
> >  		{}
> >  	}
> >  };
> > +#else
> > +TST_TEST_TCONF("Not supported on x86 in 32-bit");
> > +#endif

> I think that adding ifdefs and removing metadata is actually a step
> back. I suppose that we need .skip_on_32bit or something similar.

OK, because it's trivial I'll send a patch with rebased Andrea's commit.

Kind regards,
Petr

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