[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.