Re: [PATCH 05/11] VFS: add rename_lookup()

["NeilBrown" <neil@brown.name>]

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