Re: open_by_handle_at() in userns

[Christian Brauner <christian.brauner@ubuntu.com>]

On Thu, Apr 08, 2021 at 02:44:47PM +0300, Amir Goldstein wrote:
> > One thing your patch
> >
> > commit ea31e84fda83c17b88851de399f76f5d9fc1abf4
> > Author: Amir Goldstein <amir73il@gmail.com>
> > Date:   Sat Mar 20 12:58:12 2021 +0200
> >
> >     fs: allow open by file handle inside userns
> >
> >     open_by_handle_at(2) requires CAP_DAC_READ_SEARCH in init userns,
> >     where most filesystems are mounted.
> >
> >     Relax the requirement to allow a user with CAP_DAC_READ_SEARCH
> >     inside userns to open by file handle in filesystems that were
> >     mounted inside that userns.
> >
> >     In addition, also allow open by handle in an idmapped mount, which is
> >     mapped to the userns while verifying that the returned open file path
> >     is under the root of the idmapped mount.
> >
> >     This is going to be needed for setting an fanotify mark on a filesystem
> >     and watching events inside userns.
> >
> >     Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Requires fs/exportfs/expfs.c to be made idmapped mounts aware.
> > open_by_handle_at() uses exportfs_decode_fh() which e.g. has the
> > following and other callchains:
> >
> > exportfs_decode_fh()
> > -> exportfs_decode_fh_raw()
> >    -> lookup_one_len()
> >       -> inode_permission(mnt_userns, ...)
> >
> > That's not a huge problem though I did all these changes for the
> > overlayfs support for idmapped mounts I have in a branch from an earlier
> > version of the idmapped mounts patchset. Basically lookup_one_len(),
> > lookup_one_len_unlocked(), and lookup_positive_unlocked() need to take
> > the mnt_userns into account. I can rebase my change and send it for
> > consideration next cycle. If you can live without the
> > open_by_handle_at() support for now in this patchset (Which I think you
> > said you could.) then it's not a blocker either. Sorry for the
> > inconvenience.
> >
> 
> Christian,
> 
> I think making exportfs_decode_fh() idmapped mount aware is not
> enough, because when a dentry alias is found in dcache, none of
> those lookup functions are called.
> 
> I think we will also need something like this:
> https://github.com/amir73il/linux/commits/fhandle_userns
> 
> I factored-out a helper from nfsd_apcceptable() which implements
> the "subtree_check" nfsd logic and uses it for open_by_handle_at().
> 
> I've also added a small patch to name_to_handle_at() with a UAPI
> change that could make these changes usable by userspace nfs
> server inside userns, but I have no demo nor tests for that and frankly,
> I have little incentive to try and promote this UAPI change without
> anybody asking for it...

Ah, at first I was confused about why this would matter but it matters
because nfsd already implements a check of that sort directly in nfsd
independent of idmapped mounts:
https://github.com/amir73il/linux/commit/4bef9ff1718935b7b42afbae71cfaab7770e8436

Afaict, an nfs server can't be mounted inside of userns right now. That
is something that folks from Netflix and from Kinvolk have been
interested in enabling. They also want the ability to use idmapped
mounts + nfs. Understandable that you don't want to drive this of
course. I'll sync with them about this.

Independent of that, I thought our last understanding was that you
wouldn't need to handle open_by_handle_at() for now.

Christian