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

[Amir Goldstein <amir73il@xxxxxxxxx>]

On Thu, Nov 21, 2024 at 12:22 PM Jan Kara <jack@xxxxxxx> wrote:
>
> From: Amir Goldstein <amir73il@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> Link: https://patch.msgid.link/2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@xxxxxxxxxxxxxx
> ---
>  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;

> +
> +       /*
> +        * 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;

Thanks,
Amir.