diff mbox series

[02/10] fsnotify: introduce pre-content permission event

Message ID a6010470b2d11f186cba89b9521940716fa66f3b.1721931241.git.josef@toxicpanda.com (mailing list archive)
State New
Headers show
Series fanotify: add pre-content hooks | expand

Commit Message

Josef Bacik July 25, 2024, 6:19 p.m. UTC
From: Amir Goldstein <amir73il@gmail.com>

The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
but it meant for a different use case of filling file content before
access to a file range, so it has slightly different semantics.

Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.

FS_PRE_MODIFY is a new permission event, with similar semantics as
FS_PRE_ACCESS, which is called before a file is modified.

FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
pre-content events are only reported for regular files and dirs.

The pre-content events are meant to be used by hierarchical storage
managers that want to fill the content of files on first access.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             |  2 +-
 include/linux/fsnotify.h         | 27 ++++++++++++++++++++++++---
 include/linux/fsnotify_backend.h | 13 +++++++++++--
 security/selinux/hooks.c         |  3 ++-
 4 files changed, 38 insertions(+), 7 deletions(-)

Comments

Jan Kara Aug. 1, 2024, 4:31 p.m. UTC | #1
On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> From: Amir Goldstein <amir73il@gmail.com>
> 
> The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> but it meant for a different use case of filling file content before
> access to a file range, so it has slightly different semantics.
> 
> Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> 
> FS_PRE_MODIFY is a new permission event, with similar semantics as
> FS_PRE_ACCESS, which is called before a file is modified.
> 
> FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> pre-content events are only reported for regular files and dirs.
> 
> The pre-content events are meant to be used by hierarchical storage
> managers that want to fill the content of files on first access.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good. Just out of curiosity:

> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 8be029bc50b1..21e72b837ec5 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -56,6 +56,9 @@
>  #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
>  #define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
>  
> +#define FS_PRE_ACCESS		0x00100000	/* Pre-content access hook */
> +#define FS_PRE_MODIFY		0x00200000	/* Pre-content modify hook */

Why is a hole left here in the flag space?

								Honza
Amir Goldstein Aug. 3, 2024, 4:52 p.m. UTC | #2
On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > From: Amir Goldstein <amir73il@gmail.com>
> >
> > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > but it meant for a different use case of filling file content before
> > access to a file range, so it has slightly different semantics.
> >
> > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> >
> > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > FS_PRE_ACCESS, which is called before a file is modified.
> >
> > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > pre-content events are only reported for regular files and dirs.
> >
> > The pre-content events are meant to be used by hierarchical storage
> > managers that want to fill the content of files on first access.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> The patch looks good. Just out of curiosity:
>
> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index 8be029bc50b1..21e72b837ec5 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -56,6 +56,9 @@
> >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> >
> > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
>
> Why is a hole left here in the flag space?

Can't remember.

Currently we have a draft design for two more events
FS_PATH_ACCESS, FS_PATH_MODIFY
https://github.com/amir73il/man-pages/commits/fan_pre_path

So might have been a desire to keep the pre-events group on the nibble.

Thanks,
Amir.
Amir Goldstein Oct. 25, 2024, 7:55 a.m. UTC | #3
On Sat, Aug 3, 2024 at 6:52 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > > From: Amir Goldstein <amir73il@gmail.com>
> > >
> > > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > > but it meant for a different use case of filling file content before
> > > access to a file range, so it has slightly different semantics.
> > >
> > > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> > >
> > > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > > FS_PRE_ACCESS, which is called before a file is modified.
> > >
> > > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > > pre-content events are only reported for regular files and dirs.
> > >
> > > The pre-content events are meant to be used by hierarchical storage
> > > managers that want to fill the content of files on first access.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > The patch looks good. Just out of curiosity:
> >
> > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > > index 8be029bc50b1..21e72b837ec5 100644
> > > --- a/include/linux/fsnotify_backend.h
> > > +++ b/include/linux/fsnotify_backend.h
> > > @@ -56,6 +56,9 @@
> > >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> > >
> > > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
> >
> > Why is a hole left here in the flag space?
>
> Can't remember.
>
> Currently we have a draft design for two more events
> FS_PATH_ACCESS, FS_PATH_MODIFY
> https://github.com/amir73il/man-pages/commits/fan_pre_path
>
> So might have been a desire to keep the pre-events group on the nibble.

Funny story.

I straced a program with latest FS_PRE_ACCESS (0x00080000) and
see what I got:

