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

[Jan Kara <jack@suse.cz>]

Hi Matthew,

On Sun 07-10-18 22:13:22, Matthew Bobrowski wrote:
> On 4/10/18 2:33 am, Jan Kara wrote:
> > On Wed 03-10-18 19:18:27, Amir Goldstein wrote:
> >> 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.
> > 
> > OK, didn't notice that. Thanks for educating me.
> > 
> >> 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.
> > 
> > Cool! Less work for me as I also had tingling in my fingers to clean up
> > that mess, just didn't get to it yet :).
> > 
> >>> 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.
> > 
> > Exactly. Just I'd do the change to fanotify_should_send_event() as a
> > separate patch and rename that function to something like
> > fanotify_group_event_mask() or something like that to better express what
> > it will do.
> 
> I've gone ahead and implemented the changes based on what we agreed above.
> This first commit introduces the new event type FAN_OPEN_EXEC, as well as
> adds the necessary check within the fsnotify_open() hook. You can find this
> change here:
> 
> https://github.com/matthewbobrowski/linux/commit/2709f54a84047dc08fdc0bf3e8407a5275440c25
> 
> The second commit contains the necessary changes to
> fanotify_should_send_event(). This function basically now returns the event
> mask for the event containing flags ONLY set to those requested by the
> user. You can find this change here:
> 
> https://github.com/matthewbobrowski/linux/commit/596c531365229d439e374149f5ba62011008bb0c
> 
> Please provide feedback on the above.
> 
> Also, we haven't really discussed how we can incorporate this within
> fsnotify_perm(). However, I was thinking that we can simply do something
> along the lines of what I've done here (defining another flag
> FAN_OPEN_EXEC_PERM):
> 
> https://github.com/matthewbobrowski/linux/commit/664ba02fdb871d5e8cbf7cf59c592dd89a597a67
> 
> Thoughts?

>From a quick glance patches look OK to me but please post them normally via
email. That way it's easier for me to review and comment on various small
things in individual patches. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR