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

[Petr Vorel <pvorel@suse.cz>]

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

> 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

Kind regards,
Petr

> Must we really initialize an empty needs_cmds array for every test?
> Seems pointless to me, but I may not have the bigger picture.

> Thanks,
> Amir.

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