[PATCH 0/6 RFC v2] tidy up various VFS lookup functions

[PATCH 0/6 RFC v2] tidy up various VFS lookup functions

[NeilBrown]

This a revised version of a previous posting.  I have dropped the change
to some lookup functions to pass a vfsmount.  I have also dropped the
changes to nfsd and cachefiles which passed a mnt_idmap other than
&nop_mnt_idmap.  Those modules now explicitly pass &nop_mnt_idmap to
some lookup functions where previously that was implicit.

============== Revised cover letter.

VFS has some functions with names containing "lookup_one_len" and others
without the "_len".  This difference has nothing to do with "len".  This
is an historical accident but can be confusing.

The functions without "_len" take a "mnt_idmap" pointer.  This is found
in the "vfsmount" and that is an important question when choosing which
to use: do you have a vfsmount, or are you "inside" the filesystem.  A
related question is "is permission checking relevant here?".

nfsd and cachefiles *do* have a vfsmount but *don't* use the non-_len
functions.  They pass nop_mnt_idmap and refuse to work on filesystems
which have any other idmap.

This series changes nfsd and cachefile to use the lookup_one family of
functions and to explictily pass &nop_mnt_idmap which is consistent with
all other vfs interfaces used where &nop_mnt_idmap is explicitly passed.

The remaining uses of the "_one" functions do not require permission
checks so these are renamed to be "_noperm" and the permission checking
is removed.

This series also changes these lookup function to take a qstr instead of
separate name and len.  In many cases this simplifies the call.

I haven't included changes to afs because there are patches in vfs.all
which make a lot of changes to lookup in afs.  I think (if they are seen
as a good idea) these patches should aim to land after the afs patches
and any further fixup in afs can happen then.

These patches are based on vfs-6.15.async.dir as they touch mkdir
related code.  There is a small conflict with the recently posted patch
to remove locking from try_lookup_one_len() calls.

Thanks,
NeilBrown

 [PATCH 1/6] VFS: improve interface for lookup_one functions
 [PATCH 2/6] nfsd: Use lookup_one() rather than lookup_one_len()
 [PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()
 [PATCH 4/6] VFS: rename lookup_one_len family to lookup_noperm and
 [PATCH 5/6] Use try_lookup_noperm() instead of d_hash_and_lookup()
 [PATCH 6/6] VFS: change lookup_one_common and lookup_noperm_common to

[PATCH 1/6] VFS: improve interface for lookup_one functions

[NeilBrown]

The family of functions:
  lookup_one()
  lookup_one_unlocked()
  lookup_one_positive_unlocked()

appear designed to be used by external clients of the filesystem rather
than by filesystems acting on themselves as the lookup_one_len family
are used.

They are used by:
   btrfs/ioctl - which is a user-space interface rather than an internal
     activity
   exportfs - i.e. from nfsd or the open_by_handle_at interface
   overlayfs - at access the underlying filesystems
   smb/server - for file service

They should be used by nfsd (more than just the exportfs path) and
cachefs but aren't.

It would help if the documentation didn't claim they should "not be
called by generic code".

Also the path component name is passed as "name" and "len" which are
(confusingly?) separate by the "base".  In some cases the len in simply
"strlen" and so passing a qstr using QSTR() would make the calling
clearer.
Other callers do pass separate name and len which are stored in a
struct.  Sometimes these are already stored in a qstr, other times it
easily could be.

So this patch changes these three functions to receive a 'struct qstr',
and improves the documentation.

QSTR_LEN() is added to make it easy to pass a QSTR containing a known
len.

Signed-off-by: NeilBrown <neil@brown.name>
---
 Documentation/filesystems/porting.rst |  9 +++++
 fs/btrfs/ioctl.c                      |  9 ++---
 fs/exportfs/expfs.c                   |  5 ++-
 fs/namei.c                            | 51 ++++++++++++---------------
 fs/overlayfs/namei.c                  | 10 +++---
 fs/overlayfs/overlayfs.h              |  2 +-
 fs/overlayfs/readdir.c                |  9 ++---
 fs/smb/server/smb2pdu.c               |  7 ++--
 include/linux/dcache.h                |  3 +-
 include/linux/namei.h                 |  9 +++--
 10 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index 6817614e0820..06296ffd1e81 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1196,3 +1196,12 @@ should use d_drop();d_splice_alias() and return the result of the latter.
 If a positive dentry cannot be returned for some reason, in-kernel
 clients such as cachefiles, nfsd, smb/server may not perform ideally but
 will fail-safe.
+
+---
+
+** mandatory**
+
+lookup_one(), lookup_one_unlocked(), lookup_one_positive_unlocked() now
+take a qstr instead of a name and len.  These, not the "one_len"
+versions, should be used whenever accessing a filesystem from outside
+that filesysmtem, through a mount point - which will have a mnt_idmap.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6c18bad53cd3..f94b638f9478 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -911,7 +911,7 @@ static noinline int btrfs_mksubvol(const struct path *parent,
 	if (error == -EINTR)
 		return error;
 
-	dentry = lookup_one(idmap, name, parent->dentry, namelen);
+	dentry = lookup_one(idmap, QSTR_LEN(name, namelen), parent->dentry);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto out_unlock;
@@ -2289,7 +2289,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	char *subvol_name, *subvol_name_ptr = NULL;
-	int subvol_namelen;
 	int ret = 0;
 	bool destroy_parent = false;
 
@@ -2412,10 +2411,8 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 			goto out;
 	}
 
-	subvol_namelen = strlen(subvol_name);
-
 	if (strchr(subvol_name, '/') ||
-	    strncmp(subvol_name, "..", subvol_namelen) == 0) {
+	    strcmp(subvol_name, "..") == 0) {
 		ret = -EINVAL;
 		goto free_subvol_name;
 	}
@@ -2428,7 +2425,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
 	if (ret == -EINTR)
 		goto free_subvol_name;
-	dentry = lookup_one(idmap, subvol_name, parent, subvol_namelen);
+	dentry = lookup_one(idmap, QSTR(subvol_name), parent);
 	if (IS_ERR(dentry)) {
 		ret = PTR_ERR(dentry);
 		goto out_unlock_dir;
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 0c899cfba578..974b432087aa 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -145,7 +145,7 @@ static struct dentry *reconnect_one(struct vfsmount *mnt,
 	if (err)
 		goto out_err;
 	dprintk("%s: found name: %s\n", __func__, nbuf);
-	tmp = lookup_one_unlocked(mnt_idmap(mnt), nbuf, parent, strlen(nbuf));
+	tmp = lookup_one_unlocked(mnt_idmap(mnt), QSTR(nbuf), parent);
 	if (IS_ERR(tmp)) {
 		dprintk("lookup failed: %ld\n", PTR_ERR(tmp));
 		err = PTR_ERR(tmp);
@@ -551,8 +551,7 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
 		}
 
 		inode_lock(target_dir->d_inode);
-		nresult = lookup_one(mnt_idmap(mnt), nbuf,
-				     target_dir, strlen(nbuf));
+		nresult = lookup_one(mnt_idmap(mnt), QSTR(nbuf), target_dir);
 		if (!IS_ERR(nresult)) {
 			if (unlikely(nresult->d_inode != result->d_inode)) {
 				dput(nresult);
diff --git a/fs/namei.c b/fs/namei.c
index b5abf456c5f4..75816fa80028 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2922,19 +2922,17 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 EXPORT_SYMBOL(lookup_one_len);
 
 /**
- * lookup_one - filesystem helper to lookup single pathname component
+ * lookup_one - lookup single pathname component
  * @idmap:	idmap of the mount the lookup is performed from
- * @name:	pathname component to lookup
+ * @name:	qstr holding pathname component to lookup
  * @base:	base directory to lookup from
- * @len:	maximum length @len should be interpreted to
  *
- * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * This can be used for in-kernel filesystem clients such as file servers.
  *
  * The caller must hold base->i_mutex.
  */
-struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
-			  struct dentry *base, int len)
+struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
+			  struct dentry *base)
 {
 	struct dentry *dentry;
 	struct qstr this;
@@ -2942,7 +2940,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_one_common(idmap, name, base, len, &this);
+	err = lookup_one_common(idmap, name.name, base, name.len, &this);
 	if (err)
 		return ERR_PTR(err);
 
@@ -2952,27 +2950,24 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
 EXPORT_SYMBOL(lookup_one);
 
 /**
- * lookup_one_unlocked - filesystem helper to lookup single pathname component
+ * lookup_one_unlocked - lookup single pathname component
  * @idmap:	idmap of the mount the lookup is performed from
- * @name:	pathname component to lookup
+ * @name:	qstr olding pathname component to lookup
  * @base:	base directory to lookup from
- * @len:	maximum length @len should be interpreted to
  *
- * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * This can be used for in-kernel filesystem clients such as file servers.
  *
  * Unlike lookup_one_len, it should be called without the parent
- * i_mutex held, and will take the i_mutex itself if necessary.
+ * i_rwsem held, and will take the i_rwsem itself if necessary.
  */
 struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
-				   const char *name, struct dentry *base,
-				   int len)
+				   struct qstr name, struct dentry *base)
 {
 	struct qstr this;
 	int err;
 	struct dentry *ret;
 
-	err = lookup_one_common(idmap, name, base, len, &this);
+	err = lookup_one_common(idmap, name.name, base, name.len, &this);
 	if (err)
 		return ERR_PTR(err);
 
@@ -2984,12 +2979,10 @@ struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
 EXPORT_SYMBOL(lookup_one_unlocked);
 
 /**
- * lookup_one_positive_unlocked - filesystem helper to lookup single
- *				  pathname component
+ * lookup_one_positive_unlocked - lookup single pathname component
  * @idmap:	idmap of the mount the lookup is performed from
- * @name:	pathname component to lookup
+ * @name:	qstr holding pathname component to lookup
  * @base:	base directory to lookup from
- * @len:	maximum length @len should be interpreted to
  *
  * This helper will yield ERR_PTR(-ENOENT) on negatives. The helper returns
  * known positive or ERR_PTR(). This is what most of the users want.
@@ -2998,16 +2991,15 @@ EXPORT_SYMBOL(lookup_one_unlocked);
  * time, so callers of lookup_one_unlocked() need to be very careful; pinned
  * positives have >d_inode stable, so this one avoids such problems.
  *
- * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * This can be used for in-kernel filesystem clients such as file servers.
  *
- * The helper should be called without i_mutex held.
+ * The helper should be called without i_rwsem held.
  */
 struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
-					    const char *name,
-					    struct dentry *base, int len)
+					    struct qstr name,
+					    struct dentry *base)
 {
-	struct dentry *ret = lookup_one_unlocked(idmap, name, base, len);
+	struct dentry *ret = lookup_one_unlocked(idmap, name, base);
 
 	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
 		dput(ret);
@@ -3032,7 +3024,7 @@ EXPORT_SYMBOL(lookup_one_positive_unlocked);
 struct dentry *lookup_one_len_unlocked(const char *name,
 				       struct dentry *base, int len)
 {
-	return lookup_one_unlocked(&nop_mnt_idmap, name, base, len);
+	return lookup_one_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len), base);
 }
 EXPORT_SYMBOL(lookup_one_len_unlocked);
 
