Re: FAN_OPEN_EXEC event and ignore mask

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

[Change the subject and add fsdevel to CC]

On Tue, Oct 30, 2018 at 2:27 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
[...]
> Then I concluded that it doesn't have to be event mask specific i.e.
> FAN_OPEN_EXEC. Irrespective of what the event_mask value actually is, if
> it contains a flag that has also been set within marks_ignored_mask, then
> the event should *not* be sent through to the user:
>
> if (event_mask & marks_ignored_mask)
>         return 0;
>
> I think what Amir has proposed here is also correct and the cleanest
> implementation to achieve what we want.
>

My proposal had a bug w.r.t handling of FS_EVENT_ON_CHILD
in ignore mask. I fixed it and pushed to branch:
https://github.com/amir73il/linux/commits/fanotify_ignore
In the first commit ("fanotify: fix handling of FS_EVENT_ON_CHILD
sub-directory events")
I have made a claim about an existing bug -
that claim still needs to be proven by a test case.
it could be there is no bug and this is just an optimization.

Man page should be revised to clarify the currently expected behavior:
FAN_EVENT_ON_CHILD ...
  The flag has no effect when marking mounts
+ or filesystems and has no effect when set in ignore mask

Please include that change in your man page draft for new
ignore mask interpretations.

> >
> > Nothing will need to be change for FS_OPEN_EXEC with this change
> > applied to implement semantics #2 above.
> > I only hope this change is correct... (it did pass existing and new ltp tests)
> >
>
> I also tested this on my side the using existing and newly proposed LTP
> test cases, all tests had passed. This change also covers the last test
> that I defined in fanotify12 where the mark_mask = FAN_OPEN_EXEC and
> ignore_mask = FAN_OPEN. Prior to this change, an event for FAN_OPEN_EXEC
> would still slip through despite being a subtype of an open, which
> shouldn't be the case. With this change implemented, NO events are sent
> through, which is exactly what is expected.
>
> > >   Pros: FAN_OPEN_NOEXEC easy to do using ignore masks.
> > >   Cons: Semantics is IMO somewhat difficult to explain in the manpage.
> > >         Probably we'd need to really explain that FAN_OPEN is really a
> > >         compound of FS_OPEN_NOEXEC and FS_OPEN_EXEC.
> > >
> > > So overall I think the semantics from 2) is more useful. But I'd start with
> > > manpage properly explaining the semantics and interactions between FAN_OPEN
> > > and FAN_OPEN_EXEC. If that can be written so that user's head does not
> > > spin, I think the implementation can be done in a reasonably simple way as
> > > well. And I'm really sorry for leading you in circles Matthew...
> > >
> >
> > Agree to the "man page criteria"
>
> This is fine. I can document the FAN_OPEN and FAN_OPEN_EXEC semantics
> really well, whenever required. But, I'd like to agree upon the solution
> and have that finalised before I shift my focus on documentation.
>

The idea of "document first" is that if you cannot come up with simple man page
we are not going to implement those semantics, because they will be unusable.

So I agree with Jan. Please take a swing at man page change.
It can be a very rough draft, you can send a link to github, so long
as we can see
that a simple man page is "doable".

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