fanotify_mark(3, FAN_MARK_ADD|FAN_MARK_MOUNT,
FAN_CLOSE_WRITE|FAN_OPEN_PERM|FAN_ACCESS_PERM|FAN_DIR_MODIFY|FAN_ONDIR,
AT_FDCWD, "/vdd") = 0

"FAN_DIR_MODIFY"! a blast from the past [1]

It would have been nice if we reserved 0x00080000 for FAN_PATH_MODIFY [2]
to be a bit less confusing for users with old strace.

WDYT?

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20200612093343.5669-18-amir73il@gmail.com/
[2] https://github.com/amir73il/man-pages/commits/fan_pre_path
Jan Kara Oct. 25, 2024, 1:09 p.m. UTC | #4
On Fri 25-10-24 09:55:21, Amir Goldstein wrote:
> On Sat, Aug 3, 2024 at 6:52 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
> > > On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > > > From: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > > > but it meant for a different use case of filling file content before
> > > > access to a file range, so it has slightly different semantics.
> > > >
> > > > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > > > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> > > >
> > > > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > > > FS_PRE_ACCESS, which is called before a file is modified.
> > > >
> > > > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > > > pre-content events are only reported for regular files and dirs.
> > > >
> > > > The pre-content events are meant to be used by hierarchical storage
> > > > managers that want to fill the content of files on first access.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > The patch looks good. Just out of curiosity:
> > >
> > > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > > > index 8be029bc50b1..21e72b837ec5 100644
> > > > --- a/include/linux/fsnotify_backend.h
> > > > +++ b/include/linux/fsnotify_backend.h
> > > > @@ -56,6 +56,9 @@
> > > >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> > > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> > > >
> > > > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > > > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
> > >
> > > Why is a hole left here in the flag space?
> >
> > Can't remember.
> >
> > Currently we have a draft design for two more events
> > FS_PATH_ACCESS, FS_PATH_MODIFY
> > https://github.com/amir73il/man-pages/commits/fan_pre_path
> >
> > So might have been a desire to keep the pre-events group on the nibble.
> 
> Funny story.
> 
> I straced a program with latest FS_PRE_ACCESS (0x00080000) and
> see what I got:
> 
> fanotify_mark(3, FAN_MARK_ADD|FAN_MARK_MOUNT,
> FAN_CLOSE_WRITE|FAN_OPEN_PERM|FAN_ACCESS_PERM|FAN_DIR_MODIFY|FAN_ONDIR,
> AT_FDCWD, "/vdd") = 0
> 
> "FAN_DIR_MODIFY"! a blast from the past [1]
> 
> It would have been nice if we reserved 0x00080000 for FAN_PATH_MODIFY [2]
> to be a bit less confusing for users with old strace.
> 
> WDYT?

Yeah, reusing that bit for something semantically close would reduce some
confusion. But realistically I don't think FAN_DIR_MODIFY go wide use when
it was never supported in a released upstream kernel.

								Honza
Amir Goldstein Oct. 25, 2024, 1:39 p.m. UTC | #5
On Fri, Oct 25, 2024 at 3:09 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 25-10-24 09:55:21, Amir Goldstein wrote:
> > On Sat, Aug 3, 2024 at 6:52 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
> > > > On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > > > > but it meant for a different use case of filling file content before
> > > > > access to a file range, so it has slightly different semantics.
> > > > >
> > > > > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > > > > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> > > > >
> > > > > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > > > > FS_PRE_ACCESS, which is called before a file is modified.
> > > > >
> > > > > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > > > > pre-content events are only reported for regular files and dirs.
> > > > >
> > > > > The pre-content events are meant to be used by hierarchical storage
> > > > > managers that want to fill the content of files on first access.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > The patch looks good. Just out of curiosity:
> > > >
> > > > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > > > > index 8be029bc50b1..21e72b837ec5 100644
> > > > > --- a/include/linux/fsnotify_backend.h
> > > > > +++ b/include/linux/fsnotify_backend.h
> > > > > @@ -56,6 +56,9 @@
> > > > >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> > > > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> > > > >
> > > > > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > > > > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
> > > >
> > > > Why is a hole left here in the flag space?
> > >
> > > Can't remember.
> > >
> > > Currently we have a draft design for two more events
> > > FS_PATH_ACCESS, FS_PATH_MODIFY
> > > https://github.com/amir73il/man-pages/commits/fan_pre_path
> > >
> > > So might have been a desire to keep the pre-events group on the nibble.
> >
> > Funny story.
> >
> > I straced a program with latest FS_PRE_ACCESS (0x00080000) and
> > see what I got:
> >
> > fanotify_mark(3, FAN_MARK_ADD|FAN_MARK_MOUNT,
> > FAN_CLOSE_WRITE|FAN_OPEN_PERM|FAN_ACCESS_PERM|FAN_DIR_MODIFY|FAN_ONDIR,
> > AT_FDCWD, "/vdd") = 0
> >
> > "FAN_DIR_MODIFY"! a blast from the past [1]
> >
> > It would have been nice if we reserved 0x00080000 for FAN_PATH_MODIFY [2]
> > to be a bit less confusing for users with old strace.
> >
> > WDYT?
>
> Yeah, reusing that bit for something semantically close would reduce some
> confusion. But realistically I don't think FAN_DIR_MODIFY go wide use when
> it was never supported in a released upstream kernel.

