[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Petr Vorel]

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(-)

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

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Amir Goldstein]

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
>

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Petr Vorel]

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

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Jan Kara]

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

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

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Amir Goldstein]

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

[LTP] [PATCH 1/1] fanotify10: Treat ignore mask bug as TCONF for < v5.9

[Petr Vorel]

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