[LTP] [PATCH v1] Test madvise with advise value'MADV_WIPEONFORK'
kewal
kewal@zilogic.com
Fri Jul 13 08:27:25 CEST 2018
Hi Jan,
As per your below comment,
> -drop madvise11, errno tests can be added to existing madvise02
we found that in madvise02, the below structure uses mapped address which
is mapped with the flag value as "MAP_SHARED" but as per my test cases we
have to vary the flag value with "MAP_SHARED | MAP_ANONYMOUS" &
"MAP_PRIVATE".
I didn't found a way to add our testcases in madvise02.
>static struct tcase {
> int advice;
> char *name;
> char **addr;
> int exp_errno;
> int skip;
>} tcases[] = {
> {MADV_NORMAL, "MADV_NORMAL", &nonalign, EINVAL, 0},
> {1212, "MADV_NORMAL", &file1, EINVAL, 0},
> {MADV_REMOVE, "MADV_REMOVE", &file1, EINVAL, 0},
> {MADV_DONTNEED, "MADV_DONTNEED", &file1, EINVAL, 1},
> {MADV_MERGEABLE, "MADV_MERGEABLE", &file1, EINVAL, 0},
> {MADV_UNMERGEABLE, "MADV_UNMERGEABLE", &file1, EINVAL, 0},
> {MADV_NORMAL, "MADV_NORMAL", &file2, ENOMEM, 0},
> {MADV_WILLNEED, "MADV_WILLNEED", &file2, ENOMEM, 0},
> {MADV_WILLNEED, "MADV_WILLNEED", &tmp_addr, EBADF, 0},
> {MADV_FREE, "MADV_FREE", &file1, EINVAL, 0},
> {MADV_WIPEONFORK, "MADV_WIPEONFORK", &file1, EINVAL, 0},
>};
> file1 = SAFE_MMAP(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
> file2 = SAFE_MMAP(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
We will go through the rest of the comments and update with v2.
Regards,
Kewal Ukunde
> On 07/11/2018 05:59 PM, kewal wrote:
>>
>> * madvise10.c :- Present the child process with zero-filled memory in
>> this range after a fork(2).
>>
>> Test-Case 1 : madvise with 'MADV_WIPEONFORK'
>> Test-Case 2 : madvise with 'MADV_WIPEONFORK' as size 'ZERO'
>>
>> * madvise11.c:- The MADV_WIPEONFORK operation can be applied only to
>> private anonymous pages.
>>
>> Test-Case 1 : mmap with 'MAP_SHARED | MAP_ANONYMOUS'
>> Test-Case 2 : mmap with 'MAP_PRIVATE'
>>
>> * madvise12.c:- Within the child created by fork(2), the MADV_WIPEONFORK
>> setting remains in place on the specified address range.
>>
>> Test-Case 1: 'MADV_WIPEONFORK' on Grand child
>>
>> * madvise13.c:- MADV_KEEPONFORK Undo the effect of an earlier
>> MADV_WIPEONFORK
>>
>> Test-Case 1 : Undo 'MADV_WIPEONFORK' by 'MADV_KEEPONFORK'
>
> Hi,
>
> These tests seem to have lot of common code. We should simplify
> that by a header/include or combining some of them together.
>
> I'm thinking:
> - drop madvise11, errno tests can be added to existing madvise02
> - combine the rest together, see my attached example
>
> Other comments are below/inline:
>
>>
>> Signed-off-by: Subash Ganesan <subash@zilogic.com>
>> Signed-off-by: Kewal Ukunde <kewal@zilogic.com>
>> ---
>> runtest/syscalls | 4 +
>
> New tests should be added also to respective .gitignore
>
>> testcases/kernel/syscalls/madvise/madvise10.c | 179
>> ++++++++++++++++++++++++++
>> testcases/kernel/syscalls/madvise/madvise11.c | 132 +++++++++++++++++++
>> testcases/kernel/syscalls/madvise/madvise12.c | 171
>> ++++++++++++++++++++++++
>> testcases/kernel/syscalls/madvise/madvise13.c | 162
>> +++++++++++++++++++++++
>> 5 files changed, 648 insertions(+)
>> create mode 100644 testcases/kernel/syscalls/madvise/madvise10.c
>> create mode 100644 testcases/kernel/syscalls/madvise/madvise11.c
>> create mode 100644 testcases/kernel/syscalls/madvise/madvise12.c
>> create mode 100644 testcases/kernel/syscalls/madvise/madvise13.c
>>
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index a9afecf57..a95c47c3a 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -773,6 +773,10 @@ madvise06 madvise06
>> madvise07 madvise07
>> madvise08 madvise08
>> madvise09 madvise09
>> +madvise10 madvise10
>> +madvise11 madvise11
>> +madvise12 madvise12
>> +madvise13 madvise13
>>
>> newuname01 newuname01
>>
>> diff --git a/testcases/kernel/syscalls/madvise/madvise10.c
>> b/testcases/kernel/syscalls/madvise/madvise10.c
>> new file mode 100644
>> index 000000000..1cf3097b0
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/madvise/madvise10.c
>> @@ -0,0 +1,179 @@
>> +// SPDX-License-Identifier: GPL-2.0 or later
>> +/*
>> + * Copyright (c) Zilogic Systems Pvt. Ltd., 2007
>
> 2018
>
>> + * Email : code@zilogic.com
>> + *
>> + * 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.
>> + */
>> +
>> +/*
>> + * test cases for madvise(2) system call, advise value as
>> "MADV_WIPEONFORK".
>> + *
>> + * DESCRIPTION :
>> + * Present the child process with zero-filled memory in this
>> + * range after a fork(2).
>> + * The MADV_WIPEONFORK operation can be applied only to
>> + * private anonymous pages.
>> + *
>> + * Test-Case 1 : madvise with "MADV_WIPEONFORK"
>> + * flow : Map memory area as private anonymous page.
>> + * Mark memory area as wipe-on-fork.
>> + * On fork, child process memory should be zeroed.
>> + *
>> + * Test-Case 2 : madvise with "MADV_WIPEONFORK" as size "ZERO"
>> + * flow : Map memory area as private anonymous page.
>> + * Mark memory area as wipe-on-fork, with length zero.
>> + * On fork, child process memory should be accessible.
>> + **/
>> +
>> +#include <stdio.h>
>> +#include <signal.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <stdlib.h>
>> +#include <fcntl.h>
>> +#include <sys/mman.h>
>> +#include <error.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/wait.h>
>> +#include <stdlib.h>
>> +
>> +#include "lapi/mmap.h"
>> +#include "tst_test.h"
>> +#include "tst_safe_macros.h"
>> +
>> +#define TEST_FILE "test_file.txt"
>
> Why do you need a file when you're mapping as ANONYMOUS?
>
>> +#define MAP_SIZE (4 * 1024)
>> +
>> +static int test_file_fd;
>> +
>> +static const struct test_case {
>> + int size;
>> + int advise;
>> + char *advise_name;
>> + int expect;
>> +} tcases[] = {
>> + {.size = MAP_SIZE,
>> + .advise = MADV_WIPEONFORK,
>> + .advise_name = "MADV_WIPEONFORK",
>> + .expect = 0},
>> +
>> + {.size = 0,
>> + .advise = MADV_WIPEONFORK,
>> + .advise_name = "MADV_WIPEONFORK",
>> + .expect = 1}
>> +};
>> +
>> +static void assert_equal(int status, const struct test_case *tc)
>> +{
>> + if (status == tc->expect)
>> + tst_res(TPASS, "In child data %d : madvise(%s)",
>> + tc->expect, tc->advise_name);
>> + else
>> + tst_res(TFAIL, "In child data %d : madvise(%s)",
>> + tc->expect, tc->advise_name);
>> +}
>
> Let's compare the whole area, not just first byte.
>
>> +
>> +static void safe_wait_status(int *status)
>> +{
>> + SAFE_WAIT(status);
>> +
>> + if (!WIFEXITED(*status)) {
>> + if (WTERMSIG(*status) == SIGSEGV)
>> + tst_res(TFAIL, "Invalid access : SIGSEGV");
>> + else
>> + tst_res(TFAIL, "Abnormal termination");
>> + } else {
>> + *status = WEXITSTATUS(*status);
>> + }
>> +}
>
> You can use tst_res(TPASS/TFAIL,...) in child processes. Then you
> can avoid passing values over exit status and use tst_reap_children(),
> which does the same check.
>
>> +
>> +static void spawn_child(char *map_addr)
>> +{
>> + pid_t pid;
>> +
>> + pid = SAFE_FORK();
>> +
>> + if (!pid)
>> + exit(*map_addr);
>
> As mentioned earlier, let's check entire area.
>
>> +}
>> +
>> +static int set_advice(char *map_addr, const struct test_case *tc)
>> +{
>> + TEST(madvise(map_addr, tc->size, tc->advise));
>> +
>> + if (TEST_RETURN == -1) {
>> + tst_res(TINFO, "failed :madvise(%p, %d, %s)",
>> + map_addr, tc->size, tc->advise_name);
>
> Why not TFAIL|TTERRNO?
>
>> + return 1;
>> + }
>> +
>> + tst_res(TINFO, "success :madvise(%p, %d, %s)",
>> + map_addr, tc->size, tc->advise_name);
>> +
>> + return 0;
>> +}
>> +
>> +static void mem_map(char **map_addr)
>> +{
>> + *map_addr = SAFE_MMAP(NULL, MAP_SIZE,
>> + PROT_READ | PROT_WRITE,
>> + MAP_PRIVATE | MAP_ANON,
>
> MAP_ANON is deprecated, please use MAP_ANONYMOUS.
>
>> + test_file_fd, 0);
>
> What is the purpose of having this file?
>
>> +
>> + tst_res(TINFO, "mmap(NULL, %d, PROT_READ | PROT_WRITE, %s, %d, 0)",
>> + MAP_SIZE, "MAP_PRIVATE | MAP_ANON", test_file_fd);
>> +}
>> +
>> +static void test_madvise(unsigned int test_nr)
>> +{
>> + const struct test_case *tc = &tcases[test_nr];
>> + char *map_addr;
>> + int status;
>> +
>> + mem_map(&map_addr);
>> + memset(map_addr, 1, MAP_SIZE);
>> +
>> + if (set_advice(map_addr, tc)) {
>> + tst_res(TFAIL, "madvise failed");
>
> Please, print also errno code.
>
>> + } else {
>> + spawn_child(map_addr);
>> + safe_wait_status(&status);
>> + assert_equal(status, tc);
>> + }
>> +
>> + SAFE_MUNMAP(map_addr, MAP_SIZE);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + SAFE_CLOSE(test_file_fd);
>> + SAFE_UNLINK(TEST_FILE);
>> +}
>> +
>> +static void setup(void)
>> +{
>> + test_file_fd = SAFE_OPEN(TEST_FILE, O_RDWR | O_CREAT, 0777);
>> +}
>> +
>> +static struct tst_test test = {
>> + .tcnt = ARRAY_SIZE(tcases),
>> + .needs_root = 1,
>
> Is root really needed?
>
>> + .forks_child = 1,
>> + .needs_tmpdir = 1,
>> + .test = test_madvise,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .min_kver = "4.14",
>> +};
>
> Similar comments apply to other tests, which share code above.
>
> Regards,
> Jan
>
>
More information about the ltp
mailing list