Re: [PATCH v9 03/12] ceph: handle idmapped mounts in create_request_message()

From: Aleksandr Mikhalitsyn
Date: Mon Aug 07 2023 - 07:46:48 EST

Next message: Manivannan Sadhasivam: "Re: [PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups"
Previous message: max.chou: "[PATCH v4] Bluetooth: btrtl: Load FW v2 otherwise FW v1 for RTL8852C"
In reply to: Aleksandr Mikhalitsyn: "Re: [PATCH v9 03/12] ceph: handle idmapped mounts in create_request_message()"
Next in thread: Alexander Mikhalitsyn: "[PATCH v9 04/12] ceph: add enable_unsafe_idmap module parameter"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Aug 7, 2023 at 12:28 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
>
> On 8/7/23 14:51, Aleksandr Mikhalitsyn wrote:
> > On Mon, Aug 7, 2023 at 3:25 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
> >>
> >> On 8/4/23 16:48, Alexander Mikhalitsyn wrote:
> >>> From: Christian Brauner <brauner@xxxxxxxxxx>
> >>>
> >>> 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.
> >>>
> >>> 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.
> >>>
> >>> This change uses a new cephfs protocol extension CEPHFS_FEATURE_HAS_OWNER_UIDGID
> >>> which adds two new fields (owner_{u,g}id) to the request head structure.
> >>> So, we need to ensure that MDS supports it otherwise we need to fail
> >>> any IO that comes through an idmapped mount because we can't process it
> >>> in a proper way. MDS server without such an extension will use caller_{u,g}id
> >>> fields to set a new inode owner UID/GID which is incorrect because caller_{u,g}id
> >>> values are unmapped. At the same time we can't map these fields with an
> >>> idmapping as it can break UID/GID-based permission checks logic on the
> >>> MDS side. This problem was described with a lot of details at [1], [2].
> >>>
> >>> [1] https://lore.kernel.org/lkml/CAEivzxfw1fHO2TFA4dx3u23ZKK6Q+EThfzuibrhA3RKM=ZOYLg@xxxxxxxxxxxxxx/
> >>> [2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@xxxxxxxxxx/
> >>>
> >>> Link: https://github.com/ceph/ceph/pull/52575
> >>> Link: https://tracker.ceph.com/issues/62217
> >>> Cc: Xiubo Li <xiubli@xxxxxxxxxx>
> >>> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
> >>> Cc: Ilya Dryomov <idryomov@xxxxxxxxx>
> >>> Cc: ceph-devel@xxxxxxxxxxxxxxx
> >>> Co-Developed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
> >>> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> >>> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
> >>> ---
> >>> v7:
> >>> - reworked to use two new fields for owner UID/GID (https://github.com/ceph/ceph/pull/52575)
> >>> v8:
> >>> - properly handled case when old MDS used with new kernel client
> >>> ---
> >>> fs/ceph/mds_client.c | 47 +++++++++++++++++++++++++++++++++---
> >>> fs/ceph/mds_client.h | 5 +++-
> >>> include/linux/ceph/ceph_fs.h | 5 +++-
> >>> 3 files changed, 52 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> >>> index 8829f55103da..41e4bf3811c4 100644
> >>> --- a/fs/ceph/mds_client.c
> >>> +++ b/fs/ceph/mds_client.c
> >>> @@ -2902,6 +2902,17 @@ static void encode_mclientrequest_tail(void **p, const struct ceph_mds_request *
> >>> }
> >>> }
> >>>
> >>> +static inline u16 mds_supported_head_version(struct ceph_mds_session *session)
> >>> +{
> >>> + if (!test_bit(CEPHFS_FEATURE_32BITS_RETRY_FWD, &session->s_features))
> >>> + return 1;
> >>> +
> >>> + if (!test_bit(CEPHFS_FEATURE_HAS_OWNER_UIDGID, &session->s_features))
> >>> + return 2;
> >>> +
> >>> + return CEPH_MDS_REQUEST_HEAD_VERSION;
> >>> +}
> >>> +
> >>> static struct ceph_mds_request_head_legacy *
> >>> find_legacy_request_head(void *p, u64 features)
> >>> {
> >>> @@ -2923,6 +2934,7 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>> {
> >>> int mds = session->s_mds;
> >>> struct ceph_mds_client *mdsc = session->s_mdsc;
> >>> + struct ceph_client *cl = mdsc->fsc->client;
> >>> struct ceph_msg *msg;
> >>> struct ceph_mds_request_head_legacy *lhead;
> >>> const char *path1 = NULL;
> >>> @@ -2936,7 +2948,7 @@ 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);
> >>> - bool old_version = !test_bit(CEPHFS_FEATURE_32BITS_RETRY_FWD, &session->s_features);
> >>> + u16 request_head_version = mds_supported_head_version(session);
> >>>
> >>> ret = set_request_path_attr(mdsc, req->r_inode, req->r_dentry,
> >>> req->r_parent, req->r_path1, req->r_ino1.ino,
> >>> @@ -2977,8 +2989,10 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>> */
> >>> if (legacy)
> >>> len = sizeof(struct ceph_mds_request_head_legacy);
> >>> - else if (old_version)
> >>> + else if (request_head_version == 1)
> >>> len = sizeof(struct ceph_mds_request_head_old);
> >>> + else if (request_head_version == 2)
> >>> + len = offsetofend(struct ceph_mds_request_head, ext_num_fwd);
> >>> else
> >>> len = sizeof(struct ceph_mds_request_head);
> >>>
> >>> @@ -3028,6 +3042,16 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>> lhead = find_legacy_request_head(msg->front.iov_base,
> >>> session->s_con.peer_features);
> >>>
> >>> + if ((req->r_mnt_idmap != &nop_mnt_idmap) &&
> >>> + !test_bit(CEPHFS_FEATURE_HAS_OWNER_UIDGID, &session->s_features)) {
> >>> + pr_err_ratelimited_client(cl,
> >>> + "idmapped mount is used and CEPHFS_FEATURE_HAS_OWNER_UIDGID"
> >>> + " is not supported by MDS. Fail request with -EIO.\n");
> >>> +
> >>> + ret = -EIO;
> >>> + goto out_err;
> >>> + }
> >>> +
> >>> /*
> >>> * The ceph_mds_request_head_legacy didn't contain a version field, and
> >>> * one was added when we moved the message version from 3->4.
> >>> @@ -3035,17 +3059,34 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
> >>> if (legacy) {
> >>> msg->hdr.version = cpu_to_le16(3);
> >>> p = msg->front.iov_base + sizeof(*lhead);
> >>> - } else if (old_version) {
> >>> + } else if (request_head_version == 1) {
> >>> struct ceph_mds_request_head_old *ohead = msg->front.iov_base;
> >>>
> >>> msg->hdr.version = cpu_to_le16(4);
> >>> ohead->version = cpu_to_le16(1);
> >>> p = msg->front.iov_base + sizeof(*ohead);
> >>> + } else if (request_head_version == 2) {
> >>> + struct ceph_mds_request_head *nhead = msg->front.iov_base;
> >>> +
> >>> + msg->hdr.version = cpu_to_le16(6);
> >>> + nhead->version = cpu_to_le16(2);
> >>> +
> >>> + p = msg->front.iov_base + offsetofend(struct ceph_mds_request_head, ext_num_fwd);
> >>> } else {
> >>> struct ceph_mds_request_head *nhead = msg->front.iov_base;
> >>> + kuid_t owner_fsuid;
> >>> + kgid_t owner_fsgid;
> >>>
> >>> msg->hdr.version = cpu_to_le16(6);
> >>> nhead->version = cpu_to_le16(CEPH_MDS_REQUEST_HEAD_VERSION);
> >>> + nhead->struct_len = sizeof(struct ceph_mds_request_head);
> >>> +
> >>> + owner_fsuid = from_vfsuid(req->r_mnt_idmap, &init_user_ns,
> >>> + VFSUIDT_INIT(req->r_cred->fsuid));
> >>> + owner_fsgid = from_vfsgid(req->r_mnt_idmap, &init_user_ns,
> >>> + VFSGIDT_INIT(req->r_cred->fsgid));
> >>> + nhead->owner_uid = cpu_to_le32(from_kuid(&init_user_ns, owner_fsuid));
> >>> + nhead->owner_gid = cpu_to_le32(from_kgid(&init_user_ns, owner_fsgid));
> >>> p = msg->front.iov_base + sizeof(*nhead);
> >>> }
> >>>
> >>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> >>> index e3bbf3ba8ee8..8f683e8203bd 100644
> >>> --- a/fs/ceph/mds_client.h
> >>> +++ b/fs/ceph/mds_client.h
> >>> @@ -33,8 +33,10 @@ enum ceph_feature_type {
> >>> CEPHFS_FEATURE_NOTIFY_SESSION_STATE,
> >>> CEPHFS_FEATURE_OP_GETVXATTR,
> >>> CEPHFS_FEATURE_32BITS_RETRY_FWD,
> >>> + CEPHFS_FEATURE_NEW_SNAPREALM_INFO,
> >>> + CEPHFS_FEATURE_HAS_OWNER_UIDGID,
> >>>
> >>> - CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_32BITS_RETRY_FWD,
> >>> + CEPHFS_FEATURE_MAX = CEPHFS_FEATURE_HAS_OWNER_UIDGID,
> >>> };
> >>>
> >>> #define CEPHFS_FEATURES_CLIENT_SUPPORTED { \
> >>> @@ -49,6 +51,7 @@ enum ceph_feature_type {
> >>> CEPHFS_FEATURE_NOTIFY_SESSION_STATE, \
> >>> CEPHFS_FEATURE_OP_GETVXATTR, \
> >>> CEPHFS_FEATURE_32BITS_RETRY_FWD, \
> >>> + CEPHFS_FEATURE_HAS_OWNER_UIDGID, \
> >>> }
> >>>
> >>> /*
> >>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
> >>> index 5f2301ee88bc..b91699b08f26 100644
> >>> --- a/include/linux/ceph/ceph_fs.h
> >>> +++ b/include/linux/ceph/ceph_fs.h
> >>> @@ -499,7 +499,7 @@ struct ceph_mds_request_head_legacy {
> >>> union ceph_mds_request_args args;
> >>> } __attribute__ ((packed));
> >>>
> >>> -#define CEPH_MDS_REQUEST_HEAD_VERSION 2
> >>> +#define CEPH_MDS_REQUEST_HEAD_VERSION 3
> >>>
> >>> struct ceph_mds_request_head_old {
> >>> __le16 version; /* struct version */
> >>> @@ -530,6 +530,9 @@ struct ceph_mds_request_head {
> >>>
> >>> __le32 ext_num_retry; /* new count retry attempts */
> >>> __le32 ext_num_fwd; /* new count fwd attempts */
> >>> +
> >>> + __le32 struct_len; /* to store size of struct ceph_mds_request_head */
> >>> + __le32 owner_uid, owner_gid; /* used for OPs which create inodes */
> >> Let's also initialize them to -1 for all the other requests as we do in
> >> your PR.
> > They are always initialized already. As you can see from the code we
> > don't have any extra conditions
> > on filling these fields. We always fill them with an appropriate
> > UID/GID. If mount is not idmapped then it's just == caller_uid/gid,
> > if mount idmapped then it's idmapped caller_uid/gid.
>
> Then in kclient all the request will always initialized the
> 'owner_{uid/gid}' with 'caller_{uid/gid}'. While in userspace libcephfs
> it will only set them for 'create/mknod/mkdir/symlink` instead.
>
> I'd prefer to make them consistent, which is what I am still focusing
> on, to make them easier to read and comparing when troubles hooting.

Have fixed:
https://github.com/mihalicyn/linux/commit/5a5b590ca5aa9ec81d68ff60d77ea54fc86bf33a

Also have added appropriate checks in mkdir/atomic_open:
https://github.com/mihalicyn/linux/commit/bc1fa68f7143a58af8c181bbfab64edf0397dca5
https://github.com/mihalicyn/linux/commit/30e21387063710a10cdca15a5d840fcf8e1e6ccd

Will send v10 soon.

Kind regards,
Alex

>
> Thanks
>
> - Xiubo
>
> > Kind regards,
> > Alex
> >
> >> Thanks
> >>
> >> - Xiubo
> >>
> >>
> >>
> >>> } __attribute__ ((packed));
> >>>
> >>> /* cap/lease release record */
>

Next message: Manivannan Sadhasivam: "Re: [PATCH 00/10] mtd: rawnand: qcom: Bunch of fixes and cleanups"
Previous message: max.chou: "[PATCH v4] Bluetooth: btrtl: Load FW v2 otherwise FW v1 for RTL8852C"
In reply to: Aleksandr Mikhalitsyn: "Re: [PATCH v9 03/12] ceph: handle idmapped mounts in create_request_message()"
Next in thread: Alexander Mikhalitsyn: "[PATCH v9 04/12] ceph: add enable_unsafe_idmap module parameter"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]