Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Tue, Oct 30, 2018 at 11:17:06AM +0200, Amir Goldstein wrote:
> [Change the subject and add fsdevel to CC]
> 
> On Tue, Oct 30, 2018 at 2:27 AM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> [...]
> > Then I concluded that it doesn't have to be event mask specific i.e.
> > FAN_OPEN_EXEC. Irrespective of what the event_mask value actually is, if
> > it contains a flag that has also been set within marks_ignored_mask, then
> > the event should *not* be sent through to the user:
> >
> > if (event_mask & marks_ignored_mask)
> >         return 0;
> >
> > I think what Amir has proposed here is also correct and the cleanest
> > implementation to achieve what we want.
> >
> 
> My proposal had a bug w.r.t handling of FS_EVENT_ON_CHILD
> in ignore mask. I fixed it and pushed to branch:
> https://github.com/amir73il/linux/commits/fanotify_ignore
> In the first commit ("fanotify: fix handling of FS_EVENT_ON_CHILD
> sub-directory events")
> I have made a claim about an existing bug -
> that claim still needs to be proven by a test case.
> it could be there is no bug and this is just an optimization.

Personally, I think it's addressing behaviour that was not previously
anticipated or accounted for. That being said, I'm in favour of this
change.

> 
> Man page should be revised to clarify the currently expected behavior:
> FAN_EVENT_ON_CHILD ...
>   The flag has no effect when marking mounts
> + or filesystems and has no effect when set in ignore mask
> 
> Please include that change in your man page draft for new
> ignore mask interpretations.

OK. I've updated the man pages to include the clarification around the
revised handling of ignore mask. These can be found here:

https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore

Wasn't overly confident about where I've placed the explanations, but I
felt that's where they fitted best. I was also thinking that we could have
an example of a compound event to illustrate the functionality further? 

> 
> > >
> > > Nothing will need to be change for FS_OPEN_EXEC with this change
> > > applied to implement semantics #2 above.
> > > I only hope this change is correct... (it did pass existing and new ltp tests)
> > >
> >
> > I also tested this on my side the using existing and newly proposed LTP
> > test cases, all tests had passed. This change also covers the last test
> > that I defined in fanotify12 where the mark_mask = FAN_OPEN_EXEC and
> > ignore_mask = FAN_OPEN. Prior to this change, an event for FAN_OPEN_EXEC
> > would still slip through despite being a subtype of an open, which
> > shouldn't be the case. With this change implemented, NO events are sent
> > through, which is exactly what is expected.
> >
> > > >   Pros: FAN_OPEN_NOEXEC easy to do using ignore masks.
> > > >   Cons: Semantics is IMO somewhat difficult to explain in the manpage.
> > > >         Probably we'd need to really explain that FAN_OPEN is really a
> > > >         compound of FS_OPEN_NOEXEC and FS_OPEN_EXEC.
> > > >
> > > > So overall I think the semantics from 2) is more useful. But I'd start with
> > > > manpage properly explaining the semantics and interactions between FAN_OPEN
> > > > and FAN_OPEN_EXEC. If that can be written so that user's head does not
> > > > spin, I think the implementation can be done in a reasonably simple way as
> > > > well. And I'm really sorry for leading you in circles Matthew...
> > > >
> > >
> > > Agree to the "man page criteria"
> >
> > This is fine. I can document the FAN_OPEN and FAN_OPEN_EXEC semantics
> > really well, whenever required. But, I'd like to agree upon the solution
> > and have that finalised before I shift my focus on documentation.
> >
> 
> The idea of "document first" is that if you cannot come up with simple man page
> we are not going to implement those semantics, because they will be unusable.
> 
> So I agree with Jan. Please take a swing at man page change.
> It can be a very rough draft, you can send a link to github, so long
> as we can see
> that a simple man page is "doable".

Roger that. Thanks for sharing the view point. I've supplied the link that
contains the refactored ignore mask explanation above. I think this is
what you meant when you wanted to see the documentation first, however if
I've completely misinterpreted something, please clarify.

-- 
Matthew Bobrowski <mbobrowski@mbobrowski.org>