linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthew Bobrowski <mbobrowski@mbobrowski.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Steve Grubb <sgrubb@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC
Date: Mon, 1 Oct 2018 18:21:36 +1000	[thread overview]
Message-ID: <92b88900-c4be-7acf-b2f4-81d52eab1f5e@mbobrowski.org> (raw)
In-Reply-To: <CAOQ4uxh4wwmO0JWqY1ceJ2ApEYhQWS1rpK1hPiXGzLDxGJ2qMQ@mail.gmail.com>

Hi Amir,

On 28/09/18 15:39, Amir Goldstein wrote:
> On Fri, Sep 28, 2018 at 4:28 AM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> 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@mbobrowski.org> 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

  reply	other threads:[~2018-10-01 14:58 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-27 13:05 [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC Matthew Bobrowski
2018-09-27 13:57 ` Amir Goldstein
2018-09-28  1:27   ` Matthew Bobrowski
2018-09-28  5:39     ` Amir Goldstein
2018-10-01  8:21       ` Matthew Bobrowski [this message]
2018-10-01  9:13         ` Amir Goldstein
2018-10-01 10:58 ` Jan Kara
2018-10-01 14:01   ` Amir Goldstein
2018-10-02  9:24     ` Jan Kara
2018-10-02 10:37       ` Amir Goldstein
2018-10-03 15:40         ` Jan Kara
2018-10-03 16:18           ` Amir Goldstein
2018-10-03 16:33             ` Jan Kara
2018-10-03 20:45               ` Matthew Bobrowski
2018-10-07 11:13               ` Matthew Bobrowski
2018-10-07 13:40                 ` Amir Goldstein
2018-10-08  9:35                 ` Jan Kara
2018-10-02 11:50       ` Matthew Bobrowski
2018-10-03 15:45         ` Jan Kara
2018-10-01 11:06 ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=92b88900-c4be-7acf-b2f4-81d52eab1f5e@mbobrowski.org \
    --to=mbobrowski@mbobrowski.org \
    --cc=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=sgrubb@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).