From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f179.google.com ([209.85.215.179]:34899 "EHLO mail-pg1-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726120AbeKGGHM (ORCPT ); Wed, 7 Nov 2018 01:07:12 -0500 Received: by mail-pg1-f179.google.com with SMTP id 32-v6so6335303pgu.2 for ; Tue, 06 Nov 2018 12:40:10 -0800 (PST) Date: Wed, 7 Nov 2018 07:40:00 +1100 From: Matthew Bobrowski To: Amir Goldstein Cc: Jan Kara , linux-fsdevel Subject: Re: FAN_OPEN_EXEC event and ignore mask Message-ID: <20181106203952.GA1726@development.internal.lab> References: <20181030002744.GA4214@workstation> <20181031103939.GA8325@development.internal.lab> <20181102125000.GB15086@quack2.suse.cz> <20181103003411.GA1314@development.internal.lab> <20181105084147.GC6953@quack2.suse.cz> <20181106130837.GA2206@development.internal.lab> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Nov 06, 2018 at 03:45:43PM +0200, Amir Goldstein wrote: > On Tue, Nov 6, 2018 at 3:08 PM Matthew Bobrowski > wrote: > > > > 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. > > > > IDGI. What is the problem with: > > if (mask & MAY_OPEN) { > fsnotify_mask = FS_OPEN_PERM; > if (file->f_flags & __FMODE_EXEC) { > ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM); > if (ret) return ret; > } > } else if (mask & MAY_READ) { > fsnotify_mask = FS_ACCESS_PERM; > } > > return fsnotify_path(inode, path, FS_OPEN_EXEC_PERM); > > You can consolidate all 5 calls to fsnotify_parent();fsnotify() of the same > pattern to fsnotify_path(). There is nothing wrong with this and what this does in fact simplifies the call site for fsnotify_parent()/fsnotify(), which is very nice and clean in my opinion. What I'm referring to though is different. All I'm saying is that if I was a user and I wanted to capture each time a file was opened regardless whether it was for execution, for read, for write, I'd expect to capture these events by just registering for FAN_OPEN_PERM and it would be sufficient. After applying these updates, for a user to capture *all* open related events, they're going to have to now supply both FAN_OPEN_PERM and FAN_OPEN_EXEC_PERM. I just don't want to be in a position where we've completely changed the expectation of FAN_OPEN_PERM, as I can imagine this would really frustrate people. Maybe I'm over thinking it and it's OK? -- Matthew Bobrowski