[PATCH 0/7] Test the new fanotify FAN_FS_ERROR event

[PATCH 0/7] Test the new fanotify FAN_FS_ERROR event

[Gabriel Krisman Bertazi]

FAN_FS_ERROR is a new (still unmerged) fanotify event to monitor
fileystem errors.  This patchset introduces a new LTP test for this
feature.

Testing file system errors is slightly tricky, in particular because
they are mostly file system dependent.  Since there are only patches for
ext4, I choose to make the test around it, since there wouldn't be much
to do with other file systems.  The second challenge is how we cause the
file system errors, since there is no error injection for ext4 in Linux.
In this series, this is done by corrupting specific data in the
test device with the help of debugfs.

The FAN_FS_ERROR feature is flying around linux-ext4 and fsdevel, and
the latest version is available on the branch below:

    https://gitlab.collabora.com/krisman/linux -b fanotify-notifications-single-slot

A proper manpage description is also available on the respective mailing
list, or in the branch below:

    https://gitlab.collabora.com/krisman/man-pages.git -b fan-fs-error

Please, let me know your thoughts.

Gabriel Krisman Bertazi (7):
  syscalls/fanotify20: Introduce helpers for FAN_FS_ERROR test
  syscalls/fanotify20: Validate the generic error info
  syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR
  syscalls/fanotify20: Watch event after filesystem abort
  syscalls/fanotify20: Support submission of debugfs commands
  syscalls/fanotify20: Test file event with broken inode
  syscalls/fanotify20: Test capture of multiple errors

 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 .../kernel/syscalls/fanotify/fanotify20.c     | 328 ++++++++++++++++++
 2 files changed, 329 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify20.c

-- 
2.32.0

[PATCH 1/7] syscalls/fanotify20: Introduce helpers for FAN_FS_ERROR test

[Gabriel Krisman Bertazi]

fanotify20 is a new test validating the FAN_FS_ERROR file system error
event.  This adds some basic structure for the next patches.

The strategy for error reporting testing in fanotify20 goes like this:

  - Generate a broken filesystem
  - Start FAN_FS_ERROR monitoring group
  - Make the file system  notice the error through ordinary operations
  - Observe the event generated

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 .../kernel/syscalls/fanotify/fanotify20.c     | 135 ++++++++++++++++++
 2 files changed, 136 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify20.c

diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index 9554b16b196e..c99e6fff76d6 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -17,4 +17,5 @@
 /fanotify17
 /fanotify18
 /fanotify19
+/fanotify20
 /fanotify_child
diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
new file mode 100644
index 000000000000..50531bd99cc9
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2021 Collabora Ltd.
+ *
+ * Author: Gabriel Krisman Bertazi <gabriel@krisman.be>
+ * Based on previous work by Amir Goldstein <amir73il@gmail.com>
+ */
+
+/*\
+ * [Description]
+ * Check fanotify FAN_ERROR_FS events triggered by intentionally
+ * corrupted filesystems:
+ *
+ * - Generate a broken filesystem
+ * - Start FAN_FS_ERROR monitoring group
+ * - Make the file system notice the error through ordinary operations
+ * - Observe the event generated
+ */
+
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/syscall.h>
+#include "tst_test.h"
+#include <sys/fanotify.h>
+#include <sys/types.h>
+#include <fcntl.h>
+
+#ifdef HAVE_SYS_FANOTIFY_H
+#include "fanotify.h"
+
+#ifndef FAN_FS_ERROR
+#define FAN_FS_ERROR		0x00008000
+#endif
+
+#define BUF_SIZE 256
+static char event_buf[BUF_SIZE];
+int fd_notify;
+
+#define MOUNT_PATH "test_mnt"
+
+static const struct test_case {
+	char *name;
+	void (*trigger_error)(void);
+	void (*prepare_fs)(void);
+} testcases[] = {
+};
+
+int check_error_event_metadata(struct fanotify_event_metadata *event)
+{
+	int fail = 0;
+
+	if (event->mask != FAN_FS_ERROR) {
+		fail++;
+		tst_res(TFAIL, "got unexpected event %llx",
+			(unsigned long long)event->mask);
+	}
+
+	if (event->fd != FAN_NOFD) {
+		fail++;
+		tst_res(TFAIL, "Weird FAN_FD %llx",
+			(unsigned long long)event->mask);
+	}
+	return fail;
+}
+
+void check_event(char *buf, size_t len, const struct test_case *ex)
+{
+	struct fanotify_event_metadata *event =
+		(struct fanotify_event_metadata *) buf;
+
+	if (len < FAN_EVENT_METADATA_LEN)
+		tst_res(TFAIL, "No event metadata found");
+
+	if (check_error_event_metadata(event))
+		return;
+
+	tst_res(TPASS, "Successfully received: %s", ex->name);
+}
+
+static void do_test(unsigned int i)
+{
+	const struct test_case *tcase = &testcases[i];
+	size_t read_len;
+
+	tcase->trigger_error();
+
+	read_len = SAFE_READ(0, fd_notify, event_buf, BUF_SIZE);
+
+	check_event(event_buf, read_len, tcase);
+}
+
+static void setup(void)
+{
+	unsigned long i;
+
+	for (i = 0; i < ARRAY_SIZE(testcases); i++)
+		if (testcases[i].prepare_fs)
+			testcases[i].prepare_fs();
+
+	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF|FAN_REPORT_FID,
+				       O_RDONLY);
+
+	SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD|FAN_MARK_FILESYSTEM,
+			   FAN_FS_ERROR, AT_FDCWD, MOUNT_PATH);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+}
+
+static struct tst_test test = {
+	.test = do_test,
+	.tcnt = ARRAY_SIZE(testcases),
+	.setup = setup,
+	.cleanup = cleanup,
+	.mount_device = 1,
+	.mntpoint = MOUNT_PATH,
+	.all_filesystems = 0,
+	.needs_root = 1,
+	.dev_fs_type = "ext4"
+
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
-- 
2.32.0

[PATCH 2/7] syscalls/fanotify20: Validate the generic error info

[Gabriel Krisman Bertazi]

Implement some validation for the generic error info record emitted by
the kernel.  The error number is fs-specific but, well, we only support
ext4 for now anyway.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 .../kernel/syscalls/fanotify/fanotify20.c     | 59 ++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index 50531bd99cc9..fd5cfb8744f1 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -37,6 +37,14 @@
 
 #ifndef FAN_FS_ERROR
 #define FAN_FS_ERROR		0x00008000
+
+#define FAN_EVENT_INFO_TYPE_ERROR	4
+
+struct fanotify_event_info_error {
+	struct fanotify_event_info_header hdr;
+	__s32 error;
+	__u32 error_count;
+};
 #endif
 
 #define BUF_SIZE 256
@@ -47,11 +55,54 @@ int fd_notify;
 
 static const struct test_case {
 	char *name;
+	int error;
+	unsigned int error_count;
 	void (*trigger_error)(void);
 	void (*prepare_fs)(void);
 } testcases[] = {
 };
 
+struct fanotify_event_info_header *get_event_info(
+					struct fanotify_event_metadata *event,
+					int info_type)
+{
+	struct fanotify_event_info_header *hdr = NULL;
+	char *start = (char *) event;
+	int off;
+
+	for (off = event->metadata_len; (off+sizeof(*hdr)) < event->event_len;
+	     off += hdr->len) {
+		hdr = (struct fanotify_event_info_header *) &(start[off]);
+		if (hdr->info_type == info_type)
+			return hdr;
+	}
+	return NULL;
+}
+
+#define get_event_info_error(event)					\
+	((struct fanotify_event_info_error *)				\
+	 get_event_info((event), FAN_EVENT_INFO_TYPE_ERROR))
+
+int check_error_event_info_error(struct fanotify_event_info_error *info_error,
+				 const struct test_case *ex)
+{
+	int fail = 0;
+
+	if (info_error->error_count != ex->error_count) {
+		tst_res(TFAIL, "%s: Unexpected error_count (%d!=%d)",
+			ex->name, info_error->error_count, ex->error_count);
+		fail++;
+	}
+
+	if (info_error->error != ex->error) {
+		tst_res(TFAIL, "%s: Unexpected error code value (%d!=%d)",
+			ex->name, info_error->error, ex->error);
+		fail++;
+	}
+
+	return fail;
+}
+
 int check_error_event_metadata(struct fanotify_event_metadata *event)
 {
 	int fail = 0;
@@ -74,6 +125,8 @@ void check_event(char *buf, size_t len, const struct test_case *ex)
 {
 	struct fanotify_event_metadata *event =
 		(struct fanotify_event_metadata *) buf;
+	struct fanotify_event_info_error *info_error;
+	int fail = 0;
 
 	if (len < FAN_EVENT_METADATA_LEN)
 		tst_res(TFAIL, "No event metadata found");
@@ -81,7 +134,11 @@ void check_event(char *buf, size_t len, const struct test_case *ex)
 	if (check_error_event_metadata(event))
 		return;
 
-	tst_res(TPASS, "Successfully received: %s", ex->name);
+	info_error = get_event_info_error(event);
+	fail += info_error ? check_error_event_info_error(info_error, ex) : 1;
+
+	if (!fail)
+		tst_res(TPASS, "Successfully received: %s", ex->name);
 }
 
 static void do_test(unsigned int i)
-- 
2.32.0

[PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Gabriel Krisman Bertazi]

Verify the FID provided in the event.  If the testcase has a null inode,
this is assumed to be a superblock error (i.e. null FH).

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index fd5cfb8744f1..d8d788ae685f 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -40,6 +40,14 @@
 
 #define FAN_EVENT_INFO_TYPE_ERROR	4
 
+#ifndef FILEID_INVALID
+#define	FILEID_INVALID		0xff
+#endif
+
+#ifndef FILEID_INO32_GEN
+#define FILEID_INO32_GEN	1
+#endif
+
 struct fanotify_event_info_error {
 	struct fanotify_event_info_header hdr;
 	__s32 error;
@@ -57,6 +65,9 @@ static const struct test_case {
 	char *name;
 	int error;
 	unsigned int error_count;
+
+	/* inode can be null for superblock errors */
+	unsigned int *inode;
 	void (*trigger_error)(void);
 	void (*prepare_fs)(void);
 } testcases[] = {
@@ -83,6 +94,42 @@ struct fanotify_event_info_header *get_event_info(
 	((struct fanotify_event_info_error *)				\
 	 get_event_info((event), FAN_EVENT_INFO_TYPE_ERROR))
 
+#define get_event_info_fid(event)					\
+	((struct fanotify_event_info_fid *)				\
+	 get_event_info((event), FAN_EVENT_INFO_TYPE_FID))
+
+int check_error_event_info_fid(struct fanotify_event_info_fid *fid,
+				 const struct test_case *ex)
+{
+	int fail = 0;
+	struct file_handle *fh = (struct file_handle *) &fid->handle;
+
+	if (!ex->inode) {
+		uint32_t *h = (uint32_t *) fh->f_handle;
+
+		if (!(fh->handle_type == FILEID_INVALID && !h[0] && !h[1])) {
+			tst_res(TFAIL, "%s: file handle should have been invalid",
+				ex->name);
+			fail++;
+		}
+		return fail;
+	} else if (fh->handle_type == FILEID_INO32_GEN) {
+		uint32_t *h = (uint32_t *) fh->f_handle;
+
+		if (h[0] != *ex->inode) {
+			tst_res(TFAIL,
+				"%s: Unexpected file handle inode (%u!=%u)",
+				ex->name, *ex->inode, h[0]);
+			fail++;
+		}
+	} else {
+		tst_res(TFAIL, "%s: Test can't handle received FH type (%d)",
+			ex->name, fh->handle_type);
+	}
+
+	return fail;
+}
+
 int check_error_event_info_error(struct fanotify_event_info_error *info_error,
 				 const struct test_case *ex)
 {
@@ -126,6 +173,7 @@ void check_event(char *buf, size_t len, const struct test_case *ex)
 	struct fanotify_event_metadata *event =
 		(struct fanotify_event_metadata *) buf;
 	struct fanotify_event_info_error *info_error;
+	struct fanotify_event_info_fid *info_fid;
 	int fail = 0;
 
 	if (len < FAN_EVENT_METADATA_LEN)
@@ -137,6 +185,9 @@ void check_event(char *buf, size_t len, const struct test_case *ex)
 	info_error = get_event_info_error(event);
 	fail += info_error ? check_error_event_info_error(info_error, ex) : 1;
 
+	info_fid = get_event_info_fid(event);
+	fail += info_fid ? check_error_event_info_fid(info_fid, ex) : 1;
+
 	if (!fail)
 		tst_res(TPASS, "Successfully received: %s", ex->name);
 }
-- 
2.32.0

[PATCH 4/7] syscalls/fanotify20: Watch event after filesystem abort

[Gabriel Krisman Bertazi]

This test monitors the EXT4 specific error triggered after a file system
abort.  It works by forcing a remount with the option "abort".  This is
an error not related to a file so it is reported against the superblock
with a NULL FH.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 testcases/kernel/syscalls/fanotify/fanotify20.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index d8d788ae685f..7a9601072139 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -61,6 +61,14 @@ int fd_notify;
 
 #define MOUNT_PATH "test_mnt"
 
+#define EXT4_ERR_ESHUTDOWN 16
+
+static void trigger_fs_abort(void)
+{
+	SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type,
+		   MS_REMOUNT|MS_RDONLY, "abort");
+}
+
 static const struct test_case {
 	char *name;
 	int error;
@@ -71,6 +79,13 @@ static const struct test_case {
 	void (*trigger_error)(void);
 	void (*prepare_fs)(void);
 } testcases[] = {
+	{
+		.name = "Trigger abort",
+		.trigger_error = &trigger_fs_abort,
+		.error_count = 1,
+		.error = EXT4_ERR_ESHUTDOWN,
+		.inode = NULL
+	}
 };
 
 struct fanotify_event_info_header *get_event_info(
-- 
2.32.0

[PATCH 5/7] syscalls/fanotify20: Support submission of debugfs commands

[Gabriel Krisman Bertazi]

In order to test FAN_FS_ERROR, we want to corrupt the filesystem.  The
easiest way to do it is by using debugfs.  Add a small helper to issue
debugfs requests.  Since most likely this will be the only testcase to
need this, don't bother making it a proper helper for now.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 testcases/kernel/syscalls/fanotify/fanotify20.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index 7a9601072139..e7ced28eb61d 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -63,6 +63,13 @@ int fd_notify;
 
 #define EXT4_ERR_ESHUTDOWN 16
 
+static void do_debugfs_request(const char *dev, char *request)
+{
+	char *cmd[] = {"debugfs", "-w", dev, "-R", request, NULL};
+
+	SAFE_CMD(cmd, NULL, NULL);
+}
+
 static void trigger_fs_abort(void)
 {
 	SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type,
-- 
2.32.0

[PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Gabriel Krisman Bertazi]

This test corrupts an inode entry with an invalid mode through debugfs
and then tries to access it.  This should result in a ext4 error, which
we monitor through the fanotify group.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 .../kernel/syscalls/fanotify/fanotify20.c     | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index e7ced28eb61d..0c63e90edc3a 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -76,6 +76,36 @@ static void trigger_fs_abort(void)
 		   MS_REMOUNT|MS_RDONLY, "abort");
 }
 
+#define TCASE2_BASEDIR "tcase2"
+#define TCASE2_BAD_DIR TCASE2_BASEDIR"/bad_dir"
+
+static unsigned int tcase2_bad_ino;
+static void tcase2_prepare_fs(void)
+{
+	struct stat stat;
+
+	SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BASEDIR, 0777);
+	SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BAD_DIR, 0777);
+
+	SAFE_STAT(MOUNT_PATH"/"TCASE2_BAD_DIR, &stat);
+	tcase2_bad_ino = stat.st_ino;
+
+	SAFE_UMOUNT(MOUNT_PATH);
+	do_debugfs_request(tst_device->dev, "sif " TCASE2_BAD_DIR " mode 0xff");
+	SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL);
+}
+
+static void tcase2_trigger_lookup(void)
+{
+	int ret;
+
+	/* SAFE_OPEN cannot be used here because we expect it to fail. */
+	ret = open(MOUNT_PATH"/"TCASE2_BAD_DIR, O_RDONLY, 0);
+	if (ret != -1 && errno != EUCLEAN)
+		tst_res(TFAIL, "Unexpected lookup result(%d) of %s (%d!=%d)",
+			ret, TCASE2_BAD_DIR, errno, EUCLEAN);
+}
+
 static const struct test_case {
 	char *name;
 	int error;
@@ -92,6 +122,14 @@ static const struct test_case {
 		.error_count = 1,
 		.error = EXT4_ERR_ESHUTDOWN,
 		.inode = NULL
+	},
+	{
+		.name = "Lookup of inode with invalid mode",
+		.prepare_fs = tcase2_prepare_fs,
+		.trigger_error = &tcase2_trigger_lookup,
+		.error_count = 1,
+		.error = 0,
+		.inode = &tcase2_bad_ino,
 	}
 };
 
-- 
2.32.0

[PATCH 7/7] syscalls/fanotify20: Test capture of multiple errors

[Gabriel Krisman Bertazi]

When multiple FS errors occur, only the first is stored.  This testcase
validates this behavior by issuing two different errors and making sure
only the first is stored, while the second is simply accumulated in
error_count.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 .../kernel/syscalls/fanotify/fanotify20.c     | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index 0c63e90edc3a..07040cb7fa7c 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -106,6 +106,18 @@ static void tcase2_trigger_lookup(void)
 			ret, TCASE2_BAD_DIR, errno, EUCLEAN);
 }
 
