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

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

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.

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?

-- 

Kind regards,
Matthew Bobrowski