Linux Test Project
 help / color / mirror / Atom feed
* [LTP] [PATCH 0/4] New tests for fanotify pre-content events
@ 2025-02-10 15:13 Amir Goldstein
  2025-02-10 15:13 ` [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and " Amir Goldstein
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 15:13 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

Petr,

These are the tests used for the development of the fanotify pre-content
(a.k.a HSM) events feature that was merged to v6.14-rc1.

Because this is a new test for a new feature, which is skipped on old
kernels, I am posting it early, so that others could run the tests.

Following the request to split out large fanotify LTP tests, I resisted
the temptation to add a pre-content events variant to the existing
fanotify03 test and I forked it to a new test.

I did however add one test case to fanotify14, because as I wrote
before, I find it useful to keep the test matrix of allowed flag
combinations within the same test.

I also added two test cases to fanotify03 before forking it for cases
that were regressed during the 6.14 devlopment cycle and did not have
proper test converage.

The number of test cases in fanotify is still below 10, so I was hoping
this addition is still acceptable.

Thanks,
Amir.

Amir Goldstein (4):
  fanotify14: Test invalid init flags with permission and pre-content
    events
  fanotify03: Add test cases for permission events on children
  fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  fanotify24: Test open for write of executable files with pre-content
    watch

 include/lapi/fanotify.h                       |  12 +
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 testcases/kernel/syscalls/fanotify/fanotify.h |   3 +
 .../kernel/syscalls/fanotify/fanotify03.c     |  47 +-
 .../kernel/syscalls/fanotify/fanotify14.c     |  20 +
 .../kernel/syscalls/fanotify/fanotify24.c     | 472 ++++++++++++++++++
 7 files changed, 550 insertions(+), 6 deletions(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify24.c

-- 
2.34.1


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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and pre-content events
  2025-02-10 15:13 [LTP] [PATCH 0/4] New tests for fanotify pre-content events Amir Goldstein
@ 2025-02-10 15:13 ` Amir Goldstein
  2025-02-10 15:19   ` Jan Kara
  2025-02-10 15:13 ` [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children Amir Goldstein
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 15:13 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

Those events require an high priority fanotify group.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/lapi/fanotify.h                       |  5 +++++
 .../kernel/syscalls/fanotify/fanotify14.c     | 20 +++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/lapi/fanotify.h b/include/lapi/fanotify.h
index 40ea7ead7..e5b930f4e 100644
--- a/include/lapi/fanotify.h
+++ b/include/lapi/fanotify.h
@@ -109,6 +109,9 @@
 #ifndef FAN_FS_ERROR
 #define FAN_FS_ERROR		0x00008000
 #endif
+#ifndef FAN_PRE_ACCESS
+#define FAN_PRE_ACCESS		0x00100000
+#endif
 #ifndef FAN_RENAME
 #define FAN_RENAME		0x10000000
 #endif
@@ -134,6 +137,8 @@
 #define LTP_ALL_PERM_EVENTS	(FAN_OPEN_PERM | FAN_OPEN_EXEC_PERM | \
 				 FAN_ACCESS_PERM)
 
