[LTP] [PATCH 0/5] fanotify filesystem mark tests

[LTP] [PATCH 0/5] fanotify filesystem mark tests

[Amir Goldstein]

Hi Cyril,

This series adds some new tests to fanotify and is also available on
my github tree [1].

Patches 1-2 re-structure tests fanotify{01,03} to iterate over the
fanotify mark types and add some missing test coverage for FAN_MARK_MOUNT.

Patches 3-5 add test coverage for the new FAN_MARK_FILESYSTEM API, that
was merged for kernel v4.20-rc1, to tests fanotify{01,03,10}.
Older kernels will report TCONF on the new test cases.

Previous patches from me pending your review:
- Add test case to fanotify09 to track a bug fix in linux-next [2]
- Enhance readahead02 to track an overlayfs regression fix in v4.19 [3]

Thanks,
Amir.

[1] https://github.com/amir73il/ltp/commits/fanotify_sb
[2] https://github.com/amir73il/ltp/commits/fanotify_ignore
[3] https://github.com/amir73il/ltp/commits/readahead_fadvise

Amir Goldstein (5):
  syscalls/fanotify01: check events also on mountpoint mark
  syscalls/fanotify03: check permission events also on mountpoint mark
  syscalls/fanotify01: add test for FAN_MARK_FILESYSTEM events
  syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission
    events
  syscalls/fanotify10: add tests for FAN_MARK_FILESYSTEM and ignore mask

 testcases/kernel/syscalls/fanotify/fanotify.h |  3 +
 .../kernel/syscalls/fanotify/fanotify01.c     | 92 ++++++++++++-------
 .../kernel/syscalls/fanotify/fanotify03.c     | 55 +++++++++--
 .../kernel/syscalls/fanotify/fanotify10.c     | 40 +++++++-
 4 files changed, 148 insertions(+), 42 deletions(-)

-- 
2.17.1

[LTP] [PATCH 1/5] syscalls/fanotify01: check events also on mountpoint mark

[Amir Goldstein]

Add index to test and repeat it for mark types inode and mountpoint.
Move fanotify_init() into the test to simplify setup/cleanup.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify01.c     | 83 ++++++++++++-------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify01.c b/testcases/kernel/syscalls/fanotify/fanotify01.c
index e1210134b..5dfb67b61 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify01.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify01.c
@@ -32,26 +32,46 @@
 #define BUF_SIZE 256
 #define TST_TOTAL 12
 
+static struct tcase {
+	const char *tname;
+	struct fanotify_mark_type mark;
+} tcases[] = {
+	{
+		"inode mark events",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+	},
+	{
+		"mount mark events",
+		INIT_FANOTIFY_MARK_TYPE(MOUNT),
+	},
+};
+
 static char fname[BUF_SIZE];
 static char buf[BUF_SIZE];
-static int fd, fd_notify;
+static int fd_notify;
 
 static unsigned long long event_set[EVENT_MAX];
 
 static char event_buf[EVENT_BUF_LEN];
 
-void test01(void)
+static void test_fanotify(unsigned int n)
 {
-	int ret, len, i = 0, test_num = 0;
-
+	struct tcase *tc = &tcases[n];
+	struct fanotify_mark_type *mark = &tc->mark;
+	int fd, ret, len, i = 0, test_num = 0;
 	int tst_count = 0;
 
-	if (fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_ACCESS | FAN_MODIFY |
-			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
+	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+
+	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
+
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
+			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
+			  AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | %s | "
 			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-			"failed", fd_notify, fname);
+			"failed", fd_notify, mark->name, fname);
 	}
 
 	/*
@@ -101,12 +121,12 @@ void test01(void)
 
 	/* Ignore access events */
 	if (fanotify_mark(fd_notify,
-			  FAN_MARK_ADD | FAN_MARK_IGNORED_MASK,
+			  FAN_MARK_ADD | mark->flag | FAN_MARK_IGNORED_MASK,
 			  FAN_ACCESS, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD | "
-			"FAN_MARK_IGNORED_MASK, FAN_ACCESS, "
-			"AT_FDCWD, %s) failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | %s | "
+			"FAN_MARK_IGNORED_MASK, FAN_ACCESS, AT_FDCWD, %s) "
+			"failed", fd_notify, mark->name, fname);
 	}
 
 	fd = SAFE_OPEN(fname, O_RDWR);
