Re: [PATCH 03/19] fsnotify: check if file is actually being watched for pre-content events on open

[Jan Kara <jack@suse.cz>]

On Thu 21-11-24 17:01:13, Amir Goldstein wrote:
> On Thu, Nov 21, 2024 at 12:22 PM Jan Kara <jack@suse.cz> wrote:
> >
> > From: Amir Goldstein <amir73il@gmail.com>
> >
> > So far, we set FMODE_NONOTIFY_ flags at open time if we know that there
> > are no permission event watchers at all on the filesystem, but lack of
> > FMODE_NONOTIFY_ flags does not mean that the file is actually watched.
> >
> > For pre-content events, it is possible to optimize things so that we
> > don't bother trying to send pre-content events if file was not watched
> > (through sb, mnt, parent or inode itself) on open. Set FMODE_NONOTIFY_
> > flags according to that.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > Link: https://patch.msgid.link/2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@toxicpanda.com
> > ---
> >  fs/notify/fsnotify.c             | 27 +++++++++++++++++++++++++--
> >  include/linux/fsnotify_backend.h |  3 +++
> >  2 files changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index 569ec356e4ce..dd1dffd89fd6 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -193,7 +193,7 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask,
> >         return mask & marks_mask;
> >  }
> >
> > -/* Are there any inode/mount/sb objects that are interested in this event? */
> > +/* Are there any inode/mount/sb objects that watch for these events? */
> >  static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
> >                                            __u32 mask)
> >  {
> > @@ -632,7 +632,9 @@ EXPORT_SYMBOL_GPL(fsnotify);
> >   */
> >  void file_set_fsnotify_mode(struct file *file)
> >  {
> > -       struct super_block *sb = file->f_path.dentry->d_sb;
> > +       struct dentry *dentry = file->f_path.dentry, *parent;
> > +       struct super_block *sb = dentry->d_sb;
> > +       __u32 mnt_mask, p_mask;
> >
> >         /* Is it a file opened by fanotify? */
> >         if (FMODE_FSNOTIFY_NONE(file->f_mode))
> > @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file)
> >                 file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> >                 return;
> >         }
> 
> This was lost in translation:
> 
> @@ -672,8 +672,10 @@ void file_set_fsnotify_mode(struct file *file)
>         /*
>          * If there are permission event watchers but no pre-content event
>          * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> +        * Pre-content events are only reported for regular files and dirs.
>          */
> -       if (likely(!fsnotify_sb_has_priority_watchers(sb,
> +       if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
> +           likely(!fsnotify_sb_has_priority_watchers(sb,
>                                                 FSNOTIFY_PRIO_PRE_CONTENT))) {
>                 file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
>                 return;

Right. Specifically the (!d_is_dir(dentry) && !d_is_reg(dentry)) got lost,
likely when I moved stuff between different commits and then was resolving
patch conflicts. I'll add it back. Thanks for finding this!

> > +
> > +       /*
> > +        * OK, there are some pre-content watchers. Check if anybody can be
> > +        * watching for pre-content events on *this* file.
> > +        */
> > +       mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > +       if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) &&
> > +           !fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > +                                    FSNOTIFY_PRE_CONTENT_EVENTS))) {
> > +               file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> > +               return;
> > +       }
> > +
> > +       /* Even parent is not watching for pre-content events on this file? */
> > +       parent = dget_parent(dentry);
> > +       p_mask = fsnotify_inode_watches_children(d_inode(parent));
> > +       dput(parent);
> > +       if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
> > +               file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> > +               return;
> > +       }
> 
> inlining broke the logic and your branch fails the new PRE_ACCESS
> test cases of fanotify03 LTP test (now pushed to branch fan_hsm).
> 
> Specifically in the test case that fails, parent is not watching and
> inode is watching pre-content and your code gets to the p_mask
> test and marks this file as no-pre-content watchers.
> 
> This passes the test:
> 
> @@ -684,17 +686,18 @@ void file_set_fsnotify_mode(struct file *file)
>          * watching for pre-content events on *this* file.
>          */
>         mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> -       if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) &&
> -           !fsnotify_object_watched(d_inode(dentry), mnt_mask,
> -                                    FSNOTIFY_PRE_CONTENT_EVENTS))) {
> -               file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> +       if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask,
> +                                            FSNOTIFY_PRE_CONTENT_EVENTS))) {
>                 return;
>         }
> 
>         /* Even parent is not watching for pre-content events on this file? */
> -       parent = dget_parent(dentry);
> -       p_mask = fsnotify_inode_watches_children(d_inode(parent));
> -       dput(parent);
> +       p_mask = 0;
> +       if (unlikely(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) {
> +               parent = dget_parent(dentry);
> +               p_mask = fsnotify_inode_watches_children(d_inode(parent));
> +               dput(parent);
> +       }
>         if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
>                 file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
>                 return;

Right. I've fixed that up. Thanks!

LTP from your fan_hsm branch is now passing so I've re-pushed the current
state to my fsnotify_hsm branch.

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