[LTP] [PATCH 7/7] syscalls: mremap05: Convert to new API
Andrea Cervesato
andrea.cervesato@suse.com
Mon Jul 14 14:59:44 CEST 2025
Hi!
On 7/8/25 10:12 PM, Ricardo B. Marlière via ltp wrote:
> From: Ricardo B. Marlière <rbm@suse.com>
>
> Signed-off-by: Ricardo B. Marlière <rbm@suse.com>
> ---
> testcases/kernel/syscalls/mremap/mremap05.c | 270 ++++++++++++----------------
> 1 file changed, 119 insertions(+), 151 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/mremap/mremap05.c b/testcases/kernel/syscalls/mremap/mremap05.c
> index d85ebb068dc42f3fa3856bba8614aeea0caf90ab..832bc305bb4f7dc3164186241ce0bd0ae202826a 100644
> --- a/testcases/kernel/syscalls/mremap/mremap05.c
> +++ b/testcases/kernel/syscalls/mremap/mremap05.c
> @@ -1,190 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> /*
> * Copyright (C) 2012 Linux Test Project, 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) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
> */
> -/*
> - * Test Name: mremap05
> +
> +/*\
> + * Verify that mremap():
> *
> - * Test Description:
> - * Verify that MREMAP_FIXED fails without MREMAP_MAYMOVE.
> - * Verify that MREMAP_FIXED|MREMAP_MAYMOVE fails if target address
> - * is not page aligned.
> - * Verify that MREMAP_FIXED|MREMAP_MAYMOVE fails if old range
> - * overlaps with new range.
> - * Verify that MREMAP_FIXED|MREMAP_MAYMOVE can move mapping to new address.
> - * Verify that MREMAP_FIXED|MREMAP_MAYMOVE unmaps previous mapping
> - * at the address range specified by new_address and new_size.
> + * - Fails with EINVAL, when using MREMAP_FIXED flag without MREMAP_MAYMOVE
> + * - Fails with EINVAL, when using MREMAP_FIXED|MREMAP_MAYMOVE flags with
> + * target address not page aligned.
> + * - Fails with EINVAL, when using MREMAP_FIXED|MREMAP_MAYMOVE flags with old
> + * range overlapping the new range.
Here we are mixing different tests. We need two files: one for mremap()
failures and one for mremap() success. And for the first case, we have
many other cases to test.
EINVAL An invalid argument was given. Possible causes are:
• old_address was not page aligned;
• a value other than MREMAP_MAYMOVE or MREMAP_FIXED or
MREMAP_DONTUNMAP was specified in flags;
• new_size was zero;
• new_size or new_address was invalid;
• the new address range specified by new_address and
new_size overlapped the old address range specified by
old_address and old_size;
• MREMAP_FIXED or MREMAP_DONTUNMAP was specified without
also specifying MREMAP_MAYMOVE;
• MREMAP_DONTUNMAP was specified, but one or more pages
in the range specified by old_address and old_size were
not private anonymous;
• MREMAP_DONTUNMAP was specified and old_size was not
equal to new_size;
• old_size was zero and old_address does not refer to a
shareable mapping (but see BUGS);
• old_size was zero and the MREMAP_MAYMOVE flag was not
specified.
And we should eventually check if it's possible to trigger also all the
others.
> + * - Succeeds, when moving a mapping to new address using
> + * MREMAP_FIXED|MREMAP_MAYMOVE flags.
> + * - Succeeds, when unmapping previous mapping at the address range specified
> + * by new_address and new_size using MREMAP_FIXED|MREMAP_MAYMOVE flags.
> */
>
> #define _GNU_SOURCE
> -#include "config.h"
> -#include <sys/mman.h>
> -#include <errno.h>
> -#include <unistd.h>
> -#include "test.h"
> -#include "safe_macros.h"
>
> -char *TCID = "mremap05";
> +#include "tst_test.h"
> +
> +static void setup(void);
> +static void setup0(unsigned int i);
> +static void setup1(unsigned int i);
> +static void setup2(unsigned int i);
> +static void setup3(unsigned int i);
> +static void setup4(unsigned int i);
> +static void cleanup0(unsigned int i);
> +static void cleanup1(unsigned int i);
I would use different names for setup and cleanup, in a way that we know
what they are doing. The numeric increment approach is a bit confusing
at the moment.
For example, it would make more sense to have "setup_not_aligned",
"setup_overlap", etc.
>
> -struct test_case_t {
> +static int pagesize;
> +static struct test_case_t {
> char *old_address;
> char *new_address;
> - size_t old_size; /* in pages */
> - size_t new_size; /* in pages */
> + size_t old_size; /* in pages */
> + size_t new_size; /* in pages */
> int flags;
> const char *msg;
> void *exp_ret;
> int exp_errno;
> char *ret;
> - void (*setup) (struct test_case_t *);
> - void (*cleanup) (struct test_case_t *);
> -};
> -
> -static void setup(void);
> -static void cleanup(void);
> -static void setup0(struct test_case_t *);
> -static void setup1(struct test_case_t *);
> -static void setup2(struct test_case_t *);
> -static void setup3(struct test_case_t *);
> -static void setup4(struct test_case_t *);
> -static void cleanup0(struct test_case_t *);
> -static void cleanup1(struct test_case_t *);
> -
> -struct test_case_t tdat[] = {
> + void (*setup)(unsigned int i);
> + void (*cleanup)(unsigned int i);
> +} tcases[] = {
> {
> - .old_size = 1,
> - .new_size = 1,
> - .flags = MREMAP_FIXED,
> - .msg = "MREMAP_FIXED requires MREMAP_MAYMOVE",
> - .exp_ret = MAP_FAILED,
> - .exp_errno = EINVAL,
> - .setup = setup0,
> - .cleanup = cleanup0},
> + .old_size = 1,
> + .new_size = 1,
> + .flags = MREMAP_FIXED,
> + .msg = "MREMAP_FIXED requires MREMAP_MAYMOVE",
> + .exp_ret = MAP_FAILED,
> + .exp_errno = EINVAL,
> + .setup = setup0,
> + .cleanup = cleanup0,
> + },
> {
> - .old_size = 1,
> - .new_size = 1,
> - .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> - .msg = "new_addr has to be page aligned",
> - .exp_ret = MAP_FAILED,
> - .exp_errno = EINVAL,
> - .setup = setup1,
> - .cleanup = cleanup0},
> + .old_size = 1,
> + .new_size = 1,
> + .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> + .msg = "new_addr has to be page aligned",
> + .exp_ret = MAP_FAILED,
> + .exp_errno = EINVAL,
> + .setup = setup1,
> + .cleanup = cleanup0,
> + },
> {
> - .old_size = 2,
> - .new_size = 1,
> - .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> - .msg = "old/new area must not overlap",
> - .exp_ret = MAP_FAILED,
> - .exp_errno = EINVAL,
> - .setup = setup2,
> - .cleanup = cleanup0},
> + .old_size = 2,
> + .new_size = 1,
> + .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> + .msg = "old/new area must not overlap",
> + .exp_ret = MAP_FAILED,
> + .exp_errno = EINVAL,
> + .setup = setup2,
> + .cleanup = cleanup0,
> + },
> {
> - .old_size = 1,
> - .new_size = 1,
> - .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> - .msg = "mremap #1",
> - .setup = setup3,
> - .cleanup = cleanup0},
> + .old_size = 1,
> + .new_size = 1,
> + .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> + .msg = "mremap #1",
> + .setup = setup3,
> + .cleanup = cleanup0,
> + },
> {
> - .old_size = 1,
> - .new_size = 1,
> - .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> - .msg = "mremap #2",
> - .setup = setup4,
> - .cleanup = cleanup1},
> + .old_size = 1,
> + .new_size = 1,
> + .flags = MREMAP_FIXED | MREMAP_MAYMOVE,
> + .msg = "mremap #2",
> + .setup = setup4,
> + .cleanup = cleanup1,
> + },
> };
>
> -static int pagesize;
> -static int TST_TOTAL = sizeof(tdat) / sizeof(tdat[0]);
> -
> static void free_test_area(void *p, int size)
> {
> - SAFE_MUNMAP(cleanup, p, size);
> + SAFE_MUNMAP(p, size);
> }
>
> static void *get_test_area(int size, int free_area)
> {
> - void *p;
> - p = mmap(NULL, size, PROT_READ | PROT_WRITE,
> - MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> - if (p == MAP_FAILED)
> - tst_brkm(TBROK | TERRNO, cleanup, "get_test_area mmap");
> + void *p = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
> if (free_area)
> free_test_area(p, size);
> return p;
> }
This approach is too cryptic. We need to get rid of this free_area flag
and to find a more readable way to provide an address for destination.
>
> -static void test_mremap(struct test_case_t *t)
> +static void setup0(unsigned int i)
> {
> - t->ret = mremap(t->old_address, t->old_size, t->new_size, t->flags,
> - t->new_address);
> -
> - if (t->ret == t->exp_ret) {
> - if (t->ret != MAP_FAILED) {
> - tst_resm(TPASS, "%s", t->msg);
> - if (*(t->ret) == 0x1)
> - tst_resm(TPASS, "%s value OK", t->msg);
> - else
> - tst_resm(TPASS, "%s value failed", t->msg);
> - } else {
> - if (errno == t->exp_errno)
> - tst_resm(TPASS, "%s", t->msg);
> - else
> - tst_resm(TFAIL | TERRNO, "%s", t->msg);
> - }
> - } else {
> - tst_resm(TFAIL, "%s ret: %p, expected: %p", t->msg,
> - t->ret, t->exp_ret);
> - }
> -}
> + struct test_case_t *t = &tcases[i];
>
> -static void setup0(struct test_case_t *t)
> -{
> t->old_address = get_test_area(t->old_size * pagesize, 0);
> t->new_address = get_test_area(t->new_size * pagesize, 1);
> }
>
> -static void setup1(struct test_case_t *t)
> +static void setup1(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> t->old_address = get_test_area(t->old_size * pagesize, 0);
> t->new_address = get_test_area((t->new_size + 1) * pagesize, 1) + 1;
Why not just setting new_size to 2 ? Perhaps even in this case we would
be page aligned anyway.
What really makes the difference is "+ 1" at the end.
Perhaps, once we reach cleanup() function we always de-allocate a memory
that is "t->new_size * pagesize" and that is wrong, since we are adding
1 more value to new_size during setup. The only reason why we are not
facing any error, is that get_test_area() is releasing memory already.
> }
>
> -static void setup2(struct test_case_t *t)
> +static void setup2(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> t->old_address = get_test_area(t->old_size * pagesize, 0);
> t->new_address = t->old_address;
> }
>
> -static void setup3(struct test_case_t *t)
> +static void setup3(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> t->old_address = get_test_area(t->old_size * pagesize, 0);
> t->new_address = get_test_area(t->new_size * pagesize, 1);
> t->exp_ret = t->new_address;
> *(t->old_address) = 0x1;
> }
>
> -static void setup4(struct test_case_t *t)
> +static void setup4(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> t->old_address = get_test_area(t->old_size * pagesize, 0);
> t->new_address = get_test_area(t->new_size * pagesize, 0);
> t->exp_ret = t->new_address;
> @@ -192,16 +152,20 @@ static void setup4(struct test_case_t *t)
> *(t->new_address) = 0x2;
> }
>
> -static void cleanup0(struct test_case_t *t)
> +static void cleanup0(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> if (t->ret == MAP_FAILED)
> free_test_area(t->old_address, t->old_size * pagesize);
> else
> free_test_area(t->ret, t->new_size * pagesize);
> }
>
> -static void cleanup1(struct test_case_t *t)
> +static void cleanup1(unsigned int i)
> {
> + struct test_case_t *t = &tcases[i];
> +
> if (t->ret == MAP_FAILED) {
> free_test_area(t->old_address, t->old_size * pagesize);
> free_test_area(t->new_address, t->new_size * pagesize);
> @@ -210,30 +174,34 @@ static void cleanup1(struct test_case_t *t)
> }
> }
>
> -int main(int ac, char **av)
> +static void run(unsigned int i)
> {
> - int lc, testno;
> -
> - tst_parse_opts(ac, av, NULL, NULL);
> -
> - setup();
> - for (lc = 0; TEST_LOOPING(lc); lc++) {
> - tst_count = 0;
> - for (testno = 0; testno < TST_TOTAL; testno++) {
> - tdat[testno].setup(&tdat[testno]);
> - test_mremap(&tdat[testno]);
> - tdat[testno].cleanup(&tdat[testno]);
> - }
> - }
> - cleanup();
> - tst_exit();
> + struct test_case_t *t = &tcases[i];
> +
> + if (t->setup)
> + t->setup(i);
> +
> + if (t->exp_errno)
> + TST_EXP_FAIL_PTR_VOID(mremap(t->old_address, t->old_size,
> + t->new_size, t->flags,
> + t->new_address),
> + t->exp_errno, "%s", t->msg);
> + else
> + TST_EXP_PASS_PTR_VOID(mremap(t->old_address, t->old_size,
> + t->new_size, t->flags,
> + t->new_address));
Missing parenthesis in both statements.
> +
> + if (t->cleanup)
> + t->cleanup(i);
> }
>
> static void setup(void)
> {
> - pagesize = getpagesize();
> + pagesize = SAFE_SYSCONF(_SC_PAGESIZE);
> }
>
> -static void cleanup(void)
> -{
> -}
> +static struct tst_test test = {
> + .test = run,
> + .tcnt = ARRAY_SIZE(tcases),
> + .setup = setup,
> +};
>
In general, this test has to be probably rewritten, since we are mixing
working and failing scenarios, with a code that is difficult to read. If
I can give a suggestion, to refactor tests is not only about making a
1:1 from old to new API, blindly accepting what was written before, but
especially to simplify the old code, deeply understand it and make it
more easy to read. This test is not bringing much new at the moment
compared to the old version.
- Andrea
More information about the ltp
mailing list