No, but its legacy lives in strace forever...

Anyway I included a patch in my fan_pre_access branch to reserve this bit
because what have we got to loose..

Thanks,
Amir.
Amir Goldstein Oct. 26, 2024, 6:58 a.m. UTC | #6
On Fri, Oct 25, 2024 at 3:39 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 3:09 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 25-10-24 09:55:21, Amir Goldstein wrote:
> > > On Sat, Aug 3, 2024 at 6:52 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
> > > > > On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > > >
> > > > > > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > > > > > but it meant for a different use case of filling file content before
> > > > > > access to a file range, so it has slightly different semantics.
> > > > > >
> > > > > > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > > > > > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> > > > > >
> > > > > > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > > > > > FS_PRE_ACCESS, which is called before a file is modified.
> > > > > >
> > > > > > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > > > > > pre-content events are only reported for regular files and dirs.
> > > > > >
> > > > > > The pre-content events are meant to be used by hierarchical storage
> > > > > > managers that want to fill the content of files on first access.
> > > > > >
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > The patch looks good. Just out of curiosity:
> > > > >
> > > > > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > > > > > index 8be029bc50b1..21e72b837ec5 100644
> > > > > > --- a/include/linux/fsnotify_backend.h
> > > > > > +++ b/include/linux/fsnotify_backend.h
> > > > > > @@ -56,6 +56,9 @@
> > > > > >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> > > > > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> > > > > >
> > > > > > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > > > > > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
> > > > >
> > > > > Why is a hole left here in the flag space?
> > > >
> > > > Can't remember.
> > > >
> > > > Currently we have a draft design for two more events
> > > > FS_PATH_ACCESS, FS_PATH_MODIFY
> > > > https://github.com/amir73il/man-pages/commits/fan_pre_path
> > > >
> > > > So might have been a desire to keep the pre-events group on the nibble.
> > >
> > > Funny story.
> > >
> > > I straced a program with latest FS_PRE_ACCESS (0x00080000) and
> > > see what I got:
> > >
> > > fanotify_mark(3, FAN_MARK_ADD|FAN_MARK_MOUNT,
> > > FAN_CLOSE_WRITE|FAN_OPEN_PERM|FAN_ACCESS_PERM|FAN_DIR_MODIFY|FAN_ONDIR,
> > > AT_FDCWD, "/vdd") = 0
> > >
> > > "FAN_DIR_MODIFY"! a blast from the past [1]
> > >
> > > It would have been nice if we reserved 0x00080000 for FAN_PATH_MODIFY [2]
> > > to be a bit less confusing for users with old strace.
> > >
> > > WDYT?
> >
> > Yeah, reusing that bit for something semantically close would reduce some
> > confusion. But realistically I don't think FAN_DIR_MODIFY go wide use when
> > it was never supported in a released upstream kernel.
>
> No, but its legacy lives in strace forever...
>

Speaking of legacy events, you will notice that in the fan_pre_access
branch I swapped the order of FS_PRE_ACCESS to be generated
before FS_ACCESS_PERM.

It is a semantic difference that probably does not matter much in practice,
but I justified it as "need to fill the content before content can be inspected"
because FS_ACCESS_PERM is the legacy Anti-malware event.

This order is also aligned with the priority group associated with those
events (PRE_CONTENT before CONTENT).

But from a wider POV, my feeling is that FS_ACCESS_PERM is not
really used by anyone and it is baggage that we need to try to get rid of.
It is not worth the bloat of the inlined fsnotify_file_area_perm() hook.
It is not worth the wasted cycles in the __fsnotify_parent() call that will
not be optimized when there is any high priority group listener on the sb.

I am tempted to try and combine the PRE/PERM access events into
a single event and make sure that no fanotify group can subscribe to
both of them at the same time, so a combined event can never be seen,
but it is not very easy to rationalize this API.

