[LTP] [PATCH v2 0/2] Test support for new fanotify FAN_REPORT_PIDFD feature

[LTP] [PATCH v2 0/2] Test support for new fanotify FAN_REPORT_PIDFD feature

[Matthew Bobrowski via ltp]

Hi,

This patch series introduces LTP tests for the new fanotify
FAN_REPORT_PIDFD feature which has been merged as part of the v5.15
release.

I've addressed the minor nits that came out of the first round of
review, which can be found here [0].

[0] https://lore.kernel.org/ltp/cover.1635327490.git.repnop@google.com/

Matthew Bobrowski (2):
  syscalls/fanotify20: add new test for FAN_REPORT_PIDFD
  syscalls/fanotify21: add new test checking the returned pidfd from
    fanotify in FAN_REPORT_PIDFD mode

 configure.ac                                  |   2 +-
 testcases/kernel/syscalls/fanotify/.gitignore |   2 +
 testcases/kernel/syscalls/fanotify/fanotify.h |  26 ++
 .../kernel/syscalls/fanotify/fanotify20.c     | 128 ++++++++
 .../kernel/syscalls/fanotify/fanotify21.c     | 306 ++++++++++++++++++
 5 files changed, 463 insertions(+), 1 deletion(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify20.c
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify21.c

-- 
2.33.1.1089.g2158813163f-goog

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 1/2] syscalls/fanotify20: add new test for FAN_REPORT_PIDFD

[Matthew Bobrowski via ltp]

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().

 configure.ac                                  |   2 +-
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 testcases/kernel/syscalls/fanotify/fanotify.h |  26 ++++
 .../kernel/syscalls/fanotify/fanotify20.c     | 128 ++++++++++++++++++
 4 files changed, 156 insertions(+), 1 deletion(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify20.c

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 */
-- 
2.33.1.1089.g2158813163f-goog

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/2] syscalls/fanotify21: add new test checking the returned pidfd from fanotify in FAN_REPORT_PIDFD mode

[Matthew Bobrowski via ltp]

A new test that performs verification on the values returned within
the struct fanotify_event_info_pidfd record when the notification
group has been intialized in FAN_REPORT_PIDFD mode.

Signed-off-by: Matthew Bobrowski <repnop@google.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
Changes since v1:
 - Adapted do_setup() to make use of the
   REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL() rather than open
   coding it.

 - For parsing procfs information, rather than reimplementing parsing
   utility functions, make use of the existing SAFE_FILE_LINES_SCANF()
   library function.

 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 .../kernel/syscalls/fanotify/fanotify21.c     | 306 ++++++++++++++++++
 2 files changed, 307 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify21.c

diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index c99e6fff7..35e73b91e 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -18,4 +18,5 @@
 /fanotify18
 /fanotify19
 /fanotify20
+/fanotify21
 /fanotify_child
