On Thu, Apr 8, 2021 at 5:15 PM J. Bruce Fields [off-list ref] wrote:

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

quoted

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....)

They are understood to me :) but I didn't want to get into it, because it is
complicated to explain and I wasn't sure if anyone cared...

I started working on open_by_handle_at() in userns for fanotify and fanotify
mostly reports directory fhandle, so no issues with cross-directory renames.
In any case, fanotify never reports "connectable" non-dir file handles.

Because my proposed change ALSO makes it possible to start talking about
userspace nfs server inside userns (in case anyone cares), I wanted to lay
out the path towards a userspace "subtree_check" like solution.

Another thing I am contemplating is, if and when idmapped mount support
is added to overlayfs, we can store an additional "connectable" file handle
in the overlayfs index (whose key is the non-connectable fhandle) and
fix ovl_acceptable() similar to nfsd_acceptable() and then we will be able
to mount an overlayfs inside userns with nfs_export support.

I've included a two liner patch on the fhandle_userns branch to allow
overlayfs inside userns with nfs_export support in the case that
underlying filesystem was mounted inside userns, but that is not such
an interesting use case IMO.

Thanks,
Amir.