linux-kernel - Re: [PATCH v4 2/5] fsnotify: Use d_find_any_alias to get dentry associated with inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxhXYzX=tKyY7BRp=KhuEZY6FrYsZYyD-rLiYfPJfh8WXg@mail.gmail.com>
Date:   Sat, 12 Nov 2022 10:53:21 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     Stephen Brennan <stephen.s.brennan@...cle.com>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 2/5] fsnotify: Use d_find_any_alias to get dentry
 associated with inode

On Sat, Nov 12, 2022 at 12:06 AM Stephen Brennan
<stephen.s.brennan@...cle.com> wrote:
>
> Rather than iterating over the inode's i_dentry (requiring holding the
> i_lock for the entire duration of the function), we know that there
> should be only one item in the list. Use d_find_any_alias() and no
> longer hold i_lock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@...cle.com>
> Reviewed-by: Amir Goldstein <amir73il@...il.com>
> ---
>
> Notes:
>     Changes in v4:
>     - Bail out if d_find_any_alias() returns NULL
>     - Rebase on Amir's patch
>     Changes in v3:
>     - Add newlines in block comment
>     - d_find_any_alias() returns a reference, which I was leaking. Add
>       a dput(alias) at the end.
>     - Add Amir's R-b
>
>  fs/notify/fsnotify.c | 46 ++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 2c50e9e50d35..409d479cbbc6 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -105,35 +105,35 @@ void fsnotify_sb_delete(struct super_block *sb)
>   */
>  void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched)
>  {
> -       struct dentry *alias;
> +       struct dentry *alias, *child;
>
>         if (!S_ISDIR(inode->i_mode))
>                 return;
>
> -       spin_lock(&inode->i_lock);
> -       /* run all of the dentries associated with this inode.  Since this is a
> -        * directory, there damn well better only be one item on this list */
> -       hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
> -               struct dentry *child;
> -
> -               /* run all of the children of the original inode and fix their
> -                * d_flags to indicate parental interest (their parent is the
> -                * original inode) */
> -               spin_lock(&alias->d_lock);
> -               list_for_each_entry(child, &alias->d_subdirs, d_child) {
> -                       if (!child->d_inode)
> -                               continue;
> +       /* Since this is a directory, there damn well better only be one child */
> +       alias = d_find_any_alias(inode);
> +       if (!alias)
> +               return;
>
> -                       spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> -                       if (watched)
> -                               child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> -                       else
> -                               child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> -                       spin_unlock(&child->d_lock);
> -               }
> -               spin_unlock(&alias->d_lock);
> +       /*
> +        * run all of the children of the original inode and fix their
> +        * d_flags to indicate parental interest (their parent is the
> +        * original inode)

nit: this comment can probably fit in two nicer lines

> +        */
> +       spin_lock(&alias->d_lock);
> +       list_for_each_entry(child, &alias->d_subdirs, d_child) {
> +               if (!child->d_inode)
> +                       continue;
> +
> +               spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
> +               if (watched)
> +                       child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> +               else
> +                       child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> +               spin_unlock(&child->d_lock);
>         }
> -       spin_unlock(&inode->i_lock);
> +       spin_unlock(&alias->d_lock);
> +       dput(alias);
>  }
>
>  /*
> --
> 2.34.1
>