[LTP] [PATCH v2 0/3] Fix fanotify14

[LTP] [PATCH v2 0/3] Fix fanotify14

[Martin Doucha]

Fanotify14 tests some fanotify_init() flags which are not supported
on older kernels but doesn't properly check for their availability.
Fix the init flag availability checks and print more verbose
information about which test case is currently being executed.

Martin Doucha (3):
  fanotify14: Print human-readable test case flags
  Add fanotify_get_supported_init_flags() helper function
  fanotify14: Improve check for unsupported init flags

 testcases/kernel/syscalls/fanotify/fanotify.h |  26 ++
 .../kernel/syscalls/fanotify/fanotify14.c     | 234 ++++++++++--------
 2 files changed, 155 insertions(+), 105 deletions(-)

-- 
2.37.3


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

[LTP] [PATCH v2 1/3] fanotify14: Print human-readable test case flags

[Martin Doucha]

It's hard to tell which test case is failing from the current fanotify14
output. Print test case flags to make failure analysis easier.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- Fixed checkpatch.pl complaints about FLAGS_DESC() macro by adding
  a helper struct into testcases

 .../kernel/syscalls/fanotify/fanotify14.c     | 221 ++++++++++--------
 1 file changed, 121 insertions(+), 100 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
index 594259ccf..bfa0349fe 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify14.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
@@ -38,137 +38,158 @@
 #define INODE_EVENTS (FAN_ATTRIB | FAN_CREATE | FAN_DELETE | FAN_MOVE | \
 		      FAN_DELETE_SELF | FAN_MOVE_SELF)
 
+#define FLAGS_DESC(flags) {(flags), (#flags)}
+
 static int fanotify_fd;
 static int fan_report_target_fid_unsupported;
 static int ignore_mark_unsupported;
 
