On Mon, Aug 7, 2023 at 1:21 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote: > > > On 8/7/23 18:34, Aleksandr Mikhalitsyn wrote: > > 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. > > Dear Xiubo, > > > > Sure, I will do it. > > > > Couldn't you please review all the rest of the patches before I fix > > this particular thing? > > It will allow me to fix and send -v10 with all required fixes > > incorporated in it. > > I have gone through them all and they LGTM. Thanks! Kind regards, Alex > > Thanks > > - Xiubo > > > > Kind regards, > > Alex > > > >> Thanks > >> > >> - Xiubo > >> > >>> Kind regards, > >>> Alex > >>> > >>>> Thanks > >>>> > >>>> - Xiubo > >>>> > >>>> > >>>> > >>>>> } __attribute__ ((packed)); > >>>>> > >>>>> /* cap/lease release record */ >