[PATCH v2 00/13] ceph: support idmapped mounts

[PATCH v2 00/13] ceph: support idmapped mounts

[Alexander Mikhalitsyn]

Dear friends,

This patchset was originally developed by Christian Brauner but I'll continue
to push it forward. Christian allowed me to do that :)

This feature is already actively used/tested with LXD/LXC project.

v2 is just a rebased version of the original series with some small field naming change.

Git tree (based on https://github.com/ceph/ceph-client.git master):
https://github.com/mihalicyn/linux/tree/fs.idmapped.ceph.v2

Original description from Christian:
========================================================================
This patch series enables cephfs to support idmapped mounts, i.e. the
ability to alter ownership information on a per-mount basis.

Container managers such as LXD support sharaing data via cephfs between
the host and unprivileged containers and between unprivileged containers.
They may all use different idmappings. Idmapped mounts can be used to
create mounts with the idmapping used for the container (or a different
one specific to the use-case).

There are in fact more use-cases such as remapping ownership for
mountpoints on the host itself to grant or restrict access to different
users or to make it possible to enforce that programs running as root
will write with a non-zero {g,u}id to disk.

The patch series is simple overall and few changes are needed to cephfs.
There is one cephfs specific issue that I would like to discuss and
solve which I explain in detail in:

[PATCH 02/12] ceph: handle idmapped mounts in create_request_message()

It has to do with how to handle mds serves which have id-based access
restrictions configured. I would ask you to please take a look at the
explanation in the aforementioned patch.

The patch series passes the vfs and idmapped mount testsuite as part of
xfstests. To run it you will need a config like:

[ceph]
export FSTYP=ceph
export TEST_DIR=/mnt/test
export TEST_DEV=10.103.182.10:6789:/
export TEST_FS_MOUNT_OPTS="-o name=admin,secret=$password

and then simply call

sudo ./check -g idmapped

========================================================================

Alexander Mikhalitsyn (1):
  fs: export mnt_idmap_get/mnt_idmap_put

Christian Brauner (12):
  ceph: stash idmapping in mdsc request
  ceph: handle idmapped mounts in create_request_message()
  ceph: allow idmapped mknod inode op
  ceph: allow idmapped symlink inode op
  ceph: allow idmapped mkdir inode op
  ceph: allow idmapped rename inode op
  ceph: allow idmapped getattr inode op
  ceph: allow idmapped permission inode op
  ceph: allow idmapped setattr inode op
  ceph/acl: allow idmapped set_acl inode op
  ceph/file: allow idmapped atomic_open inode op
  ceph: allow idmapped mounts

 fs/ceph/acl.c                 |  2 +-
 fs/ceph/dir.c                 |  4 ++++
 fs/ceph/file.c                | 10 ++++++++--
 fs/ceph/inode.c               | 15 +++++++++++----
 fs/ceph/mds_client.c          | 29 +++++++++++++++++++++++++----
 fs/ceph/mds_client.h          |  1 +
 fs/ceph/super.c               |  2 +-
 fs/mnt_idmapping.c            |  2 ++
 include/linux/mnt_idmapping.h |  3 +++
 9 files changed, 56 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Alexander Mikhalitsyn]

These helpers are required to support idmapped mounts in the Cephfs.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/mnt_idmapping.c            | 2 ++
 include/linux/mnt_idmapping.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
index 4905665c47d0..5a579e809bcf 100644
--- a/fs/mnt_idmapping.c
+++ b/fs/mnt_idmapping.c
@@ -256,6 +256,7 @@ struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap)
 
 	return idmap;
 }
+EXPORT_SYMBOL(mnt_idmap_get);
 
 /**
  * mnt_idmap_put - put a reference to an idmapping
@@ -271,3 +272,4 @@ void mnt_idmap_put(struct mnt_idmap *idmap)
 		kfree(idmap);
 	}
 }
+EXPORT_SYMBOL(mnt_idmap_put);
diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
index 057c89867aa2..b8da2db4ecd2 100644
--- a/include/linux/mnt_idmapping.h
+++ b/include/linux/mnt_idmapping.h
@@ -115,6 +115,9 @@ static inline bool vfsgid_eq_kgid(vfsgid_t vfsgid, kgid_t kgid)
 
 int vfsgid_in_group_p(vfsgid_t vfsgid);
 
+struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
+void mnt_idmap_put(struct mnt_idmap *idmap);
+
 vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
 		     struct user_namespace *fs_userns, kuid_t kuid);
 
-- 
2.34.1

[PATCH v2 02/13] ceph: stash idmapping in mdsc request

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

When sending a mds request cephfs will send relevant data for the
requested operation. For creation requests the caller's fs{g,u}id is
used to set the ownership of the newly created filesystem object. For
setattr requests the caller can pass in arbitrary {g,u}id values to
which the relevant filesystem object is supposed to be changed.

If the caller is performing the relevant operation via an idmapped mount
cephfs simply needs to take the idmapping into account when it sends the
relevant mds request.

In order to support idmapped mounts for cephfs we stash the idmapping
whenever they are relevant for the operation for the duration of the
request. Since mds requests can be queued and performed asynchronously
we make sure to keep the idmapping around and release it once the
request has finished.

In follow-up patches we will use this to send correct ownership
information over the wire. This patch just adds the basic infrastructure
to keep the idmapping around. The actual conversion patches are all
fairly minimal.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
v2:
	- added "r_" prefix to the field name to make it consistent with others
---
 fs/ceph/mds_client.c | 7 +++++++
 fs/ceph/mds_client.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 4c0f22acf53d..810c3db2e369 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -12,6 +12,7 @@
 #include <linux/bits.h>
 #include <linux/ktime.h>
 #include <linux/bitmap.h>
+#include <linux/mnt_idmapping.h>
 
 #include "super.h"
 #include "mds_client.h"
@@ -962,6 +963,8 @@ void ceph_mdsc_release_request(struct kref *kref)
 	kfree(req->r_path1);
 	kfree(req->r_path2);
 	put_cred(req->r_cred);
+	if (req->r_mnt_idmap != &nop_mnt_idmap)
+		mnt_idmap_put(req->r_mnt_idmap);
 	if (req->r_pagelist)
 		ceph_pagelist_release(req->r_pagelist);
 	put_request_session(req);
@@ -1018,6 +1021,10 @@ static void __register_request(struct ceph_mds_client *mdsc,
 	insert_request(&mdsc->request_tree, req);
 
 	req->r_cred = get_current_cred();
+	if (!req->r_mnt_idmap)
+		req->r_mnt_idmap = &nop_mnt_idmap;
+	else
+		mnt_idmap_get(req->r_mnt_idmap);
 
 	if (mdsc->oldest_tid == 0 && req->r_op != CEPH_MDS_OP_SETFILELOCK)
 		mdsc->oldest_tid = req->r_tid;
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 724307ff89cd..32001ade1ea7 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -280,6 +280,7 @@ struct ceph_mds_request {
 	int r_fmode;        /* file mode, if expecting cap */
 	int r_request_release_offset;
 	const struct cred *r_cred;
+	struct mnt_idmap *r_mnt_idmap;
 	struct timespec64 r_stamp;
 
 	/* for choosing which mds to send this request to */
-- 
2.34.1

[PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Inode operations that create a new filesystem object such as ->mknod,
->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
Instead the caller's fs{g,u}id is used for the {g,u}id of the new
filesystem object.

Cephfs mds creation request argument structures mirror this filesystem
behavior. They don't encode a {g,u}id explicitly. Instead the caller's
fs{g,u}id that is always sent as part of any mds request is used by the
servers to set the {g,u}id of the new filesystem object.

In order to ensure that the correct {g,u}id is used map the caller's
fs{g,u}id for creation requests. This doesn't require complex changes.
It suffices to pass in the relevant idmapping recorded in the request
message. If this request message was triggered from an inode operation
that creates filesystem objects it will have passed down the relevant
idmaping. If this is a request message that was triggered from an inode
operation that doens't need to take idmappings into account the initial
idmapping is passed down which is an identity mapping and thus is
guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.

The last few weeks before Christmas 2021 I have spent time not just
reading and poking the cephfs kernel code but also took a look at the
ceph mds server userspace to ensure I didn't miss some subtlety.

This made me aware of one complication to solve. All requests send the
caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
server in exactly two cases:

1. to set the ownership for creation requests
2. to determine whether this client is allowed access on this server

Case 1. we already covered and explained. Case 2. is only relevant for
servers where an explicit uid access restriction has been set. That is
to say the mds server restricts access to requests coming from a
specific uid. Servers without uid restrictions will grant access to
requests from any uid by setting MDS_AUTH_UID_ANY.

Case 2. introduces the complication because the caller's fs{g,u}id is
not just used to record ownership but also serves as the {g,u}id used
when checking access to the server.

Consider a user mounting a cephfs client and creating an idmapped mount
from it that maps files owned by uid 1000 to be owned uid 0:

mount -t cephfs -o [...] /unmapped
mount-idmapped --map-mount 1000:0:1 /idmapped

That is to say if the mounted cephfs filesystem contains a file "file1"
which is owned by uid 1000:

- looking at it via /unmapped/file1 will report it as owned by uid 1000
  (One can think of this as the on-disk value.)
- looking at it via /idmapped/file1 will report it as owned by uid 0

Now, consider creating new files via the idmapped mount at /idmapped.
When a caller with fs{g,u}id 1000 creates a file "file2" by going
through the idmapped mount mounted at /idmapped it will create a file
that is owned by uid 1000 on-disk, i.e.:

- looking at it via /unmapped/file2 will report it as owned by uid 1000
- looking at it via /idmapped/file2 will report it as owned by uid 0

Now consider an mds server that has a uid access restriction set and
only grants access to requests from uid 0.

If the client sends a creation request for a file e.g. /idmapped/file2
it will send the caller's fs{g,u}id idmapped according to the idmapped
mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
0 in the idmapped mount and will be sent over the wire allowing the
caller access to the mds server.

However, if the caller is not issuing a creation request the caller's
fs{g,u}id will be send without the mount's idmapping applied. So if the
caller that just successfully created a new file on the restricted mds
server sends a request as fs{g,u}id 1000 access will be refused. This
however is inconsistent.

From my perspective the root of the problem lies in the fact that
creation requests implicitly infer the ownership from the {g,u}id that
gets sent along with every mds request.

I have thought of multiple ways of addressing this problem but the one I
prefer is to give all mds requests that create a filesystem object a
proper, separate {g,u}id field entry in the argument struct. This is,
for example how ->setattr mds requests work.

This way the caller's fs{g,u}id can be used consistenly for server
access checks and is separated from the ownership for new filesystem
objects.

Servers could then be updated to refuse creation requests whenever the
{g,u}id used for access checking doesn't match the {g,u}id used for
creating the filesystem object just as is done for setattr requests on a
uid restricted server. But I am, of course, open to other suggestions.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/mds_client.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 810c3db2e369..e4265843b838 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
 	void *p, *end;
 	int ret;
 	bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
+	kuid_t caller_fsuid;
+	kgid_t caller_fsgid;
 
 	ret = set_request_path_attr(req->r_inode, req->r_dentry,
 			      req->r_parent, req->r_path1, req->r_ino1.ino,
@@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
 
 	head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
 	head->op = cpu_to_le32(req->r_op);
-	head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
-						 req->r_cred->fsuid));
-	head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
-						 req->r_cred->fsgid));
+	/*
+	 * Inode operations that create filesystem objects based on the
+	 * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
+	 * have separate {g,u}id fields in their respective structs in the
+	 * ceph_mds_request_args union. Instead the caller_{g,u}id field is
+	 * used to set ownership of the newly created inode by the mds server.
+	 * For these inode operations we need to send the mapped fs{g,u}id over
+	 * the wire. For other cases we simple set req->r_mnt_idmap to the
+	 * initial idmapping meaning the unmapped fs{g,u}id is sent.
+	 */
+	caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
+					VFSUIDT_INIT(req->r_cred->fsuid));
+	caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
+					VFSGIDT_INIT(req->r_cred->fsgid));
+	head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
+	head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
 	head->ino = cpu_to_le64(req->r_deleg_ino);
 	head->args = req->r_args;
 
-- 
2.34.1

[PATCH v2 04/13] ceph: allow idmapped mknod inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_mknod() to handle idmapped mounts. This is just a matter of
passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index cb67ac821f0e..8d3fedb3629b 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -884,6 +884,7 @@ static int ceph_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	req->r_parent = dir;
 	ihold(dir);
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
+	req->r_mnt_idmap = idmap;
 	req->r_args.mknod.mode = cpu_to_le32(mode);
 	req->r_args.mknod.rdev = cpu_to_le32(rdev);
 	req->r_dentry_drop = CEPH_CAP_FILE_SHARED | CEPH_CAP_AUTH_EXCL;
-- 
2.34.1

[PATCH v2 05/13] ceph: allow idmapped symlink inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_symlink() to handle idmapped mounts. This is just a matter
of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 8d3fedb3629b..3996572060da 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -956,6 +956,7 @@ static int ceph_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	req->r_num_caps = 2;
 	req->r_dentry_drop = CEPH_CAP_FILE_SHARED | CEPH_CAP_AUTH_EXCL;
 	req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