+struct test_case_flags_t {
+	unsigned long long flags;
+	const char *desc;
+};
+
 /*
  * Each test case has been designed in a manner whereby the values defined
  * within should result in the interface to return an error to the calling
  * process.
  */
 static struct test_case_t {
-	unsigned int init_flags;
-	unsigned int mark_flags;
-	/* zero mask expects to fail on fanotify_init() */
-	unsigned long long mask;
+	struct test_case_flags_t init;
+	struct test_case_flags_t mark;
+	/* when mask.flags == 0, fanotify_init() is expected to fail */
+	struct test_case_flags_t mask;
 	int expected_errno;
 } test_cases[] = {
-	{
-		/* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
-		FAN_CLASS_CONTENT | FAN_REPORT_FID, 0, 0, EINVAL
-	},
-	{
-		/* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
-		FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID, 0, 0, EINVAL
-	},
-	{
-		/* INODE_EVENTS in mask without class FAN_REPORT_FID are not valid */
-		FAN_CLASS_NOTIF, 0, INODE_EVENTS, EINVAL
-	},
-	{
-		/* INODE_EVENTS in mask with FAN_MARK_MOUNT are not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_FID, FAN_MARK_MOUNT, INODE_EVENTS, EINVAL
-	},
-	{
-		/* FAN_REPORT_NAME without FAN_REPORT_DIR_FID is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_NAME, 0, 0, EINVAL
-	},
-	{
-		/* FAN_REPORT_NAME without FAN_REPORT_DIR_FID is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_FID | FAN_REPORT_NAME, 0, 0, EINVAL
-	},
-	{
-		/* FAN_REPORT_TARGET_FID without FAN_REPORT_FID is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_TARGET_FID | FAN_REPORT_DFID_NAME, 0, 0, EINVAL
-	},
-	{
-		/* FAN_REPORT_TARGET_FID without FAN_REPORT_NAME is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_TARGET_FID | FAN_REPORT_DFID_FID, 0, 0, EINVAL
-	},
-	{
-		/* FAN_RENAME without FAN_REPORT_NAME is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_FID, 0, FAN_RENAME, EINVAL
-	},
-	{
-		/* With FAN_MARK_ONLYDIR on non-dir is not valid */
-		FAN_CLASS_NOTIF, FAN_MARK_ONLYDIR, FAN_OPEN, ENOTDIR
-	},
-	{
-		/* With FAN_REPORT_TARGET_FID, FAN_DELETE on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET, 0, FAN_DELETE, ENOTDIR
-	},
-	{
-		/* With FAN_REPORT_TARGET_FID, FAN_RENAME on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET, 0, FAN_RENAME, ENOTDIR
-	},
-	{
-		/* With FAN_REPORT_TARGET_FID, FAN_ONDIR on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET, 0, FAN_OPEN | FAN_ONDIR, ENOTDIR
-	},
-	{
-		/* With FAN_REPORT_TARGET_FID, FAN_EVENT_ON_CHILD on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET, 0, FAN_OPEN | FAN_EVENT_ON_CHILD, ENOTDIR
-	},
-	{
-		/* FAN_MARK_IGNORE_SURV with FAN_DELETE on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME, FAN_MARK_IGNORE_SURV, FAN_DELETE, ENOTDIR
-	},
-	{
-		/* FAN_MARK_IGNORE_SURV with FAN_RENAME on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME, FAN_MARK_IGNORE_SURV, FAN_RENAME, ENOTDIR
-	},
-	{
-		/* FAN_MARK_IGNORE_SURV with FAN_ONDIR on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME, FAN_MARK_IGNORE_SURV, FAN_OPEN | FAN_ONDIR, ENOTDIR
-	},
-	{
-		/* FAN_MARK_IGNORE_SURV with FAN_EVENT_ON_CHILD on non-dir is not valid */
-		FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME, FAN_MARK_IGNORE_SURV, FAN_OPEN | FAN_EVENT_ON_CHILD, ENOTDIR
-	},
-	{
-		/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on directory is not valid */
-		FAN_CLASS_NOTIF, FAN_MARK_IGNORE, FAN_OPEN, EISDIR
-	},
-	{
-		/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on mount mark is not valid */
-		FAN_CLASS_NOTIF, FAN_MARK_MOUNT | FAN_MARK_IGNORE, FAN_OPEN, EINVAL
-	},
-	{
-		/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on filesystem mark is not valid */
-		FAN_CLASS_NOTIF, FAN_MARK_FILESYSTEM | FAN_MARK_IGNORE, FAN_OPEN, EINVAL
-	},
+	/* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
+	{FLAGS_DESC(FAN_CLASS_CONTENT | FAN_REPORT_FID), {}, {}, EINVAL},
+
+	/* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
+	{FLAGS_DESC(FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID), {}, {}, EINVAL},
+
+	/* INODE_EVENTS in mask without class FAN_REPORT_FID are not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF), FLAGS_DESC(0), FLAGS_DESC(INODE_EVENTS),
+		EINVAL},
+
+	/* INODE_EVENTS in mask with FAN_MARK_MOUNT are not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_FID),
+		FLAGS_DESC(FAN_MARK_MOUNT), FLAGS_DESC(INODE_EVENTS), EINVAL},
+
+	/* FAN_REPORT_NAME without FAN_REPORT_DIR_FID is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_NAME), {}, {}, EINVAL},
+
+	/* FAN_REPORT_NAME without FAN_REPORT_DIR_FID is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_FID | FAN_REPORT_NAME), {},
+		{}, EINVAL},
+
+	/* FAN_REPORT_TARGET_FID without FAN_REPORT_FID is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_TARGET_FID | FAN_REPORT_DFID_NAME),
+		{}, {}, EINVAL},
+
+	/* FAN_REPORT_TARGET_FID without FAN_REPORT_NAME is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_TARGET_FID | FAN_REPORT_DFID_FID),
+		{}, {}, EINVAL},
+
+	/* FAN_RENAME without FAN_REPORT_NAME is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_FID), FLAGS_DESC(0),
+		FLAGS_DESC(FAN_RENAME), EINVAL},
+
+	/* With FAN_MARK_ONLYDIR on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF), FLAGS_DESC(FAN_MARK_ONLYDIR),
+		FLAGS_DESC(FAN_OPEN), ENOTDIR},
+
+	/* With FAN_REPORT_TARGET_FID, FAN_DELETE on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET),
+		FLAGS_DESC(0), FLAGS_DESC(FAN_DELETE), ENOTDIR},
+
+	/* With FAN_REPORT_TARGET_FID, FAN_RENAME on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET),
+		FLAGS_DESC(0), FLAGS_DESC(FAN_RENAME), ENOTDIR},
+
+	/* With FAN_REPORT_TARGET_FID, FAN_ONDIR on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET),
+		FLAGS_DESC(0), FLAGS_DESC(FAN_OPEN | FAN_ONDIR), ENOTDIR},
+
+	/* With FAN_REPORT_TARGET_FID, FAN_EVENT_ON_CHILD on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME_TARGET),
+		FLAGS_DESC(0), FLAGS_DESC(FAN_OPEN | FAN_EVENT_ON_CHILD),
+		ENOTDIR},
+
+	/* FAN_MARK_IGNORE_SURV with FAN_DELETE on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME),
+		FLAGS_DESC(FAN_MARK_IGNORE_SURV), FLAGS_DESC(FAN_DELETE),
+		ENOTDIR},
+
+	/* FAN_MARK_IGNORE_SURV with FAN_RENAME on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME),
+		FLAGS_DESC(FAN_MARK_IGNORE_SURV), FLAGS_DESC(FAN_RENAME),
+		ENOTDIR},
+
+	/* FAN_MARK_IGNORE_SURV with FAN_ONDIR on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME),
+		FLAGS_DESC(FAN_MARK_IGNORE_SURV),
+		FLAGS_DESC(FAN_OPEN | FAN_ONDIR), ENOTDIR},
+
+	/* FAN_MARK_IGNORE_SURV with FAN_EVENT_ON_CHILD on non-dir is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF | FAN_REPORT_DFID_NAME),
+		FLAGS_DESC(FAN_MARK_IGNORE_SURV),
+		FLAGS_DESC(FAN_OPEN | FAN_EVENT_ON_CHILD), ENOTDIR},
+
+	/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on directory is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF), FLAGS_DESC(FAN_MARK_IGNORE),
+		FLAGS_DESC(FAN_OPEN), EISDIR},
+
+	/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on mount mark is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF),
+		FLAGS_DESC(FAN_MARK_MOUNT | FAN_MARK_IGNORE),
+		FLAGS_DESC(FAN_OPEN), EINVAL},
+
+	/* FAN_MARK_IGNORE without FAN_MARK_IGNORED_SURV_MODIFY on filesystem mark is not valid */
+	{FLAGS_DESC(FAN_CLASS_NOTIF),
+		FLAGS_DESC(FAN_MARK_FILESYSTEM | FAN_MARK_IGNORE),
+		FLAGS_DESC(FAN_OPEN), EINVAL},
 };
 
 static void do_test(unsigned int number)
 {
 	struct test_case_t *tc = &test_cases[number];
 
-	if (fan_report_target_fid_unsupported && tc->init_flags & FAN_REPORT_TARGET_FID) {
+	tst_res(TINFO, "Test case %d: fanotify_init(%s, O_RDONLY)", number,
+		tc->init.desc);
+
+	if (fan_report_target_fid_unsupported && tc->init.flags & FAN_REPORT_TARGET_FID) {
 		FANOTIFY_INIT_FLAGS_ERR_MSG(FAN_REPORT_TARGET_FID,
 					    fan_report_target_fid_unsupported);
 		return;
 	}
 
-	if (ignore_mark_unsupported && tc->mark_flags & FAN_MARK_IGNORE) {
+	if (ignore_mark_unsupported && tc->mark.flags & FAN_MARK_IGNORE) {
 		tst_res(TCONF, "FAN_MARK_IGNORE not supported in kernel?");
 		return;
 	}
 
-	TST_EXP_FD_OR_FAIL(fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY),
-			   !tc->mask && tc->expected_errno ? tc->expected_errno : 0);
+	if (!tc->mask.flags && tc->expected_errno) {
+		TST_EXP_FAIL(fanotify_init(tc->init.flags, O_RDONLY),
+			tc->expected_errno);
+	} else {
+		TST_EXP_FD(fanotify_init(tc->init.flags, O_RDONLY));
+	}
+
+	fanotify_fd = TST_RET;
 
 	if (fanotify_fd < 0)
 		return;
 
-	if (!tc->mask)
+	if (!tc->mask.flags)
 		goto out;
 
 	/* Set mark on non-dir only when expecting error ENOTDIR */
 	const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT;
 
-	TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
-					 tc->mask, AT_FDCWD, path),
+	tst_res(TINFO, "Testing fanotify_mark(FAN_MARK_ADD | %s, %s)",
+		tc->mark.desc, tc->mask.desc);
+	TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags,
+					 tc->mask.flags, AT_FDCWD, path),
 					 tc->expected_errno);
 
 	/*
@@ -178,15 +199,15 @@ static void do_test(unsigned int number)
 	 * and it should succeed.
 	 */
 	if (TST_PASS && tc->expected_errno == ENOTDIR) {
-		SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
-				   tc->mask, AT_FDCWD, MNTPOINT);
+		SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark.flags,
+				   tc->mask.flags, AT_FDCWD, MNTPOINT);
 		tst_res(TPASS,
 			"Adding an inode mark on directory did not fail with "
 			"ENOTDIR error as on non-dir inode");
 
