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

[Petr Vorel <pvorel@suse.cz>]

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