+	req->r_mnt_idmap = idmap;
 	if (as_ctx.pagelist) {
 		req->r_pagelist = as_ctx.pagelist;
 		as_ctx.pagelist = NULL;
-- 
2.34.1

[PATCH v2 06/13] ceph: allow idmapped mkdir inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_mkdir() to handle idmapped mounts. This is just a matter of
passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 3996572060da..a4b1ee5ce6b6 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1024,6 +1024,7 @@ static int ceph_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	ihold(dir);
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
 	req->r_args.mkdir.mode = cpu_to_le32(mode);
+	req->r_mnt_idmap = idmap;
 	req->r_dentry_drop = CEPH_CAP_FILE_SHARED | CEPH_CAP_AUTH_EXCL;
 	req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
 	if (as_ctx.pagelist) {
-- 
2.34.1

[PATCH v2 07/13] ceph: allow idmapped rename inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_rename() to handle idmapped mounts. This is just a matter of
passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/dir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index a4b1ee5ce6b6..7ae02a690464 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1327,6 +1327,7 @@ static int ceph_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 	req->r_old_dentry_unless = CEPH_CAP_FILE_EXCL;
 	req->r_dentry_drop = CEPH_CAP_FILE_SHARED;
 	req->r_dentry_unless = CEPH_CAP_FILE_EXCL;
+	req->r_mnt_idmap = idmap;
 	/* release LINK_RDCACHE on source inode (mds will lock it) */
 	req->r_old_inode_drop = CEPH_CAP_LINK_SHARED | CEPH_CAP_LINK_EXCL;
 	if (d_really_is_positive(new_dentry)) {
-- 
2.34.1

[PATCH v2 08/13] ceph: allow idmapped getattr inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_getattr() to handle idmapped mounts. This is just a matter
of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8e5f41d45283..2e988612ed6c 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2465,7 +2465,7 @@ int ceph_getattr(struct mnt_idmap *idmap, const struct path *path,
 			return err;
 	}
 
-	generic_fillattr(&nop_mnt_idmap, inode, stat);
+	generic_fillattr(idmap, inode, stat);
 	stat->ino = ceph_present_inode(inode);
 
 	/*
-- 
2.34.1

[PATCH v2 09/13] ceph: allow idmapped permission inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_permission() to handle idmapped mounts. This is just a
matter of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 2e988612ed6c..37e1cbfc7c89 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2408,7 +2408,7 @@ int ceph_permission(struct mnt_idmap *idmap, struct inode *inode,
 	err = ceph_do_getattr(inode, CEPH_CAP_AUTH_SHARED, false);
 
 	if (!err)
-		err = generic_permission(&nop_mnt_idmap, inode, mask);
+		err = generic_permission(idmap, inode, mask);
 	return err;
 }
 
-- 
2.34.1

[PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/inode.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 37e1cbfc7c89..f1f934439be0 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
 
 	dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
 
+	/*
+	 * The attr->ia_{g,u}id members contain the target {g,u}id we're
+	 * sending over the wire. The mount idmapping only matters when we
+	 * create new filesystem objects based on the caller's mapped
+	 * fs{g,u}id.
+	 */
+	req->r_mnt_idmap = &nop_mnt_idmap;
 	if (ia_valid & ATTR_UID) {
 		dout("setattr %p uid %d -> %d\n", inode,
 		     from_kuid(&init_user_ns, inode->i_uid),
@@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	if (ceph_inode_is_shutdown(inode))
 		return -ESTALE;
 
-	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
+	err = setattr_prepare(idmap, dentry, attr);
 	if (err != 0)
 		return err;
 
@@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	err = __ceph_setattr(inode, attr);
 
 	if (err >= 0 && (attr->ia_valid & ATTR_MODE))
-		err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
+		err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
 
 	return err;
 }
-- 
2.34.1

[PATCH v2 11/13] ceph/acl: allow idmapped set_acl inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_set_acl() to handle idmapped mounts. This is just a matter
of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 6945a938d396..4291c890e324 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -105,7 +105,7 @@ int ceph_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			ret = posix_acl_update_mode(&nop_mnt_idmap, inode,
+			ret = posix_acl_update_mode(idmap, inode,
 						    &new_mode, &acl);
 			if (ret)
 				goto out;
-- 
2.34.1

[PATCH v2 12/13] ceph/file: allow idmapped atomic_open inode op

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Enable ceph_atomic_open() to handle idmapped mounts. This is just a
matter of passing down the mount's idmapping.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
v2:
	- rebased, see also 5fadbd9929 ("ceph: rely on vfs for setgid stripping")
---
 fs/ceph/file.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index f4d8bf7dec88..f00bfda4b1d2 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -654,7 +654,9 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 	in.truncate_seq = cpu_to_le32(1);
 	in.truncate_size = cpu_to_le64(-1ULL);
 	in.xattr_version = cpu_to_le64(1);
-	in.uid = cpu_to_le32(from_kuid(&init_user_ns, current_fsuid()));
+	in.uid = cpu_to_le32(from_kuid(&init_user_ns,
+				       mapped_fsuid(req->r_mnt_idmap,
+						    &init_user_ns)));
 	if (dir->i_mode & S_ISGID) {
 		in.gid = cpu_to_le32(from_kgid(&init_user_ns, dir->i_gid));
 
@@ -662,7 +664,9 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
 	} else {
-		in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid()));
+		in.gid = cpu_to_le32(from_kgid(&init_user_ns,
+				     mapped_fsgid(req->r_mnt_idmap,
+						  &init_user_ns)));
 	}
 	in.mode = cpu_to_le32((u32)mode);
 
@@ -731,6 +735,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 		     struct file *file, unsigned flags, umode_t mode)
 {
 	struct ceph_fs_client *fsc = ceph_sb_to_client(dir->i_sb);
+	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct ceph_mds_client *mdsc = fsc->mdsc;
 	struct ceph_mds_request *req;
 	struct dentry *dn;
@@ -786,6 +791,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 		mask |= CEPH_CAP_XATTR_SHARED;
 	req->r_args.open.mask = cpu_to_le32(mask);
 	req->r_parent = dir;
+	req->r_mnt_idmap = idmap;
 	ihold(dir);
 
 	if (flags & O_CREAT) {
-- 
2.34.1

[PATCH v2 13/13] ceph: allow idmapped mounts

[Alexander Mikhalitsyn]

From: Christian Brauner <christian.brauner@ubuntu.com>

Now that we converted cephfs internally to account for idmapped mounts
allow the creation of idmapped mounts on by setting the FS_ALLOW_IDMAP
flag.

Cc: Jeff Layton <jlayton@kernel.org>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: ceph-devel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
 fs/ceph/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 3fc48b43cab0..4f6e6d57f3f1 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1389,7 +1389,7 @@ static struct file_system_type ceph_fs_type = {
 	.name		= "ceph",
 	.init_fs_context = ceph_init_fs_context,
 	.kill_sb	= ceph_kill_sb,
-	.fs_flags	= FS_RENAME_DOES_D_MOVE,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE | FS_ALLOW_IDMAP,
 };
 MODULE_ALIAS_FS("ceph");
 
-- 
2.34.1

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Xiubo Li]

On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
>
> Inode operations that create a new filesystem object such as ->mknod,
> ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
> Instead the caller's fs{g,u}id is used for the {g,u}id of the new
> filesystem object.
>
> Cephfs mds creation request argument structures mirror this filesystem
> behavior. They don't encode a {g,u}id explicitly. Instead the caller's
> fs{g,u}id that is always sent as part of any mds request is used by the
> servers to set the {g,u}id of the new filesystem object.
>
> In order to ensure that the correct {g,u}id is used map the caller's
> fs{g,u}id for creation requests. This doesn't require complex changes.
> It suffices to pass in the relevant idmapping recorded in the request
> message. If this request message was triggered from an inode operation
> that creates filesystem objects it will have passed down the relevant
> idmaping. If this is a request message that was triggered from an inode
> operation that doens't need to take idmappings into account the initial
> idmapping is passed down which is an identity mapping and thus is
> guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
>
> The last few weeks before Christmas 2021 I have spent time not just
> reading and poking the cephfs kernel code but also took a look at the
> ceph mds server userspace to ensure I didn't miss some subtlety.
>
> This made me aware of one complication to solve. All requests send the
> caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
> server in exactly two cases:
>
> 1. to set the ownership for creation requests
> 2. to determine whether this client is allowed access on this server
>
> Case 1. we already covered and explained. Case 2. is only relevant for
> servers where an explicit uid access restriction has been set. That is
> to say the mds server restricts access to requests coming from a
> specific uid. Servers without uid restrictions will grant access to
> requests from any uid by setting MDS_AUTH_UID_ANY.
>
> Case 2. introduces the complication because the caller's fs{g,u}id is
> not just used to record ownership but also serves as the {g,u}id used
> when checking access to the server.
>
> Consider a user mounting a cephfs client and creating an idmapped mount
> from it that maps files owned by uid 1000 to be owned uid 0:
>
> mount -t cephfs -o [...] /unmapped
> mount-idmapped --map-mount 1000:0:1 /idmapped
>
> That is to say if the mounted cephfs filesystem contains a file "file1"
> which is owned by uid 1000:
>
> - looking at it via /unmapped/file1 will report it as owned by uid 1000
>    (One can think of this as the on-disk value.)
> - looking at it via /idmapped/file1 will report it as owned by uid 0
>
> Now, consider creating new files via the idmapped mount at /idmapped.
> When a caller with fs{g,u}id 1000 creates a file "file2" by going
> through the idmapped mount mounted at /idmapped it will create a file
> that is owned by uid 1000 on-disk, i.e.:
>
> - looking at it via /unmapped/file2 will report it as owned by uid 1000
> - looking at it via /idmapped/file2 will report it as owned by uid 0
>
> Now consider an mds server that has a uid access restriction set and
> only grants access to requests from uid 0.
>
> If the client sends a creation request for a file e.g. /idmapped/file2
> it will send the caller's fs{g,u}id idmapped according to the idmapped
> mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
> 0 in the idmapped mount and will be sent over the wire allowing the
> caller access to the mds server.
>
> However, if the caller is not issuing a creation request the caller's
> fs{g,u}id will be send without the mount's idmapping applied. So if the
> caller that just successfully created a new file on the restricted mds
> server sends a request as fs{g,u}id 1000 access will be refused. This
> however is inconsistent.
>
>  From my perspective the root of the problem lies in the fact that
> creation requests implicitly infer the ownership from the {g,u}id that
> gets sent along with every mds request.
>
> I have thought of multiple ways of addressing this problem but the one I
> prefer is to give all mds requests that create a filesystem object a
> proper, separate {g,u}id field entry in the argument struct. This is,
> for example how ->setattr mds requests work.
>
> This way the caller's fs{g,u}id can be used consistenly for server
> access checks and is separated from the ownership for new filesystem
> objects.
>
> Servers could then be updated to refuse creation requests whenever the
> {g,u}id used for access checking doesn't match the {g,u}id used for
> creating the filesystem object just as is done for setattr requests on a
> uid restricted server. But I am, of course, open to other suggestions.
>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Ilya Dryomov <idryomov@gmail.com>
> Cc: ceph-devel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>   fs/ceph/mds_client.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 810c3db2e369..e4265843b838 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>   	void *p, *end;
>   	int ret;
>   	bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
> +	kuid_t caller_fsuid;
> +	kgid_t caller_fsgid;
>   
>   	ret = set_request_path_attr(req->r_inode, req->r_dentry,
>   			      req->r_parent, req->r_path1, req->r_ino1.ino,
> @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>   
>   	head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
>   	head->op = cpu_to_le32(req->r_op);
> -	head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
> -						 req->r_cred->fsuid));
> -	head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
> -						 req->r_cred->fsgid));
> +	/*
> +	 * Inode operations that create filesystem objects based on the
> +	 * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
> +	 * have separate {g,u}id fields in their respective structs in the
> +	 * ceph_mds_request_args union. Instead the caller_{g,u}id field is
> +	 * used to set ownership of the newly created inode by the mds server.
> +	 * For these inode operations we need to send the mapped fs{g,u}id over
> +	 * the wire. For other cases we simple set req->r_mnt_idmap to the
> +	 * initial idmapping meaning the unmapped fs{g,u}id is sent.
> +	 */
> +	caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
> +					VFSUIDT_INIT(req->r_cred->fsuid));
> +	caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
> +					VFSGIDT_INIT(req->r_cred->fsgid));
> +	head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
> +	head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));

Hi Alexander,

You didn't answer Jeff and Greg's concerns in the first version 
https://www.spinics.net/lists/ceph-devel/msg53356.html.

I am also confused as Greg mentioned. If we just map the ids as 1000:0 
and created a file and then map the ids 1000:10, then the file couldn't 
be accessible, right ? Is this normal and as expected ?

IMO the idmapping should be client-side feature and we should make it 
consistent by using the unmapped fs{g,u}id always here.

Thanks

- Xiubo

>   	head->ino = cpu_to_le64(req->r_deleg_ino);
>   	head->args = req->r_args;
>   

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Aleksandr Mikhalitsyn]

On Mon, May 29, 2023 at 5:52 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > Inode operations that create a new filesystem object such as ->mknod,
> > ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
> > Instead the caller's fs{g,u}id is used for the {g,u}id of the new
> > filesystem object.
> >
> > Cephfs mds creation request argument structures mirror this filesystem
> > behavior. They don't encode a {g,u}id explicitly. Instead the caller's
> > fs{g,u}id that is always sent as part of any mds request is used by the
> > servers to set the {g,u}id of the new filesystem object.
> >
> > In order to ensure that the correct {g,u}id is used map the caller's
> > fs{g,u}id for creation requests. This doesn't require complex changes.
> > It suffices to pass in the relevant idmapping recorded in the request
> > message. If this request message was triggered from an inode operation
> > that creates filesystem objects it will have passed down the relevant
> > idmaping. If this is a request message that was triggered from an inode
> > operation that doens't need to take idmappings into account the initial
> > idmapping is passed down which is an identity mapping and thus is
> > guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
> >
> > The last few weeks before Christmas 2021 I have spent time not just
> > reading and poking the cephfs kernel code but also took a look at the
> > ceph mds server userspace to ensure I didn't miss some subtlety.
> >
> > This made me aware of one complication to solve. All requests send the
> > caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
> > server in exactly two cases:
> >
> > 1. to set the ownership for creation requests
> > 2. to determine whether this client is allowed access on this server
> >
> > Case 1. we already covered and explained. Case 2. is only relevant for
> > servers where an explicit uid access restriction has been set. That is
> > to say the mds server restricts access to requests coming from a
> > specific uid. Servers without uid restrictions will grant access to
> > requests from any uid by setting MDS_AUTH_UID_ANY.
> >
> > Case 2. introduces the complication because the caller's fs{g,u}id is
> > not just used to record ownership but also serves as the {g,u}id used
> > when checking access to the server.
> >
> > Consider a user mounting a cephfs client and creating an idmapped mount
> > from it that maps files owned by uid 1000 to be owned uid 0:
> >
> > mount -t cephfs -o [...] /unmapped
> > mount-idmapped --map-mount 1000:0:1 /idmapped
> >
> > That is to say if the mounted cephfs filesystem contains a file "file1"
> > which is owned by uid 1000:
> >
> > - looking at it via /unmapped/file1 will report it as owned by uid 1000
> >    (One can think of this as the on-disk value.)
> > - looking at it via /idmapped/file1 will report it as owned by uid 0
> >
> > Now, consider creating new files via the idmapped mount at /idmapped.
> > When a caller with fs{g,u}id 1000 creates a file "file2" by going
> > through the idmapped mount mounted at /idmapped it will create a file
> > that is owned by uid 1000 on-disk, i.e.:
> >
> > - looking at it via /unmapped/file2 will report it as owned by uid 1000
> > - looking at it via /idmapped/file2 will report it as owned by uid 0
> >
> > Now consider an mds server that has a uid access restriction set and
> > only grants access to requests from uid 0.
> >
> > If the client sends a creation request for a file e.g. /idmapped/file2
> > it will send the caller's fs{g,u}id idmapped according to the idmapped
> > mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
> > 0 in the idmapped mount and will be sent over the wire allowing the
> > caller access to the mds server.
> >
> > However, if the caller is not issuing a creation request the caller's
> > fs{g,u}id will be send without the mount's idmapping applied. So if the
> > caller that just successfully created a new file on the restricted mds
> > server sends a request as fs{g,u}id 1000 access will be refused. This
> > however is inconsistent.
> >
> >  From my perspective the root of the problem lies in the fact that
> > creation requests implicitly infer the ownership from the {g,u}id that
> > gets sent along with every mds request.
> >
> > I have thought of multiple ways of addressing this problem but the one I
> > prefer is to give all mds requests that create a filesystem object a
> > proper, separate {g,u}id field entry in the argument struct. This is,
> > for example how ->setattr mds requests work.
> >
> > This way the caller's fs{g,u}id can be used consistenly for server
> > access checks and is separated from the ownership for new filesystem
> > objects.
> >
> > Servers could then be updated to refuse creation requests whenever the
> > {g,u}id used for access checking doesn't match the {g,u}id used for
> > creating the filesystem object just as is done for setattr requests on a
> > uid restricted server. But I am, of course, open to other suggestions.
> >
> > Cc: Jeff Layton <jlayton@kernel.org>
> > Cc: Ilya Dryomov <idryomov@gmail.com>
> > Cc: ceph-devel@vger.kernel.org
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >   fs/ceph/mds_client.c | 22 ++++++++++++++++++----
> >   1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 810c3db2e369..e4265843b838 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >       void *p, *end;
> >       int ret;
> >       bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
> > +     kuid_t caller_fsuid;
> > +     kgid_t caller_fsgid;
> >
> >       ret = set_request_path_attr(req->r_inode, req->r_dentry,
> >                             req->r_parent, req->r_path1, req->r_ino1.ino,
> > @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >
> >       head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
> >       head->op = cpu_to_le32(req->r_op);
> > -     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
> > -                                              req->r_cred->fsuid));
> > -     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
> > -                                              req->r_cred->fsgid));
> > +     /*
> > +      * Inode operations that create filesystem objects based on the
> > +      * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
> > +      * have separate {g,u}id fields in their respective structs in the
> > +      * ceph_mds_request_args union. Instead the caller_{g,u}id field is
> > +      * used to set ownership of the newly created inode by the mds server.
> > +      * For these inode operations we need to send the mapped fs{g,u}id over
> > +      * the wire. For other cases we simple set req->r_mnt_idmap to the
> > +      * initial idmapping meaning the unmapped fs{g,u}id is sent.
> > +      */
> > +     caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
> > +                                     VFSUIDT_INIT(req->r_cred->fsuid));
> > +     caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
> > +                                     VFSGIDT_INIT(req->r_cred->fsgid));
> > +     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
> > +     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
>
> Hi Alexander,

