To properly support vfs idmappings we need to provide
a fuse daemon with the correct owner uid/gid for
inode creation requests like mkdir, mknod, atomic_open,
symlink.

Right now, fuse daemons use req->in.h.uid/req->in.h.gid
to set inode owner. These fields contain fsuid/fsgid of the
syscall's caller. And that's perfectly fine, because inode
owner have to be set to these values. But, for idmapped mounts
it's not the case and caller fsuid/fsgid != inode owner, because
idmapped mounts do nothing with the caller fsuid/fsgid, but
affect inode owner uid/gid. It means that we can't apply vfsid
mapping to caller fsuid/fsgid, but instead we have to introduce
a new fields to store inode owner uid/gid which will be appropriately
transformed.

Christian and I have done the same to support idmapped mounts in
the cephfs recently [1].

[1] 5ccd8530 ("ceph: handle idmapped mounts in create_request_message()")

Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Seth Forshee <sforshee@kernel.org>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Bernd Schubert <bschubert@ddn.com>
Cc: <linux-fsdevel@vger.kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/fuse/dir.c             | 34 +++++++++++++++++++++++++++++++---
 fs/fuse/fuse_i.h          |  3 +++
 fs/fuse/inode.c           |  4 +++-
 include/uapi/linux/fuse.h | 19 +++++++++++++++++++
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 6f5f9ff95380..e78ad4742aef 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -568,7 +568,33 @@ static int get_create_supp_group(struct inode *dir, struct fuse_in_arg *ext)
 	return 0;
 }
 