diff --git a/testcases/kernel/syscalls/fanotify/fanotify21.c b/testcases/kernel/syscalls/fanotify/fanotify21.c
new file mode 100644
index 000000000..e842dad17
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify21.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Google. All Rights Reserved.
+ *
+ * Started by Matthew Bobrowski <repnop@google.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * A test which verifies whether the returned struct
+ * fanotify_event_info_pidfd in FAN_REPORT_PIDFD mode contains the
+ * expected set of information.
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <ctype.h>
+#include <stdlib.h>
+#include <string.h>
+#include "tst_test.h"
+#include "tst_safe_stdio.h"
+#include "lapi/pidfd_open.h"
+
+#ifdef HAVE_SYS_FANOTIFY_H
+#include "fanotify.h"
+
+#define BUF_SZ		4096
+#define MOUNT_PATH	"fs_mnt"
+#define TEST_FILE	MOUNT_PATH "/testfile"
+
+struct pidfd_fdinfo_t {
+	int pos;
+	int flags;
+	int mnt_id;
+	int pid;
+	int ns_pid;
+};
+
+struct test_case_t {
+	char *name;
+	int fork;
+	int want_pidfd_err;
+} test_cases[] = {
+	{
+		"return a valid pidfd for event created by self",
+		0,
+		0,
+	},
+	{
+		"return invalid pidfd for event created by terminated child",
+		1,
+		FAN_NOPIDFD,
+	},
+};
+
+static int fanotify_fd;
+static char event_buf[BUF_SZ];
+static struct pidfd_fdinfo_t *self_pidfd_fdinfo = NULL;
+
+static struct pidfd_fdinfo_t *read_pidfd_fdinfo(int pidfd)
+{
+	char *fdinfo_path;
+	struct pidfd_fdinfo_t *pidfd_fdinfo;
+
+	pidfd_fdinfo = SAFE_MALLOC(sizeof(struct pidfd_fdinfo_t));
+
+	SAFE_ASPRINTF(&fdinfo_path, "/proc/self/fdinfo/%d", pidfd);
+	SAFE_FILE_LINES_SCANF(fdinfo_path, "pos: %d", &pidfd_fdinfo->pos);
+	SAFE_FILE_LINES_SCANF(fdinfo_path, "flags: %d", &pidfd_fdinfo->flags);
+	SAFE_FILE_LINES_SCANF(fdinfo_path, "mnt_id: %d", &pidfd_fdinfo->mnt_id);
+	SAFE_FILE_LINES_SCANF(fdinfo_path, "Pid: %d", &pidfd_fdinfo->pid);
+	SAFE_FILE_LINES_SCANF(fdinfo_path, "NSpid: %d", &pidfd_fdinfo->ns_pid);
+
+	free(fdinfo_path);
+
+	return pidfd_fdinfo;
+}
+
+static void generate_event(void)
+{
+	int fd;
+
+	/* Generate a single FAN_OPEN event on the watched object. */
+	fd = SAFE_OPEN(TEST_FILE, O_RDONLY);
+	SAFE_CLOSE(fd);
+}
+
+static void do_fork(void)
+{
+	int status;
+	pid_t child;
+
+	child = SAFE_FORK();
+	if (child == 0) {
+		SAFE_CLOSE(fanotify_fd);
+		generate_event();
+		exit(EXIT_SUCCESS);
+	}
+
+	SAFE_WAITPID(child, &status, 0);
+	if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
+		tst_brk(TBROK,
+			"child process terminated incorrectly");
+}
+
+static void do_setup(void)
+{
+	int pidfd;
+
+	SAFE_TOUCH(TEST_FILE, 0666, NULL);
+
+	/*
+	 * 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);
+
+	fanotify_fd = SAFE_FANOTIFY_INIT(FAN_REPORT_PIDFD, O_RDONLY);
+	SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD, FAN_OPEN, AT_FDCWD,
+			   TEST_FILE);
+
+	pidfd = pidfd_open(getpid(), 0);
+	if (pidfd < 0) {
+		tst_brk(TBROK | TERRNO,
+			"pidfd=%d, pidfd_open(%d, 0) failed",
+			pidfd, getpid());
+	}
+
+	self_pidfd_fdinfo = read_pidfd_fdinfo(pidfd);
+	if (self_pidfd_fdinfo == NULL) {
+		tst_brk(TBROK,
+			"pidfd=%d, failed to read pidfd fdinfo",
+			pidfd);
+	}
+}
+
+static void do_test(unsigned int num)
+{
+	int i = 0, len;
+	struct test_case_t *tc = &test_cases[num];
+
+	tst_res(TINFO, "Test #%d: %s", num, tc->name);
+
+	/*
+	 * Generate the event in either self or a child process. Event
+	 * generation in a child process is done so that the FAN_NOPIDFD case
+	 * can be verified.
+	 */
+	if (tc->fork)
+		do_fork();
+	else
+		generate_event();
+
+	/*
+	 * Read all of the queued events into the provided event
+	 * buffer.
+	 */
+	len = SAFE_READ(0, fanotify_fd, event_buf, sizeof(event_buf));
+	while(i < len) {
+		struct fanotify_event_metadata *event;
+		struct fanotify_event_info_pidfd *info;
+		struct pidfd_fdinfo_t *event_pidfd_fdinfo = NULL;
+
+		event = (struct fanotify_event_metadata *)&event_buf[i];
+		info = (struct fanotify_event_info_pidfd *)(event + 1);
+
+		/*
+		 * Checks ensuring that pidfd information record object header
+		 * fields are set correctly.
+		 */
+		if (info->hdr.info_type != FAN_EVENT_INFO_TYPE_PIDFD) {
+			tst_res(TFAIL,
+				"unexpected info_type received in info "
+				"header (expected: %d, got: %d",
+				FAN_EVENT_INFO_TYPE_PIDFD,
+				info->hdr.info_type);
+			info = NULL;
+			goto next_event;
+		} else if (info->hdr.len !=
+			   sizeof(struct fanotify_event_info_pidfd)) {
+			tst_res(TFAIL,
+				"unexpected info object length "
+				"(expected: %lu, got: %d",
+				sizeof(struct fanotify_event_info_pidfd),
+				info->hdr.len);
+			info = NULL;
+			goto next_event;
+		}
+
+		/*
+		 * Check if pidfd information object reported any errors during
+		 * creation and whether they're expected.
+		 */
+		if (info->pidfd < 0 && !tc->want_pidfd_err) {
+			tst_res(TFAIL,
+				"pidfd creation failed for pid: %u with pidfd error value "
+				"set to: %d",
+				(unsigned)event->pid,
+				info->pidfd);
+			goto next_event;
+		} else if (tc->want_pidfd_err &&
+			   info->pidfd != tc->want_pidfd_err) {
+			tst_res(TFAIL,
+				"pidfd set to an unexpected error: %d for pid: %u",
+				info->pidfd,
+				(unsigned)event->pid);
+			goto next_event;
+		} else if (tc->want_pidfd_err &&
+			   info->pidfd == tc->want_pidfd_err) {
+			tst_res(TPASS,
+				"pid: %u terminated before pidfd was created, "
+				"pidfd set to the value of: %d, as expected",
+				(unsigned)event->pid,
+				FAN_NOPIDFD);
+			goto next_event;
+		}
+
+		/*
+		 * No pidfd errors occurred, continue with verifying pidfd
+		 * fdinfo validity.
+		 */
+		event_pidfd_fdinfo = read_pidfd_fdinfo(info->pidfd);
+		if (event_pidfd_fdinfo == NULL) {
+			tst_brk(TBROK,
+				"reading fdinfo for pidfd: %d "
+				"describing pid: %u failed",
+				info->pidfd,
+				(unsigned)event->pid);
+			goto next_event;
+		} else if (event_pidfd_fdinfo->pid != event->pid) {
+			tst_res(TFAIL,
+				"pidfd provided for incorrect pid "
+				"(expected pidfd for pid: %u, got pidfd for "
+				"pid: %u)",
+				(unsigned)event->pid,
+				(unsigned)event_pidfd_fdinfo->pid);
+			goto next_event;
+		} else if (memcmp(event_pidfd_fdinfo, self_pidfd_fdinfo,
+				  sizeof(struct pidfd_fdinfo_t))) {
+			tst_res(TFAIL,
+				"pidfd fdinfo values for self and event differ "
+				"(expected pos: %d, flags: %x, mnt_id: %d, "
+				"pid: %d, ns_pid: %d, got pos: %d, "
+				"flags: %x, mnt_id: %d, pid: %d, ns_pid: %d",
+				self_pidfd_fdinfo->pos,
+				self_pidfd_fdinfo->flags,
+				self_pidfd_fdinfo->mnt_id,
+				self_pidfd_fdinfo->pid,
+				self_pidfd_fdinfo->ns_pid,
+				event_pidfd_fdinfo->pos,
+				event_pidfd_fdinfo->flags,
+				event_pidfd_fdinfo->mnt_id,
+				event_pidfd_fdinfo->pid,
+				event_pidfd_fdinfo->ns_pid);
+			goto next_event;
+		} else {
+			tst_res(TPASS,
+				"got an event with a valid pidfd info record, "
+				"mask: %lld, pid: %u, fd: %d, "
+				"pidfd: %d, info_type: %d, info_len: %d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid,
+				event->fd,
+				info->pidfd,
+				info->hdr.info_type,
+				info->hdr.len);
+		}
+
+next_event:
+		i += event->event_len;
+		if (event->fd >= 0)
+			SAFE_CLOSE(event->fd);
+
+		if (info && info->pidfd >= 0)
+			SAFE_CLOSE(info->pidfd);
+
+		if (event_pidfd_fdinfo)
+			free(event_pidfd_fdinfo);
+	}
+}
+
+static void do_cleanup(void)
+{
+	if (fanotify_fd >= 0)
+		SAFE_CLOSE(fanotify_fd);
+
+	if (self_pidfd_fdinfo)
+		free(self_pidfd_fdinfo);
+}
+
+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,
+	.forks_child = 1,
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif /* HAVE_SYS_FANOTIFY_H */
-- 
2.33.1.1089.g2158813163f-goog

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] syscalls/fanotify20: add new test for FAN_REPORT_PIDFD

