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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
