| Message ID | 20230323121330.92244-1-paulson@zilogic.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Added test for mmap() with MAP_SHARED_VALIDATE. | expand |
Hi paulson, Thanks for your patch. Generally the idea LGTM, also the implementation, but I suggest various fixes. There are many fixes (whole diff is below), I put them into my github fork, if you agree I can merge it as is. https://github.com/pevik/ltp/commits/paulson/mmap.fixes https://github.com/pevik/ltp/blob/36ebc4d8900ea8438dc7839f03b7921f0ed9243a/testcases/kernel/syscalls/mmap/mmap20.c > From: paulson <lpaulsonraja@gmail.com> Maybe you want to setup your git better to have your name and surname. Here is missing in the git commit message (or with fixed name): Signed-off-by: paulson <lpaulsonraja@gmail.com> CI shows failures on CentOS 7: https://github.com/pevik/ltp/actions/runs/4509751194/jobs/7939880067 /__w/ltp/ltp/testcases/kernel/syscalls/mmap/mmap20.c:48:25: error: 'MAP_SHARED_VALIDATE' undeclared (first use in this function) INVALID_FLAG | MAP_SHARED_VALIDATE, fd, 0); /__w/ltp/ltp/testcases/kernel/syscalls/mmap/mmap20.c:48:25: note: each undeclared identifier is reported only once for each function it appears in There needs to be change in lapi file (which will be used later): https://patchwork.ozlabs.org/project/ltp/patch/20230324101630.562727-1-pvorel@suse.cz/ You're missing record in runtest/syscalls mmap20 mmap20 and in .gitignore. Our checker prints many errors cd testcases/kernel/syscalls/mmap; make check-mmap20 mmap20.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 make: [../../../../include/mk/rules.mk:56: check-mmap20] Error 1 (ignored) mmap20.c: note: in included file (through /usr/include/asm-generic/mman.h, /usr/include/asm/mman.h, /usr/include/linux/mman.h): /usr/include/asm-generic/mman-common.h:26:9: warning: preprocessor token MAP_POPULATE redefined mmap20.c: note: in included file (through /usr/include/bits/mman.h, /usr/include/sys/mman.h): /usr/include/bits/mman-map-flags-generic.h:34:10: this was the original definition ... will be described later > --- > testcases/kernel/syscalls/mmap/mmap20.c | 61 +++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100644 testcases/kernel/syscalls/mmap/mmap20.c > diff --git a/testcases/kernel/syscalls/mmap/mmap20.c b/testcases/kernel/syscalls/mmap/mmap20.c > new file mode 100644 > index 000000000..ca5bfccd7 > --- /dev/null > +++ b/testcases/kernel/syscalls/mmap/mmap20.c > @@ -0,0 +1,61 @@ > +//SPDX-License-Identifier: GPL-2.0-or-later Missing space will cause invalid license for tools. Found by: mmap20.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 > + > +/* > + * Test mmap with MAP_SHARED_VALIDATE flag > + * > + * We are testing the MAP_SHARED_VALIDATE flag of mmap() syscall. To check > + * if there is an invalid flag value, the MAP_SHARED_VALIDATE return > + * EOPNOTSUPP. The unused bit in the MAP_SHARED_VALIDATE is found, and by > + * setting the unused bits of the flag argument the flag value becomes > + * invalid and the error EOPNOTSUPP is produced as expected. > + */ I'd phrase it as: /*\ * [Description] * * Test mmap(2) with MAP_SHARED_VALIDATE flag. * * Test expected EOPNOTSUPP errno when testing mmap(2) with MAP_SHARED_VALIDATE * flag and invalid flag. */ > +#include <stdio.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <fcntl.h> Some > +#include <sys/mman.h> > +#include <linux/mman.h> 1) mixing <linux/mman.h> and <sys/mman.h> does not work well: mmap20.c: note: in included file (through /usr/include/asm-generic/mman.h, /usr/include/asm/mman.h, /usr/include/linux/mman.h): /usr/include/asm-generic/mman-common.h:26:9: warning: preprocessor token MAP_POPULATE redefined mmap20.c: note: in included file (through /usr/include/bits/mman.h, /usr/include/sys/mman.h): /usr/include/bits/mman-map-flags-generic.h:34:10: this was the original definition I pushed fix https://github.com/linux-test-project/ltp/commit/32aa5c30c257b2021f9648df186d5b2c7a57dfad + with my patch https://patchwork.ozlabs.org/project/ltp/patch/20230324101630.562727-1-pvorel@suse.cz/ you can instead of > +#include <errno.h> > +#include "tst_test.h" > + > +#define TEST_FILE "file_to_mmap" > +#define TEST_FILE_SIZE 1024 > +#define TEST_FILE_MODE 0600 File mod is used just once, it's not really important to have it defined. But I'd define (1 << 7), because that is expected to be invalid (one day this value can appear in <linux/mmap.h> and the test will fail. > + > +static int fd_file = -1; > +static void *mapped_address; > + > +static void setup(void) > +{ > + fd_file = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, TEST_FILE_MODE); > + if (tst_fill_file(TEST_FILE, 'a', TEST_FILE_SIZE, 1)) > + tst_brk(TBROK, "Could not fill the testfile."); nit: we don't use dots in the end. Whole diff is here: diff --git include/lapi/mmap.h include/lapi/mmap.h index 48795369d..7512e9f81 100644 --- include/lapi/mmap.h +++ include/lapi/mmap.h @@ -10,6 +10,10 @@ #include "config.h" #include <sys/mman.h> +#ifndef MAP_SHARED_VALIDATE +# define MAP_SHARED_VALIDATE 0x03 +#endif + #ifndef MAP_HUGETLB # define MAP_HUGETLB 0x40000 #endif diff --git runtest/syscalls runtest/syscalls index b9d4a43c8..8b002e989 100644 --- runtest/syscalls +++ runtest/syscalls @@ -794,6 +794,7 @@ mmap16 mmap16 mmap17 mmap17 mmap18 mmap18 mmap19 mmap19 +mmap20 mmap20 modify_ldt01 modify_ldt01 modify_ldt02 modify_ldt02 diff --git testcases/kernel/syscalls/mmap/.gitignore testcases/kernel/syscalls/mmap/.gitignore index 8811226be..569a76ac1 100644 --- testcases/kernel/syscalls/mmap/.gitignore +++ testcases/kernel/syscalls/mmap/.gitignore @@ -18,3 +18,4 @@ /mmap17 /mmap18 /mmap19 +/mmap20 diff --git testcases/kernel/syscalls/mmap/mmap20.c testcases/kernel/syscalls/mmap/mmap20.c index ca5bfccd7..54ddb2087 100644 --- testcases/kernel/syscalls/mmap/mmap20.c +++ testcases/kernel/syscalls/mmap/mmap20.c @@ -1,55 +1,58 @@ -//SPDX-License-Identifier: GPL-2.0-or-later - +// SPDX-License-Identifier: GPL-2.0-or-later /* - * Test mmap with MAP_SHARED_VALIDATE flag + * Copyright (c) 2023 paulson <lpaulsonraja@gmail.com> + */ + +/*\ + * [Description] * - * We are testing the MAP_SHARED_VALIDATE flag of mmap() syscall. To check - * if there is an invalid flag value, the MAP_SHARED_VALIDATE return - * EOPNOTSUPP. The unused bit in the MAP_SHARED_VALIDATE is found, and by - * setting the unused bits of the flag argument the flag value becomes - * invalid and the error EOPNOTSUPP is produced as expected. + * Test mmap(2) with MAP_SHARED_VALIDATE flag. + * + * Test expected EOPNOTSUPP errno when testing mmap(2) with MAP_SHARED_VALIDATE + * flag and invalid flag. */ + +#include <errno.h> #include <stdio.h> #include <sys/types.h> -#include <sys/stat.h> -#include <fcntl.h> -#include <sys/mman.h> -#include <linux/mman.h> -#include <errno.h> #include "tst_test.h" +#include "lapi/mmap.h" #define TEST_FILE "file_to_mmap" #define TEST_FILE_SIZE 1024 -#define TEST_FILE_MODE 0600 +#define INVALID_FLAG (1 << 7) -static int fd_file = -1; -static void *mapped_address; +static int fd = -1; +static void *addr; static void setup(void) { - fd_file = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, TEST_FILE_MODE); + fd = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, 0600); + if (tst_fill_file(TEST_FILE, 'a', TEST_FILE_SIZE, 1)) - tst_brk(TBROK, "Could not fill the testfile."); + tst_brk(TBROK, "Could not fill the testfile"); } static void cleanup(void) { - if (fd_file > -1) - SAFE_CLOSE(fd_file); - if (mapped_address != NULL && mapped_address != MAP_FAILED) - SAFE_MUNMAP(mapped_address, TEST_FILE_SIZE); + if (fd > -1) + SAFE_CLOSE(fd); + + if (addr && addr != MAP_FAILED) + SAFE_MUNMAP(addr, TEST_FILE_SIZE); } static void test_mmap(void) { - mapped_address = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, - (1 << 7) | MAP_SHARED_VALIDATE, fd_file, 0); - if (mapped_address != MAP_FAILED) - tst_res(TFAIL | TERRNO, "mmap() is successful, but it should have failed."); + addr = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, + INVALID_FLAG | MAP_SHARED_VALIDATE, fd, 0); + + if (addr != MAP_FAILED) + tst_res(TFAIL | TERRNO, "mmap() is successful, but it should have failed"); else if (errno == EOPNOTSUPP) - tst_res(TPASS, "mmap() failed with errno set to EOPNOTSUPP."); + tst_res(TPASS, "mmap() failed with errno set to EOPNOTSUPP"); else - tst_res(TFAIL | TERRNO, "mmap() failed with unexpected error."); + tst_res(TFAIL | TERRNO, "mmap() failed with unexpected error"); } static struct tst_test test = {
Hi Petr Vorel, Thanks for the comments, I have just started contributing to LTP, and this is my first patch. I agree to merge it as is. https://github.com/pevik/ltp/commits/paulson/mmap.fixes https://github.com/pevik/ltp/blob/36ebc4d8900ea8438dc7839f03b7921f0ed9243a/testcases/kernel/syscalls/mmap/mmap20.c On Fri, Mar 24, 2023 at 4:03 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi paulson, > > Thanks for your patch. Generally the idea LGTM, also the implementation, > but I suggest various fixes. > > There are many fixes (whole diff is below), I put them into my github > fork, if > you agree I can merge it as is. > > https://github.com/pevik/ltp/commits/paulson/mmap.fixes > > https://github.com/pevik/ltp/blob/36ebc4d8900ea8438dc7839f03b7921f0ed9243a/testcases/kernel/syscalls/mmap/mmap20.c > > > From: paulson <lpaulsonraja@gmail.com> > Maybe you want to setup your git better to have your name and surname. > > Here is missing in the git commit message (or with fixed name): > Signed-off-by: paulson <lpaulsonraja@gmail.com> > > CI shows failures on CentOS 7: > > https://github.com/pevik/ltp/actions/runs/4509751194/jobs/7939880067 > > /__w/ltp/ltp/testcases/kernel/syscalls/mmap/mmap20.c:48:25: error: > 'MAP_SHARED_VALIDATE' undeclared (first use in this function) > INVALID_FLAG | MAP_SHARED_VALIDATE, fd, 0); > /__w/ltp/ltp/testcases/kernel/syscalls/mmap/mmap20.c:48:25: note: each > undeclared identifier is reported only once for each function it appears in > > There needs to be change in lapi file (which will be used later): > > https://patchwork.ozlabs.org/project/ltp/patch/20230324101630.562727-1-pvorel@suse.cz/ > > You're missing record in runtest/syscalls > > mmap20 mmap20 > > and in .gitignore. > > Our checker prints many errors > cd testcases/kernel/syscalls/mmap; make check-mmap20 > > mmap20.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in > line 1 > make: [../../../../include/mk/rules.mk:56: check-mmap20] Error 1 (ignored) > mmap20.c: note: in included file (through /usr/include/asm-generic/mman.h, > /usr/include/asm/mman.h, /usr/include/linux/mman.h): > /usr/include/asm-generic/mman-common.h:26:9: warning: preprocessor token > MAP_POPULATE redefined > mmap20.c: note: in included file (through /usr/include/bits/mman.h, > /usr/include/sys/mman.h): > /usr/include/bits/mman-map-flags-generic.h:34:10: this was the original > definition > ... > > will be described later > > > --- > > testcases/kernel/syscalls/mmap/mmap20.c | 61 +++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > create mode 100644 testcases/kernel/syscalls/mmap/mmap20.c > > > diff --git a/testcases/kernel/syscalls/mmap/mmap20.c > b/testcases/kernel/syscalls/mmap/mmap20.c > > new file mode 100644 > > index 000000000..ca5bfccd7 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/mmap/mmap20.c > > @@ -0,0 +1,61 @@ > > +//SPDX-License-Identifier: GPL-2.0-or-later > Missing space will cause invalid license for tools. > > Found by: > mmap20.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in > line 1 > > > + > > +/* > > + * Test mmap with MAP_SHARED_VALIDATE flag > > + * > > + * We are testing the MAP_SHARED_VALIDATE flag of mmap() syscall. To > check > > + * if there is an invalid flag value, the MAP_SHARED_VALIDATE return > > + * EOPNOTSUPP. The unused bit in the MAP_SHARED_VALIDATE is found, and > by > > + * setting the unused bits of the flag argument the flag value becomes > > + * invalid and the error EOPNOTSUPP is produced as expected. > > + */ > I'd phrase it as: > > /*\ > * [Description] > * > * Test mmap(2) with MAP_SHARED_VALIDATE flag. > * > * Test expected EOPNOTSUPP errno when testing mmap(2) with > MAP_SHARED_VALIDATE > * flag and invalid flag. > */ > > > +#include <stdio.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <fcntl.h> > > Some > > > +#include <sys/mman.h> > > +#include <linux/mman.h> > > 1) mixing <linux/mman.h> and <sys/mman.h> does not work well: > > mmap20.c: note: in included file (through /usr/include/asm-generic/mman.h, > /usr/include/asm/mman.h, /usr/include/linux/mman.h): > /usr/include/asm-generic/mman-common.h:26:9: warning: preprocessor token > MAP_POPULATE redefined > mmap20.c: note: in included file (through /usr/include/bits/mman.h, > /usr/include/sys/mman.h): > /usr/include/bits/mman-map-flags-generic.h:34:10: this was the original > definition > > > I pushed fix > > https://github.com/linux-test-project/ltp/commit/32aa5c30c257b2021f9648df186d5b2c7a57dfad > > + with my patch > > https://patchwork.ozlabs.org/project/ltp/patch/20230324101630.562727-1-pvorel@suse.cz/ > > you can instead of > > > +#include <errno.h> > > +#include "tst_test.h" > > + > > +#define TEST_FILE "file_to_mmap" > > +#define TEST_FILE_SIZE 1024 > > +#define TEST_FILE_MODE 0600 > File mod is used just once, it's not really important to have it defined. > But I'd define (1 << 7), because that is expected to be invalid > (one day this value can appear in <linux/mmap.h> and the test will fail. > > > + > > +static int fd_file = -1; > > +static void *mapped_address; > > + > > +static void setup(void) > > +{ > > + fd_file = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, TEST_FILE_MODE); > > + if (tst_fill_file(TEST_FILE, 'a', TEST_FILE_SIZE, 1)) > > + tst_brk(TBROK, "Could not fill the testfile."); > nit: we don't use dots in the end. > > Whole diff is here: > > diff --git include/lapi/mmap.h include/lapi/mmap.h > index 48795369d..7512e9f81 100644 > --- include/lapi/mmap.h > +++ include/lapi/mmap.h > @@ -10,6 +10,10 @@ > #include "config.h" > #include <sys/mman.h> > > +#ifndef MAP_SHARED_VALIDATE > +# define MAP_SHARED_VALIDATE 0x03 > +#endif > + > #ifndef MAP_HUGETLB > # define MAP_HUGETLB 0x40000 > #endif > diff --git runtest/syscalls runtest/syscalls > index b9d4a43c8..8b002e989 100644 > --- runtest/syscalls > +++ runtest/syscalls > @@ -794,6 +794,7 @@ mmap16 mmap16 > mmap17 mmap17 > mmap18 mmap18 > mmap19 mmap19 > +mmap20 mmap20 > > modify_ldt01 modify_ldt01 > modify_ldt02 modify_ldt02 > diff --git testcases/kernel/syscalls/mmap/.gitignore > testcases/kernel/syscalls/mmap/.gitignore > index 8811226be..569a76ac1 100644 > --- testcases/kernel/syscalls/mmap/.gitignore > +++ testcases/kernel/syscalls/mmap/.gitignore > @@ -18,3 +18,4 @@ > /mmap17 > /mmap18 > /mmap19 > +/mmap20 > diff --git testcases/kernel/syscalls/mmap/mmap20.c > testcases/kernel/syscalls/mmap/mmap20.c > index ca5bfccd7..54ddb2087 100644 > --- testcases/kernel/syscalls/mmap/mmap20.c > +++ testcases/kernel/syscalls/mmap/mmap20.c > @@ -1,55 +1,58 @@ > -//SPDX-License-Identifier: GPL-2.0-or-later > - > +// SPDX-License-Identifier: GPL-2.0-or-later > /* > - * Test mmap with MAP_SHARED_VALIDATE flag > + * Copyright (c) 2023 paulson <lpaulsonraja@gmail.com> > + */ > + > +/*\ > + * [Description] > * > - * We are testing the MAP_SHARED_VALIDATE flag of mmap() syscall. To check > - * if there is an invalid flag value, the MAP_SHARED_VALIDATE return > - * EOPNOTSUPP. The unused bit in the MAP_SHARED_VALIDATE is found, and by > - * setting the unused bits of the flag argument the flag value becomes > - * invalid and the error EOPNOTSUPP is produced as expected. > + * Test mmap(2) with MAP_SHARED_VALIDATE flag. > + * > + * Test expected EOPNOTSUPP errno when testing mmap(2) with > MAP_SHARED_VALIDATE > + * flag and invalid flag. > */ > + > +#include <errno.h> > #include <stdio.h> > #include <sys/types.h> > -#include <sys/stat.h> > -#include <fcntl.h> > -#include <sys/mman.h> > -#include <linux/mman.h> > -#include <errno.h> > #include "tst_test.h" > +#include "lapi/mmap.h" > > #define TEST_FILE "file_to_mmap" > #define TEST_FILE_SIZE 1024 > -#define TEST_FILE_MODE 0600 > +#define INVALID_FLAG (1 << 7) > > -static int fd_file = -1; > -static void *mapped_address; > +static int fd = -1; > +static void *addr; > > static void setup(void) > { > - fd_file = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, TEST_FILE_MODE); > + fd = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, 0600); > + > if (tst_fill_file(TEST_FILE, 'a', TEST_FILE_SIZE, 1)) > - tst_brk(TBROK, "Could not fill the testfile."); > + tst_brk(TBROK, "Could not fill the testfile"); > } > > static void cleanup(void) > { > - if (fd_file > -1) > - SAFE_CLOSE(fd_file); > - if (mapped_address != NULL && mapped_address != MAP_FAILED) > - SAFE_MUNMAP(mapped_address, TEST_FILE_SIZE); > + if (fd > -1) > + SAFE_CLOSE(fd); > + > + if (addr && addr != MAP_FAILED) > + SAFE_MUNMAP(addr, TEST_FILE_SIZE); > } > > static void test_mmap(void) > { > - mapped_address = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, > - (1 << 7) | MAP_SHARED_VALIDATE, fd_file, 0); > - if (mapped_address != MAP_FAILED) > - tst_res(TFAIL | TERRNO, "mmap() is successful, but it > should have failed."); > + addr = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, > + INVALID_FLAG | MAP_SHARED_VALIDATE, fd, 0); > + > + if (addr != MAP_FAILED) > + tst_res(TFAIL | TERRNO, "mmap() is successful, but it > should have failed"); > else if (errno == EOPNOTSUPP) > - tst_res(TPASS, "mmap() failed with errno set to > EOPNOTSUPP."); > + tst_res(TPASS, "mmap() failed with errno set to > EOPNOTSUPP"); > else > - tst_res(TFAIL | TERRNO, "mmap() failed with unexpected > error."); > + tst_res(TFAIL | TERRNO, "mmap() failed with unexpected > error"); > } > > static struct tst_test test = { >
Hi Paulson, > Hi Petr Vorel, > Thanks for the comments, I have just started contributing to LTP, and > this is my first patch. I agree to merge it as is. > https://github.com/pevik/ltp/commits/paulson/mmap.fixes > https://github.com/pevik/ltp/blob/36ebc4d8900ea8438dc7839f03b7921f0ed9243a/testcases/kernel/syscalls/mmap/mmap20.c Good to know. I'll wait few days if anybody has additional comments before merge. Kind regards, Petr
Hi, > +static void test_mmap(void) > +{ > + mapped_address = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, > + (1 << 7) | MAP_SHARED_VALIDATE, fd_file, 0); > + if (mapped_address != MAP_FAILED) > + tst_res(TFAIL | TERRNO, "mmap() is successful, but it should have failed."); > + else if (errno == EOPNOTSUPP) > + tst_res(TPASS, "mmap() failed with errno set to EOPNOTSUPP."); > + else > + tst_res(TFAIL | TERRNO, "mmap() failed with unexpected error."); > +} FYI we have various helpers in include/tst_test_macros.h, e.g. TST_EXP_FAIL() for expected failures where return is just classical syscalls result (0 for pass, -1 for error) or TST_EXP_FD() for file descriptors. But these cannot be used here, because mmap returns pointer to void. We might want to write helpers which use TEST_VOID() instead of TEST(), after tests for mmap() are rewritten to new API, they could use it. Kind regards, Petr
Hi Paulson, Petr, Paulson Raja L <lpaulsonraja@gmail.com> wrote: > Hi Petr Vorel, > Thanks for the comments, I have just started contributing to LTP, and > this is my first patch. I agree to merge it as is. Good to see a new case for MAP_SHARED_VALIDATE. As this is the first test to cover the new flags argument, so I'm wondering can we add the functional verification to check if this works correctly for mapping a valid shared memory which equal to the behavior of MAP_SHARED? (or do this in a separate patch) But anyway, I'd suggest you send a patch V2 for achieving all requested changes.
On Mon, Mar 27, 2023 at 6:51 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi, > > > +static void test_mmap(void) > > +{ > > + mapped_address = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, > > + (1 << 7) | MAP_SHARED_VALIDATE, fd_file, 0); > > + if (mapped_address != MAP_FAILED) > > + tst_res(TFAIL | TERRNO, "mmap() is successful, but it should have failed."); > > + else if (errno == EOPNOTSUPP) > > + tst_res(TPASS, "mmap() failed with errno set to EOPNOTSUPP."); > > + else > > + tst_res(TFAIL | TERRNO, "mmap() failed with unexpected error."); > > +} > > FYI we have various helpers in include/tst_test_macros.h, e.g. TST_EXP_FAIL() > for expected failures where return is just classical syscalls result > (0 for pass, -1 for error) or TST_EXP_FD() for file descriptors. > > But these cannot be used here, because mmap returns pointer to void. > We might want to write helpers which use TEST_VOID() instead of TEST(), > after tests for mmap() are rewritten to new API, they could use it. Sounds good, but we can do this in later refactoring work on mmap tests. Now I'm slightly OK with not using TEST_ macros for everything:).
... > > FYI we have various helpers in include/tst_test_macros.h, e.g. TST_EXP_FAIL() > > for expected failures where return is just classical syscalls result > > (0 for pass, -1 for error) or TST_EXP_FD() for file descriptors. > > But these cannot be used here, because mmap returns pointer to void. > > We might want to write helpers which use TEST_VOID() instead of TEST(), > > after tests for mmap() are rewritten to new API, they could use it. > Sounds good, but we can do this in later refactoring work on mmap tests. > Now I'm slightly OK with not using TEST_ macros for everything:). Sure (it will not block this effort, definitely for separate effort with low priority). Kind regards, Petr
Hi Li, > Hi Paulson, Petr, > Paulson Raja L <lpaulsonraja@gmail.com> wrote: > > Hi Petr Vorel, > > Thanks for the comments, I have just started contributing to LTP, and > > this is my first patch. I agree to merge it as is. > Good to see a new case for MAP_SHARED_VALIDATE. > As this is the first test to cover the new flags argument, so I'm > wondering can we add the functional verification to check if > this works correctly for mapping a valid shared memory which > equal to the behavior of MAP_SHARED? > (or do this in a separate patch) +1 > But anyway, I'd suggest you send a patch V2 for achieving all > requested changes. Li, if you don't mind, I'll merge "v2" I prepared [1] (part of paulson/mmap.fixes branch in my fork). Can I add your Reviewed-by tag? As the work is already done, I'd prefer to postpone the work you propose to separate patch after. Kind regards, Petr [1] https://github.com/pevik/ltp/blob/36ebc4d8900ea8438dc7839f03b7921f0ed9243a/testcases/kernel/syscalls/mmap/mmap20.c
On Tue, Mar 28, 2023 at 4:17 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, > > > Hi Paulson, Petr, > > > Paulson Raja L <lpaulsonraja@gmail.com> wrote: > > > > Hi Petr Vorel, > > > Thanks for the comments, I have just started contributing to LTP, and > > > this is my first patch. I agree to merge it as is. > > > Good to see a new case for MAP_SHARED_VALIDATE. > > > As this is the first test to cover the new flags argument, so I'm > > wondering can we add the functional verification to check if > > this works correctly for mapping a valid shared memory which > > equal to the behavior of MAP_SHARED? > > (or do this in a separate patch) > +1 > > > But anyway, I'd suggest you send a patch V2 for achieving all > > requested changes. > > Li, if you don't mind, I'll merge "v2" I prepared [1] (part of > paulson/mmap.fixes branch in my fork). Can I add your Reviewed-by tag? As the > work is already done, I'd prefer to postpone the work you propose to separate > patch after. Of course yes!! Feel free to do that. My suggestion is only to Paulson for familiarity with the patch contribute workflow, but he can get to know this by your operation too.
... > > Li, if you don't mind, I'll merge "v2" I prepared [1] (part of > > paulson/mmap.fixes branch in my fork). Can I add your Reviewed-by tag? As the > > work is already done, I'd prefer to postpone the work you propose to separate > > patch after. > Of course yes!! Feel free to do that. Li, thanks for your ack, merged. > My suggestion is only to Paulson for familiarity with the patch > contribute workflow, > but he can get to know this by your operation too. +1 Kind regards, Petr
Hi all, FYI test fails on ppc64le: mmap() with MAP_SHARED_VALIDATE | (1 << 7) is supposed to fail with EOPNOTSUPP, but on ppc64le it fails with EFAULT. Obviously (1 << 7) being expected as invalid flag is not working well at least on ppc64le. It might be something page size related or alignment. I was looking into SYSCALL_DEFINE6(mmap, ...) implementations. Although ppc64le implementation is not the same as most of implementations on other archs, in the end it also calls ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> shift) as the other archs. Any idea what could be a problem? Kind regards, Petr
> Hi all, > FYI test fails on ppc64le: mmap() with MAP_SHARED_VALIDATE | (1 << 7) is > supposed to fail with EOPNOTSUPP, but on ppc64le it fails with EFAULT. > Obviously (1 << 7) being expected as invalid flag is not working well at least > on ppc64le. It might be something page size related or alignment. I was looking > into SYSCALL_DEFINE6(mmap, ...) implementations. Although ppc64le implementation > is not the same as most of implementations on other archs, in the end it also > calls ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> shift) as the other > archs. Any idea what could be a problem? OK, 1 << 7 defined as MAP_LOCKED on ppc64le and mips. 1 << 9 looks to be working on more archs, I'll send a patch. Kind regards, Petr > Kind regards, > Petr
diff --git a/testcases/kernel/syscalls/mmap/mmap20.c b/testcases/kernel/syscalls/mmap/mmap20.c new file mode 100644 index 000000000..ca5bfccd7 --- /dev/null +++ b/testcases/kernel/syscalls/mmap/mmap20.c @@ -0,0 +1,61 @@ +//SPDX-License-Identifier: GPL-2.0-or-later + +/* + * Test mmap with MAP_SHARED_VALIDATE flag + * + * We are testing the MAP_SHARED_VALIDATE flag of mmap() syscall. To check + * if there is an invalid flag value, the MAP_SHARED_VALIDATE return + * EOPNOTSUPP. The unused bit in the MAP_SHARED_VALIDATE is found, and by + * setting the unused bits of the flag argument the flag value becomes + * invalid and the error EOPNOTSUPP is produced as expected. + */ +#include <stdio.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <sys/mman.h> +#include <linux/mman.h> +#include <errno.h> +#include "tst_test.h" + +#define TEST_FILE "file_to_mmap" +#define TEST_FILE_SIZE 1024 +#define TEST_FILE_MODE 0600 + +static int fd_file = -1; +static void *mapped_address; + +static void setup(void) +{ + fd_file = SAFE_OPEN(TEST_FILE, O_CREAT | O_RDWR, TEST_FILE_MODE); + if (tst_fill_file(TEST_FILE, 'a', TEST_FILE_SIZE, 1)) + tst_brk(TBROK, "Could not fill the testfile."); +} + +static void cleanup(void) +{ + if (fd_file > -1) + SAFE_CLOSE(fd_file); + if (mapped_address != NULL && mapped_address != MAP_FAILED) + SAFE_MUNMAP(mapped_address, TEST_FILE_SIZE); +} + +static void test_mmap(void) +{ + mapped_address = mmap(NULL, TEST_FILE_SIZE, PROT_READ | PROT_WRITE, + (1 << 7) | MAP_SHARED_VALIDATE, fd_file, 0); + if (mapped_address != MAP_FAILED) + tst_res(TFAIL | TERRNO, "mmap() is successful, but it should have failed."); + else if (errno == EOPNOTSUPP) + tst_res(TPASS, "mmap() failed with errno set to EOPNOTSUPP."); + else + tst_res(TFAIL | TERRNO, "mmap() failed with unexpected error."); +} + +static struct tst_test test = { + .min_kver = "4.15", + .setup = setup, + .cleanup = cleanup, + .test_all = test_mmap, + .needs_tmpdir = 1, +};
From: paulson <lpaulsonraja@gmail.com> --- testcases/kernel/syscalls/mmap/mmap20.c | 61 +++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 testcases/kernel/syscalls/mmap/mmap20.c