-		if (!(tc->mark_flags & FAN_MARK_ONLYDIR)) {
-			SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags |
-					   FAN_MARK_FILESYSTEM, tc->mask,
+		if (!(tc->mark.flags & FAN_MARK_ONLYDIR)) {
+			SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark.flags |
+					   FAN_MARK_FILESYSTEM, tc->mask.flags,
 					   AT_FDCWD, FILE1);
 			tst_res(TPASS,
 				"Adding a filesystem mark on non-dir did not fail with "
-- 
2.37.3


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

Re: [LTP] [PATCH v2 1/3] fanotify14: Print human-readable test case flags

[Richard Palethorpe]

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> It's hard to tell which test case is failing from the current fanotify14
> output. Print test case flags to make failure analysis easier.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>

I'd be happy to merge this part of the patch-set at least.

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v2 1/3] fanotify14: Print human-readable test case flags

[Richard Palethorpe]

Hello,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Martin Doucha <mdoucha@suse.cz> writes:
>
>> It's hard to tell which test case is failing from the current fanotify14
>> output. Print test case flags to make failure analysis easier.
>>
>> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
>
> I'd be happy to merge this part of the patch-set at least.
>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

Merged, thanks!

-- 
Thank you,
Richard.

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

[LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

Since FAN_ALL_INIT_FLAGS constant is deprecated, the kernel has added
new fanotify feature flags and there is no other way to check
for their support, we need to manually check which init flags needed
by our tests are available.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- Fixed check for FAN_REPORT_NAME
- Added longer patch description

Thanks for pointing out the flag dependency between FAN_REPORT_NAME and
FAN_REPORT_DIR_FID. I must have misread the documentation on that one.
Since this appears to be the only flag with a dependency for now, let's
keep the special handling simple. If the kernel adds more flags that are
invalid on their own, we should handle that using a table.

These flag support checks will be needed in multiple tests so it's better
to have one common function that'll do them in one call than to copy-paste
multiple setup steps from one test to another.

Though it'd be great if kernel itself would provide a syscall that returns
all supported fanotify init, mark or mask flags in one call.

 testcases/kernel/syscalls/fanotify/fanotify.h | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 51078103e..f3ac1630f 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -213,6 +213,32 @@ static inline int fanotify_init_flags_supported_by_kernel(unsigned int flags)
 	return fanotify_init_flags_supported_on_fs(flags, NULL);
 }
 
+/*
+ * Check support of given init flags one by one and return those which are
+ * supported.
+ */
+static inline unsigned int fanotify_get_supported_init_flags(unsigned int flags,
+	const char *fname)
+{
+	unsigned int flg, arg, ret = 0;
+
+	for (flg = 1; flg; flg <<= 1) {
+		if (!(flags & flg))
+			continue;
+
+		arg = flg;
+
+		// FAN_REPORT_NAME is invalid without FAN_REPORT_DIR_FID
+		if (flg == FAN_REPORT_NAME)
+			arg |= FAN_REPORT_DIR_FID;
+
+		if (!fanotify_init_flags_supported_on_fs(arg, fname))
+			ret |= flg;
+	}
+
+	return ret;
+}
+
 typedef void (*tst_res_func_t)(const char *file, const int lineno,
 			       int ttype, const char *fmt, ...);
 
-- 
2.37.3


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Thu, Oct 20, 2022 at 4:08 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> Since FAN_ALL_INIT_FLAGS constant is deprecated, the kernel has added
> new fanotify feature flags and there is no other way to check
> for their support, we need to manually check which init flags needed
> by our tests are available.
>
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
>
> Changes since v1:
> - Fixed check for FAN_REPORT_NAME
> - Added longer patch description
>
> Thanks for pointing out the flag dependency between FAN_REPORT_NAME and
> FAN_REPORT_DIR_FID. I must have misread the documentation on that one.
> Since this appears to be the only flag with a dependency for now, let's
> keep the special handling simple. If the kernel adds more flags that are
> invalid on their own, we should handle that using a table.
>
> These flag support checks will be needed in multiple tests so it's better
> to have one common function that'll do them in one call than to copy-paste
> multiple setup steps from one test to another.
>
> Though it'd be great if kernel itself would provide a syscall that returns
> all supported fanotify init, mark or mask flags in one call.
>
>  testcases/kernel/syscalls/fanotify/fanotify.h | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
> index 51078103e..f3ac1630f 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> @@ -213,6 +213,32 @@ static inline int fanotify_init_flags_supported_by_kernel(unsigned int flags)
>         return fanotify_init_flags_supported_on_fs(flags, NULL);
>  }
>
> +/*
> + * Check support of given init flags one by one and return those which are
> + * supported.
> + */
> +static inline unsigned int fanotify_get_supported_init_flags(unsigned int flags,
> +       const char *fname)
> +{
> +       unsigned int flg, arg, ret = 0;
> +
> +       for (flg = 1; flg; flg <<= 1) {
> +               if (!(flags & flg))
> +                       continue;
> +
> +               arg = flg;
> +
> +               // FAN_REPORT_NAME is invalid without FAN_REPORT_DIR_FID
> +               if (flg == FAN_REPORT_NAME)
> +                       arg |= FAN_REPORT_DIR_FID;
> +

NACK
this is not the only dependency
this is not a valid generic function.

I only gave a recipe in v1 review how I think the checks should be done.

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 20. 10. 22 17:36, Amir Goldstein wrote:
> NACK
> this is not the only dependency
> this is not a valid generic function.
> 
> I only gave a recipe in v1 review how I think the checks should be done.

I still want to make something that is easy to reuse in other fanotify 
tests. It doesn't have to be fully generic. If you want, I can add a 
list of manually validated init flags into the support check function 
and make the function terminate the program when somebody passes flags 
that haven't been validated. That'll ensure that the flag dependency 
list will be kept up to date.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Fri, Oct 21, 2022 at 4:49 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 20. 10. 22 17:36, Amir Goldstein wrote:
> > NACK
> > this is not the only dependency
> > this is not a valid generic function.
> >
> > I only gave a recipe in v1 review how I think the checks should be done.
>
> I still want to make something that is easy to reuse in other fanotify
> tests. It doesn't have to be fully generic. If you want, I can add a
> list of manually validated init flags into the support check function
> and make the function terminate the program when somebody passes flags
> that haven't been validated. That'll ensure that the flag dependency
> list will be kept up to date.
>

I don't have a vision of what you are proposing.
Make a proposal and I will see if it is correct.

I must say I don't understand what it is that you are trying to improve.
All the test needs to know is if the specific combinations of flags that
the test uses are supported by the kernel/fs.

Trying to figure out which of the bits from a specific combination is
not supported? how does that help users?
Maybe in kernel 5.10 flag X is supported and in kernel 5.11 flag
Y is also supported, but only in kernel 5.12 the combination X | Y
is supported? Do you see why your generic function doesn't make
much sense? or is just too complex to be worth the trouble
for an informational print?

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 21. 10. 22 21:03, Amir Goldstein wrote:
> I don't have a vision of what you are proposing.
> Make a proposal and I will see if it is correct.
> 
> I must say I don't understand what it is that you are trying to improve.
> All the test needs to know is if the specific combinations of flags that
> the test uses are supported by the kernel/fs.
> 
> Trying to figure out which of the bits from a specific combination is
> not supported? how does that help users?
> Maybe in kernel 5.10 flag X is supported and in kernel 5.11 flag
> Y is also supported, but only in kernel 5.12 the combination X | Y
> is supported? Do you see why your generic function doesn't make
> much sense? or is just too complex to be worth the trouble
> for an informational print?

The function I'm trying to write is supposed to check whether a 
particular flag is implemented by the kernel. Whether a particular 
combination of implemented flags is also *allowed* is out of scope.

Note that the test I'm writing this for is fanotify14, which checks that 
various invalid combinations of flags will correctly return error. But 
since the error code for "this flag is not implemented" and "this flag 
was used incorrectly" is the same, I need to somehow get the fanotify 
feature set so that I can skip test cases which are not compatible with 
the running kernel. I don't really care about which specific flag is not 
implemented, comparing flags against a bitmask is just a quick and 
convenient way to check that running the test case makes sense.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 2314 bytes --]

On Mon, Oct 24, 2022, 12:03 PM Martin Doucha <mdoucha@suse.cz> wrote:

> On 21. 10. 22 21:03, Amir Goldstein wrote:
> > I don't have a vision of what you are proposing.
> > Make a proposal and I will see if it is correct.
> >
> > I must say I don't understand what it is that you are trying to improve.
> > All the test needs to know is if the specific combinations of flags that
> > the test uses are supported by the kernel/fs.
> >
> > Trying to figure out which of the bits from a specific combination is
> > not supported? how does that help users?
> > Maybe in kernel 5.10 flag X is supported and in kernel 5.11 flag
> > Y is also supported, but only in kernel 5.12 the combination X | Y
> > is supported? Do you see why your generic function doesn't make
> > much sense? or is just too complex to be worth the trouble
> > for an informational print?
>
> The function I'm trying to write is supposed to check whether a
> particular flag is implemented by the kernel. Whether a particular
> combination of implemented flags is also *allowed* is out of scope.
>
> Note that the test I'm writing this for is fanotify14, which checks that
> various invalid combinations of flags will correctly return error. But
> since the error code for "this flag is not implemented" and "this flag
> was used incorrectly" is the same, I need to somehow get the fanotify
> feature set so that I can skip test cases which are not compatible with
> the running kernel. I don't really care about which specific flag is not
> implemented, comparing flags against a bitmask is just a quick and
> convenient way to check that running the test case makes sense.
>


Why is skipping the test better than passing the test?

The test wants to know that a specific flag combination is not allowed.

It is particarly not allowed also on old kernels that do not support either
individual flag.

What's the difference?

Who is going to gain anything from this change?

Sorry for being strict on this point
I may be missing something.

Please clarify what it is the problem use case is and I will suggest a
solution, because I disagree with this solution.

Thanks,
Amir.


> --
> Martin Doucha   mdoucha@suse.cz
> QA Engineer for Software Maintenance
> SUSE LINUX, s.r.o.
> CORSO IIa
> Krizikova 148/34
> 186 00 Prague 8
> Czech Republic
>
>

[-- Attachment #1.2: Type: text/html, Size: 3509 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 24. 10. 22 15:08, Amir Goldstein wrote:
> Why is skipping the test better than passing the test?
> 
> The test wants to know that a specific flag combination is not allowed.
> 
> It is particarly not allowed also on old kernels that do not support 
> either individual flag.
> 
> What's the difference?
> 
> Who is going to gain anything from this change?
> 
> Sorry for being strict on this point
> I may be missing something.
> 
> Please clarify what it is the problem use case is and I will suggest a 
> solution, because I disagree with this solution.

We're running fanotify14 on kernel 5.3 and one of the subtests (test 
case #8) is using a flag that was added in kernel 5.9. But in this case, 
fanotify_init() is supposed to pass and then fanotify_mark() is supposed 
to fail because of mismatch between mark() and init() flags. I'm trying 
to fix this particular subtest failure caused by wrong feature check.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Mon, Oct 24, 2022 at 4:16 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 24. 10. 22 15:08, Amir Goldstein wrote:
> > Why is skipping the test better than passing the test?
> >
> > The test wants to know that a specific flag combination is not allowed.
> >
> > It is particarly not allowed also on old kernels that do not support
> > either individual flag.
> >
> > What's the difference?
> >
> > Who is going to gain anything from this change?
> >
> > Sorry for being strict on this point
> > I may be missing something.
> >
> > Please clarify what it is the problem use case is and I will suggest a
> > solution, because I disagree with this solution.
>
> We're running fanotify14 on kernel 5.3 and one of the subtests (test
> case #8) is using a flag that was added in kernel 5.9. But in this case,
> fanotify_init() is supposed to pass and then fanotify_mark() is supposed
> to fail because of mismatch between mark() and init() flags. I'm trying
> to fix this particular subtest failure caused by wrong feature check.
>

Got it. If you wrote this information somewhere else
I must have missed it - if you did not, please write explicitly
which test case is being fixed on which kernel version/config.

This is how I would fix the problem:

        if (!tc->mask) {
                /* tc->expected_errno refers to the expected result of
fanotify_init() */
                TST_EXP_FD_OR_FAIL(fanotify_fd =
fanotify_init(tc->init_flags, O_RDONLY),
                                   tc->expected_errno);
        } else {
                /* tc->expected_errno refers to the expected result of
fanotify_mark() */
                fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY);
                if (fanotify_fd < 0) {
                        if (errno == EINVAL)
                                tst_res(TCONF, "fanotify_init flags
XXX not supported");
                        else
                                tst_res(TFAIL, "fanotify_init failed");
                }
        }

Give or take using more macros and your nicer flag prints.
There is no need for auto-detection of the unsupported kernel flags.

If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
EINVAL from fanotify_init() always means "unsupported".

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 24. 10. 22 16:34, Amir Goldstein wrote:
> This is how I would fix the problem:
> 
> <snip>
> 
> Give or take using more macros and your nicer flag prints.
> There is no need for auto-detection of the unsupported kernel flags.
> 
> If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
> EINVAL from fanotify_init() always means "unsupported".

This would be a good approach if fanotify_init() returned distinct error 
code for "flag not implemented", like ENOTSUP. But when EINVAL is 
returned for both "not implemented" and "wrong use", this has a risk of 
hiding real bugs. That's why I'm trying to detect the actual set of 
flags implemented in the running kernel before running the real tests.

And since some flags may be backported to older kernels, generating 
feature sets based on kernel version is not a solution.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Petr Vorel]

Hi all,

> On 24. 10. 22 16:34, Amir Goldstein wrote:
> > This is how I would fix the problem:

> > <snip>

> > Give or take using more macros and your nicer flag prints.
> > There is no need for auto-detection of the unsupported kernel flags.

> > If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
> > EINVAL from fanotify_init() always means "unsupported".

> This would be a good approach if fanotify_init() returned distinct error
> code for "flag not implemented", like ENOTSUP. But when EINVAL is returned
> for both "not implemented" and "wrong use", this has a risk of hiding real
> bugs. That's why I'm trying to detect the actual set of flags implemented in
> the running kernel before running the real tests.
Indeed, that's quite surprising (not really, it was added in 2.6.36 and remember
Jan Kara's talk about dnotify/inotify/fanotify history). I wonder if it's
possible to fix (backward compatibility would suffer).

> And since some flags may be backported to older kernels, generating feature
> sets based on kernel version is not a solution.
I guess even some not-important fix was not backported. I guess features aren't
backported to the stable kernel maybe to enterprise kernels (SLES, RHEL), but
even there I'd guess there are related patches backported, not features. But
maybe I'm wrong. Jan and Amir?

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Mon, Oct 24, 2022 at 7:15 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi all,
>
> > On 24. 10. 22 16:34, Amir Goldstein wrote:
> > > This is how I would fix the problem:
>
> > > <snip>
>
> > > Give or take using more macros and your nicer flag prints.
> > > There is no need for auto-detection of the unsupported kernel flags.
>
> > > If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
> > > EINVAL from fanotify_init() always means "unsupported".
>
> > This would be a good approach if fanotify_init() returned distinct error
> > code for "flag not implemented", like ENOTSUP. But when EINVAL is returned
> > for both "not implemented" and "wrong use", this has a risk of hiding real
> > bugs. That's why I'm trying to detect the actual set of flags implemented in
> > the running kernel before running the real tests.
> Indeed, that's quite surprising (not really, it was added in 2.6.36 and remember
> Jan Kara's talk about dnotify/inotify/fanotify history). I wonder if it's
> possible to fix (backward compatibility would suffer).
>

The kernel UAPI is not very consistent about ENOTSUP vs. EINVAL
renameat(2), unlink(2), link(2) and other return EINVAL for unsupported flags
fallocate(2), ioctl(2) and others return ENOTSUP for unsupported commands.
It's not a clear cut, but ENOTSUP is generally for unsupported fs methods,
not for unsupported options.

> > And since some flags may be backported to older kernels, generating feature
> > sets based on kernel version is not a solution.
> I guess even some not-important fix was not backported. I guess features aren't
> backported to the stable kernel maybe to enterprise kernels (SLES, RHEL), but
> even there I'd guess there are related patches backported, not features. But
> maybe I'm wrong. Jan and Amir?
>

I am fine with writing tests that are friendly to distros that want to backport
features, that is not what my complaint is about.
As I wrote to Martin, I will accept any imaginary kernel as an example
for why the test is wrong for that kernel, but I don't see the bug.

The desire to distinguish between "this kernel should support
these init flags but failed" and "this kernel does not support those init flags"
is not realistic, given that fanotify_init() return codes do not
distinguish between those two cases.

The suggested logic to work this out by testing flag by flags is faulty
because of current and future flag dependencies.

So show me a real bug, as Martin did, and we will fix it.

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Petr Vorel]

