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

[Jeff Layton <jlayton@kernel.org>]

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>