[Amir Goldstein]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] syscalls/fanotify20: add new test for FAN_REPORT_PIDFD

[Matthew Bobrowski via ltp]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] syscalls/fanotify20: add new test for FAN_REPORT_PIDFD

[Amir Goldstein]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] syscalls/fanotify20: add new test for FAN_REPORT_PIDFD

[Matthew Bobrowski via ltp]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 2/2] syscalls/fanotify21: add new test checking the returned pidfd from fanotify in FAN_REPORT_PIDFD mode

[Amir Goldstein]

> +next_event:
> +               i += event->event_len;
> +               if (event->fd >= 0)
> +                       SAFE_CLOSE(event->fd);
> +
> +               if (info && info->pidfd >= 0)
> +                       SAFE_CLOSE(info->pidfd);
> +
> +               if (event_pidfd_fdinfo)
> +                       free(event_pidfd_fdinfo);

Need to reset event_pidfd_fdinfo = NULL
to avoid double free in do_cleanup

> +       }
> +}
> +
> +static void do_cleanup(void)
> +{
> +       if (fanotify_fd >= 0)
> +               SAFE_CLOSE(fanotify_fd);
> +
> +       if (self_pidfd_fdinfo)
> +               free(self_pidfd_fdinfo);
> +}
> +

Thanks,
Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 2/2] syscalls/fanotify21: add new test checking the returned pidfd from fanotify in FAN_REPORT_PIDFD mode

