Hi Amir, On 28/09/18 15:39, Amir Goldstein wrote: > On Fri, Sep 28, 2018 at 4:28 AM Matthew Bobrowski > <mbobrowski@xxxxxxxxxxxxxx> wrote: >> >> Hi Amir, >> >> On 27/9/18 11:57 pm, Amir Goldstein wrote: >>> [cc: linux-api] >>> >>> On Thu, Sep 27, 2018 at 4:05 PM Matthew Bobrowski >>> <mbobrowski@xxxxxxxxxxxxxx> wrote: >>>> >>>> This is a reduced version of a patch that I originally submitted a while ago. >>>> >>>> In short, the fanotify API currently does not provide any means for user space >>>> programs to receive events specifically when a file has been opened with the >>>> intent to be executed. The FAN_EXEC flag will be set within the event mask when >>>> a object has been opened with one of the open flags being __FMODE_EXEC. >>>> > ... >>>> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h >>>> index fd1ce10553bf..aad174c81322 100644 >>>> --- a/include/linux/fsnotify.h >>>> +++ b/include/linux/fsnotify.h >>>> @@ -216,6 +216,9 @@ static inline void fsnotify_open(struct file *file) >>>> if (S_ISDIR(inode->i_mode)) >>>> mask |= FS_ISDIR; >>>> >>>> + if (file->f_flags & __FMODE_EXEC) >>>> + mask |= FS_EXEC; >>>> + >>> >>> I think that may be breaking existing programs that do not expect to see >>> this bit in the event mask (i.e. if they only requested to see FAN_OPEN). >> >> A very good point and is definitely something that did cross my mind while >> writing this patch. >> > > My only issue with my own suggestion is that this implicit behavior is harder > to document than the explicit behavior (i.e. "user will get FAN_EXEC only if > user sets fanotify_init flag FAN_ENABLE_EXEC"), so I haven't decided which > I prefer yet - waiting for Jan to weight in on this point. > > The concept of "bonus flags" (flags that you got but did not ask for) is not > new to inotify (I think you can get IN_ATTRIB if you asked for IN_MODIFY), > but AFAIKT, it would be new to fanotify. > > OTOH, getting an event that you asked for in the past and since then > removed the event bit from the mark is not a new behavior. Although this > is not the same as the implicit global enable flag I proposed. > > The more I write about it, the more I am leaning towards explicit enable... Hm, I can't think of a good justification as to why something that provides this type of behavior can't be an explicit enable, and why we'd go for an implicit enable instead? I really like the idea of having an initialization flag (i.e. FAN_ENABLE_EXECUTE) that controls whether your program is to receive events that may potentially contain additional "bonus" flags (i.e. FAN_EXECUTE), or not. The intent of having a flag of this nature is clear, it makes sense, and would be a good way to potentially enable/disable any API features moving forward. >>> A possible mitigation would be to set a group flag FAN_ENABLE_EXEC >>> on the first time that user requests the FAN_EXEC event and then >>> compute the FAN_ALL_OUTGOING_EVENTS at runtime based on that >>> group flag (see example with FAN_ONDIR in my dev branch [1]). >>> Unlike my example, I don't think you have to expose the init flag >>> FAN_ENABLE_EXEC to users, because it can be set implicitly on the >>> first FAN_EXEC mark request. >> >> Ah yes, I think this is quite elegant and could actually work well. Let me >> review your suggestions and write an alternative patch using this >> particular approach. >> > > If Jan accepts my proposal, you can base your patch on: > https://github.com/amir73il/linux/commits/fanotify_api-v3 Ok, great. I've gone ahead and written an updated patch that adds the ability for programs to explicitly enable the receiving of events that contain the "FAN_EXECUTE" bonus flag. Programs will have to provide "FAN_ENABLE_EXECUTE" as one of their initialization flags when calling fanotify_init() in order to receive any events where the "FAN_EXECUTE" flag has been set. I think doing it this way would be a far better solution to mitigate possibly breaking any existing programs. You can find the patch here: https://github.com/matthewbobrowski/linux/commit/3325b3bbd8613b0de1dd6bac24b52c481b760f1d These changes are based off your branch 'fanotify_api-v3'. Let me know what you think. -- Kind regards, Matthew Bobrowski