Dear Xiubo,

Thanks for paying attention to this series!

>
> You didn't answer Jeff and Greg's concerns in the first version
> https://www.spinics.net/lists/ceph-devel/msg53356.html.

I've tried to respin discussion in the -v1 thread:
https://lore.kernel.org/all/20230519134420.2d04e5f70aad15679ab566fc@canonical.com/

No one replied, so I decided to send rebased and slightly changed -v2,
where I've fixed this:
https://lore.kernel.org/all/041afbfd171915d62ab9a93c7a35d9c9d5c5bf7b.camel@kernel.org/

>
> I am also confused as Greg mentioned. If we just map the ids as 1000:0
> and created a file and then map the ids 1000:10, then the file couldn't
> be accessible, right ? Is this normal and as expected ?

This can be a problem only if filtering based on the UID is turned on
on the server side (which is a relatively rare case).

idmapped mounts are not about mapping a caller UID/GID, idmapped
mounts are about mapping inode owner's UID/GID.
So, for example if you have UID 1000 (on disk) and have an idmapping
1000:0 then it will be shown as owned by 0.
If you create a file from a user with UID 0 then you will get UID 1000
on disk. To achieve that, we map a current user fs{g,u}id
when sending a creation request according to the idmapping mount to
make things consistent. But when a user opens a file,
we are sending UID/GID as they are without applying an idmapping. Of
course, generic_permission() kernel helper is aware of
mount idmapping and before open request will go to the server we will
check that current user is allowed to open this file (and during
this check UID/GID of a current user and UID/GID of the file owner
will be properly compared). I.e. this issue is only relevant for the
case
when we have additional permission checks on the network file system
server side.

>
> IMO the idmapping should be client-side feature and we should make it
> consistent by using the unmapped fs{g,u}id always here.

To make the current user fs{g,u}id always idmapped we need to make
really big changes in the VFS layer. And it's not obvious
that it justifies the cost. Because this particular feature with
Cephfs idmapped mounts is already used/tested with LXD/LXC workloads
and it works perfectly well. And as far as I know, LXD/LXC were the
first idmapped mount adopters. IMHO, it's better to
start from this approach and if someone will want to extend this
functionality for network filesystems and want to map fs{g,u}id which
are sent over the
wire we will take a look at that. Because anyway, integration with
Cephfs is important for the LXD project and we are looking closely at
this.

Kind regards,
Alex

>
> Thanks
>
> - Xiubo
>
> >       head->ino = cpu_to_le64(req->r_deleg_ino);
> >       head->args = req->r_args;
> >
>

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Xiubo Li]

On 6/1/23 00:32, Aleksandr Mikhalitsyn wrote:
> On Mon, May 29, 2023 at 5:52 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
>>> From: Christian Brauner <christian.brauner@ubuntu.com>
>>>
>>> Inode operations that create a new filesystem object such as ->mknod,
>>> ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
>>> Instead the caller's fs{g,u}id is used for the {g,u}id of the new
>>> filesystem object.
>>>
>>> Cephfs mds creation request argument structures mirror this filesystem
>>> behavior. They don't encode a {g,u}id explicitly. Instead the caller's
>>> fs{g,u}id that is always sent as part of any mds request is used by the
>>> servers to set the {g,u}id of the new filesystem object.
>>>
>>> In order to ensure that the correct {g,u}id is used map the caller's
>>> fs{g,u}id for creation requests. This doesn't require complex changes.
>>> It suffices to pass in the relevant idmapping recorded in the request
>>> message. If this request message was triggered from an inode operation
>>> that creates filesystem objects it will have passed down the relevant
>>> idmaping. If this is a request message that was triggered from an inode
>>> operation that doens't need to take idmappings into account the initial
>>> idmapping is passed down which is an identity mapping and thus is
>>> guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
>>>
>>> The last few weeks before Christmas 2021 I have spent time not just
>>> reading and poking the cephfs kernel code but also took a look at the
>>> ceph mds server userspace to ensure I didn't miss some subtlety.
>>>
>>> This made me aware of one complication to solve. All requests send the
>>> caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
>>> server in exactly two cases:
>>>
>>> 1. to set the ownership for creation requests
>>> 2. to determine whether this client is allowed access on this server
>>>
>>> Case 1. we already covered and explained. Case 2. is only relevant for
>>> servers where an explicit uid access restriction has been set. That is
>>> to say the mds server restricts access to requests coming from a
>>> specific uid. Servers without uid restrictions will grant access to
>>> requests from any uid by setting MDS_AUTH_UID_ANY.
>>>
>>> Case 2. introduces the complication because the caller's fs{g,u}id is
>>> not just used to record ownership but also serves as the {g,u}id used
>>> when checking access to the server.
>>>
>>> Consider a user mounting a cephfs client and creating an idmapped mount
>>> from it that maps files owned by uid 1000 to be owned uid 0:
>>>
>>> mount -t cephfs -o [...] /unmapped
>>> mount-idmapped --map-mount 1000:0:1 /idmapped
>>>
>>> That is to say if the mounted cephfs filesystem contains a file "file1"
>>> which is owned by uid 1000:
>>>
>>> - looking at it via /unmapped/file1 will report it as owned by uid 1000
>>>     (One can think of this as the on-disk value.)
>>> - looking at it via /idmapped/file1 will report it as owned by uid 0
>>>
>>> Now, consider creating new files via the idmapped mount at /idmapped.
>>> When a caller with fs{g,u}id 1000 creates a file "file2" by going
>>> through the idmapped mount mounted at /idmapped it will create a file
>>> that is owned by uid 1000 on-disk, i.e.:
>>>
>>> - looking at it via /unmapped/file2 will report it as owned by uid 1000
>>> - looking at it via /idmapped/file2 will report it as owned by uid 0
>>>
>>> Now consider an mds server that has a uid access restriction set and
>>> only grants access to requests from uid 0.
>>>
>>> If the client sends a creation request for a file e.g. /idmapped/file2
>>> it will send the caller's fs{g,u}id idmapped according to the idmapped
>>> mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
>>> 0 in the idmapped mount and will be sent over the wire allowing the
>>> caller access to the mds server.
>>>
>>> However, if the caller is not issuing a creation request the caller's
>>> fs{g,u}id will be send without the mount's idmapping applied. So if the
>>> caller that just successfully created a new file on the restricted mds
>>> server sends a request as fs{g,u}id 1000 access will be refused. This
>>> however is inconsistent.
>>>
>>>   From my perspective the root of the problem lies in the fact that
>>> creation requests implicitly infer the ownership from the {g,u}id that
>>> gets sent along with every mds request.
>>>
>>> I have thought of multiple ways of addressing this problem but the one I
>>> prefer is to give all mds requests that create a filesystem object a
>>> proper, separate {g,u}id field entry in the argument struct. This is,
>>> for example how ->setattr mds requests work.
>>>
>>> This way the caller's fs{g,u}id can be used consistenly for server
>>> access checks and is separated from the ownership for new filesystem
>>> objects.
>>>
>>> Servers could then be updated to refuse creation requests whenever the
>>> {g,u}id used for access checking doesn't match the {g,u}id used for
>>> creating the filesystem object just as is done for setattr requests on a
>>> uid restricted server. But I am, of course, open to other suggestions.
>>>
>>> Cc: Jeff Layton <jlayton@kernel.org>
>>> Cc: Ilya Dryomov <idryomov@gmail.com>
>>> Cc: ceph-devel@vger.kernel.org
>>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
>>> ---
>>>    fs/ceph/mds_client.c | 22 ++++++++++++++++++----
>>>    1 file changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 810c3db2e369..e4265843b838 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>>>        void *p, *end;
>>>        int ret;
>>>        bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
>>> +     kuid_t caller_fsuid;
>>> +     kgid_t caller_fsgid;
>>>
>>>        ret = set_request_path_attr(req->r_inode, req->r_dentry,
>>>                              req->r_parent, req->r_path1, req->r_ino1.ino,
>>> @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>>>
>>>        head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
>>>        head->op = cpu_to_le32(req->r_op);
>>> -     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
>>> -                                              req->r_cred->fsuid));
>>> -     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
>>> -                                              req->r_cred->fsgid));
>>> +     /*
>>> +      * Inode operations that create filesystem objects based on the
>>> +      * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
>>> +      * have separate {g,u}id fields in their respective structs in the
>>> +      * ceph_mds_request_args union. Instead the caller_{g,u}id field is
>>> +      * used to set ownership of the newly created inode by the mds server.
>>> +      * For these inode operations we need to send the mapped fs{g,u}id over
>>> +      * the wire. For other cases we simple set req->r_mnt_idmap to the
>>> +      * initial idmapping meaning the unmapped fs{g,u}id is sent.
>>> +      */
>>> +     caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
>>> +                                     VFSUIDT_INIT(req->r_cred->fsuid));
>>> +     caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
>>> +                                     VFSGIDT_INIT(req->r_cred->fsgid));
>>> +     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
>>> +     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
>> Hi Alexander,
> Dear Xiubo,
>
> Thanks for paying attention to this series!
>
>> You didn't answer Jeff and Greg's concerns in the first version
>> https://www.spinics.net/lists/ceph-devel/msg53356.html.
> I've tried to respin discussion in the -v1 thread:
> https://lore.kernel.org/all/20230519134420.2d04e5f70aad15679ab566fc@canonical.com/
>
> No one replied, so I decided to send rebased and slightly changed -v2,
> where I've fixed this:
> https://lore.kernel.org/all/041afbfd171915d62ab9a93c7a35d9c9d5c5bf7b.camel@kernel.org/
>
>> I am also confused as Greg mentioned. If we just map the ids as 1000:0
>> and created a file and then map the ids 1000:10, then the file couldn't
>> be accessible, right ? Is this normal and as expected ?
> This can be a problem only if filtering based on the UID is turned on
> on the server side (which is a relatively rare case).
>
> idmapped mounts are not about mapping a caller UID/GID, idmapped
> mounts are about mapping inode owner's UID/GID.
> So, for example if you have UID 1000 (on disk) and have an idmapping
> 1000:0 then it will be shown as owned by 0.

My understanding was that on the disk the files' owner UID should be 
1000 always, while in the client side it will show file's owner as the 
mapped UID 0 with an idmapping 1000:0.

This should be the same as what you mentioned above, right ?

> If you create a file from a user with UID 0 then you will get UID 1000
> on disk. To achieve that, we map a current user fs{g,u}id
> when sending a creation request according to the idmapping mount to
> make things consistent.

As you know the cephfs MDSs will use the creation requests' caller UID 
as the owner's UID when creating new inodes.

Which means that if the creation requests switches to use the mapped UID 
0 as the caller UID then the file's owner will be UID 0 instead of UID 
1000 in cephfs MDSs. Does this what this patch want to do ?


>   But when a user opens a file,
> we are sending UID/GID as they are without applying an idmapping.

If my understanding is correct above, then when opening the file with 
non-mapped UID 1000 it may fail because the files' owner is UID 0.

Correct me if my understanding is wrong.

>   Of
> course, generic_permission() kernel helper is aware of
> mount idmapping

Yeah, this was also what I thought it should be.

There is another client auth feature [1] for cephfs. The MDS will allow 
us to set a path restriction for specify UID, more detail please see [2]:

  allow rw path=/dir1 uid=1000 gids=1000

This may cause the creation requests to fail if you set the caller UID 
to the mapped UID.


[1] https://docs.ceph.com/en/latest/cephfs/client-auth/
[2] https://tracker.ceph.com/issues/59388


Thanks

- Xiubo

> and before open request will go to the server we will
> check that current user is allowed to open this file (and during
> this check UID/GID of a current user and UID/GID of the file owner
> will be properly compared). I.e. this issue is only relevant for the
> case
> when we have additional permission checks on the network file system
> server side.
>
>> IMO the idmapping should be client-side feature and we should make it
>> consistent by using the unmapped fs{g,u}id always here.
> To make the current user fs{g,u}id always idmapped we need to make
> really big changes in the VFS layer. And it's not obvious
> that it justifies the cost. Because this particular feature with
> Cephfs idmapped mounts is already used/tested with LXD/LXC workloads
> and it works perfectly well. And as far as I know, LXD/LXC were the
> first idmapped mount adopters. IMHO, it's better to
> start from this approach and if someone will want to extend this
> functionality for network filesystems and want to map fs{g,u}id which
> are sent over the
> wire we will take a look at that. Because anyway, integration with
> Cephfs is important for the LXD project and we are looking closely at
> this.
>
> Kind regards,
> Alex
>
>> Thanks
>>
>> - Xiubo
>>
>>>        head->ino = cpu_to_le64(req->r_deleg_ino);
>>>        head->args = req->r_args;
>>>

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Aleksandr Mikhalitsyn]

On Thu, Jun 1, 2023 at 4:29 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 6/1/23 00:32, Aleksandr Mikhalitsyn wrote:
> > On Mon, May 29, 2023 at 5:52 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>
> >> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> >>> From: Christian Brauner <christian.brauner@ubuntu.com>
> >>>
> >>> Inode operations that create a new filesystem object such as ->mknod,
> >>> ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
> >>> Instead the caller's fs{g,u}id is used for the {g,u}id of the new
> >>> filesystem object.
> >>>
> >>> Cephfs mds creation request argument structures mirror this filesystem
> >>> behavior. They don't encode a {g,u}id explicitly. Instead the caller's
> >>> fs{g,u}id that is always sent as part of any mds request is used by the
> >>> servers to set the {g,u}id of the new filesystem object.
> >>>
> >>> In order to ensure that the correct {g,u}id is used map the caller's
> >>> fs{g,u}id for creation requests. This doesn't require complex changes.
> >>> It suffices to pass in the relevant idmapping recorded in the request
> >>> message. If this request message was triggered from an inode operation
> >>> that creates filesystem objects it will have passed down the relevant
> >>> idmaping. If this is a request message that was triggered from an inode
> >>> operation that doens't need to take idmappings into account the initial
> >>> idmapping is passed down which is an identity mapping and thus is
> >>> guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
> >>>
> >>> The last few weeks before Christmas 2021 I have spent time not just
> >>> reading and poking the cephfs kernel code but also took a look at the
> >>> ceph mds server userspace to ensure I didn't miss some subtlety.
> >>>
> >>> This made me aware of one complication to solve. All requests send the
> >>> caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
> >>> server in exactly two cases:
> >>>
> >>> 1. to set the ownership for creation requests
> >>> 2. to determine whether this client is allowed access on this server
> >>>
> >>> Case 1. we already covered and explained. Case 2. is only relevant for
> >>> servers where an explicit uid access restriction has been set. That is
> >>> to say the mds server restricts access to requests coming from a
> >>> specific uid. Servers without uid restrictions will grant access to
> >>> requests from any uid by setting MDS_AUTH_UID_ANY.
> >>>
> >>> Case 2. introduces the complication because the caller's fs{g,u}id is
> >>> not just used to record ownership but also serves as the {g,u}id used
> >>> when checking access to the server.
> >>>
> >>> Consider a user mounting a cephfs client and creating an idmapped mount
> >>> from it that maps files owned by uid 1000 to be owned uid 0:
> >>>
> >>> mount -t cephfs -o [...] /unmapped
> >>> mount-idmapped --map-mount 1000:0:1 /idmapped
> >>>
> >>> That is to say if the mounted cephfs filesystem contains a file "file1"
> >>> which is owned by uid 1000:
> >>>
> >>> - looking at it via /unmapped/file1 will report it as owned by uid 1000
> >>>     (One can think of this as the on-disk value.)
> >>> - looking at it via /idmapped/file1 will report it as owned by uid 0
> >>>
> >>> Now, consider creating new files via the idmapped mount at /idmapped.
> >>> When a caller with fs{g,u}id 1000 creates a file "file2" by going
> >>> through the idmapped mount mounted at /idmapped it will create a file
> >>> that is owned by uid 1000 on-disk, i.e.:
> >>>
> >>> - looking at it via /unmapped/file2 will report it as owned by uid 1000
> >>> - looking at it via /idmapped/file2 will report it as owned by uid 0
> >>>
> >>> Now consider an mds server that has a uid access restriction set and
> >>> only grants access to requests from uid 0.
> >>>
> >>> If the client sends a creation request for a file e.g. /idmapped/file2
> >>> it will send the caller's fs{g,u}id idmapped according to the idmapped
> >>> mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
> >>> 0 in the idmapped mount and will be sent over the wire allowing the
> >>> caller access to the mds server.
> >>>
> >>> However, if the caller is not issuing a creation request the caller's
> >>> fs{g,u}id will be send without the mount's idmapping applied. So if the
> >>> caller that just successfully created a new file on the restricted mds
> >>> server sends a request as fs{g,u}id 1000 access will be refused. This
> >>> however is inconsistent.
> >>>
> >>>   From my perspective the root of the problem lies in the fact that
> >>> creation requests implicitly infer the ownership from the {g,u}id that
> >>> gets sent along with every mds request.
> >>>
> >>> I have thought of multiple ways of addressing this problem but the one I
> >>> prefer is to give all mds requests that create a filesystem object a
> >>> proper, separate {g,u}id field entry in the argument struct. This is,
> >>> for example how ->setattr mds requests work.
> >>>
> >>> This way the caller's fs{g,u}id can be used consistenly for server
> >>> access checks and is separated from the ownership for new filesystem
> >>> objects.
> >>>
> >>> Servers could then be updated to refuse creation requests whenever the
> >>> {g,u}id used for access checking doesn't match the {g,u}id used for
> >>> creating the filesystem object just as is done for setattr requests on a
> >>> uid restricted server. But I am, of course, open to other suggestions.
> >>>
> >>> Cc: Jeff Layton <jlayton@kernel.org>
> >>> Cc: Ilya Dryomov <idryomov@gmail.com>
> >>> Cc: ceph-devel@vger.kernel.org
> >>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> >>> ---
> >>>    fs/ceph/mds_client.c | 22 ++++++++++++++++++----
> >>>    1 file changed, 18 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >>> index 810c3db2e369..e4265843b838 100644
> >>> --- a/fs/ceph/mds_client.c
> >>> +++ b/fs/ceph/mds_client.c
> >>> @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>>        void *p, *end;
> >>>        int ret;
> >>>        bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
> >>> +     kuid_t caller_fsuid;
> >>> +     kgid_t caller_fsgid;
> >>>
> >>>        ret = set_request_path_attr(req->r_inode, req->r_dentry,
> >>>                              req->r_parent, req->r_path1, req->r_ino1.ino,
> >>> @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>>
> >>>        head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
> >>>        head->op = cpu_to_le32(req->r_op);
> >>> -     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
> >>> -                                              req->r_cred->fsuid));
> >>> -     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
> >>> -                                              req->r_cred->fsgid));
> >>> +     /*
> >>> +      * Inode operations that create filesystem objects based on the
> >>> +      * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
> >>> +      * have separate {g,u}id fields in their respective structs in the
> >>> +      * ceph_mds_request_args union. Instead the caller_{g,u}id field is
> >>> +      * used to set ownership of the newly created inode by the mds server.
> >>> +      * For these inode operations we need to send the mapped fs{g,u}id over
> >>> +      * the wire. For other cases we simple set req->r_mnt_idmap to the
> >>> +      * initial idmapping meaning the unmapped fs{g,u}id is sent.
> >>> +      */
> >>> +     caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
> >>> +                                     VFSUIDT_INIT(req->r_cred->fsuid));
> >>> +     caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
> >>> +                                     VFSGIDT_INIT(req->r_cred->fsgid));
> >>> +     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
> >>> +     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
> >> Hi Alexander,
> > Dear Xiubo,
> >
> > Thanks for paying attention to this series!
> >
> >> You didn't answer Jeff and Greg's concerns in the first version
> >> https://www.spinics.net/lists/ceph-devel/msg53356.html.
> > I've tried to respin discussion in the -v1 thread:
> > https://lore.kernel.org/all/20230519134420.2d04e5f70aad15679ab566fc@canonical.com/
> >
> > No one replied, so I decided to send rebased and slightly changed -v2,
> > where I've fixed this:
> > https://lore.kernel.org/all/041afbfd171915d62ab9a93c7a35d9c9d5c5bf7b.camel@kernel.org/
> >
> >> I am also confused as Greg mentioned. If we just map the ids as 1000:0
> >> and created a file and then map the ids 1000:10, then the file couldn't
> >> be accessible, right ? Is this normal and as expected ?
> > This can be a problem only if filtering based on the UID is turned on
> > on the server side (which is a relatively rare case).
> >
> > idmapped mounts are not about mapping a caller UID/GID, idmapped
> > mounts are about mapping inode owner's UID/GID.
> > So, for example if you have UID 1000 (on disk) and have an idmapping
> > 1000:0 then it will be shown as owned by 0.
>
> My understanding was that on the disk the files' owner UID should be
> 1000 always, while in the client side it will show file's owner as the
> mapped UID 0 with an idmapping 1000:0.

Hi, Xiubo!

>
> This should be the same as what you mentioned above, right ?

Right.

Let me show a real output from a real command line experiment :-)

1. Mount cephfs

mount.ceph admin@XYZ.cephfs=/ /mnt/ceph -o
mon_addr=127.0.0.1:6789,secret=very_secret_key

2. Make 1000:1000 a root dentry owner (it will be convenient because
we want to use mapping 1000:0:1 for simplicity)

chown 1000:1000 /mnt/ceph

3. create an idmapped mount based on a regular /mnt/ceph mount using a
mount-idmapped tool that was written by Christian.
[ taken from https://raw.githubusercontent.com/brauner/mount-idmapped/master/mount-idmapped.c
]

./mount-idmapped --map-mount b:1000:0:1 /mnt/ceph /mnt/ceph_idmapped

"b" stands for "both", so we are creating a mapping of length 1 for
both UID and GID.
1000 is a UID/GID "on-disk", 0 is a mapped UID/GID.

4. Just to be precise, let's look at which UID/GID we have now.

root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
total 4
drwxrwxrwx 2 1000 1000    0 Jun  1 17:51 .
drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..

root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
total 4
drwxrwxrwx 2 0 0    0 Jun  1 17:51 .
drwxr-xr-x 4 0 0 4096 Jun  1 16:55 ..

5. Now let's create a bunch of files with different owners and through
different mounts (idmapped/non-idmapped).

5.1. Create a file from 0:0 through the idmapped mount (it should
appear as 1000:1000 on disk)
root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
/mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid0

5.2. Create a file from 1000:1000 through the idmapped mount (should
fail because 1000:1000 is not a valid UID/GID as it can't be mapped
back to the "on-disk" UID/GID set).
root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
/mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000
touch: cannot touch
'/mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000': Value
too large for defined data type

... and we've got EOVERFLOW. That's correct!

5.3. Create a file from 0:0 but through the regular mount. (it should
appear as overflowuid(=65534) in idmapped mount, because 0:0 on-disk
is not mapped to the UID/GID set).

root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
/mnt/ceph/created_directly_with_uid0

5.4. Create a file from 1000:1000 but through the regular mount. (it
should appear as 0:0 in idmapped mount, because 1000 (on-disk) mapped
to 0).

root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
/mnt/ceph/created_directly_with_uid1000

6. Now let's look on the result:

root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
total 4
drwxrwxrwx 2 1000 1000    3 Jun  1 17:54 .
drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..
-rw-r--r-- 1    0    0    0 Jun  1 17:54 created_directly_with_uid0
-rw-rw-r-- 1 1000 1000    0 Jun  1 17:54 created_directly_with_uid1000
-rw-r--r-- 1 1000 1000    0 Jun  1 17:53 created_through_idmapped_mnt_with_uid0

root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
total 4
drwxrwxrwx 2     0     0    3 Jun  1 17:54 .
drwxr-xr-x 4     0     0 4096 Jun  1 16:55 ..
-rw-r--r-- 1 65534 65534    0 Jun  1 17:54 created_directly_with_uid0
-rw-rw-r-- 1     0     0    0 Jun  1 17:54 created_directly_with_uid1000
-rw-r--r-- 1     0     0    0 Jun  1 17:53
created_through_idmapped_mnt_with_uid0

>
> > If you create a file from a user with UID 0 then you will get UID 1000
> > on disk. To achieve that, we map a current user fs{g,u}id
> > when sending a creation request according to the idmapping mount to
> > make things consistent.
>
> As you know the cephfs MDSs will use the creation requests' caller UID
> as the owner's UID when creating new inodes.

Yes, that's why we have to map a caller UID to end up with the correct
value of a file owner.

>
> Which means that if the creation requests switches to use the mapped UID
> 0 as the caller UID then the file's owner will be UID 0 instead of UID
> 1000 in cephfs MDSs. Does this what this patch want to do ?

In my example we have a caller with UID equal 0, then the mapped UID
will be 1000. So, the file will be created with UID = 1000.

>
>
> >   But when a user opens a file,
> > we are sending UID/GID as they are without applying an idmapping.
>
> If my understanding is correct above, then when opening the file with
> non-mapped UID 1000 it may fail because the files' owner is UID 0.
>
> Correct me if my understanding is wrong.
>
> >   Of
> > course, generic_permission() kernel helper is aware of
> > mount idmapping
>
> Yeah, this was also what I thought it should be.
>
> There is another client auth feature [1] for cephfs. The MDS will allow
> us to set a path restriction for specify UID, more detail please see [2]:
>
>   allow rw path=/dir1 uid=1000 gids=1000
>
> This may cause the creation requests to fail if you set the caller UID
> to the mapped UID.

Yes, that can be a problem of course. But it will only affect users
who want to use this feature and it doesn't open any security holes.
It's just a limitation of this approach. Unfortunately it's barely
fixable without massive VFS changes and until we have no real use
cases
for this combination of idmapped mounts + MDS UID/GID-based path
restriction we are not sure that it makes sense to implement this
right now.

>
>
> [1] https://docs.ceph.com/en/latest/cephfs/client-auth/
> [2] https://tracker.ceph.com/issues/59388

Thanks, I'll take a look closer at that!

Thanks for closely looking into this patchset, Xiubo!

Kind regards,
Alex

>
>
> Thanks
>
> - Xiubo
>
> > and before open request will go to the server we will
> > check that current user is allowed to open this file (and during
> > this check UID/GID of a current user and UID/GID of the file owner
> > will be properly compared). I.e. this issue is only relevant for the
> > case
> > when we have additional permission checks on the network file system
> > server side.
> >
> >> IMO the idmapping should be client-side feature and we should make it
> >> consistent by using the unmapped fs{g,u}id always here.
> > To make the current user fs{g,u}id always idmapped we need to make
> > really big changes in the VFS layer. And it's not obvious
> > that it justifies the cost. Because this particular feature with
> > Cephfs idmapped mounts is already used/tested with LXD/LXC workloads
> > and it works perfectly well. And as far as I know, LXD/LXC were the
> > first idmapped mount adopters. IMHO, it's better to
> > start from this approach and if someone will want to extend this
> > functionality for network filesystems and want to map fs{g,u}id which
> > are sent over the
> > wire we will take a look at that. Because anyway, integration with
> > Cephfs is important for the LXD project and we are looking closely at
> > this.
> >
> > Kind regards,
> > Alex
> >
> >> Thanks
> >>
> >> - Xiubo
> >>
> >>>        head->ino = cpu_to_le64(req->r_deleg_ino);
> >>>        head->args = req->r_args;
> >>>
>

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Xiubo Li]

