| Message ID | b404edc9a25e9ec09fc975b1bac1947c5eb0408a.1635849607.git.repnop@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Test support for new fanotify FAN_REPORT_PIDFD feature | expand |
On Tue, Nov 2, 2021 at 12:57 PM Matthew Bobrowski <repnop@google.com> wrote: > > This test ensures that the fanotify API returns the expected error > status code -EINVAL when an invalid flag is supplied alongside the new > FAN_REPORT_PIDFD initialization flag. Currently, FAN_REPORT_TID is the > only initialization flag that is not permitted in conjunction with > FAN_REPORT_PIDFD, so we explicitly provide test coverage for this. > > We also add an extra trivial test case to ensure that the > initialization behavior with the other FAN_REPORT_* related flags is > working as intended. > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > Changes since v1: > - Introduced a new macro > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL() that is > responsible for testing whether the supplied initialization flags > are supported by the underlying kernel. This is used from > do_setup(). Using this is less ambiguous then using something like > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(). Not like this. Please start your branch with the first two prep patches from Gabriel's LTP post (including my reviewed-by tag) preserving Gabriel's authorship and signed-of-by and adding your own signed-off-by. Your LTP tests are expected to be merged before Gabriel's test because your tests are for a 5.15 feature. Once your tests are merge, Gabriel would be able to rebase on master and drop his prep patches. Thanks, Amir.
On Tue, Nov 02, 2021 at 01:02:48PM +0200, Amir Goldstein wrote: > On Tue, Nov 2, 2021 at 12:57 PM Matthew Bobrowski <repnop@google.com> wrote: > > > > This test ensures that the fanotify API returns the expected error > > status code -EINVAL when an invalid flag is supplied alongside the new > > FAN_REPORT_PIDFD initialization flag. Currently, FAN_REPORT_TID is the > > only initialization flag that is not permitted in conjunction with > > FAN_REPORT_PIDFD, so we explicitly provide test coverage for this. > > > > We also add an extra trivial test case to ensure that the > > initialization behavior with the other FAN_REPORT_* related flags is > > working as intended. > > > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > --- > > Changes since v1: > > - Introduced a new macro > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL() that is > > responsible for testing whether the supplied initialization flags > > are supported by the underlying kernel. This is used from > > do_setup(). Using this is less ambiguous then using something like > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(). > > Not like this. > Please start your branch with the first two prep patches from > Gabriel's LTP post (including my reviewed-by tag) preserving > Gabriel's authorship and signed-of-by and adding your own > signed-off-by. > > Your LTP tests are expected to be merged before Gabriel's test > because your tests are for a 5.15 feature. > Once your tests are merge, Gabriel would be able to rebase on master > and drop his prep patches. Am I reading all the words, or only some of the words? AFAICT, the macro that I've defined here is different to that of what has been implemented in Gabriel's series. /M
On Tue, Nov 2, 2021 at 1:16 PM Matthew Bobrowski <repnop@google.com> wrote: > > On Tue, Nov 02, 2021 at 01:02:48PM +0200, Amir Goldstein wrote: > > On Tue, Nov 2, 2021 at 12:57 PM Matthew Bobrowski <repnop@google.com> wrote: > > > > > > This test ensures that the fanotify API returns the expected error > > > status code -EINVAL when an invalid flag is supplied alongside the new > > > FAN_REPORT_PIDFD initialization flag. Currently, FAN_REPORT_TID is the > > > only initialization flag that is not permitted in conjunction with > > > FAN_REPORT_PIDFD, so we explicitly provide test coverage for this. > > > > > > We also add an extra trivial test case to ensure that the > > > initialization behavior with the other FAN_REPORT_* related flags is > > > working as intended. > > > > > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > Changes since v1: > > > - Introduced a new macro > > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL() that is > > > responsible for testing whether the supplied initialization flags > > > are supported by the underlying kernel. This is used from > > > do_setup(). Using this is less ambiguous then using something like > > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(). > > > > Not like this. > > Please start your branch with the first two prep patches from > > Gabriel's LTP post (including my reviewed-by tag) preserving > > Gabriel's authorship and signed-of-by and adding your own > > signed-off-by. > > > > Your LTP tests are expected to be merged before Gabriel's test > > because your tests are for a 5.15 feature. > > Once your tests are merge, Gabriel would be able to rebase on master > > and drop his prep patches. > > Am I reading all the words, or only some of the words? > > AFAICT, the macro that I've defined here is different to that of what > has been implemented in Gabriel's series. > My bad. I wasn't paying attention to the difference. It wouldn't hurt to split the macro patch anyway. Thanks, Amir.
On Tue, Nov 02, 2021 at 02:15:51PM +0200, Amir Goldstein wrote: > On Tue, Nov 2, 2021 at 1:16 PM Matthew Bobrowski <repnop@google.com> wrote: > > > > On Tue, Nov 02, 2021 at 01:02:48PM +0200, Amir Goldstein wrote: > > > On Tue, Nov 2, 2021 at 12:57 PM Matthew Bobrowski <repnop@google.com> wrote: > > > > > > > > This test ensures that the fanotify API returns the expected error > > > > status code -EINVAL when an invalid flag is supplied alongside the new > > > > FAN_REPORT_PIDFD initialization flag. Currently, FAN_REPORT_TID is the > > > > only initialization flag that is not permitted in conjunction with > > > > FAN_REPORT_PIDFD, so we explicitly provide test coverage for this. > > > > > > > > We also add an extra trivial test case to ensure that the > > > > initialization behavior with the other FAN_REPORT_* related flags is > > > > working as intended. > > > > > > > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > Changes since v1: > > > > - Introduced a new macro > > > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL() that is > > > > responsible for testing whether the supplied initialization flags > > > > are supported by the underlying kernel. This is used from > > > > do_setup(). Using this is less ambiguous then using something like > > > > REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(). > > > > > > Not like this. > > > Please start your branch with the first two prep patches from > > > Gabriel's LTP post (including my reviewed-by tag) preserving > > > Gabriel's authorship and signed-of-by and adding your own > > > signed-off-by. > > > > > > Your LTP tests are expected to be merged before Gabriel's test > > > because your tests are for a 5.15 feature. > > > Once your tests are merge, Gabriel would be able to rebase on master > > > and drop his prep patches. > > > > Am I reading all the words, or only some of the words? > > > > AFAICT, the macro that I've defined here is different to that of what > > has been implemented in Gabriel's series. > > > > My bad. > I wasn't paying attention to the difference. > It wouldn't hurt to split the macro patch anyway. Right, I'll split and repost the series. Thanks for the review! /M
diff --git a/configure.ac b/configure.ac index 5bf3c52ec..b62ec5e15 100644 --- a/configure.ac +++ b/configure.ac @@ -159,7 +159,7 @@ AC_CHECK_MEMBERS([struct utsname.domainname],,,[ AC_CHECK_TYPES([enum kcmp_type],,,[#include <linux/kcmp.h>]) AC_CHECK_TYPES([struct acct_v3],,,[#include <sys/acct.h>]) AC_CHECK_TYPES([struct af_alg_iv, struct sockaddr_alg],,,[# include <linux/if_alg.h>]) -AC_CHECK_TYPES([struct fanotify_event_info_fid, struct fanotify_event_info_header],,,[#include <sys/fanotify.h>]) +AC_CHECK_TYPES([struct fanotify_event_info_fid, struct fanotify_event_info_header, struct fanotify_event_info_pidfd],,,[#include <sys/fanotify.h>]) AC_CHECK_TYPES([struct file_dedupe_range],,,[#include <linux/fs.h>]) AC_CHECK_TYPES([struct file_handle],,,[ diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore index 9554b16b1..c99e6fff7 100644 --- a/testcases/kernel/syscalls/fanotify/.gitignore +++ b/testcases/kernel/syscalls/fanotify/.gitignore @@ -17,4 +17,5 @@ /fanotify17 /fanotify18 /fanotify19 +/fanotify20 /fanotify_child diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h index a2be18338..c91162d97 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify.h +++ b/testcases/kernel/syscalls/fanotify/fanotify.h @@ -78,6 +78,9 @@ static inline int safe_fanotify_mark(const char *file, const int lineno, #define FAN_REPORT_NAME 0x00000800 #define FAN_REPORT_DFID_NAME (FAN_REPORT_DIR_FID | FAN_REPORT_NAME) #endif +#ifndef FAN_REPORT_PIDFD +#define FAN_REPORT_PIDFD 0x00000080 +#endif /* Non-uapi convenience macros */ #ifndef FAN_REPORT_DFID_NAME_FID @@ -125,6 +128,14 @@ static inline int safe_fanotify_mark(const char *file, const int lineno, #define FAN_OPEN_EXEC_PERM 0x00040000 #endif +/* Additional error status codes that can be returned to userspace */ +#ifndef FAN_NOPIDFD +#define FAN_NOPIDFD -1 +#endif +#ifndef FAN_EPIDFD +#define FAN_EPIDFD -2 +#endif + /* Flags required for unprivileged user group */ #define FANOTIFY_REQUIRED_USER_INIT_FLAGS (FAN_REPORT_FID) @@ -164,6 +175,9 @@ typedef struct { #ifndef FAN_EVENT_INFO_TYPE_DFID #define FAN_EVENT_INFO_TYPE_DFID 3 #endif +#ifndef FAN_EVENT_INFO_TYPE_PIDFD +#define FAN_EVENT_INFO_TYPE_PIDFD 4 +#endif #ifndef HAVE_STRUCT_FANOTIFY_EVENT_INFO_HEADER struct fanotify_event_info_header { @@ -181,6 +195,13 @@ struct fanotify_event_info_fid { }; #endif /* HAVE_STRUCT_FANOTIFY_EVENT_INFO_FID */ +#ifndef HAVE_STRUCT_FANOTIFY_EVENT_INFO_PIDFD +struct fanotify_event_info_pidfd { + struct fanotify_event_info_header hdr; + int32_t pidfd; +}; +#endif /* HAVE_STRUCT_FANOTIFY_EVENT_INFO_PIDFD */ + /* NOTE: only for struct fanotify_event_info_fid */ #ifdef HAVE_STRUCT_FANOTIFY_EVENT_INFO_FID_FSID___VAL # define FSID_VAL_MEMBER(fsid, i) (fsid.__val[i]) @@ -352,6 +373,11 @@ static inline void fanotify_init_flags_err_msg(const char *flags_str, fanotify_init_flags_supported_on_fs(flags, fname)); \ } while (0) +#define REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL(flags) do { \ + fanotify_init_flags_err_msg(#flags, __FILE__, __LINE__, tst_brk_, \ + fanotify_init_flags_supported_by_kernel(flags)); \ + } while (0) + static inline int fanotify_mark_supported_by_kernel(uint64_t flag) { int fd; diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c new file mode 100644 index 000000000..9960d85eb --- /dev/null +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2021 Google. All Rights Reserved. + * + * Started by Matthew Bobrowski <repnop@google.com> + */ + +/*\ + * [Description] + * + * This source file contains a test case which ensures that the fanotify API + * returns an expected error code when provided an invalid initialization flag + * alongside FAN_REPORT_PIDFD. Additionally, it checks that the operability with + * existing FAN_REPORT_* flags is maintained and functioning as intended. + */ + +#define _GNU_SOURCE +#include "tst_test.h" +#include <errno.h> + +#ifdef HAVE_SYS_FANOTIFY_H +#include "fanotify.h" + +#define MOUNT_PATH "fs_mnt" + +static int fanotify_fd; + +static struct test_case_t { + char *name; + unsigned int init_flags; + int want_err; + int want_errno; +} test_cases[] = { + { + "fail on FAN_REPORT_PIDFD | FAN_REPORT_TID", + FAN_REPORT_PIDFD | FAN_REPORT_TID, + 1, + EINVAL, + }, + { + "pass on FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME", + FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME , + 0, + 0, + }, +}; + +static void do_setup(void) +{ + /* + * An explicit check for FAN_REPORT_PIDFD is performed early on in the + * test initialization as it's a prerequisite for all test cases. + */ + REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL(FAN_REPORT_PIDFD); +} + +static void do_test(unsigned int num) +{ + struct test_case_t *tc = &test_cases[num]; + + tst_res(TINFO, "Test #%d: %s", num, tc->name); + + fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY); + if (fanotify_fd < 0) { + if (!tc->want_err) { + tst_res(TFAIL, + "fanotify_fd=%d, fanotify_init(%x, O_RDONLY) " + "failed with error -%d but wanted success", + fanotify_fd, tc->init_flags, errno); + return; + } + + if (errno != tc->want_errno) { + tst_res(TFAIL, + "fanotify_fd=%d, fanotify_init(%x, O_RDONLY) " + "failed with an unexpected error code -%d but " + "wanted -%d", + fanotify_fd, tc->init_flags, + errno, tc->want_errno); + return; + } + + tst_res(TPASS, + "fanotify_fd=%d, fanotify_init(%x, O_RDONLY) " + "failed with error -%d as expected", + fanotify_fd, tc->init_flags, errno); + return; + } + + /* + * Catch test cases that had expected to receive an error upon calling + * fanotify_init() but had unexpectedly resulted in a success. + */ + if (tc->want_err) { + tst_res(TFAIL, + "fanotify_fd=%d, fanotify_init(%x, O_RDONLY) " + "unexpectedly returned successfully, wanted error -%d", + fanotify_fd, tc->init_flags, tc->want_errno); + return; + } + + tst_res(TPASS, + "fanotify_fd=%d, fanotify_init(%x, O_RDONLY) " + "successfully initialized notification group", + fanotify_fd, tc->init_flags); + + SAFE_CLOSE(fanotify_fd); +} + +static void do_cleanup(void) +{ + if (fanotify_fd >= 0) + SAFE_CLOSE(fanotify_fd); +} + +static struct tst_test test = { + .setup = do_setup, + .test = do_test, + .tcnt = ARRAY_SIZE(test_cases), + .cleanup = do_cleanup, + .all_filesystems = 1, + .needs_root = 1, + .mntpoint = MOUNT_PATH, +}; + +#else + TST_TEST_TCONF("system doesn't have required fanotify support"); +#endif /* HAVE_SYS_FANOTIFY_H */