[Matthew Bobrowski via ltp]

On Fri, Nov 05, 2021 at 06:21:56AM +0200, Amir Goldstein wrote:
> > +next_event:
> > +               i += event->event_len;
> > +               if (event->fd >= 0)
> > +                       SAFE_CLOSE(event->fd);
> > +
> > +               if (info && info->pidfd >= 0)
> > +                       SAFE_CLOSE(info->pidfd);
> > +
> > +               if (event_pidfd_fdinfo)
> > +                       free(event_pidfd_fdinfo);
> 
> Need to reset event_pidfd_fdinfo = NULL
> to avoid double free in do_cleanup

But, the variable event_pidfd_fdinfo is not referenced nor is visible
outside the scope of the do_test() function and therefore no double
free would occur, right?

> > +       }
> > +}
> > +
> > +static void do_cleanup(void)
> > +{
> > +       if (fanotify_fd >= 0)
> > +               SAFE_CLOSE(fanotify_fd);
> > +
> > +       if (self_pidfd_fdinfo)
> > +               free(self_pidfd_fdinfo);

Variables self_pidfd_fdinfo do_cleanup() and event_pidfd_fdinfo
do_test() are different.

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 2/2] syscalls/fanotify21: add new test checking the returned pidfd from fanotify in FAN_REPORT_PIDFD mode

[Amir Goldstein]

On Tue, Nov 9, 2021 at 11:36 AM Matthew Bobrowski <repnop@google.com> wrote:
>
> On Fri, Nov 05, 2021 at 06:21:56AM +0200, Amir Goldstein wrote:
> > > +next_event:
> > > +               i += event->event_len;
> > > +               if (event->fd >= 0)
> > > +                       SAFE_CLOSE(event->fd);
> > > +
> > > +               if (info && info->pidfd >= 0)
> > > +                       SAFE_CLOSE(info->pidfd);
> > > +
> > > +               if (event_pidfd_fdinfo)
> > > +                       free(event_pidfd_fdinfo);
> >
> > Need to reset event_pidfd_fdinfo = NULL
> > to avoid double free in do_cleanup
>
> But, the variable event_pidfd_fdinfo is not referenced nor is visible
> outside the scope of the do_test() function and therefore no double
> free would occur, right?
>
> > > +       }
> > > +}
> > > +
> > > +static void do_cleanup(void)
> > > +{
> > > +       if (fanotify_fd >= 0)
> > > +               SAFE_CLOSE(fanotify_fd);
> > > +
> > > +       if (self_pidfd_fdinfo)
> > > +               free(self_pidfd_fdinfo);
>
> Variables self_pidfd_fdinfo do_cleanup() and event_pidfd_fdinfo
> do_test() are different.
>

Right. my bad :)

Thanks,
Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp