Message-ID: <CAOQ4uxgNoDUcgSjo=oK6QmzCuEdgauDs2H6WGbD=RuzkZX115Q@mail.gmail.com>
Date: Thu, 14 Nov 2024 18:22:25 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Josef Bacik <josef@...icpanda.com>, 
	kernel-team@...com, linux-fsdevel@...r.kernel.org, brauner@...nel.org, 
	linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, 
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events

On Thu, Nov 14, 2024 at 4:01 PM Jan Kara <jack@...e.cz> wrote:
>
> On Wed 13-11-24 19:49:31, Amir Goldstein wrote:
> > From 7a2cd74654a53684d545b96c57c9091e420b3add Mon Sep 17 00:00:00 2001
> > From: Amir Goldstein <amir73il@...il.com>
> > Date: Tue, 12 Nov 2024 13:46:08 +0100
> > Subject: [PATCH] fsnotify: opt-in for permission events at file open time
> >
> > Legacy inotify/fanotify listeners can add watches for events on inode,
> > parent or mount and expect to get events (e.g. FS_MODIFY) on files that
> > were already open at the time of setting up the watches.
> >
> > fanotify permission events are typically used by Anti-malware sofware,
> > that is watching the entire mount and it is not common to have more that
> > one Anti-malware engine installed on a system.
> >
> > To reduce the overhead of the fsnotify_file_perm() hooks on every file
> > access, relax the semantics of the legacy FAN_ACCESS_PERM event to generate
> > events only if there were *any* permission event listeners on the
> > filesystem at the time that the file was opened.
> >
> > The new semantic is implemented by extending the FMODE_NONOTIFY bit into
> > two FMODE_NONOTIFY_* bits, that are used to store a mode for which of the
> > events types to report.
> >
> > This is going to apply to the new fanotify pre-content events in order
> > to reduce the cost of the new pre-content event vfs hooks.
> >
> > Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wj8L=mtcRTi=NECHMGfZQgXOp_uix1YVh04fEmrKaMnXA@mail.gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@...il.com>
>
> Couple of notes below.
>
> > diff --git a/fs/open.c b/fs/open.c
> > index 226aca8c7909..194c2c8d8cd4 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -901,7 +901,7 @@ static int do_dentry_open(struct file *f,
> >       f->f_sb_err = file_sample_sb_err(f);
> >
> >       if (unlikely(f->f_flags & O_PATH)) {
> > -             f->f_mode = FMODE_PATH | FMODE_OPENED;
> > +             f->f_mode = FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY;
> >               f->f_op = &empty_fops;
> >               return 0;
> >       }
> > @@ -929,6 +929,12 @@ static int do_dentry_open(struct file *f,
> >       if (error)
> >               goto cleanup_all;
> >
> > +     /*
> > +      * Set FMODE_NONOTIFY_* bits according to existing permission watches.
> > +      * If FMODE_NONOTIFY was already set for an fanotify fd, this doesn't
> > +      * change anything.
> > +      */
> > +     f->f_mode |= fsnotify_file_mode(f);
>
> Maybe it would be obvious to do this like:
>
>         file_set_fsnotify_mode(f);
>
> Because currently this depends on the details of how exactly FMODE_NONOTIFY
> is encoded.
>

ok. makes sense.

> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 70359dd669ff..dd583ce7dba8 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -173,13 +173,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> >
> >  #define      FMODE_NOREUSE           ((__force fmode_t)(1 << 23))
> >
> > -/* FMODE_* bit 24 */
> > -
> >  /* File is embedded in backing_file object */
> > -#define FMODE_BACKING                ((__force fmode_t)(1 << 25))
> > +#define FMODE_BACKING                ((__force fmode_t)(1 << 24))
> > +
> > +/* File shouldn't generate fanotify pre-content events */
> > +#define FMODE_NONOTIFY_HSM   ((__force fmode_t)(1 << 25))
> >
> > -/* File was opened by fanotify and shouldn't generate fanotify events */
> > -#define FMODE_NONOTIFY               ((__force fmode_t)(1 << 26))
> > +/* File shouldn't generate fanotify permission events */
> > +#define FMODE_NONOTIFY_PERM  ((__force fmode_t)(1 << 26))
> >
> >  /* File is capable of returning -EAGAIN if I/O will block */
> >  #define FMODE_NOWAIT         ((__force fmode_t)(1 << 27))
> > @@ -190,6 +191,21 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> >  /* File does not contribute to nr_files count */
> >  #define FMODE_NOACCOUNT              ((__force fmode_t)(1 << 29))
> >
> > +/*
> > + * The two FMODE_NONOTIFY_ bits used together have a special meaning of
> > + * not reporting any events at all including non-permission events.
> > + * These are the possible values of FMODE_NOTIFY(f->f_mode) and their meaning:
> > + *
> > + * FMODE_NONOTIFY_HSM - suppress only pre-content events.
> > + * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> > + * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> > + */
> > +#define FMODE_NONOTIFY_MASK \
> > +     (FMODE_NONOTIFY_HSM | FMODE_NONOTIFY_PERM)
> > +#define FMODE_NONOTIFY FMODE_NONOTIFY_MASK
> > +#define FMODE_NOTIFY(mode) \
> > +     ((mode) & FMODE_NONOTIFY_MASK)
>
> This looks a bit error-prone to me (FMODE_NONOTIFY looks like another FMODE
> flag but in fact it is not which is an invitation for subtle bugs) and the
> tests below which are sometimes done as (FMODE_NOTIFY(mode) == xxx) and
> sometimes as (file->f_mode & xxx) are inconsistent and confusing (unless you
> understand what's happening under the hood).
>
> So how about defining macros like FMODE_FSNOTIFY_NORMAL(),
> FMODE_FSNOTIFY_CONTENT() and FMODE_FSNOTIFY_PRE_CONTENT() which evaluate to
> true if we should be sending normal/content/pre-content events to the file.
> With appropriate comments this should make things more obvious.
>

ok, maybe something like this:

#define FMODE_FSNOTIFY_NONE(mode) \
        (FMODE_FSNOTIFY(mode) == FMODE_NONOTIFY)
#define FMODE_FSNOTIFY_NORMAL(mode) \
        (FMODE_FSNOTIFY(mode) == FMODE_NONOTIFY_PERM)
#define FMODE_FSNOTIFY_PERM(mode) \
        (!((mode) & FMODE_NONOTIFY_PERM))
#define FMODE_FSNOTIFY_HSM(mode) \
        (FMODE_FSNOTIFY(mode) == 0)

At least keeping the double negatives contained in one place.

And then we have these users in the final code:

static inline bool fsnotify_file_has_pre_content_watches(struct file *file)
{
        return file && unlikely(FMODE_FSNOTIFY_HSM(file->f_mode));
}

static inline int fsnotify_open_perm(struct file *file)
{
        int ret;

        if (likely(!FMODE_FSNOTIFY_PERM(file->f_mode)))
                return 0;
...

static inline int fsnotify_file(struct file *file, __u32 mask)
{
        if (FMODE_FSNOTIFY_NONE(file->f_mode))
                return 0;
...

BTW, I prefer using PERM,HSM instead of the FSNOTIFY_PRIO_
names for brevity, but also because at the moment of this patch
FMODE_NONOTIFY_PERM means "suppress permission events
if there are no listeners with priority >= FSNOTIFY_PRIO_CONTENT
at all on any objects of the filesystem".

It does NOT mean that there ARE permission events watchers on the file's
sb/mnt/inode or parent, but what the users of the flag care about really is
whether the specific file is being watched for permission events.

I was contemplating if we should add the following check at open time
as following patches add for pre-content watchers also for
permission watchers on the specific file:

        /*
         * Permission events is a super set of pre-content events, so if there
         * are no permission event watchers, there are also no pre-content event
         * watchers and this is implied from the single FMODE_NONOTIFY_PERM bit.
         */
        if (likely(!fsnotify_sb_has_priority_watchers(sb,
                                                FSNOTIFY_PRIO_CONTENT)))
                return FMODE_NONOTIFY_PERM;

+        /*
+         * There are content watchers in the filesystem, but are there
+         * permission event watchers on this specific file?
+         */
+        if (likely(!fsnotify_file_object_watched(file,
+                                                 ALL_FSNOTIFY_PERM_EVENTS)))
+                return FMODE_NONOTIFY_PERM;
+

I decided not to stretch the behavior change too much and also since
Anti-malware permission watchers often watch all the mounts of a
filesystem, there is probably little to gain from this extra check.
But we can reconsider this in the future.

WDYT?

In any case, IMO the language of FMODE_FSNOTIFY_PERM() matches
the meaning of the users better and makes the code easier to understand.

FMODE_FSNOTIFY_HSM() is debatable, but at least it is short ;)

Anyway, I will send v2 with your suggestions.

Thanks,
Amir.

Powered by blists - more mailing lists