On Tue, Aug 12, 2025 at 12:25:08PM +1000, NeilBrown wrote:
> rename_lookup() combines lookup and locking for a rename.
>
> Two names - new_last and old_last - are added to struct renamedata so it
> can be passed to rename_lookup() to have the old and new dentries filled
> in.
>
> __rename_lookup() in vfs-internal and assumes that the names are already
> hashed and skips permission checking. This is appropriate for use after
> filename_parentat().
>
> rename_lookup_noperm() does hash the name but avoids permission
> checking. This will be used by debugfs.
WTF would debugfs do anything of that sort? Explain. Unlike vfs_rename(),
there we
* are given the source dentry
* are limited to pure name changes - same-directory only and
target must not exist.
* do not take ->s_vfs_rename_mutex
...
> If either old_dentry or new_dentry are not NULL, the corresponding
> "last" is ignored and the dentry is used as-is. This provides similar
> functionality to dentry_lookup_continue(). After locks are obtained we
> check that the parent is still correct. If old_parent was not given,
> then it is set to the parent of old_dentry which was locked. new_parent
> must never be NULL.
That screams "bad API" to me... Again, I want to see the users; you are
asking to accept a semantics that smells really odd, and it's impossible
to review without seeing the users.
> On success new references are geld on old_dentry, new_dentry and old_parent.
>
> done_rename_lookup() unlocks and drops those three references.
>
> No __free() support is provided as done_rename_lookup() cannot be safely
> called after rename_lookup() returns an error.
>
> Signed-off-by: NeilBrown <n...@brown.name>
> ---
> fs/namei.c | 318 ++++++++++++++++++++++++++++++++++--------
> include/linux/fs.h | 4 +
> include/linux/namei.h | 3 +
> 3 files changed, 263 insertions(+), 62 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index df21b6fa5a0e..cead810d53c6 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3507,6 +3507,233 @@ void unlock_rename(struct dentry *p1, struct dentry
> *p2)
> }
> EXPORT_SYMBOL(unlock_rename);
>
> +/**
> + * __rename_lookup - lookup and lock names for rename
> + * @rd: rename data containing relevant details
> + * @lookup_flags: extra flags to pass to ->lookup (e.g. LOOKUP_REVAL,
> + * LOOKUP_NO_SYMLINKS etc).
> + *
> + * Optionally look up two names and ensure locks are in place for
> + * rename.
> + * Normally @rd.old_dentry and @rd.new_dentry are %NULL and the
> + * old and new directories and last names are given in @rd. In this
> + * case the names are looked up with appropriate locking and the
> + * results stored in @rd.old_dentry and @rd.new_dentry.
> + *
> + * If either are not NULL, then the corresponding lookup is avoided but
> + * the required locks are still taken. In this case @rd.old_parent may
> + * be %NULL, otherwise @rd.old_dentry must still have @rd.old_parent as
> + * its d_parent after the locks are obtained. @rd.new_parent must
> + * always be non-NULL, and must always be the correct parent after
> + * locking.
> + *
> + * On success a reference is held on @rd.old_dentry, @rd.new_dentry,
> + * and @rd.old_parent whether they were originally %NULL or not. These
> + * references are dropped by done_rename_lookup(). @rd.new_parent
> + * must always be non-NULL and no extra reference is taken.
> + *
> + * The passed in qstrs must have the hash calculated, and no permission
> + * checking is performed.
> + *
> + * Returns: zero or an error.
> + */
> +static int
> +__rename_lookup(struct renamedata *rd, int lookup_flags)
> +{
> + struct dentry *p;
> + struct dentry *d1, *d2;
> + int target_flags = LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
> + int err;
> +
> + if (rd->flags & RENAME_EXCHANGE)
> + target_flags = 0;
> + if (rd->flags & RENAME_NOREPLACE)
> + target_flags |= LOOKUP_EXCL;
> +
> + if (rd->old_dentry) {
> + /* Already have the dentry - need to be sure to lock the
> correct parent */
> + p = lock_rename_child(rd->old_dentry, rd->new_parent);
> + if (IS_ERR(p))
> + return PTR_ERR(p);
> + if (d_unhashed(rd->old_dentry) ||
> + (rd->old_parent && rd->old_parent !=
> rd->old_dentry->d_parent)) {
> + /* dentry was removed, or moved and explicit parent
> requested */
> + unlock_rename(rd->old_dentry->d_parent, rd->new_parent);
> + return -EINVAL;
> + }
> + rd->old_parent = dget(rd->old_dentry->d_parent);
> + d1 = dget(rd->old_dentry);
> + } else {
> + p = lock_rename(rd->old_parent, rd->new_parent);
> + if (IS_ERR(p))
> + return PTR_ERR(p);
> + dget(rd->old_parent);
> +
> + d1 = lookup_one_qstr_excl(&rd->old_last, rd->old_parent,
> + lookup_flags);
> + if (IS_ERR(d1))
> + goto out_unlock_1;
> + }
> + if (rd->new_dentry) {
> + if (d_unhashed(rd->new_dentry) ||
> + rd->new_dentry->d_parent != rd->new_parent) {
> + /* new_dentry was moved or removed! */
> + goto out_unlock_2;
> + }
> + d2 = dget(rd->new_dentry);
> + } else {
> + d2 = lookup_one_qstr_excl(&rd->new_last, rd->new_parent,
> + lookup_flags | target_flags);
> + if (IS_ERR(d2))
> + goto out_unlock_2;
> + }
> +
> + if (d1 == p) {
> + /* source is an ancestor of target */
> + err = -EINVAL;
> + goto out_unlock_3;
> + }
> +
> + if (d2 == p) {
> + /* target is an ancestor of source */
> + if (rd->flags & RENAME_EXCHANGE)
> + err = -EINVAL;
> + else
> + err = -ENOTEMPTY;
> + goto out_unlock_3;
> + }
> +
> + rd->old_dentry = d1;
> + rd->new_dentry = d2;
> + return 0;
> +
> +out_unlock_3:
> + dput(d2);
> + d2 = ERR_PTR(err);
> +out_unlock_2:
> + dput(d1);
> + d1 = d2;
> +out_unlock_1:
> + unlock_rename(rd->old_parent, rd->new_parent);
> + dput(rd->old_parent);
> + return PTR_ERR(d1);
> +}
This is too fucking ugly to live, IMO. Too many things are mixed into it.
I will NAK that until I get a chance to see the users of all that stuff.
Sorry.
