[LTP] [PATCH] lib: Add SAFE_MSYNC() macro to LTP library

[LTP] [PATCH] lib: Add SAFE_MSYNC() macro to LTP library

[Avinesh Kumar]

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 include/tst_safe_macros.h |  6 ++++++
 lib/tst_safe_macros.c     | 19 +++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 0cf3d7878..c899c4f2c 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -86,6 +86,12 @@ void *safe_realloc(const char *file, const int lineno, void *ptr, size_t size);
 #define SAFE_MUNMAP(addr, length) \
 	safe_munmap(__FILE__, __LINE__, NULL, (addr), (length))
 
+int safe_msync(const char *file, const int lineno, void *addr,
+				size_t length, int flags);
+
+#define SAFE_MSYNC(addr, length, flags) \
+	safe_msync(__FILE__, __LINE__, (addr), (length), (flags))
+
 #define SAFE_OPEN(pathname, oflags, ...) \
 	safe_open(__FILE__, __LINE__, NULL, (pathname), (oflags), \
 	    ##__VA_ARGS__)
diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
index c4cdc87e7..7f28d43e5 100644
--- a/lib/tst_safe_macros.c
+++ b/lib/tst_safe_macros.c
@@ -591,3 +591,22 @@ void safe_cmd(const char *file, const int lineno, const char *const argv[],
 		tst_brk_(file, lineno, TBROK, "%s failed (%d)", argv[0], rval);
 	}
 }
+
+int safe_msync(const char *file, const int lineno, void *addr,
+				size_t length, int flags)
+{
+	int rval;
+
+	rval = msync(addr, length, flags);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"msync(%p, %zu, %d) failed", addr, length, flags);
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"Invalid msync(%p, %zu, %d) return value %d",
+			addr, length, flags, rval);
+	}
+
+	return rval;
+}
-- 
2.42.0


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

[LTP] [PATCH v3] syscalls/mmap01: Rewrite the test using new LTP API

[Avinesh Kumar]

- use SAFE_MSYNC() macro
- fixed the test for iterations > 1

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++-----------------
 1 file changed, 63 insertions(+), 160 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
index 99266b57f..a1e6e9d59 100644
--- a/testcases/kernel/syscalls/mmap/mmap01.c
+++ b/testcases/kernel/syscalls/mmap/mmap01.c
@@ -1,194 +1,97 @@
+// 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
+ *	07/2001 Ported by Wayne Boyer
+ * Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
  */
 
-/*
- * Test Description:
- *  Verify that, mmap() succeeds when used to map a file where size of the
- *  file is not a multiple of the page size, the memory area beyond the end
- *  of the file to the end of the page is accessible. Also, verify that
- *  this area is all zeroed and the modifications done to this area are
- *  not written to the file.
- *
- * Expected Result:
- *  mmap() should succeed returning the address of the mapped region.
- *  The memory area beyond the end of file to the end of page should be
- *  filled with zero.
- *  The changes beyond the end of file should not get written to the file.
+/*\
+ * [Description]
  *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
+ * Verify that, mmap() succeeds when used to map a file where size of the
+ * file is not a multiple of the page size, the memory area beyond the end
+ * of the file to the end of the page is accessible. Also, verify that
+ * this area is all zeroed and the modifications done to this area are
+ * not written to the file.
  */
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/types.h>
-#include <errno.h>
-#include <unistd.h>
-#include <fcntl.h>
-#include <string.h>
-#include <signal.h>
-#include <stdint.h>
-#include <sys/stat.h>
-#include <sys/mman.h>
-#include <sys/shm.h>
-
-#include "test.h"
-
-#define TEMPFILE	"mmapfile"
 
-char *TCID = "mmap01";
-int TST_TOTAL = 1;
+#include <stdlib.h>
+#include "tst_test.h"
 
-static char *addr;
-static char *dummy;
+#define TEMPFILE "mmapfile"
+static int fd;
 static size_t page_sz;
 static size_t file_sz;
-static int fildes;
-static char cmd_buffer[BUFSIZ];
-
-static void setup(void);
-static void cleanup(void);
+static char *dummy;
+static char *addr;
 