@@ -150,15 +170,14 @@ void test01(void)
 	 * Now ignore open & close events regardless of file
 	 * modifications
 	 */
-	if (fanotify_mark(fd_notify,
-			    FAN_MARK_ADD | FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
-			    FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag |
+			  FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
+			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD | "
-			"FAN_MARK_IGNORED_MASK | "
-			"FAN_MARK_IGNORED_SURV_MODIFY, FAN_OPEN | "
-			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-			fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | %s | "
+			"FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY, "
+			"FAN_OPEN | FAN_CLOSE, AT_FDCWD, %s) failed",
+			fd_notify, mark->name, fname);
 	}
 
 	/* This event should be ignored */
@@ -181,13 +200,13 @@ void test01(void)
 
 	/* Now remove open and close from ignored mask */
 	if (fanotify_mark(fd_notify,
-			  FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
+			  FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
 			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_REMOVE | "
-			"FAN_MARK_IGNORED_MASK, FAN_OPEN | "
-			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-			fname);
+			"fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
+			"FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
+			"AT_FDCWD, %s) failed", fd_notify,
+			mark->name, fname);
 	}
 
 	SAFE_CLOSE(fd);
@@ -283,12 +302,13 @@ pass:
 
 	}
 	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
-	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
-			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
+	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
+			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
+			  AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
+			"fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
 			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-			"failed", fd_notify, fname);
+			"failed", fd_notify, mark->name, fname);
 	}
 }
 
@@ -296,8 +316,6 @@ static void setup(void)
 {
 	sprintf(fname, "tfile_%d", getpid());
 	SAFE_FILE_PRINTF(fname, "1");
-
-	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
 }
 
 static void cleanup(void)
@@ -307,7 +325,8 @@ static void cleanup(void)
 }
 
 static struct tst_test test = {
-	.test_all = test01,
+	.test = test_fanotify,
+	.tcnt = ARRAY_SIZE(tcases),
 	.setup = setup,
 	.cleanup = cleanup,
 	.needs_tmpdir = 1,
-- 
2.17.1

[LTP] [PATCH 2/5] syscalls/fanotify03: check permission events also on mountpoint mark

[Amir Goldstein]

Add index to test and repeat it for mark types inode and mountpoint.

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

diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
index 42aac813e..19daf57ef 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify03.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -47,6 +47,20 @@ static unsigned int event_resp[EVENT_MAX];
 
 static char event_buf[EVENT_BUF_LEN];
 
+static struct tcase {
+	const char *tname;
+	struct fanotify_mark_type mark;
+} tcases[] = {
+	{
+		"inode mark permission events",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+	},
+	{
+		"mount mark permission events",
+		INIT_FANOTIFY_MARK_TYPE(MOUNT),
+	},
+};
+
 static void generate_events(void)
 {
 	int fd;
@@ -120,11 +134,14 @@ static void check_child(void)
 		tst_res(TFAIL, "child %s", tst_strstatus(child_ret));
 }
 
-static void setup_mark(void)
+static void setup_mark(unsigned int n)
 {
+	struct tcase *tc = &tcases[n];
+	struct fanotify_mark_type *mark = &tc->mark;
+
 	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
 
-	if (fanotify_mark(fd_notify, FAN_MARK_ADD,
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
 			  FAN_ACCESS_PERM | FAN_OPEN_PERM,
 			  AT_FDCWD, fname) < 0) {
 		if (errno == EINVAL) {
@@ -133,20 +150,22 @@ static void setup_mark(void)
 				"configured in kernel?");
 		} else {
 			tst_brk(TBROK | TERRNO,
-				"fanotify_mark (%d, FAN_MARK_ADD, "
+				"fanotify_mark (%d, FAN_MARK_ADD | %s, "
 				"FAN_ACCESS_PERM | FAN_OPEN_PERM, "
 				"AT_FDCWD, %s) failed.",
-				fd_notify, fname);
+				fd_notify, mark->name, fname);
 		}
 	}