> On Mon, Oct 24, 2022 at 7:15 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi all,

> > > On 24. 10. 22 16:34, Amir Goldstein wrote:
> > > > This is how I would fix the problem:

> > > > <snip>

> > > > Give or take using more macros and your nicer flag prints.
> > > > There is no need for auto-detection of the unsupported kernel flags.

> > > > If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
> > > > EINVAL from fanotify_init() always means "unsupported".

> > > This would be a good approach if fanotify_init() returned distinct error
> > > code for "flag not implemented", like ENOTSUP. But when EINVAL is returned
> > > for both "not implemented" and "wrong use", this has a risk of hiding real
> > > bugs. That's why I'm trying to detect the actual set of flags implemented in
> > > the running kernel before running the real tests.
> > Indeed, that's quite surprising (not really, it was added in 2.6.36 and remember
> > Jan Kara's talk about dnotify/inotify/fanotify history). I wonder if it's
> > possible to fix (backward compatibility would suffer).

Hi Amir,

> The kernel UAPI is not very consistent about ENOTSUP vs. EINVAL
> renameat(2), unlink(2), link(2) and other return EINVAL for unsupported flags
> fallocate(2), ioctl(2) and others return ENOTSUP for unsupported commands.
> It's not a clear cut, but ENOTSUP is generally for unsupported fs methods,
> not for unsupported options.

