From: Amir Goldstein <amir73il@gmail.com>
To: NeilBrown <neil@brown.name>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
Christian Brauner <brauner@kernel.org>,
Jeff Layton <jlayton@kernel.org>, Jan Kara <jack@suse.cz>,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 12/14] ecryptfs: use new start_creating/start_removing APIs
Date: Sun, 19 Oct 2025 12:38:12 +0200 [thread overview]
Message-ID: <CAOQ4uxjQsAT6HRdt0dP-xws2_pVcB8Ou_iTACOj6OqRG5fCSZg@mail.gmail.com> (raw)
In-Reply-To: <20251015014756.2073439-13-neilb@ownmail.net>
On Wed, Oct 15, 2025 at 3:49 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> This requires the addition of start_creating_dentry() which is given the
> dentry which has already been found, and asks for it to be locked and
> its parent validated.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/ecryptfs/inode.c | 153 ++++++++++++++++++++----------------------
> fs/namei.c | 33 +++++++++
> include/linux/namei.h | 2 +
> 3 files changed, 107 insertions(+), 81 deletions(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index ed1394da8d6b..b3702105d236 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -24,18 +24,26 @@
> #include <linux/unaligned.h>
> #include "ecryptfs_kernel.h"
>
> -static int lock_parent(struct dentry *dentry,
> - struct dentry **lower_dentry,
> - struct inode **lower_dir)
> +static struct dentry *ecryptfs_start_creating_dentry(struct dentry *dentry)
> {
> - struct dentry *lower_dir_dentry;
> + struct dentry *parent = dget_parent(dentry->d_parent);
> + struct dentry *ret;
>
> - lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);
> - *lower_dir = d_inode(lower_dir_dentry);
> - *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> + ret = start_creating_dentry(ecryptfs_dentry_to_lower(parent),
> + ecryptfs_dentry_to_lower(dentry));
> + dput(parent);
> + return ret;
> +}
>
> - inode_lock_nested(*lower_dir, I_MUTEX_PARENT);
> - return (*lower_dentry)->d_parent == lower_dir_dentry ? 0 : -EINVAL;
> +static struct dentry *ecryptfs_start_removing_dentry(struct dentry *dentry)
> +{
> + struct dentry *parent = dget_parent(dentry->d_parent);
> + struct dentry *ret;
> +
> + ret = start_removing_dentry(ecryptfs_dentry_to_lower(parent),
> + ecryptfs_dentry_to_lower(dentry));
> + dput(parent);
> + return ret;
> }
>
> static int ecryptfs_inode_test(struct inode *inode, void *lower_inode)
> @@ -141,15 +149,12 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
> struct inode *lower_dir;
> int rc;
>
> - rc = lock_parent(dentry, &lower_dentry, &lower_dir);
> - dget(lower_dentry); // don't even try to make the lower negative
> - if (!rc) {
> - if (d_unhashed(lower_dentry))
> - rc = -EINVAL;
> - else
> - rc = vfs_unlink(&nop_mnt_idmap, lower_dir, lower_dentry,
> - NULL);
> - }
> + lower_dentry = ecryptfs_start_removing_dentry(dentry);
> + if (IS_ERR(lower_dentry))
> + return PTR_ERR(lower_dentry);
> +
> + lower_dir = lower_dentry->d_parent->d_inode;
> + rc = vfs_unlink(&nop_mnt_idmap, lower_dir, lower_dentry, NULL);
> if (rc) {
> printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
> goto out_unlock;
> @@ -158,8 +163,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
> set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
> inode_set_ctime_to_ts(inode, inode_get_ctime(dir));
> out_unlock:
> - dput(lower_dentry);
> - inode_unlock(lower_dir);
> + end_removing(lower_dentry);
> if (!rc)
> d_drop(dentry);
> return rc;
> @@ -186,10 +190,12 @@ ecryptfs_do_create(struct inode *directory_inode,
> struct inode *lower_dir;
> struct inode *inode;
>
> - rc = lock_parent(ecryptfs_dentry, &lower_dentry, &lower_dir);
> - if (!rc)
> - rc = vfs_create(&nop_mnt_idmap, lower_dir,
> - lower_dentry, mode, true);
> + lower_dentry = ecryptfs_start_creating_dentry(ecryptfs_dentry);
> + if (IS_ERR(lower_dentry))
> + return ERR_CAST(lower_dentry);
> + lower_dir = lower_dentry->d_parent->d_inode;
> + rc = vfs_create(&nop_mnt_idmap, lower_dir,
> + lower_dentry, mode, true);
> if (rc) {
> printk(KERN_ERR "%s: Failure to create dentry in lower fs; "
> "rc = [%d]\n", __func__, rc);
> @@ -205,7 +211,7 @@ ecryptfs_do_create(struct inode *directory_inode,
> fsstack_copy_attr_times(directory_inode, lower_dir);
> fsstack_copy_inode_size(directory_inode, lower_dir);
> out_lock:
> - inode_unlock(lower_dir);
> + end_creating(lower_dentry, NULL);
> return inode;
> }
>
> @@ -433,10 +439,12 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,
>
> file_size_save = i_size_read(d_inode(old_dentry));
> lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
> - rc = lock_parent(new_dentry, &lower_new_dentry, &lower_dir);
> - if (!rc)
> - rc = vfs_link(lower_old_dentry, &nop_mnt_idmap, lower_dir,
> - lower_new_dentry, NULL);
> + lower_new_dentry = ecryptfs_start_creating_dentry(new_dentry);
> + if (IS_ERR(lower_new_dentry))
> + return PTR_ERR(lower_new_dentry);
> + lower_dir = lower_new_dentry->d_parent->d_inode;
> + rc = vfs_link(lower_old_dentry, &nop_mnt_idmap, lower_dir,
> + lower_new_dentry, NULL);
> if (rc || d_really_is_negative(lower_new_dentry))
> goto out_lock;
> rc = ecryptfs_interpose(lower_new_dentry, new_dentry, dir->i_sb);
> @@ -448,7 +456,7 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,
> ecryptfs_inode_to_lower(d_inode(old_dentry))->i_nlink);
> i_size_write(d_inode(new_dentry), file_size_save);
> out_lock:
> - inode_unlock(lower_dir);
> + end_creating(lower_new_dentry, NULL);
> return rc;
> }
>
> @@ -468,9 +476,11 @@ static int ecryptfs_symlink(struct mnt_idmap *idmap,
> size_t encoded_symlen;
> struct ecryptfs_mount_crypt_stat *mount_crypt_stat = NULL;
>
> - rc = lock_parent(dentry, &lower_dentry, &lower_dir);
> - if (rc)
> - goto out_lock;
> + lower_dentry = ecryptfs_start_creating_dentry(dentry);
> + if (IS_ERR(lower_dentry))
> + return PTR_ERR(lower_dentry);
> + lower_dir = lower_dentry->d_parent->d_inode;
> +
> mount_crypt_stat = &ecryptfs_superblock_to_private(
> dir->i_sb)->mount_crypt_stat;
> rc = ecryptfs_encrypt_and_encode_filename(&encoded_symname,
> @@ -490,7 +500,7 @@ static int ecryptfs_symlink(struct mnt_idmap *idmap,
> fsstack_copy_attr_times(dir, lower_dir);
> fsstack_copy_inode_size(dir, lower_dir);
> out_lock:
> - inode_unlock(lower_dir);
> + end_creating(lower_dentry, NULL);
> if (d_really_is_negative(dentry))
> d_drop(dentry);
> return rc;
> @@ -501,12 +511,14 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> {
> int rc;
> struct dentry *lower_dentry;
> + struct dentry *lower_dir_dentry;
> struct inode *lower_dir;
>
> - rc = lock_parent(dentry, &lower_dentry, &lower_dir);
> - if (rc)
> - goto out;
> -
> + lower_dentry = ecryptfs_start_creating_dentry(dentry);
> + if (IS_ERR(lower_dentry))
> + return lower_dentry;
> + lower_dir_dentry = dget(lower_dentry->d_parent);
> + lower_dir = lower_dir_dentry->d_inode;
> lower_dentry = vfs_mkdir(&nop_mnt_idmap, lower_dir,
> lower_dentry, mode);
> rc = PTR_ERR(lower_dentry);
> @@ -522,7 +534,7 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> fsstack_copy_inode_size(dir, lower_dir);
> set_nlink(dir, lower_dir->i_nlink);
> out:
> - inode_unlock(lower_dir);
> + end_creating(lower_dentry, lower_dir_dentry);
> if (d_really_is_negative(dentry))
> d_drop(dentry);
> return ERR_PTR(rc);
> @@ -534,21 +546,18 @@ static int ecryptfs_rmdir(struct inode *dir, struct dentry *dentry)
> struct inode *lower_dir;
> int rc;
>
> - rc = lock_parent(dentry, &lower_dentry, &lower_dir);
> - dget(lower_dentry); // don't even try to make the lower negative
> - if (!rc) {
> - if (d_unhashed(lower_dentry))
> - rc = -EINVAL;
> - else
> - rc = vfs_rmdir(&nop_mnt_idmap, lower_dir, lower_dentry);
> - }
> + lower_dentry = ecryptfs_start_removing_dentry(dentry);
> + if (IS_ERR(lower_dentry))
> + return PTR_ERR(lower_dentry);
> + lower_dir = lower_dentry->d_parent->d_inode;
> +
> + rc = vfs_rmdir(&nop_mnt_idmap, lower_dir, lower_dentry);
> if (!rc) {
> clear_nlink(d_inode(dentry));
> fsstack_copy_attr_times(dir, lower_dir);
> set_nlink(dir, lower_dir->i_nlink);
> }
> - dput(lower_dentry);
> - inode_unlock(lower_dir);
> + end_removing(lower_dentry);
> if (!rc)
> d_drop(dentry);
> return rc;
> @@ -562,10 +571,12 @@ ecryptfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> struct dentry *lower_dentry;
> struct inode *lower_dir;
>
> - rc = lock_parent(dentry, &lower_dentry, &lower_dir);
> - if (!rc)
> - rc = vfs_mknod(&nop_mnt_idmap, lower_dir,
> - lower_dentry, mode, dev);
> + lower_dentry = ecryptfs_start_creating_dentry(dentry);
> + if (IS_ERR(lower_dentry))
> + return PTR_ERR(lower_dentry);
> + lower_dir = lower_dentry->d_parent->d_inode;
> +
> + rc = vfs_mknod(&nop_mnt_idmap, lower_dir, lower_dentry, mode, dev);
> if (rc || d_really_is_negative(lower_dentry))
> goto out;
> rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb);
> @@ -574,7 +585,7 @@ ecryptfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> fsstack_copy_attr_times(dir, lower_dir);
> fsstack_copy_inode_size(dir, lower_dir);
> out:
> - inode_unlock(lower_dir);
> + end_removing(lower_dentry);
> if (d_really_is_negative(dentry))
> d_drop(dentry);
> return rc;
> @@ -590,7 +601,6 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> struct dentry *lower_new_dentry;
> struct dentry *lower_old_dir_dentry;
> struct dentry *lower_new_dir_dentry;
> - struct dentry *trap;
> struct inode *target_inode;
> struct renamedata rd = {};
>
> @@ -605,31 +615,13 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>
> target_inode = d_inode(new_dentry);
>
> - trap = lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> - if (IS_ERR(trap))
> - return PTR_ERR(trap);
> - dget(lower_new_dentry);
> - rc = -EINVAL;
> - if (lower_old_dentry->d_parent != lower_old_dir_dentry)
> - goto out_lock;
> - if (lower_new_dentry->d_parent != lower_new_dir_dentry)
> - goto out_lock;
> - if (d_unhashed(lower_old_dentry) || d_unhashed(lower_new_dentry))
> - goto out_lock;
> - /* source should not be ancestor of target */
> - if (trap == lower_old_dentry)
> - goto out_lock;
> - /* target should not be ancestor of source */
> - if (trap == lower_new_dentry) {
> - rc = -ENOTEMPTY;
> - goto out_lock;
> - }
> + rd.mnt_idmap = &nop_mnt_idmap;
> + rd.old_parent = lower_old_dir_dentry;
> + rd.new_parent = lower_new_dir_dentry;
> + rc = start_renaming_two_dentries(&rd, lower_old_dentry, lower_new_dentry);
> + if (rc)
> + return rc;
>
> - rd.mnt_idmap = &nop_mnt_idmap;
> - rd.old_parent = lower_old_dir_dentry;
> - rd.old_dentry = lower_old_dentry;
> - rd.new_parent = lower_new_dir_dentry;
> - rd.new_dentry = lower_new_dentry;
> rc = vfs_rename(&rd);
> if (rc)
> goto out_lock;
> @@ -640,8 +632,7 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
> if (new_dir != old_dir)
> fsstack_copy_attr_all(old_dir, d_inode(lower_old_dir_dentry));
> out_lock:
> - dput(lower_new_dentry);
> - unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
> + end_renaming(&rd);
> return rc;
> }
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0a5261640ae5..91e484dbc239 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3397,6 +3397,39 @@ struct dentry *start_removing_noperm(struct dentry *parent,
> }
> EXPORT_SYMBOL(start_removing_noperm);
>
> +/**
> + * start_creating_dentry - prepare to create a given dentry
> + * @parent: directory from which dentry should be removed
> + * @child: the dentry to be removed
> + *
> + * A lock is taken to protect the dentry again other dirops and
> + * the validity of the dentry is checked: correct parent and still hashed.
> + *
> + * If the dentry is valid and negative a reference is taken and
> + * returned. If not an error is returned.
> + *
> + * end_creating() should be called when creation is complete, or aborted.
> + *
> + * Returns: the valid dentry, or an error.
> + */
> +struct dentry *start_creating_dentry(struct dentry *parent,
> + struct dentry *child)
> +{
> + inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> + if (unlikely(IS_DEADDIR(parent->d_inode) ||
> + child->d_parent != parent ||
> + d_unhashed(child))) {
> + inode_unlock(parent->d_inode);
> + return ERR_PTR(-EINVAL);
> + }
> + if (d_is_positive(child)) {
> + inode_unlock(parent->d_inode);
> + return ERR_PTR(-EEXIST);
> + }
> + return dget(child);
> +}
> +EXPORT_SYMBOL(start_creating_dentry);
> +
> /**
> * start_removing_dentry - prepare to remove a given dentry
> * @parent: directory from which dentry should be removed
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index a99ac8b7e24a..208aed1d6728 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -100,6 +100,8 @@ struct dentry *start_removing_killable(struct mnt_idmap *idmap,
> struct qstr *name);
> struct dentry *start_creating_noperm(struct dentry *parent, struct qstr *name);
> struct dentry *start_removing_noperm(struct dentry *parent, struct qstr *name);
> +struct dentry *start_creating_dentry(struct dentry *parent,
> + struct dentry *child);
> struct dentry *start_removing_dentry(struct dentry *parent,
> struct dentry *child);
>
> --
> 2.50.0.107.gf914562f5916.dirty
>
next prev parent reply other threads:[~2025-10-19 10:38 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-15 1:46 [PATCH v2 00/14] Create and use APIs to centralise locking for directory ops NeilBrown
2025-10-15 1:46 ` [PATCH v2 01/14] debugfs: rename end_creating() to debugfs_end_creating() NeilBrown
2025-10-21 13:26 ` Christian Brauner
2025-10-15 1:46 ` [PATCH v2 02/14] VFS: introduce start_dirop() and end_dirop() NeilBrown
2025-10-19 9:56 ` Amir Goldstein
2025-10-15 1:46 ` [PATCH v2 03/14] VFS: tidy up do_unlinkat() NeilBrown
2025-10-19 10:02 ` Amir Goldstein
2025-10-15 1:46 ` [PATCH v2 04/14] VFS/nfsd/cachefiles/ovl: add start_creating() and end_creating() NeilBrown
2025-10-19 10:10 ` Amir Goldstein
2025-10-15 1:46 ` [PATCH v2 05/14] VFS/nfsd/cachefiles/ovl: introduce start_removing() and end_removing() NeilBrown
2025-10-15 1:46 ` [PATCH v2 06/14] VFS: introduce start_creating_noperm() and start_removing_noperm() NeilBrown
2025-10-19 10:15 ` Amir Goldstein
2025-10-22 3:20 ` NeilBrown
2025-10-20 8:36 ` kernel test robot
2025-10-15 1:46 ` [PATCH v2 07/14] VFS: introduce start_removing_dentry() NeilBrown
2025-10-15 1:47 ` [PATCH v2 08/14] VFS: add start_creating_killable() and start_removing_killable() NeilBrown
2025-10-15 1:47 ` [PATCH v2 09/14] VFS/nfsd/ovl: introduce start_renaming() and end_renaming() NeilBrown
2025-10-19 10:25 ` Amir Goldstein
2025-10-19 10:33 ` Amir Goldstein
2025-10-21 13:25 ` Christian Brauner
2025-10-22 3:35 ` NeilBrown
2025-10-15 1:47 ` [PATCH v2 10/14] VFS/ovl/smb: introduce start_renaming_dentry() NeilBrown
2025-10-19 10:31 ` Amir Goldstein
2025-10-15 1:47 ` [PATCH v2 11/14] Add start_renaming_two_dentries() NeilBrown
2025-10-15 1:47 ` [PATCH v2 12/14] ecryptfs: use new start_creating/start_removing APIs NeilBrown
2025-10-19 10:38 ` Amir Goldstein [this message]
2025-10-15 1:47 ` [PATCH v2 13/14] VFS: change vfs_mkdir() to unlock on failure NeilBrown
2025-10-19 10:46 ` Amir Goldstein
2025-10-22 3:54 ` NeilBrown
2025-10-15 1:47 ` [PATCH v2 14/14] VFS: introduce end_creating_keep() NeilBrown
2025-10-19 10:39 ` Amir Goldstein
2025-10-19 10:50 ` [PATCH v2 00/14] Create and use APIs to centralise locking for directory ops Amir Goldstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxjQsAT6HRdt0dP-xws2_pVcB8Ou_iTACOj6OqRG5fCSZg@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=jack@suse.cz \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=neil@brown.name \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).