On Tue, Oct 30, 2018 at 11:17:06AM +0200, Amir Goldstein wrote:
> [Change the subject and add fsdevel to CC]
> 
> On Tue, Oct 30, 2018 at 2:27 AM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> [...]
> > Then I concluded that it doesn't have to be event mask specific i.e.
> > FAN_OPEN_EXEC. Irrespective of what the event_mask value actually is, if
> > it contains a flag that has also been set within marks_ignored_mask, then
> > the event should *not* be sent through to the user:
> >
> > if (event_mask & marks_ignored_mask)
> >         return 0;
> >
> > I think what Amir has proposed here is also correct and the cleanest
> > implementation to achieve what we want.
> >
> 
> My proposal had a bug w.r.t handling of FS_EVENT_ON_CHILD
> in ignore mask. I fixed it and pushed to branch:
> https://github.com/amir73il/linux/commits/fanotify_ignore
> In the first commit ("fanotify: fix handling of FS_EVENT_ON_CHILD
> sub-directory events")
> I have made a claim about an existing bug -
> that claim still needs to be proven by a test case.
> it could be there is no bug and this is just an optimization.

Personally, I think it's addressing behaviour that was not previously
anticipated or accounted for. That being said, I'm in favour of this
change.

> 
> Man page should be revised to clarify the currently expected behavior:
> FAN_EVENT_ON_CHILD ...
>   The flag has no effect when marking mounts
> + or filesystems and has no effect when set in ignore mask
> 
> Please include that change in your man page draft for new
> ignore mask interpretations.

OK. I've updated the man pages to include the clarification around the
revised handling of ignore mask. These can be found here:

https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore

Wasn't overly confident about where I've placed the explanations, but I
felt that's where they fitted best. I was also thinking that we could have
an example of a compound event to illustrate the functionality further? 

> 
> > >
> > > Nothing will need to be change for FS_OPEN_EXEC with this change
> > > applied to implement semantics #2 above.
> > > I only hope this change is correct... (it did pass existing and new ltp tests)
> > >
> >
> > I also tested this on my side the using existing and newly proposed LTP
> > test cases, all tests had passed. This change also covers the last test
> > that I defined in fanotify12 where the mark_mask = FAN_OPEN_EXEC and
> > ignore_mask = FAN_OPEN. Prior to this change, an event for FAN_OPEN_EXEC
> > would still slip through despite being a subtype of an open, which
> > shouldn't be the case. With this change implemented, NO events are sent
> > through, which is exactly what is expected.
> >
> > > >   Pros: FAN_OPEN_NOEXEC easy to do using ignore masks.
> > > >   Cons: Semantics is IMO somewhat difficult to explain in the manpage.
> > > >         Probably we'd need to really explain that FAN_OPEN is really a
> > > >         compound of FS_OPEN_NOEXEC and FS_OPEN_EXEC.
> > > >
> > > > So overall I think the semantics from 2) is more useful. But I'd start with
> > > > manpage properly explaining the semantics and interactions between FAN_OPEN
> > > > and FAN_OPEN_EXEC. If that can be written so that user's head does not
> > > > spin, I think the implementation can be done in a reasonably simple way as
> > > > well. And I'm really sorry for leading you in circles Matthew...
> > > >
> > >
> > > Agree to the "man page criteria"
> >
> > This is fine. I can document the FAN_OPEN and FAN_OPEN_EXEC semantics
> > really well, whenever required. But, I'd like to agree upon the solution
> > and have that finalised before I shift my focus on documentation.
> >
> 
> The idea of "document first" is that if you cannot come up with simple man page
> we are not going to implement those semantics, because they will be unusable.
> 
> So I agree with Jan. Please take a swing at man page change.
> It can be a very rough draft, you can send a link to github, so long
> as we can see
> that a simple man page is "doable".

Roger that. Thanks for sharing the view point. I've supplied the link that
contains the refactored ignore mask explanation above. I think this is
what you meant when you wanted to see the documentation first, however if
I've completely misinterpreted something, please clarify.

-- 
Matthew Bobrowski <mbobrowski@mbobrowski.org>

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

> >
> > Man page should be revised to clarify the currently expected behavior:
> > FAN_EVENT_ON_CHILD ...
> >   The flag has no effect when marking mounts
> > + or filesystems and has no effect when set in ignore mask
> >
> > Please include that change in your man page draft for new
> > ignore mask interpretations.
>
> OK. I've updated the man pages to include the clarification around the
> revised handling of ignore mask. These can be found here:
>
> https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore
>
> Wasn't overly confident about where I've placed the explanations, but I
> felt that's where they fitted best. I was also thinking that we could have
> an example of a compound event to illustrate the functionality further?
>

I can see it clearly now - Jan was right all along -
We cannot afford to add new constructs to this man page
like "compound event" - it will just be too complicated to understand.

