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

Petr Vorel
2023-07-11T08:05:39Z

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