[LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[paulson]

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

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,
+};
-- 
2.25.1


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

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 = {

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Paulson Raja L]

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 = {
>

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

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

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Li Wang]

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.

-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

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

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Li Wang]

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.


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

...
> > 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

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

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


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

> 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


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

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

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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Li Wang]

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:).


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH] Added test for mmap() with MAP_SHARED_VALIDATE.

[Petr Vorel]

...
> > 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

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