Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Tue, Nov 06, 2018 at 03:45:43PM +0200, Amir Goldstein wrote:
> On Tue, Nov 6, 2018 at 3:08 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > On Mon, Nov 05, 2018 at 09:41:47AM +0100, Jan Kara wrote:
> > > On Sat 03-11-18 11:34:13, Matthew Bobrowski wrote:
> > > > On Fri, Nov 02, 2018 at 01:50:00PM +0100, Jan Kara wrote:
> > > > > On Thu 01-11-18 16:45:47, Amir Goldstein wrote:
> > > > > > Permission events cannot
> > > > > > be merged, but man page doesn't say anything about that.
> > > > > > It might be worth dropping a note about OPEN_EXEC_PERM
> > > > > > that it could be expected to appear together in the same permission
> > > > > > event with OPEN_PERM and user response will apply to both.
> > > > >
> > > > > Umm, I'd actually prefer if the OPEN_PERM and OPEN_EXEC_PERM events didn't
> > > > > get merged. The overhead is just an additional call to fsnotify() to find
> > > > > out one of the events is uninteresting (realistically, 99% of users will be
> > > > > looking OPEN_PERM or OPEN_EXEC_PERM but not both) and it just keeps things
> > > > > simple in the API. I understand that it may seem somewhat unexpected that
> > > > > single file open will generate two different fsnotify permission events
> > > > > (again 99% users won't observe this anyway) but if we start "merging"
> > > > > permission events I think we open more space for confusion - like when
> > > > > event arrives with some bits trimmed due to ignore mask masking bits out or
> > > > > what not. What do you think Amir?
> > > >
> > > > This is something that I was going to bring up in my response yesterday,
> > > > however I wasn't sure how you guys would take it. In my opinion, I think
> > > > if we did merge the two open permission events then it would be
> > > > contradicting with all the existing comments and code related to the
> > > > permission events that we have scattered around the API. Thus, I'm in
> > > > favour of adding the additional fsnotify()/fsnotify_parent() calls to
> > > > minimise any potential confusion in regards to permission events being
> > > > merged moving forward.
> > >
> > > Yes, so please update your patch adding FAN_OPEN_EXEC_PERM to send this
> > > event separately from FAN_OPEN_PERM. Thanks!
> >
> > Hm, I was thinking about this a little further just before sending through
> > the updated patch series.
> >
> > If we include additional calls to fsnotify_parent()/fsnotify() when
> > file->f_flags & __FMODE_EXEC with just the FS_OPEN_EXEC_PERM flag set,
> > then this may almost certainly cause unnecessary confusion from an API
> > consumer perspective.
> >
> > Think of the situation where the user asks for FAN_OPEN_PERM and is
> > working with the assumption that this _should_ cover any given operation
> > being performed on a file, ever. If they register for FAN_OPEN_PERM and an
> > execve() occurs on the marked object, then they won't end up receiving the
> > event despite it fundamentally being an open(). To cover this case, we're
> > forcing the user to also register for FAN_OPEN_EXEC_PERM in order to
> > receive events when a file has been opened for execution. I don't want to
> > be misleading a users understanding of FAN_OPEN_PERM, but I'm also not
> > sure whether there is any other way around this if we're wanting to keep
> > permission events separate. This is probably something that we'll face
> > with each permission sub-type moving forward i.e. FAN_OPEN_WRITE_PERM, as
> > Amir previously mentioned.
> >
> > We can of course add these caveats within the documentation which cover
> > all these different semantics. But, I also don't want to get to a stage
> > where we're detailing all these little "gotchas", because we all know what
> > that means.
> >
> > I just wanted to make sure that we're all OK with what I've mentioned
> > above.
> >
> 
> IDGI. What is the problem with:
> 
>        if (mask & MAY_OPEN) {
>                 fsnotify_mask = FS_OPEN_PERM;
>                 if (file->f_flags & __FMODE_EXEC) {
>                        ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
>                        if (ret) return ret;
>                 }
>        } else if (mask & MAY_READ) {
>                 fsnotify_mask = FS_ACCESS_PERM;
>        }
> 
>        return fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
> 
> You can consolidate all 5 calls to fsnotify_parent();fsnotify() of the same
> pattern to fsnotify_path().

There is nothing wrong with this and what this does in fact simplifies
the call site for fsnotify_parent()/fsnotify(), which is very nice and
clean in my opinion.

What I'm referring to though is different. All I'm saying is that if I was
a user and I wanted to capture each time a file was opened regardless
whether it was for execution, for read, for write, I'd expect to capture
these events by just registering for FAN_OPEN_PERM and it would be
sufficient. After applying these updates, for a user to capture *all* open
related events, they're going to have to now supply both FAN_OPEN_PERM and
FAN_OPEN_EXEC_PERM. I just don't want to be in a position where we've
completely changed the expectation of FAN_OPEN_PERM, as I can imagine this
would really frustrate people.

Maybe I'm over thinking it and it's OK?

-- 
Matthew Bobrowski <mbobrowski@mbobrowski.org>