On 6/2/23 02:29, Aleksandr Mikhalitsyn wrote:
> On Thu, Jun 1, 2023 at 4:29 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 6/1/23 00:32, Aleksandr Mikhalitsyn wrote:
>>> On Mon, May 29, 2023 at 5:52 AM Xiubo Li <xiubli@redhat.com> wrote:
>>>> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
>>>>> From: Christian Brauner <christian.brauner@ubuntu.com>
>>>>>
>>>>> Inode operations that create a new filesystem object such as ->mknod,
>>>>> ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
>>>>> Instead the caller's fs{g,u}id is used for the {g,u}id of the new
>>>>> filesystem object.
>>>>>
>>>>> Cephfs mds creation request argument structures mirror this filesystem
>>>>> behavior. They don't encode a {g,u}id explicitly. Instead the caller's
>>>>> fs{g,u}id that is always sent as part of any mds request is used by the
>>>>> servers to set the {g,u}id of the new filesystem object.
>>>>>
>>>>> In order to ensure that the correct {g,u}id is used map the caller's
>>>>> fs{g,u}id for creation requests. This doesn't require complex changes.
>>>>> It suffices to pass in the relevant idmapping recorded in the request
>>>>> message. If this request message was triggered from an inode operation
>>>>> that creates filesystem objects it will have passed down the relevant
>>>>> idmaping. If this is a request message that was triggered from an inode
>>>>> operation that doens't need to take idmappings into account the initial
>>>>> idmapping is passed down which is an identity mapping and thus is
>>>>> guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
>>>>>
>>>>> The last few weeks before Christmas 2021 I have spent time not just
>>>>> reading and poking the cephfs kernel code but also took a look at the
>>>>> ceph mds server userspace to ensure I didn't miss some subtlety.
>>>>>
>>>>> This made me aware of one complication to solve. All requests send the
>>>>> caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
>>>>> server in exactly two cases:
>>>>>
>>>>> 1. to set the ownership for creation requests
>>>>> 2. to determine whether this client is allowed access on this server
>>>>>
>>>>> Case 1. we already covered and explained. Case 2. is only relevant for
>>>>> servers where an explicit uid access restriction has been set. That is
>>>>> to say the mds server restricts access to requests coming from a
>>>>> specific uid. Servers without uid restrictions will grant access to
>>>>> requests from any uid by setting MDS_AUTH_UID_ANY.
>>>>>
>>>>> Case 2. introduces the complication because the caller's fs{g,u}id is
>>>>> not just used to record ownership but also serves as the {g,u}id used
>>>>> when checking access to the server.
>>>>>
>>>>> Consider a user mounting a cephfs client and creating an idmapped mount
>>>>> from it that maps files owned by uid 1000 to be owned uid 0:
>>>>>
>>>>> mount -t cephfs -o [...] /unmapped
>>>>> mount-idmapped --map-mount 1000:0:1 /idmapped
>>>>>
>>>>> That is to say if the mounted cephfs filesystem contains a file "file1"
>>>>> which is owned by uid 1000:
>>>>>
>>>>> - looking at it via /unmapped/file1 will report it as owned by uid 1000
>>>>>      (One can think of this as the on-disk value.)
>>>>> - looking at it via /idmapped/file1 will report it as owned by uid 0
>>>>>
>>>>> Now, consider creating new files via the idmapped mount at /idmapped.
>>>>> When a caller with fs{g,u}id 1000 creates a file "file2" by going
>>>>> through the idmapped mount mounted at /idmapped it will create a file
>>>>> that is owned by uid 1000 on-disk, i.e.:
>>>>>
>>>>> - looking at it via /unmapped/file2 will report it as owned by uid 1000
>>>>> - looking at it via /idmapped/file2 will report it as owned by uid 0
>>>>>
>>>>> Now consider an mds server that has a uid access restriction set and
>>>>> only grants access to requests from uid 0.
>>>>>
>>>>> If the client sends a creation request for a file e.g. /idmapped/file2
>>>>> it will send the caller's fs{g,u}id idmapped according to the idmapped
>>>>> mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
>>>>> 0 in the idmapped mount and will be sent over the wire allowing the
>>>>> caller access to the mds server.
>>>>>
>>>>> However, if the caller is not issuing a creation request the caller's
>>>>> fs{g,u}id will be send without the mount's idmapping applied. So if the
>>>>> caller that just successfully created a new file on the restricted mds
>>>>> server sends a request as fs{g,u}id 1000 access will be refused. This
>>>>> however is inconsistent.
>>>>>
>>>>>    From my perspective the root of the problem lies in the fact that
>>>>> creation requests implicitly infer the ownership from the {g,u}id that
>>>>> gets sent along with every mds request.
>>>>>
>>>>> I have thought of multiple ways of addressing this problem but the one I
>>>>> prefer is to give all mds requests that create a filesystem object a
>>>>> proper, separate {g,u}id field entry in the argument struct. This is,
>>>>> for example how ->setattr mds requests work.
>>>>>
>>>>> This way the caller's fs{g,u}id can be used consistenly for server
>>>>> access checks and is separated from the ownership for new filesystem
>>>>> objects.
>>>>>
>>>>> Servers could then be updated to refuse creation requests whenever the
>>>>> {g,u}id used for access checking doesn't match the {g,u}id used for
>>>>> creating the filesystem object just as is done for setattr requests on a
>>>>> uid restricted server. But I am, of course, open to other suggestions.
>>>>>
>>>>> Cc: Jeff Layton <jlayton@kernel.org>
>>>>> Cc: Ilya Dryomov <idryomov@gmail.com>
>>>>> Cc: ceph-devel@vger.kernel.org
>>>>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>>>>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
>>>>> ---
>>>>>     fs/ceph/mds_client.c | 22 ++++++++++++++++++----
>>>>>     1 file changed, 18 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>> index 810c3db2e369..e4265843b838 100644
>>>>> --- a/fs/ceph/mds_client.c
>>>>> +++ b/fs/ceph/mds_client.c
>>>>> @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>>>>>         void *p, *end;
>>>>>         int ret;
>>>>>         bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
>>>>> +     kuid_t caller_fsuid;
>>>>> +     kgid_t caller_fsgid;
>>>>>
>>>>>         ret = set_request_path_attr(req->r_inode, req->r_dentry,
>>>>>                               req->r_parent, req->r_path1, req->r_ino1.ino,
>>>>> @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
>>>>>
>>>>>         head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
>>>>>         head->op = cpu_to_le32(req->r_op);
>>>>> -     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
>>>>> -                                              req->r_cred->fsuid));
>>>>> -     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
>>>>> -                                              req->r_cred->fsgid));
>>>>> +     /*
>>>>> +      * Inode operations that create filesystem objects based on the
>>>>> +      * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
>>>>> +      * have separate {g,u}id fields in their respective structs in the
>>>>> +      * ceph_mds_request_args union. Instead the caller_{g,u}id field is
>>>>> +      * used to set ownership of the newly created inode by the mds server.
>>>>> +      * For these inode operations we need to send the mapped fs{g,u}id over
>>>>> +      * the wire. For other cases we simple set req->r_mnt_idmap to the
>>>>> +      * initial idmapping meaning the unmapped fs{g,u}id is sent.
>>>>> +      */
>>>>> +     caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
>>>>> +                                     VFSUIDT_INIT(req->r_cred->fsuid));
>>>>> +     caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
>>>>> +                                     VFSGIDT_INIT(req->r_cred->fsgid));
>>>>> +     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
>>>>> +     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
>>>> Hi Alexander,
>>> Dear Xiubo,
>>>
>>> Thanks for paying attention to this series!
>>>
>>>> You didn't answer Jeff and Greg's concerns in the first version
>>>> https://www.spinics.net/lists/ceph-devel/msg53356.html.
>>> I've tried to respin discussion in the -v1 thread:
>>> https://lore.kernel.org/all/20230519134420.2d04e5f70aad15679ab566fc@canonical.com/
>>>
>>> No one replied, so I decided to send rebased and slightly changed -v2,
>>> where I've fixed this:
>>> https://lore.kernel.org/all/041afbfd171915d62ab9a93c7a35d9c9d5c5bf7b.camel@kernel.org/
>>>
>>>> I am also confused as Greg mentioned. If we just map the ids as 1000:0
>>>> and created a file and then map the ids 1000:10, then the file couldn't
>>>> be accessible, right ? Is this normal and as expected ?
>>> This can be a problem only if filtering based on the UID is turned on
>>> on the server side (which is a relatively rare case).
>>>
>>> idmapped mounts are not about mapping a caller UID/GID, idmapped
>>> mounts are about mapping inode owner's UID/GID.
>>> So, for example if you have UID 1000 (on disk) and have an idmapping
>>> 1000:0 then it will be shown as owned by 0.
>> My understanding was that on the disk the files' owner UID should be
>> 1000 always, while in the client side it will show file's owner as the
>> mapped UID 0 with an idmapping 1000:0.
> Hi, Xiubo!
>
>> This should be the same as what you mentioned above, right ?
> Right.
>
> Let me show a real output from a real command line experiment :-)
>
> 1. Mount cephfs
>
> mount.ceph admin@XYZ.cephfs=/ /mnt/ceph -o
> mon_addr=127.0.0.1:6789,secret=very_secret_key
>
> 2. Make 1000:1000 a root dentry owner (it will be convenient because
> we want to use mapping 1000:0:1 for simplicity)
>
> chown 1000:1000 /mnt/ceph
>
> 3. create an idmapped mount based on a regular /mnt/ceph mount using a
> mount-idmapped tool that was written by Christian.
> [ taken from https://raw.githubusercontent.com/brauner/mount-idmapped/master/mount-idmapped.c
> ]
>
> ./mount-idmapped --map-mount b:1000:0:1 /mnt/ceph /mnt/ceph_idmapped
>
> "b" stands for "both", so we are creating a mapping of length 1 for
> both UID and GID.
> 1000 is a UID/GID "on-disk", 0 is a mapped UID/GID.
>
> 4. Just to be precise, let's look at which UID/GID we have now.
>
> root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
> total 4
> drwxrwxrwx 2 1000 1000    0 Jun  1 17:51 .
> drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..
>
> root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
> total 4
> drwxrwxrwx 2 0 0    0 Jun  1 17:51 .
> drwxr-xr-x 4 0 0 4096 Jun  1 16:55 ..
>
> 5. Now let's create a bunch of files with different owners and through
> different mounts (idmapped/non-idmapped).
>
> 5.1. Create a file from 0:0 through the idmapped mount (it should
> appear as 1000:1000 on disk)
> root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
> /mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid0
>
> 5.2. Create a file from 1000:1000 through the idmapped mount (should
> fail because 1000:1000 is not a valid UID/GID as it can't be mapped
> back to the "on-disk" UID/GID set).
> root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
> /mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000
> touch: cannot touch
> '/mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000': Value
> too large for defined data type
>
> ... and we've got EOVERFLOW. That's correct!
>
> 5.3. Create a file from 0:0 but through the regular mount. (it should
> appear as overflowuid(=65534) in idmapped mount, because 0:0 on-disk
> is not mapped to the UID/GID set).
>
> root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
> /mnt/ceph/created_directly_with_uid0
>
> 5.4. Create a file from 1000:1000 but through the regular mount. (it
> should appear as 0:0 in idmapped mount, because 1000 (on-disk) mapped
> to 0).
>
> root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
> /mnt/ceph/created_directly_with_uid1000
>
> 6. Now let's look on the result:
>
> root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
> total 4
> drwxrwxrwx 2 1000 1000    3 Jun  1 17:54 .
> drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..
> -rw-r--r-- 1    0    0    0 Jun  1 17:54 created_directly_with_uid0
> -rw-rw-r-- 1 1000 1000    0 Jun  1 17:54 created_directly_with_uid1000
> -rw-r--r-- 1 1000 1000    0 Jun  1 17:53 created_through_idmapped_mnt_with_uid0
>
> root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
> total 4
> drwxrwxrwx 2     0     0    3 Jun  1 17:54 .
> drwxr-xr-x 4     0     0 4096 Jun  1 16:55 ..
> -rw-r--r-- 1 65534 65534    0 Jun  1 17:54 created_directly_with_uid0
> -rw-rw-r-- 1     0     0    0 Jun  1 17:54 created_directly_with_uid1000
> -rw-r--r-- 1     0     0    0 Jun  1 17:53
> created_through_idmapped_mnt_with_uid0
>
>>> If you create a file from a user with UID 0 then you will get UID 1000
>>> on disk. To achieve that, we map a current user fs{g,u}id
>>> when sending a creation request according to the idmapping mount to
>>> make things consistent.
>> As you know the cephfs MDSs will use the creation requests' caller UID
>> as the owner's UID when creating new inodes.
> Yes, that's why we have to map a caller UID to end up with the correct
> value of a file owner.
>
Hmm, I think my understanding was incorrect. This patch here is trying 
to get the correct value of UID 1000 from a mapped mount, which the UID 0.


>> Which means that if the creation requests switches to use the mapped UID
>> 0 as the caller UID then the file's owner will be UID 0 instead of UID
>> 1000 in cephfs MDSs. Does this what this patch want to do ?
> In my example we have a caller with UID equal 0, then the mapped UID
> will be 1000. So, the file will be created with UID = 1000.

Okay, thanks for your above example it helped me to understand the idmap 
logic. Before I tried to read the xfstests test cases and VFS code about 
the idmap but didn't totally done yet.

I will test and review the patches again today or next week.

Thanks

- Xiubo

>>
>>>    But when a user opens a file,
>>> we are sending UID/GID as they are without applying an idmapping.
>> If my understanding is correct above, then when opening the file with
>> non-mapped UID 1000 it may fail because the files' owner is UID 0.
>>
>> Correct me if my understanding is wrong.
>>
>>>    Of
>>> course, generic_permission() kernel helper is aware of
>>> mount idmapping
>> Yeah, this was also what I thought it should be.
>>
>> There is another client auth feature [1] for cephfs. The MDS will allow
>> us to set a path restriction for specify UID, more detail please see [2]:
>>
>>    allow rw path=/dir1 uid=1000 gids=1000
>>
>> This may cause the creation requests to fail if you set the caller UID
>> to the mapped UID.
> Yes, that can be a problem of course. But it will only affect users
> who want to use this feature and it doesn't open any security holes.
> It's just a limitation of this approach. Unfortunately it's barely
> fixable without massive VFS changes and until we have no real use
> cases
> for this combination of idmapped mounts + MDS UID/GID-based path
> restriction we are not sure that it makes sense to implement this
> right now.
>
>>
>> [1] https://docs.ceph.com/en/latest/cephfs/client-auth/
>> [2] https://tracker.ceph.com/issues/59388
> Thanks, I'll take a look closer at that!
>
> Thanks for closely looking into this patchset, Xiubo!
>
> Kind regards,
> Alex
>
>>
>> Thanks
>>
>> - Xiubo
>>
>>> and before open request will go to the server we will
>>> check that current user is allowed to open this file (and during
>>> this check UID/GID of a current user and UID/GID of the file owner
>>> will be properly compared). I.e. this issue is only relevant for the
>>> case
>>> when we have additional permission checks on the network file system
>>> server side.
>>>
>>>> IMO the idmapping should be client-side feature and we should make it
>>>> consistent by using the unmapped fs{g,u}id always here.
>>> To make the current user fs{g,u}id always idmapped we need to make
>>> really big changes in the VFS layer. And it's not obvious
>>> that it justifies the cost. Because this particular feature with
>>> Cephfs idmapped mounts is already used/tested with LXD/LXC workloads
>>> and it works perfectly well. And as far as I know, LXD/LXC were the
>>> first idmapped mount adopters. IMHO, it's better to
>>> start from this approach and if someone will want to extend this
>>> functionality for network filesystems and want to map fs{g,u}id which
>>> are sent over the
>>> wire we will take a look at that. Because anyway, integration with
>>> Cephfs is important for the LXD project and we are looking closely at
>>> this.
>>>
>>> Kind regards,
>>> Alex
>>>
>>>> Thanks
>>>>
>>>> - Xiubo
>>>>
>>>>>         head->ino = cpu_to_le64(req->r_deleg_ino);
>>>>>         head->args = req->r_args;
>>>>>

Re: [PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Xiubo Li]

On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> These helpers are required to support idmapped mounts in the Cephfs.
>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>   fs/mnt_idmapping.c            | 2 ++
>   include/linux/mnt_idmapping.h | 3 +++
>   2 files changed, 5 insertions(+)
>
> diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
> index 4905665c47d0..5a579e809bcf 100644
> --- a/fs/mnt_idmapping.c
> +++ b/fs/mnt_idmapping.c
> @@ -256,6 +256,7 @@ struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap)
>   
>   	return idmap;
>   }
> +EXPORT_SYMBOL(mnt_idmap_get);
>   
>   /**
>    * mnt_idmap_put - put a reference to an idmapping
> @@ -271,3 +272,4 @@ void mnt_idmap_put(struct mnt_idmap *idmap)
>   		kfree(idmap);
>   	}
>   }
> +EXPORT_SYMBOL(mnt_idmap_put);
> diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
> index 057c89867aa2..b8da2db4ecd2 100644
> --- a/include/linux/mnt_idmapping.h
> +++ b/include/linux/mnt_idmapping.h
> @@ -115,6 +115,9 @@ static inline bool vfsgid_eq_kgid(vfsgid_t vfsgid, kgid_t kgid)
>   
>   int vfsgid_in_group_p(vfsgid_t vfsgid);
>   
> +struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
> +void mnt_idmap_put(struct mnt_idmap *idmap);
> +
>   vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
>   		     struct user_namespace *fs_userns, kuid_t kuid);
>   

Hi Alexander,

This needs the "fs/mnt_idmapping.c" maintainer's ack.

Thanks

- Xiubo

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Xiubo Li]

On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
>
> Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> of passing down the mount's idmapping.
>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Ilya Dryomov <idryomov@gmail.com>
> Cc: ceph-devel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>   fs/ceph/inode.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 37e1cbfc7c89..f1f934439be0 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
>   
>   	dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
>   
> +	/*
> +	 * The attr->ia_{g,u}id members contain the target {g,u}id we're
> +	 * sending over the wire. The mount idmapping only matters when we
> +	 * create new filesystem objects based on the caller's mapped
> +	 * fs{g,u}id.
> +	 */
> +	req->r_mnt_idmap = &nop_mnt_idmap;