+static void tcase3_trigger(void)
+{
+	trigger_fs_abort();
+	tcase2_trigger_lookup();
+}
+
+static void tcase4_trigger(void)
+{
+	tcase2_trigger_lookup();
+	trigger_fs_abort();
+}
+
 static const struct test_case {
 	char *name;
 	int error;
@@ -130,6 +142,19 @@ static const struct test_case {
 		.error_count = 1,
 		.error = 0,
 		.inode = &tcase2_bad_ino,
+	},
+	{
+		.name = "Multiple error submission",
+		.trigger_error = &tcase3_trigger,
+		.error_count = 2,
+		.error = EXT4_ERR_ESHUTDOWN,
+	},
+	{
+		.name = "Multiple error submission 2",
+		.trigger_error = &tcase4_trigger,
+		.error_count = 2,
+		.error = 0,
+		.inode = &tcase2_bad_ino,
 	}
 };
 
-- 
2.32.0

Re: [PATCH 1/7] syscalls/fanotify20: Introduce helpers for FAN_FS_ERROR test

[Amir Goldstein]

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> fanotify20 is a new test validating the FAN_FS_ERROR file system error
> event.  This adds some basic structure for the next patches.
>
> The strategy for error reporting testing in fanotify20 goes like this:
>
>   - Generate a broken filesystem
>   - Start FAN_FS_ERROR monitoring group
>   - Make the file system  notice the error through ordinary operations
>   - Observe the event generated
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  testcases/kernel/syscalls/fanotify/.gitignore |   1 +
>  .../kernel/syscalls/fanotify/fanotify20.c     | 135 ++++++++++++++++++
>  2 files changed, 136 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify20.c
>
> diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
> index 9554b16b196e..c99e6fff76d6 100644
> --- a/testcases/kernel/syscalls/fanotify/.gitignore
> +++ b/testcases/kernel/syscalls/fanotify/.gitignore
> @@ -17,4 +17,5 @@
>  /fanotify17
>  /fanotify18
>  /fanotify19
> +/fanotify20
>  /fanotify_child
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> new file mode 100644
> index 000000000000..50531bd99cc9
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Collabora Ltd.
> + *
> + * Author: Gabriel Krisman Bertazi <gabriel@krisman.be>
> + * Based on previous work by Amir Goldstein <amir73il@gmail.com>
> + */
> +
> +/*\
> + * [Description]
> + * Check fanotify FAN_ERROR_FS events triggered by intentionally
> + * corrupted filesystems:
> + *
> + * - Generate a broken filesystem
> + * - Start FAN_FS_ERROR monitoring group
> + * - Make the file system notice the error through ordinary operations
> + * - Observe the event generated
> + */
> +
> +#define _GNU_SOURCE
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <sys/mount.h>
> +#include <sys/syscall.h>
> +#include "tst_test.h"
> +#include <sys/fanotify.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
> +
> +#ifdef HAVE_SYS_FANOTIFY_H
> +#include "fanotify.h"
> +
> +#ifndef FAN_FS_ERROR
> +#define FAN_FS_ERROR           0x00008000
> +#endif
> +
> +#define BUF_SIZE 256
> +static char event_buf[BUF_SIZE];
> +int fd_notify;
> +
> +#define MOUNT_PATH "test_mnt"
> +
> +static const struct test_case {
> +       char *name;
> +       void (*trigger_error)(void);
> +       void (*prepare_fs)(void);
> +} testcases[] = {
> +};
> +
> +int check_error_event_metadata(struct fanotify_event_metadata *event)
> +{
> +       int fail = 0;
> +
> +       if (event->mask != FAN_FS_ERROR) {
> +               fail++;
> +               tst_res(TFAIL, "got unexpected event %llx",
> +                       (unsigned long long)event->mask);
> +       }
> +
> +       if (event->fd != FAN_NOFD) {
> +               fail++;
> +               tst_res(TFAIL, "Weird FAN_FD %llx",
> +                       (unsigned long long)event->mask);
> +       }
> +       return fail;
> +}
> +
> +void check_event(char *buf, size_t len, const struct test_case *ex)
> +{
> +       struct fanotify_event_metadata *event =
> +               (struct fanotify_event_metadata *) buf;
> +
> +       if (len < FAN_EVENT_METADATA_LEN)
> +               tst_res(TFAIL, "No event metadata found");
> +
> +       if (check_error_event_metadata(event))
> +               return;
> +
> +       tst_res(TPASS, "Successfully received: %s", ex->name);
> +}
> +
> +static void do_test(unsigned int i)
> +{
> +       const struct test_case *tcase = &testcases[i];
> +       size_t read_len;
> +
> +       tcase->trigger_error();
> +
> +       read_len = SAFE_READ(0, fd_notify, event_buf, BUF_SIZE);
> +
> +       check_event(event_buf, read_len, tcase);
> +}
> +
> +static void setup(void)
> +{
> +       unsigned long i;
> +
> +       for (i = 0; i < ARRAY_SIZE(testcases); i++)
> +               if (testcases[i].prepare_fs)
> +                       testcases[i].prepare_fs();
> +

Why is prepare_fs called up front and not on every test case?

> +       fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF|FAN_REPORT_FID,
> +                                      O_RDONLY);
> +
> +       SAFE_FANOTIFY_MARK(fd_notify, FAN_MARK_ADD|FAN_MARK_FILESYSTEM,
> +                          FAN_FS_ERROR, AT_FDCWD, MOUNT_PATH);

