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

[Amir Goldstein <amir73il@gmail.com>]

On Sun, Oct 7, 2018 at 2:13 PM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> Hi Jan/Amir,
>
> 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.

Generally looks good, please post as proper path series.
Comments on github version:
commit message for fanotify_should_send_event() is vague because it does
not name the use case of 2 events that are bundled together and does not
specify that this is a new use case introduced by the new event FAN_OPEN_EXEC
that is bundled together (in reporting) with FAN_OPEN.
I think if you try to explain the specific use case and not the abstract case,
result will be more understandable.

I probably said it before, but since we are way beyond initial RFC I
will repeat.
When posting an API change:
- Please CC linux-api
- Please prepare a man-pages patch and reference it (github?) from
your cover letter
- The sooner you write and LTP test to cover the new API the better
- A reference to a test in cover letter to say "this is how I tested"
is very nice to
  have, but you can also follow up on that later (assuming that you
did test somehow...)


>
> 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?
>

Seems reasonable to me. I think you should include this patch in the
series you post,
Jan can always choose to take only part of the series.

As I wrote in github comment, some lines are just too ugly to live,
even if you didn't
write them, so if you touch them and it is not a great effort, you
better fix them.
The BUG() statement in fsnotify_perm() simply cannot live.
Even the compiler should know this line cannot be reached, so it
should be removed.

Thanks,
Amir.