On Mon, 2025-07-21 at 17:59 +1000, NeilBrown wrote:
> done_dentry_lookup() is the first step in introducing a new API for
> locked operation on names in directories - those that create or remove
> names. Rename operations will also be part of this API but will
> use separate interfaces.
>
> The plan is to lock just the dentry (or dentries), not the whole
> directory. A "dentry_lookup()" operation will perform the locking and
> lookup with a corresponding "done_dentry_lookup()" releasing the
> resulting dentry and dropping any locks.
>
> This done_dentry_lookup() can immediately be used to complete updates
> started with kern_path_locked() (much as done_path_create() already
> completes operations started with kern_path_create()).
>
> So this patch adds done_dentry_lookup() and uses it where
> kern_path_locked() is used. It also adds done_dentry_lookup_return()
> which returns a reference to the dentry rather than dropping it. This
> is a less common need in existing code, but still worth its own interface.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
> drivers/base/devtmpfs.c | 7 ++-----
> fs/bcachefs/fs-ioctl.c | 3 +--
> fs/namei.c | 38 ++++++++++++++++++++++++++++++++++++++
> include/linux/namei.h | 3 +++
> kernel/audit_fsnotify.c | 9 ++++-----
> kernel/audit_watch.c | 3 +--
> 6 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 31bfb3194b4c..47bee8166c8d 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -265,8 +265,7 @@ static int dev_rmdir(const char *name)
> else
> err = -EPERM;
>
> - dput(dentry);
> - inode_unlock(d_inode(parent.dentry));
> + done_dentry_lookup(dentry);
> path_put(&parent);
> return err;
> }
> @@ -349,9 +348,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> if (!err || err == -ENOENT)
> deleted = 1;
> }
> - dput(dentry);
> - inode_unlock(d_inode(parent.dentry));
> -
> + done_dentry_lookup(dentry);
> path_put(&parent);
> if (deleted && strchr(nodename, '/'))
> delete_path(nodename);
> diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
> index 4e72e654da96..8077ddf4ddc4 100644
> --- a/fs/bcachefs/fs-ioctl.c
> +++ b/fs/bcachefs/fs-ioctl.c
> @@ -351,8 +351,7 @@ static long bch2_ioctl_subvolume_destroy(struct bch_fs *c, struct file *filp,
> d_invalidate(victim);
> }
> err:
> - inode_unlock(dir);
> - dput(victim);
> + done_dentry_lookup(victim);
> path_put(&path);
> return ret;
> }
> diff --git a/fs/namei.c b/fs/namei.c
> index 1c80445693d4..da160a01e23d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1714,6 +1714,44 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> }
> EXPORT_SYMBOL(lookup_one_qstr_excl);
>
> +/**
> + * done_dentry_lookup - finish a lookup used for create/delete
> + * @dentry: the target dentry
> + *
> + * After locking the directory and lookup or validating a dentry
> + * an attempt can be made to create (including link) or remove (including
> + * rmdir) a dentry. After this, done_dentry_lookup() can be used to both
> + * unlock the parent directory and dput() the dentry.
> + *
> + * This interface allows a smooth transition from parent-dir based
> + * locking to dentry based locking.
> + */
> +void done_dentry_lookup(struct dentry *dentry)
> +{
> + inode_unlock(dentry->d_parent->d_inode);
> + dput(dentry);
> +}
> +EXPORT_SYMBOL(done_dentry_lookup);
> +
> +/**
> + * done_dentry_lookup_return - finish a lookup sequence, returning the dentry
> + * @dentry: the target dentry
> + *
> + * After locking the directory and lookup or validating a dentry
> + * an attempt can be made to create (including link) or remove (including
> + * rmdir) a dentry. After this, done_dentry_lookup_return() can be used to
> + * unlock the parent directory. The dentry is returned for further use.
> + *
> + * This interface allows a smooth transition from parent-dir based
> + * locking to dentry based locking.
> + */
> +struct dentry *done_dentry_lookup_return(struct dentry *dentry)
> +{
> + inode_unlock(dentry->d_parent->d_inode);
> + return dentry;
> +}
> +EXPORT_SYMBOL(done_dentry_lookup_return);
> +
> /**
> * lookup_fast - do fast lockless (but racy) lookup of a dentry
> * @nd: current nameidata
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 5d085428e471..e097f11587c9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -81,6 +81,9 @@ struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
> struct qstr *name,
> struct dentry *base);
>
> +void done_dentry_lookup(struct dentry *dentry);
> +struct dentry *done_dentry_lookup_return(struct dentry *dentry);
> +
> extern int follow_down_one(struct path *);
> extern int follow_down(struct path *path, unsigned int flags);
> extern int follow_up(struct path *);
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index c565fbf66ac8..170836c3520f 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -85,8 +85,8 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
> dentry = kern_path_locked(pathname, &path);
> if (IS_ERR(dentry))
> return ERR_CAST(dentry); /* returning an error */
> - inode = path.dentry->d_inode;
> - inode_unlock(inode);
> + inode = igrab(dentry->d_inode);
This is a little confusing. This patch changes "inode" from pointing to
the parent inode to point to the child inode instead. That actually
makes a bit more sense given the naming, but might best be done in a
separate patch.
> + done_dentry_lookup(dentry);
>
> audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
> if (unlikely(!audit_mark)) {
> @@ -97,17 +97,16 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
> fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_group);
> audit_mark->mark.mask = AUDIT_FS_EVENTS;
> audit_mark->path = pathname;
> - audit_update_mark(audit_mark, dentry->d_inode);
> + audit_update_mark(audit_mark, inode);
> audit_mark->rule = krule;
>
> - ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, 0);
> + ret = fsnotify_add_inode_mark(&audit_mark->mark, path.dentry->d_inode, 0);
> if (ret < 0) {
> audit_mark->path = NULL;
> fsnotify_put_mark(&audit_mark->mark);
> audit_mark = ERR_PTR(ret);
> }
> out:
> - dput(dentry);
> path_put(&path);
> return audit_mark;
> }
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 0ebbbe37a60f..f6ecac2109d4 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -359,8 +359,7 @@ static int audit_get_nd(struct audit_watch *watch, struct path *parent)
> watch->ino = d_backing_inode(d)->i_ino;
> }
>
> - inode_unlock(d_backing_inode(parent->dentry));
> - dput(d);
> + done_dentry_lookup(d);
> return 0;
> }
>
It looks right to me though.
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