For example with an idmapping 1000:0 and in the /mnt/idmapped_ceph/.

This means the "__ceph_setattr()" will always use UID 0 to set the 
caller_uid, right ? If it is then the client auth checking for the 
setattr requests in cephfs MDS will succeed, since the UID 0 is root. 
But if you use a different idmapping, such as 1000:2000, it will fail.

So here IMO we should set it to 'idmap' too ?

Thanks

- Xiubo

>   	if (ia_valid & ATTR_UID) {
>   		dout("setattr %p uid %d -> %d\n", inode,
>   		     from_kuid(&init_user_ns, inode->i_uid),
> @@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   	if (ceph_inode_is_shutdown(inode))
>   		return -ESTALE;
>   
> -	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> +	err = setattr_prepare(idmap, dentry, attr);
>   	if (err != 0)
>   		return err;
>   
> @@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>   	err = __ceph_setattr(inode, attr);
>   
>   	if (err >= 0 && (attr->ia_valid & ATTR_MODE))
> -		err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
> +		err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
>   
>   	return err;
>   }

Re: [PATCH v2 08/13] ceph: allow idmapped getattr inode op

[Xiubo Li]

On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
>
> Enable ceph_getattr() to handle idmapped mounts. This is just a matter
> of passing down the mount's idmapping.
>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Ilya Dryomov <idryomov@gmail.com>
> Cc: ceph-devel@vger.kernel.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
>   fs/ceph/inode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8e5f41d45283..2e988612ed6c 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -2465,7 +2465,7 @@ int ceph_getattr(struct mnt_idmap *idmap, const struct path *path,
>   			return err;
>   	}
>   
> -	generic_fillattr(&nop_mnt_idmap, inode, stat);
> +	generic_fillattr(idmap, inode, stat);
>   	stat->ino = ceph_present_inode(inode);
>   
>   	/*

As mentioned in my comment in "[PATCH v2 10/13] ceph: allow idmapped 
setattr inode op". The getattr requests may fail too in the MDS when 
doing the client auth checking.

So for all the requests we should always get the correct UID/GID instead 
of only for the creating requests, then we can make sure that the idmap 
is only a feature in client side and then in cephfs MDS side it will 
always get a consistent UID/GID no matter what idmappings the clients 
are using.

Right ?

Thanks

- Xiubo

Re: [PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 3:17 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > These helpers are required to support idmapped mounts in the Cephfs.
> >
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >   fs/mnt_idmapping.c            | 2 ++
> >   include/linux/mnt_idmapping.h | 3 +++
> >   2 files changed, 5 insertions(+)
> >
> > diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
> > index 4905665c47d0..5a579e809bcf 100644
> > --- a/fs/mnt_idmapping.c
> > +++ b/fs/mnt_idmapping.c
> > @@ -256,6 +256,7 @@ struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap)
> >
> >       return idmap;
> >   }
> > +EXPORT_SYMBOL(mnt_idmap_get);
> >
> >   /**
> >    * mnt_idmap_put - put a reference to an idmapping
> > @@ -271,3 +272,4 @@ void mnt_idmap_put(struct mnt_idmap *idmap)
> >               kfree(idmap);
> >       }
> >   }
> > +EXPORT_SYMBOL(mnt_idmap_put);
> > diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
> > index 057c89867aa2..b8da2db4ecd2 100644
> > --- a/include/linux/mnt_idmapping.h
> > +++ b/include/linux/mnt_idmapping.h
> > @@ -115,6 +115,9 @@ static inline bool vfsgid_eq_kgid(vfsgid_t vfsgid, kgid_t kgid)
> >
> >   int vfsgid_in_group_p(vfsgid_t vfsgid);
> >
> > +struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
> > +void mnt_idmap_put(struct mnt_idmap *idmap);
> > +
> >   vfsuid_t make_vfsuid(struct mnt_idmap *idmap,
> >                    struct user_namespace *fs_userns, kuid_t kuid);
> >
>
> Hi Alexander,

Hi, Xiubo!

>
> This needs the "fs/mnt_idmapping.c" maintainer's ack.

Sure, I hope that Christian will take a look.

Thanks,
Alex

>
> Thanks
>
> - Xiubo
>

Re: [PATCH v2 03/13] ceph: handle idmapped mounts in create_request_message()

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 2:41 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 6/2/23 02:29, Aleksandr Mikhalitsyn wrote:
> > On Thu, Jun 1, 2023 at 4:29 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>
> >> On 6/1/23 00:32, Aleksandr Mikhalitsyn wrote:
> >>> On Mon, May 29, 2023 at 5:52 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>>> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> >>>>> From: Christian Brauner <christian.brauner@ubuntu.com>
> >>>>>
> >>>>> Inode operations that create a new filesystem object such as ->mknod,
> >>>>> ->create, ->mkdir() and others don't take a {g,u}id argument explicitly.
> >>>>> Instead the caller's fs{g,u}id is used for the {g,u}id of the new
> >>>>> filesystem object.
> >>>>>
> >>>>> Cephfs mds creation request argument structures mirror this filesystem
> >>>>> behavior. They don't encode a {g,u}id explicitly. Instead the caller's
> >>>>> fs{g,u}id that is always sent as part of any mds request is used by the
> >>>>> servers to set the {g,u}id of the new filesystem object.
> >>>>>
> >>>>> In order to ensure that the correct {g,u}id is used map the caller's
> >>>>> fs{g,u}id for creation requests. This doesn't require complex changes.
> >>>>> It suffices to pass in the relevant idmapping recorded in the request
> >>>>> message. If this request message was triggered from an inode operation
> >>>>> that creates filesystem objects it will have passed down the relevant
> >>>>> idmaping. If this is a request message that was triggered from an inode
> >>>>> operation that doens't need to take idmappings into account the initial
> >>>>> idmapping is passed down which is an identity mapping and thus is
> >>>>> guaranteed to leave the caller's fs{g,u}id unchanged.,u}id is sent.
> >>>>>
> >>>>> The last few weeks before Christmas 2021 I have spent time not just
> >>>>> reading and poking the cephfs kernel code but also took a look at the
> >>>>> ceph mds server userspace to ensure I didn't miss some subtlety.
> >>>>>
> >>>>> This made me aware of one complication to solve. All requests send the
> >>>>> caller's fs{g,u}id over the wire. The caller's fs{g,u}id matters for the
> >>>>> server in exactly two cases:
> >>>>>
> >>>>> 1. to set the ownership for creation requests
> >>>>> 2. to determine whether this client is allowed access on this server
> >>>>>
> >>>>> Case 1. we already covered and explained. Case 2. is only relevant for
> >>>>> servers where an explicit uid access restriction has been set. That is
> >>>>> to say the mds server restricts access to requests coming from a
> >>>>> specific uid. Servers without uid restrictions will grant access to
> >>>>> requests from any uid by setting MDS_AUTH_UID_ANY.
> >>>>>
> >>>>> Case 2. introduces the complication because the caller's fs{g,u}id is
> >>>>> not just used to record ownership but also serves as the {g,u}id used
> >>>>> when checking access to the server.
> >>>>>
> >>>>> Consider a user mounting a cephfs client and creating an idmapped mount
> >>>>> from it that maps files owned by uid 1000 to be owned uid 0:
> >>>>>
> >>>>> mount -t cephfs -o [...] /unmapped
> >>>>> mount-idmapped --map-mount 1000:0:1 /idmapped
> >>>>>
> >>>>> That is to say if the mounted cephfs filesystem contains a file "file1"
> >>>>> which is owned by uid 1000:
> >>>>>
> >>>>> - looking at it via /unmapped/file1 will report it as owned by uid 1000
> >>>>>      (One can think of this as the on-disk value.)
> >>>>> - looking at it via /idmapped/file1 will report it as owned by uid 0
> >>>>>
> >>>>> Now, consider creating new files via the idmapped mount at /idmapped.
> >>>>> When a caller with fs{g,u}id 1000 creates a file "file2" by going
> >>>>> through the idmapped mount mounted at /idmapped it will create a file
> >>>>> that is owned by uid 1000 on-disk, i.e.:
> >>>>>
> >>>>> - looking at it via /unmapped/file2 will report it as owned by uid 1000
> >>>>> - looking at it via /idmapped/file2 will report it as owned by uid 0
> >>>>>
> >>>>> Now consider an mds server that has a uid access restriction set and
> >>>>> only grants access to requests from uid 0.
> >>>>>
> >>>>> If the client sends a creation request for a file e.g. /idmapped/file2
> >>>>> it will send the caller's fs{g,u}id idmapped according to the idmapped
> >>>>> mount. So if the caller has fs{g,u}id 1000 it will be mapped to {g,u}id
> >>>>> 0 in the idmapped mount and will be sent over the wire allowing the
> >>>>> caller access to the mds server.
> >>>>>
> >>>>> However, if the caller is not issuing a creation request the caller's
> >>>>> fs{g,u}id will be send without the mount's idmapping applied. So if the
> >>>>> caller that just successfully created a new file on the restricted mds
> >>>>> server sends a request as fs{g,u}id 1000 access will be refused. This
> >>>>> however is inconsistent.
> >>>>>
> >>>>>    From my perspective the root of the problem lies in the fact that
> >>>>> creation requests implicitly infer the ownership from the {g,u}id that
> >>>>> gets sent along with every mds request.
> >>>>>
> >>>>> I have thought of multiple ways of addressing this problem but the one I
> >>>>> prefer is to give all mds requests that create a filesystem object a
> >>>>> proper, separate {g,u}id field entry in the argument struct. This is,
> >>>>> for example how ->setattr mds requests work.
> >>>>>
> >>>>> This way the caller's fs{g,u}id can be used consistenly for server
> >>>>> access checks and is separated from the ownership for new filesystem
> >>>>> objects.
> >>>>>
> >>>>> Servers could then be updated to refuse creation requests whenever the
> >>>>> {g,u}id used for access checking doesn't match the {g,u}id used for
> >>>>> creating the filesystem object just as is done for setattr requests on a
> >>>>> uid restricted server. But I am, of course, open to other suggestions.
> >>>>>
> >>>>> Cc: Jeff Layton <jlayton@kernel.org>
> >>>>> Cc: Ilya Dryomov <idryomov@gmail.com>
> >>>>> Cc: ceph-devel@vger.kernel.org
> >>>>> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >>>>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> >>>>> ---
> >>>>>     fs/ceph/mds_client.c | 22 ++++++++++++++++++----
> >>>>>     1 file changed, 18 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >>>>> index 810c3db2e369..e4265843b838 100644
> >>>>> --- a/fs/ceph/mds_client.c
> >>>>> +++ b/fs/ceph/mds_client.c
> >>>>> @@ -2583,6 +2583,8 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>>>>         void *p, *end;
> >>>>>         int ret;
> >>>>>         bool legacy = !(session->s_con.peer_features & CEPH_FEATURE_FS_BTIME);
> >>>>> +     kuid_t caller_fsuid;
> >>>>> +     kgid_t caller_fsgid;
> >>>>>
> >>>>>         ret = set_request_path_attr(req->r_inode, req->r_dentry,
> >>>>>                               req->r_parent, req->r_path1, req->r_ino1.ino,
> >>>>> @@ -2651,10 +2653,22 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>>>>
> >>>>>         head->mdsmap_epoch = cpu_to_le32(mdsc->mdsmap->m_epoch);
> >>>>>         head->op = cpu_to_le32(req->r_op);
> >>>>> -     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns,
> >>>>> -                                              req->r_cred->fsuid));
> >>>>> -     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns,
> >>>>> -                                              req->r_cred->fsgid));
> >>>>> +     /*
> >>>>> +      * Inode operations that create filesystem objects based on the
> >>>>> +      * caller's fs{g,u}id like ->mknod(), ->create(), ->mkdir() etc. don't
> >>>>> +      * have separate {g,u}id fields in their respective structs in the
> >>>>> +      * ceph_mds_request_args union. Instead the caller_{g,u}id field is
> >>>>> +      * used to set ownership of the newly created inode by the mds server.
> >>>>> +      * For these inode operations we need to send the mapped fs{g,u}id over
> >>>>> +      * the wire. For other cases we simple set req->r_mnt_idmap to the
> >>>>> +      * initial idmapping meaning the unmapped fs{g,u}id is sent.
> >>>>> +      */
> >>>>> +     caller_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
> >>>>> +                                     VFSUIDT_INIT(req->r_cred->fsuid));
> >>>>> +     caller_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
> >>>>> +                                     VFSGIDT_INIT(req->r_cred->fsgid));
> >>>>> +     head->caller_uid = cpu_to_le32(from_kuid(&init_user_ns, caller_fsuid));
> >>>>> +     head->caller_gid = cpu_to_le32(from_kgid(&init_user_ns, caller_fsgid));
> >>>> Hi Alexander,
> >>> Dear Xiubo,
> >>>
> >>> Thanks for paying attention to this series!
> >>>
> >>>> You didn't answer Jeff and Greg's concerns in the first version
> >>>> https://www.spinics.net/lists/ceph-devel/msg53356.html.
> >>> I've tried to respin discussion in the -v1 thread:
> >>> https://lore.kernel.org/all/20230519134420.2d04e5f70aad15679ab566fc@canonical.com/
> >>>
> >>> No one replied, so I decided to send rebased and slightly changed -v2,
> >>> where I've fixed this:
> >>> https://lore.kernel.org/all/041afbfd171915d62ab9a93c7a35d9c9d5c5bf7b.camel@kernel.org/
> >>>
> >>>> I am also confused as Greg mentioned. If we just map the ids as 1000:0
> >>>> and created a file and then map the ids 1000:10, then the file couldn't
> >>>> be accessible, right ? Is this normal and as expected ?
> >>> This can be a problem only if filtering based on the UID is turned on
> >>> on the server side (which is a relatively rare case).
> >>>
> >>> idmapped mounts are not about mapping a caller UID/GID, idmapped
> >>> mounts are about mapping inode owner's UID/GID.
> >>> So, for example if you have UID 1000 (on disk) and have an idmapping
> >>> 1000:0 then it will be shown as owned by 0.
> >> My understanding was that on the disk the files' owner UID should be
> >> 1000 always, while in the client side it will show file's owner as the
> >> mapped UID 0 with an idmapping 1000:0.
> > Hi, Xiubo!
> >
> >> This should be the same as what you mentioned above, right ?
> > Right.
> >
> > Let me show a real output from a real command line experiment :-)
> >
> > 1. Mount cephfs
> >
> > mount.ceph admin@XYZ.cephfs=/ /mnt/ceph -o
> > mon_addr=127.0.0.1:6789,secret=very_secret_key
> >
> > 2. Make 1000:1000 a root dentry owner (it will be convenient because
> > we want to use mapping 1000:0:1 for simplicity)
> >
> > chown 1000:1000 /mnt/ceph
> >
> > 3. create an idmapped mount based on a regular /mnt/ceph mount using a
> > mount-idmapped tool that was written by Christian.
> > [ taken from https://raw.githubusercontent.com/brauner/mount-idmapped/master/mount-idmapped.c
> > ]
> >
> > ./mount-idmapped --map-mount b:1000:0:1 /mnt/ceph /mnt/ceph_idmapped
> >
> > "b" stands for "both", so we are creating a mapping of length 1 for
> > both UID and GID.
> > 1000 is a UID/GID "on-disk", 0 is a mapped UID/GID.
> >
> > 4. Just to be precise, let's look at which UID/GID we have now.
> >
> > root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
> > total 4
> > drwxrwxrwx 2 1000 1000    0 Jun  1 17:51 .
> > drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..
> >
> > root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
> > total 4
> > drwxrwxrwx 2 0 0    0 Jun  1 17:51 .
> > drwxr-xr-x 4 0 0 4096 Jun  1 16:55 ..
> >
> > 5. Now let's create a bunch of files with different owners and through
> > different mounts (idmapped/non-idmapped).
> >
> > 5.1. Create a file from 0:0 through the idmapped mount (it should
> > appear as 1000:1000 on disk)
> > root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
> > /mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid0
> >
> > 5.2. Create a file from 1000:1000 through the idmapped mount (should
> > fail because 1000:1000 is not a valid UID/GID as it can't be mapped
> > back to the "on-disk" UID/GID set).
> > root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
> > /mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000
> > touch: cannot touch
> > '/mnt/ceph_idmapped/created_through_idmapped_mnt_with_uid1000': Value
> > too large for defined data type
> >
> > ... and we've got EOVERFLOW. That's correct!
> >
> > 5.3. Create a file from 0:0 but through the regular mount. (it should
> > appear as overflowuid(=65534) in idmapped mount, because 0:0 on-disk
> > is not mapped to the UID/GID set).
> >
> > root@ubuntu:/home/ubuntu# sudo -u#0 -g#0 touch
> > /mnt/ceph/created_directly_with_uid0
> >
> > 5.4. Create a file from 1000:1000 but through the regular mount. (it
> > should appear as 0:0 in idmapped mount, because 1000 (on-disk) mapped
> > to 0).
> >
> > root@ubuntu:/home/ubuntu# sudo -u#1000 -g#1000 touch
> > /mnt/ceph/created_directly_with_uid1000
> >
> > 6. Now let's look on the result:
> >
> > root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph
> > total 4
> > drwxrwxrwx 2 1000 1000    3 Jun  1 17:54 .
> > drwxr-xr-x 4    0    0 4096 Jun  1 16:55 ..
> > -rw-r--r-- 1    0    0    0 Jun  1 17:54 created_directly_with_uid0
> > -rw-rw-r-- 1 1000 1000    0 Jun  1 17:54 created_directly_with_uid1000
> > -rw-r--r-- 1 1000 1000    0 Jun  1 17:53 created_through_idmapped_mnt_with_uid0
> >
> > root@ubuntu:/home/ubuntu# ls -lan /mnt/ceph_idmapped
> > total 4
> > drwxrwxrwx 2     0     0    3 Jun  1 17:54 .
> > drwxr-xr-x 4     0     0 4096 Jun  1 16:55 ..
> > -rw-r--r-- 1 65534 65534    0 Jun  1 17:54 created_directly_with_uid0
> > -rw-rw-r-- 1     0     0    0 Jun  1 17:54 created_directly_with_uid1000
> > -rw-r--r-- 1     0     0    0 Jun  1 17:53
> > created_through_idmapped_mnt_with_uid0
> >
> >>> If you create a file from a user with UID 0 then you will get UID 1000
> >>> on disk. To achieve that, we map a current user fs{g,u}id
> >>> when sending a creation request according to the idmapping mount to
> >>> make things consistent.
> >> As you know the cephfs MDSs will use the creation requests' caller UID
> >> as the owner's UID when creating new inodes.
> > Yes, that's why we have to map a caller UID to end up with the correct
> > value of a file owner.
> >
> Hmm, I think my understanding was incorrect. This patch here is trying
> to get the correct value of UID 1000 from a mapped mount, which the UID 0.
>
>
> >> Which means that if the creation requests switches to use the mapped UID
> >> 0 as the caller UID then the file's owner will be UID 0 instead of UID
> >> 1000 in cephfs MDSs. Does this what this patch want to do ?
> > In my example we have a caller with UID equal 0, then the mapped UID
> > will be 1000. So, the file will be created with UID = 1000.
>
> Okay, thanks for your above example it helped me to understand the idmap
> logic. Before I tried to read the xfstests test cases and VFS code about
> the idmap but didn't totally done yet.