In early discussions, we spoke of two options:
- Independent event (this haven't been well defined)
- Informational flag (like IN_ISDIR), which is unprecedented in fanotify

Jan steered you towards the Independent event option, which I now
completely agree with and so I also agree with Jan that interpretation
of ignore mask should be "mask the event bit out".

On the question of whether execve() should generate two "separate"
events OPEN and OPEN_EXEC or just one combined event
OPEN | OPEN_EXEC, I am leaning towards one combined event
(like you implemented). Non permission events can be merged, so
user will not know the difference anyway. 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.

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

On Thu, Nov 01, 2018 at 04:45:47PM +0200, Amir Goldstein wrote:
> > >
> > > Man page should be revised to clarify the currently expected behavior:
> > > FAN_EVENT_ON_CHILD ...
> > >   The flag has no effect when marking mounts
> > > + or filesystems and has no effect when set in ignore mask
> > >
> > > Please include that change in your man page draft for new
> > > ignore mask interpretations.
> >
> > OK. I've updated the man pages to include the clarification around the
> > revised handling of ignore mask. These can be found here:
> >
> > https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore
> >
> > Wasn't overly confident about where I've placed the explanations, but I
> > felt that's where they fitted best. I was also thinking that we could have
> > an example of a compound event to illustrate the functionality further?
> >
> 
> I can see it clearly now - Jan was right all along -
> We cannot afford to add new constructs to this man page
> like "compound event" - it will just be too complicated to understand.
> 
> In early discussions, we spoke of two options:
> - Independent event (this haven't been well defined)
> - Informational flag (like IN_ISDIR), which is unprecedented in fanotify
> 
> Jan steered you towards the Independent event option, which I now
> completely agree with and so I also agree with Jan that interpretation
> of ignore mask should be "mask the event bit out".

Right, if that's the case then does that also mean that I can leave the
last two LTP test cases for FAN_OPEN_EXEC as they are? Based on the fact
that they've been defined to work with the ignore mask in that exact
manner?

Also, in addition to that, it means that the man-pages update that I've
linked above is completely irrelevant as we're in agreement that we're
not changing ignore mask behaviour?

> On the question of whether execve() should generate two "separate"
> events OPEN and OPEN_EXEC or just one combined event
> OPEN | OPEN_EXEC, I am leaning towards one combined event
> (like you implemented). Non permission events can be merged, so
> user will not know the difference anyway. 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.

Good point, I'll ensure to note this in my man-pages update for the EXEC
events.

-- 
Matthew Bobrowski

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

On Fri, Nov 2, 2018 at 1:36 PM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Thu, Nov 01, 2018 at 04:45:47PM +0200, Amir Goldstein wrote:
> > > >
> > > > Man page should be revised to clarify the currently expected behavior:
> > > > FAN_EVENT_ON_CHILD ...
> > > >   The flag has no effect when marking mounts
> > > > + or filesystems and has no effect when set in ignore mask
> > > >
> > > > Please include that change in your man page draft for new
> > > > ignore mask interpretations.
> > >
> > > OK. I've updated the man pages to include the clarification around the
> > > revised handling of ignore mask. These can be found here:
> > >
> > > https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore
> > >
> > > Wasn't overly confident about where I've placed the explanations, but I
> > > felt that's where they fitted best. I was also thinking that we could have
> > > an example of a compound event to illustrate the functionality further?
> > >
> >
> > I can see it clearly now - Jan was right all along -
> > We cannot afford to add new constructs to this man page
> > like "compound event" - it will just be too complicated to understand.
> >
> > In early discussions, we spoke of two options:
> > - Independent event (this haven't been well defined)
> > - Informational flag (like IN_ISDIR), which is unprecedented in fanotify
> >
> > Jan steered you towards the Independent event option, which I now
> > completely agree with and so I also agree with Jan that interpretation
> > of ignore mask should be "mask the event bit out".
>
> Right, if that's the case then does that also mean that I can leave the
> last two LTP test cases for FAN_OPEN_EXEC as they are? Based on the fact
> that they've been defined to work with the ignore mask in that exact
> manner?

Correct.

>
> Also, in addition to that, it means that the man-pages update that I've
> linked above is completely irrelevant as we're in agreement that we're
> not changing ignore mask behaviour?

Some clarifications about how ignore mask works may be in order, but that
added note is irrelevant.

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Jan Kara]

On Thu 01-11-18 16:45:47, Amir Goldstein wrote:
> > >
> > > Man page should be revised to clarify the currently expected behavior:
> > > FAN_EVENT_ON_CHILD ...
> > >   The flag has no effect when marking mounts
> > > + or filesystems and has no effect when set in ignore mask
> > >
> > > Please include that change in your man page draft for new
> > > ignore mask interpretations.
> >
> > OK. I've updated the man pages to include the clarification around the
> > revised handling of ignore mask. These can be found here:
> >
> > https://github.com/matthewbobrowski/man-pages/commits/fanotify_ignore
> >
> > Wasn't overly confident about where I've placed the explanations, but I
> > felt that's where they fitted best. I was also thinking that we could have
> > an example of a compound event to illustrate the functionality further?
> >
> 
> I can see it clearly now - Jan was right all along -
> We cannot afford to add new constructs to this man page
> like "compound event" - it will just be too complicated to understand.
> 
> In early discussions, we spoke of two options:
> - Independent event (this haven't been well defined)
> - Informational flag (like IN_ISDIR), which is unprecedented in fanotify
> 
> Jan steered you towards the Independent event option, which I now
> completely agree with and so I also agree with Jan that interpretation
> of ignore mask should be "mask the event bit out".

Good, so we are on the same page here.

> On the question of whether execve() should generate two "separate"
> events OPEN and OPEN_EXEC or just one combined event
> OPEN | OPEN_EXEC, I am leaning towards one combined event
> (like you implemented). Non permission events can be merged, so
> user will not know the difference anyway.

Agreed.

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

On Fri, Nov 2, 2018 at 2:50 PM Jan Kara <jack@suse.cz> 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?
>

I have no objections to {fsnotify()/fsnotify_parent()}x2

Speaking of which, just posted a fix patch last week to deal with double events
on sub-directories.

My only concern w.r.t separate event is, if we ever wanted to add
OPEN_WRITE_PERM, would you have made the same decisions as we are
making now for OPEN_EXEC_PERM? If the answer is yes, then separate
events are fine by me.

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

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.

-- 
Matthew Bobrowski <mbobrowski@mbobrowski.org>

Re: FAN_OPEN_EXEC event and ignore mask

[Jan Kara]

On Fri 02-11-18 15:43:04, Amir Goldstein wrote:
> On Fri, Nov 2, 2018 at 2:50 PM Jan Kara <jack@suse.cz> 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?
> >
> 
> I have no objections to {fsnotify()/fsnotify_parent()}x2
> 
> Speaking of which, just posted a fix patch last week to deal with double events
> on sub-directories.

Yeah, I know, it is sitting in my inbox waiting for me to look into in
carefully enough :)

> My only concern w.r.t separate event is, if we ever wanted to add
> OPEN_WRITE_PERM, would you have made the same decisions as we are
> making now for OPEN_EXEC_PERM? If the answer is yes, then separate
> events are fine by me.

Yes, I think separate events for OPEN_PERM and OPEN_WRITE_PERM are fine as
well. So let's go for separate events.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: FAN_OPEN_EXEC event and ignore mask

[Jan Kara]

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!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

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!

OK, great. I will send through the updated patch series shortly.

-- 
Matthew Bobrowski

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

On Mon, Nov 5, 2018 at 11:06 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:

> > Yes, so please update your patch adding FAN_OPEN_EXEC_PERM to send this
> > event separately from FAN_OPEN_PERM. Thanks!
>
> OK, great. I will send through the updated patch series shortly.
>

Matthew,

Please be advised that your patch
"fanotify: return only user requested event types in event mask"
has a (simple) conflict with my fix patch:
"fanotify: fix handling of events on child sub-directory"

You can find the conflict resolution on my fsnotify-fixes branch.
I advise that you to rebase and test FAN_OPEN_EXEC patches on
top of this branch (your first patch is already there).
fsnotify-fixes is based of off v4.20-rc1.

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

On Mon, Nov 05, 2018 at 02:27:00PM +0200, Amir Goldstein wrote:
> On Mon, Nov 5, 2018 at 11:06 AM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> 
> > > Yes, so please update your patch adding FAN_OPEN_EXEC_PERM to send this
> > > event separately from FAN_OPEN_PERM. Thanks!
> >
> > OK, great. I will send through the updated patch series shortly.
> >
> Please be advised that your patch
> "fanotify: return only user requested event types in event mask"
> has a (simple) conflict with my fix patch:
> "fanotify: fix handling of events on child sub-directory"

Noted, thanks for the heads up.

> You can find the conflict resolution on my fsnotify-fixes branch.
> I advise that you to rebase and test FAN_OPEN_EXEC patches on
> top of this branch (your first patch is already there).
> fsnotify-fixes is based of off v4.20-rc1.

Sure, I will get this done tomorrow and once confirmed will send through
the patch series. Thanks!

-- 
Matthew Bobrowski

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

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@mbobrowski.org>

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

On Tue, Nov 6, 2018 at 3:08 PM Matthew Bobrowski
<mbobrowski@mbobrowski.org> 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().

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

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

Typo:
        return fsnotify_path(inode, path, fsnotify_mask);

>
> You can consolidate all 5 calls to fsnotify_parent();fsnotify() of the same
> pattern to fsnotify_path().
>
> Thanks,
> Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

On Tue, Nov 06, 2018 at 03:45:43PM +0200, Amir Goldstein wrote:
> On Tue, Nov 6, 2018 at 3:08 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> 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 <mbobrowski@mbobrowski.org>

Re: FAN_OPEN_EXEC event and ignore mask

[Amir Goldstein]

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

I don't know if you are overthinking but I still don't understand the concern.

Before the change:
- Listen on FAN_OPEN_PERM
- open file for read
- open file for write
- execve()
User gets 3 FAN_OPEN_PERM permission events

After the change this is still the behavior with or without requesting
FAN_OPEN_EXEC_PERM.

It's quite obvious that before the change user cannot request
FAN_OPEN_EXEC_PERM and after the change requesting
FAN_OPEN_EXEC_PERM with or without FAN_OPEN_PERM,
will result in getting the new event once.

The only case where user won't get 3 FAN_OPEN_PERM events
is when user denies the FAN_OPEN_EXEC_PERM event.
What is your concern?

Thanks,
Amir.

Re: FAN_OPEN_EXEC event and ignore mask

[Matthew Bobrowski]

On Tue, Nov 06, 2018 at 11:15:11PM +0200, Amir Goldstein wrote:
> > >
> > > 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?
> >
> 
> I don't know if you are overthinking but I still don't understand the concern.
> 
> Before the change:
> - Listen on FAN_OPEN_PERM
> - open file for read
> - open file for write
> - execve()
> User gets 3 FAN_OPEN_PERM permission events
> 
> After the change this is still the behavior with or without requesting
> FAN_OPEN_EXEC_PERM.
> 
> It's quite obvious that before the change user cannot request
> FAN_OPEN_EXEC_PERM and after the change requesting
> FAN_OPEN_EXEC_PERM with or without FAN_OPEN_PERM,
> will result in getting the new event once.
> 
> The only case where user won't get 3 FAN_OPEN_PERM events
> is when user denies the FAN_OPEN_EXEC_PERM event.
> What is your concern?

OK, my apologies. It was the way I implemented it that was causing me to get
confused as the results weren't aligning with what you so rightfully outlined
above.

Sorry about that Amir.

-- 
Matthew Bobrowski