thanks for info, I didn't know that. Otherwise Martin's note to use ENOTSUP
would help.

> > > And since some flags may be backported to older kernels, generating feature
> > > sets based on kernel version is not a solution.
> > I guess even some not-important fix was not backported. I guess features aren't
> > backported to the stable kernel maybe to enterprise kernels (SLES, RHEL), but
> > even there I'd guess there are related patches backported, not features. But
> > maybe I'm wrong. Jan and Amir?


> I am fine with writing tests that are friendly to distros that want to backport
> features, that is not what my complaint is about.
> As I wrote to Martin, I will accept any imaginary kernel as an example
> for why the test is wrong for that kernel, but I don't see the bug.

> The desire to distinguish between "this kernel should support
> these init flags but failed" and "this kernel does not support those init flags"
> is not realistic, given that fanotify_init() return codes do not
> distinguish between those two cases.

> The suggested logic to work this out by testing flag by flags is faulty
> because of current and future flag dependencies.

> So show me a real bug, as Martin did, and we will fix it.
thanks for info. Fortunately there is no other bug, besides the one Martin
reported and is trying to fix.

Kind regards,
Petr

> Thanks,
> Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 24. 10. 22 19:03, Petr Vorel wrote:
>> The kernel UAPI is not very consistent about ENOTSUP vs. EINVAL
>> renameat(2), unlink(2), link(2) and other return EINVAL for unsupported flags
>> fallocate(2), ioctl(2) and others return ENOTSUP for unsupported commands.
>> It's not a clear cut, but ENOTSUP is generally for unsupported fs methods,
>> not for unsupported options.
> 
> thanks for info, I didn't know that. Otherwise Martin's note to use ENOTSUP
> would help.

I was not suggesting to change the kernel API, that's not a reasonable 
option at this point. I was just pointing out that the API design limits 
our options how to write reliable tests.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Petr Vorel]

> On 24. 10. 22 19:03, Petr Vorel wrote:
> > > The kernel UAPI is not very consistent about ENOTSUP vs. EINVAL
> > > renameat(2), unlink(2), link(2) and other return EINVAL for unsupported flags
> > > fallocate(2), ioctl(2) and others return ENOTSUP for unsupported commands.
> > > It's not a clear cut, but ENOTSUP is generally for unsupported fs methods,
> > > not for unsupported options.

> > thanks for info, I didn't know that. Otherwise Martin's note to use ENOTSUP
> > would help.

> I was not suggesting to change the kernel API, that's not a reasonable
> option at this point. I was just pointing out that the API design limits our
> options how to write reliable tests.

Sure, that was my suggestion. I meant it more as future improvement than to
solution to our problem, but I wasn't sure myself whether it be a good way.
It just remind me a different case, where errno was changed by accident and
kept that way (fix [1] was not accepted, instead the change was backported to
all live stable/LTS so that errno is at least consistent).

I also wonder whether real users of the API (not just test writers) would
appreciate to distinguish between these two cases. But anyway, I understand that
there would have to be a strong need for it to be reason to change, thus not
acceptable.

Kind regards,
Petr

[1] https://lore.kernel.org/netdev/20170630073448.GA9546@unicorn.suse.cz/

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Mon, Oct 24, 2022 at 5:58 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 24. 10. 22 16:34, Amir Goldstein wrote:
> > This is how I would fix the problem:
> >
> > <snip>
> >
> > Give or take using more macros and your nicer flag prints.
> > There is no need for auto-detection of the unsupported kernel flags.
> >
> > If the test case is expected to get to fanotify_mark() (i.e. non-zero tc->mask)
> > EINVAL from fanotify_init() always means "unsupported".
>
> This would be a good approach if fanotify_init() returned distinct error
> code for "flag not implemented", like ENOTSUP. But when EINVAL is
> returned for both "not implemented" and "wrong use", this has a risk of
> hiding real bugs.

Show me how this could hide a real bug.
Give an example.
It does not need to be a specific kernel
use an example with imaginary kernel with a backported feature if you like.

fanotify14 is not about making sure that flag combinations are allowed
it is about making sure that flag combinations are not allowed.

If the test case is testing illegal init flags, the outcome must be
fanotify_init
EINVAL.

If the test case is testing illegal mark flags, the outcome of fanotify_init
may be EINVAL meaning that this test case will be skipped.
It does not matter which specific init flag or init flag combination
causes this EINVAL.