Yeah, it's not trivial (especially when it's combined with a network
filesystem ;-) ).
But Christian had written a good documentation about mount idmappings:
https://github.com/torvalds/linux/blob/master/Documentation/filesystems/idmappings.rst

>
> I will test and review the patches again today or next week.

Huge thanks, Xiubo!

>
> Thanks
>
> - Xiubo
>
> >>
> >>>    But when a user opens a file,
> >>> we are sending UID/GID as they are without applying an idmapping.
> >> If my understanding is correct above, then when opening the file with
> >> non-mapped UID 1000 it may fail because the files' owner is UID 0.
> >>
> >> Correct me if my understanding is wrong.
> >>
> >>>    Of
> >>> course, generic_permission() kernel helper is aware of
> >>> mount idmapping
> >> Yeah, this was also what I thought it should be.
> >>
> >> There is another client auth feature [1] for cephfs. The MDS will allow
> >> us to set a path restriction for specify UID, more detail please see [2]:
> >>
> >>    allow rw path=/dir1 uid=1000 gids=1000
> >>
> >> This may cause the creation requests to fail if you set the caller UID
> >> to the mapped UID.
> > Yes, that can be a problem of course. But it will only affect users
> > who want to use this feature and it doesn't open any security holes.
> > It's just a limitation of this approach. Unfortunately it's barely
> > fixable without massive VFS changes and until we have no real use
> > cases
> > for this combination of idmapped mounts + MDS UID/GID-based path
> > restriction we are not sure that it makes sense to implement this
> > right now.
> >
> >>
> >> [1] https://docs.ceph.com/en/latest/cephfs/client-auth/
> >> [2] https://tracker.ceph.com/issues/59388
> > Thanks, I'll take a look closer at that!
> >
> > Thanks for closely looking into this patchset, Xiubo!
> >
> > Kind regards,
> > Alex
> >
> >>
> >> Thanks
> >>
> >> - Xiubo
> >>
> >>> and before open request will go to the server we will
> >>> check that current user is allowed to open this file (and during
> >>> this check UID/GID of a current user and UID/GID of the file owner
> >>> will be properly compared). I.e. this issue is only relevant for the
> >>> case
> >>> when we have additional permission checks on the network file system
> >>> server side.
> >>>
> >>>> IMO the idmapping should be client-side feature and we should make it
> >>>> consistent by using the unmapped fs{g,u}id always here.
> >>> To make the current user fs{g,u}id always idmapped we need to make
> >>> really big changes in the VFS layer. And it's not obvious
> >>> that it justifies the cost. Because this particular feature with
> >>> Cephfs idmapped mounts is already used/tested with LXD/LXC workloads
> >>> and it works perfectly well. And as far as I know, LXD/LXC were the
> >>> first idmapped mount adopters. IMHO, it's better to
> >>> start from this approach and if someone will want to extend this
> >>> functionality for network filesystems and want to map fs{g,u}id which
> >>> are sent over the
> >>> wire we will take a look at that. Because anyway, integration with
> >>> Cephfs is important for the LXD project and we are looking closely at
> >>> this.
> >>>
> >>> Kind regards,
> >>> Alex
> >>>
> >>>> Thanks
> >>>>
> >>>> - Xiubo
> >>>>
> >>>>>         head->ino = cpu_to_le64(req->r_deleg_ino);
> >>>>>         head->args = req->r_args;
> >>>>>
>

Re: [PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Christian Brauner]

On Wed, May 24, 2023 at 05:33:03PM +0200, Alexander Mikhalitsyn wrote:
> These helpers are required to support idmapped mounts in the Cephfs.
> 
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---

It's fine by me to export them. The explicit contract is that _nothing
and absolutely nothing_ outside of core VFS code can directly peak into
struct mnt_idmap internals. That's the only invariant we care about.o 

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > of passing down the mount's idmapping.
> >
> > Cc: Jeff Layton <jlayton@kernel.org>
> > Cc: Ilya Dryomov <idryomov@gmail.com>
> > Cc: ceph-devel@vger.kernel.org
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> >   fs/ceph/inode.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index 37e1cbfc7c89..f1f934439be0 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> >
> >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> >
> > +     /*
> > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're
> > +      * sending over the wire. The mount idmapping only matters when we
> > +      * create new filesystem objects based on the caller's mapped
> > +      * fs{g,u}id.
> > +      */
> > +     req->r_mnt_idmap = &nop_mnt_idmap;
>
> For example with an idmapping 1000:0 and in the /mnt/idmapped_ceph/.
>
> This means the "__ceph_setattr()" will always use UID 0 to set the
> caller_uid, right ? If it is then the client auth checking for the

Yes, if you have a mapping like b:1000:0:1 (the last number is a
length of a mapping). It means even more,
the only user from which you can create something on the filesystem
will be UID = 0,
because all other UIDs/GIDs are not mapped and you'll instantly get
-EOVERFLOW from the kernel.

> setattr requests in cephfs MDS will succeed, since the UID 0 is root.
> But if you use a different idmapping, such as 1000:2000, it will fail.

If you have a mapping b:1000:2000:1 then the only valid UID/GID from
which you can create something
on an idmapped mount will be UID/GID = 2000:2000 (and this will be
mapped to 1000:1000 and sent over the wire,
because we performing an idmapping procedure for requests those are
creating inodes).
So, even root with UID = 0 will not be able to create a file on such a
mount and get -EOVERFLOW.

>
> So here IMO we should set it to 'idmap' too ?

Good question. I can't see any obvious issue with setting an actual
idmapping here.
It will be interesting to know Christian's opinion about this.

Kind regards,
Alex

>
> Thanks
>
> - Xiubo
>
> >       if (ia_valid & ATTR_UID) {
> >               dout("setattr %p uid %d -> %d\n", inode,
> >                    from_kuid(&init_user_ns, inode->i_uid),
> > @@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> >       if (ceph_inode_is_shutdown(inode))
> >               return -ESTALE;
> >
> > -     err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> > +     err = setattr_prepare(idmap, dentry, attr);
> >       if (err != 0)
> >               return err;
> >
> > @@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> >       err = __ceph_setattr(inode, attr);
> >
> >       if (err >= 0 && (attr->ia_valid & ATTR_MODE))
> > -             err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
> > +             err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
> >
> >       return err;
> >   }
>

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Christian Brauner]