+
+	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
 }
 
-void test01(void)
+static void test_fanotify(unsigned int n)
 {
 	int tst_count;
 	int ret, len = 0, i = 0, test_num = 0;
 
-	setup_mark();
+	setup_mark(n);
 	run_child();
 
 	tst_count = 0;
@@ -247,7 +266,8 @@ static void cleanup(void)
 }
 
 static struct tst_test test = {
-	.test_all = test01,
+	.test = test_fanotify,
+	.tcnt = ARRAY_SIZE(tcases),
 	.setup = setup,
 	.cleanup = cleanup,
 	.needs_tmpdir = 1,
-- 
2.17.1

[LTP] [PATCH 3/5] syscalls/fanotify01: add test for FAN_MARK_FILESYSTEM events

[Amir Goldstein]

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 testcases/kernel/syscalls/fanotify/fanotify.h   | 3 +++
 testcases/kernel/syscalls/fanotify/fanotify01.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 5adef54d7..535f1cef2 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -57,6 +57,9 @@ static long fanotify_mark(int fd, unsigned int flags, uint64_t mask,
 #ifndef FAN_MARK_INODE
 #define FAN_MARK_INODE		0
 #endif
+#ifndef FAN_MARK_FILESYSTEM
+#define FAN_MARK_FILESYSTEM	0x00000100
+#endif
 
 struct fanotify_mark_type {
 	unsigned int flag;
diff --git a/testcases/kernel/syscalls/fanotify/fanotify01.c b/testcases/kernel/syscalls/fanotify/fanotify01.c
index 5dfb67b61..c3d299474 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify01.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify01.c
@@ -44,6 +44,10 @@ static struct tcase {
 		"mount mark events",
 		INIT_FANOTIFY_MARK_TYPE(MOUNT),
 	},
+	{
+		"filesystem mark events",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+	},
 };
 
 static char fname[BUF_SIZE];
@@ -68,6 +72,11 @@ static void test_fanotify(unsigned int n)
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
 			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
 			  AT_FDCWD, fname) < 0) {
+		if (errno == EINVAL && mark->flag == FAN_MARK_FILESYSTEM) {
+			tst_res(TCONF,
+				"FAN_MARK_FILESYSTEM not supported in kernel?");
+			return;
+		}
 		tst_brk(TBROK | TERRNO,
 			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | %s | "
 			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-- 
2.17.1

[LTP] [PATCH 4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

[Amir Goldstein]

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

diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
index 19daf57ef..5c105ed32 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify03.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -46,6 +46,7 @@ static unsigned long long event_set[EVENT_MAX];
 static unsigned int event_resp[EVENT_MAX];
 
 static char event_buf[EVENT_BUF_LEN];
+static int support_perm_events;
 
 static struct tcase {
 	const char *tname;
@@ -59,6 +60,10 @@ static struct tcase {
 		"mount mark permission events",
 		INIT_FANOTIFY_MARK_TYPE(MOUNT),
 	},
+	{
+		"filesystem mark permission events",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+	},
 };
 
 static void generate_events(void)
@@ -134,7 +139,7 @@ static void check_child(void)
 		tst_res(TFAIL, "child %s", tst_strstatus(child_ret));
 }
 
-static void setup_mark(unsigned int n)
+static int setup_mark(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
 	struct fanotify_mark_type *mark = &tc->mark;
@@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
 			  FAN_ACCESS_PERM | FAN_OPEN_PERM,
 			  AT_FDCWD, fname) < 0) {
-		if (errno == EINVAL) {
+		if (errno == EINVAL && support_perm_events &&
+		    mark->flag == FAN_MARK_FILESYSTEM) {
+			tst_res(TCONF,
+				"FAN_MARK_FILESYSTEM not supported in kernel?");
+			return -1;
+		} else if (errno == EINVAL) {
 			tst_brk(TCONF | TERRNO,
 				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
 				"configured in kernel?");
@@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
 				"AT_FDCWD, %s) failed.",
 				fd_notify, mark->name, fname);
 		}
+	} else {
+		/*
+		 * To distigouish between perm event not supported and
+		 * filesystem mark not supported.
+		 */
+		support_perm_events = 1;
 	}
 
 	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+	return 0;
 }
 
 static void test_fanotify(unsigned int n)