I am ready to be proven wrong, but with examples,
like the one you provided with test case #8 and kernel 5.3.
hand waving and talking about vague "real bugs" won't convince me.

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 24. 10. 22 18:18, Amir Goldstein wrote:
> Show me how this could hide a real bug.
> Give an example.
> It does not need to be a specific kernel
> use an example with imaginary kernel with a backported feature if you like.
> 
> fanotify14 is not about making sure that flag combinations are allowed
> it is about making sure that flag combinations are not allowed.
> 
> If the test case is testing illegal init flags, the outcome must be
> fanotify_init
> EINVAL.
> 
> If the test case is testing illegal mark flags, the outcome of fanotify_init
> may be EINVAL meaning that this test case will be skipped.
> It does not matter which specific init flag or init flag combination
> causes this EINVAL.
> 
> I am ready to be proven wrong, but with examples,
> like the one you provided with test case #8 and kernel 5.3.
> hand waving and talking about vague "real bugs" won't convince me.

Imagine two init flags, A and B (doesn't matter which ones) that are not 
supposed to conflict in any way according to documentation. And we'll 
add 3 fanotify14 test cases with the following init calls:
- fanotify_init(A)
- fanotify_init(B)
- fanotify_init(A|B)

All 3 init calls are supposed to pass and then fanotify_mark() is 
supposed to fail. Now imagine that we have a buggy kernel where both 
flags are implemented but fanotify_init(A|B) hits a weird corner case 
and returns EINVAL. In your version of the code, the test will assume 
that it's due to a missing feature and report the test case as skipped. 
In my version of the code, the test will report a bug because it knows 
that all the required features are present.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Jan Kara]

On Tue 25-10-22 10:51:01, Martin Doucha wrote:
> On 24. 10. 22 18:18, Amir Goldstein wrote:
> > Show me how this could hide a real bug.
> > Give an example.
> > It does not need to be a specific kernel
> > use an example with imaginary kernel with a backported feature if you like.
> > 
> > fanotify14 is not about making sure that flag combinations are allowed
> > it is about making sure that flag combinations are not allowed.
> > 
> > If the test case is testing illegal init flags, the outcome must be
> > fanotify_init
> > EINVAL.
> > 
> > If the test case is testing illegal mark flags, the outcome of fanotify_init
> > may be EINVAL meaning that this test case will be skipped.
> > It does not matter which specific init flag or init flag combination
> > causes this EINVAL.
> > 
> > I am ready to be proven wrong, but with examples,
> > like the one you provided with test case #8 and kernel 5.3.
> > hand waving and talking about vague "real bugs" won't convince me.
> 
> Imagine two init flags, A and B (doesn't matter which ones) that are not
> supposed to conflict in any way according to documentation. And we'll add 3
> fanotify14 test cases with the following init calls:
> - fanotify_init(A)
> - fanotify_init(B)
> - fanotify_init(A|B)
> 
> All 3 init calls are supposed to pass and then fanotify_mark() is supposed
> to fail. Now imagine that we have a buggy kernel where both flags are
> implemented but fanotify_init(A|B) hits a weird corner case and returns
> EINVAL. In your version of the code, the test will assume that it's due to a
> missing feature and report the test case as skipped. In my version of the
> code, the test will report a bug because it knows that all the required
> features are present.

Yeah, but AFAIU you are trading expanded testing for possibility of false
test failures (because the situation that for some features A and B, both A
and B are implemented but A|B got implemented only later seems equally
probable scenario).

Since I don't find this critical to test (it seems like a relatively
unlikely mistake to do), I'd prefer less testing against false test
failures. If we want to be more precise, we should be spelling out in the
testcase (ideally using some common infrastructure) that if A & B is
supported, we also expect A|B (or even some C) to work.

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

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 25. 10. 22 11:48, Jan Kara wrote:
> On Tue 25-10-22 10:51:01, Martin Doucha wrote:
>> Imagine two init flags, A and B (doesn't matter which ones) that are not
>> supposed to conflict in any way according to documentation. And we'll add 3
>> fanotify14 test cases with the following init calls:
>> - fanotify_init(A)
>> - fanotify_init(B)
>> - fanotify_init(A|B)
>>
>> All 3 init calls are supposed to pass and then fanotify_mark() is supposed
>> to fail. Now imagine that we have a buggy kernel where both flags are
>> implemented but fanotify_init(A|B) hits a weird corner case and returns
>> EINVAL. In your version of the code, the test will assume that it's due to a
>> missing feature and report the test case as skipped. In my version of the
>> code, the test will report a bug because it knows that all the required
>> features are present.
> 
> Yeah, but AFAIU you are trading expanded testing for possibility of false
> test failures (because the situation that for some features A and B, both A
> and B are implemented but A|B got implemented only later seems equally
> probable scenario).
> 
> Since I don't find this critical to test (it seems like a relatively
> unlikely mistake to do), I'd prefer less testing against false test
> failures. If we want to be more precise, we should be spelling out in the
> testcase (ideally using some common infrastructure) that if A & B is
> supported, we also expect A|B (or even some C) to work.

This kind of failure may be highly unlikely on a vanilla kernel but it 
can easily happen due to incorrectly backported patches. IMHO it's 
better to get a failure and find out that the test case was invalid than 
to ignore a bug that may indicate deeper issues. We can always fix a 
broken test and possibly also update documentation of past changes in 
syscall behavior.

On 25. 10. 22 13:11, Amir Goldstein wrote:
> It is a valid test case to assert that the support for two flags is
> independent,
> but this is not the job of fanotify14.
> fanotify14 checks for *illegal* flag combinations.
> 
> If you feel that there should be a test that verifies that
> support of flag A is independent of support of flag B,
> then please write a different test for that.
> 
> But then would you test all possible permutations of flags?
> Not only flags that are used in fanotify14?
> Not only flag pairs? but more concurrent flags?
> I don't know if other APIs have such rigorous tests (except API fuzzers).
> 
> I agree with Jan that the value of such a test would be questionable,
> but it does have a value, so I won't object to having this test, as
> long as it does not blindly check for all the known fanotify init bits
> are independent.

We find a fair amount of kernel bugs not because we have a specific 
targeted testcase for them but because they break test setup for 
something else. The subtests in fanotify14 may not comprehensively test 
all combinations of init flags but it's still "free" test coverage that 
will be useful for catching bugs.

> Asserting flag combination independence should be opt-in by the test
> not out-out like you did with REPORT_FID and REPORT_NAME.

