On Mon, Nov 05, 2018 at 09:41:47AM +0100, Jan Kara wrote: > On Sat 03-11-18 11:34:13, Matthew Bobrowski wrote: > > On Fri, Nov 02, 2018 at 01:50:00PM +0100, Jan Kara wrote: > > > On Thu 01-11-18 16:45:47, Amir Goldstein wrote: > > > > Permission events cannot > > > > be merged, but man page doesn't say anything about that. > > > > It might be worth dropping a note about OPEN_EXEC_PERM > > > > that it could be expected to appear together in the same permission > > > > event with OPEN_PERM and user response will apply to both. > > > > > > Umm, I'd actually prefer if the OPEN_PERM and OPEN_EXEC_PERM events didn't > > > get merged. The overhead is just an additional call to fsnotify() to find > > > out one of the events is uninteresting (realistically, 99% of users will be > > > looking OPEN_PERM or OPEN_EXEC_PERM but not both) and it just keeps things > > > simple in the API. I understand that it may seem somewhat unexpected that > > > single file open will generate two different fsnotify permission events > > > (again 99% users won't observe this anyway) but if we start "merging" > > > permission events I think we open more space for confusion - like when > > > event arrives with some bits trimmed due to ignore mask masking bits out or > > > what not. What do you think Amir? > > > > This is something that I was going to bring up in my response yesterday, > > however I wasn't sure how you guys would take it. In my opinion, I think > > if we did merge the two open permission events then it would be > > contradicting with all the existing comments and code related to the > > permission events that we have scattered around the API. Thus, I'm in > > favour of adding the additional fsnotify()/fsnotify_parent() calls to > > minimise any potential confusion in regards to permission events being > > merged moving forward. > > Yes, so please update your patch adding FAN_OPEN_EXEC_PERM to send this > event separately from FAN_OPEN_PERM. Thanks! Hm, I was thinking about this a little further just before sending through the updated patch series. If we include additional calls to fsnotify_parent()/fsnotify() when file->f_flags & __FMODE_EXEC with just the FS_OPEN_EXEC_PERM flag set, then this may almost certainly cause unnecessary confusion from an API consumer perspective. Think of the situation where the user asks for FAN_OPEN_PERM and is working with the assumption that this _should_ cover any given operation being performed on a file, ever. If they register for FAN_OPEN_PERM and an execve() occurs on the marked object, then they won't end up receiving the event despite it fundamentally being an open(). To cover this case, we're forcing the user to also register for FAN_OPEN_EXEC_PERM in order to receive events when a file has been opened for execution. I don't want to be misleading a users understanding of FAN_OPEN_PERM, but I'm also not sure whether there is any other way around this if we're wanting to keep permission events separate. This is probably something that we'll face with each permission sub-type moving forward i.e. FAN_OPEN_WRITE_PERM, as Amir previously mentioned. We can of course add these caveats within the documentation which cover all these different semantics. But, I also don't want to get to a stage where we're detailing all these little "gotchas", because we all know what that means. I just wanted to make sure that we're all OK with what I've mentioned above. -- Matthew Bobrowski <mbobrowski@xxxxxxxxxxxxxx>
