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

[Andrea Cervesato]

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