Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211029211732.386127-1-krisman@collabora.com>
 <20211029211732.386127-3-krisman@collabora.com> <YYEoAr743j3IO3ol@google.com>
In-Reply-To: <YYEoAr743j3IO3ol@google.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 2 Nov 2021 14:11:47 +0200
Message-ID: <CAOQ4uxiSKNcxWBu+MdhOzhYWmLC9Aj2zoquUhTVn0q2x2SbxCw@mail.gmail.com>
Subject: Re: [PATCH v3 2/9] syscalls: fanotify: Add macro to require specific events
To:     Matthew Bobrowski <repnop@google.com>
Cc:     Gabriel Krisman Bertazi <krisman@collabora.com>,
        Jan Kara <jack@suse.com>, LTP List <ltp@lists.linux.it>,
        Khazhismel Kumykov <khazhy@google.com>, kernel@collabora.com,
        Ext4 <linux-ext4@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 2, 2021 at 1:59 PM Matthew Bobrowski <repnop@google.com> wrote:
>
> On Fri, Oct 29, 2021 at 06:17:25PM -0300, Gabriel Krisman Bertazi wrote:
> > Add a helper for tests to fail if an event is not available in the
> > kernel.  Since some events only work with REPORT_FID or a specific
> > class, update the verifier to allow those to be specified.
> >
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>
> Made a single comment, otherwise this looks OK to me.
>
> Reviewed-by: Matthew Bobrowski <repnop@google.com>
>
> > ---
> > Changes since v1:
> >   - Use SAFE_FANOTIFY_INIT instead of open coding. (Amir)
> >   - Use FAN_CLASS_NOTIF for fanotify12 testcase. (Amir)
> > ---
> >  testcases/kernel/syscalls/fanotify/fanotify.h   | 17 ++++++++++++++---
> >  testcases/kernel/syscalls/fanotify/fanotify03.c |  4 ++--
> >  testcases/kernel/syscalls/fanotify/fanotify10.c |  3 ++-
> >  testcases/kernel/syscalls/fanotify/fanotify12.c |  3 ++-
> >  4 files changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
> > index c67db3117e29..820073709571 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> > @@ -266,14 +266,16 @@ static inline void require_fanotify_access_permissions_supported_by_kernel(void)
> >       SAFE_CLOSE(fd);
> >  }
> >
> > -static inline int fanotify_events_supported_by_kernel(uint64_t mask)
> > +static inline int fanotify_events_supported_by_kernel(uint64_t mask,
> > +                                                   unsigned int init_flags,
> > +                                                   unsigned int mark_flags)
> >  {
> >       int fd;
> >       int rval = 0;
> >
> > -     fd = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
> > +     fd = SAFE_FANOTIFY_INIT(init_flags, O_RDONLY);
> >
> > -     if (fanotify_mark(fd, FAN_MARK_ADD, mask, AT_FDCWD, ".") < 0) {
> > +     if (fanotify_mark(fd, FAN_MARK_ADD | mark_flags, mask, AT_FDCWD, ".") < 0) {
> >               if (errno == EINVAL) {
> >                       rval = -1;
> >               } else {
> > @@ -378,4 +380,13 @@ static inline int fanotify_mark_supported_by_kernel(uint64_t flag)
> >                                   fanotify_mark_supported_by_kernel(mark_type)); \
> >  } while (0)
> >
> > +#define REQUIRE_FANOTIFY_EVENTS_SUPPORTED_ON_FS(init_flags, mark_type, mask, fname) do { \
> > +     if (mark_type)                                                  \
> > +             REQUIRE_MARK_TYPE_SUPPORTED_ON_KERNEL(mark_type);       \
> > +     if (init_flags)                                                 \
> > +             REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_ON_FS(init_flags, fname); \
> > +     fanotify_init_flags_err_msg(#mask, __FILE__, __LINE__, tst_brk_, \
> > +             fanotify_events_supported_by_kernel(mask, init_flags, mark_type)); \
> > +} while (0)
> > +
> >  #endif /* __FANOTIFY_H__ */
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > index 26d17e64d1f5..2081f0bd1b57 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify03.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > @@ -323,8 +323,8 @@ static void setup(void)
> >       require_fanotify_access_permissions_supported_by_kernel();
> >
> >       filesystem_mark_unsupported = fanotify_mark_supported_by_kernel(FAN_MARK_FILESYSTEM);
> > -     exec_events_unsupported = fanotify_events_supported_by_kernel(FAN_OPEN_EXEC_PERM);
> > -
> > +     exec_events_unsupported = fanotify_events_supported_by_kernel(FAN_OPEN_EXEC_PERM,
> > +                                                                   FAN_CLASS_CONTENT, 0);
> >       sprintf(fname, MOUNT_PATH"/fname_%d", getpid());
> >       SAFE_FILE_PRINTF(fname, "1");
> >
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
> > index 92e4d3ff3054..0fa9d1f4f7e4 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify10.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
> > @@ -509,7 +509,8 @@ cleanup:
> >
> >  static void setup(void)
> >  {
> > -     exec_events_unsupported = fanotify_events_supported_by_kernel(FAN_OPEN_EXEC);
> > +     exec_events_unsupported = fanotify_events_supported_by_kernel(FAN_OPEN_EXEC,
> > +                                                                   FAN_CLASS_CONTENT, 0);
>
> I'm wondering whether this is the best combination of mask and
> init_flags to use in this particular case? Maybe not to confuse future
> readers, using FAN_CLASS_NOTIF explicitly here would be better, WDYT?
> It doesn't make a difference, but it's something that caught my eye
> while parsing this patch.
>

Wow.
I think you are right in that this test does not use the combination
FAN_OPEN_EXEC with FAN_CLASS_CONTENT group, but it is quite hard
figure this out.

It will also be quite hard to figure out if this ever changes if ever new
test cases are added, so it will be hard to catch a change like that in review.
Given all that I would not change the requirement.

Thanks,
Amir.