+#define LTP_PRE_CONTENT_EVENTS	(FAN_PRE_ACCESS)
+
 struct fanotify_group_type {
 	unsigned int flag;
 	const char *name;
diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
index ee583a095..b17bffd18 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify14.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
@@ -239,6 +239,26 @@ static struct test_case_t {
 		.pfd = pipes,
 		.expected_errno = EINVAL,
 	},
+	/* permission events in mask with priority < FAN_CLASS_CONTENT are not valid */
+	{
+		.init = FLAGS_DESC(FAN_CLASS_NOTIF),
+		.mark = FLAGS_DESC(FAN_MARK_INODE),
+		.mask = FLAGS_DESC(LTP_ALL_PERM_EVENTS),
+		.expected_errno = EINVAL,
+	},
+	/* pre-content events in mask with priority < FAN_CLASS_PRE_CONTENT are not valid */
+	{
+		.init = FLAGS_DESC(FAN_CLASS_NOTIF),
+		.mark = FLAGS_DESC(FAN_MARK_INODE),
+		.mask = FLAGS_DESC(LTP_PRE_CONTENT_EVENTS),
+		.expected_errno = EINVAL,
+	},
+	{
+		.init = FLAGS_DESC(FAN_CLASS_CONTENT),
+		.mark = FLAGS_DESC(FAN_MARK_INODE),
+		.mask = FLAGS_DESC(LTP_PRE_CONTENT_EVENTS),
+		.expected_errno = EINVAL,
+	},
 };
 
 static void do_test(unsigned int number)
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-10 15:13 [LTP] [PATCH 0/4] New tests for fanotify pre-content events Amir Goldstein
  2025-02-10 15:13 ` [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and " Amir Goldstein
@ 2025-02-10 15:13 ` Amir Goldstein
  2025-02-10 15:24   ` Jan Kara
  2025-02-10 15:13 ` [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO Amir Goldstein
  2025-02-10 15:13 ` [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch Amir Goldstein
  3 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 15:13 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

Verify that permission events are delivered iff parent is watching
children.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify03.c     | 47 ++++++++++++++++---
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
index 64c933c19..4b2f6e45e 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify03.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -121,12 +121,29 @@ static struct tcase {
 			{FAN_OPEN_EXEC_PERM, FAN_DENY}
 		}
 	},
+	{
+		"parent watching children, FAN_ACCESS_PERM | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(PARENT),
+		FAN_ACCESS_PERM | FAN_OPEN_EXEC_PERM | FAN_EVENT_ON_CHILD, 2,
+		{
+			{FAN_ACCESS_PERM, FAN_DENY},
+			{FAN_OPEN_EXEC_PERM, FAN_DENY}
+		}
+	},
+	{
+		"parent not watching children, FAN_ACCESS_PERM | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(PARENT),
+		FAN_ACCESS_PERM | FAN_OPEN_EXEC_PERM, 0,
+		{
+		}
+	},
 };
 
-static void generate_events(void)
+static void generate_events(struct tcase *tc)
 {
 	int fd;
 	char *const argv[] = {FILE_EXEC_PATH, NULL};
+	int exp_ret, exp_errno = tc->event_count ? EPERM : 0;
 
 	/*
 	 * Generate sequence of events
@@ -136,13 +153,25 @@ static void generate_events(void)
 	SAFE_WRITE(SAFE_WRITE_ANY, fd, fname, 1);
 	SAFE_LSEEK(fd, 0, SEEK_SET);
 
-	if (read(fd, buf, BUF_SIZE) != -1)
+	exp_ret = exp_errno ? -1 : 1;
+	errno = 0;
+	if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
+		tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
 		exit(3);
+	} else if (errno == exp_errno) {
+		tst_res(TINFO, "read() got errno %d as expected", errno);
+	}
 
 	SAFE_CLOSE(fd);
 
-	if (execve(FILE_EXEC_PATH, argv, environ) != -1)
+	exp_ret = exp_errno ? -1 : 0;
+	errno = 0;
+	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
+		tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
 		exit(5);
+	} else if (errno == exp_errno) {
+		tst_res(TINFO, "execve() got errno %d as expected", errno);
+	}
 }
 
 static void child_handler(int tmp)
@@ -156,7 +185,7 @@ static void child_handler(int tmp)
 	fd_notify = -1;
 }
 
-static void run_child(void)
+static void run_child(struct tcase *tc)
 {
 	struct sigaction child_action;
 
@@ -174,7 +203,7 @@ static void run_child(void)
 	if (child_pid == 0) {
 		/* Child will generate events now */
 		SAFE_CLOSE(fd_notify);
-		generate_events();
+		generate_events(tc);
 		exit(0);
 	}
 }
@@ -220,6 +249,12 @@ static int setup_mark(unsigned int n)
 
 	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
 
+	if (mark->flag == FAN_MARK_PARENT) {
+		SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD | mark->flag,
+				   tc->mask, AT_FDCWD, MOUNT_PATH);
+		return 0;
+	}
+
 	for (; i < ARRAY_SIZE(files); i++) {
 		SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD | mark->flag,
 				  tc->mask, AT_FDCWD, files[i]);
@@ -237,7 +272,7 @@ static void test_fanotify(unsigned int n)
 	if (setup_mark(n) != 0)
 		return;
 
-	run_child();
+	run_child(tc);
 
 	/*
 	 * Process events
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-10 15:13 [LTP] [PATCH 0/4] New tests for fanotify pre-content events Amir Goldstein
  2025-02-10 15:13 ` [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and " Amir Goldstein
  2025-02-10 15:13 ` [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children Amir Goldstein
@ 2025-02-10 15:13 ` Amir Goldstein
  2025-02-10 15:43   ` Jan Kara
  2025-02-28 20:11   ` Heiko Carstens
  2025-02-10 15:13 ` [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch Amir Goldstein
  3 siblings, 2 replies; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 15:13 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

Fork the test fanotify24 from test fanotify03, replacing the
permission event FAN_ACCESS_PERM with the new pre-content event
FAN_PRE_ACCESS.

The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
required for FAN_PRE_ACCESS and this class also enabled the response
with cutomer error code FAN_DENY_ERRNO.

Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
system call.  The test case expected results are adjusted to
respond with the default error (EPERM) to open() and write() and
to respond with custom errors (EIO, EBUSY) to read() and execve().

Not all fs support pre-content events, so run on all filesystems
to excercise FAN_PRE_ACCESS on all supported filesystems.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/lapi/fanotify.h                       |   7 +
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 testcases/kernel/syscalls/fanotify/fanotify.h |   3 +
 .../kernel/syscalls/fanotify/fanotify24.c     | 433 ++++++++++++++++++
 5 files changed, 445 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify24.c

diff --git a/include/lapi/fanotify.h b/include/lapi/fanotify.h
index e5b930f4e..9076685e8 100644
--- a/include/lapi/fanotify.h
+++ b/include/lapi/fanotify.h
@@ -124,6 +124,13 @@
 #define FAN_EPIDFD		-2
 #endif
 
+/* errno other than EPERM can specified in upper byte of deny response */
+#ifndef FAN_DENY_ERRNO
+#define FAN_ERRNO(err) (((((__u32)(err)) & 0xff) << 24))
+#define FAN_DENY_ERRNO(err) (FAN_DENY | FAN_ERRNO(err))
+#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> 24))
+#endif
+
 /* Flags required for unprivileged user group */
 #define FANOTIFY_REQUIRED_USER_INIT_FLAGS    (FAN_REPORT_FID)
 
diff --git a/runtest/syscalls b/runtest/syscalls
index 4ab8436d3..07a3d5907 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -649,6 +649,7 @@ fanotify20 fanotify20
 fanotify21 fanotify21
 fanotify22 fanotify22
 fanotify23 fanotify23
+fanotify24 fanotify24
 
 ioperm01 ioperm01
 ioperm02 ioperm02
diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index a0a7d20d3..16af3db85 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -21,4 +21,5 @@
 /fanotify21
 /fanotify22
 /fanotify23
+/fanotify24
 /fanotify_child
diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 48a44cc7e..7977b4aa4 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -277,6 +277,9 @@ static inline void fanotify_flags_err_msg(const char *flags_str,
 #define FANOTIFY_MARK_FLAGS_ERR_MSG(mark, fail) \
 	fanotify_flags_err_msg((mark)->name, __FILE__, __LINE__, tst_res_, (fail))
 
+#define FANOTIFY_EVENTS_ERR_MSG(event, fail) \
+	fanotify_flags_err_msg(#event, __FILE__, __LINE__, tst_res_, (fail))
+
 #define REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(flags, fname) \
 	fanotify_flags_err_msg(#flags, __FILE__, __LINE__, tst_brk_, \
 		fanotify_init_flags_supported_on_fs(flags, fname))
diff --git a/testcases/kernel/syscalls/fanotify/fanotify24.c b/testcases/kernel/syscalls/fanotify/fanotify24.c
new file mode 100644
index 000000000..a7aa2e052
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify24.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2013 SUSE.  All Rights Reserved.
+ * Copyright (c) 2025 CTERA Networks.  All Rights Reserved.
+ *
+ * Started by Jan Kara <jack@suse.cz>
+ */
+
+/*\
+ * [Description]
+ * - Test fanotify pre-content events
+ * - Test respond to permission/pre-content events with cutsom error code
+ */
+
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <errno.h>
+#include <string.h>
+#include <signal.h>
+#include <sys/syscall.h>
+#include <stdlib.h>
+#include "tst_test.h"
+
+#ifdef HAVE_SYS_FANOTIFY_H
+# include "fanotify.h"
+
+#define EVENT_MAX 1024
+/* size of the event structure, not counting name */
+#define EVENT_SIZE  (sizeof(struct fanotify_event_metadata))
+/* reasonable guess as to size of 1024 events */
+#define EVENT_BUF_LEN        (EVENT_MAX * EVENT_SIZE)
+/* Size large enough to hold a reasonable amount of expected event objects */
+#define EVENT_SET_MAX 16
+
+#define BUF_SIZE 256
+#define TST_TOTAL 3
+#define TEST_APP "fanotify_child"
+#define MOUNT_PATH "fs_mnt"
+#define FILE_EXEC_PATH MOUNT_PATH"/"TEST_APP
+
+static char fname[BUF_SIZE];
+static char buf[BUF_SIZE];
+static volatile int fd_notify;
+
+static pid_t child_pid;
+
+static char event_buf[EVENT_BUF_LEN];
+
+struct event {
+	unsigned long long mask;
+	unsigned int response;
+};
+
+static struct tcase {
+	const char *tname;
+	struct fanotify_mark_type mark;
+	unsigned long long mask;
+	int event_count;
+	struct event event_set[EVENT_SET_MAX];
+} tcases[] = {
+	{
+		"inode mark, FAN_OPEN_PERM | FAN_PRE_ACCESS events",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_OPEN_PERM | FAN_PRE_ACCESS, 4,
+		{
+			{FAN_OPEN_PERM, FAN_ALLOW},
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"inode mark, FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM, 3,
+		{
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_EXEC_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"mount mark, FAN_OPEN_PERM | FAN_PRE_ACCESS events",
+		INIT_FANOTIFY_MARK_TYPE(MOUNT),
+		FAN_OPEN_PERM | FAN_PRE_ACCESS, 4,
+		{
+			{FAN_OPEN_PERM, FAN_ALLOW},
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"mount mark, FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(MOUNT),
+		FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM, 3,
+		{
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_EXEC_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"filesystem mark, FAN_OPEN_PERM | FAN_PRE_ACCESS events",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+		FAN_OPEN_PERM | FAN_PRE_ACCESS, 4,
+		{
+			{FAN_OPEN_PERM, FAN_ALLOW},
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"filesystem mark, FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+		FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM, 3,
+		{
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
+			{FAN_OPEN_EXEC_PERM, FAN_DENY_ERRNO(EBUSY)}
+		}
+	},
+	{
+		"parent watching children, FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(PARENT),
+		FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM | FAN_EVENT_ON_CHILD, 3,
+		{
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_PRE_ACCESS, FAN_DENY},
+			{FAN_OPEN_EXEC_PERM, FAN_DENY}
+		}
+	},
+	{
+		"parent not watching children, FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM events",
+		INIT_FANOTIFY_MARK_TYPE(PARENT),
+		FAN_PRE_ACCESS | FAN_OPEN_EXEC_PERM, 0,
+		{
+		}
+	},
+};
+
+static int expected_errno(unsigned int response)
+{
+	switch (response) {
+		case 0:
+		case FAN_ALLOW:
+			return 0;
+		case FAN_DENY:
+			return EPERM;
+		default:
+			return FAN_RESPONSE_ERRNO(response);
+	}
+}
+
+static void generate_events(struct tcase *tc)
+{
+	int fd;
+	char *const argv[] = {FILE_EXEC_PATH, NULL};
+	struct event* event = tc->event_set;
+	int exp_ret, exp_errno = 0;
+
+	if (event->mask == FAN_OPEN_PERM)
+		event++;
+
+	/*
+	 * Generate sequence of events
+	 */
+	fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0700);
+
+	exp_errno = expected_errno(event->response);
+	event++;
+
+	exp_ret = exp_errno ? -1 : 1;
+	errno = 0;
+	/* FAN_PRE_ACCESS events are reported also on write */
+	if (write(fd, fname, 1) != exp_ret || errno != exp_errno) {
+		tst_res(TFAIL, "write() got errno %d (expected %d)", errno, exp_errno);
+		exit(3);
+	} else if (errno == exp_errno) {
+		tst_res(TINFO, "write() got errno %d as expected", errno);
+	}
+
+	SAFE_LSEEK(fd, 0, SEEK_SET);
+
+	exp_errno = expected_errno(event->response);
+	event++;
+
+	exp_ret = exp_errno ? -1 : 1;
+	errno = 0;
+	if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
+		tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
+		exit(4);
+	} else if (errno == exp_errno) {
+		tst_res(TINFO, "read() got errno %d as expected", errno);
+	}
+
+	SAFE_CLOSE(fd);
+
+	exp_errno = expected_errno(event->response);
+	event++;
+
+	exp_ret = exp_errno ? -1 : 0;
+	errno = 0;
+	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
+		tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
+		exit(5);
+	} else if (errno == exp_errno) {
+		tst_res(TINFO, "execve() got errno %d as expected", errno);
+	}
+}
+
+static void child_handler(int tmp)
+{
+	(void)tmp;
+	/*
+	 * Close notification fd so that we cannot block while reading
+	 * from it
+	 */
+	SAFE_CLOSE(fd_notify);
+	fd_notify = -1;
+}
+
+static void run_child(struct tcase *tc)
+{
+	struct sigaction child_action;
+
+	child_action.sa_handler = child_handler;
+	sigemptyset(&child_action.sa_mask);
+	child_action.sa_flags = SA_NOCLDSTOP;
+
+	if (sigaction(SIGCHLD, &child_action, NULL) < 0) {
+		tst_brk(TBROK | TERRNO,
+			"sigaction(SIGCHLD, &child_action, NULL) failed");
+	}
+
+	child_pid = SAFE_FORK();
+
+	if (child_pid == 0) {
+		/* Child will generate events now */
+		SAFE_CLOSE(fd_notify);
+		generate_events(tc);
+		exit(0);
+	}
+}
+
+static void check_child(void)
+{
+	struct sigaction child_action;
+	int child_ret;
+
+	child_action.sa_handler = SIG_IGN;
+	sigemptyset(&child_action.sa_mask);
+	child_action.sa_flags = SA_NOCLDSTOP;
+	if (sigaction(SIGCHLD, &child_action, NULL) < 0) {
+		tst_brk(TBROK | TERRNO,
+			"sigaction(SIGCHLD, &child_action, NULL) failed");
+	}
+	SAFE_WAITPID(-1, &child_ret, 0);
+
+	if (WIFEXITED(child_ret) && WEXITSTATUS(child_ret) == 0)
+		tst_res(TPASS, "child exited correctly");
+	else
+		tst_res(TFAIL, "child %s", tst_strstatus(child_ret));
+}
+
+static int setup_mark(unsigned int n)
+{
+	unsigned int i = 0;
+	struct tcase *tc = &tcases[n];
+	struct fanotify_mark_type *mark = &tc->mark;
+	char *const files[] = {fname, FILE_EXEC_PATH};
+
+	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+
+	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_PRE_CONTENT, O_RDONLY);
+
+	if (mark->flag == FAN_MARK_PARENT) {
+		SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD | mark->flag,
+				   tc->mask, AT_FDCWD, MOUNT_PATH);
+		return 0;
+	}
+
+	for (; i < ARRAY_SIZE(files); i++) {
+		SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD | mark->flag,
+				  tc->mask, AT_FDCWD, files[i]);
+	}
+
+	return 0;
+}
+
+static void test_fanotify(unsigned int n)
+{
+	int ret, len = 0, i = 0, test_num = 0;
+	struct tcase *tc = &tcases[n];
+	struct event *event_set = tc->event_set;
+
+	if (setup_mark(n) != 0)
+		return;
+
+	run_child(tc);
+
+	/*
+	 * Process events
+	 *
+	 * tc->count + 1 is to accommodate for checking the child process
+	 * return value
+	 */
+	while (test_num < tc->event_count + 1 && fd_notify != -1) {
+		struct fanotify_event_metadata *event;
+
+		if (i == len) {
+			/* Get more events */
+			ret = read(fd_notify, event_buf + len,
+				   EVENT_BUF_LEN - len);
+			if (fd_notify == -1)
+				break;
+			if (ret < 0) {
+				tst_brk(TBROK,
+					"read(%d, buf, %zu) failed",
+					fd_notify, EVENT_BUF_LEN);
+			}
+			len += ret;
+		}
+
+		event = (struct fanotify_event_metadata *)&event_buf[i];
+		/* Permission events cannot be merged, so the event mask
+		 * reported should exactly match the event mask within the
+		 * event set.
+		 */
+		if (event->mask != event_set[test_num].mask) {
+			tst_res(TFAIL,
+				"got event: mask=%llx (expected %llx) "
+				"pid=%u fd=%d",
+				(unsigned long long)event->mask,
+				event_set[test_num].mask,
+				(unsigned int)event->pid, event->fd);
+		} else if (event->pid != child_pid) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u "
+				"(expected %u) fd=%d",
+				(unsigned long long)event->mask,
+				(unsigned int)event->pid,
+				(unsigned int)child_pid,
+				event->fd);
+		} else {
+			tst_res(TPASS,
+				"got event: mask=%llx pid=%u fd=%d",
+				(unsigned long long)event->mask,
+				(unsigned int)event->pid, event->fd);
+		}
+
+		/* Write response to the permission event */
+		if (event_set[test_num].mask &
+			(LTP_ALL_PERM_EVENTS | LTP_PRE_CONTENT_EVENTS)) {
+			struct fanotify_response resp;
+
+			resp.fd = event->fd;
+			resp.response = event_set[test_num].response;
+			SAFE_WRITE(SAFE_WRITE_ALL, fd_notify, &resp, sizeof(resp));
+			tst_res(TPASS, "response=%x fd=%d\n", resp.response, resp.fd);
+		}
+
+		i += event->event_len;
+
+		if (event->fd != FAN_NOFD) {
+			char c;
+
+			/* Verify that read from event fd does not generate events */
+			SAFE_READ(0, event->fd, &c, 1);
+			SAFE_CLOSE(event->fd);
+		}
+
+		test_num++;
+	}
+
+	for (; test_num < tc->event_count; test_num++) {
+		tst_res(TFAIL, "didn't get event: mask=%llx",
+			event_set[test_num].mask);
+
+	}
+
+	check_child();
+
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+}
+
+static void setup(void)
+{
+	sprintf(fname, MOUNT_PATH"/fname_%d", getpid());
+	SAFE_FILE_PRINTF(fname, "1");
+
+	REQUIRE_FANOTIFY_EVENTS_SUPPORTED_ON_FS(FAN_CLASS_PRE_CONTENT, FAN_MARK_FILESYSTEM,
+						FAN_PRE_ACCESS, fname);
+
+	SAFE_CP(TEST_APP, FILE_EXEC_PATH);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+}
+
+static const char *const resource_files[] = {
+	TEST_APP,
+	NULL
+};
+
+static struct tst_test test = {
+	.timeout = 1,
+	.test = test_fanotify,
+	.tcnt = ARRAY_SIZE(tcases),
+	.setup = setup,
+	.cleanup = cleanup,
+	.forks_child = 1,
+	.needs_root = 1,
+	.mount_device = 1,
+	.all_filesystems = 1,
+	.mntpoint = MOUNT_PATH,
+	.resource_files = resource_files
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch
  2025-02-10 15:13 [LTP] [PATCH 0/4] New tests for fanotify pre-content events Amir Goldstein
                   ` (2 preceding siblings ...)
  2025-02-10 15:13 ` [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO Amir Goldstein
@ 2025-02-10 15:13 ` Amir Goldstein
  2025-02-10 15:59   ` Jan Kara
  3 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 15:13 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

Watching pre-content events should allow opening an executable file for
write and executing a file that is open for write.

We have an existing test where the exectable file is not watched by
pre-content events.

We add a new test case, where the executable file is watched for
FAN_PRE_ACCESS pre-content event and access is allowed.

In the former case (not watched), execution should fail with ETXTBSY and
in the latter case (per-content watched) execution should succeed.

When allowing access events, we allow for multiple events, because
read() may generate more than a single access event.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify24.c     | 43 ++++++++++++++++++-
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify24.c b/testcases/kernel/syscalls/fanotify/fanotify24.c
index a7aa2e052..537773d52 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify24.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify24.c
@@ -144,6 +144,15 @@ static struct tcase {
 		{
 		}
 	},
+	{
+		"inode mark, FAN_PRE_ACCESS event allowed",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_PRE_ACCESS, 1,
+		{
+			/* This allows multiple FAN_PRE_ACCESS events */
+			{FAN_PRE_ACCESS, FAN_ALLOW},
+		}
+	},
 };
 
 static int expected_errno(unsigned int response)
@@ -206,6 +215,21 @@ static void generate_events(struct tcase *tc)
 	exp_errno = expected_errno(event->response);
 	event++;
 
+	/*
+	 * If execve() is allowed by permission events, check if executing a
+	 * file that open for write is allowed.
+	 * HSM needs to be able to write to file during pre-content event, so it
+	 * requires that a file being executed can be open for write, which also
+	 * means that a file open for write can be executed.
+	 * Therefore, ETXTBSY is to be expected when file is not being watched
+	 * at all or being watched but not with pre-content events in mask.
+	 */
+	if (!exp_errno) {
+		fd = SAFE_OPEN(FILE_EXEC_PATH, O_RDWR);
+		if (!tc->event_count)
+			exp_errno = ETXTBSY;
+	}
+
 	exp_ret = exp_errno ? -1 : 0;
 	errno = 0;
 	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
@@ -214,6 +238,9 @@ static void generate_events(struct tcase *tc)
 	} else if (errno == exp_errno) {
 		tst_res(TINFO, "execve() got errno %d as expected", errno);
 	}
+
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
 }
 
 static void child_handler(int tmp)
@@ -309,8 +336,8 @@ static void test_fanotify(unsigned int n)
 	/*
 	 * Process events
 	 *
-	 * tc->count + 1 is to accommodate for checking the child process
-	 * return value
+	 * tc->count + 1 is to let read() wait for child process to exit
+	 * and to accomodate for extra access events
 	 */
 	while (test_num < tc->event_count + 1 && fd_notify != -1) {
 		struct fanotify_event_metadata *event;
@@ -319,6 +346,7 @@ static void test_fanotify(unsigned int n)
 			/* Get more events */
 			ret = read(fd_notify, event_buf + len,
 				   EVENT_BUF_LEN - len);
+			/* Received SIGCHLD */
 			if (fd_notify == -1)
 				break;
 			if (ret < 0) {
@@ -329,6 +357,17 @@ static void test_fanotify(unsigned int n)
 			len += ret;
 		}
 
+		/*
+		 * If we got an event after the last event and the last event was
+		 * allowed then assume this is another event of the same type.
+		 * This is to accomodate for the fact that a single read() may
+		 * generate an unknown number of access permission events if they
+		 * are allowed.
+		 */
+		if (test_num > 0 && test_num == tc->event_count &&
+		    event_set[test_num-1].response == FAN_ALLOW)
+			test_num--;
+
 		event = (struct fanotify_event_metadata *)&event_buf[i];
 		/* Permission events cannot be merged, so the event mask
 		 * reported should exactly match the event mask within the
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and pre-content events
  2025-02-10 15:13 ` [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and " Amir Goldstein
@ 2025-02-10 15:19   ` Jan Kara
  2025-02-11 17:55     ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Kara @ 2025-02-10 15:19 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

On Mon 10-02-25 16:13:13, Amir Goldstein wrote:
> Those events require an high priority fanotify group.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/lapi/fanotify.h                       |  5 +++++
>  .../kernel/syscalls/fanotify/fanotify14.c     | 20 +++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/lapi/fanotify.h b/include/lapi/fanotify.h
> index 40ea7ead7..e5b930f4e 100644
> --- a/include/lapi/fanotify.h
> +++ b/include/lapi/fanotify.h
> @@ -109,6 +109,9 @@
>  #ifndef FAN_FS_ERROR
>  #define FAN_FS_ERROR		0x00008000
>  #endif
> +#ifndef FAN_PRE_ACCESS
> +#define FAN_PRE_ACCESS		0x00100000
> +#endif
>  #ifndef FAN_RENAME
>  #define FAN_RENAME		0x10000000
>  #endif
> @@ -134,6 +137,8 @@
>  #define LTP_ALL_PERM_EVENTS	(FAN_OPEN_PERM | FAN_OPEN_EXEC_PERM | \
>  				 FAN_ACCESS_PERM)
>  
> +#define LTP_PRE_CONTENT_EVENTS	(FAN_PRE_ACCESS)
> +
>  struct fanotify_group_type {
>  	unsigned int flag;
>  	const char *name;
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
> index ee583a095..b17bffd18 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify14.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
> @@ -239,6 +239,26 @@ static struct test_case_t {
>  		.pfd = pipes,
>  		.expected_errno = EINVAL,
>  	},
> +	/* permission events in mask with priority < FAN_CLASS_CONTENT are not valid */
> +	{
> +		.init = FLAGS_DESC(FAN_CLASS_NOTIF),
> +		.mark = FLAGS_DESC(FAN_MARK_INODE),
> +		.mask = FLAGS_DESC(LTP_ALL_PERM_EVENTS),
> +		.expected_errno = EINVAL,
> +	},
> +	/* pre-content events in mask with priority < FAN_CLASS_PRE_CONTENT are not valid */
> +	{
> +		.init = FLAGS_DESC(FAN_CLASS_NOTIF),
> +		.mark = FLAGS_DESC(FAN_MARK_INODE),
> +		.mask = FLAGS_DESC(LTP_PRE_CONTENT_EVENTS),
> +		.expected_errno = EINVAL,
> +	},
> +	{
> +		.init = FLAGS_DESC(FAN_CLASS_CONTENT),
> +		.mark = FLAGS_DESC(FAN_MARK_INODE),
> +		.mask = FLAGS_DESC(LTP_PRE_CONTENT_EVENTS),
> +		.expected_errno = EINVAL,
> +	},
>  };
>  
>  static void do_test(unsigned int number)
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-10 15:13 ` [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children Amir Goldstein
@ 2025-02-10 15:24   ` Jan Kara
  2025-02-10 16:55     ` Amir Goldstein
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Kara @ 2025-02-10 15:24 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

On Mon 10-02-25 16:13:14, Amir Goldstein wrote:
> Verify that permission events are delivered iff parent is watching
> children.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Overall looks ok but:

> -	if (read(fd, buf, BUF_SIZE) != -1)
> +	exp_ret = exp_errno ? -1 : 1;
> +	errno = 0;
> +	if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
> +		tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
>  		exit(3);
> +	} else if (errno == exp_errno) {
		^^ Why is this condition needed? It is always true AFAICT.

> +		tst_res(TINFO, "read() got errno %d as expected", errno);
> +	}
>  
>  	SAFE_CLOSE(fd);
>  
> -	if (execve(FILE_EXEC_PATH, argv, environ) != -1)
> +	exp_ret = exp_errno ? -1 : 0;
> +	errno = 0;
> +	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
> +		tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
>  		exit(5);
> +	} else if (errno == exp_errno) {
		^^^ and here as well...

> +		tst_res(TINFO, "execve() got errno %d as expected", errno);
> +	}
>  }

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-10 15:13 ` [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO Amir Goldstein
@ 2025-02-10 15:43   ` Jan Kara
  2025-02-10 17:06     ` Amir Goldstein
  2025-02-28 20:11   ` Heiko Carstens
  1 sibling, 1 reply; 21+ messages in thread
From: Jan Kara @ 2025-02-10 15:43 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> Fork the test fanotify24 from test fanotify03, replacing the
> permission event FAN_ACCESS_PERM with the new pre-content event
> FAN_PRE_ACCESS.
> 
> The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> required for FAN_PRE_ACCESS and this class also enabled the response
> with cutomer error code FAN_DENY_ERRNO.
> 
> Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> system call.  The test case expected results are adjusted to
> respond with the default error (EPERM) to open() and write() and
> to respond with custom errors (EIO, EBUSY) to read() and execve().
> 
> Not all fs support pre-content events, so run on all filesystems
> to excercise FAN_PRE_ACCESS on all supported filesystems.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me. I was just wondering whether some bits like
generate_events(), mark setup, child setup, main test loop could not be
factored out into a helper functions used by both old and new tests?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch
  2025-02-10 15:13 ` [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch Amir Goldstein
@ 2025-02-10 15:59   ` Jan Kara
  0 siblings, 0 replies; 21+ messages in thread
From: Jan Kara @ 2025-02-10 15:59 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

On Mon 10-02-25 16:13:16, Amir Goldstein wrote:
> Watching pre-content events should allow opening an executable file for
> write and executing a file that is open for write.
> 
> We have an existing test where the exectable file is not watched by
> pre-content events.
> 
> We add a new test case, where the executable file is watched for
> FAN_PRE_ACCESS pre-content event and access is allowed.
> 
> In the former case (not watched), execution should fail with ETXTBSY and
> in the latter case (per-content watched) execution should succeed.
> 
> When allowing access events, we allow for multiple events, because
> read() may generate more than a single access event.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  .../kernel/syscalls/fanotify/fanotify24.c     | 43 ++++++++++++++++++-
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify24.c b/testcases/kernel/syscalls/fanotify/fanotify24.c
> index a7aa2e052..537773d52 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify24.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify24.c
> @@ -144,6 +144,15 @@ static struct tcase {
>  		{
>  		}
>  	},
> +	{
> +		"inode mark, FAN_PRE_ACCESS event allowed",
> +		INIT_FANOTIFY_MARK_TYPE(INODE),
> +		FAN_PRE_ACCESS, 1,
> +		{
> +			/* This allows multiple FAN_PRE_ACCESS events */
> +			{FAN_PRE_ACCESS, FAN_ALLOW},
> +		}
> +	},
>  };
>  
>  static int expected_errno(unsigned int response)
> @@ -206,6 +215,21 @@ static void generate_events(struct tcase *tc)
>  	exp_errno = expected_errno(event->response);
>  	event++;
>  
> +	/*
> +	 * If execve() is allowed by permission events, check if executing a
> +	 * file that open for write is allowed.
> +	 * HSM needs to be able to write to file during pre-content event, so it
> +	 * requires that a file being executed can be open for write, which also
> +	 * means that a file open for write can be executed.
> +	 * Therefore, ETXTBSY is to be expected when file is not being watched
> +	 * at all or being watched but not with pre-content events in mask.
> +	 */
> +	if (!exp_errno) {
> +		fd = SAFE_OPEN(FILE_EXEC_PATH, O_RDWR);
> +		if (!tc->event_count)
> +			exp_errno = ETXTBSY;
> +	}
> +
>  	exp_ret = exp_errno ? -1 : 0;
>  	errno = 0;
>  	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
> @@ -214,6 +238,9 @@ static void generate_events(struct tcase *tc)
>  	} else if (errno == exp_errno) {
>  		tst_res(TINFO, "execve() got errno %d as expected", errno);
>  	}
> +
> +	if (fd >= 0)
> +		SAFE_CLOSE(fd);
>  }
>  
>  static void child_handler(int tmp)
> @@ -309,8 +336,8 @@ static void test_fanotify(unsigned int n)
>  	/*
>  	 * Process events
>  	 *
> -	 * tc->count + 1 is to accommodate for checking the child process
> -	 * return value
> +	 * tc->count + 1 is to let read() wait for child process to exit
> +	 * and to accomodate for extra access events
>  	 */
>  	while (test_num < tc->event_count + 1 && fd_notify != -1) {
>  		struct fanotify_event_metadata *event;
> @@ -319,6 +346,7 @@ static void test_fanotify(unsigned int n)
>  			/* Get more events */
>  			ret = read(fd_notify, event_buf + len,
>  				   EVENT_BUF_LEN - len);
> +			/* Received SIGCHLD */
>  			if (fd_notify == -1)
>  				break;
>  			if (ret < 0) {
> @@ -329,6 +357,17 @@ static void test_fanotify(unsigned int n)
>  			len += ret;
>  		}
>  
> +		/*
> +		 * If we got an event after the last event and the last event was
> +		 * allowed then assume this is another event of the same type.
> +		 * This is to accomodate for the fact that a single read() may
> +		 * generate an unknown number of access permission events if they
> +		 * are allowed.
> +		 */
> +		if (test_num > 0 && test_num == tc->event_count &&
> +		    event_set[test_num-1].response == FAN_ALLOW)
> +			test_num--;
> +
>  		event = (struct fanotify_event_metadata *)&event_buf[i];
>  		/* Permission events cannot be merged, so the event mask
>  		 * reported should exactly match the event mask within the
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-10 15:24   ` Jan Kara
@ 2025-02-10 16:55     ` Amir Goldstein
  2025-02-11 17:56       ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 16:55 UTC (permalink / raw)
  To: Jan Kara; +Cc: ltp

On Mon, Feb 10, 2025 at 4:25 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 10-02-25 16:13:14, Amir Goldstein wrote:
> > Verify that permission events are delivered iff parent is watching
> > children.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Overall looks ok but:
>
> > -     if (read(fd, buf, BUF_SIZE) != -1)
> > +     exp_ret = exp_errno ? -1 : 1;
> > +     errno = 0;
> > +     if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
> > +             tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
> >               exit(3);
> > +     } else if (errno == exp_errno) {
>                 ^^ Why is this condition needed? It is always true AFAICT.
>
> > +             tst_res(TINFO, "read() got errno %d as expected", errno);
> > +     }
> >
> >       SAFE_CLOSE(fd);
> >
> > -     if (execve(FILE_EXEC_PATH, argv, environ) != -1)
> > +     exp_ret = exp_errno ? -1 : 0;
> > +     errno = 0;
> > +     if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
> > +             tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
> >               exit(5);
> > +     } else if (errno == exp_errno) {
>                 ^^^ and here as well...
>
> > +             tst_res(TINFO, "execve() got errno %d as expected", errno);

You are right.
I was "backported" from the pre-content test.
The two else statements can be removed in this patch.

Thanks,
Amir.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-10 15:43   ` Jan Kara
@ 2025-02-10 17:06     ` Amir Goldstein
  2025-02-11 19:09       ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-10 17:06 UTC (permalink / raw)
  To: Jan Kara; +Cc: ltp

On Mon, Feb 10, 2025 at 4:43 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> > Fork the test fanotify24 from test fanotify03, replacing the
> > permission event FAN_ACCESS_PERM with the new pre-content event
> > FAN_PRE_ACCESS.
> >
> > The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> > required for FAN_PRE_ACCESS and this class also enabled the response
> > with cutomer error code FAN_DENY_ERRNO.
> >
> > Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> > system call.  The test case expected results are adjusted to
> > respond with the default error (EPERM) to open() and write() and
> > to respond with custom errors (EIO, EBUSY) to read() and execve().
> >
> > Not all fs support pre-content events, so run on all filesystems
> > to excercise FAN_PRE_ACCESS on all supported filesystems.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks good to me. I was just wondering whether some bits like
> generate_events(), mark setup, child setup, main test loop could not be
> factored out into a helper functions used by both old and new tests?

Yes, I agree that forking the tests is bad and that we need much
more common helpers.

IIUC, LTP developers are going to try to come up with some proposals
for refactoring helpers to split some large fanotify tests [1][2].

My opinion is that factoring out helpers that are useful only for
fanotify03,fanotify24 is suboptimal and we need to see if we can
create much more generic helpers that could be shared by more tests.

BTW, if you look closer, you will see that generate_events() is quite
different between fanotify03 and fanotify24, although it is true that
fanotify24 has a more generalized version that follows the expected
events more closely.

I did start with extending fanotify03 before I forked it and before the
fork generate_events() was even more hard to follow because of
the difference in expected events for write() between permission
and pre-content events.

Thanks,
Amir.

[1] https://lore.kernel.org/ltp/71d4414b-802f-4019-8527-e8886e2d1aeb@suse.cz/
[2] https://lore.kernel.org/ltp/20250131164217.GA1135694@pevik/

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and pre-content events
  2025-02-10 15:19   ` Jan Kara
@ 2025-02-11 17:55     ` Petr Vorel
  0 siblings, 0 replies; 21+ messages in thread
From: Petr Vorel @ 2025-02-11 17:55 UTC (permalink / raw)
  To: Jan Kara; +Cc: ltp

Hi Amir, Jan,

FYI this patch merged.

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-10 16:55     ` Amir Goldstein
@ 2025-02-11 17:56       ` Petr Vorel
  2025-02-12 12:35         ` Jan Kara
  0 siblings, 1 reply; 21+ messages in thread
From: Petr Vorel @ 2025-02-11 17:56 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

Hi Amir, Jan,

> On Mon, Feb 10, 2025 at 4:25 PM Jan Kara <jack@suse.cz> wrote:

> > On Mon 10-02-25 16:13:14, Amir Goldstein wrote:
> > > Verify that permission events are delivered iff parent is watching
> > > children.

> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>

> > Overall looks ok but:

> > > -     if (read(fd, buf, BUF_SIZE) != -1)
> > > +     exp_ret = exp_errno ? -1 : 1;
> > > +     errno = 0;
> > > +     if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
> > > +             tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
> > >               exit(3);
> > > +     } else if (errno == exp_errno) {
> >                 ^^ Why is this condition needed? It is always true AFAICT.

> > > +             tst_res(TINFO, "read() got errno %d as expected", errno);
> > > +     }

> > >       SAFE_CLOSE(fd);

> > > -     if (execve(FILE_EXEC_PATH, argv, environ) != -1)
> > > +     exp_ret = exp_errno ? -1 : 0;
> > > +     errno = 0;
> > > +     if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
> > > +             tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
> > >               exit(5);
> > > +     } else if (errno == exp_errno) {
> >                 ^^^ and here as well...

> > > +             tst_res(TINFO, "execve() got errno %d as expected", errno);

> You are right.
> I was "backported" from the pre-content test.
> The two else statements can be removed in this patch.

FYI I'm going to merge with the diff below (discussed change).

Jan, may I add your RBT?

Kind regards,
Petr

> Thanks,
> Amir.

diff --git testcases/kernel/syscalls/fanotify/fanotify03.c testcases/kernel/syscalls/fanotify/fanotify03.c
index 4b2f6e45e1..a19f49d131 100644
--- testcases/kernel/syscalls/fanotify/fanotify03.c
+++ testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -158,8 +158,6 @@ static void generate_events(struct tcase *tc)
 	if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
 		tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
 		exit(3);
-	} else if (errno == exp_errno) {
-		tst_res(TINFO, "read() got errno %d as expected", errno);
 	}
 
 	SAFE_CLOSE(fd);
@@ -169,8 +167,6 @@ static void generate_events(struct tcase *tc)
 	if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
 		tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
 		exit(5);
-	} else if (errno == exp_errno) {
-		tst_res(TINFO, "execve() got errno %d as expected", errno);
 	}
 }
 

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-10 17:06     ` Amir Goldstein
@ 2025-02-11 19:09       ` Petr Vorel
  2025-02-11 20:12         ` Amir Goldstein
  0 siblings, 1 reply; 21+ messages in thread
From: Petr Vorel @ 2025-02-11 19:09 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Martin Doucha, Jan Kara, ltp

> On Mon, Feb 10, 2025 at 4:43 PM Jan Kara <jack@suse.cz> wrote:

> > On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> > > Fork the test fanotify24 from test fanotify03, replacing the
> > > permission event FAN_ACCESS_PERM with the new pre-content event
> > > FAN_PRE_ACCESS.

> > > The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> > > required for FAN_PRE_ACCESS and this class also enabled the response
> > > with cutomer error code FAN_DENY_ERRNO.

> > > Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> > > system call.  The test case expected results are adjusted to
> > > respond with the default error (EPERM) to open() and write() and
> > > to respond with custom errors (EIO, EBUSY) to read() and execve().

> > > Not all fs support pre-content events, so run on all filesystems
> > > to excercise FAN_PRE_ACCESS on all supported filesystems.

> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>

> > Looks good to me. I was just wondering whether some bits like
> > generate_events(), mark setup, child setup, main test loop could not be
> > factored out into a helper functions used by both old and new tests?

> Yes, I agree that forking the tests is bad and that we need much
> more common helpers.

> IIUC, LTP developers are going to try to come up with some proposals
> for refactoring helpers to split some large fanotify tests [1][2].

> My opinion is that factoring out helpers that are useful only for
> fanotify03,fanotify24 is suboptimal and we need to see if we can
> create much more generic helpers that could be shared by more tests.

> BTW, if you look closer, you will see that generate_events() is quite
> different between fanotify03 and fanotify24, although it is true that
> fanotify24 has a more generalized version that follows the expected
> events more closely.

> I did start with extending fanotify03 before I forked it and before the
> fork generate_events() was even more hard to follow because of
> the difference in expected events for write() between permission
> and pre-content events.

I'm not happy that tests are nearly identical, but agree that merging them would
make readability even harder. Also if there is really no value to run
FAN_ACCESS_PERM (fanotify03.c) on all filesystems it would prolong testing.

The only downside of not factoring out common code is that fix in one test will
not appear in the other test (it looks to me from what you wrote that you
improved generate_events() for fanotify24.c but not for fanotify03.c. Also now
is for me to remember remove else if (errno == exp_errno) [1] also for
fanotify24.c). We have long history of this in LTP.  But I'm also not sure if
it's worth factoring out code just for 2 tests. We might reconsider factoring
out, but unless Martin or Cyril objects I would keep it for now.

I was looking briefly for code which could be turned into more generic helpers,
but so far I haven't found anything. Maybe you have better idea.

FYI there is not only test code and test output readability, but also ability to
filter out certain fix. If particular fix is not backported to enterprise kernel
(WONTFIX). With one of many tests fails we have no way to distinguish it in the
current code (tst_kvercmp2() does not always help). Therefore we prefer to have
regression tests in separate files (don't mix them with tests for basic
functionality). But that would probably lead to even more code duplicity.

I was thinking whether we could try allow to run only subset of struct tcase
items, based on getopt parameter. E.g. run only "inode" marks (first two items),
or mount marks (3rd and 4th), ...  runtest/syscalls would get more items of
particular test. It would prolong testing (for all_filesystems quite significantly)
but besides better test output readability + allow to filter out fixes.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/CAOQ4uxgu16dOsU4uuq66CGqXw6wY8c8jK7sL1QheB8kTPU=X+g@mail.gmail.com/

> Thanks,
> Amir.

> [1] https://lore.kernel.org/ltp/71d4414b-802f-4019-8527-e8886e2d1aeb@suse.cz/
> [2] https://lore.kernel.org/ltp/20250131164217.GA1135694@pevik/

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-11 19:09       ` Petr Vorel
@ 2025-02-11 20:12         ` Amir Goldstein
  2025-02-20 20:42           ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Amir Goldstein @ 2025-02-11 20:12 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Martin Doucha, Jan Kara, ltp

On Tue, Feb 11, 2025 at 8:09 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > On Mon, Feb 10, 2025 at 4:43 PM Jan Kara <jack@suse.cz> wrote:
>
> > > On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> > > > Fork the test fanotify24 from test fanotify03, replacing the
> > > > permission event FAN_ACCESS_PERM with the new pre-content event
> > > > FAN_PRE_ACCESS.
>
> > > > The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> > > > required for FAN_PRE_ACCESS and this class also enabled the response
> > > > with cutomer error code FAN_DENY_ERRNO.
>
> > > > Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> > > > system call.  The test case expected results are adjusted to
> > > > respond with the default error (EPERM) to open() and write() and
> > > > to respond with custom errors (EIO, EBUSY) to read() and execve().
>
> > > > Not all fs support pre-content events, so run on all filesystems
> > > > to excercise FAN_PRE_ACCESS on all supported filesystems.
>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> > > Looks good to me. I was just wondering whether some bits like
> > > generate_events(), mark setup, child setup, main test loop could not be
> > > factored out into a helper functions used by both old and new tests?
>
> > Yes, I agree that forking the tests is bad and that we need much
> > more common helpers.
>
> > IIUC, LTP developers are going to try to come up with some proposals
> > for refactoring helpers to split some large fanotify tests [1][2].
>
> > My opinion is that factoring out helpers that are useful only for
> > fanotify03,fanotify24 is suboptimal and we need to see if we can
> > create much more generic helpers that could be shared by more tests.
>
> > BTW, if you look closer, you will see that generate_events() is quite
> > different between fanotify03 and fanotify24, although it is true that
> > fanotify24 has a more generalized version that follows the expected
> > events more closely.
>
> > I did start with extending fanotify03 before I forked it and before the
> > fork generate_events() was even more hard to follow because of
> > the difference in expected events for write() between permission
> > and pre-content events.
>
> I'm not happy that tests are nearly identical, but agree that merging them would
> make readability even harder. Also if there is really no value to run
> FAN_ACCESS_PERM (fanotify03.c) on all filesystems it would prolong testing.
>
> The only downside of not factoring out common code is that fix in one test will
> not appear in the other test (it looks to me from what you wrote that you
> improved generate_events() for fanotify24.c but not for fanotify03.c. Also now
> is for me to remember remove else if (errno == exp_errno) [1] also for
> fanotify24.c).

Please don't remove these else from fanotify24.
I meant that the else in fanotify03.c are not needed because they came

from the generic generate_events() of fanotify24.c which supports
different expected errno values (FAN_DENY_ERRNO(xxx)).
fanotify03 does not have FAN_DENY_ERRNO(xxx), so comparing exp_errno
is not needed in fanbotify03. It is needed in fanotify24.

> We have long history of this in LTP.  But I'm also not sure if
> it's worth factoring out code just for 2 tests. We might reconsider factoring
> out, but unless Martin or Cyril objects I would keep it for now.
>
> I was looking briefly for code which could be turned into more generic helpers,
> but so far I haven't found anything. Maybe you have better idea.
>

I do not have a better idea.
fanotify03 and fanotify24 are sufficiently similar to dup a lot of code
and sufficiently different to make common code hard or more
complex.

IMO, the code that is relatively common to many fanotify tests is the
event read and process loop.

I think it should be doable to make these loops use common helpers for
reading and verifying the expected events, but it is not a small job to make
those helpers generic enough to cater all the different tests that check
different event formats.

Thanks,
Amir.

> FYI there is not only test code and test output readability, but also ability to
> filter out certain fix. If particular fix is not backported to enterprise kernel
> (WONTFIX). With one of many tests fails we have no way to distinguish it in the
> current code (tst_kvercmp2() does not always help). Therefore we prefer to have
> regression tests in separate files (don't mix them with tests for basic
> functionality). But that would probably lead to even more code duplicity.
>
> I was thinking whether we could try allow to run only subset of struct tcase
> items, based on getopt parameter. E.g. run only "inode" marks (first two items),
> or mount marks (3rd and 4th), ...  runtest/syscalls would get more items of
> particular test. It would prolong testing (for all_filesystems quite significantly)
> but besides better test output readability + allow to filter out fixes.
>
> Kind regards,
> Petr
>
> [1] https://lore.kernel.org/ltp/CAOQ4uxgu16dOsU4uuq66CGqXw6wY8c8jK7sL1QheB8kTPU=X+g@mail.gmail.com/
>
> > Thanks,
> > Amir.
>
> > [1] https://lore.kernel.org/ltp/71d4414b-802f-4019-8527-e8886e2d1aeb@suse.cz/
> > [2] https://lore.kernel.org/ltp/20250131164217.GA1135694@pevik/

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-11 17:56       ` Petr Vorel
@ 2025-02-12 12:35         ` Jan Kara
  2025-02-13  7:54           ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Kara @ 2025-02-12 12:35 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Jan Kara, ltp

On Tue 11-02-25 18:56:39, Petr Vorel wrote:
> Hi Amir, Jan,
> 
> > On Mon, Feb 10, 2025 at 4:25 PM Jan Kara <jack@suse.cz> wrote:
> 
> > > On Mon 10-02-25 16:13:14, Amir Goldstein wrote:
> > > > Verify that permission events are delivered iff parent is watching
> > > > children.
> 
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> 
> > > Overall looks ok but:
> 
> > > > -     if (read(fd, buf, BUF_SIZE) != -1)
> > > > +     exp_ret = exp_errno ? -1 : 1;
> > > > +     errno = 0;
> > > > +     if (read(fd, buf, BUF_SIZE) != exp_ret || errno != exp_errno) {
> > > > +             tst_res(TFAIL, "read() got errno %d (expected %d)", errno, exp_errno);
> > > >               exit(3);
> > > > +     } else if (errno == exp_errno) {
> > >                 ^^ Why is this condition needed? It is always true AFAICT.
> 
> > > > +             tst_res(TINFO, "read() got errno %d as expected", errno);
> > > > +     }
> 
> > > >       SAFE_CLOSE(fd);
> 
> > > > -     if (execve(FILE_EXEC_PATH, argv, environ) != -1)
> > > > +     exp_ret = exp_errno ? -1 : 0;
> > > > +     errno = 0;
> > > > +     if (execve(FILE_EXEC_PATH, argv, environ) != exp_ret || errno != exp_errno) {
> > > > +             tst_res(TFAIL, "execve() got errno %d (expected %d)", errno, exp_errno);
> > > >               exit(5);
> > > > +     } else if (errno == exp_errno) {
> > >                 ^^^ and here as well...
> 
> > > > +             tst_res(TINFO, "execve() got errno %d as expected", errno);
> 
> > You are right.
> > I was "backported" from the pre-content test.
> > The two else statements can be removed in this patch.
> 
> FYI I'm going to merge with the diff below (discussed change).
> 
> Jan, may I add your RBT?

Yes. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children
  2025-02-12 12:35         ` Jan Kara
@ 2025-02-13  7:54           ` Petr Vorel
  0 siblings, 0 replies; 21+ messages in thread
From: Petr Vorel @ 2025-02-13  7:54 UTC (permalink / raw)
  To: Jan Kara; +Cc: ltp

Hi Amir, Jan,
...
> > > You are right.
> > > I was "backported" from the pre-content test.
> > > The two else statements can be removed in this patch.

> > FYI I'm going to merge with the diff below (discussed change).

> > Jan, may I add your RBT?

> Yes. Feel free to add:

> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, merged with the change above.

Kind regards,
Petr

> 								Honza

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-11 20:12         ` Amir Goldstein
@ 2025-02-20 20:42           ` Petr Vorel
  2025-02-21 13:26             ` Petr Vorel
  0 siblings, 1 reply; 21+ messages in thread
From: Petr Vorel @ 2025-02-20 20:42 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Martin Doucha, Jan Kara, ltp

> On Tue, Feb 11, 2025 at 8:09 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > On Mon, Feb 10, 2025 at 4:43 PM Jan Kara <jack@suse.cz> wrote:

> > > > On Mon 10-02-25 16:13:15, Amir Goldstein wrote:
> > > > > Fork the test fanotify24 from test fanotify03, replacing the
> > > > > permission event FAN_ACCESS_PERM with the new pre-content event
> > > > > FAN_PRE_ACCESS.

> > > > > The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> > > > > required for FAN_PRE_ACCESS and this class also enabled the response
> > > > > with cutomer error code FAN_DENY_ERRNO.

> > > > > Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> > > > > system call.  The test case expected results are adjusted to
> > > > > respond with the default error (EPERM) to open() and write() and
> > > > > to respond with custom errors (EIO, EBUSY) to read() and execve().

> > > > > Not all fs support pre-content events, so run on all filesystems
> > > > > to excercise FAN_PRE_ACCESS on all supported filesystems.

> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>

> > > > Looks good to me. I was just wondering whether some bits like
> > > > generate_events(), mark setup, child setup, main test loop could not be
> > > > factored out into a helper functions used by both old and new tests?

> > > Yes, I agree that forking the tests is bad and that we need much
> > > more common helpers.

> > > IIUC, LTP developers are going to try to come up with some proposals
> > > for refactoring helpers to split some large fanotify tests [1][2].

> > > My opinion is that factoring out helpers that are useful only for
> > > fanotify03,fanotify24 is suboptimal and we need to see if we can
> > > create much more generic helpers that could be shared by more tests.

> > > BTW, if you look closer, you will see that generate_events() is quite
> > > different between fanotify03 and fanotify24, although it is true that
> > > fanotify24 has a more generalized version that follows the expected
> > > events more closely.

> > > I did start with extending fanotify03 before I forked it and before the
> > > fork generate_events() was even more hard to follow because of
> > > the difference in expected events for write() between permission
> > > and pre-content events.

> > I'm not happy that tests are nearly identical, but agree that merging them would
> > make readability even harder. Also if there is really no value to run
> > FAN_ACCESS_PERM (fanotify03.c) on all filesystems it would prolong testing.

> > The only downside of not factoring out common code is that fix in one test will
> > not appear in the other test (it looks to me from what you wrote that you
> > improved generate_events() for fanotify24.c but not for fanotify03.c. Also now
> > is for me to remember remove else if (errno == exp_errno) [1] also for
> > fanotify24.c).

> Please don't remove these else from fanotify24.
> I meant that the else in fanotify03.c are not needed because they came

> from the generic generate_events() of fanotify24.c which supports
> different expected errno values (FAN_DENY_ERRNO(xxx)).
> fanotify03 does not have FAN_DENY_ERRNO(xxx), so comparing exp_errno
> is not needed in fanbotify03. It is needed in fanotify24.

Ah, sure, I'll keep them.

> > We have long history of this in LTP.  But I'm also not sure if
> > it's worth factoring out code just for 2 tests. We might reconsider factoring
> > out, but unless Martin or Cyril objects I would keep it for now.

> > I was looking briefly for code which could be turned into more generic helpers,
> > but so far I haven't found anything. Maybe you have better idea.


> I do not have a better idea.
> fanotify03 and fanotify24 are sufficiently similar to dup a lot of code
> and sufficiently different to make common code hard or more
> complex.

> IMO, the code that is relatively common to many fanotify tests is the
> event read and process loop.

> I think it should be doable to make these loops use common helpers for
> reading and verifying the expected events, but it is not a small job to make
> those helpers generic enough to cater all the different tests that check
> different event formats.

Thanks for a hint. I'll see if I find a time and come up with something useful.

We briefly talk with Cyril about this and agree that we don't want to block
this. I'll wait little longer if Cyril wants to add something and merge during
tomorrow. Again, thanks a lot for updating fanotify testcases in LTP.

Kind regards,
Petr

> Thanks,
> Amir.

> > FYI there is not only test code and test output readability, but also ability to
> > filter out certain fix. If particular fix is not backported to enterprise kernel
> > (WONTFIX). With one of many tests fails we have no way to distinguish it in the
> > current code (tst_kvercmp2() does not always help). Therefore we prefer to have
> > regression tests in separate files (don't mix them with tests for basic
> > functionality). But that would probably lead to even more code duplicity.

> > I was thinking whether we could try allow to run only subset of struct tcase
> > items, based on getopt parameter. E.g. run only "inode" marks (first two items),
> > or mount marks (3rd and 4th), ...  runtest/syscalls would get more items of
> > particular test. It would prolong testing (for all_filesystems quite significantly)
> > but besides better test output readability + allow to filter out fixes.

> > Kind regards,
> > Petr

> > [1] https://lore.kernel.org/ltp/CAOQ4uxgu16dOsU4uuq66CGqXw6wY8c8jK7sL1QheB8kTPU=X+g@mail.gmail.com/

> > > Thanks,
> > > Amir.

> > > [1] https://lore.kernel.org/ltp/71d4414b-802f-4019-8527-e8886e2d1aeb@suse.cz/
> > > [2] https://lore.kernel.org/ltp/20250131164217.GA1135694@pevik/

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-20 20:42           ` Petr Vorel
@ 2025-02-21 13:26             ` Petr Vorel
  2025-02-21 13:37               ` Amir Goldstein
  0 siblings, 1 reply; 21+ messages in thread
From: Petr Vorel @ 2025-02-21 13:26 UTC (permalink / raw)
  To: Amir Goldstein, Jan Kara, Cyril Hrubis, ltp, Martin Doucha

Hi all,

FYI remaining 3rd and 4th patch merged.
Thanks to all!

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-21 13:26             ` Petr Vorel
@ 2025-02-21 13:37               ` Amir Goldstein
  0 siblings, 0 replies; 21+ messages in thread
From: Amir Goldstein @ 2025-02-21 13:37 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Martin Doucha, Jan Kara, ltp

On Fri, Feb 21, 2025 at 2:26 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi all,
>
> FYI remaining 3rd and 4th patch merged.
> Thanks to all!

Thank !you!
Amir.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO
  2025-02-10 15:13 ` [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO Amir Goldstein
  2025-02-10 15:43   ` Jan Kara
@ 2025-02-28 20:11   ` Heiko Carstens
  1 sibling, 0 replies; 21+ messages in thread
From: Heiko Carstens @ 2025-02-28 20:11 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, ltp

On Mon, Feb 10, 2025 at 04:13:15PM +0100, Amir Goldstein wrote:
> Fork the test fanotify24 from test fanotify03, replacing the
> permission event FAN_ACCESS_PERM with the new pre-content event
> FAN_PRE_ACCESS.
> 
> The test is changed to use class FAN_CLASS_PRE_CONTENT, which is
> required for FAN_PRE_ACCESS and this class also enabled the response
> with cutomer error code FAN_DENY_ERRNO.
> 
> Unlike FAN_ACCESS_PERM, FAN_PRE_ACCESS is also created on write()
> system call.  The test case expected results are adjusted to
> respond with the default error (EPERM) to open() and write() and
> to respond with custom errors (EIO, EBUSY) to read() and execve().
> 
> Not all fs support pre-content events, so run on all filesystems
> to excercise FAN_PRE_ACCESS on all supported filesystems.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/lapi/fanotify.h                       |   7 +
>  runtest/syscalls                              |   1 +
>  testcases/kernel/syscalls/fanotify/.gitignore |   1 +
>  testcases/kernel/syscalls/fanotify/fanotify.h |   3 +
>  .../kernel/syscalls/fanotify/fanotify24.c     | 433 ++++++++++++++++++
>  5 files changed, 445 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify24.c
> 
> diff --git a/include/lapi/fanotify.h b/include/lapi/fanotify.h
> index e5b930f4e..9076685e8 100644
> --- a/include/lapi/fanotify.h
> +++ b/include/lapi/fanotify.h
> @@ -124,6 +124,13 @@
>  #define FAN_EPIDFD		-2
>  #endif
>  
> +/* errno other than EPERM can specified in upper byte of deny response */
> +#ifndef FAN_DENY_ERRNO
> +#define FAN_ERRNO(err) (((((__u32)(err)) & 0xff) << 24))
> +#define FAN_DENY_ERRNO(err) (FAN_DENY | FAN_ERRNO(err))
> +#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> 24))
> +#endif
> +

This does not work with latest+greatest kernel headers since there
FAN_DENY_ERRNO is defined but FAN_RESPONSE_ERRNO is not. Therefore
this ends up in a compile error:

fanotify24.c: In function ‘expected_errno’:
fanotify24.c:166:24: error: implicit declaration of function ‘FAN_RESPONSE_ERRNO’; did you mean ‘FAN_DENY_ERRNO’? [-Wimplicit-function-declaration]
  166 |                 return FAN_RESPONSE_ERRNO(response);
      |                        ^~~~~~~~~~~~~~~~~~
      |                        FAN_DENY_ERRNO

FWIW, converting the above to:

/* errno other than EPERM can specified in upper byte of deny response */
#ifndef FAN_DENY_ERRNO
#define FAN_ERRNO(err) (((((__u32)(err)) & 0xff) << 24))
#define FAN_DENY_ERRNO(err) (FAN_DENY | FAN_ERRNO(err))
#endif

#ifndef FAN_RESPONSE_ERRNO
#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> 24))
#endif

works for me.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2025-02-28 20:11 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-10 15:13 [LTP] [PATCH 0/4] New tests for fanotify pre-content events Amir Goldstein
2025-02-10 15:13 ` [LTP] [PATCH 1/4] fanotify14: Test invalid init flags with permission and " Amir Goldstein
2025-02-10 15:19   ` Jan Kara
2025-02-11 17:55     ` Petr Vorel
2025-02-10 15:13 ` [LTP] [PATCH 2/4] fanotify03: Add test cases for permission events on children Amir Goldstein
2025-02-10 15:24   ` Jan Kara
2025-02-10 16:55     ` Amir Goldstein
2025-02-11 17:56       ` Petr Vorel
2025-02-12 12:35         ` Jan Kara
2025-02-13  7:54           ` Petr Vorel
2025-02-10 15:13 ` [LTP] [PATCH 3/4] fanotify24: Add test for FAN_PRE_ACCESS and FAN_DENY_ERRNO Amir Goldstein
2025-02-10 15:43   ` Jan Kara
2025-02-10 17:06     ` Amir Goldstein
2025-02-11 19:09       ` Petr Vorel
2025-02-11 20:12         ` Amir Goldstein
2025-02-20 20:42           ` Petr Vorel
2025-02-21 13:26             ` Petr Vorel
2025-02-21 13:37               ` Amir Goldstein
2025-02-28 20:11   ` Heiko Carstens
2025-02-10 15:13 ` [LTP] [PATCH 4/4] fanotify24: Test open for write of executable files with pre-content watch Amir Goldstein
2025-02-10 15:59   ` Jan Kara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox