| Message ID | 20240130122540.13215-1-akumar@suse.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4] syscalls/mmap01: Rewrite the test using new LTP API | expand |
HI Avinesh, > - use SAFE_MSYNC() macro > - fixed the test for iterations > 1 > - enable test for all filesystems +1 > Signed-off-by: Avinesh Kumar <akumar@suse.de> > --- > Changes v3->v4: > * Changed the logic to verify that mapped file has not been changed. > * Enabled all filesystems. > testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++----------------- > 1 file changed, 61 insertions(+), 162 deletions(-) > diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c ... > +static char *addr; > +static char *dummy; > +static struct stat stat_buf; nit: struct stat stat_buf is used just in the setup, it could defined just there. It can be changed before merge. > +static const char write_buf[] = "HelloWorld!"; > -int main(int ac, char **av) > +static void setup(void) > { ... > + fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666); > - cleanup(); > - tst_exit(); > -} > + SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf)); > + SAFE_LSEEK(fd, 0, SEEK_SET); > + SAFE_STAT(TEMPFILE, &stat_buf); > -static void setup(void) > -{ > - struct stat stat_buf; > - char Path_name[PATH_MAX]; > - char write_buf[] = "hello world\n"; > + file_sz = stat_buf.st_size; > + page_sz = getpagesize(); > - tst_sig(FORK, DEF_HANDLER, cleanup); > + dummy = SAFE_MALLOC(page_sz); > + memset(dummy, 0, page_sz); > +} > - TEST_PAUSE; > +static void run(void) > +{ > + char buf[20]; > - tst_tmpdir(); > + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0); ... > + addr[file_sz] = 'X'; > + addr[file_sz + 1] = 'Y'; > + addr[file_sz + 2] = 'Z'; ... > + SAFE_MSYNC(addr, page_sz, MS_SYNC); ... > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf); > - page_sz = getpagesize(); > + if (strcmp(write_buf, buf)) > + tst_res(TFAIL, "File data has changed"); > + else > + tst_res(TPASS, "mmap() functionality successful"); nit: I would prefer something more descriptive (what mmap() actually did, e.g. "mapped file has not been changed"), but I guess it's ok to keep it as is. The rest LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
On Tue, Jan 30, 2024 at 01:23:57PM +0100, Avinesh Kumar wrote: > - use SAFE_MSYNC() macro > - fixed the test for iterations > 1 > - enable test for all filesystems > > Signed-off-by: Avinesh Kumar <akumar@suse.de> > --- > > Changes v3->v4: > * Changed the logic to verify that mapped file has not been changed. > * Enabled all filesystems. > > > testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++----------------- > 1 file changed, 61 insertions(+), 162 deletions(-) > > diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c > index 99266b57f..e0b36915c 100644 > --- a/testcases/kernel/syscalls/mmap/mmap01.c > +++ b/testcases/kernel/syscalls/mmap/mmap01.c > @@ -1,194 +1,93 @@ > +// 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" > +#include <stdlib.h> > +#include "tst_test.h" > > -char *TCID = "mmap01"; > -int TST_TOTAL = 1; > +#define MNT_POINT "mntpoint" > +#define TEMPFILE MNT_POINT"/mmapfile" > > -static char *addr; > -static char *dummy; > +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 *addr; > +static char *dummy; > +static struct stat stat_buf; > +static const char write_buf[] = "HelloWorld!"; > > -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"); > - } > - > - /* > - * 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"); > - } > - > - /* > - * 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"); > - } > - > - /* 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"); > - } > - } > + fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666); > > - cleanup(); > - tst_exit(); > -} > + SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf)); > + SAFE_LSEEK(fd, 0, SEEK_SET); > + SAFE_STAT(TEMPFILE, &stat_buf); > > -static void setup(void) > -{ > - struct stat stat_buf; > - char Path_name[PATH_MAX]; > - char write_buf[] = "hello world\n"; > + file_sz = stat_buf.st_size; > + page_sz = getpagesize(); > > - tst_sig(FORK, DEF_HANDLER, cleanup); > + dummy = SAFE_MALLOC(page_sz); > + memset(dummy, 0, page_sz); > +} > > - TEST_PAUSE; > +static void run(void) > +{ > + char buf[20]; > > - tst_tmpdir(); > + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0); > > - /* 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; > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf); Hmm, why do we SAFE_LSEEK() the fd if we are not using it for reading? This could be just simple SAFE_READ() instead. > - page_sz = getpagesize(); > + if (strcmp(write_buf, buf)) > + tst_res(TFAIL, "File data has changed"); > + else > + tst_res(TPASS, "mmap() functionality successful"); ^ "Data after file end were not written out" It's kind of pointless to print message that just means "success". > - /* 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)"); > - } > + SAFE_LSEEK(fd, 0, SEEK_SET); > + memset(&addr[file_sz], 0, 3); I was wondering why this is needed, seems like for tmpfs we will read back the data after the end of the file on a subsequent runs of the test, i.e. with -i 2. I wonder if that is expected or not, it's a bit strange that we can expand the file size that way. And it seems to happen for FUSE as well, that actually does sound like a bug. > - /* Create the command which will be executed in the test */ > - sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE); > +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, > + .test_all = run, > + .cleanup = cleanup, > + .mntpoint = MNT_POINT, > + .mount_device = 1, > + .all_filesystems = 1 > +}; > -- > 2.43.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi Cyril, Petr, Thank you for the review. On Wednesday, March 6, 2024 3:55:53 PM CET Cyril Hrubis wrote: > On Tue, Jan 30, 2024 at 01:23:57PM +0100, Avinesh Kumar wrote: > > - use SAFE_MSYNC() macro > > - fixed the test for iterations > 1 > > - enable test for all filesystems > > > > Signed-off-by: Avinesh Kumar <akumar@suse.de> > > --- > > > > Changes v3->v4: > > * Changed the logic to verify that mapped file has not been changed. > > * Enabled all filesystems. > > > > testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++----------------- > > 1 file changed, 61 insertions(+), 162 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/mmap/mmap01.c > > b/testcases/kernel/syscalls/mmap/mmap01.c index 99266b57f..e0b36915c > > 100644 > > --- a/testcases/kernel/syscalls/mmap/mmap01.c > > +++ b/testcases/kernel/syscalls/mmap/mmap01.c > > @@ -1,194 +1,93 @@ > > +// 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" > > +#include <stdlib.h> > > +#include "tst_test.h" > > > > -char *TCID = "mmap01"; > > -int TST_TOTAL = 1; > > +#define MNT_POINT "mntpoint" > > +#define TEMPFILE MNT_POINT"/mmapfile" > > > > -static char *addr; > > -static char *dummy; > > +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 *addr; > > +static char *dummy; > > +static struct stat stat_buf; > > +static const char write_buf[] = "HelloWorld!"; > > > > -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"); > > - } > > - > > - /* > > - * 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"); > > - } > > - > > - /* > > - * 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"); > > - } > > - > > - /* 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"); > > - } > > - } > > + fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666); > > > > - cleanup(); > > - tst_exit(); > > -} > > + SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf)); > > + SAFE_LSEEK(fd, 0, SEEK_SET); > > + SAFE_STAT(TEMPFILE, &stat_buf); > > > > -static void setup(void) > > -{ > > - struct stat stat_buf; > > - char Path_name[PATH_MAX]; > > - char write_buf[] = "hello world\n"; > > + file_sz = stat_buf.st_size; > > + page_sz = getpagesize(); > > > > - tst_sig(FORK, DEF_HANDLER, cleanup); > > + dummy = SAFE_MALLOC(page_sz); > > + memset(dummy, 0, page_sz); > > +} > > > > - TEST_PAUSE; > > +static void run(void) > > +{ > > + char buf[20]; > > > > - tst_tmpdir(); > > + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | > > MAP_SHARED, fd, 0); > > > > - /* 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; > > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf); > > Hmm, why do we SAFE_LSEEK() the fd if we are not using it for reading? I guess I can remove the SAFE_LSEEK() in setup(), as we want to read the complete file contents without knowing it's size, hence SAFE_FILE_SCANF(). Please correct me if this is not the right approach. > > This could be just simple SAFE_READ() instead. > > > - page_sz = getpagesize(); > > + if (strcmp(write_buf, buf)) > > + tst_res(TFAIL, "File data has changed"); > > + else > > + tst_res(TPASS, "mmap() functionality successful"); > > ^ > "Data after file end were not written out" > > It's kind of pointless to print message that just means "success". > > > - /* 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)"); > > - } > > + SAFE_LSEEK(fd, 0, SEEK_SET); > > + memset(&addr[file_sz], 0, 3); > > I was wondering why this is needed, seems like for tmpfs we will read > back the data after the end of the file on a subsequent runs of the > test, i.e. with -i 2. > > I wonder if that is expected or not, it's a bit strange that we can > expand the file size that way. > > And it seems to happen for FUSE as well, that actually does sound like a > bug. Thanks for pointing this out, I was overlooking this issue. I verified that we read back the data written past eof in further iteration of the test only in tmpfs and fuse.ntfs. How would you suggest to confirm if this is indeed a bug with these filesystems. Regards, Avinesh
Hi Avinesh, > Hi Cyril, Petr, > Thank you for the review. ... > > > - /* 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; > > > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf); > > Hmm, why do we SAFE_LSEEK() the fd if we are not using it for reading? > I guess I can remove the SAFE_LSEEK() in setup(), as we want to read the > complete file contents without knowing it's size, hence SAFE_FILE_SCANF(). I'm not sure if any lseek() is needed. > Please correct me if this is not the right approach. I guess Cyril means by SAFE_READ() to read just that 3 bytes changed or strlen(write_buf) (whole string). > > This could be just simple SAFE_READ() instead. > > > - page_sz = getpagesize(); > > > + if (strcmp(write_buf, buf)) > > > + tst_res(TFAIL, "File data has changed"); > > > + else > > > + tst_res(TPASS, "mmap() functionality successful"); > > ^ > > "Data after file end were not written out" > > It's kind of pointless to print message that just means "success". > > > - /* 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)"); > > > - } > > > + SAFE_LSEEK(fd, 0, SEEK_SET); > > > + memset(&addr[file_sz], 0, 3); > > I was wondering why this is needed, seems like for tmpfs we will read > > back the data after the end of the file on a subsequent runs of the > > test, i.e. with -i 2. > > I wonder if that is expected or not, it's a bit strange that we can > > expand the file size that way. > > And it seems to happen for FUSE as well, that actually does sound like a > > bug. > Thanks for pointing this out, I was overlooking this issue. I verified that we > read back the data written past eof in further iteration of the test only in > tmpfs and fuse.ntfs. How would you suggest to confirm if this is indeed a bug > with these filesystems. Interesting. Feel free to Cc LTP ML if you report to mainline developers (not sure if mainline kernel or SUSE kernel is affected). Kind regards, Petr > Regards, > Avinesh
diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c index 99266b57f..e0b36915c 100644 --- a/testcases/kernel/syscalls/mmap/mmap01.c +++ b/testcases/kernel/syscalls/mmap/mmap01.c @@ -1,194 +1,93 @@ +// 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" +#include <stdlib.h> +#include "tst_test.h" -char *TCID = "mmap01"; -int TST_TOTAL = 1; +#define MNT_POINT "mntpoint" +#define TEMPFILE MNT_POINT"/mmapfile" -static char *addr; -static char *dummy; +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 *addr; +static char *dummy; +static struct stat stat_buf; +static const char write_buf[] = "HelloWorld!"; -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"); - } - - /* - * 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"); - } - - /* - * 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"); - } - - /* 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"); - } - } + fd = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0666); - cleanup(); - tst_exit(); -} + SAFE_WRITE(SAFE_WRITE_ALL, fd, write_buf, strlen(write_buf)); + SAFE_LSEEK(fd, 0, SEEK_SET); + SAFE_STAT(TEMPFILE, &stat_buf); -static void setup(void) -{ - struct stat stat_buf; - char Path_name[PATH_MAX]; - char write_buf[] = "hello world\n"; + file_sz = stat_buf.st_size; + page_sz = getpagesize(); - tst_sig(FORK, DEF_HANDLER, cleanup); + dummy = SAFE_MALLOC(page_sz); + memset(dummy, 0, page_sz); +} - TEST_PAUSE; +static void run(void) +{ + char buf[20]; - tst_tmpdir(); + addr = SAFE_MMAP(NULL, page_sz, PROT_READ | PROT_WRITE, MAP_FILE | MAP_SHARED, fd, 0); - /* 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; + SAFE_FILE_SCANF(TEMPFILE, "%s", buf); - page_sz = getpagesize(); + if (strcmp(write_buf, buf)) + tst_res(TFAIL, "File data has changed"); + else + tst_res(TPASS, "mmap() functionality successful"); - /* 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)"); - } + SAFE_LSEEK(fd, 0, SEEK_SET); + memset(&addr[file_sz], 0, 3); - /* Create the command which will be executed in the test */ - sprintf(cmd_buffer, "grep XYZ %s/%s > /dev/null", Path_name, TEMPFILE); +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, + .test_all = run, + .cleanup = cleanup, + .mntpoint = MNT_POINT, + .mount_device = 1, + .all_filesystems = 1 +};
- use SAFE_MSYNC() macro - fixed the test for iterations > 1 - enable test for all filesystems Signed-off-by: Avinesh Kumar <akumar@suse.de> --- Changes v3->v4: * Changed the logic to verify that mapped file has not been changed. * Enabled all filesystems. testcases/kernel/syscalls/mmap/mmap01.c | 223 +++++++----------------- 1 file changed, 61 insertions(+), 162 deletions(-)