Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Wed, Nov 14, 2018 at 01:02:47PM +0100, Jan Kara wrote:
> > > > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > > > that contains the test cases are provided below:
> > > > >
> > > > > syscalls/fanotify03: test cases have been updated to cover
> > > > >                      FAN_OPEN_EXEC_PERM events
> > > > > syscalls/fanotify12: newly introduced LTP test file to cover
> > > > >                      FAN_OPEN_EXEC events
> > > >
> > > > I have been wondering for a while why the testcases passed when ignore mask
> > > > hasn't been properly treated in fanotify_group_event_mask() but then I
> > > > realized that the generic code will not even call to fanotify if ignore
> > > > masks result in clearing the event.
> > >
> > > So does that means we have missing test coverage?
> > >
> > 
> > This is covered by test case #3 of Matthew's proposed LTP test.
> > https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76
> 
> This testcase does not catch the bug we had in fanotify_group_event_mask()
> because the masking by mark->mask already hides the fact that we failed to
> apply the ignore mask.
> 
> What does catch this kind of bug (tested) is a testcase (admittedly
> somewhat silly) like this:
> 
> {
>         "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC",
>         INIT_FANOTIFY_MARK_TYPE(INODE),
>         FAN_OPEN | FAN_OPEN_EXEC,
>         FAN_OPEN_EXEC,
>         2,
>         {FAN_OPEN, FAN_OPEN}
> },

I've incorporated this^ test as part of my test cases. All tests, this one
included, are passing on kernel built on your 'fsnotify' branch. You can find
the updated test case file here:

https://github.com/matthewbobrowski/ltp/commit/d1d57d5bda8db49a26624c7737c2db88ea90f9db

> A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore
> FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug
> we'd get FAN_OPEN | FAN_OPEN_EXEC.
> 
> But creating such test would be slightly more involved. But probably it is
> worth it. Matthew?

Yeah, this shouldn't be too difficult to add at all, but as Amir pointed out,
I'd probably be in favour of putting this into a different test case i.e. one
which deals with mounts/filesystem mark types.
 
> Also I have noticed that fanotify12 test has a bug that it reports:
> 
> fanotify12.c:220: FAIL: Received event: mask=1020, pid=5142 (expected 5142), fd=5
> 
> i.e., it reports expected pid instead of expected mask when mask does not
> match. Can you please fix it Matthew?

Sure, a fix for this has also been applied here:

https://github.com/matthewbobrowski/ltp/commit/d1d57d5bda8db49a26624c7737c2db88ea90f9db

-- 
Matthew Bobrowski