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

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

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 <redacted>
---
 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

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 <redacted>
---
 .../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

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 <redacted>
---
 .../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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 .../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

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 <redacted>
---
 .../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

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

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.

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

Those defines go in fanotify.h

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

Thanks,
Amir.

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

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

Thanks,
Amir.

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

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

Thanks,
Amir.

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

Why is error 0?
What's the rationale?

Thanks,
Amir.

Amir Goldstein [off-list ref] writes:

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

quoted

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 <redacted>
---
 .../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

Amir Goldstein [off-list ref] writes:

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

quoted

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 <redacted>
---
 .../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

On Wed, Aug 4, 2021 at 7:52 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

Amir Goldstein [off-list ref] writes:

quoted

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

quoted

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 <redacted>
---
 .../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.

On Wed, Aug 4, 2021 at 7:54 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

Amir Goldstein [off-list ref] writes:

quoted

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

quoted

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 <redacted>
---
 .../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

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
[off-list ref] wrote:

quoted

Amir Goldstein [off-list ref] writes:

quoted

On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

quoted

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 <redacted>
---
 .../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

Hi Amir,

thanks for the review.

Amir Goldstein [off-list ref] 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

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

Hey Gabriel,

On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:

Hi all,

quoted

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

quoted

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.

quoted

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

quoted

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

On Sat 21-08-21 07:58:07, Matthew Bobrowski wrote:

On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:

quoted

Hi all,

quoted

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

quoted

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.

quoted

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

quoted

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 [off-list ref]
SUSE Labs, CR

On Mon, Aug 23, 2021 at 11:35:24AM +0200, Jan Kara wrote:

On Sat 21-08-21 07:58:07, Matthew Bobrowski wrote:

quoted

On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:

quoted

Hi all,

quoted

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

quoted

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.

quoted

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

quoted

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

Matthew Bobrowski [off-list ref] writes:

Hey Gabriel,

On Fri, Aug 20, 2021 at 12:21:19PM +0200, Petr Vorel wrote:

quoted

Hi all,

quoted

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

quoted

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.

quoted

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

quoted

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