| Message ID | 20200910121628.18505-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9 | expand |
On Thu, Sep 10, 2020 at 3:16 PM Petr Vorel <pvorel@suse.cz> wrote: > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi Amir, > > based on suggestion http://lists.linux.it/pipermail/ltp/2020-September/018891.html, > but not really sure if it should be applied, because we loose warning: > > HINT: You _MAY_ be missing kernel fixes, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9bdda4e9cf2d > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2f02fd3fa13e > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=497b0c5a7c06 > I think your change is fine, because this warning would only send poor stable kernel maintainers on a quest to find a patch to backport, which IMO is not going to end with a fix. The situation with ignored mask logic is that it was broken or not properly defined from day 1 of fanotify, so people are probably using ignored mask only in the very basic combinations. Lately, along with FAN_MARK_FILESYSTEM and related work, more opportunities for using ignored mask have materialized and many old bugs have surfaced. But I'm afraid it will not be feasible to backport all the fixes, so once the ignored logic settles (there are still several bugs left) I will try to properly document what is expected to work in which kernel and will update the man page BUGS section. Jan, Please let me know if we are on the same page in that regard. Thanks, Amir. > Kind regards, > Petr > > > testcases/kernel/syscalls/fanotify/fanotify10.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c > index 2c4401f61..5b4591b4a 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify10.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c > @@ -508,8 +508,8 @@ static void test_fanotify(unsigned int n) > "zero length read from fanotify fd"); > } > if (ret > 0) { > - tst_res(TFAIL, "group %d (%x) with %s and " > - "%s ignore mask got event", > + tst_res((tst_kvercmp(5, 9, 0)) < 0 ? TCONF : TFAIL, > + "group %d (%x) with %s and %s ignore mask got event", > i, fanotify_class[p], mark->name, ignore_mark->name); > if (event->fd != FAN_NOFD) > SAFE_CLOSE(event->fd); > -- > 2.28.0 >
Hi Amir, > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > Hi Amir, > > based on suggestion http://lists.linux.it/pipermail/ltp/2020-September/018891.html, > > but not really sure if it should be applied, because we loose warning: > > HINT: You _MAY_ be missing kernel fixes, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9bdda4e9cf2d > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2f02fd3fa13e > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=497b0c5a7c06 > I think your change is fine, because this warning would only send poor > stable kernel maintainers on a quest to find a patch to backport, which IMO > is not going to end with a fix. > The situation with ignored mask logic is that it was broken or not properly > defined from day 1 of fanotify, so people are probably using ignored mask > only in the very basic combinations. > Lately, along with FAN_MARK_FILESYSTEM and related work, more > opportunities for using ignored mask have materialized and many old bugs > have surfaced. > But I'm afraid it will not be feasible to backport all the fixes, so > once the ignored > logic settles (there are still several bugs left) I will try to > properly document > what is expected to work in which kernel and will update the man page > BUGS section. +1, that'd be great. Having tests early and update man page, not many syscalls have that, thank you. Kind regards, Petr > Jan, > Please let me know if we are on the same page in that regard. > Thanks, > Amir. > > Kind regards, > > Petr > > testcases/kernel/syscalls/fanotify/fanotify10.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c > > index 2c4401f61..5b4591b4a 100644 > > --- a/testcases/kernel/syscalls/fanotify/fanotify10.c > > +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c > > @@ -508,8 +508,8 @@ static void test_fanotify(unsigned int n) > > "zero length read from fanotify fd"); > > } > > if (ret > 0) { > > - tst_res(TFAIL, "group %d (%x) with %s and " > > - "%s ignore mask got event", > > + tst_res((tst_kvercmp(5, 9, 0)) < 0 ? TCONF : TFAIL, > > + "group %d (%x) with %s and %s ignore mask got event", > > i, fanotify_class[p], mark->name, ignore_mark->name); > > if (event->fd != FAN_NOFD) > > SAFE_CLOSE(event->fd); > > -- > > 2.28.0
Hi! > if (ret > 0) { > - tst_res(TFAIL, "group %d (%x) with %s and " > - "%s ignore mask got event", > + tst_res((tst_kvercmp(5, 9, 0)) < 0 ? TCONF : TFAIL, > + "group %d (%x) with %s and %s ignore mask got event", > i, fanotify_class[p], mark->name, ignore_mark->name); I do not like that much, how the tst_kvercmp() is sandwitched inside of the tst_res() call, since it makes it easier to be overlooked. The question is how can we do better, maybe set a variable in test setup? Other than that it looks fine.
On Thu 10-09-20 17:14:03, Amir Goldstein wrote: > On Thu, Sep 10, 2020 at 3:16 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Suggested-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > Hi Amir, > > > > based on suggestion http://lists.linux.it/pipermail/ltp/2020-September/018891.html, > > but not really sure if it should be applied, because we loose warning: > > > > HINT: You _MAY_ be missing kernel fixes, see: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9bdda4e9cf2d > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2f02fd3fa13e > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=497b0c5a7c06 > > > > I think your change is fine, because this warning would only send poor > stable kernel maintainers on a quest to find a patch to backport, which IMO > is not going to end with a fix. > > The situation with ignored mask logic is that it was broken or not properly > defined from day 1 of fanotify, so people are probably using ignored mask > only in the very basic combinations. > > Lately, along with FAN_MARK_FILESYSTEM and related work, more > opportunities for using ignored mask have materialized and many old bugs > have surfaced. > > But I'm afraid it will not be feasible to backport all the fixes, so > once the ignored > logic settles (there are still several bugs left) I will try to > properly document > what is expected to work in which kernel and will update the man page > BUGS section. > > Jan, > > Please let me know if we are on the same page in that regard. Yeah I agree. We've never heard any real user complaining about broken ignore masks behavior so backporting of the respective fixes (which is sometimes rather difficult) is IMHO a wasted effort. I agree that documentation in the BUGS manpage section would be worthwhile though. Honza
On Thu, Sep 10, 2020 at 6:30 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > if (ret > 0) { > > - tst_res(TFAIL, "group %d (%x) with %s and " > > - "%s ignore mask got event", > > + tst_res((tst_kvercmp(5, 9, 0)) < 0 ? TCONF : TFAIL, > > + "group %d (%x) with %s and %s ignore mask got event", > > i, fanotify_class[p], mark->name, ignore_mark->name); > > I do not like that much, how the tst_kvercmp() is sandwitched inside of > the tst_res() call, since it makes it easier to be overlooked. > > The question is how can we do better, maybe set a variable in test > setup? > I apologize. I did not review this change carefully. What I suggested was to skip only the test cases with non zero tc->ignored_onchild for kernel < 5.9. Those are the new test cases that are failing due to missing fix "497b0c5a7c06 fsnotify: send event to parent and child with single" Which is not likely to be backported. I see no reason to run those test cases and then report the failure as TCONF. The TCONF message can say that ignored mask in combination with flag FAN_EVENT_ON_CHILD have undefined behavior on kernel < 5.9. While at it, please remove the stray line from header comment: * 497b0c5a7c06 fsnotify: send event to parent and child with single... * eca4784cbb18 fsnotify: send event to parent and child with single... And please remove commit id 497b0c5a7c06 from .tags, otherwise, stable kernel maintainers are still going to be bothered by attempting to find a backport if that test fails due to other reasons on a stable kernel. Thanks, Amir.
Hi Amir, > What I suggested was to skip only the test cases with non zero > tc->ignored_onchild for kernel < 5.9. > Those are the new test cases that are failing due to missing fix > "497b0c5a7c06 fsnotify: send event to parent and child with single" > Which is not likely to be backported. > I see no reason to run those test cases and then report the failure as > TCONF. > The TCONF message can say that ignored mask in combination with > flag FAN_EVENT_ON_CHILD have undefined behavior on kernel < 5.9. Yep, that makes more sense. > While at it, please remove the stray line from header comment: > * 497b0c5a7c06 fsnotify: send event to parent and child with single... > * eca4784cbb18 fsnotify: send event to parent and child with single... Apologize for not removing eca4784cbb18. > And please remove commit id 497b0c5a7c06 from .tags, otherwise, > stable kernel maintainers are still going to be bothered by attempting > to find a backport if that test fails due to other reasons on a stable kernel. +1. I kept comment in the commit (in v2) and explicitly mention why it's not in tags. Thanks for all suggestions. Just in case I sent v2 instead of merging it directly. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c index 2c4401f61..5b4591b4a 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify10.c +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c @@ -508,8 +508,8 @@ static void test_fanotify(unsigned int n) "zero length read from fanotify fd"); } if (ret > 0) { - tst_res(TFAIL, "group %d (%x) with %s and " - "%s ignore mask got event", + tst_res((tst_kvercmp(5, 9, 0)) < 0 ? TCONF : TFAIL, + "group %d (%x) with %s and %s ignore mask got event", i, fanotify_class[p], mark->name, ignore_mark->name); if (event->fd != FAN_NOFD) SAFE_CLOSE(event->fd);
Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi Amir, based on suggestion http://lists.linux.it/pipermail/ltp/2020-September/018891.html, but not really sure if it should be applied, because we loose warning: HINT: You _MAY_ be missing kernel fixes, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9bdda4e9cf2d https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2f02fd3fa13e https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=497b0c5a7c06 Kind regards, Petr testcases/kernel/syscalls/fanotify/fanotify10.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)