This will cause test to fail on old kernels.
You need to start this test with
fanotify_events_supported_by_kernel(FAN_FS_ERROR)
but you cannot use it as is.

Create a macro like
REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS
which calls fanotify_init_flags_err_msg(...fanotify_events_supported_by_kernel())
and pass init flags as argument to fanotify_events_supported_by_kernel()
instead of using hardcoded flags FAN_CLASS_CONTENT.


> +}
> +
> +static void cleanup(void)
> +{
> +       if (fd_notify > 0)
> +               SAFE_CLOSE(fd_notify);
> +}
> +
> +static struct tst_test test = {
> +       .test = do_test,
> +       .tcnt = ARRAY_SIZE(testcases),
> +       .setup = setup,
> +       .cleanup = cleanup,
> +       .mount_device = 1,
> +       .mntpoint = MOUNT_PATH,
> +       .all_filesystems = 0,

This is 0 by default

Thanks,
Amir.

Re: [PATCH 2/7] syscalls/fanotify20: Validate the generic error info

[Amir Goldstein]

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Implement some validation for the generic error info record emitted by
> the kernel.  The error number is fs-specific but, well, we only support
> ext4 for now anyway.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 59 ++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index 50531bd99cc9..fd5cfb8744f1 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -37,6 +37,14 @@
>
>  #ifndef FAN_FS_ERROR
>  #define FAN_FS_ERROR           0x00008000
> +
> +#define FAN_EVENT_INFO_TYPE_ERROR      4
> +
> +struct fanotify_event_info_error {
> +       struct fanotify_event_info_header hdr;
> +       __s32 error;
> +       __u32 error_count;
> +};
>  #endif

Those defines go in fanotify.h

>
>  #define BUF_SIZE 256
> @@ -47,11 +55,54 @@ int fd_notify;
>
>  static const struct test_case {
>         char *name;
> +       int error;
> +       unsigned int error_count;
>         void (*trigger_error)(void);
>         void (*prepare_fs)(void);
>  } testcases[] = {
>  };
>
> +struct fanotify_event_info_header *get_event_info(
> +                                       struct fanotify_event_metadata *event,
> +                                       int info_type)
> +{
> +       struct fanotify_event_info_header *hdr = NULL;
> +       char *start = (char *) event;
> +       int off;
> +
> +       for (off = event->metadata_len; (off+sizeof(*hdr)) < event->event_len;
> +            off += hdr->len) {
> +               hdr = (struct fanotify_event_info_header *) &(start[off]);
> +               if (hdr->info_type == info_type)
> +                       return hdr;
> +       }
> +       return NULL;
> +}
> +
> +#define get_event_info_error(event)                                    \
> +       ((struct fanotify_event_info_error *)                           \
> +        get_event_info((event), FAN_EVENT_INFO_TYPE_ERROR))

This helper and macro would be very useful in fanotify.h for other tests to use.

Thanks,
Amir.

Re: [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Amir Goldstein]

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Verify the FID provided in the event.  If the testcase has a null inode,
> this is assumed to be a superblock error (i.e. null FH).
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index fd5cfb8744f1..d8d788ae685f 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -40,6 +40,14 @@
>
>  #define FAN_EVENT_INFO_TYPE_ERROR      4
>
> +#ifndef FILEID_INVALID
> +#define        FILEID_INVALID          0xff
> +#endif
> +
> +#ifndef FILEID_INO32_GEN
> +#define FILEID_INO32_GEN       1
> +#endif
> +
>  struct fanotify_event_info_error {
>         struct fanotify_event_info_header hdr;
>         __s32 error;
> @@ -57,6 +65,9 @@ static const struct test_case {
>         char *name;
>         int error;
>         unsigned int error_count;
> +
> +       /* inode can be null for superblock errors */
> +       unsigned int *inode;

Any reason not to use fanotify_fid_t * like fanotify16.c?

Thanks,
Amir.

Re: [PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Amir Goldstein]

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> This test corrupts an inode entry with an invalid mode through debugfs
> and then tries to access it.  This should result in a ext4 error, which
> we monitor through the fanotify group.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index e7ced28eb61d..0c63e90edc3a 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -76,6 +76,36 @@ static void trigger_fs_abort(void)
>                    MS_REMOUNT|MS_RDONLY, "abort");
>  }
>
> +#define TCASE2_BASEDIR "tcase2"
> +#define TCASE2_BAD_DIR TCASE2_BASEDIR"/bad_dir"
> +
> +static unsigned int tcase2_bad_ino;
> +static void tcase2_prepare_fs(void)
> +{
> +       struct stat stat;
> +
> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BASEDIR, 0777);
> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BAD_DIR, 0777);
> +
> +       SAFE_STAT(MOUNT_PATH"/"TCASE2_BAD_DIR, &stat);
> +       tcase2_bad_ino = stat.st_ino;

