Re: [PATCH v1 0/9] fuse: basic support for idmapped mounts

[Christian Brauner]

On Mon, Jan 22, 2024 at 03:13:12PM -0600, Seth Forshee wrote:
> On Sun, Jan 21, 2024 at 06:50:57PM +0100, Christian Brauner wrote:
> > > - We have a small offlist discussion with Christian about adding fs_type->allow_idmap
> > > hook. Christian pointed out that it would be nice to have a superblock flag instead like
> > > SB_I_NOIDMAP and we can set this flag during mount time if we see that the filesystem does not
> > > support idmappings. But, unfortunately, I didn't succeed here because the kernel will
> > > know if the filesystem supports idmapping or not after FUSE_INIT request, but FUSE_INIT request
> > > is being sent at the end of the mounting process, so the mount and superblock will exist and
> > > visible by the userspace in that time. It seems like setting SB_I_NOIDMAP flag, in this
> > > case, is too late as a user may do the trick by creating an idmapped mount while it wasn't
> > > restricted by SB_I_NOIDMAP. Alternatively, we can introduce a "positive" version SB_I_ALLOWIDMAP
> > 
> > I see.
> > 
> > > and a "weak" version of FS_ALLOW_IDMAP like FS_MAY_ALLOW_IDMAP. So if FS_MAY_ALLOW_IDMAP is set,
> > > then SB_I_ALLOWIDMAP has to be set on the superblock to allow the creation of an idmapped mount.
> > > But that's a matter of our discussion.
> > 
> > I dislike making adding a struct super_block method. Because it means that we
> > call into the filesystem from generic mount code and specifically with the
> > namespace semaphore held. If there's ever any network filesystem that e.g.,
> > calls to a hung server it will lockup the whole system. So I'm opposed to
> > calling into the filesystem here at all. It's also ugly because this is really
> > a vfs level change. The only involvement should be whether the filesystem type
> > can actually support this ideally.
> > 
> > I think we should handle this within FUSE. So we allow the creation of idmapped
> > mounts just based on FS_ALLOW_IDMAP. And if the server doesn't support the
> > FUSE_OWNER_UID_GID_EXT then we simply refuse all creation requests originating
> > from an idmapped mount. Either we return EOPNOSUPP or we return EOVERFLOW to
> > indicate that we can't represent the owner correctly because the server is
> > missing the required extension.
> 
> Could fuse just set SB_I_NOIDMAP initially then clear it if the init
> reply indicates idmap support? This is like the "weak" FS_ALLOW_IDMAP
> option without requiring another file_system_type flag.

Would probably works too, yes. I'm just trying to avoid any additional
flag usage. Ideally we'd just do it in FUSE but this way is possibly
also effective. Although it kinda leaks internal FUSE details as in
"SB_I_NOIDMAP" really means "SB_I_INITIALIZED" which I kind of dislike.