I don't understand this sentence, especially which patch it's referring to.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Tue, Oct 25, 2022 at 4:55 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 25. 10. 22 11:48, Jan Kara wrote:
> > On Tue 25-10-22 10:51:01, Martin Doucha wrote:
> >> Imagine two init flags, A and B (doesn't matter which ones) that are not
> >> supposed to conflict in any way according to documentation. And we'll add 3
> >> fanotify14 test cases with the following init calls:
> >> - fanotify_init(A)
> >> - fanotify_init(B)
> >> - fanotify_init(A|B)
> >>
> >> All 3 init calls are supposed to pass and then fanotify_mark() is supposed
> >> to fail. Now imagine that we have a buggy kernel where both flags are
> >> implemented but fanotify_init(A|B) hits a weird corner case and returns
> >> EINVAL. In your version of the code, the test will assume that it's due to a
> >> missing feature and report the test case as skipped. In my version of the
> >> code, the test will report a bug because it knows that all the required
> >> features are present.
> >
> > Yeah, but AFAIU you are trading expanded testing for possibility of false
> > test failures (because the situation that for some features A and B, both A
> > and B are implemented but A|B got implemented only later seems equally
> > probable scenario).
> >
> > Since I don't find this critical to test (it seems like a relatively
> > unlikely mistake to do), I'd prefer less testing against false test
> > failures. If we want to be more precise, we should be spelling out in the
> > testcase (ideally using some common infrastructure) that if A & B is
> > supported, we also expect A|B (or even some C) to work.
>
> This kind of failure may be highly unlikely on a vanilla kernel but it
> can easily happen due to incorrectly backported patches. IMHO it's
> better to get a failure and find out that the test case was invalid than
> to ignore a bug that may indicate deeper issues. We can always fix a
> broken test and possibly also update documentation of past changes in
> syscall behavior.
>
> On 25. 10. 22 13:11, Amir Goldstein wrote:
> > It is a valid test case to assert that the support for two flags is
> > independent,
> > but this is not the job of fanotify14.
> > fanotify14 checks for *illegal* flag combinations.
> >
> > If you feel that there should be a test that verifies that
> > support of flag A is independent of support of flag B,
> > then please write a different test for that.
> >
> > But then would you test all possible permutations of flags?
> > Not only flags that are used in fanotify14?
> > Not only flag pairs? but more concurrent flags?
> > I don't know if other APIs have such rigorous tests (except API fuzzers).
> >
> > I agree with Jan that the value of such a test would be questionable,
> > but it does have a value, so I won't object to having this test, as
> > long as it does not blindly check for all the known fanotify init bits
> > are independent.
>
> We find a fair amount of kernel bugs not because we have a specific
> targeted testcase for them but because they break test setup for
> something else. The subtests in fanotify14 may not comprehensively test
> all combinations of init flags but it's still "free" test coverage that
> will be useful for catching bugs.
>
> > Asserting flag combination independence should be opt-in by the test
> > not out-out like you did with REPORT_FID and REPORT_NAME.
>
> I don't understand this sentence, especially which patch it's referring to.
>

All right.

Let's see which flag combinations we have in the relevant tests in fanotify14:

FAN_REPORT_DFID_FID,
FAN_REPORT_DFID_NAME,
FAN_REPORT_DFID_NAME_TARGET,

That's all.

Support for FAN_REPORT_FID is a requirement for the entire test.

FAN_REPORT_TARGET_FID depends on all the rest of the flags
and support for it is already checked explicitly to skip test cases.

FAN_REPORT_NAME depends on FAN_REPORT_DIR_FID.

So effectively fanotify_get_supported_init_flags() only checks
FAN_REPORT_DIR_FID separately from the combination
FAN_REPORT_DFID_FID.

fanotify16, which tests *legal* combinations of these flags
already checks the separate flag FAN_REPORT_DIR_FID
so fanotify test cases with FAN_REPORT_DFID_FID would
fail if a kernel that supports FAN_REPORT_DIR_FID does not
support the combination FAN_REPORT_DFID_FID.

Bottom line:
fanotify_get_supported_init_flags() did not add any test coverage.

This is why it is a slippery slope to suggest fixes without
proving that there is a bug.

Thanks,
Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Martin Doucha]

On 25. 10. 22 18:53, Amir Goldstein wrote:
> All right.
> 
> Let's see which flag combinations we have in the relevant tests in fanotify14:
> 
> FAN_REPORT_DFID_FID,
> FAN_REPORT_DFID_NAME,
> FAN_REPORT_DFID_NAME_TARGET,
> 
> That's all.
> 
> Support for FAN_REPORT_FID is a requirement for the entire test.
> 
> FAN_REPORT_TARGET_FID depends on all the rest of the flags
> and support for it is already checked explicitly to skip test cases.
> 
> FAN_REPORT_NAME depends on FAN_REPORT_DIR_FID.
> 
> So effectively fanotify_get_supported_init_flags() only checks
> FAN_REPORT_DIR_FID separately from the combination
> FAN_REPORT_DFID_FID.
> 
> fanotify16, which tests *legal* combinations of these flags
> already checks the separate flag FAN_REPORT_DIR_FID
> so fanotify test cases with FAN_REPORT_DFID_FID would
> fail if a kernel that supports FAN_REPORT_DIR_FID does not
> support the combination FAN_REPORT_DFID_FID.
> 
> Bottom line:
> fanotify_get_supported_init_flags() did not add any test coverage.
> 
> This is why it is a slippery slope to suggest fixes without
> proving that there is a bug.

If this is a slippery slope, then we're already sliding down since 
commit fe31bfca which broke test case #8 because it's painfully hard to 
maintain multiple separate feature checks in setup() as you insist. I 
want to fix this mess with one simple helper function that will be 
reusable in all fanotify tests. The helper function is an up-to-date 
replacement for the old deprecated FAN_ALL_INIT_FLAGS macro. Nothing 
more, nothing less.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Petr Vorel]

Hi Amir, all,

> On Tue, Oct 25, 2022 at 4:55 PM Martin Doucha <mdoucha@suse.cz> wrote:

> > On 25. 10. 22 11:48, Jan Kara wrote:
> > > On Tue 25-10-22 10:51:01, Martin Doucha wrote:
> > >> Imagine two init flags, A and B (doesn't matter which ones) that are not
> > >> supposed to conflict in any way according to documentation. And we'll add 3
> > >> fanotify14 test cases with the following init calls:
> > >> - fanotify_init(A)
> > >> - fanotify_init(B)
> > >> - fanotify_init(A|B)

> > >> All 3 init calls are supposed to pass and then fanotify_mark() is supposed
> > >> to fail. Now imagine that we have a buggy kernel where both flags are
> > >> implemented but fanotify_init(A|B) hits a weird corner case and returns
> > >> EINVAL. In your version of the code, the test will assume that it's due to a
> > >> missing feature and report the test case as skipped. In my version of the
> > >> code, the test will report a bug because it knows that all the required
> > >> features are present.

> > > Yeah, but AFAIU you are trading expanded testing for possibility of false
> > > test failures (because the situation that for some features A and B, both A
> > > and B are implemented but A|B got implemented only later seems equally
> > > probable scenario).

> > > Since I don't find this critical to test (it seems like a relatively
> > > unlikely mistake to do), I'd prefer less testing against false test
> > > failures. If we want to be more precise, we should be spelling out in the
> > > testcase (ideally using some common infrastructure) that if A & B is
> > > supported, we also expect A|B (or even some C) to work.

> > This kind of failure may be highly unlikely on a vanilla kernel but it
> > can easily happen due to incorrectly backported patches. IMHO it's
> > better to get a failure and find out that the test case was invalid than
> > to ignore a bug that may indicate deeper issues. We can always fix a
> > broken test and possibly also update documentation of past changes in
> > syscall behavior.

> > On 25. 10. 22 13:11, Amir Goldstein wrote:
> > > It is a valid test case to assert that the support for two flags is
> > > independent,
> > > but this is not the job of fanotify14.
> > > fanotify14 checks for *illegal* flag combinations.

> > > If you feel that there should be a test that verifies that
> > > support of flag A is independent of support of flag B,
> > > then please write a different test for that.

> > > But then would you test all possible permutations of flags?
> > > Not only flags that are used in fanotify14?
> > > Not only flag pairs? but more concurrent flags?
> > > I don't know if other APIs have such rigorous tests (except API fuzzers).

