Message ID | 20240313124542.13636-1-andrea.cervesato@suse.de |
---|---|
State | Superseded |
Headers | show |
Series | [v4] Refactor fork14 using new LTP API | expand |
Hi Andrea, generally LGTM. With using .skip_in_compat and SPDX GPL-2.0-only: Reviewed-by: Petr Vorel <pvorel@suse.cz> Minor notes below. > --- > Reverted SAFE_MMAP to mmap > Added kernel tags +1 > testcases/kernel/syscalls/fork/fork14.c | 208 +++++++++++------------- ... > +// 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 is GPL-2.0-only. > - * > - * 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 very nit: I would trust lore more, thus I would use 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 occur, fork will incorrectly succeed. The patch above fixed it. s/occur/occurs/ > + */ > -#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" > +#include "tst_test.h" > -char *TCID = "fork14"; > -int TST_TOTAL = 1; > +#ifndef TST_ABI32 We can use .skip_in_compat = 1, flag instead. > -#define GB (1024 * 1024 * 1024L) > +#include <stdlib.h> > +#include <sys/wait.h> > -/* set mmap threshold to 16TB */ > #define LARGE (16 * 1024) > #define EXTENT (16 * 1024 + 10) > -static char **pointer_vec; > +static char **memvec; > -static void setup(void); > -static void cleanup(void); > -static int fork_test(void); > - > -int main(int ac, char **av) > +static void run(void) > { > - int lc, reproduced; > + int i, j, ret; > + pid_t pid; > + void *mem; > + int prev_failed = 0; > + int passed = 1; > + int failures = 0; > + for (i = 0; i < EXTENT; i++) { I wonder if the code would not be slightly more readable if the content of the loop was in the separate function. Probably not, feel free to ignore. > + mem = mmap(NULL, 1 * TST_GB, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, > + 0, 0); ... > + if (mem == MAP_FAILED) { > + failures++; ... > + 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); > -} > + continue; > + } > - 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); Why not SAFE_WAITPID() as it was originally? > + 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 && memvec[i]) > + SAFE_MUNMAP(memvec[i], 1 * TST_GB); > } > - return reproduced; > + if (memvec) > + free(memvec); > } > + > +static struct tst_test test = { > + .test_all = run, > + .setup = setup, > + .cleanup = cleanup, > + .forks_child = 1, > + .tags = (const struct tst_tag[]) { > + {"linux-git", "7edc8b0ac16cbaed7cb4ea4c6b95ce98d2997e84"}, nit: we usually use shorter hash 7edc8b0ac16c (as used in Fixes: in git commits). > + {} > + } > +}; > + > +#else /* TST_ABI32 */ > + TST_TEST_TCONF("Test doesn't supports 32bits architecture"); s/supports/support/ But we can use .skip_in_compat = 1, flag instead. Kind regards, Petr > +#endif
Hi, On 3/22/24 07:42, Petr Vorel wrote: > Hi Andrea, > > generally LGTM. > With using .skip_in_compat and SPDX GPL-2.0-only: Totally forgot the license.. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Minor notes below. > >> --- >> Reverted SAFE_MMAP to mmap >> Added kernel tags > +1 > >> testcases/kernel/syscalls/fork/fork14.c | 208 +++++++++++------------- > ... >> +// 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 is GPL-2.0-only. >> - * >> - * 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 > very nit: I would trust lore more, thus I would use > > 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 occur, fork will incorrectly succeed. The patch above fixed it. > s/occur/occurs/ > >> + */ >> -#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" >> +#include "tst_test.h" >> -char *TCID = "fork14"; >> -int TST_TOTAL = 1; >> +#ifndef TST_ABI32 > We can use .skip_in_compat = 1, flag instead. > >> -#define GB (1024 * 1024 * 1024L) >> +#include <stdlib.h> >> +#include <sys/wait.h> >> -/* set mmap threshold to 16TB */ >> #define LARGE (16 * 1024) >> #define EXTENT (16 * 1024 + 10) >> -static char **pointer_vec; >> +static char **memvec; >> -static void setup(void); >> -static void cleanup(void); >> -static int fork_test(void); >> - >> -int main(int ac, char **av) >> +static void run(void) >> { >> - int lc, reproduced; >> + int i, j, ret; >> + pid_t pid; >> + void *mem; >> + int prev_failed = 0; >> + int passed = 1; >> + int failures = 0; >> + for (i = 0; i < EXTENT; i++) { > I wonder if the code would not be slightly more readable if the content of the > loop was in the separate function. Probably not, feel free to ignore. > >> + mem = mmap(NULL, 1 * TST_GB, >> + PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS, >> + 0, 0); > ... >> + if (mem == MAP_FAILED) { >> + failures++; > ... >> + 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); >> -} >> + continue; >> + } > >> - 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); > Why not SAFE_WAITPID() as it was originally? Was discussed with Cyril in the previous review. It's because we don't want it to fail only once. > >> + 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 && memvec[i]) >> + SAFE_MUNMAP(memvec[i], 1 * TST_GB); >> } >> - return reproduced; >> + if (memvec) >> + free(memvec); >> } >> + >> +static struct tst_test test = { >> + .test_all = run, >> + .setup = setup, >> + .cleanup = cleanup, >> + .forks_child = 1, >> + .tags = (const struct tst_tag[]) { >> + {"linux-git", "7edc8b0ac16cbaed7cb4ea4c6b95ce98d2997e84"}, > nit: we usually use shorter hash 7edc8b0ac16c (as used in Fixes: in git > commits). > >> + {} >> + } >> +}; >> + >> +#else /* TST_ABI32 */ >> + TST_TEST_TCONF("Test doesn't supports 32bits architecture"); > s/supports/support/ > > But we can use .skip_in_compat = 1, flag instead. > > Kind regards, > Petr > >> +#endif Andrea
Hi Andrea, > > generally LGTM. > > With using .skip_in_compat and SPDX GPL-2.0-only: > Totally forgot the license.. Thanks for sending v5. ... > > > - 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); > > Why not SAFE_WAITPID() as it was originally? > Was discussed with Cyril in the previous review. It's because we don't want > it to fail only once. Ah, I'm sorry, I overlooked it. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/fork/fork14.c b/testcases/kernel/syscalls/fork/fork14.c index 93af2ebac..c5a5a96be 100644 --- a/testcases/kernel/syscalls/fork/fork14.c +++ b/testcases/kernel/syscalls/fork/fork14.c @@ -1,143 +1,127 @@ -/********************************************************************* +// 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. - ********************************************************************/ + * 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" +#include "tst_test.h" -char *TCID = "fork14"; -int TST_TOTAL = 1; +#ifndef TST_ABI32 -#define GB (1024 * 1024 * 1024L) +#include <stdlib.h> +#include <sys/wait.h> -/* set mmap threshold to 16TB */ #define LARGE (16 * 1024) #define EXTENT (16 * 1024 + 10) -static char **pointer_vec; +static char **memvec; -static void setup(void); -static void cleanup(void); -static int fork_test(void); - -int main(int ac, char **av) +static void run(void) { - int lc, reproduced; + int i, j, ret; + pid_t pid; + void *mem; + int prev_failed = 0; + int passed = 1; + int failures = 0; - 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) { + failures++; -static void setup(void) -{ - tst_sig(FORK, DEF_HANDLER, cleanup); - TEST_PAUSE; + tst_res(TINFO, "mmap() failed"); - 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); -} + continue; + } -static int fork_test(void) -{ - int i, j, prev_failed = 0, fails = 0, cnt = 0; - int reproduced = 0; - void *addr; + memvec[i] = mem; - 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 + pid = fork(); + + 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 && memvec[i]) + SAFE_MUNMAP(memvec[i], 1 * TST_GB); } - return reproduced; + if (memvec) + free(memvec); } + +static struct tst_test test = { + .test_all = run, + .setup = setup, + .cleanup = cleanup, + .forks_child = 1, + .tags = (const struct tst_tag[]) { + {"linux-git", "7edc8b0ac16cbaed7cb4ea4c6b95ce98d2997e84"}, + {} + } +}; + +#else /* TST_ABI32 */ + TST_TEST_TCONF("Test doesn't supports 32bits architecture"); +#endif