-int main(int ac, char **av)
+static void setup(void)
 {
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/*
-		 * Call mmap to map the temporary file beyond EOF
-		 * with write access.
-		 */
-		errno = 0;
-		addr = mmap(NULL, page_sz, PROT_READ | PROT_WRITE,
-			    MAP_FILE | MAP_SHARED, fildes, 0);
-
-		/* Check for the return value of mmap() */
-		if (addr == MAP_FAILED) {
-			tst_resm(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
-			continue;
-		}
-
-		/*
-		 * Check if mapped memory area beyond EOF are
-		 * zeros and changes beyond EOF are not written
-		 * to file.
-		 */
-		if (memcmp(&addr[file_sz], dummy, page_sz - file_sz)) {
-			tst_brkm(TFAIL, cleanup,
-				 "mapped memory area contains invalid "
-				 "data");
-		}
+	struct stat stat_buf;
+	char write_buf[] = "hello world\n";
 
-		/*
-		 * Initialize memory beyond file size
-		 */
-		addr[file_sz] = 'X';
-		addr[file_sz + 1] = 'Y';
-		addr[file_sz + 2] = 'Z';
-
-		/*
-		 * Synchronize the mapped memory region
-		 * with the file.
-		 */
-		if (msync(addr, page_sz, MS_SYNC) != 0) {
-			tst_brkm(TFAIL | TERRNO, cleanup,
-				 "failed to synchronize mapped file");
-		}
+	fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666);
 
-		/*
-		 * Now, Search for the pattern 'XYZ' in the
-		 * temporary file.  The pattern should not be
-		 * found and the return value should be 1.
-		 */
-		if (system(cmd_buffer) != 0) {
-			tst_resm(TPASS,
-				 "Functionality of mmap() successful");
-		} else {
-			tst_resm(TFAIL,
-				 "Specified pattern found in file");
-		}
+	SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf));
+	SAFE_LSEEK(fd, 0, SEEK_SET);
+	SAFE_STAT(TEMPFILE, &stat_buf);
 
-		/* Clean up things in case we are looping */
-		/* Unmap the mapped memory */
-		if (munmap(addr, page_sz) != 0) {
-			tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");
-		}
-	}
+	file_sz = stat_buf.st_size;
+	page_sz = getpagesize();
 
-	cleanup();
-	tst_exit();
+	dummy = SAFE_MALLOC(page_sz);
+	memset(dummy, 0, page_sz);
 }
 
-static void setup(void)
+static void run(void)
 {
-	struct stat stat_buf;
-	char Path_name[PATH_MAX];
-	char write_buf[] = "hello world\n";
+	char buf[20];
+	unsigned long read_cnt;
 
-	tst_sig(FORK, DEF_HANDLER, cleanup);
+	addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
 
-	TEST_PAUSE;
-
-	tst_tmpdir();
-
-	/* Get the path of temporary file to be created */
-	if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
-		tst_brkm(TFAIL | TERRNO, cleanup,
-			 "getcwd failed to get current working directory");
+	if (memcmp(&addr[file_sz], dummy, page_sz - file_sz) != 0) {
+		tst_res(TFAIL, "mapped memory area contains invalid data");
+		goto unmap;
 	}
 
-	/* Creat a temporary file used for mapping */
-	if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
-		tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
-	}
+	addr[file_sz] = 'X';
+	addr[file_sz + 1] = 'Y';
+	addr[file_sz + 2] = 'Z';
 
-	/* Write some data into temporary file */
-	if (write(fildes, write_buf, strlen(write_buf)) != (long)strlen(write_buf)) {
-		tst_brkm(TFAIL, cleanup, "writing to %s", TEMPFILE);
-	}
+	SAFE_MSYNC(addr, page_sz, MS_SYNC);
 
-	/* Get the size of temporary file */
-	if (stat(TEMPFILE, &stat_buf) < 0) {
-		tst_brkm(TFAIL | TERRNO, cleanup, "stat of %s failed",
-			 TEMPFILE);
-	}
-	file_sz = stat_buf.st_size;
+	memset(buf, 0, sizeof(buf));
+	read_cnt = SAFE_READ(0, fd, buf, sizeof(buf));
+	if (read_cnt != file_sz)
+		tst_res(TFAIL, "unexpected data length in buf");
 
-	page_sz = getpagesize();
+	for (int i = 0; i < 3; i++)
+		if ((memchr(buf, (int)('X') + i, sizeof(buf))) != NULL) {
+			tst_res(TFAIL, "mmap() functionality failed");
+			goto unmap;
+		}
 
-	/* Allocate and initialize dummy string of system page size bytes */
-	if ((dummy = calloc(page_sz, sizeof(char))) == NULL) {
-		tst_brkm(TFAIL, cleanup, "calloc failed (dummy)");
-	}
+	tst_res(TPASS, "mmap() functionality successful");
 
-	/* Create the command which will be executed in the test */
-	sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE);
+	SAFE_LSEEK(fd, 0, SEEK_SET);
+	memset(&addr[file_sz], 0, 3);
+
+unmap:
+	SAFE_MUNMAP(addr, page_sz);
 }
 
 static void cleanup(void)
 {
-	close(fildes);
+	if (fd > 0)
+		SAFE_CLOSE(fd);
+
 	free(dummy);
-	tst_rmdir();
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+	.needs_tmpdir = 1
+};
-- 
2.42.0


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

