| Message ID | 9c46f648cd8c784405afed565bed120f0a2f239e.1687187451.git.falcon@tinylab.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,01/10] tools/nolibc: sys.h: add a syscall return helper | expand |
On 2023-06-19 23:55:41+0800, Zhangjin Wu wrote: > Three mmap/munmap related test cases are added: > > - mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL) > > The length argument must be greater than 0, otherwise, fail with -EINVAL. > > - munmap((void *)-1, 4*1024), -1, EINVAL) > > Invalid (void *)-1 address fail with -EINVAL. > > - test_mmap_munmap(4*1024) > > It finds a init file, mmap() it and then munmap(). > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > --- > tools/testing/selftests/nolibc/nolibc-test.c | 31 ++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > index 80ab29e2887c..f7c0ca72cb28 100644 > --- a/tools/testing/selftests/nolibc/nolibc-test.c > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > @@ -592,6 +592,34 @@ static int test_stat_timestamps(void) > return 0; > } > > +int test_mmap_munmap(int size) > +{ > + char init_files[5][20] = {"/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh"}; Why not /proc/1/exe or even /proc/self/exe? I know your other series tries to remove the procfs dependency, but we're not there yet :-). Also does it make sense to pass a size parameter? Why not use either PAGE_SIZE or the real size of the binary from fstat(). > + int ret, fd, i; > + void *mem; > + > + for (i = 0; i < 5; i++) { > + ret = fd = open(init_files[i], O_RDONLY); > + if (ret < 0) > + continue; > + else > + break; > + } > + if (ret < 0) > + return ret; > + > + mem = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0); > + if (mem == MAP_FAILED) > + return -1; > + > + ret = munmap(mem, size); > + if (ret < 0) > + return ret; > + > + return close(fd); > +} > + > + > /* Run syscall tests between IDs <min> and <max>. > * Return 0 on success, non-zero on failure. > */ > @@ -666,6 +694,9 @@ int run_syscall(int min, int max) > CASE_TEST(lseek_m1); EXPECT_SYSER(1, lseek(-1, 0, SEEK_SET), -1, EBADF); break; > CASE_TEST(lseek_0); EXPECT_SYSER(1, lseek(0, 0, SEEK_SET), -1, ESPIPE); break; > CASE_TEST(mkdir_root); EXPECT_SYSER(1, mkdir("/", 0755), -1, EEXIST); break; > + CASE_TEST(mmap_bad); EXPECT_PTRER(1, mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL); break; > + CASE_TEST(munmap_bad); EXPECT_SYSER(1, munmap((void *)-1, 0), -1, EINVAL); break; > + CASE_TEST(mmap_good); EXPECT_SYSZR(1, test_mmap_munmap(4*1024)); break; > CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; > CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; > CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break; > -- > 2.25.1 >
Hi, Thomas > On 2023-06-19 23:55:41+0800, Zhangjin Wu wrote: > > Three mmap/munmap related test cases are added: > > > > - mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL) > > > > The length argument must be greater than 0, otherwise, fail with -EINVAL. > > > > - munmap((void *)-1, 4*1024), -1, EINVAL) > > > > Invalid (void *)-1 address fail with -EINVAL. > > > > - test_mmap_munmap(4*1024) > > > > It finds a init file, mmap() it and then munmap(). > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > --- > > tools/testing/selftests/nolibc/nolibc-test.c | 31 ++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > index 80ab29e2887c..f7c0ca72cb28 100644 > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > @@ -592,6 +592,34 @@ static int test_stat_timestamps(void) > > return 0; > > } > > > > +int test_mmap_munmap(int size) > > +{ > > + char init_files[5][20] = {"/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh"}; > > Why not /proc/1/exe or even /proc/self/exe? > > I know your other series tries to remove the procfs dependency, but > we're not there yet :-). > Yeah, '/proc/self/exe' is a choice, if so, the 'has_proc' should be added ;-) > Also does it make sense to pass a size parameter? > Why not use either PAGE_SIZE or the real size of the binary from > fstat(). > Ok, as the manpage of mmap shows: For mmap(), offset must be a multiple of the underlying huge page size. The system automatically aligns length to be a multiple of the underlying huge page size. For munmap(), addr, and length must both be a multiple of the underlying huge page size. perhaps we should do further tests: * the real size/length * even > the real size * the PAGE_SIZE * not aligned with PAGE_SIZE If such tests are required, the 'size' and even 'offset' arguments could be provided to cover different combination or we do such tests internally, then, the arguments are not required. Thanks, Zhangjin > > + int ret, fd, i; > > + void *mem; > > + > > + for (i = 0; i < 5; i++) { > > + ret = fd = open(init_files[i], O_RDONLY); > > + if (ret < 0) > > + continue; > > + else > > + break; > > + } > > + if (ret < 0) > > + return ret; > > + > > + mem = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0); > > + if (mem == MAP_FAILED) > > + return -1; > > + > > + ret = munmap(mem, size); > > + if (ret < 0) > > + return ret; > > + > > + return close(fd); > > +} > > + > > + > > /* Run syscall tests between IDs <min> and <max>. > > * Return 0 on success, non-zero on failure. > > */ > > @@ -666,6 +694,9 @@ int run_syscall(int min, int max) > > CASE_TEST(lseek_m1); EXPECT_SYSER(1, lseek(-1, 0, SEEK_SET), -1, EBADF); break; > > CASE_TEST(lseek_0); EXPECT_SYSER(1, lseek(0, 0, SEEK_SET), -1, ESPIPE); break; > > CASE_TEST(mkdir_root); EXPECT_SYSER(1, mkdir("/", 0755), -1, EEXIST); break; > > + CASE_TEST(mmap_bad); EXPECT_PTRER(1, mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL); break; > > + CASE_TEST(munmap_bad); EXPECT_SYSER(1, munmap((void *)-1, 0), -1, EINVAL); break; > > + CASE_TEST(mmap_good); EXPECT_SYSZR(1, test_mmap_munmap(4*1024)); break; > > CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; > > CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; > > CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break; > > -- > > 2.25.1 > >
Hi Zhangjin, On 2023-06-23 03:32:49+0800, Zhangjin Wu wrote: > > On 2023-06-19 23:55:41+0800, Zhangjin Wu wrote: > > > Three mmap/munmap related test cases are added: > > > > > > - mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL) > > > > > > The length argument must be greater than 0, otherwise, fail with -EINVAL. > > > > > > - munmap((void *)-1, 4*1024), -1, EINVAL) > > > > > > Invalid (void *)-1 address fail with -EINVAL. > > > > > > - test_mmap_munmap(4*1024) > > > > > > It finds a init file, mmap() it and then munmap(). > > > > > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org> > > > --- > > > tools/testing/selftests/nolibc/nolibc-test.c | 31 ++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c > > > index 80ab29e2887c..f7c0ca72cb28 100644 > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c > > > @@ -592,6 +592,34 @@ static int test_stat_timestamps(void) > > > return 0; > > > } > > > > > > +int test_mmap_munmap(int size) > > > +{ > > > + char init_files[5][20] = {"/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh"}; > > > > Why not /proc/1/exe or even /proc/self/exe? > > > > I know your other series tries to remove the procfs dependency, but > > we're not there yet :-). > > > > Yeah, '/proc/self/exe' is a choice, if so, the 'has_proc' should be added ;-) Currently procfs is a hard requirement. So I would leave 'has_proc' to the other series that may change this. > > Also does it make sense to pass a size parameter? > > Why not use either PAGE_SIZE or the real size of the binary from > > fstat(). > > > > Ok, as the manpage of mmap shows: > > For mmap(), offset must be a multiple of the underlying huge page > size. The system automatically aligns length to be a multiple of > the underlying huge page size. > > For munmap(), addr, and length must both be a multiple of the > underlying huge page size. > > perhaps we should do further tests: > > * the real size/length > * even > the real size > * the PAGE_SIZE > * not aligned with PAGE_SIZE > > If such tests are required, the 'size' and even 'offset' arguments could be > provided to cover different combination or we do such tests internally, then, > the arguments are not required. I think task of nolibc-test is to test the code in nolibc itself. The custom mmap implementation is trivial and directly calls the syscall. These additionally proposed tests would effectively test the core kernels implementation of mmap() and not the code of nolibc. Therefore I don't think they are necessary in nolibc-test and the functionality is hopefully already be tested in another testsuite. Note: Testing mmap is indeed useful to test the implementation of my_syscall6() by providing a bogus value in the 'offset' parameter. I think we do have such a testcase. <snip> Thomas
Hi, Thomas > Hi Zhangjin, > > On 2023-06-23 03:32:49+0800, Zhangjin Wu wrote: > > > On 2023-06-19 23:55:41+0800, Zhangjin Wu wrote: > > > > Three mmap/munmap related test cases are added: > > > > (snipped) > > > > > > > > +int test_mmap_munmap(int size) > > > > +{ > > > > + char init_files[5][20] = {"/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh"}; > > > > > > Why not /proc/1/exe or even /proc/self/exe? > > > > > > I know your other series tries to remove the procfs dependency, but > > > we're not there yet :-). > > > > > > > Yeah, '/proc/self/exe' is a choice, if so, the 'has_proc' should be added ;-) > > Currently procfs is a hard requirement. So I would leave 'has_proc' to > the other series that may change this. > Yeah, but to be consistent with the already existing 'proc' condition check, 'proc' will be used at first ;-) $ grep '(proc,' -ur tools/testing/selftests/nolibc/nolibc-test.c CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break; CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break; CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break; CASE_TEST(chroot_exe); EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break; CASE_TEST(link_cross); EXPECT_SYSER(proc, link("/proc/self/net", "/blah"), -1, EXDEV); break; Btw, for the /proc/self used in test_stat_timestamps, in the revision of the 'minimal' config support series, instead of using '/', a 'proc' should be added like above test cases. > > > Also does it make sense to pass a size parameter? > > > Why not use either PAGE_SIZE or the real size of the binary from > > > fstat(). > > > > > > > Ok, as the manpage of mmap shows: > > > > For mmap(), offset must be a multiple of the underlying huge page > > size. The system automatically aligns length to be a multiple of > > the underlying huge page size. > > > > For munmap(), addr, and length must both be a multiple of the > > underlying huge page size. > > > > perhaps we should do further tests: > > > > * the real size/length > > * even > the real size > > * the PAGE_SIZE > > * not aligned with PAGE_SIZE > > > > If such tests are required, the 'size' and even 'offset' arguments could be > > provided to cover different combination or we do such tests internally, then, > > the arguments are not required. > > I think task of nolibc-test is to test the code in nolibc itself. > The custom mmap implementation is trivial and directly calls the > syscall. These additionally proposed tests would effectively test the > core kernels implementation of mmap() and not the code of nolibc. > > Therefore I don't think they are necessary in nolibc-test and the > functionality is hopefully already be tested in another testsuite. > Ok, it is reasonable. > > Note: > > Testing mmap is indeed useful to test the implementation of > my_syscall6() by providing a bogus value in the 'offset' parameter. > I think we do have such a testcase. > Ok, we can pass a valid offset (n*PAGE_SIZE) to mmap() in test_mmap_munmap() or add a whole new mmap_offset test case with a PAGE_SIZE not aligned offset (such as 1). Thanks, Zhangjin > <snip> > > Thomas
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 80ab29e2887c..f7c0ca72cb28 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -592,6 +592,34 @@ static int test_stat_timestamps(void) return 0; } +int test_mmap_munmap(int size) +{ + char init_files[5][20] = {"/init", "/sbin/init", "/etc/init", "/bin/init", "/bin/sh"}; + int ret, fd, i; + void *mem; + + for (i = 0; i < 5; i++) { + ret = fd = open(init_files[i], O_RDONLY); + if (ret < 0) + continue; + else + break; + } + if (ret < 0) + return ret; + + mem = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0); + if (mem == MAP_FAILED) + return -1; + + ret = munmap(mem, size); + if (ret < 0) + return ret; + + return close(fd); +} + + /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. */ @@ -666,6 +694,9 @@ int run_syscall(int min, int max) CASE_TEST(lseek_m1); EXPECT_SYSER(1, lseek(-1, 0, SEEK_SET), -1, EBADF); break; CASE_TEST(lseek_0); EXPECT_SYSER(1, lseek(0, 0, SEEK_SET), -1, ESPIPE); break; CASE_TEST(mkdir_root); EXPECT_SYSER(1, mkdir("/", 0755), -1, EEXIST); break; + CASE_TEST(mmap_bad); EXPECT_PTRER(1, mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL); break; + CASE_TEST(munmap_bad); EXPECT_SYSER(1, munmap((void *)-1, 0), -1, EINVAL); break; + CASE_TEST(mmap_good); EXPECT_SYSZR(1, test_mmap_munmap(4*1024)); break; CASE_TEST(open_tty); EXPECT_SYSNE(1, tmp = open("/dev/null", 0), -1); if (tmp != -1) close(tmp); break; CASE_TEST(open_blah); EXPECT_SYSER(1, tmp = open("/proc/self/blah", 0), -1, ENOENT); if (tmp != -1) close(tmp); break; CASE_TEST(poll_null); EXPECT_SYSZR(1, poll(NULL, 0, 0)); break;
Three mmap/munmap related test cases are added: - mmap(NULL, 0, PROT_READ, MAP_PRIVATE, 0, 0), MAP_FAILED, EINVAL) The length argument must be greater than 0, otherwise, fail with -EINVAL. - munmap((void *)-1, 4*1024), -1, EINVAL) Invalid (void *)-1 address fail with -EINVAL. - test_mmap_munmap(4*1024) It finds a init file, mmap() it and then munmap(). Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/testing/selftests/nolibc/nolibc-test.c | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+)