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

[Aleksandr Mikhalitsyn]

On Fri, Aug 4, 2023 at 5:24 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
>
> On 8/4/23 10:26, Xiubo Li wrote:
> >
> > On 8/3/23 21:59, 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/
> >>
> >> https://github.com/ceph/ceph/pull/52575
> >> 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         | 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 */
>