| Message ID | 1692700900-13521-2-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] syscalls/faccessat201: Add new testcase | expand |
Hi! > +static struct passwd *ltpuser; > + > +static struct tcase { > + int *fd; > + char **filename; > + int mode; > + int flags; > + int exp_errno; > +} tcases[] = { > + {&atcwd_fd, &bad_path, R_OK, 0, EFAULT}, > + {&atcwd_fd, &rel_path, R_OK, -1, EINVAL}, > + {&atcwd_fd, &rel_path, -1, 0, EINVAL}, > + {&bad_fd, &rel_path, R_OK, 0, EBADF}, > + {&fd, &rel_path, R_OK, 0, ENOTDIR}, > + {&atcwd_fd, &rel_path, R_OK, AT_EACCESS, EACCES}, > +}; > + > +static void verify_faccessat2(unsigned int i) > +{ > + struct tcase *tc = &tcases[i]; > + > + if (tc->exp_errno == EACCES) { > + if (SAFE_FORK() == 0) { > + SAFE_SETUID(ltpuser->pw_uid); ^ Should be SAFE_SETEUID() right? Because the AT_EACESS causes the call to use EUID instead of UID so we have to change only the EUID and keep the UID to root UID. And with that we can drop the SAFE_FORK() since we can change EUID back as long as UID is priviledged, so the code should be: if (tc->exp_errno == EACESS) SAFE_SETEUID(ltpuser->pw_uid); TST_EXP_FAIL(...); if (tc->exp_errno == EACESS) SAFE_SETEUID(ltpuser->pw_uid); > + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, > + tc->mode, tc->flags), tc->exp_errno); > + } > + > + tst_reap_children(); > + } else { > + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, > + tc->mode, tc->flags), tc->exp_errno); Can we get a better message here? As it is it prints "faccessat2(*tc->fd, *tc->filename, tc->mode, tc->flags) ..." Which is a bit ugly. > + } > +} > + > +static void setup(void) > +{ > + SAFE_MKDIR(TESTDIR, 0666); > + SAFE_TOUCH(RELPATH, 0444, NULL); > + > + fd = SAFE_OPEN(RELPATH, O_RDONLY); > + bad_path = tst_get_bad_addr(NULL); > + > + ltpuser = SAFE_GETPWNAM(TESTUSER); > +} > + > +static void cleanup(void) > +{ > + if (fd > -1) > + SAFE_CLOSE(fd); > +} > + > +static struct tst_test test = { > + .test = verify_faccessat2, > + .tcnt = ARRAY_SIZE(tcases), > + .setup = setup, > + .cleanup = cleanup, > + .bufs = (struct tst_buffers []) { > + {&rel_path, .str = RELPATH}, > + {}, > + }, > + .needs_tmpdir = 1, > + .needs_root = 1, > + .forks_child = 1, > +}; > -- > 2.39.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi Cyril > Hi! >> +static struct passwd *ltpuser; >> + >> +static struct tcase { >> + int *fd; >> + char **filename; >> + int mode; >> + int flags; >> + int exp_errno; >> +} tcases[] = { >> + {&atcwd_fd, &bad_path, R_OK, 0, EFAULT}, >> + {&atcwd_fd, &rel_path, R_OK, -1, EINVAL}, >> + {&atcwd_fd, &rel_path, -1, 0, EINVAL}, >> + {&bad_fd, &rel_path, R_OK, 0, EBADF}, >> + {&fd, &rel_path, R_OK, 0, ENOTDIR}, >> + {&atcwd_fd, &rel_path, R_OK, AT_EACCESS, EACCES}, >> +}; >> + >> +static void verify_faccessat2(unsigned int i) >> +{ >> + struct tcase *tc = &tcases[i]; >> + >> + if (tc->exp_errno == EACCES) { >> + if (SAFE_FORK() == 0) { >> + SAFE_SETUID(ltpuser->pw_uid); > ^ > Should be SAFE_SETEUID() right? > > Because the AT_EACESS causes the call to use EUID instead of UID so we > have to change only the EUID and keep the UID to root UID. > > And with that we can drop the SAFE_FORK() since we can change EUID back > as long as UID is priviledged, so the code should be: > > > if (tc->exp_errno == EACESS) > SAFE_SETEUID(ltpuser->pw_uid); > > TST_EXP_FAIL(...); > > if (tc->exp_errno == EACESS) > SAFE_SETEUID(ltpuser->pw_uid); Thank you for your suggestion, but i think " SAFE_SETEUID(ltpuser->pw_uid)" should be changed to "SAFE_SETEUID(0)" to change EUID back. >> + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, >> + tc->mode, tc->flags), tc->exp_errno); >> + } >> + >> + tst_reap_children(); >> + } else { >> + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, >> + tc->mode, tc->flags), tc->exp_errno); > Can we get a better message here? As it is it prints > "faccessat2(*tc->fd, *tc->filename, tc->mode, tc->flags) ..." > > Which is a bit ugly. Because i used tst_get_bad_addr to test a bad pathname point , so if i output relevant information, the test will be killed by SIGSEGV. In the v3 version, I will change it to this: "TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, tc->mode, tc->flags), tc->exp_errno, "faccessat2()")" How about this? >> + } >> +} >> + >> +static void setup(void) >> +{ >> + SAFE_MKDIR(TESTDIR, 0666); >> + SAFE_TOUCH(RELPATH, 0444, NULL); >> + >> + fd = SAFE_OPEN(RELPATH, O_RDONLY); >> + bad_path = tst_get_bad_addr(NULL); >> + >> + ltpuser = SAFE_GETPWNAM(TESTUSER); >> +} >> + >> +static void cleanup(void) >> +{ >> + if (fd > -1) >> + SAFE_CLOSE(fd); >> +} >> + >> +static struct tst_test test = { >> + .test = verify_faccessat2, >> + .tcnt = ARRAY_SIZE(tcases), >> + .setup = setup, >> + .cleanup = cleanup, >> + .bufs = (struct tst_buffers []) { >> + {&rel_path, .str = RELPATH}, >> + {}, >> + }, >> + .needs_tmpdir = 1, >> + .needs_root = 1, >> + .forks_child = 1, >> +}; >> -- >> 2.39.1 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp Thanks for your review. Best Regards Yang Xu
Hi! > > Thank you for your suggestion, but i think " SAFE_SETEUID(ltpuser->pw_uid)" > > should be changed to "SAFE_SETEUID(0)" to change EUID back. Right, that was copy&paste typo. > >> + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, > >> + tc->mode, tc->flags), tc->exp_errno); > >> + } > >> + > >> + tst_reap_children(); > >> + } else { > >> + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, > >> + tc->mode, tc->flags), tc->exp_errno); > > Can we get a better message here? As it is it prints > > "faccessat2(*tc->fd, *tc->filename, tc->mode, tc->flags) ..." > > > > Which is a bit ugly. > > Because i used tst_get_bad_addr to test a bad pathname point , > > so if i output relevant information, the test will be killed by SIGSEGV. > > In the v3 version, I will change it to this: > > "TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, tc->mode, tc->flags), > > tc->exp_errno, "faccessat2()")" > > How about this? Either that or we can add short description to the test structure and pass TST_EXP_FAIL(..., "faccessat2() %s", tc->desc) At least some cases would be better with description, for instance for the euid test the description should be something as "with AT_EACCESS and unpriviledged EUID"
diff --git a/runtest/syscalls b/runtest/syscalls index 02768c236..7af2842f3 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -218,6 +218,7 @@ faccessat01 faccessat01 #faccessat2 test cases faccessat201 faccessat201 +faccessat202 faccessat202 #fallocate test cases fallocate01 fallocate01 diff --git a/testcases/kernel/syscalls/faccessat2/.gitignore b/testcases/kernel/syscalls/faccessat2/.gitignore index 53f700bac..f58f045f4 100644 --- a/testcases/kernel/syscalls/faccessat2/.gitignore +++ b/testcases/kernel/syscalls/faccessat2/.gitignore @@ -1 +1,2 @@ /faccessat201 +/faccessat202 diff --git a/testcases/kernel/syscalls/faccessat2/faccessat202.c b/testcases/kernel/syscalls/faccessat2/faccessat202.c new file mode 100644 index 000000000..607a615e4 --- /dev/null +++ b/testcases/kernel/syscalls/faccessat2/faccessat202.c @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved. + * Copyright (c) Linux Test Project, 2003-2023 + * Author: Yang Xu <xuyang2018.jy@fujitsu.com> + */ + +/*\ + * [Description] + * + * Test basic error handling of faccessat2 syscall: + * + * - faccessat2() fails with EFAULT if pathname is a bad pathname point. + * - faccessat2() fails with EINVAL if flags is -1. + * - faccessat2() fails with EINVAL if mode is -1. + * - faccessat2() fails with EBADF if dirfd is -1. + * - faccessat2() fails with ENOTDIR if pathname is relative path to a + * file and dir_fd is file descriptor for this file. + * - faccessat2() fails with EACCES if flags is AT_EACCESS and not using + * the effective user and group IDs. + * + * Minimum Linux version required is v5.8. + */ + +#include <pwd.h> + +#include "tst_test.h" +#include "lapi/syscalls.h" +#include "lapi/faccessat.h" + +#define TESTUSER "nobody" +#define TESTDIR "faccessat2dir" +#define RELPATH "faccessat2dir/faccessat2file" + +static int fd; +static int bad_fd = -1; +static int atcwd_fd = AT_FDCWD; +static char *bad_path; +static char *rel_path; + +static struct passwd *ltpuser; + +static struct tcase { + int *fd; + char **filename; + int mode; + int flags; + int exp_errno; +} tcases[] = { + {&atcwd_fd, &bad_path, R_OK, 0, EFAULT}, + {&atcwd_fd, &rel_path, R_OK, -1, EINVAL}, + {&atcwd_fd, &rel_path, -1, 0, EINVAL}, + {&bad_fd, &rel_path, R_OK, 0, EBADF}, + {&fd, &rel_path, R_OK, 0, ENOTDIR}, + {&atcwd_fd, &rel_path, R_OK, AT_EACCESS, EACCES}, +}; + +static void verify_faccessat2(unsigned int i) +{ + struct tcase *tc = &tcases[i]; + + if (tc->exp_errno == EACCES) { + if (SAFE_FORK() == 0) { + SAFE_SETUID(ltpuser->pw_uid); + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, + tc->mode, tc->flags), tc->exp_errno); + } + + tst_reap_children(); + } else { + TST_EXP_FAIL(faccessat2(*tc->fd, *tc->filename, + tc->mode, tc->flags), tc->exp_errno); + } +} + +static void setup(void) +{ + SAFE_MKDIR(TESTDIR, 0666); + SAFE_TOUCH(RELPATH, 0444, NULL); + + fd = SAFE_OPEN(RELPATH, O_RDONLY); + bad_path = tst_get_bad_addr(NULL); + + ltpuser = SAFE_GETPWNAM(TESTUSER); +} + +static void cleanup(void) +{ + if (fd > -1) + SAFE_CLOSE(fd); +} + +static struct tst_test test = { + .test = verify_faccessat2, + .tcnt = ARRAY_SIZE(tcases), + .setup = setup, + .cleanup = cleanup, + .bufs = (struct tst_buffers []) { + {&rel_path, .str = RELPATH}, + {}, + }, + .needs_tmpdir = 1, + .needs_root = 1, + .forks_child = 1, +};
Check various errnos for faccessat2(). Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- runtest/syscalls | 1 + .../kernel/syscalls/faccessat2/.gitignore | 1 + .../kernel/syscalls/faccessat2/faccessat202.c | 105 ++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 testcases/kernel/syscalls/faccessat2/faccessat202.c