@@ -3047,7 +3039,8 @@ EXPORT_SYMBOL(lookup_one_len_unlocked);
 struct dentry *lookup_positive_unlocked(const char *name,
 				       struct dentry *base, int len)
 {
-	return lookup_one_positive_unlocked(&nop_mnt_idmap, name, base, len);
+	return lookup_one_positive_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len),
+					    base);
 }
 EXPORT_SYMBOL(lookup_positive_unlocked);
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index be5c65d6f848..6a6301e4bba5 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -205,8 +205,8 @@ static struct dentry *ovl_lookup_positive_unlocked(struct ovl_lookup_data *d,
 						   struct dentry *base, int len,
 						   bool drop_negative)
 {
-	struct dentry *ret = lookup_one_unlocked(mnt_idmap(d->layer->mnt), name,
-						 base, len);
+	struct dentry *ret = lookup_one_unlocked(mnt_idmap(d->layer->mnt),
+						 QSTR_LEN(name, len), base);
 
 	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
 		if (drop_negative && ret->d_lockref.count == 1) {
@@ -789,8 +789,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
 	if (err)
 		return ERR_PTR(err);
 
-	index = lookup_one_positive_unlocked(ovl_upper_mnt_idmap(ofs), name.name,
-					     ofs->workdir, name.len);
+	index = lookup_one_positive_unlocked(ovl_upper_mnt_idmap(ofs), name,
+					     ofs->workdir);
 	if (IS_ERR(index)) {
 		err = PTR_ERR(index);
 		if (err == -ENOENT) {
@@ -1396,7 +1396,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 
 		this = lookup_one_positive_unlocked(
 				mnt_idmap(parentpath->layer->mnt),
-				name->name, parentpath->dentry, name->len);
+				*name, parentpath->dentry);
 		if (IS_ERR(this)) {
 			switch (PTR_ERR(this)) {
 			case -ENOENT:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6f2f8f4cfbbc..ceaf4eb199c7 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -402,7 +402,7 @@ static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
 					      const char *name,
 					      struct dentry *base, int len)
 {
-	return lookup_one(ovl_upper_mnt_idmap(ofs), name, base, len);
+	return lookup_one(ovl_upper_mnt_idmap(ofs), QSTR_LEN(name, len), base);
 }
 
 static inline bool ovl_open_flags_need_copy_up(int flags)
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 881ec5592da5..68df61f4bba7 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -271,7 +271,6 @@ static bool ovl_fill_merge(struct dir_context *ctx, const char *name,
 static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data *rdd)
 {
 	int err;
-	struct ovl_cache_entry *p;
 	struct dentry *dentry, *dir = path->dentry;
 	const struct cred *old_cred;
 
@@ -280,9 +279,11 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
 	err = down_write_killable(&dir->d_inode->i_rwsem);
 	if (!err) {
 		while (rdd->first_maybe_whiteout) {
-			p = rdd->first_maybe_whiteout;
+			struct ovl_cache_entry *p =
+				rdd->first_maybe_whiteout;
 			rdd->first_maybe_whiteout = p->next_maybe_whiteout;
-			dentry = lookup_one(mnt_idmap(path->mnt), p->name, dir, p->len);
+			dentry = lookup_one(mnt_idmap(path->mnt),
+					    QSTR_LEN(p->name, p->len), dir);
 			if (!IS_ERR(dentry)) {
 				p->is_whiteout = ovl_is_whiteout(dentry);
 				dput(dentry);
@@ -492,7 +493,7 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
 		}
 	}
 	/* This checks also for xwhiteouts */
-	this = lookup_one(mnt_idmap(path->mnt), p->name, dir, p->len);
+	this = lookup_one(mnt_idmap(path->mnt), QSTR_LEN(p->name, p->len), dir);
 	if (IS_ERR_OR_NULL(this) || !this->d_inode) {
 		/* Mark a stale entry */
 		p->is_whiteout = true;
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index f1efcd027475..c862e3bd4531 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4091,9 +4091,10 @@ static int process_query_dir_entries(struct smb2_query_dir_private *priv)
 			return -EINVAL;
 
 		lock_dir(priv->dir_fp);
-		dent = lookup_one(idmap, priv->d_info->name,
-				  priv->dir_fp->filp->f_path.dentry,
-				  priv->d_info->name_len);
+		dent = lookup_one(idmap,
+				  QSTR_LEN(priv->d_info->name,
+					   priv->d_info->name_len),
+				  priv->dir_fp->filp->f_path.dentry);
 		unlock_dir(priv->dir_fp);
 
 		if (IS_ERR(dent)) {
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 45bff10d3773..1f01f4e734c5 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -57,7 +57,8 @@ struct qstr {
 };
 
 #define QSTR_INIT(n,l) { { { .len = l } }, .name = n }
-#define QSTR(n) (struct qstr)QSTR_INIT(n, strlen(n))
+#define QSTR_LEN(n,l) (struct qstr)QSTR_INIT(n,l)
+#define QSTR(n) QSTR_LEN(n, strlen(n))
 
 extern const struct qstr empty_name;
 extern const struct qstr slash_name;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index e3042176cdf4..508dae67e3c5 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -73,13 +73,12 @@ extern struct dentry *try_lookup_one_len(const char *, struct dentry *, int);
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
 extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
 extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
-struct dentry *lookup_one(struct mnt_idmap *, const char *, struct dentry *, int);
+struct dentry *lookup_one(struct mnt_idmap *, struct qstr, struct dentry *);
 struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
-				   const char *name, struct dentry *base,
-				   int len);
+				   struct qstr name, struct dentry *base);
 struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
-					    const char *name,
-					    struct dentry *base, int len);
+					    struct qstr name,
+					    struct dentry *base);
 
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *path, unsigned int flags);
-- 
2.48.1

[PATCH 2/6] nfsd: Use lookup_one() rather than lookup_one_len()

[NeilBrown]

nfsd uses some VFS interfaces (such as vfs_mkdir) which take an explicit
mnt_idmap, and it passes &nop_mnt_idmap as nfsd doesn't yet support
idmapped mounts.

It also uses the lookup_one_len() family of functions which implicitly
use &nop_mnt_idmap.  This mixture of implicit and explicit could be
confusing.  When we eventually update nfsd to support idmap mounts it
would be best if all places which need an idmap determined from the
mount point were similar and easily found.

So this patch changes nfsd to use lookup_one(), lookup_one_unlocked(),
and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.

This has the benefit of removing some uses of the lookup_one_len
functions where permission checking is actually needed.  Many callers
don't care about permission checking and using these function only where
permission checking is needed is a valuable simplification.

This change requires passing the name in a qstr.  Currently this is a
little clumsy, but if nfsd is changed to use qstr more broadly it will
result in a net improvement.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs3proc.c    |  4 +++-
 fs/nfsd/nfs3xdr.c     |  4 +++-
 fs/nfsd/nfs4proc.c    |  4 +++-
 fs/nfsd/nfs4recover.c | 13 +++++++------
 fs/nfsd/nfs4xdr.c     |  4 +++-
 fs/nfsd/nfsproc.c     |  6 ++++--
 fs/nfsd/vfs.c         | 17 +++++++++--------
 7 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 372bdcf5e07a..9fa8ad08b1cd 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -284,7 +284,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	inode_lock_nested(inode, I_MUTEX_PARENT);
 
-	child = lookup_one_len(argp->name, parent, argp->len);
+	child = lookup_one(&nop_mnt_idmap,
+			   QSTR_LEN(argp->name, argp->len),
+			   parent);
 	if (IS_ERR(child)) {
 		status = nfserrno(PTR_ERR(child));
 		goto out;
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index a7a07470c1f8..5a626e24a334 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -1001,7 +1001,9 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp,
 		} else
 			dchild = dget(dparent);
 	} else
-		dchild = lookup_positive_unlocked(name, dparent, namlen);
+		dchild = lookup_one_positive_unlocked(&nop_mnt_idmap,
+						      QSTR_LEN(name, namlen),
+						      dparent);
 	if (IS_ERR(dchild))
 		return rv;
 	if (d_mountpoint(dchild))
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index f6e06c779d09..5860f3825be2 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -266,7 +266,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	inode_lock_nested(inode, I_MUTEX_PARENT);
 
-	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
+	child = lookup_one(&nop_mnt_idmap, QSTR_LEN(open->op_fname,
+						    open->op_fnamelen),
+			   parent);
 	if (IS_ERR(child)) {
 		status = nfserrno(PTR_ERR(child));
 		goto out;
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index c1d9bd07285f..5c1cb5c3c13e 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -218,7 +218,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	/* lock the parent */
 	inode_lock(d_inode(dir));
 
-	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
+	dentry = lookup_one(&nop_mnt_idmap, QSTR(dname), dir);
 	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
 		goto out_unlock;
@@ -316,7 +316,8 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
 	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
 		if (!status) {
 			struct dentry *dentry;
-			dentry = lookup_one_len(entry->name, dir, HEXDIR_LEN-1);
+			dentry = lookup_one(&nop_mnt_idmap,
+					    QSTR(entry->name), dir);
 			if (IS_ERR(dentry)) {
 				status = PTR_ERR(dentry);
 				break;
@@ -339,16 +340,16 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
 }
 
 static int
-nfsd4_unlink_clid_dir(char *name, int namlen, struct nfsd_net *nn)
+nfsd4_unlink_clid_dir(char *name, struct nfsd_net *nn)
 {
 	struct dentry *dir, *dentry;
 	int status;
 
-	dprintk("NFSD: nfsd4_unlink_clid_dir. name %.*s\n", namlen, name);
+	dprintk("NFSD: nfsd4_unlink_clid_dir. name %s\n", name);
 
 	dir = nn->rec_file->f_path.dentry;
 	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
-	dentry = lookup_one_len(name, dir, namlen);
+	dentry = lookup_one(&nop_mnt_idmap, QSTR(name), dir);
 	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
 		goto out_unlock;
@@ -408,7 +409,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 	if (status < 0)
 		goto out_drop_write;
 
-	status = nfsd4_unlink_clid_dir(dname, HEXDIR_LEN-1, nn);
+	status = nfsd4_unlink_clid_dir(dname, nn);
 	nfs4_reset_creds(original_cred);
 	if (status == 0) {
 		vfs_fsync(nn->rec_file, 0);
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e67420729ecd..16be860b1f79 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3812,7 +3812,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
 	__be32 nfserr;
 	int ignore_crossmnt = 0;
 
-	dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry, namlen);
+	dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
+					      QSTR_LEN(name, namlen),
+					      cd->rd_fhp->fh_dentry);
 	if (IS_ERR(dentry))
 		return nfserrno(PTR_ERR(dentry));
 
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 6dda081eb24c..ac7d7f858846 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -312,7 +312,9 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 	}
 
 	inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
-	dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
+	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(argp->name,
+						     argp->len),
+			    dirfhp->fh_dentry);
 	if (IS_ERR(dchild)) {
 		resp->status = nfserrno(PTR_ERR(dchild));
 		goto out_unlock;
@@ -331,7 +333,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
 		 */
 		resp->status = nfserr_acces;
 		if (!newfhp->fh_dentry) {
-			printk(KERN_WARNING 
+			printk(KERN_WARNING
 				"nfsd_proc_create: file handle not verified\n");
 			goto out_unlock;
 		}
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 34d7aa531662..c0c94619af92 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -265,7 +265,8 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				goto out_nfserr;
 		}
 	} else {
-		dentry = lookup_one_len_unlocked(name, dparent, len);
+		dentry = lookup_one_unlocked(&nop_mnt_idmap,
+					     QSTR_LEN(name, len), dparent);
 		host_err = PTR_ERR(dentry);
 		if (IS_ERR(dentry))
 			goto out_nfserr;
@@ -923,7 +924,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 	 * directories, but we never have and it doesn't seem to have
 	 * caused anyone a problem.  If we were to change this, note
 	 * also that our filldir callbacks would need a variant of
-	 * lookup_one_len that doesn't check permissions.
+	 * lookup_one_positive_unlocked() that doesn't check permissions.
 	 */
 	if (type == S_IFREG)
 		may_flags |= NFSD_MAY_OWNER_OVERRIDE;
@@ -1555,7 +1556,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		return nfserrno(host_err);
 
 	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
-	dchild = lookup_one_len(fname, dentry, flen);
+	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
 	host_err = PTR_ERR(dchild);
 	if (IS_ERR(dchild)) {
 		err = nfserrno(host_err);
@@ -1660,7 +1661,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	dentry = fhp->fh_dentry;
 	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
-	dnew = lookup_one_len(fname, dentry, flen);
+	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
 	if (IS_ERR(dnew)) {
 		err = nfserrno(PTR_ERR(dnew));
 		inode_unlock(dentry->d_inode);
@@ -1726,7 +1727,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 	dirp = d_inode(ddir);
 	inode_lock_nested(dirp, I_MUTEX_PARENT);
 
-	dnew = lookup_one_len(name, ddir, len);
+	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(name, len), ddir);
 	if (IS_ERR(dnew)) {
 		err = nfserrno(PTR_ERR(dnew));
 		goto out_unlock;
@@ -1839,7 +1840,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	if (err != nfs_ok)
 		goto out_unlock;
 
-	odentry = lookup_one_len(fname, fdentry, flen);
+	odentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), fdentry);
 	host_err = PTR_ERR(odentry);
 	if (IS_ERR(odentry))
 		goto out_nfserr;
@@ -1851,7 +1852,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	if (odentry == trap)
 		goto out_dput_old;
 
-	ndentry = lookup_one_len(tname, tdentry, tlen);
+	ndentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(tname, tlen), tdentry);
 	host_err = PTR_ERR(ndentry);
 	if (IS_ERR(ndentry))
 		goto out_dput_old;
@@ -1948,7 +1949,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	dirp = d_inode(dentry);
 	inode_lock_nested(dirp, I_MUTEX_PARENT);
 
-	rdentry = lookup_one_len(fname, dentry, flen);
+	rdentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
 	host_err = PTR_ERR(rdentry);
 	if (IS_ERR(rdentry))
 		goto out_unlock;
-- 
2.48.1

[PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()

[NeilBrown]

From: NeilBrown <neilb@suse.de>

cachefiles uses some VFS interfaces (such as vfs_mkdir) which take an
explicit mnt_idmap, and it passes &nop_mnt_idmap as cachefiles doesn't
yet support idmapped mounts.

It also uses the lookup_one_len() family of functions which implicitly
use &nop_mnt_idmap.  This mixture of implicit and explicit could be
confusing.  When we eventually update cachefiles to support idmap mounts it
would be best if all places which need an idmap determined from the
mount point were similar and easily found.

So this patch changes cachefiles to use lookup_one(), lookup_one_unlocked(),
and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.

This has the benefit of removing the remaining user of the
lookup_one_len functions where permission checking is actually needed.
Other callers don't care about permission checking and using these
function only where permission checking is needed is a valuable
simplification.

This requires passing the name in a qstr.  This is easily done with
QSTR() as the name is always nul terminated, and often strlen is used
anyway.  ->d_name_len is removed as no longer useful.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/cachefiles/internal.h |  1 -
 fs/cachefiles/key.c      |  1 -
 fs/cachefiles/namei.c    | 14 +++++++-------
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
index 38c236e38cef..b62cd3e9a18e 100644
--- a/fs/cachefiles/internal.h
+++ b/fs/cachefiles/internal.h
@@ -71,7 +71,6 @@ struct cachefiles_object {
 	int				debug_id;
 	spinlock_t			lock;
 	refcount_t			ref;
-	u8				d_name_len;	/* Length of filename */
 	enum cachefiles_content		content_info:8;	/* Info about content presence */
 	unsigned long			flags;
 #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
diff --git a/fs/cachefiles/key.c b/fs/cachefiles/key.c
index bf935e25bdbe..4927b533b9ae 100644
--- a/fs/cachefiles/key.c
+++ b/fs/cachefiles/key.c
@@ -132,7 +132,6 @@ bool cachefiles_cook_key(struct cachefiles_object *object)
 success:
 	name[len] = 0;
 	object->d_name = name;
-	object->d_name_len = len;
 	_leave(" = %s", object->d_name);
 	return true;
 }
diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 83a60126de0f..4fc6f3efd3d9 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -98,7 +98,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
 retry:
 	ret = cachefiles_inject_read_error();
 	if (ret == 0)
-		subdir = lookup_one_len(dirname, dir, strlen(dirname));
+		subdir = lookup_one(&nop_mnt_idmap, QSTR(dirname), dir);
 	else
 		subdir = ERR_PTR(ret);
 	trace_cachefiles_lookup(NULL, dir, subdir);
@@ -337,7 +337,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 		return -EIO;
 	}
 
-	grave = lookup_one_len(nbuffer, cache->graveyard, strlen(nbuffer));
+	grave = lookup_one(&nop_mnt_idmap, QSTR(nbuffer), cache->graveyard);
 	if (IS_ERR(grave)) {
 		unlock_rename(cache->graveyard, dir);
 		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
@@ -629,8 +629,8 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
 	/* Look up path "cache/vol/fanout/file". */
 	ret = cachefiles_inject_read_error();
 	if (ret == 0)
-		dentry = lookup_positive_unlocked(object->d_name, fan,
-						  object->d_name_len);
+		dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
+						      QSTR(object->d_name), fan);
 	else
 		dentry = ERR_PTR(ret);
 	trace_cachefiles_lookup(object, fan, dentry);
@@ -682,7 +682,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
 	inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
 	ret = cachefiles_inject_read_error();
 	if (ret == 0)
-		dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
+		dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
 	else
 		dentry = ERR_PTR(ret);
 	if (IS_ERR(dentry)) {
@@ -701,7 +701,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
 		dput(dentry);
 		ret = cachefiles_inject_read_error();
 		if (ret == 0)
-			dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
+			dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
 		else
 			dentry = ERR_PTR(ret);
 		if (IS_ERR(dentry)) {
@@ -750,7 +750,7 @@ static struct dentry *cachefiles_lookup_for_cull(struct cachefiles_cache *cache,
 
 	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
 
-	victim = lookup_one_len(filename, dir, strlen(filename));
+	victim = lookup_one(&nop_mnt_idmap, QSTR(filename), dir);
 	if (IS_ERR(victim))
 		goto lookup_error;
 	if (d_is_negative(victim))
-- 
2.48.1

[PATCH 4/6] VFS: rename lookup_one_len family to lookup_noperm and remove permission check

[NeilBrown]

The lookup_one_len family of functions is (now) only used internally by
a filesystem on itself either
- in a context where permission checking is irrelevant such as by a
  virtual filesystem populating itself, or xfs accessing its ORPHANAGE
  or dquota accessing the quota file; or
- in a context where a permission check (MAY_EXEC on the parent) has just
  been performed such as a network filesystem finding in "silly-rename"
  file in the same directory.  This is also the context after the
  _parentat() functions where currently lookup_one_qstr_excl() is used.

So the permission check is pointless.

The name "one_len" is unhelpful in understanding the purpose of these
functions and should be changed.  Most of the callers pass the len as
"strlen()" so using a qstr and QSTR() can simplify the code.

This patch renames these functions (include lookup_positive_unlocked()
which is part of the family despite the name) to have a name based on
"lookup_noperm".  They are changed to receive a 'struct qstr' instead
of separate name and len.  In a few cases the use of QSTR() results in a
new call to strlen().

try_lookup_noperm() takes a pointer to a qstr instead of the whole
qstr.  This is consistent with d_hash_and_lookup() (which is nearly
identical) and useful for lookup_noperm_unlocked().

The new lookup_noperm_common() doesn't take a qstr yet.  That will be
tidied up in a subsequent patch.

Signed-off-by: NeilBrown <neil@brown.name>
---
 Documentation/filesystems/porting.rst | 20 +++++++
 arch/s390/hypfs/inode.c               |  2 +-
 drivers/android/binderfs.c            |  4 +-
 drivers/infiniband/hw/qib/qib_fs.c    |  4 +-
 fs/afs/dir.c                          |  2 +-
 fs/afs/dir_silly.c                    |  6 +-
 fs/autofs/dev-ioctl.c                 |  3 +-
 fs/binfmt_misc.c                      |  2 +-
 fs/debugfs/inode.c                    |  6 +-
 fs/ecryptfs/inode.c                   | 16 ++---
 fs/kernfs/mount.c                     |  4 +-
 fs/namei.c                            | 86 ++++++++++++++++-----------
 fs/nfs/unlink.c                       | 11 ++--
 fs/overlayfs/export.c                 |  6 +-
 fs/overlayfs/namei.c                  |  2 +-
 fs/quota/dquot.c                      |  2 +-
 fs/smb/client/cached_dir.c            |  5 +-
 fs/smb/client/cifsfs.c                |  3 +-
 fs/tracefs/inode.c                    |  2 +-
 fs/xfs/scrub/orphanage.c              |  3 +-
 include/linux/namei.h                 |  8 +--
 ipc/mqueue.c                          |  5 +-
 kernel/bpf/inode.c                    |  2 +-
 security/apparmor/apparmorfs.c        |  4 +-
 security/inode.c                      |  2 +-
 25 files changed, 123 insertions(+), 87 deletions(-)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index 06296ffd1e81..df9516cd82e0 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1205,3 +1205,23 @@ lookup_one(), lookup_one_unlocked(), lookup_one_positive_unlocked() now
 take a qstr instead of a name and len.  These, not the "one_len"
 versions, should be used whenever accessing a filesystem from outside
 that filesysmtem, through a mount point - which will have a mnt_idmap.
+
+---
+
+** mandatory**
+
+Functions try_lookup_one_len(), lookup_one_len(),
+lookup_one_len_unlocked() and lookup_positive_unlocked() have been
+renamed to try_lookup_noperm(), lookup_noperm(),
+lookup_noperm_unlocked(), lookup_noperm_positive_unlocked().  They now
+take a qstr instead of separate name and length.  QSTR() can be used
+when strlen() is needed for the length.
+
+For try_lookup_noperm() a reference to the qstr is passed in case the
+hash might subsequently be needed.
+
+These function no longer do any permission checking - they previously
+checked that the caller has 'X' permission on the parent.  They must
+ONLY be used internally by a filesystem on itself when it knows that
+permissions are irrelevant or in a context where permission checks have
+already been performed such as after vfs_path_parent_lookup()
diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index d428635abf08..2eed957393f4 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -341,7 +341,7 @@ static struct dentry *hypfs_create_file(struct dentry *parent, const char *name,
 	struct inode *inode;
 
 	inode_lock(d_inode(parent));
-	dentry = lookup_one_len(name, parent, strlen(name));
+	dentry = lookup_noperm(QSTR(name), parent);
 	if (IS_ERR(dentry)) {
 		dentry = ERR_PTR(-ENOMEM);
 		goto fail;
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index bc6bae76ccaf..172e825168c8 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -187,7 +187,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
 	inode_lock(d_inode(root));
 
 	/* look it up */
-	dentry = lookup_one_len(name, root, name_len);
+	dentry = lookup_noperm(QSTR(name), root);
 	if (IS_ERR(dentry)) {
 		inode_unlock(d_inode(root));
 		ret = PTR_ERR(dentry);
@@ -486,7 +486,7 @@ static struct dentry *binderfs_create_dentry(struct dentry *parent,
 {
 	struct dentry *dentry;
 
-	dentry = lookup_one_len(name, parent, strlen(name));
+	dentry = lookup_noperm(QSTR(name), parent);
 	if (IS_ERR(dentry))
 		return dentry;
 
diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
index b27791029fa9..c3aa40c3fb8f 100644
--- a/drivers/infiniband/hw/qib/qib_fs.c
+++ b/drivers/infiniband/hw/qib/qib_fs.c
@@ -89,7 +89,7 @@ static int create_file(const char *name, umode_t mode,
 	int error;
 
 	inode_lock(d_inode(parent));
-	*dentry = lookup_one_len(name, parent, strlen(name));
+	*dentry = lookup_noperm(QSTR(name), parent);
 	if (!IS_ERR(*dentry))
 		error = qibfs_mknod(d_inode(parent), *dentry,
 				    mode, fops, data);
@@ -432,7 +432,7 @@ static int remove_device_files(struct super_block *sb,
 	char unit[10];
 
 	snprintf(unit, sizeof(unit), "%u", dd->unit);
-	dir = lookup_one_len_unlocked(unit, sb->s_root, strlen(unit));
+	dir = lookup_noperm_unlocked(QSTR(unit), sb->s_root);
 
 	if (IS_ERR(dir)) {
 		pr_err("Lookup of %s failed\n", unit);
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 5bddcc20786e..0e633535d2c7 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -943,7 +943,7 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
 		}
 
 		strcpy(p, name);
-		ret = lookup_one_len(buf, dentry->d_parent, len);
+		ret = lookup_noperm(QSTR(buf), dentry->d_parent);
 		if (IS_ERR(ret) || d_is_positive(ret))
 			goto out_s;
 		dput(ret);
diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
index a1e581946b93..292a5c8c752a 100644
--- a/fs/afs/dir_silly.c
+++ b/fs/afs/dir_silly.c
@@ -113,16 +113,14 @@ int afs_sillyrename(struct afs_vnode *dvnode, struct afs_vnode *vnode,
 
 	sdentry = NULL;
 	do {
-		int slen;
-
 		dput(sdentry);
 		sillycounter++;
 
 		/* Create a silly name.  Note that the ".__afs" prefix is
 		 * understood by the salvager and must not be changed.
 		 */
-		slen = scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
-		sdentry = lookup_one_len(silly, dentry->d_parent, slen);
+		scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
+		sdentry = lookup_noperm(QSTR(silly), dentry->d_parent);
 
 		/* N.B. Better to return EBUSY here ... it could be dangerous
 		 * to delete the file while it's in use.
diff --git a/fs/autofs/dev-ioctl.c b/fs/autofs/dev-ioctl.c
index 6d57efbb8110..a577b175c38a 100644
--- a/fs/autofs/dev-ioctl.c
+++ b/fs/autofs/dev-ioctl.c
@@ -461,7 +461,8 @@ static int autofs_dev_ioctl_timeout(struct file *fp,
 				"prevent shutdown\n");
 
 		inode_lock_shared(inode);
-		dentry = try_lookup_one_len(param->path, base, path_len);
+		dentry = try_lookup_noperm(&QSTR_LEN(param->path, path_len),
+					   base);
 		inode_unlock_shared(inode);
 		if (IS_ERR_OR_NULL(dentry))
 			return dentry ? PTR_ERR(dentry) : -ENOENT;
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 5a7ebd160724..f0996a76ee7c 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -842,7 +842,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
 	}
 
 	inode_lock(d_inode(root));
-	dentry = lookup_one_len(e->name, root, strlen(e->name));
+	dentry = lookup_noperm(QSTR(e->name), root);
 	err = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto out;
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 75715d8877ee..feb5ccb5bba8 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -346,7 +346,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
 	if (!parent)
 		parent = debugfs_mount->mnt_root;
 
-	dentry = lookup_positive_unlocked(name, parent, strlen(name));
+	dentry = lookup_noperm_positive_unlocked(QSTR(name), parent);
 	if (IS_ERR(dentry))
 		return NULL;
 	return dentry;
@@ -388,7 +388,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
 	if (unlikely(IS_DEADDIR(d_inode(parent))))
 		dentry = ERR_PTR(-ENOENT);
 	else
-		dentry = lookup_one_len(name, parent, strlen(name));
+		dentry = lookup_noperm(QSTR(name), parent);
 	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
 		if (d_is_dir(dentry))
 			pr_err("Directory '%s' with parent '%s' already present!\n",
@@ -872,7 +872,7 @@ int __printf(2, 3) debugfs_change_name(struct dentry *dentry, const char *fmt, .
 	}
 	if (strcmp(old_name.name.name, new_name) == 0)
 		goto out;
-	target = lookup_one_len(new_name, parent, strlen(new_name));
+	target = lookup_noperm(QSTR(new_name), parent);
 	if (IS_ERR(target)) {
 		error = PTR_ERR(target);
 		goto out;
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 51a5c54eb740..9a0d40bb43d6 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -394,8 +394,8 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode,
 	char *encrypted_and_encoded_name = NULL;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
 	struct dentry *lower_dir_dentry, *lower_dentry;
-	const char *name = ecryptfs_dentry->d_name.name;
-	size_t len = ecryptfs_dentry->d_name.len;
+	struct qstr qname = QSTR_INIT(ecryptfs_dentry->d_name.name,
+				      ecryptfs_dentry->d_name.len);
 	struct dentry *res;
 	int rc = 0;
 
@@ -404,23 +404,25 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode,
 	mount_crypt_stat = &ecryptfs_superblock_to_private(
 				ecryptfs_dentry->d_sb)->mount_crypt_stat;
 	if (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) {
+		size_t len = qname.len;
 		rc = ecryptfs_encrypt_and_encode_filename(
 			&encrypted_and_encoded_name, &len,
-			mount_crypt_stat, name, len);
+			mount_crypt_stat, qname.name, len);
 		if (rc) {
 			printk(KERN_ERR "%s: Error attempting to encrypt and encode "
 			       "filename; rc = [%d]\n", __func__, rc);
 			return ERR_PTR(rc);
 		}
-		name = encrypted_and_encoded_name;
+		qname.name = encrypted_and_encoded_name;
+		qname.len = len;
 	}
 
-	lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len);
+	lower_dentry = lookup_noperm_unlocked(qname, lower_dir_dentry);
 	if (IS_ERR(lower_dentry)) {
-		ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned "
+		ecryptfs_printk(KERN_DEBUG, "%s: lookup_noperm() returned "
 				"[%ld] on lower_dentry = [%s]\n", __func__,
 				PTR_ERR(lower_dentry),
-				name);
+				qname.name);
 		res = ERR_CAST(lower_dentry);
 	} else {
 		res = ecryptfs_lookup_interpose(ecryptfs_dentry, lower_dentry);
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index 1358c21837f1..46943a7fb4df 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -233,8 +233,8 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
 			dput(dentry);
 			return ERR_PTR(-EINVAL);
 		}
-		dtmp = lookup_positive_unlocked(kntmp->name, dentry,
-					       strlen(kntmp->name));
+		dtmp = lookup_noperm_positive_unlocked(QSTR(kntmp->name),
+							 dentry);
 		dput(dentry);
 		if (IS_ERR(dtmp))
 			return dtmp;
diff --git a/fs/namei.c b/fs/namei.c
index 75816fa80028..16605f7108c0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2833,9 +2833,9 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 }
 EXPORT_SYMBOL(vfs_path_lookup);
 
-static int lookup_one_common(struct mnt_idmap *idmap,
-			     const char *name, struct dentry *base, int len,
-			     struct qstr *this)
+static int lookup_noperm_common(const char *name, struct dentry *base,
+				  int len,
+				  struct qstr *this)
 {
 	this->name = name;
 	this->len = len;
@@ -2860,51 +2860,59 @@ static int lookup_one_common(struct mnt_idmap *idmap,
 		if (err < 0)
 			return err;
 	}
+	return 0;
+}
 
+static int lookup_one_common(struct mnt_idmap *idmap,
+			     const char *name, struct dentry *base, int len,
+			     struct qstr *this) {
+	int err;
+	err = lookup_noperm_common(name, base, len, this);
+	if (err < 0)
+		return err;
 	return inode_permission(idmap, base->d_inode, MAY_EXEC);
 }
 
 /**
- * try_lookup_one_len - filesystem helper to lookup single pathname component
- * @name:	pathname component to lookup
+ * try_lookup_noperm - filesystem helper to lookup single pathname component
+ * @name:	qstr storing pathname component to lookup
  * @base:	base directory to lookup from
- * @len:	maximum length @len should be interpreted to
  *
  * Look up a dentry by name in the dcache, returning NULL if it does not
  * currently exist.  The function does not try to create a dentry.
  *
  * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * not be called by generic code.  It does no permission checking.
  *
  * The caller must hold base->i_mutex.
  */
-struct dentry *try_lookup_one_len(const char *name, struct dentry *base, int len)
+struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
 {
 	struct qstr this;
 	int err;
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
+	err = lookup_noperm_common(name->name, base, name->len, &this);
 	if (err)
 		return ERR_PTR(err);
 
-	return lookup_dcache(&this, base, 0);
+	name->hash = this.hash;
+	return lookup_dcache(name, base, 0);
 }
-EXPORT_SYMBOL(try_lookup_one_len);
+EXPORT_SYMBOL(try_lookup_noperm);
 
 /**
- * lookup_one_len - filesystem helper to lookup single pathname component
- * @name:	pathname component to lookup
+ * lookup_noperm - filesystem helper to lookup single pathname component
+ * @name:	qstr storing pathname component to lookup
  * @base:	base directory to lookup from
- * @len:	maximum length @len should be interpreted to
  *
  * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * not be called by generic code.  It does no permission checking.
  *
  * The caller must hold base->i_mutex.
  */
-struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
+struct dentry *lookup_noperm(struct qstr name, struct dentry *base)
 {
 	struct dentry *dentry;
 	struct qstr this;
@@ -2912,14 +2920,14 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
+	err = lookup_noperm_common(name.name, base, name.len, &this);
 	if (err)
 		return ERR_PTR(err);
 
 	dentry = lookup_dcache(&this, base, 0);
 	return dentry ? dentry : __lookup_slow(&this, base, 0);
 }
-EXPORT_SYMBOL(lookup_one_len);
+EXPORT_SYMBOL(lookup_noperm);
 
 /**
  * lookup_one - lookup single pathname component
@@ -2957,7 +2965,7 @@ EXPORT_SYMBOL(lookup_one);
  *
  * This can be used for in-kernel filesystem clients such as file servers.
  *
- * Unlike lookup_one_len, it should be called without the parent
+ * Unlike lookup_one, it should be called without the parent
  * i_rwsem held, and will take the i_rwsem itself if necessary.
  */
 struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
@@ -3010,39 +3018,49 @@ struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
 EXPORT_SYMBOL(lookup_one_positive_unlocked);
 
 /**
- * lookup_one_len_unlocked - filesystem helper to lookup single pathname component
+ * lookup_noperm_unlocked - filesystem helper to lookup single pathname component
  * @name:	pathname component to lookup
  * @base:	base directory to lookup from
  * @len:	maximum length @len should be interpreted to
  *
  * Note that this routine is purely a helper for filesystem usage and should
- * not be called by generic code.
+ * not be called by generic code. It does no permission checking.
  *
- * Unlike lookup_one_len, it should be called without the parent
- * i_mutex held, and will take the i_mutex itself if necessary.
+ * Unlike lookup_noperm, it should be called without the parent
+ * i_rwsem held, and will take the i_rwsem itself if necessary.
  */
-struct dentry *lookup_one_len_unlocked(const char *name,
-				       struct dentry *base, int len)
+struct dentry *lookup_noperm_unlocked(struct qstr name,
+					struct dentry *base)
 {
-	return lookup_one_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len), base);
+	struct dentry *ret;
+
+	ret = try_lookup_noperm(&name, base);
+	if (!ret)
+		ret = lookup_slow(&name, base, 0);
+	return ret;
 }
-EXPORT_SYMBOL(lookup_one_len_unlocked);
+EXPORT_SYMBOL(lookup_noperm_unlocked);
 
 /*
- * Like lookup_one_len_unlocked(), except that it yields ERR_PTR(-ENOENT)
+ * Like lookup_noperm_unlocked(), except that it yields ERR_PTR(-ENOENT)
  * on negatives.  Returns known positive or ERR_PTR(); that's what
  * most of the users want.  Note that pinned negative with unlocked parent
- * _can_ become positive at any time, so callers of lookup_one_len_unlocked()
+ * _can_ become positive at any time, so callers of lookup_noperm_unlocked()
  * need to be very careful; pinned positives have ->d_inode stable, so
  * this one avoids such problems.
  */
-struct dentry *lookup_positive_unlocked(const char *name,
-				       struct dentry *base, int len)
+struct dentry *lookup_noperm_positive_unlocked(struct qstr name,
+					       struct dentry *base)
 {
-	return lookup_one_positive_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len),
-					    base);
+	struct dentry *ret = lookup_noperm_unlocked(name, base);
+
+	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
+		dput(ret);
+		ret = ERR_PTR(-ENOENT);
+	}
+	return ret;
 }
-EXPORT_SYMBOL(lookup_positive_unlocked);
+EXPORT_SYMBOL(lookup_noperm_positive_unlocked);
 
 #ifdef CONFIG_UNIX98_PTYS
 int path_pts(struct path *path)
diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index bf77399696a7..f52e57c31ae4 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -464,18 +464,17 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
 
 	sdentry = NULL;
 	do {
-		int slen;
 		dput(sdentry);
 		sillycounter++;
-		slen = scnprintf(silly, sizeof(silly),
-				SILLYNAME_PREFIX "%0*llx%0*x",
-				SILLYNAME_FILEID_LEN, fileid,
-				SILLYNAME_COUNTER_LEN, sillycounter);
+		scnprintf(silly, sizeof(silly),
+			  SILLYNAME_PREFIX "%0*llx%0*x",
+			  SILLYNAME_FILEID_LEN, fileid,
+			  SILLYNAME_COUNTER_LEN, sillycounter);
 
 		dfprintk(VFS, "NFS: trying to rename %pd to %s\n",
 				dentry, silly);
 
-		sdentry = lookup_one_len(silly, dentry->d_parent, slen);
+		sdentry = lookup_noperm(QSTR(silly), dentry->d_parent);
 		/*
 		 * N.B. Better to return EBUSY here ... it could be
 		 * dangerous to delete the file while it's in use.
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 444aeeccb6da..a51025e8b2d0 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -385,11 +385,9 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
 	 */
 	take_dentry_name_snapshot(&name, real);
 	/*
-	 * No idmap handling here: it's an internal lookup.  Could skip
-	 * permission checking altogether, but for now just use non-idmap
-	 * transformed ids.
+	 * No idmap handling here: it's an internal lookup.
 	 */
-	this = lookup_one_len(name.name.name, connected, name.name.len);
+	this = lookup_noperm(name.name, connected);
 	release_dentry_name_snapshot(&name);
 	err = PTR_ERR(this);
 	if (IS_ERR(this)) {
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6a6301e4bba5..adb4af8d08db 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -757,7 +757,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh)
 	if (err)
 		return ERR_PTR(err);
 
-	index = lookup_positive_unlocked(name.name, ofs->workdir, name.len);
+	index = lookup_noperm_positive_unlocked(name, ofs->workdir);
 	kfree(name.name);
 	if (IS_ERR(index)) {
 		if (PTR_ERR(index) == -ENOENT)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 825c5c2e0962..ea426467f26b 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2560,7 +2560,7 @@ int dquot_quota_on_mount(struct super_block *sb, char *qf_name,
 	struct dentry *dentry;
 	int error;
 
-	dentry = lookup_positive_unlocked(qf_name, sb->s_root, strlen(qf_name));
+	dentry = lookup_noperm_positive_unlocked(QSTR(qf_name), sb->s_root);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index fe738623cf1b..854de94fc394 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -109,7 +109,8 @@ path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
 		while (*s && *s != sep)
 			s++;
 
-		child = lookup_positive_unlocked(p, dentry, s - p);
+		child = lookup_noperm_positive_unlocked(QSTR_LEN(p, s-p),
+							dentry);
 		dput(dentry);
 		dentry = child;
 	} while (!IS_ERR(dentry));
@@ -207,7 +208,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
 	spin_unlock(&cfids->cfid_list_lock);
 
 	/*
-	 * Skip any prefix paths in @path as lookup_positive_unlocked() ends up
+	 * Skip any prefix paths in @path as lookup_noperm_positive_unlocked() ends up
 	 * calling ->lookup() which already adds those through
 	 * build_path_from_dentry().  Also, do it earlier as we might reconnect
 	 * below when trying to send compounded request and then potentially
diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index 6a3bd652d251..71eadc0305b5 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -925,7 +925,8 @@ cifs_get_root(struct smb3_fs_context *ctx, struct super_block *sb)
 		while (*s && *s != sep)
 			s++;
 
-		child = lookup_positive_unlocked(p, dentry, s - p);
+		child = lookup_noperm_positive_unlocked(QSTR_LEN(p, s - p),
+							dentry);
 		dput(dentry);
 		dentry = child;
 	} while (!IS_ERR(dentry));
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index cb1af30b49f5..aa7982e9b579 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -555,7 +555,7 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
 	if (unlikely(IS_DEADDIR(d_inode(parent))))
 		dentry = ERR_PTR(-ENOENT);
 	else
-		dentry = lookup_one_len(name, parent, strlen(name));
+		dentry = lookup_noperm(QSTR(name), parent);
 	if (!IS_ERR(dentry) && d_inode(dentry)) {
 		dput(dentry);
 		dentry = ERR_PTR(-EEXIST);
diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
index 3537f3cca6d5..987af5b2bb82 100644
--- a/fs/xfs/scrub/orphanage.c
+++ b/fs/xfs/scrub/orphanage.c
@@ -153,8 +153,7 @@ xrep_orphanage_create(
 
 	/* Try to find the orphanage directory. */
 	inode_lock_nested(root_inode, I_MUTEX_PARENT);
-	orphanage_dentry = lookup_one_len(ORPHANAGE, root_dentry,
-			strlen(ORPHANAGE));
+	orphanage_dentry = lookup_noperm(QSTR(ORPHANAGE), root_dentry);
 	if (IS_ERR(orphanage_dentry)) {
 		error = PTR_ERR(orphanage_dentry);
 		goto out_unlock_root;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 508dae67e3c5..befe75439d7e 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -69,10 +69,10 @@ int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
 int vfs_path_lookup(struct dentry *, struct vfsmount *, const char *,
 		    unsigned int, struct path *);
 
-extern struct dentry *try_lookup_one_len(const char *, struct dentry *, int);
-extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
-extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
-extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
+extern struct dentry *try_lookup_noperm(struct qstr *, struct dentry *);
+extern struct dentry *lookup_noperm(struct qstr, struct dentry *);
+extern struct dentry *lookup_noperm_unlocked(struct qstr, struct dentry *);
+extern struct dentry *lookup_noperm_positive_unlocked(struct qstr, struct dentry *);
 struct dentry *lookup_one(struct mnt_idmap *, struct qstr, struct dentry *);
 struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
 				   struct qstr name, struct dentry *base);
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 35b4f8659904..f3d76c4b6013 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -913,7 +913,7 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode,
 
 	ro = mnt_want_write(mnt);	/* we'll drop it in any case */
 	inode_lock(d_inode(root));
-	path.dentry = lookup_one_len(name->name, root, strlen(name->name));
+	path.dentry = lookup_noperm(QSTR(name->name), root);
 	if (IS_ERR(path.dentry)) {
 		error = PTR_ERR(path.dentry);
 		goto out_putfd;
@@ -969,8 +969,7 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
 	if (err)
 		goto out_name;
 	inode_lock_nested(d_inode(mnt->mnt_root), I_MUTEX_PARENT);
-	dentry = lookup_one_len(name->name, mnt->mnt_root,
-				strlen(name->name));
+	dentry = lookup_noperm(QSTR(name->name), mnt->mnt_root);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		goto out_unlock;
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index dc3aa91a6ba0..77fda4101b06 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -421,7 +421,7 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent,
 	int ret;
 
 	inode_lock(parent->d_inode);
-	dentry = lookup_one_len(name, parent, strlen(name));
+	dentry = lookup_noperm(QSTR(name), parent);
 	if (IS_ERR(dentry)) {
 		inode_unlock(parent->d_inode);
 		return PTR_ERR(dentry);
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 6039afae4bfc..dfc56cec5ab5 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -283,7 +283,7 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
 	dir = d_inode(parent);
 
 	inode_lock(dir);
-	dentry = lookup_one_len(name, parent, strlen(name));
+	dentry = lookup_noperm(QSTR(name), parent);
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
 		goto fail_lock;
@@ -2551,7 +2551,7 @@ static int aa_mk_null_file(struct dentry *parent)
 		return error;
 
 	inode_lock(d_inode(parent));
-	dentry = lookup_one_len(NULL_FILE_NAME, parent, strlen(NULL_FILE_NAME));
+	dentry = lookup_noperm(QSTR(NULL_FILE_NAME), parent);
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
 		goto out;
diff --git a/security/inode.c b/security/inode.c
index da3ab44c8e57..d04a52a52bdd 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -128,7 +128,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 	dir = d_inode(parent);
 
 	inode_lock(dir);
-	dentry = lookup_one_len(name, parent, strlen(name));
+	dentry = lookup_noperm(QSTR(name), parent);
 	if (IS_ERR(dentry))
 		goto out;
 
-- 
2.48.1

[PATCH 5/6] Use try_lookup_noperm() instead of d_hash_and_lookup() outside of VFS

[NeilBrown]

From: NeilBrown <neilb@suse.de>

try_lookup_noperm() and d_hash_and_lookup() are nearly identical.  The
former does some validation of the name where the latter doesn't.
Outside of the VFS that validation is likely valuable, and having only
one exported function for this task is certainly a good idea.

So make d_hash_and_lookup() local to VFS files and change all other
callers to try_lookup_noperm().  Note that the arguments are swapped.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 Documentation/filesystems/porting.rst | 11 +++++++++++
 fs/dcache.c                           |  1 -
 fs/efivarfs/super.c                   | 14 ++++----------
 fs/internal.h                         |  1 +
 fs/proc/base.c                        |  2 +-
 fs/smb/client/readdir.c               |  3 ++-
 fs/xfs/scrub/orphanage.c              |  4 ++--
 include/linux/dcache.h                |  1 -
 net/sunrpc/rpc_pipe.c                 | 12 ++++++------
 security/selinux/selinuxfs.c          |  4 ++--
 10 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index df9516cd82e0..626f094787e8 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1225,3 +1225,14 @@ checked that the caller has 'X' permission on the parent.  They must
 ONLY be used internally by a filesystem on itself when it knows that
 permissions are irrelevant or in a context where permission checks have
 already been performed such as after vfs_path_parent_lookup()
+
+---
+
+** mandatory**
+
+d_hash_and_lookup() is no longer exported or available outside the VFS.
+Use try_lookup_noperm() instead.  This adds name validation and takes
+arguments in the opposite order but is otherwise identical.
+
+Using try_lookup_noperm() will require linux/namei.h to be included.
+
diff --git a/fs/dcache.c b/fs/dcache.c
index 726a5be2747b..17f8e0b7f04f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2395,7 +2395,6 @@ struct dentry *d_hash_and_lookup(struct dentry *dir, struct qstr *name)
 	}
 	return d_lookup(dir, name);
 }
-EXPORT_SYMBOL(d_hash_and_lookup);
 
 /*
  * When a file is deleted, we have two options:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 09fcf731e65d..867cd6e0fbad 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -204,7 +204,6 @@ bool efivarfs_variable_is_present(efi_char16_t *variable_name,
 	char *name = efivar_get_utf8name(variable_name, vendor);
 	struct super_block *sb = data;
 	struct dentry *dentry;
-	struct qstr qstr;
 
 	if (!name)
 		/*
@@ -217,9 +216,7 @@ bool efivarfs_variable_is_present(efi_char16_t *variable_name,
 		 */
 		return true;
 
-	qstr.name = name;
-	qstr.len = strlen(name);
-	dentry = d_hash_and_lookup(sb->s_root, &qstr);
+	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
 	kfree(name);
 	if (!IS_ERR_OR_NULL(dentry))
 		dput(dentry);
@@ -402,8 +399,8 @@ static bool efivarfs_actor(struct dir_context *ctx, const char *name, int len,
 {
 	unsigned long size;
 	struct efivarfs_ctx *ectx = container_of(ctx, struct efivarfs_ctx, ctx);
-	struct qstr qstr = { .name = name, .len = len };
-	struct dentry *dentry = d_hash_and_lookup(ectx->sb->s_root, &qstr);
+	struct dentry *dentry = try_lookup_noperm(QSTR_LEN(name, len),
+						  ectx->sb->s_root);
 	struct inode *inode;
 	struct efivar_entry *entry;
 	int err;
@@ -439,7 +436,6 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
 	char *name;
 	struct super_block *sb = data;
 	struct dentry *dentry;
-	struct qstr qstr;
 	int err;
 
 	if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
@@ -449,9 +445,7 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
 	if (!name)
 		return -ENOMEM;
 
-	qstr.name = name;
-	qstr.len = strlen(name);
-	dentry = d_hash_and_lookup(sb->s_root, &qstr);
+	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		goto out;
diff --git a/fs/internal.h b/fs/internal.h
index e7f02ae1e098..c21534a23196 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -66,6 +66,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
 int vfs_tmpfile(struct mnt_idmap *idmap,
 		const struct path *parentpath,
 		struct file *file, umode_t mode);
+struct dentry *d_hash_and_lookup(struct dentry *, struct qstr *);
 
 /*
  * namespace.c
diff --git a/fs/proc/base.c b/fs/proc/base.c
index cd89e956c322..7d36c7567c31 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2124,7 +2124,7 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
 	unsigned type = DT_UNKNOWN;
 	ino_t ino = 1;
 
-	child = d_hash_and_lookup(dir, &qname);
+	child = try_lookup_noperm(&qname, dir);
 	if (!child) {
 		DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 		child = d_alloc_parallel(dir, &qname, &wq);
diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index 50f96259d9ad..7329ec532bcf 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -9,6 +9,7 @@
  *
  */
 #include <linux/fs.h>
+#include <linux/namei.h>
 #include <linux/pagemap.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
@@ -78,7 +79,7 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
 
 	cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);
 
-	dentry = d_hash_and_lookup(parent, name);
+	dentry = try_lookup_noperm(name, parent);
 	if (!dentry) {
 		/*
 		 * If we know that the inode will need to be revalidated
diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
index 987af5b2bb82..f42ffad5a7b9 100644
--- a/fs/xfs/scrub/orphanage.c
+++ b/fs/xfs/scrub/orphanage.c
@@ -444,7 +444,7 @@ xrep_adoption_check_dcache(
 	if (!d_orphanage)
 		return 0;
 
-	d_child = d_hash_and_lookup(d_orphanage, &qname);
+	d_child = try_lookup_noperm(&qname, d_orphanage);
 	if (d_child) {
 		trace_xrep_adoption_check_child(sc->mp, d_child);
 
@@ -481,7 +481,7 @@ xrep_adoption_zap_dcache(
 	if (!d_orphanage)
 		return;
 
-	d_child = d_hash_and_lookup(d_orphanage, &qname);
+	d_child = try_lookup_noperm(&qname, d_orphanage);
 	while (d_child != NULL) {
 		trace_xrep_adoption_invalidate_child(sc->mp, d_child);
 
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 1f01f4e734c5..cf37ae54955d 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -288,7 +288,6 @@ extern void d_exchange(struct dentry *, struct dentry *);
 extern struct dentry *d_ancestor(struct dentry *, struct dentry *);
 
 extern struct dentry *d_lookup(const struct dentry *, const struct qstr *);
-extern struct dentry *d_hash_and_lookup(struct dentry *, struct qstr *);
 
 static inline unsigned d_count(const struct dentry *dentry)
 {
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index eadc00410ebc..98f78cd55905 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -631,7 +631,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 					  const char *name)
 {
 	struct qstr q = QSTR(name);
-	struct dentry *dentry = d_hash_and_lookup(parent, &q);
+	struct dentry *dentry = try_lookup_noperm(&q, parent);
 	if (!dentry) {
 		dentry = d_alloc(parent, &q);
 		if (!dentry)
@@ -658,7 +658,7 @@ static void __rpc_depopulate(struct dentry *parent,
 	for (i = start; i < eof; i++) {
 		name.name = files[i].name;
 		name.len = strlen(files[i].name);
-		dentry = d_hash_and_lookup(parent, &name);
+		dentry = try_lookup_noperm(&name, parent);
 
 		if (dentry == NULL)
 			continue;
@@ -1190,7 +1190,7 @@ static const struct rpc_filelist files[] = {
 struct dentry *rpc_d_lookup_sb(const struct super_block *sb,
 			       const unsigned char *dir_name)
 {
-	return d_hash_and_lookup(sb->s_root, &QSTR(dir_name));
+	return try_lookup_noperm(&QSTR(dir_name), sb->s_root);
 }
 EXPORT_SYMBOL_GPL(rpc_d_lookup_sb);
 
@@ -1301,7 +1301,7 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 	struct dentry *pipe_dentry = NULL;
 
 	/* We should never get this far if "gssd" doesn't exist */
-	gssd_dentry = d_hash_and_lookup(root, &QSTR(files[RPCAUTH_gssd].name));
+	gssd_dentry = try_lookup_noperm(&QSTR(files[RPCAUTH_gssd].name), root);
 	if (!gssd_dentry)
 		return ERR_PTR(-ENOENT);
 
@@ -1311,8 +1311,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
 		goto out;
 	}
 
-	clnt_dentry = d_hash_and_lookup(gssd_dentry,
-					&QSTR(gssd_dummy_clnt_dir[0].name));
+	clnt_dentry = try_lookup_noperm(&QSTR(gssd_dummy_clnt_dir[0].name),
+					  gssd_dentry);
 	if (!clnt_dentry) {
 		__rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
 		pipe_dentry = ERR_PTR(-ENOENT);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 47480eb2189b..e67a8ce4b64c 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2158,8 +2158,8 @@ static int __init init_sel_fs(void)
 		return err;
 	}
 
-	selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root,
-						&null_name);
+	selinux_null.dentry = try_lookup_noperm(&null_name,
+						  selinux_null.mnt->mnt_root);
 	if (IS_ERR(selinux_null.dentry)) {
 		pr_err("selinuxfs:  could not lookup null!\n");
 		err = PTR_ERR(selinux_null.dentry);
-- 
2.48.1

[PATCH 6/6] VFS: change lookup_one_common and lookup_noperm_common to take a qstr

[NeilBrown]

From: NeilBrown <neilb@suse.de>

These function already take a qstr of course, but they also currently
take a name/len was well and fill in the qstr.
Now they take a qstr that is already filled in, which is what all the
callers have.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/namei.c | 44 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 16605f7108c0..e2fb61573f13 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2833,13 +2833,12 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 }
 EXPORT_SYMBOL(vfs_path_lookup);
 
-static int lookup_noperm_common(const char *name, struct dentry *base,
-				  int len,
-				  struct qstr *this)
+static int lookup_noperm_common(struct qstr *qname, struct dentry *base)
 {
-	this->name = name;
-	this->len = len;
-	this->hash = full_name_hash(base, name, len);
+	const char *name = qname->name;
+	u32 len = qname->len;
+
+	qname->hash = full_name_hash(base, name, len);
 	if (!len)
 		return -EACCES;
 
@@ -2856,7 +2855,7 @@ static int lookup_noperm_common(const char *name, struct dentry *base,
 	 * to use its own hash..
 	 */
 	if (base->d_flags & DCACHE_OP_HASH) {
-		int err = base->d_op->d_hash(base, this);
+		int err = base->d_op->d_hash(base, qname);
 		if (err < 0)
 			return err;
 	}
@@ -2864,10 +2863,10 @@ static int lookup_noperm_common(const char *name, struct dentry *base,
 }
 
 static int lookup_one_common(struct mnt_idmap *idmap,
-			     const char *name, struct dentry *base, int len,
-			     struct qstr *this) {
+			     struct qstr *qname, struct dentry *base)
+{
 	int err;
-	err = lookup_noperm_common(name, base, len, this);
+	err = lookup_noperm_common(qname, base);
 	if (err < 0)
 		return err;
 	return inode_permission(idmap, base->d_inode, MAY_EXEC);
@@ -2888,16 +2887,14 @@ static int lookup_one_common(struct mnt_idmap *idmap,
  */
 struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
 {
-	struct qstr this;
 	int err;
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_noperm_common(name->name, base, name->len, &this);
+	err = lookup_noperm_common(name, base);
 	if (err)
 		return ERR_PTR(err);
 
-	name->hash = this.hash;
 	return lookup_dcache(name, base, 0);
 }
 EXPORT_SYMBOL(try_lookup_noperm);
@@ -2915,17 +2912,16 @@ EXPORT_SYMBOL(try_lookup_noperm);
 struct dentry *lookup_noperm(struct qstr name, struct dentry *base)
 {
 	struct dentry *dentry;
-	struct qstr this;
 	int err;
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_noperm_common(name.name, base, name.len, &this);
+	err = lookup_noperm_common(&name, base);
 	if (err)
 		return ERR_PTR(err);
 
-	dentry = lookup_dcache(&this, base, 0);
-	return dentry ? dentry : __lookup_slow(&this, base, 0);
+	dentry = lookup_dcache(&name, base, 0);
+	return dentry ? dentry : __lookup_slow(&name, base, 0);
 }
 EXPORT_SYMBOL(lookup_noperm);
 
@@ -2943,17 +2939,16 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
 			  struct dentry *base)
 {
 	struct dentry *dentry;
-	struct qstr this;
 	int err;
 
 	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
 
-	err = lookup_one_common(idmap, name.name, base, name.len, &this);
+	err = lookup_one_common(idmap, &name, base);
 	if (err)
 		return ERR_PTR(err);
 
-	dentry = lookup_dcache(&this, base, 0);
-	return dentry ? dentry : __lookup_slow(&this, base, 0);
+	dentry = lookup_dcache(&name, base, 0);
+	return dentry ? dentry : __lookup_slow(&name, base, 0);
 }
 EXPORT_SYMBOL(lookup_one);
 
@@ -2971,17 +2966,16 @@ EXPORT_SYMBOL(lookup_one);
 struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
 				   struct qstr name, struct dentry *base)
 {
-	struct qstr this;
 	int err;
 	struct dentry *ret;
 
-	err = lookup_one_common(idmap, name.name, base, name.len, &this);
+	err = lookup_one_common(idmap, &name, base);
 	if (err)
 		return ERR_PTR(err);
 
-	ret = lookup_dcache(&this, base, 0);
+	ret = lookup_dcache(&name, base, 0);
 	if (!ret)
-		ret = lookup_slow(&this, base, 0);
+		ret = lookup_slow(&name, base, 0);
 	return ret;
 }
 EXPORT_SYMBOL(lookup_one_unlocked);
-- 
2.48.1

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Christian Brauner]

On Wed, 19 Mar 2025 14:01:32 +1100, NeilBrown wrote:
> The family of functions:
>   lookup_one()
>   lookup_one_unlocked()
>   lookup_one_positive_unlocked()
> 
> appear designed to be used by external clients of the filesystem rather
> than by filesystems acting on themselves as the lookup_one_len family
> are used.
> 
> [...]

Applied to the vfs-6.16.async.dir branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.async.dir branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.async.dir

[1/6] VFS: improve interface for lookup_one functions
      https://git.kernel.org/vfs/vfs/c/d01d332dbf28
[2/6] nfsd: Use lookup_one() rather than lookup_one_len()
      https://git.kernel.org/vfs/vfs/c/4c664a5962b7
[3/6] cachefiles: Use lookup_one() rather than lookup_one_len()
      https://git.kernel.org/vfs/vfs/c/ebc0dcbf5ba2
[4/6] VFS: rename lookup_one_len family to lookup_noperm and remove permission check
      https://git.kernel.org/vfs/vfs/c/12597aa5fea6
[5/6] Use try_lookup_noperm() instead of d_hash_and_lookup() outside of VFS
      https://git.kernel.org/vfs/vfs/c/d2bacbb4495a
[6/6] VFS: change lookup_one_common and lookup_noperm_common to take a qstr
      https://git.kernel.org/vfs/vfs/c/b496b0712e63

Re: [PATCH 0/6 RFC v2] tidy up various VFS lookup functions

[Christian Brauner]

On Wed, Mar 19, 2025 at 02:01:31PM +1100, NeilBrown wrote:
> This a revised version of a previous posting.  I have dropped the change
> to some lookup functions to pass a vfsmount.  I have also dropped the

Thank you for compromising! I appreciate it.

> I haven't included changes to afs because there are patches in vfs.all
> which make a lot of changes to lookup in afs.  I think (if they are seen
> as a good idea) these patches should aim to land after the afs patches
> and any further fixup in afs can happen then.

If you're fine with this then I suggest we delay this to v6.16. So I've
moved this to the vfs-6.16.async.dir branch which won't show up in -next
before the v6.15 merge window has concluded. I'm pushing this out now.

Re: [PATCH 0/6 RFC v2] tidy up various VFS lookup functions

[NeilBrown]

On Wed, 19 Mar 2025, Christian Brauner wrote:
> On Wed, Mar 19, 2025 at 02:01:31PM +1100, NeilBrown wrote:
> > This a revised version of a previous posting.  I have dropped the change
> > to some lookup functions to pass a vfsmount.  I have also dropped the
> 
> Thank you for compromising! I appreciate it.
> 
> > I haven't included changes to afs because there are patches in vfs.all
> > which make a lot of changes to lookup in afs.  I think (if they are seen
> > as a good idea) these patches should aim to land after the afs patches
> > and any further fixup in afs can happen then.
> 
> If you're fine with this then I suggest we delay this to v6.16. So I've
> moved this to the vfs-6.16.async.dir branch which won't show up in -next
> before the v6.15 merge window has concluded. I'm pushing this out now.
> 

I'm completely find with that - thanks.

NeilBrown

Re: [PATCH 2/6] nfsd: Use lookup_one() rather than lookup_one_len()

[Chuck Lever]

On 3/18/25 11:01 PM, NeilBrown wrote:
> nfsd uses some VFS interfaces (such as vfs_mkdir) which take an explicit
> mnt_idmap, and it passes &nop_mnt_idmap as nfsd doesn't yet support
> idmapped mounts.
> 
> It also uses the lookup_one_len() family of functions which implicitly
> use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> confusing.  When we eventually update nfsd to support idmap mounts it
> would be best if all places which need an idmap determined from the
> mount point were similar and easily found.
> 
> So this patch changes nfsd to use lookup_one(), lookup_one_unlocked(),
> and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.
> 
> This has the benefit of removing some uses of the lookup_one_len
> functions where permission checking is actually needed.  Many callers
> don't care about permission checking and using these function only where
> permission checking is needed is a valuable simplification.
> 
> This change requires passing the name in a qstr.  Currently this is a
> little clumsy, but if nfsd is changed to use qstr more broadly it will
> result in a net improvement.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs3proc.c    |  4 +++-
>  fs/nfsd/nfs3xdr.c     |  4 +++-
>  fs/nfsd/nfs4proc.c    |  4 +++-
>  fs/nfsd/nfs4recover.c | 13 +++++++------
>  fs/nfsd/nfs4xdr.c     |  4 +++-
>  fs/nfsd/nfsproc.c     |  6 ++++--
>  fs/nfsd/vfs.c         | 17 +++++++++--------
>  7 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 372bdcf5e07a..9fa8ad08b1cd 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -284,7 +284,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	inode_lock_nested(inode, I_MUTEX_PARENT);
>  
> -	child = lookup_one_len(argp->name, parent, argp->len);
> +	child = lookup_one(&nop_mnt_idmap,
> +			   QSTR_LEN(argp->name, argp->len),
> +			   parent);
>  	if (IS_ERR(child)) {
>  		status = nfserrno(PTR_ERR(child));
>  		goto out;
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index a7a07470c1f8..5a626e24a334 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -1001,7 +1001,9 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp,
>  		} else
>  			dchild = dget(dparent);
>  	} else
> -		dchild = lookup_positive_unlocked(name, dparent, namlen);
> +		dchild = lookup_one_positive_unlocked(&nop_mnt_idmap,
> +						      QSTR_LEN(name, namlen),
> +						      dparent);
>  	if (IS_ERR(dchild))
>  		return rv;
>  	if (d_mountpoint(dchild))
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f6e06c779d09..5860f3825be2 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -266,7 +266,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	inode_lock_nested(inode, I_MUTEX_PARENT);
>  
> -	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> +	child = lookup_one(&nop_mnt_idmap, QSTR_LEN(open->op_fname,
> +						    open->op_fnamelen),
> +			   parent);
>  	if (IS_ERR(child)) {
>  		status = nfserrno(PTR_ERR(child));
>  		goto out;
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index c1d9bd07285f..5c1cb5c3c13e 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -218,7 +218,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  	/* lock the parent */
>  	inode_lock(d_inode(dir));
>  
> -	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
> +	dentry = lookup_one(&nop_mnt_idmap, QSTR(dname), dir);
>  	if (IS_ERR(dentry)) {
>  		status = PTR_ERR(dentry);
>  		goto out_unlock;
> @@ -316,7 +316,8 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
>  	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
>  		if (!status) {
>  			struct dentry *dentry;
> -			dentry = lookup_one_len(entry->name, dir, HEXDIR_LEN-1);
> +			dentry = lookup_one(&nop_mnt_idmap,
> +					    QSTR(entry->name), dir);
>  			if (IS_ERR(dentry)) {
>  				status = PTR_ERR(dentry);
>  				break;
> @@ -339,16 +340,16 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
>  }
>  
>  static int
> -nfsd4_unlink_clid_dir(char *name, int namlen, struct nfsd_net *nn)
> +nfsd4_unlink_clid_dir(char *name, struct nfsd_net *nn)
>  {
>  	struct dentry *dir, *dentry;
>  	int status;
>  
> -	dprintk("NFSD: nfsd4_unlink_clid_dir. name %.*s\n", namlen, name);
> +	dprintk("NFSD: nfsd4_unlink_clid_dir. name %s\n", name);
>  
>  	dir = nn->rec_file->f_path.dentry;
>  	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
> -	dentry = lookup_one_len(name, dir, namlen);
> +	dentry = lookup_one(&nop_mnt_idmap, QSTR(name), dir);
>  	if (IS_ERR(dentry)) {
>  		status = PTR_ERR(dentry);
>  		goto out_unlock;
> @@ -408,7 +409,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
>  	if (status < 0)
>  		goto out_drop_write;
>  
> -	status = nfsd4_unlink_clid_dir(dname, HEXDIR_LEN-1, nn);
> +	status = nfsd4_unlink_clid_dir(dname, nn);
>  	nfs4_reset_creds(original_cred);
>  	if (status == 0) {
>  		vfs_fsync(nn->rec_file, 0);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e67420729ecd..16be860b1f79 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3812,7 +3812,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
>  	__be32 nfserr;
>  	int ignore_crossmnt = 0;
>  
> -	dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry, namlen);
> +	dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
> +					      QSTR_LEN(name, namlen),
> +					      cd->rd_fhp->fh_dentry);
>  	if (IS_ERR(dentry))
>  		return nfserrno(PTR_ERR(dentry));
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 6dda081eb24c..ac7d7f858846 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -312,7 +312,9 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  	}
>  
>  	inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
> -	dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
> +	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(argp->name,
> +						     argp->len),
> +			    dirfhp->fh_dentry);
>  	if (IS_ERR(dchild)) {
>  		resp->status = nfserrno(PTR_ERR(dchild));
>  		goto out_unlock;
> @@ -331,7 +333,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 */
>  		resp->status = nfserr_acces;
>  		if (!newfhp->fh_dentry) {
> -			printk(KERN_WARNING 
> +			printk(KERN_WARNING
>  				"nfsd_proc_create: file handle not verified\n");
>  			goto out_unlock;
>  		}
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 34d7aa531662..c0c94619af92 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -265,7 +265,8 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  				goto out_nfserr;
>  		}
>  	} else {
> -		dentry = lookup_one_len_unlocked(name, dparent, len);
> +		dentry = lookup_one_unlocked(&nop_mnt_idmap,
> +					     QSTR_LEN(name, len), dparent);
>  		host_err = PTR_ERR(dentry);
>  		if (IS_ERR(dentry))
>  			goto out_nfserr;
> @@ -923,7 +924,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
>  	 * directories, but we never have and it doesn't seem to have
>  	 * caused anyone a problem.  If we were to change this, note
>  	 * also that our filldir callbacks would need a variant of
> -	 * lookup_one_len that doesn't check permissions.
> +	 * lookup_one_positive_unlocked() that doesn't check permissions.
>  	 */
>  	if (type == S_IFREG)
>  		may_flags |= NFSD_MAY_OWNER_OVERRIDE;
> @@ -1555,7 +1556,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		return nfserrno(host_err);
>  
>  	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> -	dchild = lookup_one_len(fname, dentry, flen);
> +	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	host_err = PTR_ERR(dchild);
>  	if (IS_ERR(dchild)) {
>  		err = nfserrno(host_err);
> @@ -1660,7 +1661,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	dentry = fhp->fh_dentry;
>  	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> -	dnew = lookup_one_len(fname, dentry, flen);
> +	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	if (IS_ERR(dnew)) {
>  		err = nfserrno(PTR_ERR(dnew));
>  		inode_unlock(dentry->d_inode);
> @@ -1726,7 +1727,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  	dirp = d_inode(ddir);
>  	inode_lock_nested(dirp, I_MUTEX_PARENT);
>  
> -	dnew = lookup_one_len(name, ddir, len);
> +	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(name, len), ddir);
>  	if (IS_ERR(dnew)) {
>  		err = nfserrno(PTR_ERR(dnew));
>  		goto out_unlock;
> @@ -1839,7 +1840,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>  	if (err != nfs_ok)
>  		goto out_unlock;
>  
> -	odentry = lookup_one_len(fname, fdentry, flen);
> +	odentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), fdentry);
>  	host_err = PTR_ERR(odentry);
>  	if (IS_ERR(odentry))
>  		goto out_nfserr;
> @@ -1851,7 +1852,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>  	if (odentry == trap)
>  		goto out_dput_old;
>  
> -	ndentry = lookup_one_len(tname, tdentry, tlen);
> +	ndentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(tname, tlen), tdentry);
>  	host_err = PTR_ERR(ndentry);
>  	if (IS_ERR(ndentry))
>  		goto out_dput_old;
> @@ -1948,7 +1949,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	dirp = d_inode(dentry);
>  	inode_lock_nested(dirp, I_MUTEX_PARENT);
>  
> -	rdentry = lookup_one_len(fname, dentry, flen);
> +	rdentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	host_err = PTR_ERR(rdentry);
>  	if (IS_ERR(rdentry))
>  		goto out_unlock;

Acked-by: Chuck Lever <chuck.lever@oracle.com>

-- 
Chuck Lever

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> The family of functions:
>   lookup_one()
>   lookup_one_unlocked()
>   lookup_one_positive_unlocked()
> 
> appear designed to be used by external clients of the filesystem rather
> than by filesystems acting on themselves as the lookup_one_len family
> are used.
> 
> They are used by:
>    btrfs/ioctl - which is a user-space interface rather than an internal
>      activity
>    exportfs - i.e. from nfsd or the open_by_handle_at interface
>    overlayfs - at access the underlying filesystems
>    smb/server - for file service
> 
> They should be used by nfsd (more than just the exportfs path) and
> cachefs but aren't.
> 
> It would help if the documentation didn't claim they should "not be
> called by generic code".
> 

I read that as generic VFS code, since this is really intended to used
outside that, but I agree the term "generic" is vague here.

> Also the path component name is passed as "name" and "len" which are
> (confusingly?) separate by the "base".  In some cases the len in simply
> "strlen" and so passing a qstr using QSTR() would make the calling
> clearer.
> Other callers do pass separate name and len which are stored in a
> struct.  Sometimes these are already stored in a qstr, other times it
> easily could be.
> 
> So this patch changes these three functions to receive a 'struct qstr',
> and improves the documentation.
> 
> QSTR_LEN() is added to make it easy to pass a QSTR containing a known
> len.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst |  9 +++++
>  fs/btrfs/ioctl.c                      |  9 ++---
>  fs/exportfs/expfs.c                   |  5 ++-
>  fs/namei.c                            | 51 ++++++++++++---------------
>  fs/overlayfs/namei.c                  | 10 +++---
>  fs/overlayfs/overlayfs.h              |  2 +-
>  fs/overlayfs/readdir.c                |  9 ++---
>  fs/smb/server/smb2pdu.c               |  7 ++--
>  include/linux/dcache.h                |  3 +-
>  include/linux/namei.h                 |  9 +++--
>  10 files changed, 57 insertions(+), 57 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index 6817614e0820..06296ffd1e81 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1196,3 +1196,12 @@ should use d_drop();d_splice_alias() and return the result of the latter.
>  If a positive dentry cannot be returned for some reason, in-kernel
>  clients such as cachefiles, nfsd, smb/server may not perform ideally but
>  will fail-safe.
> +
> +---
> +
> +** mandatory**
> +
> +lookup_one(), lookup_one_unlocked(), lookup_one_positive_unlocked() now
> +take a qstr instead of a name and len.  These, not the "one_len"
> +versions, should be used whenever accessing a filesystem from outside
> +that filesysmtem, through a mount point - which will have a mnt_idmap.
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6c18bad53cd3..f94b638f9478 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -911,7 +911,7 @@ static noinline int btrfs_mksubvol(const struct path *parent,
>  	if (error == -EINTR)
>  		return error;
>  
> -	dentry = lookup_one(idmap, name, parent->dentry, namelen);
> +	dentry = lookup_one(idmap, QSTR_LEN(name, namelen), parent->dentry);
>  	error = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
>  		goto out_unlock;
> @@ -2289,7 +2289,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  	struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
>  	struct mnt_idmap *idmap = file_mnt_idmap(file);
>  	char *subvol_name, *subvol_name_ptr = NULL;
> -	int subvol_namelen;
>  	int ret = 0;
>  	bool destroy_parent = false;
>  
> @@ -2412,10 +2411,8 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  			goto out;
>  	}
>  
> -	subvol_namelen = strlen(subvol_name);
> -
>  	if (strchr(subvol_name, '/') ||
> -	    strncmp(subvol_name, "..", subvol_namelen) == 0) {
> +	    strcmp(subvol_name, "..") == 0) {
>  		ret = -EINVAL;
>  		goto free_subvol_name;
>  	}
> @@ -2428,7 +2425,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
>  	if (ret == -EINTR)
>  		goto free_subvol_name;
> -	dentry = lookup_one(idmap, subvol_name, parent, subvol_namelen);
> +	dentry = lookup_one(idmap, QSTR(subvol_name), parent);
>  	if (IS_ERR(dentry)) {
>  		ret = PTR_ERR(dentry);
>  		goto out_unlock_dir;
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 0c899cfba578..974b432087aa 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -145,7 +145,7 @@ static struct dentry *reconnect_one(struct vfsmount *mnt,
>  	if (err)
>  		goto out_err;
>  	dprintk("%s: found name: %s\n", __func__, nbuf);
> -	tmp = lookup_one_unlocked(mnt_idmap(mnt), nbuf, parent, strlen(nbuf));
> +	tmp = lookup_one_unlocked(mnt_idmap(mnt), QSTR(nbuf), parent);
>  	if (IS_ERR(tmp)) {
>  		dprintk("lookup failed: %ld\n", PTR_ERR(tmp));
>  		err = PTR_ERR(tmp);
> @@ -551,8 +551,7 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>  		}
>  
>  		inode_lock(target_dir->d_inode);
> -		nresult = lookup_one(mnt_idmap(mnt), nbuf,
> -				     target_dir, strlen(nbuf));
> +		nresult = lookup_one(mnt_idmap(mnt), QSTR(nbuf), target_dir);
>  		if (!IS_ERR(nresult)) {
>  			if (unlikely(nresult->d_inode != result->d_inode)) {
>  				dput(nresult);
> diff --git a/fs/namei.c b/fs/namei.c
> index b5abf456c5f4..75816fa80028 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2922,19 +2922,17 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  EXPORT_SYMBOL(lookup_one_len);
>  
>  /**
> - * lookup_one - filesystem helper to lookup single pathname component
> + * lookup_one - lookup single pathname component
>   * @idmap:	idmap of the mount the lookup is performed from
> - * @name:	pathname component to lookup
> + * @name:	qstr holding pathname component to lookup
>   * @base:	base directory to lookup from
> - * @len:	maximum length @len should be interpreted to
>   *
> - * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * This can be used for in-kernel filesystem clients such as file servers.
>   *
>   * The caller must hold base->i_mutex.
>   */
> -struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
> -			  struct dentry *base, int len)
> +struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
> +			  struct dentry *base)
>  {
>  	struct dentry *dentry;
>  	struct qstr this;
> @@ -2942,7 +2940,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(idmap, name, base, len, &this);
> +	err = lookup_one_common(idmap, name.name, base, name.len, &this);
>  	if (err)
>  		return ERR_PTR(err);
>  
> @@ -2952,27 +2950,24 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
>  EXPORT_SYMBOL(lookup_one);
>  
>  /**
> - * lookup_one_unlocked - filesystem helper to lookup single pathname component
> + * lookup_one_unlocked - lookup single pathname component
>   * @idmap:	idmap of the mount the lookup is performed from
> - * @name:	pathname component to lookup
> + * @name:	qstr olding pathname component to lookup
>   * @base:	base directory to lookup from
> - * @len:	maximum length @len should be interpreted to
>   *
> - * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * This can be used for in-kernel filesystem clients such as file servers.
>   *
>   * Unlike lookup_one_len, it should be called without the parent
> - * i_mutex held, and will take the i_mutex itself if necessary.
> + * i_rwsem held, and will take the i_rwsem itself if necessary.
>   */
>  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
> -				   const char *name, struct dentry *base,
> -				   int len)
> +				   struct qstr name, struct dentry *base)
>  {
>  	struct qstr this;
>  	int err;
>  	struct dentry *ret;
>  
> -	err = lookup_one_common(idmap, name, base, len, &this);
> +	err = lookup_one_common(idmap, name.name, base, name.len, &this);
>  	if (err)
>  		return ERR_PTR(err);
>  
> @@ -2984,12 +2979,10 @@ struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
>  EXPORT_SYMBOL(lookup_one_unlocked);
>  
>  /**
> - * lookup_one_positive_unlocked - filesystem helper to lookup single
> - *				  pathname component
> + * lookup_one_positive_unlocked - lookup single pathname component
>   * @idmap:	idmap of the mount the lookup is performed from
> - * @name:	pathname component to lookup
> + * @name:	qstr holding pathname component to lookup
>   * @base:	base directory to lookup from
> - * @len:	maximum length @len should be interpreted to
>   *
>   * This helper will yield ERR_PTR(-ENOENT) on negatives. The helper returns
>   * known positive or ERR_PTR(). This is what most of the users want.
> @@ -2998,16 +2991,15 @@ EXPORT_SYMBOL(lookup_one_unlocked);
>   * time, so callers of lookup_one_unlocked() need to be very careful; pinned
>   * positives have >d_inode stable, so this one avoids such problems.
>   *
> - * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * This can be used for in-kernel filesystem clients such as file servers.
>   *
> - * The helper should be called without i_mutex held.
> + * The helper should be called without i_rwsem held.
>   */
>  struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
> -					    const char *name,
> -					    struct dentry *base, int len)
> +					    struct qstr name,
> +					    struct dentry *base)
>  {
> -	struct dentry *ret = lookup_one_unlocked(idmap, name, base, len);
> +	struct dentry *ret = lookup_one_unlocked(idmap, name, base);
>  
>  	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
>  		dput(ret);
> @@ -3032,7 +3024,7 @@ EXPORT_SYMBOL(lookup_one_positive_unlocked);
>  struct dentry *lookup_one_len_unlocked(const char *name,
>  				       struct dentry *base, int len)
>  {
> -	return lookup_one_unlocked(&nop_mnt_idmap, name, base, len);
> +	return lookup_one_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len), base);
>  }
>  EXPORT_SYMBOL(lookup_one_len_unlocked);
>  
> @@ -3047,7 +3039,8 @@ EXPORT_SYMBOL(lookup_one_len_unlocked);
>  struct dentry *lookup_positive_unlocked(const char *name,
>  				       struct dentry *base, int len)
>  {
> -	return lookup_one_positive_unlocked(&nop_mnt_idmap, name, base, len);
> +	return lookup_one_positive_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len),
> +					    base);
>  }
>  EXPORT_SYMBOL(lookup_positive_unlocked);
>  
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index be5c65d6f848..6a6301e4bba5 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -205,8 +205,8 @@ static struct dentry *ovl_lookup_positive_unlocked(struct ovl_lookup_data *d,
>  						   struct dentry *base, int len,
>  						   bool drop_negative)
>  {
> -	struct dentry *ret = lookup_one_unlocked(mnt_idmap(d->layer->mnt), name,
> -						 base, len);
> +	struct dentry *ret = lookup_one_unlocked(mnt_idmap(d->layer->mnt),
> +						 QSTR_LEN(name, len), base);
>  
>  	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
>  		if (drop_negative && ret->d_lockref.count == 1) {
> @@ -789,8 +789,8 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	index = lookup_one_positive_unlocked(ovl_upper_mnt_idmap(ofs), name.name,
> -					     ofs->workdir, name.len);
> +	index = lookup_one_positive_unlocked(ovl_upper_mnt_idmap(ofs), name,
> +					     ofs->workdir);
>  	if (IS_ERR(index)) {
>  		err = PTR_ERR(index);
>  		if (err == -ENOENT) {
> @@ -1396,7 +1396,7 @@ bool ovl_lower_positive(struct dentry *dentry)
>  
>  		this = lookup_one_positive_unlocked(
>  				mnt_idmap(parentpath->layer->mnt),
> -				name->name, parentpath->dentry, name->len);
> +				*name, parentpath->dentry);
>  		if (IS_ERR(this)) {
>  			switch (PTR_ERR(this)) {
>  			case -ENOENT:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 6f2f8f4cfbbc..ceaf4eb199c7 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -402,7 +402,7 @@ static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
>  					      const char *name,
>  					      struct dentry *base, int len)
>  {
> -	return lookup_one(ovl_upper_mnt_idmap(ofs), name, base, len);
> +	return lookup_one(ovl_upper_mnt_idmap(ofs), QSTR_LEN(name, len), base);
>  }
>  
>  static inline bool ovl_open_flags_need_copy_up(int flags)
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 881ec5592da5..68df61f4bba7 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -271,7 +271,6 @@ static bool ovl_fill_merge(struct dir_context *ctx, const char *name,
>  static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data *rdd)
>  {
>  	int err;
> -	struct ovl_cache_entry *p;
>  	struct dentry *dentry, *dir = path->dentry;
>  	const struct cred *old_cred;
>  
> @@ -280,9 +279,11 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
>  	err = down_write_killable(&dir->d_inode->i_rwsem);
>  	if (!err) {
>  		while (rdd->first_maybe_whiteout) {
> -			p = rdd->first_maybe_whiteout;
> +			struct ovl_cache_entry *p =
> +				rdd->first_maybe_whiteout;
>  			rdd->first_maybe_whiteout = p->next_maybe_whiteout;
> -			dentry = lookup_one(mnt_idmap(path->mnt), p->name, dir, p->len);
> +			dentry = lookup_one(mnt_idmap(path->mnt),
> +					    QSTR_LEN(p->name, p->len), dir);
>  			if (!IS_ERR(dentry)) {
>  				p->is_whiteout = ovl_is_whiteout(dentry);
>  				dput(dentry);
> @@ -492,7 +493,7 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
>  		}
>  	}
>  	/* This checks also for xwhiteouts */
> -	this = lookup_one(mnt_idmap(path->mnt), p->name, dir, p->len);
> +	this = lookup_one(mnt_idmap(path->mnt), QSTR_LEN(p->name, p->len), dir);
>  	if (IS_ERR_OR_NULL(this) || !this->d_inode) {
>  		/* Mark a stale entry */
>  		p->is_whiteout = true;
> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index f1efcd027475..c862e3bd4531 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -4091,9 +4091,10 @@ static int process_query_dir_entries(struct smb2_query_dir_private *priv)
>  			return -EINVAL;
>  
>  		lock_dir(priv->dir_fp);
> -		dent = lookup_one(idmap, priv->d_info->name,
> -				  priv->dir_fp->filp->f_path.dentry,
> -				  priv->d_info->name_len);
> +		dent = lookup_one(idmap,
> +				  QSTR_LEN(priv->d_info->name,
> +					   priv->d_info->name_len),
> +				  priv->dir_fp->filp->f_path.dentry);
>  		unlock_dir(priv->dir_fp);
>  
>  		if (IS_ERR(dent)) {
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 45bff10d3773..1f01f4e734c5 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -57,7 +57,8 @@ struct qstr {
>  };
>  
>  #define QSTR_INIT(n,l) { { { .len = l } }, .name = n }
> -#define QSTR(n) (struct qstr)QSTR_INIT(n, strlen(n))
> +#define QSTR_LEN(n,l) (struct qstr)QSTR_INIT(n,l)
> +#define QSTR(n) QSTR_LEN(n, strlen(n))
>  
>  extern const struct qstr empty_name;
>  extern const struct qstr slash_name;
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index e3042176cdf4..508dae67e3c5 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -73,13 +73,12 @@ extern struct dentry *try_lookup_one_len(const char *, struct dentry *, int);
>  extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
>  extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
>  extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
> -struct dentry *lookup_one(struct mnt_idmap *, const char *, struct dentry *, int);
> +struct dentry *lookup_one(struct mnt_idmap *, struct qstr, struct dentry *);
>  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
> -				   const char *name, struct dentry *base,
> -				   int len);
> +				   struct qstr name, struct dentry *base);
>  struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
> -					    const char *name,
> -					    struct dentry *base, int len);
> +					    struct qstr name,
> +					    struct dentry *base);
>  
>  extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *path, unsigned int flags);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/6] nfsd: Use lookup_one() rather than lookup_one_len()

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> nfsd uses some VFS interfaces (such as vfs_mkdir) which take an explicit
> mnt_idmap, and it passes &nop_mnt_idmap as nfsd doesn't yet support
> idmapped mounts.
> 
> It also uses the lookup_one_len() family of functions which implicitly
> use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> confusing.  When we eventually update nfsd to support idmap mounts it
> would be best if all places which need an idmap determined from the
> mount point were similar and easily found.
> 
> So this patch changes nfsd to use lookup_one(), lookup_one_unlocked(),
> and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.
> 
> This has the benefit of removing some uses of the lookup_one_len
> functions where permission checking is actually needed.  Many callers
> don't care about permission checking and using these function only where
> permission checking is needed is a valuable simplification.
> 
> This change requires passing the name in a qstr.  Currently this is a
> little clumsy, but if nfsd is changed to use qstr more broadly it will
> result in a net improvement.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs3proc.c    |  4 +++-
>  fs/nfsd/nfs3xdr.c     |  4 +++-
>  fs/nfsd/nfs4proc.c    |  4 +++-
>  fs/nfsd/nfs4recover.c | 13 +++++++------
>  fs/nfsd/nfs4xdr.c     |  4 +++-
>  fs/nfsd/nfsproc.c     |  6 ++++--
>  fs/nfsd/vfs.c         | 17 +++++++++--------
>  7 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 372bdcf5e07a..9fa8ad08b1cd 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -284,7 +284,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	inode_lock_nested(inode, I_MUTEX_PARENT);
>  
> -	child = lookup_one_len(argp->name, parent, argp->len);
> +	child = lookup_one(&nop_mnt_idmap,
> +			   QSTR_LEN(argp->name, argp->len),
> +			   parent);
>  	if (IS_ERR(child)) {
>  		status = nfserrno(PTR_ERR(child));
>  		goto out;
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index a7a07470c1f8..5a626e24a334 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -1001,7 +1001,9 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp,
>  		} else
>  			dchild = dget(dparent);
>  	} else
> -		dchild = lookup_positive_unlocked(name, dparent, namlen);
> +		dchild = lookup_one_positive_unlocked(&nop_mnt_idmap,
> +						      QSTR_LEN(name, namlen),
> +						      dparent);
>  	if (IS_ERR(dchild))
>  		return rv;
>  	if (d_mountpoint(dchild))
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f6e06c779d09..5860f3825be2 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -266,7 +266,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	inode_lock_nested(inode, I_MUTEX_PARENT);
>  
> -	child = lookup_one_len(open->op_fname, parent, open->op_fnamelen);
> +	child = lookup_one(&nop_mnt_idmap, QSTR_LEN(open->op_fname,
> +						    open->op_fnamelen),
> +			   parent);
>  	if (IS_ERR(child)) {
>  		status = nfserrno(PTR_ERR(child));
>  		goto out;
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index c1d9bd07285f..5c1cb5c3c13e 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -218,7 +218,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  	/* lock the parent */
>  	inode_lock(d_inode(dir));
>  
> -	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
> +	dentry = lookup_one(&nop_mnt_idmap, QSTR(dname), dir);
>  	if (IS_ERR(dentry)) {
>  		status = PTR_ERR(dentry);
>  		goto out_unlock;
> @@ -316,7 +316,8 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
>  	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
>  		if (!status) {
>  			struct dentry *dentry;
> -			dentry = lookup_one_len(entry->name, dir, HEXDIR_LEN-1);
> +			dentry = lookup_one(&nop_mnt_idmap,
> +					    QSTR(entry->name), dir);
>  			if (IS_ERR(dentry)) {
>  				status = PTR_ERR(dentry);
>  				break;
> @@ -339,16 +340,16 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
>  }
>  
>  static int
> -nfsd4_unlink_clid_dir(char *name, int namlen, struct nfsd_net *nn)
> +nfsd4_unlink_clid_dir(char *name, struct nfsd_net *nn)
>  {
>  	struct dentry *dir, *dentry;
>  	int status;
>  
> -	dprintk("NFSD: nfsd4_unlink_clid_dir. name %.*s\n", namlen, name);
> +	dprintk("NFSD: nfsd4_unlink_clid_dir. name %s\n", name);
>  
>  	dir = nn->rec_file->f_path.dentry;
>  	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
> -	dentry = lookup_one_len(name, dir, namlen);
> +	dentry = lookup_one(&nop_mnt_idmap, QSTR(name), dir);
>  	if (IS_ERR(dentry)) {
>  		status = PTR_ERR(dentry);
>  		goto out_unlock;
> @@ -408,7 +409,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
>  	if (status < 0)
>  		goto out_drop_write;
>  
> -	status = nfsd4_unlink_clid_dir(dname, HEXDIR_LEN-1, nn);
> +	status = nfsd4_unlink_clid_dir(dname, nn);
>  	nfs4_reset_creds(original_cred);
>  	if (status == 0) {
>  		vfs_fsync(nn->rec_file, 0);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e67420729ecd..16be860b1f79 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3812,7 +3812,9 @@ nfsd4_encode_entry4_fattr(struct nfsd4_readdir *cd, const char *name,
>  	__be32 nfserr;
>  	int ignore_crossmnt = 0;
>  
> -	dentry = lookup_positive_unlocked(name, cd->rd_fhp->fh_dentry, namlen);
> +	dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
> +					      QSTR_LEN(name, namlen),
> +					      cd->rd_fhp->fh_dentry);
>  	if (IS_ERR(dentry))
>  		return nfserrno(PTR_ERR(dentry));
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 6dda081eb24c..ac7d7f858846 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -312,7 +312,9 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  	}
>  
>  	inode_lock_nested(dirfhp->fh_dentry->d_inode, I_MUTEX_PARENT);
> -	dchild = lookup_one_len(argp->name, dirfhp->fh_dentry, argp->len);
> +	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(argp->name,
> +						     argp->len),
> +			    dirfhp->fh_dentry);
>  	if (IS_ERR(dchild)) {
>  		resp->status = nfserrno(PTR_ERR(dchild));
>  		goto out_unlock;
> @@ -331,7 +333,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  		 */
>  		resp->status = nfserr_acces;
>  		if (!newfhp->fh_dentry) {
> -			printk(KERN_WARNING 
> +			printk(KERN_WARNING
>  				"nfsd_proc_create: file handle not verified\n");
>  			goto out_unlock;
>  		}
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 34d7aa531662..c0c94619af92 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -265,7 +265,8 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  				goto out_nfserr;
>  		}
>  	} else {
> -		dentry = lookup_one_len_unlocked(name, dparent, len);
> +		dentry = lookup_one_unlocked(&nop_mnt_idmap,
> +					     QSTR_LEN(name, len), dparent);
>  		host_err = PTR_ERR(dentry);
>  		if (IS_ERR(dentry))
>  			goto out_nfserr;
> @@ -923,7 +924,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
>  	 * directories, but we never have and it doesn't seem to have
>  	 * caused anyone a problem.  If we were to change this, note
>  	 * also that our filldir callbacks would need a variant of
> -	 * lookup_one_len that doesn't check permissions.
> +	 * lookup_one_positive_unlocked() that doesn't check permissions.
>  	 */
>  	if (type == S_IFREG)
>  		may_flags |= NFSD_MAY_OWNER_OVERRIDE;
> @@ -1555,7 +1556,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		return nfserrno(host_err);
>  
>  	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> -	dchild = lookup_one_len(fname, dentry, flen);
> +	dchild = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	host_err = PTR_ERR(dchild);
>  	if (IS_ERR(dchild)) {
>  		err = nfserrno(host_err);
> @@ -1660,7 +1661,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  
>  	dentry = fhp->fh_dentry;
>  	inode_lock_nested(dentry->d_inode, I_MUTEX_PARENT);
> -	dnew = lookup_one_len(fname, dentry, flen);
> +	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	if (IS_ERR(dnew)) {
>  		err = nfserrno(PTR_ERR(dnew));
>  		inode_unlock(dentry->d_inode);
> @@ -1726,7 +1727,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
>  	dirp = d_inode(ddir);
>  	inode_lock_nested(dirp, I_MUTEX_PARENT);
>  
> -	dnew = lookup_one_len(name, ddir, len);
> +	dnew = lookup_one(&nop_mnt_idmap, QSTR_LEN(name, len), ddir);
>  	if (IS_ERR(dnew)) {
>  		err = nfserrno(PTR_ERR(dnew));
>  		goto out_unlock;
> @@ -1839,7 +1840,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>  	if (err != nfs_ok)
>  		goto out_unlock;
>  
> -	odentry = lookup_one_len(fname, fdentry, flen);
> +	odentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), fdentry);
>  	host_err = PTR_ERR(odentry);
>  	if (IS_ERR(odentry))
>  		goto out_nfserr;
> @@ -1851,7 +1852,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
>  	if (odentry == trap)
>  		goto out_dput_old;
>  
> -	ndentry = lookup_one_len(tname, tdentry, tlen);
> +	ndentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(tname, tlen), tdentry);
>  	host_err = PTR_ERR(ndentry);
>  	if (IS_ERR(ndentry))
>  		goto out_dput_old;
> @@ -1948,7 +1949,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
>  	dirp = d_inode(dentry);
>  	inode_lock_nested(dirp, I_MUTEX_PARENT);
>  
> -	rdentry = lookup_one_len(fname, dentry, flen);
> +	rdentry = lookup_one(&nop_mnt_idmap, QSTR_LEN(fname, flen), dentry);
>  	host_err = PTR_ERR(rdentry);
>  	if (IS_ERR(rdentry))
>  		goto out_unlock;

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
> 
> cachefiles uses some VFS interfaces (such as vfs_mkdir) which take an
> explicit mnt_idmap, and it passes &nop_mnt_idmap as cachefiles doesn't
> yet support idmapped mounts.
> 
> It also uses the lookup_one_len() family of functions which implicitly
> use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> confusing.  When we eventually update cachefiles to support idmap mounts it

Is that something we ever plan to do?

> would be best if all places which need an idmap determined from the
> mount point were similar and easily found.
> 
> So this patch changes cachefiles to use lookup_one(), lookup_one_unlocked(),
> and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.
> 
> This has the benefit of removing the remaining user of the
> lookup_one_len functions where permission checking is actually needed.
> Other callers don't care about permission checking and using these
> function only where permission checking is needed is a valuable
> simplification.
> 
> This requires passing the name in a qstr.  This is easily done with
> QSTR() as the name is always nul terminated, and often strlen is used
> anyway.  ->d_name_len is removed as no longer useful.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/cachefiles/internal.h |  1 -
>  fs/cachefiles/key.c      |  1 -
>  fs/cachefiles/namei.c    | 14 +++++++-------
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index 38c236e38cef..b62cd3e9a18e 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -71,7 +71,6 @@ struct cachefiles_object {
>  	int				debug_id;
>  	spinlock_t			lock;
>  	refcount_t			ref;
> -	u8				d_name_len;	/* Length of filename */
>  	enum cachefiles_content		content_info:8;	/* Info about content presence */
>  	unsigned long			flags;
>  #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
> diff --git a/fs/cachefiles/key.c b/fs/cachefiles/key.c
> index bf935e25bdbe..4927b533b9ae 100644
> --- a/fs/cachefiles/key.c
> +++ b/fs/cachefiles/key.c
> @@ -132,7 +132,6 @@ bool cachefiles_cook_key(struct cachefiles_object *object)
>  success:
>  	name[len] = 0;
>  	object->d_name = name;
> -	object->d_name_len = len;
>  	_leave(" = %s", object->d_name);
>  	return true;
>  }
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index 83a60126de0f..4fc6f3efd3d9 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -98,7 +98,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
>  retry:
>  	ret = cachefiles_inject_read_error();
>  	if (ret == 0)
> -		subdir = lookup_one_len(dirname, dir, strlen(dirname));
> +		subdir = lookup_one(&nop_mnt_idmap, QSTR(dirname), dir);
>  	else
>  		subdir = ERR_PTR(ret);
>  	trace_cachefiles_lookup(NULL, dir, subdir);
> @@ -337,7 +337,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  		return -EIO;
>  	}
>  
> -	grave = lookup_one_len(nbuffer, cache->graveyard, strlen(nbuffer));
> +	grave = lookup_one(&nop_mnt_idmap, QSTR(nbuffer), cache->graveyard);
>  	if (IS_ERR(grave)) {
>  		unlock_rename(cache->graveyard, dir);
>  		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> @@ -629,8 +629,8 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
>  	/* Look up path "cache/vol/fanout/file". */
>  	ret = cachefiles_inject_read_error();
>  	if (ret == 0)
> -		dentry = lookup_positive_unlocked(object->d_name, fan,
> -						  object->d_name_len);
> +		dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
> +						      QSTR(object->d_name), fan);
>  	else
>  		dentry = ERR_PTR(ret);
>  	trace_cachefiles_lookup(object, fan, dentry);
> @@ -682,7 +682,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
>  	inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
>  	ret = cachefiles_inject_read_error();
>  	if (ret == 0)
> -		dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
> +		dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
>  	else
>  		dentry = ERR_PTR(ret);
>  	if (IS_ERR(dentry)) {
> @@ -701,7 +701,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
>  		dput(dentry);
>  		ret = cachefiles_inject_read_error();
>  		if (ret == 0)
> -			dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
> +			dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
>  		else
>  			dentry = ERR_PTR(ret);
>  		if (IS_ERR(dentry)) {
> @@ -750,7 +750,7 @@ static struct dentry *cachefiles_lookup_for_cull(struct cachefiles_cache *cache,
>  
>  	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
>  
> -	victim = lookup_one_len(filename, dir, strlen(filename));
> +	victim = lookup_one(&nop_mnt_idmap, QSTR(filename), dir);
>  	if (IS_ERR(victim))
>  		goto lookup_error;
>  	if (d_is_negative(victim))

Patch looks sane though.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/6] VFS: rename lookup_one_len family to lookup_noperm and remove permission check

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> The lookup_one_len family of functions is (now) only used internally by
> a filesystem on itself either
> - in a context where permission checking is irrelevant such as by a
>   virtual filesystem populating itself, or xfs accessing its ORPHANAGE
>   or dquota accessing the quota file; or
> - in a context where a permission check (MAY_EXEC on the parent) has just
>   been performed such as a network filesystem finding in "silly-rename"
>   file in the same directory.  This is also the context after the
>   _parentat() functions where currently lookup_one_qstr_excl() is used.
> 
> So the permission check is pointless.
> 
> The name "one_len" is unhelpful in understanding the purpose of these
> functions and should be changed.  Most of the callers pass the len as
> "strlen()" so using a qstr and QSTR() can simplify the code.
> 
> This patch renames these functions (include lookup_positive_unlocked()
> which is part of the family despite the name) to have a name based on
> "lookup_noperm".  They are changed to receive a 'struct qstr' instead
> of separate name and len.  In a few cases the use of QSTR() results in a
> new call to strlen().
> 
> try_lookup_noperm() takes a pointer to a qstr instead of the whole
> qstr.  This is consistent with d_hash_and_lookup() (which is nearly
> identical) and useful for lookup_noperm_unlocked().
> 
> The new lookup_noperm_common() doesn't take a qstr yet.  That will be
> tidied up in a subsequent patch.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst | 20 +++++++
>  arch/s390/hypfs/inode.c               |  2 +-
>  drivers/android/binderfs.c            |  4 +-
>  drivers/infiniband/hw/qib/qib_fs.c    |  4 +-
>  fs/afs/dir.c                          |  2 +-
>  fs/afs/dir_silly.c                    |  6 +-
>  fs/autofs/dev-ioctl.c                 |  3 +-
>  fs/binfmt_misc.c                      |  2 +-
>  fs/debugfs/inode.c                    |  6 +-
>  fs/ecryptfs/inode.c                   | 16 ++---
>  fs/kernfs/mount.c                     |  4 +-
>  fs/namei.c                            | 86 ++++++++++++++++-----------
>  fs/nfs/unlink.c                       | 11 ++--
>  fs/overlayfs/export.c                 |  6 +-
>  fs/overlayfs/namei.c                  |  2 +-
>  fs/quota/dquot.c                      |  2 +-
>  fs/smb/client/cached_dir.c            |  5 +-
>  fs/smb/client/cifsfs.c                |  3 +-
>  fs/tracefs/inode.c                    |  2 +-
>  fs/xfs/scrub/orphanage.c              |  3 +-
>  include/linux/namei.h                 |  8 +--
>  ipc/mqueue.c                          |  5 +-
>  kernel/bpf/inode.c                    |  2 +-
>  security/apparmor/apparmorfs.c        |  4 +-
>  security/inode.c                      |  2 +-
>  25 files changed, 123 insertions(+), 87 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index 06296ffd1e81..df9516cd82e0 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1205,3 +1205,23 @@ lookup_one(), lookup_one_unlocked(), lookup_one_positive_unlocked() now
>  take a qstr instead of a name and len.  These, not the "one_len"
>  versions, should be used whenever accessing a filesystem from outside
>  that filesysmtem, through a mount point - which will have a mnt_idmap.
> +
> +---
> +
> +** mandatory**
> +
> +Functions try_lookup_one_len(), lookup_one_len(),
> +lookup_one_len_unlocked() and lookup_positive_unlocked() have been
> +renamed to try_lookup_noperm(), lookup_noperm(),
> +lookup_noperm_unlocked(), lookup_noperm_positive_unlocked().  They now
> +take a qstr instead of separate name and length.  QSTR() can be used
> +when strlen() is needed for the length.
> +
> +For try_lookup_noperm() a reference to the qstr is passed in case the
> +hash might subsequently be needed.
> +
> +These function no longer do any permission checking - they previously
> +checked that the caller has 'X' permission on the parent.  They must
> +ONLY be used internally by a filesystem on itself when it knows that
> +permissions are irrelevant or in a context where permission checks have
> +already been performed such as after vfs_path_parent_lookup()
> diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
> index d428635abf08..2eed957393f4 100644
> --- a/arch/s390/hypfs/inode.c
> +++ b/arch/s390/hypfs/inode.c
> @@ -341,7 +341,7 @@ static struct dentry *hypfs_create_file(struct dentry *parent, const char *name,
>  	struct inode *inode;
>  
>  	inode_lock(d_inode(parent));
> -	dentry = lookup_one_len(name, parent, strlen(name));
> +	dentry = lookup_noperm(QSTR(name), parent);
>  	if (IS_ERR(dentry)) {
>  		dentry = ERR_PTR(-ENOMEM);
>  		goto fail;
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index bc6bae76ccaf..172e825168c8 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -187,7 +187,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
>  	inode_lock(d_inode(root));
>  
>  	/* look it up */
> -	dentry = lookup_one_len(name, root, name_len);
> +	dentry = lookup_noperm(QSTR(name), root);
>  	if (IS_ERR(dentry)) {
>  		inode_unlock(d_inode(root));
>  		ret = PTR_ERR(dentry);
> @@ -486,7 +486,7 @@ static struct dentry *binderfs_create_dentry(struct dentry *parent,
>  {
>  	struct dentry *dentry;
>  
> -	dentry = lookup_one_len(name, parent, strlen(name));
> +	dentry = lookup_noperm(QSTR(name), parent);
>  	if (IS_ERR(dentry))
>  		return dentry;
>  
> diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
> index b27791029fa9..c3aa40c3fb8f 100644
> --- a/drivers/infiniband/hw/qib/qib_fs.c
> +++ b/drivers/infiniband/hw/qib/qib_fs.c
> @@ -89,7 +89,7 @@ static int create_file(const char *name, umode_t mode,
>  	int error;
>  
>  	inode_lock(d_inode(parent));
> -	*dentry = lookup_one_len(name, parent, strlen(name));
> +	*dentry = lookup_noperm(QSTR(name), parent);
>  	if (!IS_ERR(*dentry))
>  		error = qibfs_mknod(d_inode(parent), *dentry,
>  				    mode, fops, data);
> @@ -432,7 +432,7 @@ static int remove_device_files(struct super_block *sb,
>  	char unit[10];
>  
>  	snprintf(unit, sizeof(unit), "%u", dd->unit);
> -	dir = lookup_one_len_unlocked(unit, sb->s_root, strlen(unit));
> +	dir = lookup_noperm_unlocked(QSTR(unit), sb->s_root);
>  
>  	if (IS_ERR(dir)) {
>  		pr_err("Lookup of %s failed\n", unit);
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 5bddcc20786e..0e633535d2c7 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -943,7 +943,7 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
>  		}
>  
>  		strcpy(p, name);
> -		ret = lookup_one_len(buf, dentry->d_parent, len);
> +		ret = lookup_noperm(QSTR(buf), dentry->d_parent);
>  		if (IS_ERR(ret) || d_is_positive(ret))
>  			goto out_s;
>  		dput(ret);
> diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
> index a1e581946b93..292a5c8c752a 100644
> --- a/fs/afs/dir_silly.c
> +++ b/fs/afs/dir_silly.c
> @@ -113,16 +113,14 @@ int afs_sillyrename(struct afs_vnode *dvnode, struct afs_vnode *vnode,
>  
>  	sdentry = NULL;
>  	do {
> -		int slen;
> -
>  		dput(sdentry);
>  		sillycounter++;
>  
>  		/* Create a silly name.  Note that the ".__afs" prefix is
>  		 * understood by the salvager and must not be changed.
>  		 */
> -		slen = scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
> -		sdentry = lookup_one_len(silly, dentry->d_parent, slen);
> +		scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
> +		sdentry = lookup_noperm(QSTR(silly), dentry->d_parent);
>  
>  		/* N.B. Better to return EBUSY here ... it could be dangerous
>  		 * to delete the file while it's in use.
> diff --git a/fs/autofs/dev-ioctl.c b/fs/autofs/dev-ioctl.c
> index 6d57efbb8110..a577b175c38a 100644
> --- a/fs/autofs/dev-ioctl.c
> +++ b/fs/autofs/dev-ioctl.c
> @@ -461,7 +461,8 @@ static int autofs_dev_ioctl_timeout(struct file *fp,
>  				"prevent shutdown\n");
>  
>  		inode_lock_shared(inode);
> -		dentry = try_lookup_one_len(param->path, base, path_len);
> +		dentry = try_lookup_noperm(&QSTR_LEN(param->path, path_len),
> +					   base);
>  		inode_unlock_shared(inode);
>  		if (IS_ERR_OR_NULL(dentry))
>  			return dentry ? PTR_ERR(dentry) : -ENOENT;
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 5a7ebd160724..f0996a76ee7c 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -842,7 +842,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>  	}
>  
>  	inode_lock(d_inode(root));
> -	dentry = lookup_one_len(e->name, root, strlen(e->name));
> +	dentry = lookup_noperm(QSTR(e->name), root);
>  	err = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
>  		goto out;
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 75715d8877ee..feb5ccb5bba8 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -346,7 +346,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
>  	if (!parent)
>  		parent = debugfs_mount->mnt_root;
>  
> -	dentry = lookup_positive_unlocked(name, parent, strlen(name));
> +	dentry = lookup_noperm_positive_unlocked(QSTR(name), parent);
>  	if (IS_ERR(dentry))
>  		return NULL;
>  	return dentry;
> @@ -388,7 +388,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
>  	if (unlikely(IS_DEADDIR(d_inode(parent))))
>  		dentry = ERR_PTR(-ENOENT);
>  	else
> -		dentry = lookup_one_len(name, parent, strlen(name));
> +		dentry = lookup_noperm(QSTR(name), parent);
>  	if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
>  		if (d_is_dir(dentry))
>  			pr_err("Directory '%s' with parent '%s' already present!\n",
> @@ -872,7 +872,7 @@ int __printf(2, 3) debugfs_change_name(struct dentry *dentry, const char *fmt, .
>  	}
>  	if (strcmp(old_name.name.name, new_name) == 0)
>  		goto out;
> -	target = lookup_one_len(new_name, parent, strlen(new_name));
> +	target = lookup_noperm(QSTR(new_name), parent);
>  	if (IS_ERR(target)) {
>  		error = PTR_ERR(target);
>  		goto out;
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 51a5c54eb740..9a0d40bb43d6 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -394,8 +394,8 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode,
>  	char *encrypted_and_encoded_name = NULL;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
>  	struct dentry *lower_dir_dentry, *lower_dentry;
> -	const char *name = ecryptfs_dentry->d_name.name;
> -	size_t len = ecryptfs_dentry->d_name.len;
> +	struct qstr qname = QSTR_INIT(ecryptfs_dentry->d_name.name,
> +				      ecryptfs_dentry->d_name.len);
>  	struct dentry *res;
>  	int rc = 0;
>  
> @@ -404,23 +404,25 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode,
>  	mount_crypt_stat = &ecryptfs_superblock_to_private(
>  				ecryptfs_dentry->d_sb)->mount_crypt_stat;
>  	if (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) {
> +		size_t len = qname.len;
>  		rc = ecryptfs_encrypt_and_encode_filename(
>  			&encrypted_and_encoded_name, &len,
> -			mount_crypt_stat, name, len);
> +			mount_crypt_stat, qname.name, len);
>  		if (rc) {
>  			printk(KERN_ERR "%s: Error attempting to encrypt and encode "
>  			       "filename; rc = [%d]\n", __func__, rc);
>  			return ERR_PTR(rc);
>  		}
> -		name = encrypted_and_encoded_name;
> +		qname.name = encrypted_and_encoded_name;
> +		qname.len = len;
>  	}
>  
> -	lower_dentry = lookup_one_len_unlocked(name, lower_dir_dentry, len);
> +	lower_dentry = lookup_noperm_unlocked(qname, lower_dir_dentry);
>  	if (IS_ERR(lower_dentry)) {
> -		ecryptfs_printk(KERN_DEBUG, "%s: lookup_one_len() returned "
> +		ecryptfs_printk(KERN_DEBUG, "%s: lookup_noperm() returned "
>  				"[%ld] on lower_dentry = [%s]\n", __func__,
>  				PTR_ERR(lower_dentry),
> -				name);
> +				qname.name);
>  		res = ERR_CAST(lower_dentry);
>  	} else {
>  		res = ecryptfs_lookup_interpose(ecryptfs_dentry, lower_dentry);
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index 1358c21837f1..46943a7fb4df 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -233,8 +233,8 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
>  			dput(dentry);
>  			return ERR_PTR(-EINVAL);
>  		}
> -		dtmp = lookup_positive_unlocked(kntmp->name, dentry,
> -					       strlen(kntmp->name));
> +		dtmp = lookup_noperm_positive_unlocked(QSTR(kntmp->name),
> +							 dentry);
>  		dput(dentry);
>  		if (IS_ERR(dtmp))
>  			return dtmp;
> diff --git a/fs/namei.c b/fs/namei.c
> index 75816fa80028..16605f7108c0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2833,9 +2833,9 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
>  }
>  EXPORT_SYMBOL(vfs_path_lookup);
>  
> -static int lookup_one_common(struct mnt_idmap *idmap,
> -			     const char *name, struct dentry *base, int len,
> -			     struct qstr *this)
> +static int lookup_noperm_common(const char *name, struct dentry *base,
> +				  int len,
> +				  struct qstr *this)
>  {
>  	this->name = name;
>  	this->len = len;
> @@ -2860,51 +2860,59 @@ static int lookup_one_common(struct mnt_idmap *idmap,
>  		if (err < 0)
>  			return err;
>  	}
> +	return 0;
> +}
>  
> +static int lookup_one_common(struct mnt_idmap *idmap,
> +			     const char *name, struct dentry *base, int len,
> +			     struct qstr *this) {
> +	int err;
> +	err = lookup_noperm_common(name, base, len, this);
> +	if (err < 0)
> +		return err;
>  	return inode_permission(idmap, base->d_inode, MAY_EXEC);
>  }
>  
>  /**
> - * try_lookup_one_len - filesystem helper to lookup single pathname component
> - * @name:	pathname component to lookup
> + * try_lookup_noperm - filesystem helper to lookup single pathname component
> + * @name:	qstr storing pathname component to lookup
>   * @base:	base directory to lookup from
> - * @len:	maximum length @len should be interpreted to
>   *
>   * Look up a dentry by name in the dcache, returning NULL if it does not
>   * currently exist.  The function does not try to create a dentry.
>   *
>   * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * not be called by generic code.  It does no permission checking.
>   *
>   * The caller must hold base->i_mutex.
>   */
> -struct dentry *try_lookup_one_len(const char *name, struct dentry *base, int len)
> +struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
>  {
>  	struct qstr this;
>  	int err;
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
> +	err = lookup_noperm_common(name->name, base, name->len, &this);
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	return lookup_dcache(&this, base, 0);
> +	name->hash = this.hash;
> +	return lookup_dcache(name, base, 0);
>  }
> -EXPORT_SYMBOL(try_lookup_one_len);
> +EXPORT_SYMBOL(try_lookup_noperm);
>  
>  /**
> - * lookup_one_len - filesystem helper to lookup single pathname component
> - * @name:	pathname component to lookup
> + * lookup_noperm - filesystem helper to lookup single pathname component
> + * @name:	qstr storing pathname component to lookup
>   * @base:	base directory to lookup from
> - * @len:	maximum length @len should be interpreted to
>   *
>   * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * not be called by generic code.  It does no permission checking.
>   *
>   * The caller must hold base->i_mutex.
>   */
> -struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> +struct dentry *lookup_noperm(struct qstr name, struct dentry *base)
>  {
>  	struct dentry *dentry;
>  	struct qstr this;
> @@ -2912,14 +2920,14 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
> +	err = lookup_noperm_common(name.name, base, name.len, &this);
>  	if (err)
>  		return ERR_PTR(err);
>  
>  	dentry = lookup_dcache(&this, base, 0);
>  	return dentry ? dentry : __lookup_slow(&this, base, 0);
>  }
> -EXPORT_SYMBOL(lookup_one_len);
> +EXPORT_SYMBOL(lookup_noperm);
>  
>  /**
>   * lookup_one - lookup single pathname component
> @@ -2957,7 +2965,7 @@ EXPORT_SYMBOL(lookup_one);
>   *
>   * This can be used for in-kernel filesystem clients such as file servers.
>   *
> - * Unlike lookup_one_len, it should be called without the parent
> + * Unlike lookup_one, it should be called without the parent
>   * i_rwsem held, and will take the i_rwsem itself if necessary.
>   */
>  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
> @@ -3010,39 +3018,49 @@ struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
>  EXPORT_SYMBOL(lookup_one_positive_unlocked);
>  
>  /**
> - * lookup_one_len_unlocked - filesystem helper to lookup single pathname component
> + * lookup_noperm_unlocked - filesystem helper to lookup single pathname component
>   * @name:	pathname component to lookup
>   * @base:	base directory to lookup from
>   * @len:	maximum length @len should be interpreted to
>   *
>   * Note that this routine is purely a helper for filesystem usage and should
> - * not be called by generic code.
> + * not be called by generic code. It does no permission checking.
>   *
> - * Unlike lookup_one_len, it should be called without the parent
> - * i_mutex held, and will take the i_mutex itself if necessary.
> + * Unlike lookup_noperm, it should be called without the parent
> + * i_rwsem held, and will take the i_rwsem itself if necessary.
>   */
> -struct dentry *lookup_one_len_unlocked(const char *name,
> -				       struct dentry *base, int len)
> +struct dentry *lookup_noperm_unlocked(struct qstr name,
> +					struct dentry *base)
>  {
> -	return lookup_one_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len), base);
> +	struct dentry *ret;
> +
> +	ret = try_lookup_noperm(&name, base);
> +	if (!ret)
> +		ret = lookup_slow(&name, base, 0);
> +	return ret;
>  }
> -EXPORT_SYMBOL(lookup_one_len_unlocked);
> +EXPORT_SYMBOL(lookup_noperm_unlocked);
>  
>  /*
> - * Like lookup_one_len_unlocked(), except that it yields ERR_PTR(-ENOENT)
> + * Like lookup_noperm_unlocked(), except that it yields ERR_PTR(-ENOENT)
>   * on negatives.  Returns known positive or ERR_PTR(); that's what
>   * most of the users want.  Note that pinned negative with unlocked parent
> - * _can_ become positive at any time, so callers of lookup_one_len_unlocked()
> + * _can_ become positive at any time, so callers of lookup_noperm_unlocked()
>   * need to be very careful; pinned positives have ->d_inode stable, so
>   * this one avoids such problems.
>   */
> -struct dentry *lookup_positive_unlocked(const char *name,
> -				       struct dentry *base, int len)
> +struct dentry *lookup_noperm_positive_unlocked(struct qstr name,
> +					       struct dentry *base)
>  {
> -	return lookup_one_positive_unlocked(&nop_mnt_idmap, QSTR_LEN(name, len),
> -					    base);
> +	struct dentry *ret = lookup_noperm_unlocked(name, base);
> +
> +	if (!IS_ERR(ret) && d_flags_negative(smp_load_acquire(&ret->d_flags))) {
> +		dput(ret);
> +		ret = ERR_PTR(-ENOENT);
> +	}
> +	return ret;
>  }
> -EXPORT_SYMBOL(lookup_positive_unlocked);
> +EXPORT_SYMBOL(lookup_noperm_positive_unlocked);
>  
>  #ifdef CONFIG_UNIX98_PTYS
>  int path_pts(struct path *path)
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index bf77399696a7..f52e57c31ae4 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -464,18 +464,17 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
>  
>  	sdentry = NULL;
>  	do {
> -		int slen;
>  		dput(sdentry);
>  		sillycounter++;
> -		slen = scnprintf(silly, sizeof(silly),
> -				SILLYNAME_PREFIX "%0*llx%0*x",
> -				SILLYNAME_FILEID_LEN, fileid,
> -				SILLYNAME_COUNTER_LEN, sillycounter);
> +		scnprintf(silly, sizeof(silly),
> +			  SILLYNAME_PREFIX "%0*llx%0*x",
> +			  SILLYNAME_FILEID_LEN, fileid,
> +			  SILLYNAME_COUNTER_LEN, sillycounter);
>  
>  		dfprintk(VFS, "NFS: trying to rename %pd to %s\n",
>  				dentry, silly);
>  
> -		sdentry = lookup_one_len(silly, dentry->d_parent, slen);
> +		sdentry = lookup_noperm(QSTR(silly), dentry->d_parent);
>  		/*
>  		 * N.B. Better to return EBUSY here ... it could be
>  		 * dangerous to delete the file while it's in use.
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 444aeeccb6da..a51025e8b2d0 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -385,11 +385,9 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
>  	 */
>  	take_dentry_name_snapshot(&name, real);
>  	/*
> -	 * No idmap handling here: it's an internal lookup.  Could skip
> -	 * permission checking altogether, but for now just use non-idmap
> -	 * transformed ids.
> +	 * No idmap handling here: it's an internal lookup.
>  	 */
> -	this = lookup_one_len(name.name.name, connected, name.name.len);
> +	this = lookup_noperm(name.name, connected);
>  	release_dentry_name_snapshot(&name);
>  	err = PTR_ERR(this);
>  	if (IS_ERR(this)) {
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 6a6301e4bba5..adb4af8d08db 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -757,7 +757,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh)
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	index = lookup_positive_unlocked(name.name, ofs->workdir, name.len);
> +	index = lookup_noperm_positive_unlocked(name, ofs->workdir);
>  	kfree(name.name);
>  	if (IS_ERR(index)) {
>  		if (PTR_ERR(index) == -ENOENT)
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 825c5c2e0962..ea426467f26b 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2560,7 +2560,7 @@ int dquot_quota_on_mount(struct super_block *sb, char *qf_name,
>  	struct dentry *dentry;
>  	int error;
>  
> -	dentry = lookup_positive_unlocked(qf_name, sb->s_root, strlen(qf_name));
> +	dentry = lookup_noperm_positive_unlocked(QSTR(qf_name), sb->s_root);
>  	if (IS_ERR(dentry))
>  		return PTR_ERR(dentry);
>  
> diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> index fe738623cf1b..854de94fc394 100644
> --- a/fs/smb/client/cached_dir.c
> +++ b/fs/smb/client/cached_dir.c
> @@ -109,7 +109,8 @@ path_to_dentry(struct cifs_sb_info *cifs_sb, const char *path)
>  		while (*s && *s != sep)
>  			s++;
>  
> -		child = lookup_positive_unlocked(p, dentry, s - p);
> +		child = lookup_noperm_positive_unlocked(QSTR_LEN(p, s-p),
> +							dentry);
>  		dput(dentry);
>  		dentry = child;
>  	} while (!IS_ERR(dentry));
> @@ -207,7 +208,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>  	spin_unlock(&cfids->cfid_list_lock);
>  
>  	/*
> -	 * Skip any prefix paths in @path as lookup_positive_unlocked() ends up
> +	 * Skip any prefix paths in @path as lookup_noperm_positive_unlocked() ends up
>  	 * calling ->lookup() which already adds those through
>  	 * build_path_from_dentry().  Also, do it earlier as we might reconnect
>  	 * below when trying to send compounded request and then potentially
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 6a3bd652d251..71eadc0305b5 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -925,7 +925,8 @@ cifs_get_root(struct smb3_fs_context *ctx, struct super_block *sb)
>  		while (*s && *s != sep)
>  			s++;
>  
> -		child = lookup_positive_unlocked(p, dentry, s - p);
> +		child = lookup_noperm_positive_unlocked(QSTR_LEN(p, s - p),
> +							dentry);
>  		dput(dentry);
>  		dentry = child;
>  	} while (!IS_ERR(dentry));
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index cb1af30b49f5..aa7982e9b579 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -555,7 +555,7 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
>  	if (unlikely(IS_DEADDIR(d_inode(parent))))
>  		dentry = ERR_PTR(-ENOENT);
>  	else
> -		dentry = lookup_one_len(name, parent, strlen(name));
> +		dentry = lookup_noperm(QSTR(name), parent);
>  	if (!IS_ERR(dentry) && d_inode(dentry)) {
>  		dput(dentry);
>  		dentry = ERR_PTR(-EEXIST);
> diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
> index 3537f3cca6d5..987af5b2bb82 100644
> --- a/fs/xfs/scrub/orphanage.c
> +++ b/fs/xfs/scrub/orphanage.c
> @@ -153,8 +153,7 @@ xrep_orphanage_create(
>  
>  	/* Try to find the orphanage directory. */
>  	inode_lock_nested(root_inode, I_MUTEX_PARENT);
> -	orphanage_dentry = lookup_one_len(ORPHANAGE, root_dentry,
> -			strlen(ORPHANAGE));
> +	orphanage_dentry = lookup_noperm(QSTR(ORPHANAGE), root_dentry);
>  	if (IS_ERR(orphanage_dentry)) {
>  		error = PTR_ERR(orphanage_dentry);
>  		goto out_unlock_root;
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 508dae67e3c5..befe75439d7e 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -69,10 +69,10 @@ int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
>  int vfs_path_lookup(struct dentry *, struct vfsmount *, const char *,
>  		    unsigned int, struct path *);
>  
> -extern struct dentry *try_lookup_one_len(const char *, struct dentry *, int);
> -extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
> -extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
> -extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
> +extern struct dentry *try_lookup_noperm(struct qstr *, struct dentry *);
> +extern struct dentry *lookup_noperm(struct qstr, struct dentry *);
> +extern struct dentry *lookup_noperm_unlocked(struct qstr, struct dentry *);
> +extern struct dentry *lookup_noperm_positive_unlocked(struct qstr, struct dentry *);
>  struct dentry *lookup_one(struct mnt_idmap *, struct qstr, struct dentry *);
>  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
>  				   struct qstr name, struct dentry *base);
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 35b4f8659904..f3d76c4b6013 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -913,7 +913,7 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode,
>  
>  	ro = mnt_want_write(mnt);	/* we'll drop it in any case */
>  	inode_lock(d_inode(root));
> -	path.dentry = lookup_one_len(name->name, root, strlen(name->name));
> +	path.dentry = lookup_noperm(QSTR(name->name), root);
>  	if (IS_ERR(path.dentry)) {
>  		error = PTR_ERR(path.dentry);
>  		goto out_putfd;
> @@ -969,8 +969,7 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
>  	if (err)
>  		goto out_name;
>  	inode_lock_nested(d_inode(mnt->mnt_root), I_MUTEX_PARENT);
> -	dentry = lookup_one_len(name->name, mnt->mnt_root,
> -				strlen(name->name));
> +	dentry = lookup_noperm(QSTR(name->name), mnt->mnt_root);
>  	if (IS_ERR(dentry)) {
>  		err = PTR_ERR(dentry);
>  		goto out_unlock;
> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index dc3aa91a6ba0..77fda4101b06 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c
> @@ -421,7 +421,7 @@ static int bpf_iter_link_pin_kernel(struct dentry *parent,
>  	int ret;
>  
>  	inode_lock(parent->d_inode);
> -	dentry = lookup_one_len(name, parent, strlen(name));
> +	dentry = lookup_noperm(QSTR(name), parent);
>  	if (IS_ERR(dentry)) {
>  		inode_unlock(parent->d_inode);
>  		return PTR_ERR(dentry);
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 6039afae4bfc..dfc56cec5ab5 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -283,7 +283,7 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
>  	dir = d_inode(parent);
>  
>  	inode_lock(dir);
> -	dentry = lookup_one_len(name, parent, strlen(name));
> +	dentry = lookup_noperm(QSTR(name), parent);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
>  		goto fail_lock;
> @@ -2551,7 +2551,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  		return error;
>  
>  	inode_lock(d_inode(parent));
> -	dentry = lookup_one_len(NULL_FILE_NAME, parent, strlen(NULL_FILE_NAME));
> +	dentry = lookup_noperm(QSTR(NULL_FILE_NAME), parent);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
>  		goto out;
> diff --git a/security/inode.c b/security/inode.c
> index da3ab44c8e57..d04a52a52bdd 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -128,7 +128,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
>  	dir = d_inode(parent);
>  
>  	inode_lock(dir);
> -	dentry = lookup_one_len(name, parent, strlen(name));
> +	dentry = lookup_noperm(QSTR(name), parent);
>  	if (IS_ERR(dentry))
>  		goto out;
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 5/6] Use try_lookup_noperm() instead of d_hash_and_lookup() outside of VFS

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
> 
> try_lookup_noperm() and d_hash_and_lookup() are nearly identical.  The
> former does some validation of the name where the latter doesn't.
> Outside of the VFS that validation is likely valuable, and having only
> one exported function for this task is certainly a good idea.
> 
> So make d_hash_and_lookup() local to VFS files and change all other
> callers to try_lookup_noperm().  Note that the arguments are swapped.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  Documentation/filesystems/porting.rst | 11 +++++++++++
>  fs/dcache.c                           |  1 -
>  fs/efivarfs/super.c                   | 14 ++++----------
>  fs/internal.h                         |  1 +
>  fs/proc/base.c                        |  2 +-
>  fs/smb/client/readdir.c               |  3 ++-
>  fs/xfs/scrub/orphanage.c              |  4 ++--
>  include/linux/dcache.h                |  1 -
>  net/sunrpc/rpc_pipe.c                 | 12 ++++++------
>  security/selinux/selinuxfs.c          |  4 ++--
>  10 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index df9516cd82e0..626f094787e8 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1225,3 +1225,14 @@ checked that the caller has 'X' permission on the parent.  They must
>  ONLY be used internally by a filesystem on itself when it knows that
>  permissions are irrelevant or in a context where permission checks have
>  already been performed such as after vfs_path_parent_lookup()
> +
> +---
> +
> +** mandatory**
> +
> +d_hash_and_lookup() is no longer exported or available outside the VFS.
> +Use try_lookup_noperm() instead.  This adds name validation and takes
> +arguments in the opposite order but is otherwise identical.
> +
> +Using try_lookup_noperm() will require linux/namei.h to be included.
> +
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 726a5be2747b..17f8e0b7f04f 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2395,7 +2395,6 @@ struct dentry *d_hash_and_lookup(struct dentry *dir, struct qstr *name)
>  	}
>  	return d_lookup(dir, name);
>  }
> -EXPORT_SYMBOL(d_hash_and_lookup);
>  
>  /*
>   * When a file is deleted, we have two options:
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 09fcf731e65d..867cd6e0fbad 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -204,7 +204,6 @@ bool efivarfs_variable_is_present(efi_char16_t *variable_name,
>  	char *name = efivar_get_utf8name(variable_name, vendor);
>  	struct super_block *sb = data;
>  	struct dentry *dentry;
> -	struct qstr qstr;
>  
>  	if (!name)
>  		/*
> @@ -217,9 +216,7 @@ bool efivarfs_variable_is_present(efi_char16_t *variable_name,
>  		 */
>  		return true;
>  
> -	qstr.name = name;
> -	qstr.len = strlen(name);
> -	dentry = d_hash_and_lookup(sb->s_root, &qstr);
> +	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
>  	kfree(name);
>  	if (!IS_ERR_OR_NULL(dentry))
>  		dput(dentry);
> @@ -402,8 +399,8 @@ static bool efivarfs_actor(struct dir_context *ctx, const char *name, int len,
>  {
>  	unsigned long size;
>  	struct efivarfs_ctx *ectx = container_of(ctx, struct efivarfs_ctx, ctx);
> -	struct qstr qstr = { .name = name, .len = len };
> -	struct dentry *dentry = d_hash_and_lookup(ectx->sb->s_root, &qstr);
> +	struct dentry *dentry = try_lookup_noperm(QSTR_LEN(name, len),
> +						  ectx->sb->s_root);
>  	struct inode *inode;
>  	struct efivar_entry *entry;
>  	int err;
> @@ -439,7 +436,6 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
>  	char *name;
>  	struct super_block *sb = data;
>  	struct dentry *dentry;
> -	struct qstr qstr;
>  	int err;
>  
>  	if (guid_equal(&vendor, &LINUX_EFI_RANDOM_SEED_TABLE_GUID))
> @@ -449,9 +445,7 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
>  	if (!name)
>  		return -ENOMEM;
>  
> -	qstr.name = name;
> -	qstr.len = strlen(name);
> -	dentry = d_hash_and_lookup(sb->s_root, &qstr);
> +	dentry = try_lookup_noperm(&QSTR(name), sb->s_root);
>  	if (IS_ERR(dentry)) {
>  		err = PTR_ERR(dentry);
>  		goto out;
> diff --git a/fs/internal.h b/fs/internal.h
> index e7f02ae1e098..c21534a23196 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -66,6 +66,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
>  int vfs_tmpfile(struct mnt_idmap *idmap,
>  		const struct path *parentpath,
>  		struct file *file, umode_t mode);
> +struct dentry *d_hash_and_lookup(struct dentry *, struct qstr *);
>  
>  /*
>   * namespace.c
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index cd89e956c322..7d36c7567c31 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2124,7 +2124,7 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
>  	unsigned type = DT_UNKNOWN;
>  	ino_t ino = 1;
>  
> -	child = d_hash_and_lookup(dir, &qname);
> +	child = try_lookup_noperm(&qname, dir);
>  	if (!child) {
>  		DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
>  		child = d_alloc_parallel(dir, &qname, &wq);
> diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
> index 50f96259d9ad..7329ec532bcf 100644
> --- a/fs/smb/client/readdir.c
> +++ b/fs/smb/client/readdir.c
> @@ -9,6 +9,7 @@
>   *
>   */
>  #include <linux/fs.h>
> +#include <linux/namei.h>
>  #include <linux/pagemap.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> @@ -78,7 +79,7 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
>  
>  	cifs_dbg(FYI, "%s: for %s\n", __func__, name->name);
>  
> -	dentry = d_hash_and_lookup(parent, name);
> +	dentry = try_lookup_noperm(name, parent);
>  	if (!dentry) {
>  		/*
>  		 * If we know that the inode will need to be revalidated
> diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
> index 987af5b2bb82..f42ffad5a7b9 100644
> --- a/fs/xfs/scrub/orphanage.c
> +++ b/fs/xfs/scrub/orphanage.c
> @@ -444,7 +444,7 @@ xrep_adoption_check_dcache(
>  	if (!d_orphanage)
>  		return 0;
>  
> -	d_child = d_hash_and_lookup(d_orphanage, &qname);
> +	d_child = try_lookup_noperm(&qname, d_orphanage);
>  	if (d_child) {
>  		trace_xrep_adoption_check_child(sc->mp, d_child);
>  
> @@ -481,7 +481,7 @@ xrep_adoption_zap_dcache(
>  	if (!d_orphanage)
>  		return;
>  
> -	d_child = d_hash_and_lookup(d_orphanage, &qname);
> +	d_child = try_lookup_noperm(&qname, d_orphanage);
>  	while (d_child != NULL) {
>  		trace_xrep_adoption_invalidate_child(sc->mp, d_child);
>  
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 1f01f4e734c5..cf37ae54955d 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -288,7 +288,6 @@ extern void d_exchange(struct dentry *, struct dentry *);
>  extern struct dentry *d_ancestor(struct dentry *, struct dentry *);
>  
>  extern struct dentry *d_lookup(const struct dentry *, const struct qstr *);
> -extern struct dentry *d_hash_and_lookup(struct dentry *, struct qstr *);
>  
>  static inline unsigned d_count(const struct dentry *dentry)
>  {
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index eadc00410ebc..98f78cd55905 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -631,7 +631,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
>  					  const char *name)
>  {
>  	struct qstr q = QSTR(name);
> -	struct dentry *dentry = d_hash_and_lookup(parent, &q);
> +	struct dentry *dentry = try_lookup_noperm(&q, parent);
>  	if (!dentry) {
>  		dentry = d_alloc(parent, &q);
>  		if (!dentry)
> @@ -658,7 +658,7 @@ static void __rpc_depopulate(struct dentry *parent,
>  	for (i = start; i < eof; i++) {
>  		name.name = files[i].name;
>  		name.len = strlen(files[i].name);
> -		dentry = d_hash_and_lookup(parent, &name);
> +		dentry = try_lookup_noperm(&name, parent);
>  
>  		if (dentry == NULL)
>  			continue;
> @@ -1190,7 +1190,7 @@ static const struct rpc_filelist files[] = {
>  struct dentry *rpc_d_lookup_sb(const struct super_block *sb,
>  			       const unsigned char *dir_name)
>  {
> -	return d_hash_and_lookup(sb->s_root, &QSTR(dir_name));
> +	return try_lookup_noperm(&QSTR(dir_name), sb->s_root);
>  }
>  EXPORT_SYMBOL_GPL(rpc_d_lookup_sb);
>  
> @@ -1301,7 +1301,7 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
>  	struct dentry *pipe_dentry = NULL;
>  
>  	/* We should never get this far if "gssd" doesn't exist */
> -	gssd_dentry = d_hash_and_lookup(root, &QSTR(files[RPCAUTH_gssd].name));
> +	gssd_dentry = try_lookup_noperm(&QSTR(files[RPCAUTH_gssd].name), root);
>  	if (!gssd_dentry)
>  		return ERR_PTR(-ENOENT);
>  
> @@ -1311,8 +1311,8 @@ rpc_gssd_dummy_populate(struct dentry *root, struct rpc_pipe *pipe_data)
>  		goto out;
>  	}
>  
> -	clnt_dentry = d_hash_and_lookup(gssd_dentry,
> -					&QSTR(gssd_dummy_clnt_dir[0].name));
> +	clnt_dentry = try_lookup_noperm(&QSTR(gssd_dummy_clnt_dir[0].name),
> +					  gssd_dentry);
>  	if (!clnt_dentry) {
>  		__rpc_depopulate(gssd_dentry, gssd_dummy_clnt_dir, 0, 1);
>  		pipe_dentry = ERR_PTR(-ENOENT);
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 47480eb2189b..e67a8ce4b64c 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -2158,8 +2158,8 @@ static int __init init_sel_fs(void)
>  		return err;
>  	}
>  
> -	selinux_null.dentry = d_hash_and_lookup(selinux_null.mnt->mnt_root,
> -						&null_name);
> +	selinux_null.dentry = try_lookup_noperm(&null_name,
> +						  selinux_null.mnt->mnt_root);
>  	if (IS_ERR(selinux_null.dentry)) {
>  		pr_err("selinuxfs:  could not lookup null!\n");
>  		err = PTR_ERR(selinux_null.dentry);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 6/6] VFS: change lookup_one_common and lookup_noperm_common to take a qstr

[Jeff Layton]

On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> From: NeilBrown <neilb@suse.de>
> 
> These function already take a qstr of course, but they also currently
> take a name/len was well and fill in the qstr.
> Now they take a qstr that is already filled in, which is what all the
> callers have.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/namei.c | 44 +++++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 16605f7108c0..e2fb61573f13 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2833,13 +2833,12 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
>  }
>  EXPORT_SYMBOL(vfs_path_lookup);
>  
> -static int lookup_noperm_common(const char *name, struct dentry *base,
> -				  int len,
> -				  struct qstr *this)
> +static int lookup_noperm_common(struct qstr *qname, struct dentry *base)
>  {
> -	this->name = name;
> -	this->len = len;
> -	this->hash = full_name_hash(base, name, len);
> +	const char *name = qname->name;
> +	u32 len = qname->len;
> +
> +	qname->hash = full_name_hash(base, name, len);
>  	if (!len)
>  		return -EACCES;
>  
> @@ -2856,7 +2855,7 @@ static int lookup_noperm_common(const char *name, struct dentry *base,
>  	 * to use its own hash..
>  	 */
>  	if (base->d_flags & DCACHE_OP_HASH) {
> -		int err = base->d_op->d_hash(base, this);
> +		int err = base->d_op->d_hash(base, qname);
>  		if (err < 0)
>  			return err;
>  	}
> @@ -2864,10 +2863,10 @@ static int lookup_noperm_common(const char *name, struct dentry *base,
>  }
>  
>  static int lookup_one_common(struct mnt_idmap *idmap,
> -			     const char *name, struct dentry *base, int len,
> -			     struct qstr *this) {
> +			     struct qstr *qname, struct dentry *base)
> +{
>  	int err;
> -	err = lookup_noperm_common(name, base, len, this);
> +	err = lookup_noperm_common(qname, base);
>  	if (err < 0)
>  		return err;
>  	return inode_permission(idmap, base->d_inode, MAY_EXEC);
> @@ -2888,16 +2887,14 @@ static int lookup_one_common(struct mnt_idmap *idmap,
>   */
>  struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
>  {
> -	struct qstr this;
>  	int err;
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_noperm_common(name->name, base, name->len, &this);
> +	err = lookup_noperm_common(name, base);
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	name->hash = this.hash;
>  	return lookup_dcache(name, base, 0);
>  }
>  EXPORT_SYMBOL(try_lookup_noperm);
> @@ -2915,17 +2912,16 @@ EXPORT_SYMBOL(try_lookup_noperm);
>  struct dentry *lookup_noperm(struct qstr name, struct dentry *base)
>  {
>  	struct dentry *dentry;
> -	struct qstr this;
>  	int err;
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_noperm_common(name.name, base, name.len, &this);
> +	err = lookup_noperm_common(&name, base);
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	dentry = lookup_dcache(&this, base, 0);
> -	return dentry ? dentry : __lookup_slow(&this, base, 0);
> +	dentry = lookup_dcache(&name, base, 0);
> +	return dentry ? dentry : __lookup_slow(&name, base, 0);
>  }
>  EXPORT_SYMBOL(lookup_noperm);
>  
> @@ -2943,17 +2939,16 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
>  			  struct dentry *base)
>  {
>  	struct dentry *dentry;
> -	struct qstr this;
>  	int err;
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(idmap, name.name, base, name.len, &this);
> +	err = lookup_one_common(idmap, &name, base);
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	dentry = lookup_dcache(&this, base, 0);
> -	return dentry ? dentry : __lookup_slow(&this, base, 0);
> +	dentry = lookup_dcache(&name, base, 0);
> +	return dentry ? dentry : __lookup_slow(&name, base, 0);
>  }
>  EXPORT_SYMBOL(lookup_one);
>  
> @@ -2971,17 +2966,16 @@ EXPORT_SYMBOL(lookup_one);
>  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
>  				   struct qstr name, struct dentry *base)
>  {
> -	struct qstr this;
>  	int err;
>  	struct dentry *ret;
>  
> -	err = lookup_one_common(idmap, name.name, base, name.len, &this);
> +	err = lookup_one_common(idmap, &name, base);
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	ret = lookup_dcache(&this, base, 0);
> +	ret = lookup_dcache(&name, base, 0);
>  	if (!ret)
> -		ret = lookup_slow(&this, base, 0);
> +		ret = lookup_slow(&name, base, 0);
>  	return ret;
>  }
>  EXPORT_SYMBOL(lookup_one_unlocked);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()

[Christian Brauner]

On Thu, Mar 20, 2025 at 06:22:08AM -0400, Jeff Layton wrote:
> On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > cachefiles uses some VFS interfaces (such as vfs_mkdir) which take an
> > explicit mnt_idmap, and it passes &nop_mnt_idmap as cachefiles doesn't
> > yet support idmapped mounts.
> > 
> > It also uses the lookup_one_len() family of functions which implicitly
> > use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> > confusing.  When we eventually update cachefiles to support idmap mounts it
> 
> Is that something we ever plan to do?

It should be pretty easy to do. I just didn't see a reason to do it yet.

Fwiw, the cache paths that cachefiles uses aren't private mounts like overlayfs
does it, i.e., cachefiles doesn't do clone_private_mount() before stashing
cache->mnt. That means properties of the mount can simply change beneath
cachefilesd. And afaict cache->mnt isn't even opened for writing so the mount
could suddenly go read-only behind cachefilesd's back. It probably will ignore
that since it doesn't use mnt_want_write() apart for xattrs.

> 
> > would be best if all places which need an idmap determined from the
> > mount point were similar and easily found.
> > 
> > So this patch changes cachefiles to use lookup_one(), lookup_one_unlocked(),
> > and lookup_one_positive_unlocked(), passing &nop_mnt_idmap.
> > 
> > This has the benefit of removing the remaining user of the
> > lookup_one_len functions where permission checking is actually needed.
> > Other callers don't care about permission checking and using these
> > function only where permission checking is needed is a valuable
> > simplification.
> > 
> > This requires passing the name in a qstr.  This is easily done with
> > QSTR() as the name is always nul terminated, and often strlen is used
> > anyway.  ->d_name_len is removed as no longer useful.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/cachefiles/internal.h |  1 -
> >  fs/cachefiles/key.c      |  1 -
> >  fs/cachefiles/namei.c    | 14 +++++++-------
> >  3 files changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> > index 38c236e38cef..b62cd3e9a18e 100644
> > --- a/fs/cachefiles/internal.h
> > +++ b/fs/cachefiles/internal.h
> > @@ -71,7 +71,6 @@ struct cachefiles_object {
> >  	int				debug_id;
> >  	spinlock_t			lock;
> >  	refcount_t			ref;
> > -	u8				d_name_len;	/* Length of filename */
> >  	enum cachefiles_content		content_info:8;	/* Info about content presence */
> >  	unsigned long			flags;
> >  #define CACHEFILES_OBJECT_USING_TMPFILE	0		/* Have an unlinked tmpfile */
> > diff --git a/fs/cachefiles/key.c b/fs/cachefiles/key.c
> > index bf935e25bdbe..4927b533b9ae 100644
> > --- a/fs/cachefiles/key.c
> > +++ b/fs/cachefiles/key.c
> > @@ -132,7 +132,6 @@ bool cachefiles_cook_key(struct cachefiles_object *object)
> >  success:
> >  	name[len] = 0;
> >  	object->d_name = name;
> > -	object->d_name_len = len;
> >  	_leave(" = %s", object->d_name);
> >  	return true;
> >  }
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index 83a60126de0f..4fc6f3efd3d9 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -98,7 +98,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
> >  retry:
> >  	ret = cachefiles_inject_read_error();
> >  	if (ret == 0)
> > -		subdir = lookup_one_len(dirname, dir, strlen(dirname));
> > +		subdir = lookup_one(&nop_mnt_idmap, QSTR(dirname), dir);
> >  	else
> >  		subdir = ERR_PTR(ret);
> >  	trace_cachefiles_lookup(NULL, dir, subdir);
> > @@ -337,7 +337,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
> >  		return -EIO;
> >  	}
> >  
> > -	grave = lookup_one_len(nbuffer, cache->graveyard, strlen(nbuffer));
> > +	grave = lookup_one(&nop_mnt_idmap, QSTR(nbuffer), cache->graveyard);
> >  	if (IS_ERR(grave)) {
> >  		unlock_rename(cache->graveyard, dir);
> >  		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> > @@ -629,8 +629,8 @@ bool cachefiles_look_up_object(struct cachefiles_object *object)
> >  	/* Look up path "cache/vol/fanout/file". */
> >  	ret = cachefiles_inject_read_error();
> >  	if (ret == 0)
> > -		dentry = lookup_positive_unlocked(object->d_name, fan,
> > -						  object->d_name_len);
> > +		dentry = lookup_one_positive_unlocked(&nop_mnt_idmap,
> > +						      QSTR(object->d_name), fan);
> >  	else
> >  		dentry = ERR_PTR(ret);
> >  	trace_cachefiles_lookup(object, fan, dentry);
> > @@ -682,7 +682,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
> >  	inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);
> >  	ret = cachefiles_inject_read_error();
> >  	if (ret == 0)
> > -		dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
> > +		dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
> >  	else
> >  		dentry = ERR_PTR(ret);
> >  	if (IS_ERR(dentry)) {
> > @@ -701,7 +701,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
> >  		dput(dentry);
> >  		ret = cachefiles_inject_read_error();
> >  		if (ret == 0)
> > -			dentry = lookup_one_len(object->d_name, fan, object->d_name_len);
> > +			dentry = lookup_one(&nop_mnt_idmap, QSTR(object->d_name), fan);
> >  		else
> >  			dentry = ERR_PTR(ret);
> >  		if (IS_ERR(dentry)) {
> > @@ -750,7 +750,7 @@ static struct dentry *cachefiles_lookup_for_cull(struct cachefiles_cache *cache,
> >  
> >  	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
> >  
> > -	victim = lookup_one_len(filename, dir, strlen(filename));
> > +	victim = lookup_one(&nop_mnt_idmap, QSTR(filename), dir);
> >  	if (IS_ERR(victim))
> >  		goto lookup_error;
> >  	if (d_is_negative(victim))
> 
> Patch looks sane though.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()

[David Howells]

Christian Brauner <brauner@kernel.org> wrote:

> > > It also uses the lookup_one_len() family of functions which implicitly
> > > use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> > > confusing.  When we eventually update cachefiles to support idmap mounts
> > > it
> > 
> > Is that something we ever plan to do?
> 
> It should be pretty easy to do. I just didn't see a reason to do it yet.
> 
> Fwiw, the cache paths that cachefiles uses aren't private mounts like
> overlayfs does it, i.e., cachefiles doesn't do clone_private_mount() before
> stashing cache->mnt. ...

This is probably something cachefilesd needs to do in userspace before telling
the kernel through /dev/cachefiles where to find the cache.

David

Re: [PATCH 3/6] cachefiles: Use lookup_one() rather than lookup_one_len()

[Christian Brauner]

On Thu, Mar 20, 2025 at 01:49:25PM +0000, David Howells wrote:
> Christian Brauner <brauner@kernel.org> wrote:
> 
> > > > It also uses the lookup_one_len() family of functions which implicitly
> > > > use &nop_mnt_idmap.  This mixture of implicit and explicit could be
> > > > confusing.  When we eventually update cachefiles to support idmap mounts
> > > > it
> > > 
> > > Is that something we ever plan to do?
> > 
> > It should be pretty easy to do. I just didn't see a reason to do it yet.
> > 
> > Fwiw, the cache paths that cachefiles uses aren't private mounts like
> > overlayfs does it, i.e., cachefiles doesn't do clone_private_mount() before
> > stashing cache->mnt. ...
> 
> This is probably something cachefilesd needs to do in userspace before telling
> the kernel through /dev/cachefiles where to find the cache.

Afaict, I don't think it matters whether the mount is actually attached
to a mount namespace so cachefilesd could just do:

fd_tree = open_tree(AT_FDCWD, "/path/to/cache", AT_EMPTY_PATH | OPEN_TREE_CLONE);

and use the detached mount for all cachefilesd interactions.

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[David Howells]

NeilBrown <neil@brown.name> wrote:

> Also the path component name is passed as "name" and "len" which are
> (confusingly?) separate by the "base".  In some cases the len in simply
> "strlen" and so passing a qstr using QSTR() would make the calling
> clearer.
> Other callers do pass separate name and len which are stored in a
> struct.  Sometimes these are already stored in a qstr, other times it
> easily could be.
> 
> So this patch changes these three functions to receive a 'struct qstr',
> and improves the documentation.

You did want 'struct qstr' not 'struct qstr *' right?  I think there are
arches where this will cause the compiler to skip a register argument or two
if it's the second argument or third argument - i386 for example.  Plus you
have an 8-byte alignment requirement because of the u64 in it that may suck if
passed through several layers of function.

David

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Al Viro]

On Wed, Mar 19, 2025 at 02:01:32PM +1100, NeilBrown wrote:

> -struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
> -			  struct dentry *base, int len)
> +struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
> +			  struct dentry *base)

>  {
>  	struct dentry *dentry;
>  	struct qstr this;
> @@ -2942,7 +2940,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(idmap, name, base, len, &this);
> +	err = lookup_one_common(idmap, name.name, base, name.len, &this);

No.  Just look at what lookup_one_common() is doing as the first step.

        this->name = name;
	this->len = len;

You copy your argument's fields to corresponding fields of *&this.  It might make
sense to pass a qstr, but not like that - just pass a _pointer_ to struct qstr instead.

Have lookup_one_common() do this:

static int lookup_one_common(struct mnt_idmap *idmap,
                             struct qstr *this, struct dentry *base)
{
	const unsigned char *name = this->name;
	int len = this->len;
        if (!len)
                return -EACCES;

        this->hash = full_name_hash(base, name, len);
        if (is_dot_dotdot(name, len))
                return -EACCES;

        while (len--) {
                unsigned int c = *name++;
                if (c == '/' || c == '\0')
                        return -EACCES;
        }
        /*
         * See if the low-level filesystem might want
         * to use its own hash..
         */
        if (base->d_flags & DCACHE_OP_HASH) {
                int err = base->d_op->d_hash(base, this);
                if (err < 0)
                        return err;
        }

        return inode_permission(idmap, base->d_inode, MAY_EXEC);
}

and adjust the callers; e.g.
struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr *this,
			  struct dentry *base)
{
        struct dentry *dentry;
        int err;

        WARN_ON_ONCE(!inode_is_locked(base->d_inode));

        err = lookup_one_common(idmap, this, base);
        if (err)
                return ERR_PTR(err);

        dentry = lookup_dcache(this, base, 0);
        return dentry ? dentry : __lookup_slow(this, base, 0);
}

with callers passing idmap, &QSTR_LEN(name, len), base instead of
idmap, name, base, len.  lookup_one_common() looks at the fields
separately; its callers do not.

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Al Viro]

On Thu, Mar 20, 2025 at 02:04:49PM +0000, David Howells wrote:
> NeilBrown <neil@brown.name> wrote:
> 
> > Also the path component name is passed as "name" and "len" which are
> > (confusingly?) separate by the "base".  In some cases the len in simply
> > "strlen" and so passing a qstr using QSTR() would make the calling
> > clearer.
> > Other callers do pass separate name and len which are stored in a
> > struct.  Sometimes these are already stored in a qstr, other times it
> > easily could be.
> > 
> > So this patch changes these three functions to receive a 'struct qstr',
> > and improves the documentation.
> 
> You did want 'struct qstr' not 'struct qstr *' right?  I think there are
> arches where this will cause the compiler to skip a register argument or two
> if it's the second argument or third argument - i386 for example.  Plus you
> have an 8-byte alignment requirement because of the u64 in it that may suck if
> passed through several layers of function.

Not just that - you end up with *two* struct qstr instances for no good reason,
copying from the one passed by value field-by-field into the other one, then
passing the *address* of the copy to the functions that do actual work.

Re: [PATCH 4/6] VFS: rename lookup_one_len family to lookup_noperm and remove permission check

[Al Viro]

On Wed, Mar 19, 2025 at 02:01:35PM +1100, NeilBrown wrote:

Quite a few of those should be switched to a common helper,
lifted out of debugfs/tracefs start_creating()...

I can live with that, but it'll cause fuckloads of noise in
persistency queue... ;-/

Re: [PATCH 5/6] Use try_lookup_noperm() instead of d_hash_and_lookup() outside of VFS

[Al Viro]

On Wed, Mar 19, 2025 at 02:01:36PM +1100, NeilBrown wrote:

	efivarfs stuff conflicts with jejb's fixes.  And rpc_pipe
part adds fuckload of conflicts for me; what's more, the only caller to
survive is rpc_d_lookup_sb(); the rest will be gone anyway...

Oh, well...

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[NeilBrown]

On Sat, 22 Mar 2025, Al Viro wrote:
> On Wed, Mar 19, 2025 at 02:01:32PM +1100, NeilBrown wrote:
> 
> > -struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
> > -			  struct dentry *base, int len)
> > +struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
> > +			  struct dentry *base)
> 
> >  {
> >  	struct dentry *dentry;
> >  	struct qstr this;
> > @@ -2942,7 +2940,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
> >  
> >  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
> >  
> > -	err = lookup_one_common(idmap, name, base, len, &this);
> > +	err = lookup_one_common(idmap, name.name, base, name.len, &this);
> 
> No.  Just look at what lookup_one_common() is doing as the first step.
> 
>         this->name = name;
> 	this->len = len;

This code is cleaned up in a later patch. lookup_one_common receives the
address of just one qstr which is initialised with qstr that is passed
in.
So on x86_64, the original qstr is passed in as 2 registers.  These are
stored in the stack and the address is passed to lookup_noperm_common(),
as lookup_one_common() gets inlined.

We have to put the two values onto the stack at some point, either in
the original callers, or in the lookup_one family of functions.  I think
it is cleaner in lookup_one as we don't need to put a & in front of all
the QSTR calls.  But we can change it to pass the pointer if you really
think that is better.

Thanks,
NeilBrown


> 
> You copy your argument's fields to corresponding fields of *&this.  It might make
> sense to pass a qstr, but not like that - just pass a _pointer_ to struct qstr instead.
> 
> Have lookup_one_common() do this:
> 
> static int lookup_one_common(struct mnt_idmap *idmap,
>                              struct qstr *this, struct dentry *base)
> {
> 	const unsigned char *name = this->name;
> 	int len = this->len;
>         if (!len)
>                 return -EACCES;
> 
>         this->hash = full_name_hash(base, name, len);
>         if (is_dot_dotdot(name, len))
>                 return -EACCES;
> 
>         while (len--) {
>                 unsigned int c = *name++;
>                 if (c == '/' || c == '\0')
>                         return -EACCES;
>         }
>         /*
>          * See if the low-level filesystem might want
>          * to use its own hash..
>          */
>         if (base->d_flags & DCACHE_OP_HASH) {
>                 int err = base->d_op->d_hash(base, this);
>                 if (err < 0)
>                         return err;
>         }
> 
>         return inode_permission(idmap, base->d_inode, MAY_EXEC);
> }
> 
> and adjust the callers; e.g.
> struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr *this,
> 			  struct dentry *base)
> {
>         struct dentry *dentry;
>         int err;
> 
>         WARN_ON_ONCE(!inode_is_locked(base->d_inode));
> 
>         err = lookup_one_common(idmap, this, base);
>         if (err)
>                 return ERR_PTR(err);
> 
>         dentry = lookup_dcache(this, base, 0);
>         return dentry ? dentry : __lookup_slow(this, base, 0);
> }
> 
> with callers passing idmap, &QSTR_LEN(name, len), base instead of
> idmap, name, base, len.  lookup_one_common() looks at the fields
> separately; its callers do not.
> 

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[NeilBrown]

On Fri, 21 Mar 2025, David Howells wrote:
> NeilBrown <neil@brown.name> wrote:
> 
> > Also the path component name is passed as "name" and "len" which are
> > (confusingly?) separate by the "base".  In some cases the len in simply
> > "strlen" and so passing a qstr using QSTR() would make the calling
> > clearer.
> > Other callers do pass separate name and len which are stored in a
> > struct.  Sometimes these are already stored in a qstr, other times it
> > easily could be.
> > 
> > So this patch changes these three functions to receive a 'struct qstr',
> > and improves the documentation.
> 
> You did want 'struct qstr' not 'struct qstr *' right?  I think there are
> arches where this will cause the compiler to skip a register argument or two
> if it's the second argument or third argument - i386 for example.  Plus you
> have an 8-byte alignment requirement because of the u64 in it that may suck if
> passed through several layers of function.

I don't think it is passed through several layers - except where the
intermediate are inlined.
And gcc enforces 16 byte alignment of the stack on function calls for
i386, so I don't think alignment will be an issue.

I thought 'struct qstr' would result in slightly cleaner calling.  But I
cannot make a strong argument in favour of it so I'm willing to change
if there are concerns.

Thanks,
NeilBrown

Re: [PATCH 4/6] VFS: rename lookup_one_len family to lookup_noperm and remove permission check

[NeilBrown]

On Sat, 22 Mar 2025, Al Viro wrote:
> On Wed, Mar 19, 2025 at 02:01:35PM +1100, NeilBrown wrote:
> 
> Quite a few of those should be switched to a common helper,
> lifted out of debugfs/tracefs start_creating()...

The next batch of patches will add "lookup_and_lock" family of functions
and change various calls to use them, including the "start_creating"
functions. 

The goal there is to get all the code for lock/unlock around
create/remove/rename to be in namei.c so we can change it in one place.

Thanks,
NeilBrown


> 
> I can live with that, but it'll cause fuckloads of noise in
> persistency queue... ;-/
> 

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Christian Brauner]

On Fri, Mar 28, 2025 at 12:18:16PM +1100, NeilBrown wrote:
> On Fri, 21 Mar 2025, David Howells wrote:
> > NeilBrown <neil@brown.name> wrote:
> > 
> > > Also the path component name is passed as "name" and "len" which are
> > > (confusingly?) separate by the "base".  In some cases the len in simply
> > > "strlen" and so passing a qstr using QSTR() would make the calling
> > > clearer.
> > > Other callers do pass separate name and len which are stored in a
> > > struct.  Sometimes these are already stored in a qstr, other times it
> > > easily could be.
> > > 
> > > So this patch changes these three functions to receive a 'struct qstr',
> > > and improves the documentation.
> > 
> > You did want 'struct qstr' not 'struct qstr *' right?  I think there are
> > arches where this will cause the compiler to skip a register argument or two
> > if it's the second argument or third argument - i386 for example.  Plus you
> > have an 8-byte alignment requirement because of the u64 in it that may suck if
> > passed through several layers of function.
> 
> I don't think it is passed through several layers - except where the
> intermediate are inlined.
> And gcc enforces 16 byte alignment of the stack on function calls for
> i386, so I don't think alignment will be an issue.
> 
> I thought 'struct qstr' would result in slightly cleaner calling.  But I
> cannot make a strong argument in favour of it so I'm willing to change
> if there are concerns.

Fwiw, I massaged the whole series to pass struct qstr * instead of
struct qstr. I just forgot to finish that rebase and push.
/me doing so now.

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Christian Brauner]

On Fri, Apr 04, 2025 at 03:41:01PM +0200, Christian Brauner wrote:
> On Fri, Mar 28, 2025 at 12:18:16PM +1100, NeilBrown wrote:
> > On Fri, 21 Mar 2025, David Howells wrote:
> > > NeilBrown <neil@brown.name> wrote:
> > > 
> > > > Also the path component name is passed as "name" and "len" which are
> > > > (confusingly?) separate by the "base".  In some cases the len in simply
> > > > "strlen" and so passing a qstr using QSTR() would make the calling
> > > > clearer.
> > > > Other callers do pass separate name and len which are stored in a
> > > > struct.  Sometimes these are already stored in a qstr, other times it
> > > > easily could be.
> > > > 
> > > > So this patch changes these three functions to receive a 'struct qstr',
> > > > and improves the documentation.
> > > 
> > > You did want 'struct qstr' not 'struct qstr *' right?  I think there are
> > > arches where this will cause the compiler to skip a register argument or two
> > > if it's the second argument or third argument - i386 for example.  Plus you
> > > have an 8-byte alignment requirement because of the u64 in it that may suck if
> > > passed through several layers of function.
> > 
> > I don't think it is passed through several layers - except where the
> > intermediate are inlined.
> > And gcc enforces 16 byte alignment of the stack on function calls for
> > i386, so I don't think alignment will be an issue.
> > 
> > I thought 'struct qstr' would result in slightly cleaner calling.  But I
> > cannot make a strong argument in favour of it so I'm willing to change
> > if there are concerns.
> 
> Fwiw, I massaged the whole series to pass struct qstr * instead of
> struct qstr. I just forgot to finish that rebase and push.
> /me doing so now.

Fwiw, there were a bunch of build failures for me when I built the
individual commits that I fixed up. I generally do:

git rebase -i HEAD~XXXXX -x "make -j512"

with an allmodconfig to make sure that it cleanly builds at each commit.

Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[NeilBrown]

On Sat, 05 Apr 2025, Christian Brauner wrote:
> On Fri, Apr 04, 2025 at 03:41:01PM +0200, Christian Brauner wrote:
> > On Fri, Mar 28, 2025 at 12:18:16PM +1100, NeilBrown wrote:
> > > On Fri, 21 Mar 2025, David Howells wrote:
> > > > NeilBrown <neil@brown.name> wrote:
> > > > 
> > > > > Also the path component name is passed as "name" and "len" which are
> > > > > (confusingly?) separate by the "base".  In some cases the len in simply
> > > > > "strlen" and so passing a qstr using QSTR() would make the calling
> > > > > clearer.
> > > > > Other callers do pass separate name and len which are stored in a
> > > > > struct.  Sometimes these are already stored in a qstr, other times it
> > > > > easily could be.
> > > > > 
> > > > > So this patch changes these three functions to receive a 'struct qstr',
> > > > > and improves the documentation.
> > > > 
> > > > You did want 'struct qstr' not 'struct qstr *' right?  I think there are
> > > > arches where this will cause the compiler to skip a register argument or two
> > > > if it's the second argument or third argument - i386 for example.  Plus you
> > > > have an 8-byte alignment requirement because of the u64 in it that may suck if
> > > > passed through several layers of function.
> > > 
> > > I don't think it is passed through several layers - except where the
> > > intermediate are inlined.
> > > And gcc enforces 16 byte alignment of the stack on function calls for
> > > i386, so I don't think alignment will be an issue.
> > > 
> > > I thought 'struct qstr' would result in slightly cleaner calling.  But I
> > > cannot make a strong argument in favour of it so I'm willing to change
> > > if there are concerns.
> > 
> > Fwiw, I massaged the whole series to pass struct qstr * instead of
> > struct qstr. I just forgot to finish that rebase and push.
> > /me doing so now.
> 
> Fwiw, there were a bunch of build failures for me when I built the
> individual commits that I fixed up. I generally do:
> 
> git rebase -i HEAD~XXXXX -x "make -j512"

Thanks for the fix-up! I'll have a look and compare with what I have.
I generally do something similar to the above - though my hardware
wouldn't handle -j512.

> 
> with an allmodconfig to make sure that it cleanly builds at each commit.
> 
Part of the code I'm changing is in arch/s390.  I'd need to get a
cross-compiler to build that.... maybe I should.

Thanks again.  I have a few more small cleanup (probably post on Monday)
and then a large batch which changes all the code which locks
directories for create/remove/rename.

One awkwardness which you might have an opinion on:
 In the final state we will be locking just the dentry, not the
 directory.  So the unlock only needs to be given the dentry.
 Something like "dentry_unlock(de);".

 Today we can implement that as "inode_unlock(de->d_parent->d_inode)"
 because d_parent is stable until the unlock happens.  After the switch
 it will involves clearing a bit and a possible wakeup.

 However:  vfs_mkdir() consumes the dentry on failure so there won't be
 any dentry to unlock - unless the caller keeps a copy, which would be
 clumsy.
 In the case where vfs_mkdir() returns a different dentry it will have
 to transfer the lock to that dentry (I don't see a problem there) so it
 will need to know about locking.  So I'm planing to have vfs_mkdir()
 drop the lock as well as put the dentry on failure.

 This ends up being quite an intrusive change.  Several other functions
 (in nfsd and particularly overlayfs) need to be changed to also drop
 the lock on failure.

 Do you think this is an acceptable API or should I explore other
 approaches?

 I've included my current draft patch so you can see the extent of the
 changes.  overlayfs is particularly interesting - it sometimes holds a
 rename lock across vfs_mkdir(), so we need to unlock the "other"
 directory after failure.  I think it will get cleaner later but I don't
 have code to prove it yet.

Thanks,
NeilBrown




Author: NeilBrown <neil@brown.name>
Date:   Fri Apr 4 16:43:25 2025 +1100

    Change vfs_mkdir() to unlock on failure.
    
    Proposed changes to directory-op locking will lock the dentry rather
    than the whole directory.  So the dentry will need to be unlocked.
    
    vfs_mkdir() consumes the dentry on error, so there will be no dentry to
    be unlocked.
    
    So change vfs_mkdir() to unlock on error.  This requires various other
    functions in various callers to also unlock on error.
    
    At present this results in some clumsy code.  Once the transition to
    dentry locking is complete the clumsiness will be gone.
    
    overlays looks particularly clumsy as in some cases a double-directory
    rename lock is taken, and a mkdir is then performed in one of the
    directories.  If that fails the other directory must be unlocked.
    
    Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 14d0cc894000..1a5905c313f1 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -131,8 +131,11 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
 		ret = cachefiles_inject_write_error();
 		if (ret == 0)
 			subdir = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), subdir, 0700);
-		else
+		else {
+			/* vfs_mkdir() unlocks on failure */
+			inode_unlock(d_inode(dir));
 			subdir = ERR_PTR(ret);
+		}
 		if (IS_ERR(subdir)) {
 			trace_cachefiles_vfs_error(NULL, d_inode(dir), ret,
 						   cachefiles_trace_mkdir_error);
@@ -196,9 +199,10 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
 	return ERR_PTR(-EBUSY);
 
 mkdir_error:
-	inode_unlock(d_inode(dir));
-	if (!IS_ERR(subdir))
+	if (!IS_ERR(subdir)) {
+		inode_unlock(d_inode(dir));
 		dput(subdir);
+	}
 	pr_err("mkdir %s failed with error %d\n", dirname, ret);
 	return ERR_PTR(ret);
 
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 51a5c54eb740..31e8abfff490 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -518,7 +518,7 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 				 lower_dentry, mode);
 	rc = PTR_ERR(lower_dentry);
 	if (IS_ERR(lower_dentry))
-		goto out;
+		goto out_unlocked;
 	rc = 0;
 	if (d_unhashed(lower_dentry))
 		goto out;
@@ -530,6 +530,7 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	set_nlink(dir, lower_dir->i_nlink);
 out:
 	inode_unlock(lower_dir);
+out_unlocked:
 	if (d_really_is_negative(dentry))
 		d_drop(dentry);
 	return ERR_PTR(rc);
diff --git a/fs/namei.c b/fs/namei.c
index 360a86ca1f02..ba36883dd70a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4130,9 +4130,10 @@ EXPORT_SYMBOL(kern_path_create);
 
 void done_path_create(struct path *path, struct dentry *dentry)
 {
-	if (!IS_ERR(dentry))
+	if (!IS_ERR(dentry)) {
 		dput(dentry);
-	inode_unlock(path->dentry->d_inode);
+		inode_unlock(path->dentry->d_inode);
+	}
 	mnt_drop_write(path->mnt);
 	path_put(path);
 }
@@ -4295,7 +4296,8 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d
  * negative or unhashes it and possibly splices a different one returning it,
  * the original dentry is dput() and the alternate is returned.
  *
- * In case of an error the dentry is dput() and an ERR_PTR() is returned.
+ * In case of an error the dentry is dput(), the parent is unlocked, and
+ * an ERR_PTR() is returned.
  */
 struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 			 struct dentry *dentry, umode_t mode)
@@ -4333,6 +4335,8 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	return dentry;
 
 err:
+	/* Caller only needs to unlock if dentry is not an error */
+	inode_unlock(dir);
 	dput(dentry);
 	return ERR_PTR(error);
 }
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index c1d9bd07285f..8907967135c6 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -221,7 +221,8 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
 	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
-		goto out_unlock;
+		inode_unlock(d_inode(dir));
+		goto out;
 	}
 	if (d_really_is_positive(dentry))
 		/*
@@ -234,13 +235,14 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 		 */
 		goto out_put;
 	dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, S_IRWXU);
-	if (IS_ERR(dentry))
+	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
+		goto out;
+	}
 out_put:
-	if (!status)
-		dput(dentry);
-out_unlock:
+	dput(dentry);
 	inode_unlock(d_inode(dir));
+out:
 	if (status == 0) {
 		if (nn->in_grace)
 			__nfsd4_create_reclaim_record_grace(clp, dname,
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 9abdc4b75813..0e935be8c2c1 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1450,7 +1450,9 @@ nfsd_check_ignore_resizing(struct iattr *iap)
 		iap->ia_valid &= ~ATTR_SIZE;
 }
 
-/* The parent directory should already be locked: */
+/* The parent directory should already be locked.  The lock
+ * will be dropped on error.
+ */
 __be32
 nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		   struct nfsd_attrs *attrs,
@@ -1516,8 +1518,11 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	err = nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
 
 out:
-	if (!IS_ERR(dchild))
+	if (!IS_ERR(dchild)) {
+		if (err)
+			inode_unlock(dirp);
 		dput(dchild);
+	}
 	return err;
 
 out_nfserr:
@@ -1572,6 +1577,9 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (err != nfs_ok)
 		goto out_unlock;
 	err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
+	if (err)
+		/* lock will have been dropped */
+		return err;
 	fh_fill_post_attrs(fhp);
 out_unlock:
 	inode_unlock(dentry->d_inode);
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d7310fcf3888..324429d02569 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -518,7 +518,7 @@ static int ovl_set_upper_fh(struct ovl_fs *ofs, struct dentry *upper,
 /*
  * Create and install index entry.
  *
- * Caller must hold i_mutex on indexdir.
+ * Caller must hold i_mutex on indexdir.  It will be unlocked on error.
  */
 static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 			    struct dentry *upper)
@@ -539,16 +539,22 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	 * TODO: implement create index for non-dir, so we can call it when
 	 * encoding file handle for non-dir in case index does not exist.
 	 */
-	if (WARN_ON(!d_is_dir(dentry)))
+	if (WARN_ON(!d_is_dir(dentry))) {
+		inode_unlock(dir);
 		return -EIO;
+	}
 
 	/* Directory not expected to be indexed before copy up */
-	if (WARN_ON(ovl_test_flag(OVL_INDEX, d_inode(dentry))))
+	if (WARN_ON(ovl_test_flag(OVL_INDEX, d_inode(dentry)))) {
+		inode_unlock(dir);
 		return -EIO;
+	}
 
 	err = ovl_get_index_name_fh(fh, &name);
-	if (err)
+	if (err) {
+		inode_unlock(dir);
 		return err;
+	}
 
 	temp = ovl_create_temp(ofs, indexdir, OVL_CATTR(S_IFDIR | 0));
 	err = PTR_ERR(temp);
@@ -567,8 +573,10 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 		dput(index);
 	}
 out:
-	if (err)
+	if (err) {
 		ovl_cleanup(ofs, dir, temp);
+		inode_unlock(dir);
+	}
 	dput(temp);
 free_name:
 	kfree(name.name);
@@ -781,7 +789,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_start_write(c->dentry);
 	inode_lock(wdir);
 	temp = ovl_create_temp(ofs, c->workdir, &cattr);
-	inode_unlock(wdir);
+	if (!IS_ERR(wdir))
+		inode_unlock(wdir);
 	ovl_end_write(c->dentry);
 	ovl_revert_cu_creds(&cc);
 
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index fe493f3ed6b6..b4d92b51b63f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -138,13 +138,16 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
 	goto out;
 }
 
+/* dir will be unlocked on failure */
 struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
 			       struct dentry *newdentry, struct ovl_cattr *attr)
 {
 	int err;
 
-	if (IS_ERR(newdentry))
+	if (IS_ERR(newdentry)) {
+		inode_unlock(dir);
 		return newdentry;
+	}
 
 	err = -ESTALE;
 	if (newdentry->d_inode)
@@ -189,13 +192,16 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
 	}
 out:
 	if (err) {
-		if (!IS_ERR(newdentry))
+		if (!IS_ERR(newdentry)) {
+			inode_unlock(dir);
 			dput(newdentry);
+		}
 		return ERR_PTR(err);
 	}
 	return newdentry;
 }
 
+/* Note workdir will be unlocked on failure */
 struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
 			       struct ovl_cattr *attr)
 {
@@ -309,7 +315,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 				    attr);
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
-		goto out_unlock;
+		goto out;
 
 	if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
 	    !ovl_allow_offline_changes(ofs)) {
@@ -323,6 +329,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		goto out_cleanup;
 out_unlock:
 	inode_unlock(udir);
+out:
 	return err;
 
 out_cleanup:
@@ -367,9 +374,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 
 	opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
 	err = PTR_ERR(opaquedir);
-	if (IS_ERR(opaquedir))
-		goto out_unlock;
-
+	if (IS_ERR(opaquedir)) {
+		/* workdir was unlocked, no upperdir */
+		inode_unlock(upperdir->d_inode);
+		mutex_unlock(&upperdir->d_sb->s_vfs_rename_mutex);
+		return ERR_PTR(err);
+	}
 	err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
 	if (err)
 		goto out_cleanup;
@@ -455,8 +465,13 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 
 	newdentry = ovl_create_temp(ofs, workdir, cattr);
 	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
-		goto out_dput;
+	if (IS_ERR(newdentry)) {
+		/* workdir was unlocked, not upperdir */
+		inode_unlock(upperdir->d_inode);
+		mutex_unlock(&upperdir->d_sb->s_vfs_rename_mutex);
+		dput(upper);
+		goto out;
+	}
 
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6f2f8f4cfbbc..f7f7c3d1ad63 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -248,6 +248,7 @@ static inline struct dentry *ovl_do_mkdir(struct ovl_fs *ofs,
 {
 	dentry = vfs_mkdir(ovl_upper_mnt_idmap(ofs), dir, dentry, mode);
 	pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, PTR_ERR_OR_ZERO(dentry));
+	/* Note: dir will have been unlocked on failure */
 	return dentry;
 }
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b63474d1b064..56055c70f200 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -366,14 +366,17 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto out_dput;
 	} else {
 		err = PTR_ERR(work);
+		inode_unlock(dir);
 		goto out_err;
 	}
 out_unlock:
-	inode_unlock(dir);
+	if (work && !IS_ERR(work))
+		inode_unlock(dir);
 	return work;
 
 out_dput:
 	dput(work);
+	inode_unlock(dir);
 out_err:
 	pr_warn("failed to create directory %s/%s (errno: %i); mounting read-only\n",
 		ofs->config.workdir, name, -err);
@@ -569,7 +572,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 	temp = ovl_create_temp(ofs, workdir, OVL_CATTR(S_IFREG | 0));
 	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
-		goto out_unlock;
+		return err;
 
 	dest = ovl_lookup_temp(ofs, workdir);
 	err = PTR_ERR(dest);
@@ -620,10 +623,13 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
 
 	inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
 	child = ovl_lookup_upper(ofs, name, parent, len);
-	if (!IS_ERR(child) && !child->d_inode)
+	if (!IS_ERR(child) && !child->d_inode) {
 		child = ovl_create_real(ofs, parent->d_inode, child,
 					OVL_CATTR(mode));
-	inode_unlock(parent->d_inode);
+		if (!IS_ERR(child))
+			inode_unlock(parent->d_inode);
+	} else
+		inode_unlock(parent->d_inode);
 	dput(parent);
 
 	return child;
diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
index 3537f3cca6d5..af59183374ae 100644
--- a/fs/xfs/scrub/orphanage.c
+++ b/fs/xfs/scrub/orphanage.c
@@ -171,7 +171,7 @@ xrep_orphanage_create(
 					     orphanage_dentry, 0750);
 		error = PTR_ERR(orphanage_dentry);
 		if (IS_ERR(orphanage_dentry))
-			goto out_unlock_root;
+			goto out_dput_root;
 	}
 
 	/* Not a directory? Bail out. */