linux-kernel - Re: [PATCH v2 07/16] VFS: introduce end_dirop() and end_dirop

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: <CAOQ4uxj37GYrg=wfPRSr-7meK_QOpRbefJ_sShuVpzVfb2iisQ@mail.gmail.com>
Date: Fri, 22 Aug 2025 11:30:15 +0200
From: Amir Goldstein <amir73il@...il.com>
To: NeilBrown <neil@...wn.name>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 07/16] VFS: introduce end_dirop() and end_dirop_mkdir()

On Fri, Aug 22, 2025 at 2:39 AM NeilBrown <neil@...wn.name> wrote:
>
> end_dirop() is the partner of start_dirop().  It drops the lock and

If they are partners I think it is better to introduce them together
in the same patch.

This goes for all the pairs that your series introduces.

It simply makes sense from review POV to be able to
verify that all callers have been properly converted.

> releases the reference on the dentry.
> It *is* exported and can be used by all callers.
>
> As vfs_mkdir() drops the dentry on error we cannot use end_dirop() as
> that won't unlock when the dentry IS_ERR().  For those cases we have
> end_dirop_mkdir().
>
> end_dirop() can always be called on the result of start_dirop(), but not
> after vfs_mkdir().
> end_dirop_mkdir() can only be called on the result of start_dirop() if
> that was not an error, and can also be called on the result of
> vfs_mkdir().

These are very confusing semantics.
I doubt these can hold for a long time,
but I guess if this is temporary then maybe...

>
> We can change vfs_mkdir() to drop the lock when it drops the dentry,
> end_dirop_mkdir() can be discarded.

Fixed some typos above ^

>
> Signed-off-by: NeilBrown <neil@...wn.name>
> ---
>  fs/namei.c            | 50 +++++++++++++++++++++++++++++++++++--------
>  include/linux/namei.h |  3 +++
>  2 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 4f1eddaff63f..8121550f20aa 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2778,6 +2778,43 @@ static struct dentry *start_dirop(struct dentry *parent, struct qstr *name,
>         return dentry;
>  }
>
> +/**
> + * end_dirop - signal completion of a dirop
> + * @de - the dentry which was returned by start_dirop or similar.
> + *
> + * If the de is an error, nothing happens. Otherwise any lock taken to
> + * protect the dentry is dropped and the dentry itself is release (dput()).
> + */
> +void end_dirop(struct dentry *de)
> +{
> +       if (!IS_ERR(de)) {
> +               inode_unlock(de->d_parent->d_inode);
> +               dput(de);
> +       }
> +}
> +EXPORT_SYMBOL(end_dirop);
> +
> +/**
> + * end_dirop_mkdir - signal completion of a dirop which could have been vfs_mkdir
> + * @de - the dentry which was returned by start_dirop or similar.
> + * @parent - the parent in which the mkdir happened.
> + *
> + * Because vfs_mkdir() dput()s the dentry on failure, end_dirop() cannot be
> + * used with it.  Instead this function must be used, and it must not be caller
> + * if the original lookup failed.
> + *
> + * If de is an error the parent is unlocked, else this behaves the same as
> + * end_dirop().
> + */
> +void end_dirop_mkdir(struct dentry *de, struct dentry *parent)
> +{
> +       if (IS_ERR(de))
> +               inode_unlock(parent->d_inode);
> +       else
> +               end_dirop(de);
> +}
> +EXPORT_SYMBOL(end_dirop_mkdir);
> +
>  /* does lookup, returns the object with parent locked */
>  static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct path *path)
>  {
> @@ -4174,9 +4211,8 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>
>         return dentry;
>  fail:
> -       dput(dentry);
> +       end_dirop(dentry);
>         dentry = ERR_PTR(error);
> -       inode_unlock(path->dentry->d_inode);
>  out_drop_write:
>         if (!error)
>                 mnt_drop_write(path->mnt);
> @@ -4198,9 +4234,7 @@ EXPORT_SYMBOL(kern_path_create);
>
>  void done_path_create(struct path *path, struct dentry *dentry)
>  {
> -       if (!IS_ERR(dentry))
> -               dput(dentry);
> -       inode_unlock(path->dentry->d_inode);
> +       end_dirop_mkdir(dentry, path->dentry);

Like here we have end_dirop_mkdir() after operations that
are certainly not mkdir.

It's setting developers to fail IMO.

Thanks,
Amir.