Re: [LTP] [PATCH] lib: Add SAFE_MSYNC() macro to LTP library

[Petr Vorel]

Hi Avinesh,

Merged, thank you!
I also used it in two tests. Do you plan to do the rest?

I guess modifying old Cyril's script could help to avoid manual change

https://lore.kernel.org/ltp/20171003114349.GA19124@rei/
https://lists.linux.it/pipermail/ltp/attachments/20171003/23122597/attachment.sh

Kind regards,
Petr

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

Re: [LTP] [PATCH v3] syscalls/mmap01: Rewrite the test using new LTP API

[Richard Palethorpe]

Hello,

Avinesh Kumar <akumar@suse.de> writes:

> - use SAFE_MSYNC() macro
> - fixed the test for iterations > 1
>
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++-----------------
>  1 file changed, 63 insertions(+), 160 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
> index 99266b57f..a1e6e9d59 100644
> --- a/testcases/kernel/syscalls/mmap/mmap01.c
> +++ b/testcases/kernel/syscalls/mmap/mmap01.c
> @@ -1,194 +1,97 @@
> +// 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
> + *	07/2001 Ported by Wayne Boyer
> + * Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
>   */
>  
> -/*
> - * Test Description:
> - *  Verify that, mmap() succeeds when used to map a file where size of the
> - *  file is not a multiple of the page size, the memory area beyond the end
> - *  of the file to the end of the page is accessible. Also, verify that
> - *  this area is all zeroed and the modifications done to this area are
> - *  not written to the file.
> - *
> - * Expected Result:
> - *  mmap() should succeed returning the address of the mapped region.
> - *  The memory area beyond the end of file to the end of page should be
> - *  filled with zero.
> - *  The changes beyond the end of file should not get written to the file.
> +/*\
> + * [Description]
>   *
> - * HISTORY
> - *	07/2001 Ported by Wayne Boyer
> + * Verify that, mmap() succeeds when used to map a file where size of the
> + * file is not a multiple of the page size, the memory area beyond the end
> + * of the file to the end of the page is accessible. Also, verify that
> + * this area is all zeroed and the modifications done to this area are
> + * not written to the file.
>   */
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <sys/types.h>
> -#include <errno.h>
> -#include <unistd.h>
> -#include <fcntl.h>
> -#include <string.h>
> -#include <signal.h>
> -#include <stdint.h>
> -#include <sys/stat.h>
> -#include <sys/mman.h>
> -#include <sys/shm.h>
> -
> -#include "test.h"
> -
> -#define TEMPFILE	"mmapfile"
>  
> -char *TCID = "mmap01";
> -int TST_TOTAL = 1;
> +#include <stdlib.h>
> +#include "tst_test.h"
>  
> -static char *addr;
> -static char *dummy;
> +#define TEMPFILE "mmapfile"
> +static int fd;
>  static size_t page_sz;
>  static size_t file_sz;
> -static int fildes;
> -static char cmd_buffer[BUFSIZ];
> -
> -static void setup(void);
> -static void cleanup(void);
> +static char *dummy;
> +static char *addr;
>  
> -int main(int ac, char **av)
> +static void setup(void)
>  {
> -	int lc;
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> -
> -		/*
> -		 * Call mmap to map the temporary file beyond EOF
> -		 * with write access.
> -		 */
> -		errno = 0;
> -		addr = mmap(NULL, page_sz, PROT_READ | PROT_WRITE,
> -			    MAP_FILE | MAP_SHARED, fildes, 0);
> -
> -		/* Check for the return value of mmap() */
> -		if (addr == MAP_FAILED) {
> -			tst_resm(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
> -			continue;
> -		}
> -
> -		/*
> -		 * Check if mapped memory area beyond EOF are
> -		 * zeros and changes beyond EOF are not written
> -		 * to file.
> -		 */
> -		if (memcmp(&addr[file_sz], dummy, page_sz - file_sz)) {
> -			tst_brkm(TFAIL, cleanup,
> -				 "mapped memory area contains invalid "
> -				 "data");
> -		}
> +	struct stat stat_buf;
> +	char write_buf[] = "hello world\n";
>  
> -		/*
> -		 * Initialize memory beyond file size
> -		 */
> -		addr[file_sz] = 'X';
> -		addr[file_sz + 1] = 'Y';
> -		addr[file_sz + 2] = 'Z';
> -
> -		/*
> -		 * Synchronize the mapped memory region
> -		 * with the file.
> -		 */
> -		if (msync(addr, page_sz, MS_SYNC) != 0) {
> -			tst_brkm(TFAIL | TERRNO, cleanup,
> -				 "failed to synchronize mapped file");
> -		}
> +	fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666);
>  
> -		/*
> -		 * Now, Search for the pattern 'XYZ' in the
> -		 * temporary file.  The pattern should not be
> -		 * found and the return value should be 1.
> -		 */
> -		if (system(cmd_buffer) != 0) {
> -			tst_resm(TPASS,
> -				 "Functionality of mmap() successful");
> -		} else {
> -			tst_resm(TFAIL,
> -				 "Specified pattern found in file");
> -		}
> +	SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf));
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +	SAFE_STAT(TEMPFILE, &stat_buf);
>  
> -		/* Clean up things in case we are looping */
> -		/* Unmap the mapped memory */
> -		if (munmap(addr, page_sz) != 0) {
> -			tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");
> -		}
> -	}
> +	file_sz = stat_buf.st_size;