> > > I agree with Jan that the value of such a test would be questionable,
> > > but it does have a value, so I won't object to having this test, as
> > > long as it does not blindly check for all the known fanotify init bits
> > > are independent.

> > We find a fair amount of kernel bugs not because we have a specific
> > targeted testcase for them but because they break test setup for
> > something else. The subtests in fanotify14 may not comprehensively test
> > all combinations of init flags but it's still "free" test coverage that
> > will be useful for catching bugs.

> > > Asserting flag combination independence should be opt-in by the test
> > > not out-out like you did with REPORT_FID and REPORT_NAME.

> > I don't understand this sentence, especially which patch it's referring to.


> All right.

> Let's see which flag combinations we have in the relevant tests in fanotify14:

> FAN_REPORT_DFID_FID,
> FAN_REPORT_DFID_NAME,
> FAN_REPORT_DFID_NAME_TARGET,

> That's all.

> Support for FAN_REPORT_FID is a requirement for the entire test.

> FAN_REPORT_TARGET_FID depends on all the rest of the flags
> and support for it is already checked explicitly to skip test cases.

> FAN_REPORT_NAME depends on FAN_REPORT_DIR_FID.

> So effectively fanotify_get_supported_init_flags() only checks
> FAN_REPORT_DIR_FID separately from the combination
> FAN_REPORT_DFID_FID.

> fanotify16, which tests *legal* combinations of these flags
> already checks the separate flag FAN_REPORT_DIR_FID
> so fanotify test cases with FAN_REPORT_DFID_FID would
> fail if a kernel that supports FAN_REPORT_DIR_FID does not
> support the combination FAN_REPORT_DFID_FID.

> Bottom line:
> fanotify_get_supported_init_flags() did not add any test coverage.

> This is why it is a slippery slope to suggest fixes without
> proving that there is a bug.

Naresh Kamboju reported [1]  5.4 mainline LTS kernels are failing on fanotify14,
exactly the same way Martin reported on SLES kernel based 5.3.18 (+ tons of
backported patches). Linaro did not mention this before, because they tested 5.4
on older LTP.

@Amir, Isn't this a reason to either accept these 2 Martin's patches or bring
another approach which fixes the detection in fanotify_init() ?

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/CA+G9fYuT3N0LFaJGzQW2SYPJxEbEWLONDZO2OfBbeHNrsowy2w@mail.gmail.com/

> Thanks,
> Amir.

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

Re: [LTP] [PATCH v2 2/3] Add fanotify_get_supported_init_flags() helper function

[Amir Goldstein]

On Tue, Oct 25, 2022 at 11:51 AM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 24. 10. 22 18:18, Amir Goldstein wrote:
> > Show me how this could hide a real bug.
> > Give an example.
> > It does not need to be a specific kernel
> > use an example with imaginary kernel with a backported feature if you like.
> >
> > fanotify14 is not about making sure that flag combinations are allowed
> > it is about making sure that flag combinations are not allowed.
> >
> > If the test case is testing illegal init flags, the outcome must be
> > fanotify_init
> > EINVAL.
> >
> > If the test case is testing illegal mark flags, the outcome of fanotify_init
> > may be EINVAL meaning that this test case will be skipped.
> > It does not matter which specific init flag or init flag combination
> > causes this EINVAL.
> >
> > I am ready to be proven wrong, but with examples,
> > like the one you provided with test case #8 and kernel 5.3.
> > hand waving and talking about vague "real bugs" won't convince me.
>
> Imagine two init flags, A and B (doesn't matter which ones) that are not
> supposed to conflict in any way according to documentation. And we'll
> add 3 fanotify14 test cases with the following init calls:
> - fanotify_init(A)
> - fanotify_init(B)
> - fanotify_init(A|B)
>
> All 3 init calls are supposed to pass and then fanotify_mark() is
> supposed to fail. Now imagine that we have a buggy kernel where both
> flags are implemented but fanotify_init(A|B) hits a weird corner case
> and returns EINVAL.
> In your version of the code, the test will assume
> that it's due to a missing feature and report the test case as skipped.
> In my version of the code, the test will report a bug because it knows
> that all the required features are present.
>

It is a valid test case to assert that the support for two flags is
independent,
but this is not the job of fanotify14.
fanotify14 checks for *illegal* flag combinations.

If you feel that there should be a test that verifies that
support of flag A is independent of support of flag B,
then please write a different test for that.

But then would you test all possible permutations of flags?
Not only flags that are used in fanotify14?
Not only flag pairs? but more concurrent flags?
I don't know if other APIs have such rigorous tests (except API fuzzers).

I agree with Jan that the value of such a test would be questionable,
but it does have a value, so I won't object to having this test, as
long as it does not blindly check for all the known fanotify init bits
are independent.

Asserting flag combination independence should be opt-in by the test
not out-out like you did with REPORT_FID and REPORT_NAME.

Thanks,
Amir.

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

[LTP] [PATCH v2 3/3] fanotify14: Improve check for unsupported init flags

[Martin Doucha]

Test case 8 of fanotify14 uses init flags supported only on kernel 5.9+
but does not properly check for their support. Rewrite fanotify feature
checks using new helper function.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

Changes since v1:
- Added FAN_CLASS_* constants to support check in setup()
- Added longer patch description

I'd rather not squash this patch so that it can be reverted without
potentially breaking other tests.

 testcases/kernel/syscalls/fanotify/fanotify14.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
index bfa0349fe..8d7282d43 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify14.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
@@ -41,8 +41,8 @@
 #define FLAGS_DESC(flags) {(flags), (#flags)}
 
 static int fanotify_fd;
-static int fan_report_target_fid_unsupported;
 static int ignore_mark_unsupported;
+static unsigned int supported_init_flags;
 
 struct test_case_flags_t {
 	unsigned long long flags;
@@ -157,9 +157,8 @@ static void do_test(unsigned int number)
 	tst_res(TINFO, "Test case %d: fanotify_init(%s, O_RDONLY)", number,
 		tc->init.desc);
 
-	if (fan_report_target_fid_unsupported && tc->init.flags & FAN_REPORT_TARGET_FID) {
-		FANOTIFY_INIT_FLAGS_ERR_MSG(FAN_REPORT_TARGET_FID,
-					    fan_report_target_fid_unsupported);
+	if (tc->init.flags & ~supported_init_flags) {
+		tst_res(TCONF, "Unsupported init flags");
 		return;
 	}
 
@@ -222,11 +221,15 @@ out:
 
 static void do_setup(void)
 {
+	unsigned int all_init_flags = FAN_REPORT_DFID_NAME_TARGET |
+		FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | FAN_CLASS_PRE_CONTENT;
+
 	/* Require FAN_REPORT_FID support for all tests to simplify per test case requirements */
 	REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(FAN_REPORT_FID, MNTPOINT);
 
-	fan_report_target_fid_unsupported =
-		fanotify_init_flags_supported_on_fs(FAN_REPORT_DFID_NAME_TARGET, MNTPOINT);
+	supported_init_flags = fanotify_get_supported_init_flags(all_init_flags,
+		MNTPOINT);
+
 	ignore_mark_unsupported = fanotify_mark_supported_by_kernel(FAN_MARK_IGNORE_SURV);
 
 	/* Create temporary test file to place marks on */
-- 
2.37.3


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