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

[Christian Brauner <brauner@kernel.org>]

On Fri, Mar 14, 2025 at 11:34:06AM +1100, NeilBrown wrote:
> VFS has some functions with names containing "lookup_one_len" and others
> without the "_len".  This difference has nothing to do with "len".
> 
> The functions without "_len" take a "mnt_idmap" pointer.  This is found

When we added idmapped mounts there were all these *_len() helpers and I
orignally had just ported them to pass mnt_idmap. But we decided to not
do this. The argument might have been that most callers don't need to be
switched (I'm not actually sure if that's still true now that we have
quite a few filesystems that do support idmapped mounts.).

So then we added new helper and then we decided to use better naming
then that *_len() stuff. That's about it.

> 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 which is not correct if the vfsmount
> is actually idmaped.  For cachefiles it probably (?) doesn't matter as
> the accesses to the backing filesystem are always does with elevated privileged (?).

Cachefiles explicitly refuse being mounted on top of an idmapped mount
and they require that the mount is attached (check_mnt()) and an
attached mount can never be idmapped as it has already been exposed in
the filesystem hierarchy.

> 
> For nfsd it would matter if anyone exported an idmapped filesystem.  I
> wonder if anyone has tried...

nfsd doesn't support exporting idmapped mounts. See check_export() where
that's explicitly checked.

If there are ways to circumvent this I'd be good to know.

> 
> These patches change the "lookup_one" functions to take a vfsmount
> instead of a mnt_idmap because I think that makes the intention clearer.

Please don't!

These internal lookup helpers intentionally do not take a vfsmount.
First, because they can be called in places where access to a vfsmount
isn't possible and we don't want to pass vfsmounts down to filesystems
ever!

Second, the mnt_idmap pointer is - with few safe exceptions - is
retrieved once in the VFS and then passed down so that e.g., permission
checking and file creation are guaranteed to use the same mnt_idmap
pointer.

A caller may start out with a non-idmapped detached mount (e.g., via
fsmount() or OPEN_TREE_CLONE) (nop_mnt_idmap) and call
inode_permission(). Now someone idmaps that mount. Now further down the
callchain someone calls lookup_one() which now retrieves the idmapping
again and now it's an idmapped mount. Now permission checking is out of
sync. That's an unlikely scenario but it's possible so lookup_one() is
not supposed to retrieve the idmapping again. Please keep passing it
explicitly. I've also written that down in the Documenation somewhere.

> 
> It also renames the "_one" functions to be "_noperm" and removes the
> permission checking completely.  In all cases where they are (correctly)
> used permission checking is irrelevant.

Ok, that sounds fine. Though I haven't taken the time to check the
callers yet. I'll try to do that during the weekend.

> 
> 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.
> 
> The nfsd and cachefiles patches probably should be separate.  Maybe I
> should submit those to relevant maintainers first, and one afs,
> cachefiles, and nfsd changes have landed I can submit this series with
> appropriate modifications.
> 
> May main question for review is : have I understood mnt_idmap correctly?

I mean, you didn't ask semantic questions so much as syntactic, I think.
I hope I explained the reasoning sufficiently.

Thanks for the cleanups.