[LTP] [PATCH] fanotify14: Test disallow sb/mount mark on anonymous pipe

[Amir Goldstein]

On Tue, Jul 11, 2023 at 9:34 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> > On Mon, Jul 10, 2023 at 6:50 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > Hi Amir,
>
> > > > This case was retroactively disallowed.
>
> > > > This test is meant to encourage the backporting of commit 69562eb0bd3e
> > > > ("fanotify: disallow mount/sb marks on kernel internal pseudo fs") to
> > > > all stable kernels.
>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
>
> > > > Petr,
>
> > > > This tests for a behavior that we consider broken since the dawn of
> > > > fanotify.
>
> > > > The fix was merged to v6.5-rc1.
> > > > I've already posted backport patches for kernels > v5.0.
> > > > I am not planning to post backport patches for older kernels.
>
> > > I see
> > > https://lore.kernel.org/stable/20230710133205.1154168-1-amir73il@gmail.com/
>
> > > I'll suggest to wait till Greg releases the backport (should be quick enough).
>
>
> > ok.
>
> > > > Even though the two new test cases do not use FAN_REPORT_FID,
> > > > fanotify14 requires FAN_REPORT_FID, so it is not going to run these
> > > > test cases on kernel < v5.1 anyway.
>
> > > > Thanks,
> > > > Amir.
>
> > > >  .../kernel/syscalls/fanotify/fanotify14.c     | 32 +++++++++++++++++--
> > > >  1 file changed, 30 insertions(+), 2 deletions(-)
>
> > > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
> > > > index bfa0349fe..063a9f96f 100644
> > > > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c
> > > > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
> > > > @@ -19,6 +19,9 @@
> > > >   *
> > > >   *     ceaf69f8eadc fanotify: do not allow setting dirent events in mask of non-dir
> > > >   *     8698e3bab4dd fanotify: refine the validation checks on non-dir inode mask
> > > > + *
> > > > + * The pipes test cases are regression tests for commit:
> > > > + *     69562eb0bd3e fanotify: disallow mount/sb marks on kernel internal pseudo fs
> > > >   */
>
> > > >  #define _GNU_SOURCE
> > > > @@ -40,6 +43,7 @@
>
> > > >  #define FLAGS_DESC(flags) {(flags), (#flags)}
>
> > > > +static int pipes[2] = {-1, -1};
> > > >  static int fanotify_fd;
> > > >  static int fan_report_target_fid_unsupported;
> > > >  static int ignore_mark_unsupported;
> > > > @@ -60,6 +64,7 @@ static struct test_case_t {
> > > >       /* when mask.flags == 0, fanotify_init() is expected to fail */
> > > >       struct test_case_flags_t mask;
> > > >       int expected_errno;
> > > > +     int *pfd;
>
> > > This produces warnings:
> > > fanotify14.c:70:9: warning: missing initializer for field ‘pfd’ of ‘struct test_case_t’ [-Wmissing-field-initializers]
> > >    70 |         {FLAGS_DESC(FAN_CLASS_CONTENT | FAN_REPORT_FID), {}, {}, EINVAL},
> > >       |         ^
> > > fanotify14.c:67:14: note: ‘pfd’ declared here
> > >    67 |         int *pfd;
> > >       |              ^~~
> > > fanotify14.c:73:9: warning: missing initializer for field ‘pfd’ of ‘struct test_case_t’ [-Wmissing-field-initializers]
> > >    73 |         {FLAGS_DESC(FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID), {}, {}, EINVAL},
> > >       |         ^
> > > fanotify14.c:67:14: note: ‘pfd’ declared here
> > >    67 |         int *pfd;
> > >       |              ^~~
>
> > > Could you please fix them? I guess pfd must be NULL when unused.
>
>
> > ok. but I have to ask,
> > what is the value of explicitly initializing all the old test cases to
> > pfd = NULL?
> > and what is wrong with default NULL initializers?
> > Is it a deliberate decision of LTP to care about this warning?
> > it's a classic pattern for what this patch does -
> > add a new field to test case which all the existing test cases
> > should not care about.
>
> Well, we try to avoid warnings in new API tests (and rewriting legacy API tests
> into new API to cleanup the code).
>
> The solution to avoid warnings is to use designated initializers (named
> initializers), the same way as in ede7f095e ("fanotify10: Use named
> initializers"):
>
>         /* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
>         {
>                 .init = FLAGS_DESC(FAN_CLASS_CONTENT | FAN_REPORT_FID),
>                 .expected_errno = EINVAL
>         },
>
>         /* FAN_REPORT_FID without class FAN_CLASS_NOTIF is not valid */
>         {
>                 .init = FLAGS_DESC(FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID),
>                 .expected_errno = EINVAL
>         },
>
>         ...
>         {
>                 .init = FLAGS_DESC(FAN_CLASS_NOTIF),
>                 .mark = FLAGS_DESC(FAN_MARK_FILESYSTEM),
>                 .mask = { FAN_ACCESS, "anonymous pipe"},
>                 .expected_errno = EINVAL,
>                 .pfd = pipes
>         },
>
> The last one could be without designated initializers, because we pass all
> struct members, but IMHO it's better for readability.
>
> Therefore ideal solution would be to turn the test into designated initializers
> in separate commit, followed by this change.
>

Ah yes, I remember now. Will do that.

> > Also, I have always seen these warnings for struct tst_test.
>
> > fanotify14.c:284:1: warning: missing initializer for field
> > 'needs_cmds' of 'struct tst_test' [-Wmissing-field-initializers]
> >   284 | };
> >       | ^
> > In file included from fanotify14.c:28:
> > ../../../../include/tst_test.h:324:21: note: 'needs_cmds' declared here
> >   324 |  const char *const *needs_cmds;
> >       |                     ^~~~~~~~~~
>
> These warnings were caused by these GCC bugs (fixed in gcc 12 and backported to
> gcc 11):
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84685
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82283
>

Good to know.
Thanks!

Amir.