| Message ID | 20221020130843.15147-3-mdoucha@suse.cz |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Fix fanotify14 | expand |
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.
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.
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.
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.
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 > >
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.
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.
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.
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
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.
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.
> 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.
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.
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.
> 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/
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
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.
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.
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.
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.
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.
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, ...);
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(+)