On Thu, Apr 08, 2021 at 02:55:30PM +0200, Christian Brauner wrote:

On Thu, Apr 08, 2021 at 02:44:47PM +0300, Amir Goldstein wrote:

quoted

quoted

One thing your patch

commit ea31e84fda83c17b88851de399f76f5d9fc1abf4
Author: Amir Goldstein [off-list ref]
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 [off-list ref]

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

Only in the NFSEXP_NOSUBTREECHECK case.  Taking a quick look, I think
Amir's not proposing a check like that by default, so, fine.  (I assume
problems with e.g. subtreechecking and cross-directory renames are
understood....)

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.

I think those would both be reasonable things to do.

--b.

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

Christian