Adding to what Cyril said; I see no reason to stat the file. We simply
expect to read back the same data we wrote.

If the file system transforms the data and adds to it, then it is not
even guaranteed file_sz < page_sz.

> +	page_sz = getpagesize();
>  
> -	cleanup();
> -	tst_exit();
> +	dummy = SAFE_MALLOC(page_sz);
> +	memset(dummy, 0, page_sz);
>  }
>  
> -static void setup(void)
> +static void run(void)
>  {
> -	struct stat stat_buf;
> -	char Path_name[PATH_MAX];
> -	char write_buf[] = "hello world\n";
> +	char buf[20];
> +	unsigned long read_cnt;
>  
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> +	addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0);
>  
> -	TEST_PAUSE;
> -
> -	tst_tmpdir();
> -
> -	/* Get the path of temporary file to be created */
> -	if (getcwd(Path_name, sizeof(Path_name)) == NULL) {
> -		tst_brkm(TFAIL | TERRNO, cleanup,
> -			 "getcwd failed to get current working directory");
> +	if (memcmp(&addr[file_sz], dummy, page_sz - file_sz) != 0) {
> +		tst_res(TFAIL, "mapped memory area contains invalid data");
> +		goto unmap;
>  	}
>  
> -	/* Creat a temporary file used for mapping */
> -	if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
> -		tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
> -	}
> +	addr[file_sz] = 'X';
> +	addr[file_sz + 1] = 'Y';
> +	addr[file_sz + 2] = 'Z';
>  
> -	/* Write some data into temporary file */
> -	if (write(fildes, write_buf, strlen(write_buf)) != (long)strlen(write_buf)) {
> -		tst_brkm(TFAIL, cleanup, "writing to %s", TEMPFILE);
> -	}
> +	SAFE_MSYNC(addr, page_sz, MS_SYNC);
>  
> -	/* Get the size of temporary file */
> -	if (stat(TEMPFILE, &stat_buf) < 0) {
> -		tst_brkm(TFAIL | TERRNO, cleanup, "stat of %s failed",
> -			 TEMPFILE);
> -	}
> -	file_sz = stat_buf.st_size;
> +	memset(buf, 0, sizeof(buf));
> +	read_cnt = SAFE_READ(0, fd, buf, sizeof(buf));
> +	if (read_cnt != file_sz)
> +		tst_res(TFAIL, "unexpected data length in buf");
>  
> -	page_sz = getpagesize();
> +	for (int i = 0; i < 3; i++)
> +		if ((memchr(buf, (int)('X') + i, sizeof(buf))) != NULL) {
> +			tst_res(TFAIL, "mmap() functionality failed");
> +			goto unmap;
> +		}

This seems overly complicated and would miss corruption. If any byte has
changed from the original string, then that is an error.

>  
> -	/* Allocate and initialize dummy string of system page size bytes */
> -	if ((dummy = calloc(page_sz, sizeof(char))) == NULL) {
> -		tst_brkm(TFAIL, cleanup, "calloc failed (dummy)");
> -	}
> +	tst_res(TPASS, "mmap() functionality successful");
>  
> -	/* Create the command which will be executed in the test */
> -	sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE);
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +	memset(&addr[file_sz], 0, 3);
> +
> +unmap:
> +	SAFE_MUNMAP(addr, page_sz);
>  }
>  
>  static void cleanup(void)
>  {
> -	close(fildes);
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +
>  	free(dummy);
> -	tst_rmdir();
>  }
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.needs_tmpdir = 1

We should add all filesystems here.

> +};
> -- 
> 2.42.0


-- 
Thank you,
Richard.

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