For example, if we would have required FAN_REPORT_RANGE init flag
for subscribing to FAN_PRE_ACCESS, then we could have denied the legacy
FAN_ACCESS_PERM in this group, but I don't think that we want to do that (?).

WDYT? Am I overthinking again?

Thanks,
Amir.
Jan Kara Oct. 31, 2024, 12:47 p.m. UTC | #7
On Sat 26-10-24 08:58:47, Amir Goldstein wrote:
> On Fri, Oct 25, 2024 at 3:39 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 3:09 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Fri 25-10-24 09:55:21, Amir Goldstein wrote:
> > > > On Sat, Aug 3, 2024 at 6:52 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > On Thu, Aug 1, 2024 at 6:31 PM Jan Kara <jack@suse.cz> wrote:
> > > > > > On Thu 25-07-24 14:19:39, Josef Bacik wrote:
> > > > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > > > >
> > > > > > > The new FS_PRE_ACCESS permission event is similar to FS_ACCESS_PERM,
> > > > > > > but it meant for a different use case of filling file content before
> > > > > > > access to a file range, so it has slightly different semantics.
> > > > > > >
> > > > > > > Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events, same as
> > > > > > > we did for FS_OPEN_PERM/FS_OPEN_EXEC_PERM.
> > > > > > >
> > > > > > > FS_PRE_MODIFY is a new permission event, with similar semantics as
> > > > > > > FS_PRE_ACCESS, which is called before a file is modified.
> > > > > > >
> > > > > > > FS_ACCESS_PERM is reported also on blockdev and pipes, but the new
> > > > > > > pre-content events are only reported for regular files and dirs.
> > > > > > >
> > > > > > > The pre-content events are meant to be used by hierarchical storage
> > > > > > > managers that want to fill the content of files on first access.
> > > > > > >
> > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > >
> > > > > > The patch looks good. Just out of curiosity:
> > > > > >
> > > > > > > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > > > > > > index 8be029bc50b1..21e72b837ec5 100644
> > > > > > > --- a/include/linux/fsnotify_backend.h
> > > > > > > +++ b/include/linux/fsnotify_backend.h
> > > > > > > @@ -56,6 +56,9 @@
> > > > > > >  #define FS_ACCESS_PERM               0x00020000      /* access event in a permissions hook */
> > > > > > >  #define FS_OPEN_EXEC_PERM    0x00040000      /* open/exec event in a permission hook */
> > > > > > >
> > > > > > > +#define FS_PRE_ACCESS                0x00100000      /* Pre-content access hook */
> > > > > > > +#define FS_PRE_MODIFY                0x00200000      /* Pre-content modify hook */
> > > > > >
> > > > > > Why is a hole left here in the flag space?
> > > > >
> > > > > Can't remember.
> > > > >
> > > > > Currently we have a draft design for two more events
> > > > > FS_PATH_ACCESS, FS_PATH_MODIFY
> > > > > https://github.com/amir73il/man-pages/commits/fan_pre_path
> > > > >
> > > > > So might have been a desire to keep the pre-events group on the nibble.
> > > >
> > > > Funny story.
> > > >
> > > > I straced a program with latest FS_PRE_ACCESS (0x00080000) and
> > > > see what I got:
> > > >
> > > > fanotify_mark(3, FAN_MARK_ADD|FAN_MARK_MOUNT,
> > > > FAN_CLOSE_WRITE|FAN_OPEN_PERM|FAN_ACCESS_PERM|FAN_DIR_MODIFY|FAN_ONDIR,
> > > > AT_FDCWD, "/vdd") = 0
> > > >
> > > > "FAN_DIR_MODIFY"! a blast from the past [1]
> > > >
> > > > It would have been nice if we reserved 0x00080000 for FAN_PATH_MODIFY [2]
> > > > to be a bit less confusing for users with old strace.
> > > >
> > > > WDYT?
> > >
> > > Yeah, reusing that bit for something semantically close would reduce some
> > > confusion. But realistically I don't think FAN_DIR_MODIFY go wide use when
> > > it was never supported in a released upstream kernel.
> >
> > No, but its legacy lives in strace forever...
> >
> 
> Speaking of legacy events, you will notice that in the fan_pre_access
> branch I swapped the order of FS_PRE_ACCESS to be generated
> before FS_ACCESS_PERM.
> 
> It is a semantic difference that probably does not matter much in practice,
> but I justified it as "need to fill the content before content can be inspected"
> because FS_ACCESS_PERM is the legacy Anti-malware event.
> 
> This order is also aligned with the priority group associated with those
> events (PRE_CONTENT before CONTENT).