@@ -165,7 +182,9 @@ static void test_fanotify(unsigned int n)
 	int tst_count;
 	int ret, len = 0, i = 0, test_num = 0;
 
-	setup_mark(n);
+	if (setup_mark(n) != 0)
+		return;
+
 	run_child();
 
 	tst_count = 0;
-- 
2.17.1

[LTP] [PATCH 5/5] syscalls/fanotify10: add tests for FAN_MARK_FILESYSTEM and ignore mask

[Amir Goldstein]

A watch with FAN_MARK_FILESYSTEM should ignore events that were
generated on files whose inode have an ignore mask and ignore events
generated on a mount that has a mark with ignore mask.

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

diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
index 2b96de2f1..820c50a8a 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify10.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
@@ -66,11 +66,13 @@ static int bind_mount_created;
 enum {
 	FANOTIFY_INODE,
 	FANOTIFY_MOUNT,
+	FANOTIFY_FILESYSTEM,
 };
 
 static struct fanotify_mark_type fanotify_mark_types[] = {
 	INIT_FANOTIFY_MARK_TYPE(INODE),
 	INIT_FANOTIFY_MARK_TYPE(MOUNT),
+	INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
 };
 
 static struct tcase {
@@ -106,9 +108,33 @@ static struct tcase {
 		MNT2_PATH, FANOTIFY_MOUNT,
 		FILE_PATH, 1
 	},
+	{
+		"ignore fs events created on a specific file",
+		MOUNT_PATH, FANOTIFY_FILESYSTEM,
+		FILE_PATH, FANOTIFY_INODE,
+		FILE_PATH, 0
+	},
+	{
+		"don't ignore mount events created on another file",
+		MOUNT_PATH, FANOTIFY_FILESYSTEM,
+		FILE_PATH, FANOTIFY_INODE,
+		FILE2_PATH, 1
+	},
+	{
+		"ignore fs events created on a specific mount point",
+		MOUNT_PATH, FANOTIFY_FILESYSTEM,
+		MNT2_PATH, FANOTIFY_MOUNT,
+		FILE_MNT2, 0
+	},
+	{
+		"don't ignore fs events created on another mount point",
+		MOUNT_PATH, FANOTIFY_FILESYSTEM,
+		MNT2_PATH, FANOTIFY_MOUNT,
+		FILE_PATH, 1
+	},
 };
 
-static void create_fanotify_groups(unsigned int n)
+static int create_fanotify_groups(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
 	struct fanotify_mark_type *mark, *ignore_mark;
@@ -129,6 +155,13 @@ static void create_fanotify_groups(unsigned int n)
 					    FAN_MARK_ADD | mark->flag,
 					    FAN_OPEN, AT_FDCWD, tc->mark_path);
 			if (ret < 0) {
+				if (errno == EINVAL &&
+				    tc->mark_type == FANOTIFY_FILESYSTEM) {
+					tst_res(TCONF,
+						"FAN_MARK_FILESYSTEM not "
+						"supported in kernel?");
+					return -1;
+				}
 				tst_brk(TBROK | TERRNO,
 					"fanotify_mark(%d, FAN_MARK_ADD | %s,"
 					"FAN_OPEN, AT_FDCWD, %s) failed",
@@ -155,6 +188,7 @@ static void create_fanotify_groups(unsigned int n)
 			}
 		}
 	}
+	return 0;
 }
 
 static void cleanup_fanotify_groups(void)
@@ -198,7 +232,8 @@ static void test_fanotify(unsigned int n)
 
 	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
 
-	create_fanotify_groups(n);
+	if (create_fanotify_groups(n) != 0)
+		goto cleanup;
 
 	mark = &fanotify_mark_types[tc->mark_type];
 	ignore_mark = &fanotify_mark_types[tc->ignore_mark_type];
@@ -268,6 +303,7 @@ static void test_fanotify(unsigned int n)
 			}
 		}
 	}
