linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Matthew Bobrowski <mbobrowski@mbobrowski.org>
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: Fri, 28 Sep 2018 08:39:21 +0300	[thread overview]
Message-ID: <CAOQ4uxh4wwmO0JWqY1ceJ2ApEYhQWS1rpK1hPiXGzLDxGJ2qMQ@mail.gmail.com> (raw)
In-Reply-To: <e35aa7fb-13fc-4298-15f6-df6ddceecf86@mbobrowski.org>

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...

> > 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

Now I've modified my dev branch to use the implicit enable:
- Requesting any dentry event sets internal group flag FAN_ENABLE_DENTRY
- "bonus" event bit FAN_ONDIR is exposed if group has FAN_ENABLE_DENTRY
https://github.com/amir73il/linux/commits/fanotify_dentry

Thanks,
Amir.

  reply	other threads:[~2018-09-28 12:01 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 [this message]
2018-10-01  8:21       ` Matthew Bobrowski
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=CAOQ4uxh4wwmO0JWqY1ceJ2ApEYhQWS1rpK1hPiXGzLDxGJ2qMQ@mail.gmail.com \
    --to=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=mbobrowski@mbobrowski.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).