On Wed, 2025-03-19 at 14:01 +1100, NeilBrown wrote:
> From: NeilBrown <neilb@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
