| Message ID | 1645005868-2373-5-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] kcmp.h: move it to ltp include/lapi directory | expand |
Hi Xu, Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks! Few comments below, can be fixed before merge. > +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore > @@ -1 +1,2 @@ > pidfd_getfd01 > +pidfd_getfd02 Again /pidfd_getfd02 > diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c ... > +/*\ > + * [Description] > + * > + * Tests basic error handling of the pidfd_open syscall. > + * > + * - EBADF pidfd is not a valid PID file descriptor > + * - EBADF targetfd is not an open file descriptor in the process referred > + * to by pidfd > + * - EINVAL flags is not 0 > + * - ESRCH the process referred to by pidfd does not exist(it has terminated and > + * been waited on)) nit: add space and remove redundant bracket * - ESRCH the process referred to by pidfd does not exist (it has terminated and * been waited on) > + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions > + * over the process referred to by pidfd +1 (only ENFILE "The system-wide limit on the total number of open files has been reached." which is probably not worth of implementing). ... > +static void setup(void) > +{ > + struct passwd *pw; > + > + pw = SAFE_GETPWNAM("nobody"); > + uid = pw->pw_uid; > + > + pidfd_open_supported(); > + pidfd_getfd_supported(); nit: I'd put these before SAFE_GETPWNAM(). > + > + TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open"); If you wait for Cyril's patch adding auto stringification https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/ you can use just: TST_EXP_FD_SILENT(pidfd_open(getpid(), 0)); and get more info. > + if (!TST_PASS) > + tst_brk(TFAIL | TTERRNO, "pidfd_open failed"); @Cyril: would it be possible to to allow using also tst_brk() in macros in tst_test_macros.h? Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated > + valid_pidfd = TST_RET; > +} > + > +static void run(unsigned int n) > +{ > + struct tcase *tc = &tcases[n]; > + int pid; > + > + switch (tc->exp_errno) { > + case EPERM: > + pid = SAFE_FORK(); SAFE_FORK() can be before switch. > + if (!pid) { > + SAFE_SETUID(uid); > + TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags), > + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", > + valid_pidfd, tc->targetfd, tc->flags, tc->name); > + TST_CHECKPOINT_WAKE(0); > + exit(0); > + } > + TST_CHECKPOINT_WAIT(0); > + SAFE_WAIT(NULL); > + return; > + break; > + case ESRCH: > + pid = SAFE_FORK(); > + if (!pid) { > + TST_CHECKPOINT_WAIT(0); > + exit(0); > + } > + TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open"); > + *tc->pidfd = TST_RET; > + TST_CHECKPOINT_WAKE(0); > + SAFE_WAIT(NULL); > + break; > + default: > + break; > + }; IMHO more readable would be instead of switch use if/else if or 2 functions. Kind regards, Petr > + > + TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags), > + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", > + *tc->pidfd, tc->targetfd, tc->flags, tc->name); > +}
Hi Petr > Hi Xu, > > Reviewed-by: Petr Vorel<pvorel@suse.cz> > > Thanks! Few comments below, can be fixed before merge. > >> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore >> @@ -1 +1,2 @@ >> pidfd_getfd01 >> +pidfd_getfd02 > Again /pidfd_getfd02 > >> diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c > ... >> +/*\ >> + * [Description] >> + * >> + * Tests basic error handling of the pidfd_open syscall. >> + * >> + * - EBADF pidfd is not a valid PID file descriptor >> + * - EBADF targetfd is not an open file descriptor in the process referred >> + * to by pidfd >> + * - EINVAL flags is not 0 >> + * - ESRCH the process referred to by pidfd does not exist(it has terminated and >> + * been waited on)) > > nit: add space and remove redundant bracket > * - ESRCH the process referred to by pidfd does not exist (it has terminated and > * been waited on) OK. > >> + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions >> + * over the process referred to by pidfd > > +1 (only ENFILE "The system-wide limit on the total number of open files has been > reached." which is probably not worth of implementing). > ... > >> +static void setup(void) >> +{ >> + struct passwd *pw; >> + >> + pw = SAFE_GETPWNAM("nobody"); >> + uid = pw->pw_uid; >> + >> + pidfd_open_supported(); >> + pidfd_getfd_supported(); > nit: I'd put these before SAFE_GETPWNAM(). OK. > >> + >> + TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open"); > If you wait for Cyril's patch adding auto stringification > https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/ > > you can use just: > TST_EXP_FD_SILENT(pidfd_open(getpid(), 0)); > > and get more info. I will look Cyril's patch and wait. > >> + if (!TST_PASS) >> + tst_brk(TFAIL | TTERRNO, "pidfd_open failed"); > > @Cyril: would it be possible to to allow using also tst_brk() in macros in > tst_test_macros.h? > Maybe can add SAFE_PIDFD_OPEN. > Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated > >> + valid_pidfd = TST_RET; >> +} >> + >> +static void run(unsigned int n) >> +{ >> + struct tcase *tc =&tcases[n]; >> + int pid; >> + >> + switch (tc->exp_errno) { >> + case EPERM: >> + pid = SAFE_FORK(); > SAFE_FORK() can be before switch. > >> + if (!pid) { >> + SAFE_SETUID(uid); >> + TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags), >> + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", >> + valid_pidfd, tc->targetfd, tc->flags, tc->name); >> + TST_CHECKPOINT_WAKE(0); >> + exit(0); >> + } >> + TST_CHECKPOINT_WAIT(0); >> + SAFE_WAIT(NULL); >> + return; >> + break; >> + case ESRCH: >> + pid = SAFE_FORK(); >> + if (!pid) { >> + TST_CHECKPOINT_WAIT(0); >> + exit(0); >> + } >> + TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open"); >> + *tc->pidfd = TST_RET; >> + TST_CHECKPOINT_WAKE(0); >> + SAFE_WAIT(NULL); >> + break; >> + default: >> + break; >> + }; > > IMHO more readable would be instead of switch use if/else if or 2 functions. > Will try. Best Regards Yang Xu > > Kind regards, > Petr > >> + >> + TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags), >> + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", >> + *tc->pidfd, tc->targetfd, tc->flags, tc->name); >> +}
Hi Xu, > > and get more info. > I will look Cyril's patch and wait. Thx! > >> + if (!TST_PASS) > >> + tst_brk(TFAIL | TTERRNO, "pidfd_open failed"); > > @Cyril: would it be possible to to allow using also tst_brk() in macros in > > tst_test_macros.h? > Maybe can add SAFE_PIDFD_OPEN. I was thinking about it. But you use TFAIL - part of testing. But using just SAFE_PIDFD_OPEN() even it's using TBROK might be best. Kind regards, Petr > > Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated
Hi Petr > Hi Xu, > >>> and get more info. >> I will look Cyril's patch and wait. > Thx! The Cyril's patch seems only a single patch and doesn't affect the other TST_TEST macros. So how I just only use TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));? > >>>> + if (!TST_PASS) >>>> + tst_brk(TFAIL | TTERRNO, "pidfd_open failed"); > >>> @Cyril: would it be possible to to allow using also tst_brk() in macros in >>> tst_test_macros.h? > >> Maybe can add SAFE_PIDFD_OPEN. > I was thinking about it. But you use TFAIL - part of testing. > But using just SAFE_PIDFD_OPEN() even it's using TBROK might be best. My v2 just keep this. ps: I also remove useless static expire_pidfd because we can use TST_RET as pidfd when tesing ESRCH error. Best Regards Yang Xu > > Kind regards, > Petr > >>> Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated
Hi Xu, ... > >> + TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open"); > > If you wait for Cyril's patch adding auto stringification > > https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/ > > you can use just: > > TST_EXP_FD_SILENT(pidfd_open(getpid(), 0)); > > and get more info. > I will look Cyril's patch and wait. FYI Cyril is not planning to merge that patch, replaced by https://lore.kernel.org/ltp/20220218103413.18540-1-chrubis@suse.cz/ But I'd still drop "pidfd_open", because pidfd_getfd02.c:55: TFAIL: pidfd_open(getpid(), -9) failed: EINVAL (22) is better than: pidfd_getfd02.c:55: TFAIL: pidfd_open failed: EINVAL (22) But as fanotify21.c also needs SAFE_PIDFD_OPEN() (and more tests will come in the future I'd vote for adding SAFE_PIDFD_OPEN() as you suggested. Kind regards, Petr
Hi Petr > Hi Xu, > > ... >>>> + TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open"); >>> If you wait for Cyril's patch adding auto stringification >>> https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/ > >>> you can use just: >>> TST_EXP_FD_SILENT(pidfd_open(getpid(), 0)); > >>> and get more info. >> I will look Cyril's patch and wait. > > FYI Cyril is not planning to merge that patch, replaced by > https://lore.kernel.org/ltp/20220218103413.18540-1-chrubis@suse.cz/ > > But I'd still drop "pidfd_open", because > pidfd_getfd02.c:55: TFAIL: pidfd_open(getpid(), -9) failed: EINVAL (22) > is better than: > pidfd_getfd02.c:55: TFAIL: pidfd_open failed: EINVAL (22) > > But as fanotify21.c also needs SAFE_PIDFD_OPEN() (and more tests will come in > the future I'd vote for adding SAFE_PIDFD_OPEN() as you suggested. I will add SAFE_PIDFD_OPEN into lapi/pidfd_open.h. But it seems kernel doesn't have pidfd_getfd.h/pidfd_send_signal.h/pidfd_open.h, I think we can merge them into lapi/pidfd.h. So in the future, we can introduce other pidfd macro and case wants to use these pidfd macro they just only include one header(lapi/pidfd.h). What do you think about this? Best Regards Yang Xu > > Kind regards, > Petr
diff --git a/runtest/syscalls b/runtest/syscalls index 6155712cc..436cc2a0a 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -952,6 +952,7 @@ personality01 personality01 personality02 personality02 pidfd_getfd01 pidfd_getfd01 +pidfd_getfd02 pidfd_getfd02 pidfd_open01 pidfd_open01 pidfd_open02 pidfd_open02 diff --git a/testcases/kernel/syscalls/pidfd_getfd/.gitignore b/testcases/kernel/syscalls/pidfd_getfd/.gitignore index c193ffee7..f944adc6f 100644 --- a/testcases/kernel/syscalls/pidfd_getfd/.gitignore +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore @@ -1 +1,2 @@ pidfd_getfd01 +pidfd_getfd02 diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c new file mode 100644 index 000000000..e4f5a1467 --- /dev/null +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved. + * Author: Yang Xu <xuyang2018.jy@fujitsu.com> + */ + +/*\ + * [Description] + * + * Tests basic error handling of the pidfd_open syscall. + * + * - EBADF pidfd is not a valid PID file descriptor + * - EBADF targetfd is not an open file descriptor in the process referred + * to by pidfd + * - EINVAL flags is not 0 + * - ESRCH the process referred to by pidfd does not exist(it has terminated and + * been waited on)) + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions + * over the process referred to by pidfd + */ + +#include <stdlib.h> +#include <pwd.h> +#include "tst_test.h" +#include "lapi/pidfd_open.h" +#include "lapi/pidfd_getfd.h" + +static int valid_pidfd, expire_pidfd, invalid_pidfd = -1; +static uid_t uid; + +static struct tcase { + char *name; + int *pidfd; + int targetfd; + int flags; + int exp_errno; +} tcases[] = { + {"invalid pidfd", &invalid_pidfd, 0, 0, EBADF}, + {"invalid targetfd", &valid_pidfd, -1, 0, EBADF}, + {"invalid flags", &valid_pidfd, 0, 1, EINVAL}, + {"the process referred to by pidfd doesn't exist", &expire_pidfd, 0, 0, ESRCH}, + {"lack of required permission", &valid_pidfd, 0, 0, EPERM}, +}; + +static void setup(void) +{ + struct passwd *pw; + + pw = SAFE_GETPWNAM("nobody"); + uid = pw->pw_uid; + + pidfd_open_supported(); + pidfd_getfd_supported(); + + TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open"); + if (!TST_PASS) + tst_brk(TFAIL | TTERRNO, "pidfd_open failed"); + valid_pidfd = TST_RET; +} + +static void run(unsigned int n) +{ + struct tcase *tc = &tcases[n]; + int pid; + + switch (tc->exp_errno) { + case EPERM: + pid = SAFE_FORK(); + if (!pid) { + SAFE_SETUID(uid); + TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags), + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", + valid_pidfd, tc->targetfd, tc->flags, tc->name); + TST_CHECKPOINT_WAKE(0); + exit(0); + } + TST_CHECKPOINT_WAIT(0); + SAFE_WAIT(NULL); + return; + break; + case ESRCH: + pid = SAFE_FORK(); + if (!pid) { + TST_CHECKPOINT_WAIT(0); + exit(0); + } + TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open"); + *tc->pidfd = TST_RET; + TST_CHECKPOINT_WAKE(0); + SAFE_WAIT(NULL); + break; + default: + break; + }; + + TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags), + tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s", + *tc->pidfd, tc->targetfd, tc->flags, tc->name); +} + +static struct tst_test test = { + .tcnt = ARRAY_SIZE(tcases), + .test = run, + .setup = setup, + .needs_root = 1, + .forks_child = 1, + .needs_checkpoints = 1, +};
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- runtest/syscalls | 1 + .../kernel/syscalls/pidfd_getfd/.gitignore | 1 + .../syscalls/pidfd_getfd/pidfd_getfd02.c | 108 ++++++++++++++++++ 3 files changed, 110 insertions(+) create mode 100644 testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c