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.
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.
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 <neil@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);
+}
+
+/**
+ * rename_lookup_noperm - 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 need not have the hash calculated, and no
+ * permission checking is performed.
+ *
+ * Returns: zero or an error.
+ */
+int rename_lookup_noperm(struct renamedata *rd, int lookup_flags)
+{
+ int err;
+
+ if (!rd->old_dentry) {
+ err = lookup_noperm_common(&rd->old_last, rd->old_parent);
+ if (err)
+ return err;
+ }
+ if (!rd->new_dentry) {
+ err = lookup_noperm_common(&rd->new_last, rd->new_parent);
+ if (err)
+ return err;
+ }
+ return __rename_lookup(rd, lookup_flags);
+}
+EXPORT_SYMBOL(rename_lookup_noperm);
+
+/**
+ * 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 need not have the hash calculated, and normal
+ * permission checking for MAY_EXEC is performed.
+ *
+ * Returns: zero or an error.
+ */
+int rename_lookup(struct renamedata *rd, int lookup_flags)
+{
+ int err;
+
+ if (!rd->old_dentry) {
+ err = lookup_one_common(rd->old_mnt_idmap, &rd->old_last,
+ rd->old_parent);
+ if (err)
+ return err;
+ }
+ if (!rd->new_dentry) {
+ err = lookup_one_common(rd->new_mnt_idmap, &rd->new_last,
+ rd->new_parent);
+ if (err)
+ return err;
+ }
+ return __rename_lookup(rd, lookup_flags);
+}
+EXPORT_SYMBOL(rename_lookup);
+
+/**
+ * done_rename_lookup - unlock dentries after rename
+ * @rd: the struct renamedata that was passed to rename_lookup()
+ *
+ * After a successful rename_lookup() (or similar) call, and after
+ * any required renaming is performed, done_rename_rename() must be called
+ * to drop any locks and references that were obtained by the earlier function.
+ */
+void done_rename_lookup(struct renamedata *rd)
+{
+ unlock_rename(rd->old_parent, rd->new_parent);
+ dput(rd->old_parent);
+ dput(rd->old_dentry);
+ dput(rd->new_dentry);
+}
+EXPORT_SYMBOL(done_rename_lookup);
+
/**
* vfs_prepare_mode - prepare the mode to be used for a new inode
* @idmap: idmap of the mount the inode was found from
@@ -5336,14 +5563,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
struct filename *to, unsigned int flags)
{
struct renamedata rd;
- struct dentry *old_dentry, *new_dentry;
- struct dentry *trap;
struct path old_path, new_path;
- struct qstr old_last, new_last;
int old_type, new_type;
struct inode *delegated_inode = NULL;
- unsigned int lookup_flags = 0, target_flags =
- LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
+ unsigned int lookup_flags = 0;
bool should_retry = false;
int error = -EINVAL;
@@ -5354,19 +5577,14 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
(flags & RENAME_EXCHANGE))
goto put_names;
- if (flags & RENAME_EXCHANGE)
- target_flags = 0;
- if (flags & RENAME_NOREPLACE)
- target_flags |= LOOKUP_EXCL;
-
retry:
error = filename_parentat(olddfd, from, lookup_flags, &old_path,
- &old_last, &old_type);
+ &rd.old_last, &old_type);
if (error)
goto put_names;
- error = filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last,
- &new_type);
+ error = filename_parentat(newdfd, to, lookup_flags, &new_path,
+ &rd.new_last, &new_type);
if (error)
goto exit1;
@@ -5388,67 +5606,43 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
goto exit2;
retry_deleg:
- trap = lock_rename(new_path.dentry, old_path.dentry);
- if (IS_ERR(trap)) {
- error = PTR_ERR(trap);
+ rd.old_parent = old_path.dentry;
+ rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
+ rd.old_dentry = NULL;
+ rd.new_parent = new_path.dentry;
+ rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
+ rd.new_dentry = NULL;
+ rd.delegated_inode = &delegated_inode;
+ rd.flags = flags;
+
+ error = __rename_lookup(&rd, lookup_flags);
+ if (error)
goto exit_lock_rename;
- }
- old_dentry = lookup_one_qstr_excl(&old_last, old_path.dentry,
- lookup_flags);
- error = PTR_ERR(old_dentry);
- if (IS_ERR(old_dentry))
- goto exit3;
- new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
- lookup_flags | target_flags);
- error = PTR_ERR(new_dentry);
- if (IS_ERR(new_dentry))
- goto exit4;
if (flags & RENAME_EXCHANGE) {
- if (!d_is_dir(new_dentry)) {
+ if (!d_is_dir(rd.new_dentry)) {
error = -ENOTDIR;
- if (new_last.name[new_last.len])
- goto exit5;
+ if (rd.new_last.name[rd.new_last.len])
+ goto exit_unlock;
}
}
/* unless the source is a directory trailing slashes give -ENOTDIR */
- if (!d_is_dir(old_dentry)) {
+ if (!d_is_dir(rd.old_dentry)) {
error = -ENOTDIR;
- if (old_last.name[old_last.len])
- goto exit5;
- if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
- goto exit5;
- }
- /* source should not be ancestor of target */
- error = -EINVAL;
- if (old_dentry == trap)
- goto exit5;
- /* target should not be an ancestor of source */
- if (!(flags & RENAME_EXCHANGE))
- error = -ENOTEMPTY;
- if (new_dentry == trap)
- goto exit5;
+ if (rd.old_last.name[rd.old_last.len])
+ goto exit_unlock;
+ if (!(flags & RENAME_EXCHANGE) && rd.new_last.name[rd.new_last.len])
+ goto exit_unlock;
+ }
- error = security_path_rename(&old_path, old_dentry,
- &new_path, new_dentry, flags);
+ error = security_path_rename(&old_path, rd.old_dentry,
+ &new_path, rd.new_dentry, flags);
if (error)
- goto exit5;
+ goto exit_unlock;
- rd.old_parent = old_path.dentry;
- rd.old_dentry = old_dentry;
- rd.old_mnt_idmap = mnt_idmap(old_path.mnt);
- rd.new_parent = new_path.dentry;
- rd.new_dentry = new_dentry;
- rd.new_mnt_idmap = mnt_idmap(new_path.mnt);
- rd.delegated_inode = &delegated_inode;
- rd.flags = flags;
error = vfs_rename(&rd);
-exit5:
- dput(new_dentry);
-exit4:
- dput(old_dentry);
-exit3:
- unlock_rename(new_path.dentry, old_path.dentry);
+exit_unlock:
+ done_rename_lookup(&rd);
exit_lock_rename:
if (delegated_inode) {
error = break_deleg_wait(&delegated_inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index db644150b58f..b12203afa0da 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2011,9 +2011,11 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
* @old_mnt_idmap: idmap of the old mount the inode was found from
* @old_parent: parent of source
* @old_dentry: source
+ * @old_last: name for old_dentry in old_dir, if old_dentry not given
* @new_mnt_idmap: idmap of the new mount the inode was found from
* @new_parent: parent of destination
* @new_dentry: destination
+ * @new_last: name for new_dentry in new_dir, if new_dentry not given
* @delegated_inode: returns an inode needing a delegation break
* @flags: rename flags
*/
@@ -2021,9 +2023,11 @@ struct renamedata {
struct mnt_idmap *old_mnt_idmap;
struct dentry *old_parent;
struct dentry *old_dentry;
+ struct qstr old_last;
struct mnt_idmap *new_mnt_idmap;
struct dentry *new_parent;
struct dentry *new_dentry;
+ struct qstr new_last;
struct inode **delegated_inode;
unsigned int flags;
} __randomize_layout;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 67eef91603cc..26e5778c665f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -108,6 +108,9 @@ extern int follow_up(struct path *);
extern struct dentry *lock_rename(struct dentry *, struct dentry *);
extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
extern void unlock_rename(struct dentry *, struct dentry *);
+int rename_lookup(struct renamedata *rd, int lookup_flags);
+int rename_lookup_noperm(struct renamedata *rd, int lookup_flags);
+void done_rename_lookup(struct renamedata *rd);
/**
* mode_strip_umask - handle vfs umask stripping
--
2.50.0.107.gf914562f5916.dirtyOn 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 <neil@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.
On Wed, 13 Aug 2025, Al Viro wrote:
> 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
> ...
Sure, debugfs_change_name() could have a simplified rename_lookup()
which doesn't just skip the perm checking but also skips other
s_vfs_rename_mutex etc. But is there any value in creating a neutered
interface just because there is a case where all the functionality isn't
needed?
Or maybe I misunderstand your problem with rename_lookup_noperm().
>
> > 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.
There is a git tree you could pull.....
My API effectively supports both lock_rename() users and
lock_rename_child() users. Maybe you want to preserve the two different
APIs. I'd rather avoid the code duplication.
>
> > 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 <neil@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.
>
Can you say more about what you think it ugly?
Are you OK with combining the lookup and the locking in the one
function?
Are you OK with passing a 'struct rename_data' rather than a list of
assorted args?
Are you OK with deducing the target flags in this function, or do you
want them explicitly passed in?
Is it just that the function can use with lock_rename or
lock_rename_child depending on context?
???
Thanks,
NeilBrown
On Wed, Aug 13, 2025 at 06:04:32PM +1000, NeilBrown wrote: > There is a git tree you could pull..... > > My API effectively supports both lock_rename() users and > lock_rename_child() users. Maybe you want to preserve the two different > APIs. I'd rather avoid the code duplication. What code duplication? Seriously, how much of the logics is really shared? Error checking? So put that into a common helper... > > 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. > > > > Can you say more about what you think it ugly? > > Are you OK with combining the lookup and the locking in the one > function? > Are you OK with passing a 'struct rename_data' rather than a list of > assorted args? > Are you OK with deducing the target flags in this function, or do you > want them explicitly passed in? > Is it just that the function can use with lock_rename or > lock_rename_child depending on context? Put it that way: you are collapsing two (if not more) constructors for the same object into one. That makes describing (and proving, and verifying the callers, etc.) considerably more painful, with very little gain to be had. You are not so much modifying rename_data as creating an object - "state ready for rename". The fact that you use the same chunk of memory to encode the arguments of constructor is an implementation detail; constraints on the contents of that chunk of memory are different both from each other and from the resulting object. In effect, you have a weird union of several types here and the fact that C type system is pretty weak does not help. Having separate constructors at least documents which rules apply; conflating them is asking for trouble. It's the same problem as with flags arguments, really. It makes proofs harder. "I'm here" is a lot easier to deal with than "such and such correlations hold between the values of such and such variables". If we have several constructors (and they can easily share common helpers - no problem with that), we can always come back and decide to fold them into one; splitting is a lot harder, exactly because such flags, etc., do not stay local. I've done both kinds of transformations and in the "split" direction it ended up with bloody painful tree-wide analysis - more often than not with buggy corner cases caught in process. Let's not go there; if, in the end of process, we look at the result and see that unifying some of them makes sense - good, it won't be hard to do. But making it as "flexible" as possible as the first step pretty much locks you into a lot of decisions that are better done when the final state is seen - and possibly had been massaged for a while.
© 2016 - 2025 Red Hat, Inc.