| Message ID | 20230803135955.230449-4-aleksandr.mikhalitsyn@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: support idmapped mounts | expand |
On 8/3/23 21:59, Alexander Mikhalitsyn wrote: > From: Christian Brauner <brauner@kernel.org> > > 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@mail.gmail.com/ > [2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ > > https://github.com/ceph/ceph/pull/52575 > https://tracker.ceph.com/issues/62217 > > Cc: Xiubo Li <xiubli@redhat.com> > Cc: Jeff Layton <jlayton@kernel.org> > Cc: Ilya Dryomov <idryomov@gmail.com> > Cc: ceph-devel@vger.kernel.org > Co-Developed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > --- > 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 | 46 +++++++++++++++++++++++++++++++++--- > fs/ceph/mds_client.h | 5 +++- > include/linux/ceph/ceph_fs.h | 4 +++- > 3 files changed, 50 insertions(+), 5 deletions(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 8829f55103da..7d3106d3b726 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); > This is not what we suppose to. If we do this again and again when adding new members it will make the code very complicated to maintain. Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is not supported the decoder should skip it directly. Is the MDS side buggy ? Why you last version didn't work ? Thanks - Xiubo > @@ -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,33 @@ 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); > + > + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { > > __le32 ext_num_retry; /* new count retry attempts */ > __le32 ext_num_fwd; /* new count fwd attempts */ > + > + __le32 owner_uid, owner_gid; /* used for OPs which create inodes */ > } __attribute__ ((packed)); > > /* cap/lease release record */
On 8/4/23 10:26, Xiubo Li wrote: > > On 8/3/23 21:59, Alexander Mikhalitsyn wrote: >> From: Christian Brauner <brauner@kernel.org> >> >> 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@mail.gmail.com/ >> [2] >> https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ >> >> https://github.com/ceph/ceph/pull/52575 >> https://tracker.ceph.com/issues/62217 >> >> Cc: Xiubo Li <xiubli@redhat.com> >> Cc: Jeff Layton <jlayton@kernel.org> >> Cc: Ilya Dryomov <idryomov@gmail.com> >> Cc: ceph-devel@vger.kernel.org >> Co-Developed-by: Alexander Mikhalitsyn >> <aleksandr.mikhalitsyn@canonical.com> >> Signed-off-by: Christian Brauner <brauner@kernel.org> >> Signed-off-by: Alexander Mikhalitsyn >> <aleksandr.mikhalitsyn@canonical.com> >> --- >> 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 | 46 +++++++++++++++++++++++++++++++++--- >> fs/ceph/mds_client.h | 5 +++- >> include/linux/ceph/ceph_fs.h | 4 +++- >> 3 files changed, 50 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> index 8829f55103da..7d3106d3b726 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); > > This is not what we suppose to. If we do this again and again when > adding new members it will make the code very complicated to maintain. > > Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph > should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is > not supported the decoder should skip it directly. > > Is the MDS side buggy ? Why you last version didn't work ? > I think the ceph side is buggy. Possibly we should add one new `length` member in struct `struct ceph_mds_request_head` and just skip the extra bytes when decoding it. Could you fix it together with your ceph PR ? Thanks - Xiubo > Thanks > > - Xiubo > >> @@ -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,33 @@ 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); >> + >> + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { >> __le32 ext_num_retry; /* new count retry attempts */ >> __le32 ext_num_fwd; /* new count fwd attempts */ >> + >> + __le32 owner_uid, owner_gid; /* used for OPs which create >> inodes */ >> } __attribute__ ((packed)); >> /* cap/lease release record */
On Fri, Aug 4, 2023 at 4:26 AM Xiubo Li <xiubli@redhat.com> wrote: > > > On 8/3/23 21:59, Alexander Mikhalitsyn wrote: > > From: Christian Brauner <brauner@kernel.org> > > > > 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@mail.gmail.com/ > > [2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ > > > > https://github.com/ceph/ceph/pull/52575 > > https://tracker.ceph.com/issues/62217 > > > > Cc: Xiubo Li <xiubli@redhat.com> > > Cc: Jeff Layton <jlayton@kernel.org> > > Cc: Ilya Dryomov <idryomov@gmail.com> > > Cc: ceph-devel@vger.kernel.org > > Co-Developed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > > --- > > 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 | 46 +++++++++++++++++++++++++++++++++--- > > fs/ceph/mds_client.h | 5 +++- > > include/linux/ceph/ceph_fs.h | 4 +++- > > 3 files changed, 50 insertions(+), 5 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index 8829f55103da..7d3106d3b726 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); > > > > This is not what we suppose to. If we do this again and again when > adding new members it will make the code very complicated to maintain. > > Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph > should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is not > supported the decoder should skip it directly. I thought that too. But it doesn't work. Just try - take kernel client testing branch, and then add a new field to the struct ceph_mds_request_head. Compile and try to mount. It will stop to work and on the MDS side you will see something like: 2023-08-03T13:15:40.871+0200 7fe64ef5e640 10 mds.c ms_handle_accept v1:192.168.2.136:0/49354629 con 0x563962206880 session 0x563967054000 2023-08-03T13:15:40.871+0200 7fe650f62640 -1 failed to decode message of type 24 v6: End of buffer [buffer:2] 2023-08-03T13:15:40.871+0200 7fe650f62640 1 dump: 00000000 03 00 01 00 00 00 00 00 00 00 10 00 00 00 00 00 |................| 00000010 00 00 00 00 00 00 01 01 00 00 00 00 00 00 00 00 |................| 00000020 00 00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 |................| 00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| * 00000070 00 00 01 01 00 00 00 00 00 00 00 00 00 00 00 01 |................| 00000080 00 00 00 00 00 00 00 00 00 00 00 00 5b 8c cb 64 |............[..d| 00000090 64 78 11 13 01 00 00 00 00 00 00 00 00 00 00 00 |dx..............| 000000a0 00 00 00 00 00 00 00 00 00 00 00 00 |............| 000000ac As I understand, the MDS side is not ready to see struct ceph_mds_request_head bigger in size than supported. > > Is the MDS side buggy ? Why you last version didn't work ? > > Thanks > > - Xiubo > > > @@ -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,33 @@ 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); > > + > > + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { > > > > __le32 ext_num_retry; /* new count retry attempts */ > > __le32 ext_num_fwd; /* new count fwd attempts */ > > + > > + __le32 owner_uid, owner_gid; /* used for OPs which create inodes */ > > } __attribute__ ((packed)); > > > > /* cap/lease release record */ >
On Fri, Aug 4, 2023 at 5:24 AM Xiubo Li <xiubli@redhat.com> wrote: > > > On 8/4/23 10:26, Xiubo Li wrote: > > > > On 8/3/23 21:59, Alexander Mikhalitsyn wrote: > >> From: Christian Brauner <brauner@kernel.org> > >> > >> 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@mail.gmail.com/ > >> [2] > >> https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ > >> > >> https://github.com/ceph/ceph/pull/52575 > >> https://tracker.ceph.com/issues/62217 > >> > >> Cc: Xiubo Li <xiubli@redhat.com> > >> Cc: Jeff Layton <jlayton@kernel.org> > >> Cc: Ilya Dryomov <idryomov@gmail.com> > >> Cc: ceph-devel@vger.kernel.org > >> Co-Developed-by: Alexander Mikhalitsyn > >> <aleksandr.mikhalitsyn@canonical.com> > >> Signed-off-by: Christian Brauner <brauner@kernel.org> > >> Signed-off-by: Alexander Mikhalitsyn > >> <aleksandr.mikhalitsyn@canonical.com> > >> --- > >> 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 | 46 +++++++++++++++++++++++++++++++++--- > >> fs/ceph/mds_client.h | 5 +++- > >> include/linux/ceph/ceph_fs.h | 4 +++- > >> 3 files changed, 50 insertions(+), 5 deletions(-) > >> > >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > >> index 8829f55103da..7d3106d3b726 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); > > > > This is not what we suppose to. If we do this again and again when > > adding new members it will make the code very complicated to maintain. > > > > Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph > > should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is > > not supported the decoder should skip it directly. > > > > Is the MDS side buggy ? Why you last version didn't work ? > > > > I think the ceph side is buggy. Possibly we should add one new `length` > member in struct `struct ceph_mds_request_head` and just skip the extra > bytes when decoding it. Hm, I think I found something suspicious. In cephfs code we have many places that call the DECODE_FINISH macro, but in our decoder we don't have it. From documentation it follows that DECODE_FINISH purpose is precisely about this problem. What do you think? > > Could you fix it together with your ceph PR ? > > Thanks > > - Xiubo > > > > Thanks > > > > - Xiubo > > > >> @@ -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,33 @@ 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); > >> + > >> + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { > >> __le32 ext_num_retry; /* new count retry attempts */ > >> __le32 ext_num_fwd; /* new count fwd attempts */ > >> + > >> + __le32 owner_uid, owner_gid; /* used for OPs which create > >> inodes */ > >> } __attribute__ ((packed)); > >> /* cap/lease release record */ >
On Fri, Aug 4, 2023 at 8:35 AM Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote: > > On Fri, Aug 4, 2023 at 5:24 AM Xiubo Li <xiubli@redhat.com> wrote: > > > > > > On 8/4/23 10:26, Xiubo Li wrote: > > > > > > On 8/3/23 21:59, Alexander Mikhalitsyn wrote: > > >> From: Christian Brauner <brauner@kernel.org> > > >> > > >> 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@mail.gmail.com/ > > >> [2] > > >> https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ > > >> > > >> https://github.com/ceph/ceph/pull/52575 > > >> https://tracker.ceph.com/issues/62217 > > >> > > >> Cc: Xiubo Li <xiubli@redhat.com> > > >> Cc: Jeff Layton <jlayton@kernel.org> > > >> Cc: Ilya Dryomov <idryomov@gmail.com> > > >> Cc: ceph-devel@vger.kernel.org > > >> Co-Developed-by: Alexander Mikhalitsyn > > >> <aleksandr.mikhalitsyn@canonical.com> > > >> Signed-off-by: Christian Brauner <brauner@kernel.org> > > >> Signed-off-by: Alexander Mikhalitsyn > > >> <aleksandr.mikhalitsyn@canonical.com> > > >> --- > > >> 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 | 46 +++++++++++++++++++++++++++++++++--- > > >> fs/ceph/mds_client.h | 5 +++- > > >> include/linux/ceph/ceph_fs.h | 4 +++- > > >> 3 files changed, 50 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > >> index 8829f55103da..7d3106d3b726 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); > > > > > > This is not what we suppose to. If we do this again and again when > > > adding new members it will make the code very complicated to maintain. > > > > > > Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph > > > should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is > > > not supported the decoder should skip it directly. > > > > > > Is the MDS side buggy ? Why you last version didn't work ? > > > > > > > I think the ceph side is buggy. Possibly we should add one new `length` > > member in struct `struct ceph_mds_request_head` and just skip the extra > > bytes when decoding it. > > Hm, I think I found something suspicious. In cephfs code we have many > places that > call the DECODE_FINISH macro, but in our decoder we don't have it. > > From documentation it follows that DECODE_FINISH purpose is precisely > about this problem. > > What do you think? Upd: this thing also changes on-wire format and adds field to store length. But this will be a massive and incompatible protocol change. I don't think that we want to do this in the scope of this task. > > > > > Could you fix it together with your ceph PR ? > > > > Thanks > > > > - Xiubo > > > > > > > Thanks > > > > > > - Xiubo > > > > > >> @@ -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,33 @@ 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); > > >> + > > >> + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { > > >> __le32 ext_num_retry; /* new count retry attempts */ > > >> __le32 ext_num_fwd; /* new count fwd attempts */ > > >> + > > >> + __le32 owner_uid, owner_gid; /* used for OPs which create > > >> inodes */ > > >> } __attribute__ ((packed)); > > >> /* cap/lease release record */ > >
On Fri, Aug 4, 2023 at 8:43 AM Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote: > > On Fri, Aug 4, 2023 at 8:35 AM Aleksandr Mikhalitsyn > <aleksandr.mikhalitsyn@canonical.com> wrote: > > > > On Fri, Aug 4, 2023 at 5:24 AM Xiubo Li <xiubli@redhat.com> wrote: > > > > > > > > > On 8/4/23 10:26, Xiubo Li wrote: > > > > > > > > On 8/3/23 21:59, Alexander Mikhalitsyn wrote: > > > >> From: Christian Brauner <brauner@kernel.org> > > > >> > > > >> 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@mail.gmail.com/ > > > >> [2] > > > >> https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ > > > >> > > > >> https://github.com/ceph/ceph/pull/52575 > > > >> https://tracker.ceph.com/issues/62217 > > > >> > > > >> Cc: Xiubo Li <xiubli@redhat.com> > > > >> Cc: Jeff Layton <jlayton@kernel.org> > > > >> Cc: Ilya Dryomov <idryomov@gmail.com> > > > >> Cc: ceph-devel@vger.kernel.org > > > >> Co-Developed-by: Alexander Mikhalitsyn > > > >> <aleksandr.mikhalitsyn@canonical.com> > > > >> Signed-off-by: Christian Brauner <brauner@kernel.org> > > > >> Signed-off-by: Alexander Mikhalitsyn > > > >> <aleksandr.mikhalitsyn@canonical.com> > > > >> --- > > > >> 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 | 46 +++++++++++++++++++++++++++++++++--- > > > >> fs/ceph/mds_client.h | 5 +++- > > > >> include/linux/ceph/ceph_fs.h | 4 +++- > > > >> 3 files changed, 50 insertions(+), 5 deletions(-) > > > >> > > > >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > >> index 8829f55103da..7d3106d3b726 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); > > > > > > > > This is not what we suppose to. If we do this again and again when > > > > adding new members it will make the code very complicated to maintain. > > > > > > > > Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph > > > > should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is > > > > not supported the decoder should skip it directly. > > > > > > > > Is the MDS side buggy ? Why you last version didn't work ? > > > > > > > > > > I think the ceph side is buggy. Possibly we should add one new `length` > > > member in struct `struct ceph_mds_request_head` and just skip the extra > > > bytes when decoding it. > > > > Hm, I think I found something suspicious. In cephfs code we have many > > places that > > call the DECODE_FINISH macro, but in our decoder we don't have it. > > > > From documentation it follows that DECODE_FINISH purpose is precisely > > about this problem. > > > > What do you think? > > Upd: this thing also changes on-wire format and adds field to store length. > But this will be a massive and incompatible protocol change. I don't think that > we want to do this in the scope of this task. https://github.com/ceph/ceph/pull/52575#issuecomment-1665141641 > > > > > > > > > Could you fix it together with your ceph PR ? > > > > > > Thanks > > > > > > - Xiubo > > > > > > > > > > Thanks > > > > > > > > - Xiubo > > > > > > > >> @@ -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,33 @@ 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); > > > >> + > > > >> + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { > > > >> __le32 ext_num_retry; /* new count retry attempts */ > > > >> __le32 ext_num_fwd; /* new count fwd attempts */ > > > >> + > > > >> + __le32 owner_uid, owner_gid; /* used for OPs which create > > > >> inodes */ > > > >> } __attribute__ ((packed)); > > > >> /* cap/lease release record */ > > >
On 8/4/23 14:35, Aleksandr Mikhalitsyn wrote: > On Fri, Aug 4, 2023 at 5:24 AM Xiubo Li <xiubli@redhat.com> wrote: >> >> On 8/4/23 10:26, Xiubo Li wrote: >>> On 8/3/23 21:59, Alexander Mikhalitsyn wrote: >>>> From: Christian Brauner <brauner@kernel.org> >>>> >>>> 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@mail.gmail.com/ >>>> [2] >>>> https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/ >>>> >>>> https://github.com/ceph/ceph/pull/52575 >>>> https://tracker.ceph.com/issues/62217 >>>> >>>> Cc: Xiubo Li <xiubli@redhat.com> >>>> Cc: Jeff Layton <jlayton@kernel.org> >>>> Cc: Ilya Dryomov <idryomov@gmail.com> >>>> Cc: ceph-devel@vger.kernel.org >>>> Co-Developed-by: Alexander Mikhalitsyn >>>> <aleksandr.mikhalitsyn@canonical.com> >>>> Signed-off-by: Christian Brauner <brauner@kernel.org> >>>> Signed-off-by: Alexander Mikhalitsyn >>>> <aleksandr.mikhalitsyn@canonical.com> >>>> --- >>>> 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 | 46 +++++++++++++++++++++++++++++++++--- >>>> fs/ceph/mds_client.h | 5 +++- >>>> include/linux/ceph/ceph_fs.h | 4 +++- >>>> 3 files changed, 50 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >>>> index 8829f55103da..7d3106d3b726 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); >>> This is not what we suppose to. If we do this again and again when >>> adding new members it will make the code very complicated to maintain. >>> >>> Once the CEPHFS_FEATURE_32BITS_RETRY_FWD has been supported the ceph >>> should correctly decode it and if CEPHFS_FEATURE_HAS_OWNER_UIDGID is >>> not supported the decoder should skip it directly. >>> >>> Is the MDS side buggy ? Why you last version didn't work ? >>> >> I think the ceph side is buggy. Possibly we should add one new `length` >> member in struct `struct ceph_mds_request_head` and just skip the extra >> bytes when decoding it. > Hm, I think I found something suspicious. In cephfs code we have many > places that > call the DECODE_FINISH macro, but in our decoder we don't have it. > > From documentation it follows that DECODE_FINISH purpose is precisely > about this problem. > > What do you think? Yeah, correct. We also need to do it like this. Thanks - Xiubo >> Could you fix it together with your ceph PR ? >> >> Thanks >> >> - Xiubo >> >> >>> Thanks >>> >>> - Xiubo >>> >>>> @@ -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,33 @@ 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); >>>> + >>>> + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { >>>> __le32 ext_num_retry; /* new count retry attempts */ >>>> __le32 ext_num_fwd; /* new count fwd attempts */ >>>> + >>>> + __le32 owner_uid, owner_gid; /* used for OPs which create >>>> inodes */ >>>> } __attribute__ ((packed)); >>>> /* cap/lease release record */
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 8829f55103da..7d3106d3b726 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,33 @@ 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); + + 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..6eb83a51341c 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,8 @@ struct ceph_mds_request_head { __le32 ext_num_retry; /* new count retry attempts */ __le32 ext_num_fwd; /* new count fwd attempts */ + + __le32 owner_uid, owner_gid; /* used for OPs which create inodes */ } __attribute__ ((packed)); /* cap/lease release record */