Yes, I've noticed this and it makes sense. Thanks for the expanded
rationale.

> But from a wider POV, my feeling is that FS_ACCESS_PERM is not
> really used by anyone and it is baggage that we need to try to get rid of.
> It is not worth the bloat of the inlined fsnotify_file_area_perm() hook.
> It is not worth the wasted cycles in the __fsnotify_parent() call that will
> not be optimized when there is any high priority group listener on the sb.
> 
> I am tempted to try and combine the PRE/PERM access events into
> a single event and make sure that no fanotify group can subscribe to
> both of them at the same time, so a combined event can never be seen,
> but it is not very easy to rationalize this API.
> 
> For example, if we would have required FAN_REPORT_RANGE init flag
> for subscribing to FAN_PRE_ACCESS, then we could have denied the legacy
> FAN_ACCESS_PERM in this group, but I don't think that we want to do that (?).

Yeah, this would look a bit weird in the API. If you really think that
FAN_ACCESS_PERM is dead (which it may well be but I would not bet on it),
then we could start a deprecation period for it and if nobody comes back to
us saying they still use it, we can then remove it from the kernel
altogether.

								Honza
diff mbox series

Patch

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 272c8a1dab3c..1ca4a8da7f29 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -621,7 +621,7 @@  static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 23);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 25);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 278620e063ab..028ce807805a 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -133,12 +133,13 @@  static inline int fsnotify_file(struct file *file, __u32 mask)
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 /*
- * fsnotify_file_area_perm - permission hook before access to file range
+ * fsnotify_file_area_perm - permission hook before access/modify of file range
  */
 static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
 					  const loff_t *ppos, size_t count)
 {
-	__u32 fsnotify_mask = FS_ACCESS_PERM;
+	struct inode *inode = file_inode(file);
+	__u32 fsnotify_mask;
 
 	/*
 	 * filesystem may be modified in the context of permission events
@@ -147,7 +148,27 @@  static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
 	 */
 	lockdep_assert_once(file_write_not_started(file));
 
-	if (!(perm_mask & MAY_READ))
+	/*
+	 * Generate FS_PRE_ACCESS/FS_ACCESS_PERM as two seperate events.
+	 */
+	if (perm_mask & MAY_READ) {
+		int ret = fsnotify_file(file, FS_ACCESS_PERM);
+
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Pre-content events are only reported for regular files and dirs.
+	 */
+	if (!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode))
+		return 0;
+
+	if (perm_mask & MAY_WRITE)
+		fsnotify_mask = FS_PRE_MODIFY;
+	else if (perm_mask & MAY_READ)
+		fsnotify_mask = FS_PRE_ACCESS;
+	else
 		return 0;
 
 	return fsnotify_file(file, fsnotify_mask);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 8be029bc50b1..21e72b837ec5 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -56,6 +56,9 @@ 
 #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
 #define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
 
+#define FS_PRE_ACCESS		0x00100000	/* Pre-content access hook */
+#define FS_PRE_MODIFY		0x00200000	/* Pre-content modify hook */
+
 /*
  * Set on inode mark that cares about things that happen to its children.
  * Always set for dnotify and inotify.
@@ -77,8 +80,14 @@ 
  */
 #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME)
 
-#define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
-				  FS_OPEN_EXEC_PERM)
+/* Content events can be used to inspect file content */
+#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | \
+				      FS_ACCESS_PERM)
+/* Pre-content events can be used to fill file content */
+#define FSNOTIFY_PRE_CONTENT_EVENTS  (FS_PRE_ACCESS | FS_PRE_MODIFY)
+
+#define ALL_FSNOTIFY_PERM_EVENTS (FSNOTIFY_CONTENT_PERM_EVENTS | \
+				  FSNOTIFY_PRE_CONTENT_EVENTS)
 
 /*
  * This is a list of all events that may get sent to a parent that is watching
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 55c78c318ccd..2997edf3e7cd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3406,7 +3406,8 @@  static int selinux_path_notify(const struct path *path, u64 mask,
 		perm |= FILE__WATCH_WITH_PERM;
 
 	/* watches on read-like events need the file:watch_reads permission */
-	if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+	if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_PRE_ACCESS |
+		    FS_CLOSE_NOWRITE))
 		perm |= FILE__WATCH_READS;
 
 	return path_has_perm(current_cred(), path, perm);