From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f194.google.com ([209.85.215.194]:40174 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726264AbeJGSUX (ORCPT ); Sun, 7 Oct 2018 14:20:23 -0400 Received: by mail-pg1-f194.google.com with SMTP id n31-v6so6592282pgm.7 for ; Sun, 07 Oct 2018 04:13:27 -0700 (PDT) Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC To: Jan Kara , Amir Goldstein Cc: Steve Grubb , linux-fsdevel References: <20181001105803.GG3913@quack2.suse.cz> <20181002092448.GA4135@quack2.suse.cz> <20181003154005.GD24030@quack2.suse.cz> <20181003163343.GJ24030@quack2.suse.cz> From: Matthew Bobrowski Message-ID: Date: Sun, 7 Oct 2018 22:13:22 +1100 MIME-Version: 1.0 In-Reply-To: <20181003163343.GJ24030@quack2.suse.cz> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 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