-static int get_create_ext(struct fuse_args *args,
+static int get_owner_uid_gid(struct mnt_idmap *idmap, struct fuse_conn *fc, struct fuse_in_arg *ext)
+{
+	struct fuse_ext_header *xh;
+	struct fuse_owner_uid_gid *owner_creds;
+	u32 owner_creds_len = fuse_ext_size(sizeof(*owner_creds));
+	kuid_t owner_fsuid;
+	kgid_t owner_fsgid;
+
+	xh = extend_arg(ext, owner_creds_len);
+	if (!xh)
+		return -ENOMEM;
+
+	xh->size = owner_creds_len;
+	xh->type = FUSE_EXT_OWNER_UID_GID;
+
+	owner_creds = (struct fuse_owner_uid_gid *) &xh[1];
+
+	owner_fsuid = mapped_fsuid(idmap, fc->user_ns);
+	owner_fsgid = mapped_fsgid(idmap, fc->user_ns);
+	owner_creds->uid = from_kuid(fc->user_ns, owner_fsuid);
+	owner_creds->gid = from_kgid(fc->user_ns, owner_fsgid);
+
+	return 0;
+}
+
+static int get_create_ext(struct mnt_idmap *idmap,
+			  struct fuse_args *args,
 			  struct inode *dir, struct dentry *dentry,
 			  umode_t mode)
 {
@@ -580,6 +606,8 @@ static int get_create_ext(struct fuse_args *args,
 		err = get_security_context(dentry, mode, &ext);
 	if (!err && fc->create_supp_group)
 		err = get_create_supp_group(dir, &ext);
+	if (!err && fc->owner_uid_gid_ext)
+		err = get_owner_uid_gid(idmap, fc, &ext);
 
 	if (!err && ext.size) {
 		WARN_ON(args->in_numargs >= ARRAY_SIZE(args->in_args));
@@ -662,7 +690,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	args.out_args[1].size = sizeof(outopen);
 	args.out_args[1].value = &outopen;
 
-	err = get_create_ext(&args, dir, entry, mode);
+	err = get_create_ext(&nop_mnt_idmap, &args, dir, entry, mode);
 	if (err)
 		goto out_put_forget_req;
 
@@ -790,7 +818,7 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	args->out_args[0].value = &outarg;
 
 	if (args->opcode != FUSE_LINK) {
-		err = get_create_ext(args, dir, entry, mode);
+		err = get_create_ext(&nop_mnt_idmap, args, dir, entry, mode);
 		if (err)
 			goto out_put_forget_req;
 	}
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 1df83eebda92..15ec95dea276 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -806,6 +806,9 @@ struct fuse_conn {
 	/* Add supplementary group info when creating a new inode */
 	unsigned int create_supp_group:1;
 
+	/* Add owner_{u,g}id info when creating a new inode */
+	unsigned int owner_uid_gid_ext:1;
+
 	/* Does the filesystem support per inode DAX? */
 	unsigned int inode_dax:1;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index ab824a8908b7..08cd3714b32d 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1284,6 +1284,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->create_supp_group = 1;
 			if (flags & FUSE_DIRECT_IO_ALLOW_MMAP)
 				fc->direct_io_allow_mmap = 1;
+			if (flags & FUSE_OWNER_UID_GID_EXT)
+				fc->owner_uid_gid_ext = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1330,7 +1332,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
 		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
 		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
-		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP;
+		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP | FUSE_OWNER_UID_GID_EXT;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index e7418d15fe39..ebe82104b172 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -211,6 +211,10 @@
  *  7.39
  *  - add FUSE_DIRECT_IO_ALLOW_MMAP
  *  - add FUSE_STATX and related structures
+ *
+ *  7.40
+ *  - add FUSE_EXT_OWNER_UID_GID
+ *  - add FUSE_OWNER_UID_GID_EXT
  */
 
 #ifndef _LINUX_FUSE_H
@@ -410,6 +414,8 @@ struct fuse_file_lock {
  *			symlink and mknod (single group that matches parent)
  * FUSE_HAS_EXPIRE_ONLY: kernel supports expiry-only entry invalidation
  * FUSE_DIRECT_IO_ALLOW_MMAP: allow shared mmap in FOPEN_DIRECT_IO mode.
+ * FUSE_OWNER_UID_GID_EXT: add inode owner UID/GID info to create, mkdir,
+ *			   symlink and mknod
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -452,6 +458,7 @@ struct fuse_file_lock {
 
 /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */
 #define FUSE_DIRECT_IO_RELAX	FUSE_DIRECT_IO_ALLOW_MMAP
+#define FUSE_OWNER_UID_GID_EXT	(1ULL << 37)
 
 /**
  * CUSE INIT request/reply flags
@@ -561,11 +568,13 @@ struct fuse_file_lock {
  * extension type
  * FUSE_MAX_NR_SECCTX: maximum value of &fuse_secctx_header.nr_secctx
  * FUSE_EXT_GROUPS: &fuse_supp_groups extension
+ * FUSE_EXT_OWNER_UID_GID: &fuse_owner_uid_gid extension
  */
 enum fuse_ext_type {
 	/* Types 0..31 are reserved for fuse_secctx_header */
 	FUSE_MAX_NR_SECCTX	= 31,
 	FUSE_EXT_GROUPS		= 32,
+	FUSE_EXT_OWNER_UID_GID	= 33,
 };
 
 enum fuse_opcode {
@@ -1153,4 +1162,14 @@ struct fuse_supp_groups {
 	uint32_t	groups[];
 };
 
+/**
+ * struct fuse_owner_uid_gid - Inode owner UID/GID extension
+ * @uid: inode owner UID
+ * @gid: inode owner GID
+ */
+struct fuse_owner_uid_gid {
+	uint32_t	uid;
+	uint32_t	gid;
+};
+
 #endif /* _LINUX_FUSE_H */
-- 
2.34.1

On Mon, 8 Jan 2024 at 13:10, Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> To properly support vfs idmappings we need to provide
> a fuse daemon with the correct owner uid/gid for
> inode creation requests like mkdir, mknod, atomic_open,
> symlink.
>
> Right now, fuse daemons use req->in.h.uid/req->in.h.gid
> to set inode owner. These fields contain fsuid/fsgid of the
> syscall's caller. And that's perfectly fine, because inode
> owner have to be set to these values. But, for idmapped mounts
> it's not the case and caller fsuid/fsgid != inode owner, because
> idmapped mounts do nothing with the caller fsuid/fsgid, but
> affect inode owner uid/gid. It means that we can't apply vfsid
> mapping to caller fsuid/fsgid, but instead we have to introduce
> a new fields to store inode owner uid/gid which will be appropriately
> transformed.

Does fsuid/fsgid have any meaning to the server?

Shouldn't this just set in.h.uid/in.h.gid to the mapped ids?

Thanks,
Miklos

On Tue, Mar 5, 2024 at 3:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 8 Jan 2024 at 13:10, Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> > To properly support vfs idmappings we need to provide
> > a fuse daemon with the correct owner uid/gid for
> > inode creation requests like mkdir, mknod, atomic_open,
> > symlink.
> >
> > Right now, fuse daemons use req->in.h.uid/req->in.h.gid
> > to set inode owner. These fields contain fsuid/fsgid of the
> > syscall's caller. And that's perfectly fine, because inode
> > owner have to be set to these values. But, for idmapped mounts
> > it's not the case and caller fsuid/fsgid != inode owner, because
> > idmapped mounts do nothing with the caller fsuid/fsgid, but
> > affect inode owner uid/gid. It means that we can't apply vfsid
> > mapping to caller fsuid/fsgid, but instead we have to introduce
> > a new fields to store inode owner uid/gid which will be appropriately
> > transformed.

Hi Miklos,

>
> Does fsuid/fsgid have any meaning to the server?

As far as I know, some servers use fsuid/fsgid values in server-side
permission checks.

Sometimes, for example in Glusterfs, even when "default_permissions"
fuse mount option
is enabled these values are still used for permission checks. And yes,
this is a problem
for idmapped mount support. That's why we need to have some fuse
connection flag from
the server side which means "yes, I'm aware of idmapped mounts and I
really want it" (FUSE_ALLOW_IDMAP).

>
> Shouldn't this just set in.h.uid/in.h.gid to the mapped ids?
>

It is a very good and tricky question ;-)

We had big debates like a year ago when Christian and I were working on idmapped
mounts support for cephfs [1].

This was a first Christian's idea when he originally proposed a
patchset for cephfs [2]. The problem with this
approach is that we don't have an idmapping provided in all
inode_operations, we only have it where it is supposed to be.
To workaround that, Christian suggested applying a mapping only when
we have mnt_idmap, but if not just leave uid/gid as it is.
This, of course, leads to inconsistencies between different
inode_operations, for example ->lookup (idmapping is not applied) and
->symlink (idmapping is applied).
This inconsistency, really, is not a big deal usually, but... what if
a server does UID/GID-based permission checks? Then it is a problem,
obviously.

Xiubo Li (cephfs kernel driver maintainer) asked, why we don't just
want to pass mnt_idmap everywhere, including ->lookup.
Christian and I came up with arguments, why it's not the best idea to
pass idmapping everywhere. [3], [4], [5]

[1] https://lore.kernel.org/all/20230807132626.182101-1-aleksandr.mikhalitsyn@canonical.com/
[2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/
[3] https://lore.kernel.org/lkml/20230609-alufolie-gezaubert-f18ef17cda12@brauner/
[4] https://lore.kernel.org/lkml/20230614-westseite-urlaub-7a5afcf0577a@brauner/
[5] https://lore.kernel.org/lkml/CAEivzxfw1fHO2TFA4dx3u23ZKK6Q+EThfzuibrhA3RKM=ZOYLg@mail.gmail.com/

Kind regards,
Alex

> Thanks,
> Miklos

On Thu, 18 Jul 2024 at 21:12, Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:

> This was a first Christian's idea when he originally proposed a
> patchset for cephfs [2]. The problem with this
> approach is that we don't have an idmapping provided in all
> inode_operations, we only have it where it is supposed to be.
> To workaround that, Christian suggested applying a mapping only when
> we have mnt_idmap, but if not just leave uid/gid as it is.
> This, of course, leads to inconsistencies between different
> inode_operations, for example ->lookup (idmapping is not applied) and
> ->symlink (idmapping is applied).
> This inconsistency, really, is not a big deal usually, but... what if
> a server does UID/GID-based permission checks? Then it is a problem,
> obviously.

Is it even sensible to do UID/GID-based permission checks in the
server if idmapping is enabled?

If not, then we should just somehow disable that configuration (i.e.
by the server having to opt into idmapping), and then we can just use
the in_h.[ugi]d for creates, no?

Thanks,
Miklos

On Thu, Aug 29, 2024 at 10:24:42AM GMT, Miklos Szeredi wrote:
> On Thu, 18 Jul 2024 at 21:12, Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> 
> > This was a first Christian's idea when he originally proposed a
> > patchset for cephfs [2]. The problem with this
> > approach is that we don't have an idmapping provided in all
> > inode_operations, we only have it where it is supposed to be.
> > To workaround that, Christian suggested applying a mapping only when
> > we have mnt_idmap, but if not just leave uid/gid as it is.
> > This, of course, leads to inconsistencies between different
> > inode_operations, for example ->lookup (idmapping is not applied) and
> > ->symlink (idmapping is applied).
> > This inconsistency, really, is not a big deal usually, but... what if
> > a server does UID/GID-based permission checks? Then it is a problem,
> > obviously.
> 
> Is it even sensible to do UID/GID-based permission checks in the
> server if idmapping is enabled?

It really makes no sense.

> 
> If not, then we should just somehow disable that configuration (i.e.
> by the server having to opt into idmapping), and then we can just use
> the in_h.[ugi]d for creates, no?

Fwiw, that's what the patchset is doing. It's only supported if the
server sets "default_permissions".

On Thu, 29 Aug 2024 at 14:08, Christian Brauner <brauner@kernel.org> wrote:

> Fwiw, that's what the patchset is doing. It's only supported if the
> server sets "default_permissions".

My specific issue is with FUSE_EXT_OWNER_UID_GID, which I think is
unnecessary.  Just fill the header with the mapped uid/gid values,
which most servers already use for creating the file with the correct
st_uid/st_gid and not for checking permission.  When the mapped values
are unavailable, set the uid/gid in the header -1.  Should be better
than sending nonsense values to userspace, no?

Thanks,
Miklos

On Thu, Aug 29, 2024 at 2:30 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 29 Aug 2024 at 14:08, Christian Brauner <brauner@kernel.org> wrote:
>
> > Fwiw, that's what the patchset is doing. It's only supported if the
> > server sets "default_permissions".
>
> My specific issue is with FUSE_EXT_OWNER_UID_GID, which I think is
> unnecessary.  Just fill the header with the mapped uid/gid values,
> which most servers already use for creating the file with the correct
> st_uid/st_gid and not for checking permission.  When the mapped values
> are unavailable, set the uid/gid in the header -1.  Should be better
> than sending nonsense values to userspace, no?

Hi Miklos,

yeah, we have a discussion like that while discussing cephfs idmapped mounts.
And yes, it's a really good question and it's not obvious at all which
solution is the best.
( I believe that I have replied on that question already there:
https://lore.kernel.org/all/CAEivzxeva5ipjihSrMa4u=uk9sDm9DNg9cLoYg0O6=eU2jLNQQ@mail.gmail.com/
)

A main argument against mapping uid/gid values in fuse header is
consistency. We can map these
values in symlink/mknod/mkdir/create/tmpfile. But we don't have access
to idmapping information in
lookup, read, write, etc. What should we do for these inode operations
then? Send an unmapped uid/gid?
But then it is an inconsistency - in one inode ops we have mapped
values, in another ones - we have unmapped ones.

>When the mapped values
> are unavailable, set the uid/gid in the header -1.  Should be better
> than sending nonsense values to userspace, no?

So, your point is to set uid/gid to -1 for FUSE_{READ,WRITE,LOOKUP,RELEASE,...}?

Kind regards,
Alex

>
> Thanks,
> Miklos

On Thu, 29 Aug 2024 at 16:39, Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:

> So, your point is to set uid/gid to -1 for FUSE_{READ,WRITE,LOOKUP,RELEASE,...}?

Yes.  Not sure what will happen with those servers that check
permissions based on these values, but my guess is it's still better
than sending the unmapped value.

Thanks,
Miklos

On Thu, Aug 29, 2024 at 5:05 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 29 Aug 2024 at 16:39, Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
>
> > So, your point is to set uid/gid to -1 for FUSE_{READ,WRITE,LOOKUP,RELEASE,...}?
>
> Yes.  Not sure what will happen with those servers that check
> permissions based on these values, but my guess is it's still better
> than sending the unmapped value.

That's an interesting and a bit unexpected proposal. I have not
considered that option before,
because it feels like a bit of a radical change in the fuse protocol semantic,
while I was trying to be extremely careful and not make a revolution
in stable fuse protocol and code ;-)

We even have old commit:
c9582eb0ff7 ("fuse: Fail all requests with invalid uids or gids")
https://github.com/torvalds/linux/commit/c9582eb0ff7d2b560be60eafab29183882cdc82b

At the same time, if *you* propose that, I can't find a single word
against that. :)

You idea also solves problem with ->rename operation:
https://lore.kernel.org/all/20240815092429.103356-10-aleksandr.mikhalitsyn@canonical.com/

because if we just reuse existing fuse header fields and remap values
in them then we don't need
any extension for FUSE_RENAME2.

Let's think about it a bit more and if you confirm that we want to go
this way, then I'll rework my patches.

Kind regards,
Alex

>
> Thanks,
> Miklos

On Thu, 29 Aug 2024 at 19:41, Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:

> Let's think about it a bit more and if you confirm that we want to go
> this way, then I'll rework my patches.

And ACK from Christian would be good.

I don't see why this would be a radical change: idmapping changes the
meaning of fsuid/fsgid, so we now can't send those to the server in
the non-create case.  The only thing that changes is that an "invalid
ID" value is introduced into the protocol.  Probably makes sense to
explicitly define this valiue in <uapi/linux/fuse.h>.

Thanks,
Miklos

On Thu, Aug 29, 2024 at 08:58:55PM GMT, Miklos Szeredi wrote:
> On Thu, 29 Aug 2024 at 19:41, Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> 
> > Let's think about it a bit more and if you confirm that we want to go
> > this way, then I'll rework my patches.
> 
> And ACK from Christian would be good.

Yeah, that all sounds good to me. I think Alex just followed the
cephfs precedent.

On Fri, 30 Aug 2024 at 12:57, Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Aug 29, 2024 at 08:58:55PM GMT, Miklos Szeredi wrote:
> > On Thu, 29 Aug 2024 at 19:41, Aleksandr Mikhalitsyn
> > <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> > > Let's think about it a bit more and if you confirm that we want to go
> > > this way, then I'll rework my patches.
> >
> > And ACK from Christian would be good.
>
> Yeah, that all sounds good to me. I think Alex just followed the
> cephfs precedent.

Okay, so please drop this patchset from your tree, then I'll have a
chance to review when Alex resends it.

Thanks,
Miklos

On Fri, Aug 30, 2024 at 3:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 30 Aug 2024 at 12:57, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Aug 29, 2024 at 08:58:55PM GMT, Miklos Szeredi wrote:
> > > On Thu, 29 Aug 2024 at 19:41, Aleksandr Mikhalitsyn
> > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > >
> > > > Let's think about it a bit more and if you confirm that we want to go
> > > > this way, then I'll rework my patches.
> > >
> > > And ACK from Christian would be good.
> >
> > Yeah, that all sounds good to me. I think Alex just followed the
> > cephfs precedent.
>
> Okay, so please drop this patchset from your tree, then I'll have a
> chance to review when Alex resends it.

Dear friends,

v4 is ready https://lore.kernel.org/all/20240903151626.264609-1-aleksandr.mikhalitsyn@canonical.com
and tested with virtiofsd:

# ./check -g idmapped
FSTYP         -- virtiofs
PLATFORM      -- Linux/x86_64 ubuntu 6.11.0-rc6+ #21 SMP
PREEMPT_DYNAMIC Tue Sep  3 16:48:58 CEST 2024

generic/633 1s ...  1s
generic/644 0s ...  0s
generic/645 19s ...  18s
generic/656 1s ...  1s
generic/689 0s ...  0s
generic/696       [not run] this test requires a valid $SCRATCH_DEV
generic/697 0s ...  0s
generic/698       [not run] this test requires a valid $SCRATCH_DEV
generic/699       [not run] this test requires a valid $SCRATCH_DEV
Ran: generic/633 generic/644 generic/645 generic/656 generic/689
generic/696 generic/697 generic/698 generic/699
Not run: generic/696 generic/698 generic/699
Passed all 9 tests

Kind regards,
Alex

>
> Thanks,
> Miklos

On Thu, Aug 29, 2024 at 2:08 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Aug 29, 2024 at 10:24:42AM GMT, Miklos Szeredi wrote:
> > On Thu, 18 Jul 2024 at 21:12, Aleksandr Mikhalitsyn
> > <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> > > This was a first Christian's idea when he originally proposed a
> > > patchset for cephfs [2]. The problem with this
> > > approach is that we don't have an idmapping provided in all
> > > inode_operations, we only have it where it is supposed to be.
> > > To workaround that, Christian suggested applying a mapping only when
> > > we have mnt_idmap, but if not just leave uid/gid as it is.
> > > This, of course, leads to inconsistencies between different
> > > inode_operations, for example ->lookup (idmapping is not applied) and
> > > ->symlink (idmapping is applied).
> > > This inconsistency, really, is not a big deal usually, but... what if
> > > a server does UID/GID-based permission checks? Then it is a problem,
> > > obviously.
> >
> > Is it even sensible to do UID/GID-based permission checks in the
> > server if idmapping is enabled?

Dear friends,

>
> It really makes no sense.

+

>
> >
> > If not, then we should just somehow disable that configuration (i.e.
> > by the server having to opt into idmapping), and then we can just use
> > the in_h.[ugi]d for creates, no?
>
> Fwiw, that's what the patchset is doing. It's only supported if the
> server sets "default_permissions".

Yeah. Thanks, Christian!

That's what we have:

+++ b/fs/fuse/inode.c
@@ -1345,6 +1345,12 @@ static void process_init_reply(struct
fuse_mount *fm, struct fuse_args *args,
                 fm->sb->s_export_op = &fuse_export_fid_operations;
             if (flags & FUSE_OWNER_UID_GID_EXT)
                 fc->owner_uid_gid_ext = 1;
+            if (flags & FUSE_ALLOW_IDMAP) {
+                if (fc->owner_uid_gid_ext && fc->default_permissions)
+                    fm->sb->s_iflags &= ~SB_I_NOIDMAP;
+                else
+                    ok = false;
+            }
         } else {
             ra_pages = fc->max_read / PAGE_SIZE;

So idmapped mounts can be enabled ONLY if "default_permissions" mode
is set. At the same time,
some fuse servers (glusterfs), even when "default_permissions" is set,
still have some UID/GID-based checks.
So, they effectively duplicate permission checking logic in the
userspace. We can not do anything with that, but only
fix fuse servers to stop doing so. See also my PoC for glusterfs-fuse
idmapped mounts support:
https://github.com/mihalicyn/glusterfs/commit/ab3ec2c7cbe22618cba9cc94a52a492b1904d0b2

Kind regards,
Alex

On Thu, Aug 29, 2024 at 2:17 PM Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> On Thu, Aug 29, 2024 at 2:08 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Aug 29, 2024 at 10:24:42AM GMT, Miklos Szeredi wrote:
> > > On Thu, 18 Jul 2024 at 21:12, Aleksandr Mikhalitsyn
> > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > >
> > > > This was a first Christian's idea when he originally proposed a
> > > > patchset for cephfs [2]. The problem with this
> > > > approach is that we don't have an idmapping provided in all
> > > > inode_operations, we only have it where it is supposed to be.
> > > > To workaround that, Christian suggested applying a mapping only when
> > > > we have mnt_idmap, but if not just leave uid/gid as it is.
> > > > This, of course, leads to inconsistencies between different
> > > > inode_operations, for example ->lookup (idmapping is not applied) and
> > > > ->symlink (idmapping is applied).
> > > > This inconsistency, really, is not a big deal usually, but... what if
> > > > a server does UID/GID-based permission checks? Then it is a problem,
> > > > obviously.
> > >
> > > Is it even sensible to do UID/GID-based permission checks in the
> > > server if idmapping is enabled?
>
> Dear friends,
>
> >
> > It really makes no sense.
>
> +
>
> >
> > >
> > > If not, then we should just somehow disable that configuration (i.e.
> > > by the server having to opt into idmapping), and then we can just use
> > > the in_h.[ugi]d for creates, no?
> >
> > Fwiw, that's what the patchset is doing. It's only supported if the
> > server sets "default_permissions".
>
> Yeah. Thanks, Christian!
>
> That's what we have:
>
> +++ b/fs/fuse/inode.c
> @@ -1345,6 +1345,12 @@ static void process_init_reply(struct
> fuse_mount *fm, struct fuse_args *args,
>                  fm->sb->s_export_op = &fuse_export_fid_operations;
>              if (flags & FUSE_OWNER_UID_GID_EXT)
>                  fc->owner_uid_gid_ext = 1;
> +            if (flags & FUSE_ALLOW_IDMAP) {
> +                if (fc->owner_uid_gid_ext && fc->default_permissions)
> +                    fm->sb->s_iflags &= ~SB_I_NOIDMAP;
> +                else
> +                    ok = false;
> +            }
>          } else {
>              ra_pages = fc->max_read / PAGE_SIZE;
>
> So idmapped mounts can be enabled ONLY if "default_permissions" mode
> is set. At the same time,
> some fuse servers (glusterfs), even when "default_permissions" is set,
> still have some UID/GID-based checks.
> So, they effectively duplicate permission checking logic in the
> userspace. We can not do anything with that, but only
> fix fuse servers to stop doing so. See also my PoC for glusterfs-fuse
> idmapped mounts support:
> https://github.com/mihalicyn/glusterfs/commit/ab3ec2c7cbe22618cba9cc94a52a492b1904d0b2

and yes, latest patchset (v3) is here:
https://lore.kernel.org/all/20240815092429.103356-1-aleksandr.mikhalitsyn@canonical.com/#t

>
> Kind regards,
> Alex