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@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?
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@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>