On Fri, Jun 02, 2023 at 02:45:30PM +0200, Aleksandr Mikhalitsyn wrote:
> On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
> >
> >
> > On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > >
> > > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > > of passing down the mount's idmapping.
> > >
> > > Cc: Jeff Layton <jlayton@kernel.org>
> > > Cc: Ilya Dryomov <idryomov@gmail.com>
> > > Cc: ceph-devel@vger.kernel.org
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > ---
> > >   fs/ceph/inode.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 37e1cbfc7c89..f1f934439be0 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > >
> > >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> > >
> > > +     /*
> > > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're

This is now obsolete... In earlier imlementations attr->ia_{g,u}id was
used and contained the filesystem wide value, not the idmapped mount
value.

However, this was misleading and we changed that in commit b27c82e12965
("attr: port attribute changes to new types") and introduced dedicated
new types into struct iattr->ia_vfs{g,u}id. So the you need to use
attr->ia_vfs{g,u}id as documented in include/linux/fs.h and you need to
transform them into filesystem wide values and then to raw values you
send over the wire.

Alex should be able to figure this out though.

> > > +      * sending over the wire. The mount idmapping only matters when we
> > > +      * create new filesystem objects based on the caller's mapped
> > > +      * fs{g,u}id.
> > > +      */
> > > +     req->r_mnt_idmap = &nop_mnt_idmap;
> >
> > For example with an idmapping 1000:0 and in the /mnt/idmapped_ceph/.
> >
> > This means the "__ceph_setattr()" will always use UID 0 to set the
> > caller_uid, right ? If it is then the client auth checking for the
> 
> Yes, if you have a mapping like b:1000:0:1 (the last number is a
> length of a mapping). It means even more,
> the only user from which you can create something on the filesystem
> will be UID = 0,
> because all other UIDs/GIDs are not mapped and you'll instantly get
> -EOVERFLOW from the kernel.
> 
> > setattr requests in cephfs MDS will succeed, since the UID 0 is root.
> > But if you use a different idmapping, such as 1000:2000, it will fail.
> 
> If you have a mapping b:1000:2000:1 then the only valid UID/GID from
> which you can create something
> on an idmapped mount will be UID/GID = 2000:2000 (and this will be
> mapped to 1000:1000 and sent over the wire,
> because we performing an idmapping procedure for requests those are
> creating inodes).
> So, even root with UID = 0 will not be able to create a file on such a
> mount and get -EOVERFLOW.
> 
> >
> > So here IMO we should set it to 'idmap' too ?
> 
> Good question. I can't see any obvious issue with setting an actual
> idmapping here.
> It will be interesting to know Christian's opinion about this.
> 
> Kind regards,
> Alex
> 
> >
> > Thanks
> >
> > - Xiubo
> >
> > >       if (ia_valid & ATTR_UID) {
> > >               dout("setattr %p uid %d -> %d\n", inode,
> > >                    from_kuid(&init_user_ns, inode->i_uid),
> > > @@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > >       if (ceph_inode_is_shutdown(inode))
> > >               return -ESTALE;
> > >
> > > -     err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> > > +     err = setattr_prepare(idmap, dentry, attr);
> > >       if (err != 0)
> > >               return err;
> > >
> > > @@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > >       err = __ceph_setattr(inode, attr);
> > >
> > >       if (err >= 0 && (attr->ia_valid & ATTR_MODE))
> > > -             err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
> > > +             err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
> > >
> > >       return err;
> > >   }
> >

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Jun 02, 2023 at 02:45:30PM +0200, Aleksandr Mikhalitsyn wrote:
> > On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
> > >
> > >
> > > On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > >
> > > > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > > > of passing down the mount's idmapping.
> > > >
> > > > Cc: Jeff Layton <jlayton@kernel.org>
> > > > Cc: Ilya Dryomov <idryomov@gmail.com>
> > > > Cc: ceph-devel@vger.kernel.org
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > ---
> > > >   fs/ceph/inode.c | 11 +++++++++--
> > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > index 37e1cbfc7c89..f1f934439be0 100644
> > > > --- a/fs/ceph/inode.c
> > > > +++ b/fs/ceph/inode.c
> > > > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > > >
> > > >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> > > >
> > > > +     /*
> > > > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're
>
> This is now obsolete... In earlier imlementations attr->ia_{g,u}id was
> used and contained the filesystem wide value, not the idmapped mount
> value.
>
> However, this was misleading and we changed that in commit b27c82e12965
> ("attr: port attribute changes to new types") and introduced dedicated
> new types into struct iattr->ia_vfs{g,u}id. So the you need to use
> attr->ia_vfs{g,u}id as documented in include/linux/fs.h and you need to
> transform them into filesystem wide values and then to raw values you
> send over the wire.
>
> Alex should be able to figure this out though.

Hi Christian,

Thanks for pointing this out. Unfortunately I wasn't able to notice
that. I'll take a look closer and fix that.

>
> > > > +      * sending over the wire. The mount idmapping only matters when we
> > > > +      * create new filesystem objects based on the caller's mapped
> > > > +      * fs{g,u}id.
> > > > +      */
> > > > +     req->r_mnt_idmap = &nop_mnt_idmap;
> > >
> > > For example with an idmapping 1000:0 and in the /mnt/idmapped_ceph/.
> > >
> > > This means the "__ceph_setattr()" will always use UID 0 to set the
> > > caller_uid, right ? If it is then the client auth checking for the
> >
> > Yes, if you have a mapping like b:1000:0:1 (the last number is a
> > length of a mapping). It means even more,
> > the only user from which you can create something on the filesystem
> > will be UID = 0,
> > because all other UIDs/GIDs are not mapped and you'll instantly get
> > -EOVERFLOW from the kernel.
> >
> > > setattr requests in cephfs MDS will succeed, since the UID 0 is root.
> > > But if you use a different idmapping, such as 1000:2000, it will fail.
> >
> > If you have a mapping b:1000:2000:1 then the only valid UID/GID from
> > which you can create something
> > on an idmapped mount will be UID/GID = 2000:2000 (and this will be
> > mapped to 1000:1000 and sent over the wire,
> > because we performing an idmapping procedure for requests those are
> > creating inodes).
> > So, even root with UID = 0 will not be able to create a file on such a
> > mount and get -EOVERFLOW.
> >
> > >
> > > So here IMO we should set it to 'idmap' too ?
> >
> > Good question. I can't see any obvious issue with setting an actual
> > idmapping here.
> > It will be interesting to know Christian's opinion about this.

^

Kind regards,
Alex

> >
> > Kind regards,
> > Alex
> >
> > >
> > > Thanks
> > >
> > > - Xiubo
> > >
> > > >       if (ia_valid & ATTR_UID) {
> > > >               dout("setattr %p uid %d -> %d\n", inode,
> > > >                    from_kuid(&init_user_ns, inode->i_uid),
> > > > @@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > > >       if (ceph_inode_is_shutdown(inode))
> > > >               return -ESTALE;
> > > >
> > > > -     err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> > > > +     err = setattr_prepare(idmap, dentry, attr);
> > > >       if (err != 0)
> > > >               return err;
> > > >
> > > > @@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > > >       err = __ceph_setattr(inode, attr);
> > > >
> > > >       if (err >= 0 && (attr->ia_valid & ATTR_MODE))
> > > > -             err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
> > > > +             err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
> > > >
> > > >       return err;
> > > >   }
> > >

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Christian Brauner]

On Fri, Jun 02, 2023 at 03:05:50PM +0200, Aleksandr Mikhalitsyn wrote:
> On Fri, Jun 2, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Fri, Jun 02, 2023 at 02:45:30PM +0200, Aleksandr Mikhalitsyn wrote:
> > > On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
> > > >
> > > >
> > > > On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > >
> > > > > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > > > > of passing down the mount's idmapping.
> > > > >
> > > > > Cc: Jeff Layton <jlayton@kernel.org>
> > > > > Cc: Ilya Dryomov <idryomov@gmail.com>
> > > > > Cc: ceph-devel@vger.kernel.org
> > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > > ---
> > > > >   fs/ceph/inode.c | 11 +++++++++--
> > > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > index 37e1cbfc7c89..f1f934439be0 100644
> > > > > --- a/fs/ceph/inode.c
> > > > > +++ b/fs/ceph/inode.c
> > > > > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > > > >
> > > > >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> > > > >
> > > > > +     /*
> > > > > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're
> >
> > This is now obsolete... In earlier imlementations attr->ia_{g,u}id was
> > used and contained the filesystem wide value, not the idmapped mount
> > value.
> >
> > However, this was misleading and we changed that in commit b27c82e12965
> > ("attr: port attribute changes to new types") and introduced dedicated
> > new types into struct iattr->ia_vfs{g,u}id. So the you need to use
> > attr->ia_vfs{g,u}id as documented in include/linux/fs.h and you need to
> > transform them into filesystem wide values and then to raw values you
> > send over the wire.
> >
> > Alex should be able to figure this out though.
> 
> Hi Christian,
> 
> Thanks for pointing this out. Unfortunately I wasn't able to notice
> that. I'll take a look closer and fix that.

Just to clarify: I wasn't trying to imply that you should've figured
this out on your own. I was just trying to say that you should be able
figure out the exact details how to implement this in ceph after I told
you about the attr->ia_vfs{g,u}id change.

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 3:08 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Jun 02, 2023 at 03:05:50PM +0200, Aleksandr Mikhalitsyn wrote:
> > On Fri, Jun 2, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Fri, Jun 02, 2023 at 02:45:30PM +0200, Aleksandr Mikhalitsyn wrote:
> > > > On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
> > > > >
> > > > >
> > > > > On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > > > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > >
> > > > > > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > > > > > of passing down the mount's idmapping.
> > > > > >
> > > > > > Cc: Jeff Layton <jlayton@kernel.org>
> > > > > > Cc: Ilya Dryomov <idryomov@gmail.com>
> > > > > > Cc: ceph-devel@vger.kernel.org
> > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > > > ---
> > > > > >   fs/ceph/inode.c | 11 +++++++++--
> > > > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > > index 37e1cbfc7c89..f1f934439be0 100644
> > > > > > --- a/fs/ceph/inode.c
> > > > > > +++ b/fs/ceph/inode.c
> > > > > > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > > > > >
> > > > > >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> > > > > >
> > > > > > +     /*
> > > > > > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're
> > >
> > > This is now obsolete... In earlier imlementations attr->ia_{g,u}id was
> > > used and contained the filesystem wide value, not the idmapped mount
> > > value.
> > >
> > > However, this was misleading and we changed that in commit b27c82e12965
> > > ("attr: port attribute changes to new types") and introduced dedicated
> > > new types into struct iattr->ia_vfs{g,u}id. So the you need to use
> > > attr->ia_vfs{g,u}id as documented in include/linux/fs.h and you need to
> > > transform them into filesystem wide values and then to raw values you
> > > send over the wire.
> > >
> > > Alex should be able to figure this out though.
> >
> > Hi Christian,
> >
> > Thanks for pointing this out. Unfortunately I wasn't able to notice
> > that. I'll take a look closer and fix that.
>
> Just to clarify: I wasn't trying to imply that you should've figured
> this out on your own. I was just trying to say that you should be able
> figure out the exact details how to implement this in ceph after I told
> you about the attr->ia_vfs{g,u}id change.

No problem, I've got your idea the same as you explained it ;-)
I'll rework that place and I will recheck that we pass xfstests after that.

Re: [PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Christoph Hellwig]

On Fri, Jun 02, 2023 at 02:40:27PM +0200, Christian Brauner wrote:
> On Wed, May 24, 2023 at 05:33:03PM +0200, Alexander Mikhalitsyn wrote:
> > These helpers are required to support idmapped mounts in the Cephfs.
> > 
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> 
> It's fine by me to export them. The explicit contract is that _nothing
> and absolutely nothing_ outside of core VFS code can directly peak into
> struct mnt_idmap internals. That's the only invariant we care about.o 

It would be good if we could keep all these somewhat internal exports
as EXPORT_SYMBOL_GPL, though.

Re: [PATCH v2 01/13] fs: export mnt_idmap_get/mnt_idmap_put

[Aleksandr Mikhalitsyn]

On Mon, Jun 5, 2023 at 3:53 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jun 02, 2023 at 02:40:27PM +0200, Christian Brauner wrote:
> > On Wed, May 24, 2023 at 05:33:03PM +0200, Alexander Mikhalitsyn wrote:
> > > These helpers are required to support idmapped mounts in the Cephfs.
> > >
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > ---
> >
> > It's fine by me to export them. The explicit contract is that _nothing
> > and absolutely nothing_ outside of core VFS code can directly peak into
> > struct mnt_idmap internals. That's the only invariant we care about.o
>
> It would be good if we could keep all these somewhat internal exports
> as EXPORT_SYMBOL_GPL, though.

Dear Christoph,

Well noticed! Thanks, I will do it.

Kind regards,
Alex

>

Re: [PATCH v2 00/13] ceph: support idmapped mounts

[Aleksandr Mikhalitsyn]

version 3 was sent
https://lore.kernel.org/lkml/20230607152038.469739-1-aleksandr.mikhalitsyn@canonical.com/

On Wed, May 24, 2023 at 5:33 PM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> Dear friends,
>
> This patchset was originally developed by Christian Brauner but I'll continue
> to push it forward. Christian allowed me to do that :)
>
> This feature is already actively used/tested with LXD/LXC project.
>
> v2 is just a rebased version of the original series with some small field naming change.
>
> Git tree (based on https://github.com/ceph/ceph-client.git master):
> https://github.com/mihalicyn/linux/tree/fs.idmapped.ceph.v2
>
> Original description from Christian:
> ========================================================================
> This patch series enables cephfs to support idmapped mounts, i.e. the
> ability to alter ownership information on a per-mount basis.
>
> Container managers such as LXD support sharaing data via cephfs between
> the host and unprivileged containers and between unprivileged containers.
> They may all use different idmappings. Idmapped mounts can be used to
> create mounts with the idmapping used for the container (or a different
> one specific to the use-case).
>
> There are in fact more use-cases such as remapping ownership for
> mountpoints on the host itself to grant or restrict access to different
> users or to make it possible to enforce that programs running as root
> will write with a non-zero {g,u}id to disk.
>
> The patch series is simple overall and few changes are needed to cephfs.
> There is one cephfs specific issue that I would like to discuss and
> solve which I explain in detail in:
>
> [PATCH 02/12] ceph: handle idmapped mounts in create_request_message()
>
> It has to do with how to handle mds serves which have id-based access
> restrictions configured. I would ask you to please take a look at the
> explanation in the aforementioned patch.
>
> The patch series passes the vfs and idmapped mount testsuite as part of
> xfstests. To run it you will need a config like:
>
> [ceph]
> export FSTYP=ceph
> export TEST_DIR=/mnt/test
> export TEST_DEV=10.103.182.10:6789:/
> export TEST_FS_MOUNT_OPTS="-o name=admin,secret=$password
>
> and then simply call
>
> sudo ./check -g idmapped
>
> ========================================================================
>
> Alexander Mikhalitsyn (1):
>   fs: export mnt_idmap_get/mnt_idmap_put
>
> Christian Brauner (12):
>   ceph: stash idmapping in mdsc request
>   ceph: handle idmapped mounts in create_request_message()
>   ceph: allow idmapped mknod inode op
>   ceph: allow idmapped symlink inode op
>   ceph: allow idmapped mkdir inode op
>   ceph: allow idmapped rename inode op
>   ceph: allow idmapped getattr inode op
>   ceph: allow idmapped permission inode op
>   ceph: allow idmapped setattr inode op
>   ceph/acl: allow idmapped set_acl inode op
>   ceph/file: allow idmapped atomic_open inode op
>   ceph: allow idmapped mounts
>
>  fs/ceph/acl.c                 |  2 +-
>  fs/ceph/dir.c                 |  4 ++++
>  fs/ceph/file.c                | 10 ++++++++--
>  fs/ceph/inode.c               | 15 +++++++++++----
>  fs/ceph/mds_client.c          | 29 +++++++++++++++++++++++++----
>  fs/ceph/mds_client.h          |  1 +
>  fs/ceph/super.c               |  2 +-
>  fs/mnt_idmapping.c            |  2 ++
>  include/linux/mnt_idmapping.h |  3 +++
>  9 files changed, 56 insertions(+), 12 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v2 10/13] ceph: allow idmapped setattr inode op

[Aleksandr Mikhalitsyn]

On Fri, Jun 2, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Jun 02, 2023 at 02:45:30PM +0200, Aleksandr Mikhalitsyn wrote:
> > On Fri, Jun 2, 2023 at 3:30 AM Xiubo Li <xiubli@redhat.com> wrote:
> > >
> > >
> > > On 5/24/23 23:33, Alexander Mikhalitsyn wrote:
> > > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > >
> > > > Enable __ceph_setattr() to handle idmapped mounts. This is just a matter
> > > > of passing down the mount's idmapping.
> > > >
> > > > Cc: Jeff Layton <jlayton@kernel.org>
> > > > Cc: Ilya Dryomov <idryomov@gmail.com>
> > > > Cc: ceph-devel@vger.kernel.org
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > ---
> > > >   fs/ceph/inode.c | 11 +++++++++--
> > > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > index 37e1cbfc7c89..f1f934439be0 100644
> > > > --- a/fs/ceph/inode.c
> > > > +++ b/fs/ceph/inode.c
> > > > @@ -2050,6 +2050,13 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > > >
> > > >       dout("setattr %p issued %s\n", inode, ceph_cap_string(issued));
> > > >
> > > > +     /*
> > > > +      * The attr->ia_{g,u}id members contain the target {g,u}id we're
>
> This is now obsolete... In earlier imlementations attr->ia_{g,u}id was
> used and contained the filesystem wide value, not the idmapped mount
> value.
>
> However, this was misleading and we changed that in commit b27c82e12965
> ("attr: port attribute changes to new types") and introduced dedicated
> new types into struct iattr->ia_vfs{g,u}id. So the you need to use
> attr->ia_vfs{g,u}id as documented in include/linux/fs.h and you need to
> transform them into filesystem wide values and then to raw values you
> send over the wire.
>
> Alex should be able to figure this out though.

Hi Christian,

Reworked in v3:
https://lore.kernel.org/lkml/20230607152038.469739-12-aleksandr.mikhalitsyn@canonical.com/#t

The only thing is that I've forgotten to remove the comment, but I'll
do that in v4.

Kind regards,
Alex

>
> > > > +      * sending over the wire. The mount idmapping only matters when we
> > > > +      * create new filesystem objects based on the caller's mapped
> > > > +      * fs{g,u}id.
> > > > +      */
> > > > +     req->r_mnt_idmap = &nop_mnt_idmap;
> > >
> > > For example with an idmapping 1000:0 and in the /mnt/idmapped_ceph/.
> > >
> > > This means the "__ceph_setattr()" will always use UID 0 to set the
> > > caller_uid, right ? If it is then the client auth checking for the
> >
> > Yes, if you have a mapping like b:1000:0:1 (the last number is a
> > length of a mapping). It means even more,
> > the only user from which you can create something on the filesystem
> > will be UID = 0,
> > because all other UIDs/GIDs are not mapped and you'll instantly get
> > -EOVERFLOW from the kernel.
> >
> > > setattr requests in cephfs MDS will succeed, since the UID 0 is root.
> > > But if you use a different idmapping, such as 1000:2000, it will fail.
> >
> > If you have a mapping b:1000:2000:1 then the only valid UID/GID from
> > which you can create something
> > on an idmapped mount will be UID/GID = 2000:2000 (and this will be
> > mapped to 1000:1000 and sent over the wire,
> > because we performing an idmapping procedure for requests those are
> > creating inodes).
> > So, even root with UID = 0 will not be able to create a file on such a
> > mount and get -EOVERFLOW.
> >
> > >
> > > So here IMO we should set it to 'idmap' too ?
> >
> > Good question. I can't see any obvious issue with setting an actual
> > idmapping here.
> > It will be interesting to know Christian's opinion about this.
> >
> > Kind regards,
> > Alex
> >
> > >
> > > Thanks
> > >
> > > - Xiubo
> > >
> > > >       if (ia_valid & ATTR_UID) {
> > > >               dout("setattr %p uid %d -> %d\n", inode,
> > > >                    from_kuid(&init_user_ns, inode->i_uid),
> > > > @@ -2240,7 +2247,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > > >       if (ceph_inode_is_shutdown(inode))
> > > >               return -ESTALE;
> > > >
> > > > -     err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> > > > +     err = setattr_prepare(idmap, dentry, attr);
> > > >       if (err != 0)
> > > >               return err;
> > > >
> > > > @@ -2255,7 +2262,7 @@ int ceph_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
> > > >       err = __ceph_setattr(inode, attr);
> > > >
> > > >       if (err >= 0 && (attr->ia_valid & ATTR_MODE))
> > > > -             err = posix_acl_chmod(&nop_mnt_idmap, dentry, attr->ia_mode);
> > > > +             err = posix_acl_chmod(idmap, dentry, attr->ia_mode);
> > > >
> > > >       return err;
> > > >   }
> > >