Re: [LTP] [PATCH v2 2/3] mremap03: Convert to new API

[Avinesh Kumar <akumar@suse.de>]

Hi Yang Xu,
some comments below.

On Tuesday, February 27, 2024 9:42:43 AM CET Yang Xu via ltp wrote:
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  testcases/kernel/syscalls/mremap/mremap03.c | 201 ++++----------------
>  1 file changed, 39 insertions(+), 162 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mremap/mremap03.c
> b/testcases/kernel/syscalls/mremap/mremap03.c index 02b79bc47..4e55dbda6
> 100644
> --- a/testcases/kernel/syscalls/mremap/mremap03.c
> +++ b/testcases/kernel/syscalls/mremap/mremap03.c
> @@ -1,187 +1,64 @@
> +// 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 + * Copyright (c) International Business Machines  Corp.,
> 2001
> + * Copyright (c) Linux Test Project, 2001-2024
> + * 07/2001 Ported by Wayne Boyer
>   */
> 
> -/*
> - * Test Name: mremap03
> - *
> - * Test Description:
> - *  Verify that,
> - *   mremap() fails when used to expand the existing virtual memory mapped
> - *   region to the requested size, if there already exists mappings that
> - *   cover the whole address space requsted or the old address specified
> was - *   not mapped.
> - *
> - * Expected Result:
> - *  mremap() should return -1 and set errno to EFAULT.
> - *
> - * Algorithm:
> - *  Setup:
> - *   Setup signal handling.
> - *   Pause for SIGUSR1 if option specified.
> - *
> - *  Test:
> - *   Loop if the proper options are given.
> - *   Execute system call
> - *   Check return code, if system call failed (return=-1)
> - *	if errno set == expected errno
> - *		Issue sys call fails with expected return value and errno.
> - *	Otherwise,
> - *		Issue sys call fails with unexpected errno.
> - *   Otherwise,
> - *	Issue sys call returns unexpected value.
> - *
> - *  Cleanup:
> - *   Print errno log and/or timing stats if options given
> +/*\
> + * [Description]
>   *
> - * Usage:  <for command-line>
> - *  mremap03 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
> - *     where,  -c n : Run n copies concurrently.
> - *             -e   : Turn on errno logging.
> - *	       -i n : Execute test n times.
> - *	       -I x : Execute test for x seconds.
> - *	       -p x : Pause for x seconds between iterations.
> - *	       -t   : Turn on syscall timing.
> + * Test for EFAULT error.
>   *
> - * HISTORY
> - *	07/2001 Ported by Wayne Boyer
> - *
> - *      11/09/2001 Manoj Iyer (manjo@austin.ibm.com)
> - *      Modified.
> - *      - #include <linux/mman.h> should not be included as per man page
> for - *        mremap, #include <sys/mman.h> alone should do the job. But
> inorder - *        to include definition of MREMAP_MAYMOVE defined in
> bits/mman.h - *        (included by sys/mman.h) __USE_GNU needs to be
> defined. - *        There may be a more elegant way of doing this...
> - *
> - *
> - * RESTRICTIONS:
> - *  None.
> + * - mremap fail with the old address specified was not mapped.
>   */
> -#define _GNU_SOURCE
> -#include <errno.h>
> -#include <unistd.h>
> -#include <fcntl.h>
> -#include <sys/mman.h>
> 
> -#include "test.h"
> +#define _GNU_SOURCE
> +#include "tst_test.h"
> 
> -char *TCID = "mremap03";
> -int TST_TOTAL = 1;
>  static char *bad_addr;
> -static char *addr;		/* addr of memory mapped region */
> -int memsize;			/* memory mapped size */
> -int newsize;			/* new size of virtual memory block */
> +static char *addr;
> +static int memsize;
> +static int newsize;
> 
> -void setup();			/* Main setup function of test */
> -void cleanup();			/* cleanup function for the test */
> -
> -int main(int ac, char **av)
> +static void verify_mremap(void)
>  {
> -	int lc;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> +	errno = 0;
> +	addr = mremap(bad_addr, memsize, newsize, MREMAP_MAYMOVE);
> +	TST_ERR = errno;
We can simplify by using TESTPTR() macro.
> 
> -		/*
> -		 * Attempt to expand the existing mapped
> -		 * memory region (memsize) by newsize limits
> -		 * using mremap() should fail as specified old
> -		 * virtual address was not mapped.
> -		 */
> -		errno = 0;
> -		addr = mremap(bad_addr, memsize, newsize, MREMAP_MAYMOVE);
> -		TEST_ERRNO = errno;
> -
> -		/* Check for the return value of mremap() */
> -		if (addr != MAP_FAILED) {
> -			tst_resm(TFAIL,
> -				 "mremap returned invalid value, expected: -1");
> -
> -			/* Unmap the mapped memory region */
> -			if (munmap(addr, newsize) != 0) {
> -				tst_brkm(TFAIL, cleanup, "munmap fails to "
> -					 "unmap the expanded memory region, "
> -					 " error=%d", errno);
> -			}
> -			continue;
> -		}
> -
> -		/* Check for the expected errno */
> -		if (errno == EFAULT) {
> -			tst_resm(TPASS, "mremap() Fails, 'old region not "
> -				 "mapped', errno %d", TEST_ERRNO);
> -		} else {
> -			tst_resm(TFAIL, "mremap() Fails, "
> -				 "'Unexpected errno %d", TEST_ERRNO);
> -		}
> +	if (addr != MAP_FAILED) {
> +		tst_res(TFAIL | TTERRNO,
> +			"mremap returned invalid value, expected: -1");
>  	}
> 
> -	cleanup();
> -	tst_exit();
> -
> +	if (errno == EFAULT) {
> +		tst_res(TPASS | TTERRNO, "mremap() Failed, 'old region not "
> +				"mapped' - errno %d", TST_ERR);
> +	} else {
> +		tst_res(TFAIL | TTERRNO, "mremap() Failed, "
> +				"'Unexpected errno %d", TST_ERR);
> +	}
>  }
> 
> -/*
> - * setup() - performs all ONE TIME setup for this test.
> - *
> - * Get system page size.
> - * Set the old address point some high address which is not mapped.
> - */
>  void setup(void)
missing static
>  {
> -	int page_sz;		/* system page size */
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> +	int page_sz;
> 
> -	/* Get the system page size */
> -	if ((page_sz = getpagesize()) < 0) {
> -		tst_brkm(TFAIL, NULL,
> -			 "getpagesize() fails to get system page size");
> -	}
> -
> -	/* Get the size of virtual memory area to be mapped */
> +	page_sz = SAFE_SYSCONF(_SC_PAGESIZE);
>  	memsize = (1000 * page_sz);
> -
> -	/* Get the New size of virtual memory block after resize */
>  	newsize = (memsize * 2);
> -
> -	/*
> -	 * Set the old virtual address point to some address
> -	 * which is not mapped.
> -	 */
> -	bad_addr = tst_get_bad_addr(cleanup);
> +	bad_addr = tst_get_bad_addr(NULL);
This would not count as a not-mapped address becasue tst_get_bad_addr()
creates a mapping with
bad_addr =  mmap(0, 1, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
which maps atleast one page.

But mremap() is still failing because we are passing a different address
range with old_size value as
memsize = (1000 * page_sz);

If we simply use "memsize = page_sz" here mremap() doesn't fail.

So I think, to use and address which is not mapped in the process's address
space, we can do a SAFE_MMAP() immediately followed by SAFE_MUNMAP() and
use that address for mremap() call.
>  }
> 
> -/*
> - * cleanup() - performs all ONE TIME cleanup for this test at
> - *             completion or premature exit.
> - */
> -void cleanup(void)
> +static void cleanup(void)
>  {
> -
> -	/* Exit the program */
> -
> +	if (addr != MAP_FAILED)
> +		SAFE_MUNMAP(addr, newsize);
>  }
> +
> +static struct tst_test test = {
> +	.test_all = verify_mremap,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};

Regards,
Avinesh




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