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
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 <[email protected]>
---
.../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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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 <[email protected]>
---
.../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
On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<[email protected]> 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 <[email protected]>
> ---
> .../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.
On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<[email protected]> 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 <[email protected]>
> ---
> .../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.
On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
<[email protected]> 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 <[email protected]>
> ---
> .../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.
Amir Goldstein <[email protected]> writes:
> On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> <[email protected]> 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 <[email protected]>
>> ---
>> .../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 <[email protected]> writes:
> On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> <[email protected]> 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 <[email protected]>
>> ---
>> .../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
<[email protected]> wrote:
>
> Amir Goldstein <[email protected]> writes:
>
> > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > <[email protected]> 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 <[email protected]>
> >> ---
> >> .../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
<[email protected]> wrote:
>
> Amir Goldstein <[email protected]> writes:
>
> > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > <[email protected]> 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 <[email protected]>
> >> ---
> >> .../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/[email protected]/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
> <[email protected]> wrote:
> >
> > Amir Goldstein <[email protected]> writes:
> >
> > > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > > <[email protected]> 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 <[email protected]>
> > >> ---
> > >> .../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 <[email protected]> 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/[email protected]/T/#m9cf637c6aca94e28390f61deac5a53afbc9e88ae
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
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 <[email protected]>
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:
> > 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
Matthew Bobrowski <[email protected]> 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