[LTP] [PATCH v3 3/5] fanotify: Introduce SAFE_FANOTIFY_MARK() macro

[Amir Goldstein]

On Wed, Nov 25, 2020 at 10:25 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Amir,
>
> > On Wed, Nov 25, 2020 at 8:24 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > Hi Amir,
>
> > > > > > > +++ b/testcases/kernel/syscalls/fanotify/fanotify01.c
> > > > > > > @@ -101,19 +101,8 @@ static void test_fanotify(unsigned int n)
> > > > > > >                       "failed", tc->init_flags);
> > > > > > >       }
>
> > > > > > > -     if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> > > > > > > -                       FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
> > > > > > > -                       AT_FDCWD, fname) < 0) {
> > > > > > > -             if (errno == EINVAL && mark->flag == FAN_MARK_FILESYSTEM) {
> > > > > > > -                     tst_res(TCONF,
> > > > > > > -                             "FAN_MARK_FILESYSTEM not supported in kernel?");
> > > > > > > -                     return;
> > > > > > > -             }
>
> > > > > > Here we had tst_res(TCONF, ...) followed by a return but we will can
> > > > > > tst_brk() after the change. I guess that we may skip part of the test on
> > > > > > older kernels with that change.
>
>
> > > > > That's not good. I missed that in my review.
> > > > > There are many tests where only the FAN_MARK_FILESYSTEM
> > > > > test cases are expected to result in TCONF, but the rest of the test
> > > > > cases should run.
>
> > > > I'm not sure if I understand you. Is my approach correct here?
> > > OK, I got that, I cannot use SAFE_FANOTIFY_MARK() in test_fanotify() in fanotify01.c
> > > and in setup_marks() in fanotify13.c.
>
> > I gave fanotify01 as an example.
> > There are many such cases, like fanotify03.
>
> > The point is we cannot replace tst_res() with tst_brk() when only some of the
> > test cases may be supported.
>
> Sure, I'll check in all tests that tst_res() won't be replaced with tst_brk().
>
> > > But FAN_REPORT_FID in is on both files already checked after fanotify_init()
> > > call. Not sure if it must be check also for fanotify_mark(), because it's
> > > only in FANOTIFY_INIT_FLAGS (via FANOTIFY_FID_BITS). FANOTIFY_MARK_FLAGS has
> > > other flags.
>
> > > If yes, I'll probably need to create fanotify_supported_by_kernel(...), which
> > > check for all not supported flags and will be used in those 2 places and in
> > > safe_fanotify_init(). Something like this:
>
> > > typedef void (*tst_res_func_t)(const char *file, const int lineno,
> > >                 int ttype, const char *fmt, ...);
>
> > > int fanotify_flags_supported_by_kernel(const char *file, const int lineno,
> > >         unsigned int flags, int strict)
> > > {
> > >         tst_res_func_t res_func = tst_res_;
> > >         int unsupported = 0;
>
> > >         if (strict)
> > >                 res_func = tst_brk_;
>
> > >         if (errno == EINVAL) {
> > >                 if (flags & FAN_REPORT_TID) {
> > >                         tst_res_(file, lineno, TINFO,
> > >                                          "FAN_REPORT_TID not supported by kernel?");
> > >                         unsupported = 1;
> > >                 }
>
> > >                 if (flags & FAN_REPORT_FID) {
> > >                         tst_res_(file, lineno, TINFO,
> > >                                          "FAN_REPORT_FID not supported by kernel?");
> > >                         unsupported = 1;
> > >                 }
>
> > >                 if (flags & FAN_REPORT_DIR_FID) {
> > >                         tst_res_(file, lineno, TINFO,
> > >                                          "FAN_REPORT_DIR_FID not supported by kernel?");
> > >                         unsupported = 1;
> > >                 }
>
> > >                 if (flags & FAN_REPORT_NAME) {
> > >                         tst_res_(file, lineno, TINFO,
> > >                                          "FAN_REPORT_NAME not supported by kernel?");
> > >                         unsupported = 1;
> > >                 }
>
> > >                 if (unsupported)
> > >                         res_func(file, lineno, TCONF, "Unsupported configuration, see above");
> > >                 else
> > >                         tst_brk_(file, lineno, TBROK, "Unknown failure");
>
> > >                 return -1;
> > >         }
>
> > >         return 0;
> > > }
>
>
> > That seems too much and adds more noise than valuable info in many cases
> > or maybe I didn't understand.
>
> > > These are flags for fanotify_init(). Flags for fanotify_mark() are currently
> > > handled by fanotify_exec_events_supported_by_kernel() (used for FAN_OPEN_EXEC
> > > and FAN_OPEN_EXEC_PERM), using different approach. Testing fanotify_mark() flags
> > > support in advance in setup makes tests faster, I'm just not happy we use
> > > different approach. Any tip for improving this or improving readability is
> > > welcome.
>
>
> > I think the best would be to always test in advance like exec events,
> > for FAN_REPORT_ fanotify_init() flags and FAN_MARK_FILESYSTEM
> > fanotify_mark() flag whenever relevant.
>
> > I didn't go over all tests to see how that would look, but I have a feeling
> > that would end up being the cleanest approach.
>
> > Thanks,
> > Amir.
>
> OK, I'll have a look whether FAN_REPORT_* will be easy to transform to checks in
> advance.
>

Be aware.
For FAN_REPORT_FID, FAN_REPORT_FID (fanotify_init) and
FAN_MARK_FILESYSTEM (fanotify_mark) support can be tested without
any other flags, but FAN_REPORT_NAME is not valid by itself.
Instead, you can check in advance support for the flag combination
FAN_REPORT_DFID_NAME and you may also change the TCONF
warning accordingly.
There is no kernel where only one of the two flags is supported.

> I'll also try to wrote some automatic detection for testcases which use
> struct tcase (looping the struct and collect flags with & and pass it to some
> function). Maybe too complicated having to declare what is required to check
> is something which is IMHO error prone (we probably forget to update what is
> needed to be checked when we add/remove/change test structs).
>

IMHO this is overengineering.
Your cleanup is a huge improvement and big enough as is.
Please try to get it merged first without any further cleanups.
There is already a fix in upstream and stable kernel ("fanotify: fix logic of
reporting name info with watched parent") for which I have posted a test
but waiting for your cleanups to land before re-posting.

Thanks,
Amir.