Re: [PATCH v8 02/19] fsnotify: opt-in for permission events at file open time

[Jan Kara <jack@suse.cz>]

On Wed 20-11-24 17:12:21, Amir Goldstein wrote:
> On Wed, Nov 20, 2024 at 4:53 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 15-11-24 10:30:15, Josef Bacik wrote:
> > > From: Amir Goldstein <amir73il@gmail.com>
> > >
> > > 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@linux-foundation.org>
> > > Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wj8L=mtcRTi=NECHMGfZQgXOp_uix1YVh04fEmrKaMnXA@mail.gmail.com/
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > FWIW I've ended up somewhat massaging this patch (see below).
> >
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index 23bd058576b1..8e5c783013d2 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 was opened by fanotify and shouldn't generate fanotify events */
> > > -#define FMODE_NONOTIFY               ((__force fmode_t)(1 << 26))
> > > +/* File shouldn't generate fanotify pre-content events */
> > > +#define FMODE_NONOTIFY_HSM   ((__force fmode_t)(1 << 25))
> > > +
> > > +/* File shouldn't generate fanotify permission events */
> > > +#define FMODE_NONOTIFY_PERM  ((__force fmode_t)(1 << 26))
> >
> > Firstly, I've kept FMODE_NONOTIFY to stay a single bit instead of two bit
> > constant. I've seen too many bugs caused by people expecting the constant
> > has a single bit set when it actually had more in my life. So I've ended up
> > with:
> >
> > +/*
> > + * Together with FMODE_NONOTIFY_PERM defines which fsnotify events shouldn't be
> > + * generated (see below)
> > + */
> > +#define FMODE_NONOTIFY         ((__force fmode_t)(1 << 25))
> > +
> > +/*
> > + * Together with FMODE_NONOTIFY defines which fsnotify events shouldn't be
> > + * generated (see below)
> > + */
> > +#define FMODE_NONOTIFY_PERM    ((__force fmode_t)(1 << 26))
> >
> > and
> >
> > +/*
> > + * The two FMODE_NONOTIFY* define which fsnotify events should not be generated
> > + * for a file. These are the possible values of (f->f_mode &
> > + * FMODE_FSNOTIFY_MASK) and their meaning:
> > + *
> > + * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> > + * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> > + * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
> > + */
> > +#define FMODE_FSNOTIFY_MASK \
> > +       (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM)
> > +
> > +#define FMODE_FSNOTIFY_NONE(mode) \
> > +       ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> > +#define FMODE_FSNOTIFY_PERM(mode) \
> > +       (!(mode & FMODE_NONOTIFY_PERM))
> 
> That looks incorrect -
> It gives the wrong value for FMODE_NONOTIFY | FMODE_NONOTIFY_PERM
> 
> should be:
> != FMODE_NONOTIFY_PERM &&
> != FMODE_NONOTIFY
> 
> The simplicity of the single bit test is for permission events
> is why I chose my model, but I understand your reasoning.

Ah, thanks for catching this! I've fixed this to:

+#define FMODE_FSNOTIFY_PERM(mode) \
+       ((mode & FMODE_FSNOTIFY_MASK) == 0 || \
+        (mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))

It is not a single bit test so it ends up being:

   0x0000000060180345 <+101>:	mov    0x20(%r12),%edx
   0x000000006018034a <+106>:	and    $0x6000000,%edx
   0x0000000060180350 <+112>:	je     0x6018035a <rw_verify_area+122>
   0x0000000060180352 <+114>:	cmp    $0x6000000,%edx
   0x0000000060180358 <+120>:	jne    0x6018032e <rw_verify_area+78>

But I guess that's not terrible either.

> > +#define FMODE_FSNOTIFY_HSM(mode) \
> > +       ((mode & FMODE_FSNOTIFY_MASK) == 0)
> >
> > Also I've moved file_set_fsnotify_mode() out of line into fsnotify.c. The
> > function gets quite big and the call is not IMO so expensive to warrant
> > inlining. Furthermore it saves exporting some fsnotify internals to modules
> > (in later patches).
> 
> Sounds good.
> Since you wanted to refrain from defining a two bit constant,
> I wonder how you annotated for NONOTIFY_HSM case
> 
>    return FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;

I'm not sure I understand. What do you mean by "annotated"?

It is not that I object to "two bit constants". FMODE_FSNOTIFY_MASK is a
two-bit constant and a good one. But the name clearly suggests it is not a
single bit constant. When you have all FMODE_FOO and FMODE_BAR things
single bit except for FMODE_BAZ which is multi-bit, then this is IMHO a
recipe for problems and I rather prefer explicitely spelling the
combination out as FMODE_NONOTIFY | FMODE_NONOTIFY_PERM in the few places
that need this instead of hiding it behind some other name.

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