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

[Petr Vorel <pvorel@suse.cz>]

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

Thanks a lot!

Kind regards,
Petr

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp