Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Oct 3, 2018 at 6:40 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 02-10-18 13:37:13, Amir Goldstein wrote:
[...]
> > I am in fact in leaning to the former (as Mathew implemented it), because
> > I am looking at inotify and my effort to add the "dentry" events to fanotify.
> > First, my proposal suggests to report the optional event flag FAN_ONDIR,
> > just like inotify does.
>
> Well, we already do deliver FAN_ONDIR event flag if the event was on
> directory AFAIK. Just with fanotify you also have to explicitely ask for
> events on directories to be delivered by setting FAN_ONDIR in the mark's
> mask.
>

We actually mask it in out fanotify, so in inotify, it is out-only and
in fanotify, it is
in-only. BTW, I could not help cleaning up that horrible FAN_MARK_ONDIR
and it won us a very nice optimization of directory access events.
patches to follow soon.


>
> > How badly can a program be written that it opts into EXEC/ONDIR events
> > in fanotify_init() and doesn't request them in fanotify_mark() and it flips
> > when those "optional" flags are reported?
> > Assuming we also properly document that behavior.
>
> Yeah, so I'm not so concerned about an applicating getting surprised by
> additional event being set when it in fact explicitely asked for it. I'm
> more concerned about the "ease to understand the interface and use it
> correctly". I.e., the logic of interface design. And in this area, just
> defining new FAN_OPEN_EXEC event like any other seems to win? No need for
> special fanotify_init() flags and explanations in the manpage.
>

Ah! yes, that would be better.

> > BTW, as far as I understand the current man page, I did not find any explicit
> > statement that says that you CANNOT get an event if you did not ask for it.
> > FWIW, inotify and fanotify man pages are quite similar, so it may infer that
> > fanotify inherits the same expectations as one had from inotify.
> >
> > Having said all that, I'd like to clarify that I do not object to "new
> > event type",
> > I understand why you find it "cleaner".
> > I just find it less "efficient", because it adds extra calls to
> > fsnotify() for what
> > IMO is not a good enough reason.
>
> I'm not sure I understand your concern here. Are you concerned that
> fsnotify_open() would need to do one call for FS_OPEN event and one call
> for FS_OPEN_EXEC so that we won't "leak" FS_OPEN_EXEC event if user didn't
> ask for it?

Yes.

> If that's your concern, what if we just masked out all
> "unwanted" events in fanotify_handle_event()? fanotify_should_send_event()
> does all the masking anyway so it's not like we'd loose any performance
> with that and with current set of fanotify events it would be completely
> transparent.
>

I though about this first, but got myself confused thinking it would be messy.
Now I am looking again and don't understand why.

I will try to sum up the solution for us and for Mathew:
- No FAN_ENABLE_EXEC (sorry for that detour)
- Implementation in fsnotify_open() is exactly as Mathew did it, but
changing the
  name of the flag to FS_OPEN_EXEC
- Add FAN_OPEN_EXEC to valid user events mask and valid outgoing events
- fanotify_should_send_event() returns the mask  to be reported in the event
-- s/return false/return 0/
-- return event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask &
                                 ~marks_ignored_mask;

So we won't report events that user did not set a mask for and we won't report
events that user has set an ignore mask for.

Thanks,
Amir.