+cleanup:
 	cleanup_fanotify_groups();
 }
 
-- 
2.17.1

[LTP] [PATCH 1/5] syscalls/fanotify01: check events also on mountpoint mark

[Cyril Hrubis]

Hi!
>  	/* This event should be ignored */
> @@ -181,13 +200,13 @@ void test01(void)
>  
>  	/* Now remove open and close from ignored mask */
>  	if (fanotify_mark(fd_notify,
> -			  FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
> +			  FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
>  			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
>  		tst_brk(TBROK | TERRNO,
> -			"fanotify_mark (%d, FAN_MARK_REMOVE | "
> -			"FAN_MARK_IGNORED_MASK, FAN_OPEN | "
> -			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
> -			fname);
> +			"fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
> +			"FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
> +			"AT_FDCWD, %s) failed", fd_notify,
> +			mark->name, fname);
>  	}
>  
>  	SAFE_CLOSE(fd);
> @@ -283,12 +302,13 @@ pass:
>  
>  	}
>  	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> -	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
> -			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
> +	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
> +			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
> +			  AT_FDCWD, fname) < 0) {
>  		tst_brk(TBROK | TERRNO,
> -			"fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
> +			"fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
>  			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
> -			"failed", fd_notify, fname);
> +			"failed", fd_notify, mark->name, fname);
>  	}
>  }
>  
> @@ -296,8 +316,6 @@ static void setup(void)
>  {
>  	sprintf(fname, "tfile_%d", getpid());
>  	SAFE_FILE_PRINTF(fname, "1");
> -
> -	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);

I've added SAFE_CLOSE(fd_notify) here so that we do not use all
available file descriptors with large enough -i parameter and pushed.

I do wonder what is the reason to move the fanotify_init() from setup
the the test_fanotify(), is there a problem with recycling the same fd
for different tests?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/5] syscalls/fanotify03: check permission events also on mountpoint mark

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls/fanotify01: add test for FAN_MARK_FILESYSTEM events

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

[Cyril Hrubis]

Hi!
> -static void setup_mark(unsigned int n)
> +static int setup_mark(unsigned int n)
>  {
>  	struct tcase *tc = &tcases[n];
>  	struct fanotify_mark_type *mark = &tc->mark;
> @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
>  	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
>  			  FAN_ACCESS_PERM | FAN_OPEN_PERM,
>  			  AT_FDCWD, fname) < 0) {
> -		if (errno == EINVAL) {
> +		if (errno == EINVAL && support_perm_events &&
> +		    mark->flag == FAN_MARK_FILESYSTEM) {
> +			tst_res(TCONF,
> +				"FAN_MARK_FILESYSTEM not supported in kernel?");
> +			return -1;
> +		} else if (errno == EINVAL) {
>  			tst_brk(TCONF | TERRNO,
>  				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
>  				"configured in kernel?");
> @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
>  				"AT_FDCWD, %s) failed.",
>  				fd_notify, mark->name, fname);
>  		}
> +	} else {
> +		/*
> +		 * To distigouish between perm event not supported and
> +		 * filesystem mark not supported.
> +		 */
> +		support_perm_events = 1;

I'm a bit puzzled here, so we attempted to cache if perm_events are
supported here?

I guess that we depend on the order of the tcases[] array here, which is
not very nice.

Also it does not have to be in else branch, if we get EINVAL the first
time we call fanotify_mark() tst_brk() is called, which exits the test,
so if we ever get to this point in the program, we did at least one
successful mark.

>  	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
> +	return 0;
>  }
>  
>  static void test_fanotify(unsigned int n)
> @@ -165,7 +182,9 @@ static void test_fanotify(unsigned int n)
>  	int tst_count;
>  	int ret, len = 0, i = 0, test_num = 0;
>  
> -	setup_mark(n);
> +	if (setup_mark(n) != 0)
> +		return;
> +
>  	run_child();
>  
>  	tst_count = 0;
> -- 
> 2.17.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 5/5] syscalls/fanotify10: add tests for FAN_MARK_FILESYSTEM and ignore mask

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/5] syscalls/fanotify01: check events also on mountpoint mark