Better use fanotify_save_fid(MOUNT_PATH"/"TCASE2_BAD_DIR, &tcase2_bad_fid)

Thanks,
Amir.

Re: [PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Amir Goldstein]

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> This test corrupts an inode entry with an invalid mode through debugfs
> and then tries to access it.  This should result in a ext4 error, which
> we monitor through the fanotify group.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index e7ced28eb61d..0c63e90edc3a 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -76,6 +76,36 @@ static void trigger_fs_abort(void)
>                    MS_REMOUNT|MS_RDONLY, "abort");
>  }
>
> +#define TCASE2_BASEDIR "tcase2"
> +#define TCASE2_BAD_DIR TCASE2_BASEDIR"/bad_dir"
> +
> +static unsigned int tcase2_bad_ino;
> +static void tcase2_prepare_fs(void)
> +{
> +       struct stat stat;
> +
> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BASEDIR, 0777);
> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BAD_DIR, 0777);
> +
> +       SAFE_STAT(MOUNT_PATH"/"TCASE2_BAD_DIR, &stat);
> +       tcase2_bad_ino = stat.st_ino;
> +
> +       SAFE_UMOUNT(MOUNT_PATH);
> +       do_debugfs_request(tst_device->dev, "sif " TCASE2_BAD_DIR " mode 0xff");
> +       SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL);
> +}
> +
> +static void tcase2_trigger_lookup(void)
> +{
> +       int ret;
> +
> +       /* SAFE_OPEN cannot be used here because we expect it to fail. */
> +       ret = open(MOUNT_PATH"/"TCASE2_BAD_DIR, O_RDONLY, 0);
> +       if (ret != -1 && errno != EUCLEAN)
> +               tst_res(TFAIL, "Unexpected lookup result(%d) of %s (%d!=%d)",
> +                       ret, TCASE2_BAD_DIR, errno, EUCLEAN);
> +}
> +
>  static const struct test_case {
>         char *name;
>         int error;
> @@ -92,6 +122,14 @@ static const struct test_case {
>                 .error_count = 1,
>                 .error = EXT4_ERR_ESHUTDOWN,
>                 .inode = NULL
> +       },
> +       {
> +               .name = "Lookup of inode with invalid mode",
> +               .prepare_fs = tcase2_prepare_fs,
> +               .trigger_error = &tcase2_trigger_lookup,
> +               .error_count = 1,
> +               .error = 0,
> +               .inode = &tcase2_bad_ino,

Why is error 0?
What's the rationale?

Thanks,
Amir.

Re: [PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>> This test corrupts an inode entry with an invalid mode through debugfs
>> and then tries to access it.  This should result in a ext4 error, which
>> we monitor through the fanotify group.
>>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> ---
>>  .../kernel/syscalls/fanotify/fanotify20.c     | 38 +++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
>> index e7ced28eb61d..0c63e90edc3a 100644
>> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
>> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
>> @@ -76,6 +76,36 @@ static void trigger_fs_abort(void)
>>                    MS_REMOUNT|MS_RDONLY, "abort");
>>  }
>>
>> +#define TCASE2_BASEDIR "tcase2"
>> +#define TCASE2_BAD_DIR TCASE2_BASEDIR"/bad_dir"
>> +
>> +static unsigned int tcase2_bad_ino;
>> +static void tcase2_prepare_fs(void)
>> +{
>> +       struct stat stat;
>> +
>> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BASEDIR, 0777);
>> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BAD_DIR, 0777);
>> +
>> +       SAFE_STAT(MOUNT_PATH"/"TCASE2_BAD_DIR, &stat);
>> +       tcase2_bad_ino = stat.st_ino;
>> +
>> +       SAFE_UMOUNT(MOUNT_PATH);
>> +       do_debugfs_request(tst_device->dev, "sif " TCASE2_BAD_DIR " mode 0xff");
>> +       SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL);
>> +}
>> +
>> +static void tcase2_trigger_lookup(void)
>> +{
>> +       int ret;
>> +
>> +       /* SAFE_OPEN cannot be used here because we expect it to fail. */
>> +       ret = open(MOUNT_PATH"/"TCASE2_BAD_DIR, O_RDONLY, 0);
>> +       if (ret != -1 && errno != EUCLEAN)
>> +               tst_res(TFAIL, "Unexpected lookup result(%d) of %s (%d!=%d)",
>> +                       ret, TCASE2_BAD_DIR, errno, EUCLEAN);
>> +}
>> +
>>  static const struct test_case {
>>         char *name;
>>         int error;
>> @@ -92,6 +122,14 @@ static const struct test_case {
>>                 .error_count = 1,
>>                 .error = EXT4_ERR_ESHUTDOWN,
>>                 .inode = NULL
>> +       },
>> +       {
>> +               .name = "Lookup of inode with invalid mode",
>> +               .prepare_fs = tcase2_prepare_fs,
>> +               .trigger_error = &tcase2_trigger_lookup,
>> +               .error_count = 1,
>> +               .error = 0,
>> +               .inode = &tcase2_bad_ino,
>
> Why is error 0?
> What's the rationale?

Hi Amir,

That is specific to Ext4.  Some ext4 conditions report bogus error codes.  I will
come up with a kernel patch changing it.

-- 
Gabriel Krisman Bertazi

Re: [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>> Verify the FID provided in the event.  If the testcase has a null inode,
>> this is assumed to be a superblock error (i.e. null FH).
>>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> ---
>>  .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
>> index fd5cfb8744f1..d8d788ae685f 100644
>> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
>> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
>> @@ -40,6 +40,14 @@
>>
>>  #define FAN_EVENT_INFO_TYPE_ERROR      4
>>
>> +#ifndef FILEID_INVALID
>> +#define        FILEID_INVALID          0xff
>> +#endif
>> +
>> +#ifndef FILEID_INO32_GEN
>> +#define FILEID_INO32_GEN       1
>> +#endif
>> +
>>  struct fanotify_event_info_error {
>>         struct fanotify_event_info_header hdr;
>>         __s32 error;
>> @@ -57,6 +65,9 @@ static const struct test_case {
>>         char *name;
>>         int error;
>>         unsigned int error_count;
>> +
>> +       /* inode can be null for superblock errors */
>> +       unsigned int *inode;
>
> Any reason not to use fanotify_fid_t * like fanotify16.c?

No reason other than I didn't notice they existed. Sorry. I will get
this fixed.

-- 
Gabriel Krisman Bertazi

Re: [PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Amir Goldstein]

On Wed, Aug 4, 2021 at 7:52 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > <krisman@collabora.com> wrote:
> >>
> >> This test corrupts an inode entry with an invalid mode through debugfs
> >> and then tries to access it.  This should result in a ext4 error, which
> >> we monitor through the fanotify group.
> >>
> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> >> ---
> >>  .../kernel/syscalls/fanotify/fanotify20.c     | 38 +++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>
> >> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> index e7ced28eb61d..0c63e90edc3a 100644
> >> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> @@ -76,6 +76,36 @@ static void trigger_fs_abort(void)
> >>                    MS_REMOUNT|MS_RDONLY, "abort");
> >>  }
> >>
> >> +#define TCASE2_BASEDIR "tcase2"
> >> +#define TCASE2_BAD_DIR TCASE2_BASEDIR"/bad_dir"
> >> +
> >> +static unsigned int tcase2_bad_ino;
> >> +static void tcase2_prepare_fs(void)
> >> +{
> >> +       struct stat stat;
> >> +
> >> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BASEDIR, 0777);
> >> +       SAFE_MKDIR(MOUNT_PATH"/"TCASE2_BAD_DIR, 0777);
> >> +
> >> +       SAFE_STAT(MOUNT_PATH"/"TCASE2_BAD_DIR, &stat);
> >> +       tcase2_bad_ino = stat.st_ino;
> >> +
> >> +       SAFE_UMOUNT(MOUNT_PATH);
> >> +       do_debugfs_request(tst_device->dev, "sif " TCASE2_BAD_DIR " mode 0xff");
> >> +       SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type, 0, NULL);
> >> +}
> >> +
> >> +static void tcase2_trigger_lookup(void)
> >> +{
> >> +       int ret;
> >> +
> >> +       /* SAFE_OPEN cannot be used here because we expect it to fail. */
> >> +       ret = open(MOUNT_PATH"/"TCASE2_BAD_DIR, O_RDONLY, 0);
> >> +       if (ret != -1 && errno != EUCLEAN)
> >> +               tst_res(TFAIL, "Unexpected lookup result(%d) of %s (%d!=%d)",
> >> +                       ret, TCASE2_BAD_DIR, errno, EUCLEAN);
> >> +}
> >> +
> >>  static const struct test_case {
> >>         char *name;
> >>         int error;
> >> @@ -92,6 +122,14 @@ static const struct test_case {
> >>                 .error_count = 1,
> >>                 .error = EXT4_ERR_ESHUTDOWN,
> >>                 .inode = NULL
> >> +       },
> >> +       {
> >> +               .name = "Lookup of inode with invalid mode",
> >> +               .prepare_fs = tcase2_prepare_fs,
> >> +               .trigger_error = &tcase2_trigger_lookup,
> >> +               .error_count = 1,
> >> +               .error = 0,
> >> +               .inode = &tcase2_bad_ino,
> >
> > Why is error 0?
> > What's the rationale?
>
> Hi Amir,
>
> That is specific to Ext4.  Some ext4 conditions report bogus error codes.  I will
> come up with a kernel patch changing it.
>

Well, I would not expect a FAN_FS_ERROR event to ever have 0 error
value. Since this test practically only tests ext4, I do not think it
is reasonable
for the test to VERIFY a bug. It is fine to write this test with expectations
that are not met and let it fail.

But a better plan would probably be to merge the patches up to 5 to test
FAN_FS_ERROR and then add more test cases after ext4 is fixed
Either that or you fix the ext4 problem along with FAN_FS_ERROR.

Forgot to say that the test needs to declare .needs_cmds "debugfs".

In any case, as far as prerequisite to merging FAN_FS_ERROR
your WIP tests certainly suffice.
Please keep your test branch around so we can use it to validate
the kernel patches.
I usually hold off on submitting LTP tests for inclusion until at least -rc3
after kernel patches have been merged.

Thanks,
Amir.

Re: [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Amir Goldstein]

On Wed, Aug 4, 2021 at 7:54 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > <krisman@collabora.com> wrote:
> >>
> >> Verify the FID provided in the event.  If the testcase has a null inode,
> >> this is assumed to be a superblock error (i.e. null FH).
> >>
> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> >> ---
> >>  .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> index fd5cfb8744f1..d8d788ae685f 100644
> >> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> @@ -40,6 +40,14 @@
> >>
> >>  #define FAN_EVENT_INFO_TYPE_ERROR      4
> >>
> >> +#ifndef FILEID_INVALID
> >> +#define        FILEID_INVALID          0xff
> >> +#endif
> >> +
> >> +#ifndef FILEID_INO32_GEN
> >> +#define FILEID_INO32_GEN       1
> >> +#endif
> >> +
> >>  struct fanotify_event_info_error {
> >>         struct fanotify_event_info_header hdr;
> >>         __s32 error;
> >> @@ -57,6 +65,9 @@ static const struct test_case {
> >>         char *name;
> >>         int error;
> >>         unsigned int error_count;
> >> +
> >> +       /* inode can be null for superblock errors */
> >> +       unsigned int *inode;
> >
> > Any reason not to use fanotify_fid_t * like fanotify16.c?
>
> No reason other than I didn't notice they existed. Sorry. I will get
> this fixed.

No problem. That's what review is for ;-)

BTW, unless anyone is specifically interested I don't think there
is a reason to re post the test patches before the submission request.
Certainly not for the small fixes that I requested.

I do request that you post a link to a branch with the fixed test
so that we can experiment with the kernel patches.

I've also CC'ed Matthew who may want to help with review of the test
and man page that you posted in the cover letter [1].

Thanks,
Amir.

[1] https://lore.kernel.org/linux-ext4/20210802214645.2633028-1-krisman@collabora.com/T/#m9cf637c6aca94e28390f61deac5a53afbc9e88ae

Re: [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Matthew Bobrowski]

On Wed, Aug 04, 2021 at 08:39:55AM +0300, Amir Goldstein wrote:
> On Wed, Aug 4, 2021 at 7:54 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> >
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> > > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > > <krisman@collabora.com> wrote:
> > >>
> > >> Verify the FID provided in the event.  If the testcase has a null inode,
> > >> this is assumed to be a superblock error (i.e. null FH).
> > >>
> > >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> > >> ---
> > >>  .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
> > >>  1 file changed, 51 insertions(+)
> > >>
> > >> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > >> index fd5cfb8744f1..d8d788ae685f 100644
> > >> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> > >> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > >> @@ -40,6 +40,14 @@
> > >>
> > >>  #define FAN_EVENT_INFO_TYPE_ERROR      4
> > >>
> > >> +#ifndef FILEID_INVALID
> > >> +#define        FILEID_INVALID          0xff
> > >> +#endif
> > >> +
> > >> +#ifndef FILEID_INO32_GEN
> > >> +#define FILEID_INO32_GEN       1
> > >> +#endif
> > >> +
> > >>  struct fanotify_event_info_error {
> > >>         struct fanotify_event_info_header hdr;
> > >>         __s32 error;
> > >> @@ -57,6 +65,9 @@ static const struct test_case {
> > >>         char *name;
> > >>         int error;
> > >>         unsigned int error_count;
> > >> +
> > >> +       /* inode can be null for superblock errors */
> > >> +       unsigned int *inode;
> > >
> > > Any reason not to use fanotify_fid_t * like fanotify16.c?
> >
> > No reason other than I didn't notice they existed. Sorry. I will get
> > this fixed.
> 
> No problem. That's what review is for ;-)
> 
> BTW, unless anyone is specifically interested I don't think there
> is a reason to re post the test patches before the submission request.
> Certainly not for the small fixes that I requested.
> 
> I do request that you post a link to a branch with the fixed test
> so that we can experiment with the kernel patches.
> 
> I've also CC'ed Matthew who may want to help with review of the test
> and man page that you posted in the cover letter [1].

I'll get around to going through both the LTP and man-page series by the
end of this week. Feel free to also loop me in directly on any subsequent
iterations of the like.

/M

Re: [PATCH 6/7] syscalls/fanotify20: Test file event with broken inode

[Gabriel Krisman Bertazi]

Hi Amir,

thanks for the review.

Amir Goldstein <amir73il@gmail.com> writes:
> Well, I would not expect a FAN_FS_ERROR event to ever have 0 error
> value. Since this test practically only tests ext4, I do not think it
> is reasonable
> for the test to VERIFY a bug. It is fine to write this test with expectations
> that are not met and let it fail.

This gave me a good chuckle. :)  I will check for a
EXT4_ERR_EFSCORRUPTED and propose a fix on ext4.

>
> But a better plan would probably be to merge the patches up to 5 to test
> FAN_FS_ERROR and then add more test cases after ext4 is fixed
> Either that or you fix the ext4 problem along with FAN_FS_ERROR.
>
> Forgot to say that the test needs to declare .needs_cmds "debugfs".
>
> In any case, as far as prerequisite to merging FAN_FS_ERROR
> your WIP tests certainly suffice.
> Please keep your test branch around so we can use it to validate
> the kernel patches.
> I usually hold off on submitting LTP tests for inclusion until at least -rc3
> after kernel patches have been merged.

As requested, I will not send a new version of the test for now. I
published them on the following unstable branch:

  https://gitlab.collabora.com/krisman/ltp -b fan-fs-error

The v1, as submitted in this thread is also available at:

  https://gitlab.collabora.com/krisman/ltp -b fan-fs-error-v1

Thanks,

-- 
Gabriel Krisman Bertazi

Re: [LTP] [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Petr Vorel]

Hi all,

> No problem. That's what review is for ;-)

> BTW, unless anyone is specifically interested I don't think there
> is a reason to re post the test patches before the submission request.
> Certainly not for the small fixes that I requested.

> I do request that you post a link to a branch with the fixed test
> so that we can experiment with the kernel patches.

> I've also CC'ed Matthew who may want to help with review of the test
> and man page that you posted in the cover letter [1].

@Amir Thanks a lot for your review, agree with all you mentioned.

@Gabriel Thanks for your contribution. I'd also consider squashing some of the
commits.

Kind regards,
Petr

> Thanks,
> Amir.

> [1] https://lore.kernel.org/linux-ext4/20210802214645.2633028-1-krisman@collabora.com/T/#m9cf637c6aca94e28390f61deac5a53afbc9e88ae

Re: [LTP] [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Matthew Bobrowski]

Hey Gabriel,

On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:
> Hi all,
> 
> > No problem. That's what review is for ;-)
> 
> > BTW, unless anyone is specifically interested I don't think there
> > is a reason to re post the test patches before the submission request.
> > Certainly not for the small fixes that I requested.
> 
> > I do request that you post a link to a branch with the fixed test
> > so that we can experiment with the kernel patches.
> 
> > I've also CC'ed Matthew who may want to help with review of the test
> > and man page that you posted in the cover letter [1].
> 
> @Amir Thanks a lot for your review, agree with all you mentioned.
> 
> @Gabriel Thanks for your contribution. I'd also consider squashing some of the
> commits.

Is the FAN_FS_ERROR feature to be included within the 5.15 release? If so,
I may need to do some shuffling around as these LTP tests collide with the
ones I author for the FAN_REPORT_PIDFD series.

/M

Re: [LTP] [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Jan Kara]

On Sat 21-08-21 07:58:07, Matthew Bobrowski wrote:
> On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:
> > Hi all,
> > 
> > > No problem. That's what review is for ;-)
> > 
> > > BTW, unless anyone is specifically interested I don't think there
> > > is a reason to re post the test patches before the submission request.
> > > Certainly not for the small fixes that I requested.
> > 
> > > I do request that you post a link to a branch with the fixed test
> > > so that we can experiment with the kernel patches.
> > 
> > > I've also CC'ed Matthew who may want to help with review of the test
> > > and man page that you posted in the cover letter [1].
> > 
> > @Amir Thanks a lot for your review, agree with all you mentioned.
> > 
> > @Gabriel Thanks for your contribution. I'd also consider squashing some of the
> > commits.
> 
> Is the FAN_FS_ERROR feature to be included within the 5.15 release? If so,
> I may need to do some shuffling around as these LTP tests collide with the
> ones I author for the FAN_REPORT_PIDFD series.

No, I don't think FAN_FS_ERROR is quite ready for the coming merge window.
So you should be fine.

								Honza

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

Re: [LTP] [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Matthew Bobrowski]

On Mon, Aug 23, 2021 at 11:35:24AM +0200, Jan Kara wrote:
> On Sat 21-08-21 07:58:07, Matthew Bobrowski wrote:
> > On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:
> > > Hi all,
> > > 
> > > > No problem. That's what review is for ;-)
> > > 
> > > > BTW, unless anyone is specifically interested I don't think there
> > > > is a reason to re post the test patches before the submission request.
> > > > Certainly not for the small fixes that I requested.
> > > 
> > > > I do request that you post a link to a branch with the fixed test
> > > > so that we can experiment with the kernel patches.
> > > 
> > > > I've also CC'ed Matthew who may want to help with review of the test
> > > > and man page that you posted in the cover letter [1].
> > > 
> > > @Amir Thanks a lot for your review, agree with all you mentioned.
> > > 
> > > @Gabriel Thanks for your contribution. I'd also consider squashing some of the
> > > commits.
> > 
> > Is the FAN_FS_ERROR feature to be included within the 5.15 release? If so,
> > I may need to do some shuffling around as these LTP tests collide with the
> > ones I author for the FAN_REPORT_PIDFD series.
> 
> No, I don't think FAN_FS_ERROR is quite ready for the coming merge window.
> So you should be fine.

Alrighty, thanks for letting me know Jan.

/M

Re: [LTP] [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR

[Gabriel Krisman Bertazi]

Matthew Bobrowski <repnop@google.com> writes:

> Hey Gabriel,
>
> On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:
>> Hi all,
>> 
>> > No problem. That's what review is for ;-)
>> 
>> > BTW, unless anyone is specifically interested I don't think there
>> > is a reason to re post the test patches before the submission request.
>> > Certainly not for the small fixes that I requested.
>> 
>> > I do request that you post a link to a branch with the fixed test
>> > so that we can experiment with the kernel patches.
>> 
>> > I've also CC'ed Matthew who may want to help with review of the test
>> > and man page that you posted in the cover letter [1].
>> 
>> @Amir Thanks a lot for your review, agree with all you mentioned.
>> 
>> @Gabriel Thanks for your contribution. I'd also consider squashing some of the
>> commits.
>
> Is the FAN_FS_ERROR feature to be included within the 5.15 release? If so,
> I may need to do some shuffling around as these LTP tests collide with the
> ones I author for the FAN_REPORT_PIDFD series.
>

Matthew,

Hi, sorry for the delay.  I took a short vacation and couldn't follow
up.  I think it is too late for 5.15, please go ahead with
FAN_REPORT_PIDFD series and I will consider them in my future
submission.

Thank you,


-- 
Gabriel Krisman Bertazi