[Amir Goldstein]

On Wed, Nov 21, 2018 at 2:02 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> >       /* This event should be ignored */
> > @@ -181,13 +200,13 @@ void test01(void)
> >
> >       /* Now remove open and close from ignored mask */
> >       if (fanotify_mark(fd_notify,
> > -                       FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
> > +                       FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
> >                         FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
> >               tst_brk(TBROK | TERRNO,
> > -                     "fanotify_mark (%d, FAN_MARK_REMOVE | "
> > -                     "FAN_MARK_IGNORED_MASK, FAN_OPEN | "
> > -                     "FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
> > -                     fname);
> > +                     "fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
> > +                     "FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
> > +                     "AT_FDCWD, %s) failed", fd_notify,
> > +                     mark->name, fname);
> >       }
> >
> >       SAFE_CLOSE(fd);
> > @@ -283,12 +302,13 @@ pass:
> >
> >       }
> >       /* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> > -     if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
> > -                         FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
> > +     if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
> > +                       FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
> > +                       AT_FDCWD, fname) < 0) {
> >               tst_brk(TBROK | TERRNO,
> > -                     "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
> > +                     "fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
> >                       "FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
> > -                     "failed", fd_notify, fname);
> > +                     "failed", fd_notify, mark->name, fname);
> >       }
> >  }
> >
> > @@ -296,8 +316,6 @@ static void setup(void)
> >  {
> >       sprintf(fname, "tfile_%d", getpid());
> >       SAFE_FILE_PRINTF(fname, "1");
> > -
> > -     fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
>
> I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> available file descriptors with large enough -i parameter and pushed.
>
> I do wonder what is the reason to move the fanotify_init() from setup
> the the test_fanotify(), is there a problem with recycling the same fd
> for different tests?
>

Closing fd_notify is an easier and better cleanup between test cases.
Otherwise we would need to FAN_MARK_FLUSH all the individual marks
that we added.

Thanks,
Amir.

[LTP] [PATCH 4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

[Amir Goldstein]

On Wed, Nov 21, 2018 at 3:16 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > -static void setup_mark(unsigned int n)
> > +static int setup_mark(unsigned int n)
> >  {
> >       struct tcase *tc = &tcases[n];
> >       struct fanotify_mark_type *mark = &tc->mark;
> > @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
> >       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> >                         FAN_ACCESS_PERM | FAN_OPEN_PERM,
> >                         AT_FDCWD, fname) < 0) {
> > -             if (errno == EINVAL) {
> > +             if (errno == EINVAL && support_perm_events &&
> > +                 mark->flag == FAN_MARK_FILESYSTEM) {
> > +                     tst_res(TCONF,
> > +                             "FAN_MARK_FILESYSTEM not supported in kernel?");
> > +                     return -1;
> > +             } else if (errno == EINVAL) {
> >                       tst_brk(TCONF | TERRNO,
> >                               "CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
> >                               "configured in kernel?");
> > @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
> >                               "AT_FDCWD, %s) failed.",
> >                               fd_notify, mark->name, fname);
> >               }
> > +     } else {
> > +             /*
> > +              * To distigouish between perm event not supported and
> > +              * filesystem mark not supported.
> > +              */
> > +             support_perm_events = 1;
>
> I'm a bit puzzled here, so we attempted to cache if perm_events are
> supported here?
>

Yes.

> I guess that we depend on the order of the tcases[] array here, which is
> not very nice.
>

Indeed. It is a simplification. All test cases are permission events,
so the order of failure types is not likely to change, but I could add a
comment about the order near test cases definition.
Would you rather that I did an explicit test for permission support at setup()
time?

> Also it does not have to be in else branch, if we get EINVAL the first
> time we call fanotify_mark() tst_brk() is called, which exits the test,
> so if we ever get to this point in the program, we did at least one
> successful mark.

Right. I'll change that.

Thanks,
Amir.

[LTP] [PATCH 4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

[Amir Goldstein]

On Wed, Nov 21, 2018 at 6:36 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 3:16 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> >
> > Hi!
> > > -static void setup_mark(unsigned int n)
> > > +static int setup_mark(unsigned int n)
> > >  {
> > >       struct tcase *tc = &tcases[n];
> > >       struct fanotify_mark_type *mark = &tc->mark;
> > > @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
> > >       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> > >                         FAN_ACCESS_PERM | FAN_OPEN_PERM,
> > >                         AT_FDCWD, fname) < 0) {
> > > -             if (errno == EINVAL) {
> > > +             if (errno == EINVAL && support_perm_events &&
> > > +                 mark->flag == FAN_MARK_FILESYSTEM) {
> > > +                     tst_res(TCONF,
> > > +                             "FAN_MARK_FILESYSTEM not supported in kernel?");
> > > +                     return -1;
> > > +             } else if (errno == EINVAL) {
> > >                       tst_brk(TCONF | TERRNO,
> > >                               "CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
> > >                               "configured in kernel?");
> > > @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
> > >                               "AT_FDCWD, %s) failed.",
> > >                               fd_notify, mark->name, fname);
> > >               }
> > > +     } else {
> > > +             /*
> > > +              * To distigouish between perm event not supported and
> > > +              * filesystem mark not supported.
> > > +              */
> > > +             support_perm_events = 1;
> >
> > I'm a bit puzzled here, so we attempted to cache if perm_events are
> > supported here?
> >
>
> Yes.
>
> > I guess that we depend on the order of the tcases[] array here, which is
> > not very nice.
> >
>
> Indeed. It is a simplification. All test cases are permission events,
> so the order of failure types is not likely to change, but I could add a
> comment about the order near test cases definition.
> Would you rather that I did an explicit test for permission support at setup()
> time?
>

Nevermind. this discussion is moot because no permission events support
breaks out of the test. I'll drop this logic altogether.

It will have to make a comeback though with Mark's patches to add tests for
FAN_OPEN_EXEC_PERM. (need to distinguish between to support for
FAN_MARK_FILESYSTEM and no support for FAN_OPEN_EXEC_PERM)
so please state your preference for the best way to handle this issue.

Thanks,
Amir.

[LTP] [PATCH 4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

[Cyril Hrubis]

Hi!
> > I guess that we depend on the order of the tcases[] array here, which is
> > not very nice.
> >
> 
> Indeed. It is a simplification. All test cases are permission events,
> so the order of failure types is not likely to change, but I could add a
> comment about the order near test cases definition.
> Would you rather that I did an explicit test for permission support at setup()
> time?

Either one would be fine.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/5] syscalls/fanotify01: check events also on mountpoint mark

[Cyril Hrubis]

Hi!
> > I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> > available file descriptors with large enough -i parameter and pushed.
> >
> > I do wonder what is the reason to move the fanotify_init() from setup
> > the the test_fanotify(), is there a problem with recycling the same fd
> > for different tests?
> >
> 
> Closing fd_notify is an easier and better cleanup between test cases.
> Otherwise we would need to FAN_MARK_FLUSH all the individual marks
> that we added.

Wouldn't that cover more of kernel codepaths? If we reused the fd and
cleared all the marks we would test that the API for clearing the marks
works as expected. Or should we write a separate test for that?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/5] syscalls/fanotify01: check events also on mountpoint mark

[Amir Goldstein]

On Thu, Nov 22, 2018 at 6:07 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> > > available file descriptors with large enough -i parameter and pushed.
> > >
> > > I do wonder what is the reason to move the fanotify_init() from setup
> > > the the test_fanotify(), is there a problem with recycling the same fd
> > > for different tests?
> > >
> >
> > Closing fd_notify is an easier and better cleanup between test cases.
> > Otherwise we would need to FAN_MARK_FLUSH all the individual marks
> > that we added.
>
> Wouldn't that cover more of kernel codepaths? If we reused the fd and
> cleared all the marks we would test that the API for clearing the marks
> works as expected. Or should we write a separate test for that?

I see that fanotify04 exercises FAN_MARK_FLUSH.
Besides it is probably the less common case (??).
Most of the time I imagine that a monitoring program sets a mark
and watches events until it closes the fd.

Thanks,
Amir.