ceph_fs.h: add separate owner_{u,g}id fields by mihalicyn · Pull Request #52575 · ceph/ceph

mihalicyn · 2023-07-21T15:37:24Z

This patch adds separate fields to pass inode owner's UID/GID for operations which create new inodes:
CEPH_MDS_OP_CREATE, CEPH_MDS_OP_MKNOD, CEPH_MDS_OP_MKDIR, CEPH_MDS_OP_SYMLINK.

Before we have used caller_{u,g}id fields for two purposes:

for MDS permission checks
to set UID/GID for new inodes

but during a long discussion around adding idmapped mounts support to cephfs kernel client we decided to extend wire protocol [1].

Originally, Christian Brauner pointed to this problem [2], but a few initial versions of "ceph: support idmapped mounts" patchset were based on a compromise between bringing idmapped mounts support and changing on-wire data structures. The only problem with this approach is that if we have UID/GID-based permission checks on the MDS side they won't work as intended. Xiubo Li pointed to this issue and said that it's critical enough to not being ignored.

This patch does not change behavior for client or MDS, because in the current client implementation we do:

req->set_inode_owner_uid_gid(perms.uid(), perms.gid());

so, these new fields contain the same value as caller_{u,g}id.

This new extension is used in a non-trivial way in the Linux kernel client:
https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph

In addition to new fields we also add CEPHFS_FEATURE_HAS_OWNER_UIDGID feature bit. It can be used on the client-side
to check if MDS supports these new fields and can handle them properly.

Fixes: https://tracker.ceph.com/issues/62217

[1] https://lore.kernel.org/lkml/CAEivzxcipdTQ9-b2zk-7-AMDfwPk2Brtp=X6H8xXsHMEtQJKFQ@mail.gmail.com/
[2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/

cc @lxbsz

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

mihalicyn · 2023-07-21T15:41:21Z

jenkins test make check

src/messages/MClientRequest.h

src/include/ceph_fs.h

src/messages/MClientRequest.h

src/mds/Server.cc

lxbsz · 2023-07-24T07:47:52Z

@mihalicyn Could you raise on ceph tracker and add the detail examples, which as you did in the kernel mail list, there ? This could help use to understand it easier ?

mihalicyn · 2023-07-24T07:48:42Z

@mihalicyn Could you raise on ceph tracker and add the detail examples, which as you did in the kernel mail list, there ? This could help use to understand it easier ?

Sure. Will do!

Upd: registered an account on ceph tracker. Waiting for account approval.

vshankar · 2023-07-24T07:51:25Z

This new extension will be used in a non-trivial way in the Linux kernel client.

[1] https://lore.kernel.org/lkml/CAEivzxcipdTQ9-b2zk-7-AMDfwPk2Brtp=X6H8xXsHMEtQJKFQ@mail.gmail.com/ [2] https://lore.kernel.org/all/20220104140414.155198-3-brauner@kernel.org/

Thanks for the pointer @mihalicyn - it helps to speed up review since I wasn't following the discussion closely on the kernel list.

mihalicyn · 2023-07-24T12:55:11Z

Revision 2.

addressed all review comments/suggestions
rebased to the latest main
added CEPHFS_FEATURE_HAS_OWNER_UIDGID it will be useful in kernel client to detect old MDS servers if needed

mihalicyn · 2023-07-24T14:00:44Z

I've started reworking kernel client code (https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph) to support this protocol change and noticed that kernel client still not supports feature from #45669

Xiubo, do you have any plans to support it? I can try to support it by myself if you don't mind by using inspiration from cbd7e30 :)

As far as I understand we have to support CEPHFS_FEATURE_32BITS_RETRY_FWD feature before CEPHFS_FEATURE_HAS_OWNER_UIDGID. Please correct me if I'm wrong.

lxbsz · 2023-07-24T14:14:48Z

I've started reworking kernel client code (https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph) to support this protocol change and noticed that kernel client still not supports feature from #45669

Xiubo, do you have any plans to support it? I can try to support it by myself if you don't mind by using inspiration from cbd7e30 :)

Locally I have finished coding this two months ago, forgot to send it out after that. Thanks for reminding.

Let me finish the testing tomorrow and send it out. Then you can just focus on the owner_{uid, gid} issue in this PR.

As far as I understand we have to support CEPHFS_FEATURE_32BITS_RETRY_FWD feature before CEPHFS_FEATURE_HAS_OWNER_UIDGID. Please correct me if I'm wrong.

Yeah, correct.

src/client/MetaRequest.h

src/include/ceph_fs.h

mihalicyn · 2023-07-25T07:34:03Z

Offtop:

is it okay that ../src/vstart.sh -d -n -l fails to work with this:

/home/alex/storage/dev/ceph/build/bin/ceph -c /home/alex/storage/dev/ceph/build/ceph.conf -k /home/alex/storage/dev/ceph/build/keyring fs volume ls 
no valid command found; 10 closest matches:
fs snapshot mirror enable [<fs_name>]
fs snapshot mirror disable [<fs_name>]
fs snapshot mirror peer_add <fs_name> [<remote_cluster_spec>] [<remote_fs_name>] [<remote_mon_host>] [<cephx_key>]
fs snapshot mirror peer_list [<fs_name>]
fs snapshot mirror peer_remove <fs_name> [<peer_uuid>]
fs snapshot mirror peer_bootstrap create <fs_name> <client_name> [<site_name>]
fs snapshot mirror peer_bootstrap import <fs_name> [<token>]
fs snapshot mirror add <fs_name> [<path>]
fs snapshot mirror remove <fs_name> [<path>]
fs snapshot mirror dirmap <fs_name> [<path>]
Error EINVAL: invalid command

As far as I understand this command fails (from vstart.sh source):

	    # wait for volume module to load
	    while ! ceph_adm fs volume ls ; do sleep 1 ; done # <<<<<<<< HERE
            local fs=0
            for name in a b c d e f g h i j k l m n o p
            do
                ceph_adm fs volume create ${name}
                ceph_adm fs authorize ${name} "client.fs_${name}" / rwp >> "$keyring_fn"
                fs=$(($fs + 1))
                [ $fs -eq $CEPH_NUM_FS ] && break
            done
        fi

ceph_adm syntax was changed recently, but vstart.sh wasn't updated?

Upd: solved.
Analogical case https://tracker.ceph.com/issues/41251

cd build
cat out/*.log | grep ModuleNot

and install all python packages with pip.

lxbsz · 2023-07-25T09:10:00Z

Offtop:

is it okay that ../src/vstart.sh -d -n -l fails to work with this:

/home/alex/storage/dev/ceph/build/bin/ceph -c /home/alex/storage/dev/ceph/build/ceph.conf -k /home/alex/storage/dev/ceph/build/keyring fs volume ls 
no valid command found; 10 closest matches:
fs snapshot mirror enable [<fs_name>]
fs snapshot mirror disable [<fs_name>]
fs snapshot mirror peer_add <fs_name> [<remote_cluster_spec>] [<remote_fs_name>] [<remote_mon_host>] [<cephx_key>]
fs snapshot mirror peer_list [<fs_name>]
fs snapshot mirror peer_remove <fs_name> [<peer_uuid>]
fs snapshot mirror peer_bootstrap create <fs_name> <client_name> [<site_name>]
fs snapshot mirror peer_bootstrap import <fs_name> [<token>]
fs snapshot mirror add <fs_name> [<path>]
fs snapshot mirror remove <fs_name> [<path>]
fs snapshot mirror dirmap <fs_name> [<path>]
Error EINVAL: invalid command

As far as I understand this command fails (from vstart.sh source):

	    # wait for volume module to load
	    while ! ceph_adm fs volume ls ; do sleep 1 ; done # <<<<<<<< HERE
            local fs=0
            for name in a b c d e f g h i j k l m n o p
            do
                ceph_adm fs volume create ${name}
                ceph_adm fs authorize ${name} "client.fs_${name}" / rwp >> "$keyring_fn"
                fs=$(($fs + 1))
                [ $fs -eq $CEPH_NUM_FS ] && break
            done
        fi

ceph_adm syntax was changed recently, but vstart.sh wasn't updated?

Upd: solved. Analogical case https://tracker.ceph.com/issues/41251

cd build
cat out/*.log | grep ModuleNot

and install all python packages with pip.

You should run the ./install-deps.sh script to install the dependents.

The is what I usually use to set the cluster locally:
MDS=3 MON=3 OSD=3 MGR=1 ../src/vstart.sh -n -X -G --msgr1

mihalicyn · 2023-07-25T09:12:40Z

You should run the ./install-deps.sh script to install the dependents.

yes, that's what I did at first, but it looks like it doesn't install enough pip packages (at least on my Ubuntu 22.04), so pip install cherrypy prettytable scipy pecan did the trick.

The is what I usually use to set the cluster locally:
MDS=3 MON=3 OSD=3 MGR=1 ../src/vstart.sh -n -X -G --msgr1

Thanks! Will do the same for testing ;-)

mihalicyn · 2023-07-26T12:20:06Z

Revision 3.

addressed all review comments/suggestions
prepared and tested kernel client changes (https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph) with xfstests

I'll send kernel client patches to LKML once this PR will be merged or upon request from Xiubo.

I'm still waiting for my account at tracker.ceph.com to be approved by administrator.

lxbsz · 2023-07-26T13:25:08Z

Revision 3.

addressed all review comments/suggestions

prepared and tested kernel client changes (https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph) with xfstests

I'll send kernel client patches to LKML once this PR will be merged or upon request from Xiubo.

I'm still waiting for my account at tracker.ceph.com to be approved by administrator.

@mihalicyn Please go on to code and send the kclient patches first, we need to go through both ceph and kernel changes before we merge this PR to make sure we won't miss anything important for the idmapping feature.

Please code the kclient patches basing on the testing branch, I will apply my patch [1] to it.

[1] https://patchwork.kernel.org/project/ceph-devel/patch/20230725104438.386461-1-xiubli@redhat.com/

vshankar · 2023-07-26T13:45:56Z

Revision 2.

addressed all review comments/suggestions

rebased to the latest main

added CEPHFS_FEATURE_HAS_OWNER_UIDGID it will be useful in kernel client to detect old MDS servers if needed

Could you elaborate a bit more on this? Seeing the client side changes, its the client side that's setting the owner_{g,u}id in the request and the MDS knows how to either decode from the bufferlist or set the owner_{g,u}id from the caller_{g,u}id for older clients.

(maybe I missed a discussion somewhere)

mihalicyn · 2023-07-26T14:14:10Z

Revision 3.

addressed all review comments/suggestions

prepared and tested kernel client changes (https://github.com/mihalicyn/linux/commits/fs.idmapped.ceph) with xfstests

I'll send kernel client patches to LKML once this PR will be merged or upon request from Xiubo.
I'm still waiting for my account at tracker.ceph.com to be approved by administrator.

@mihalicyn Please go on to code and send the kclient patches first, we need to go through both ceph and kernel changes before we merge this PR to make sure we won't miss anything important for the idmapping feature.

Please code the kclient patches basing on the testing branch, I will apply my patch [1] to it.

[1] https://patchwork.kernel.org/project/ceph-devel/patch/20230725104438.386461-1-xiubli@redhat.com/

Have sent.

https://lore.kernel.org/all/20230726141026.307690-1-aleksandr.mikhalitsyn@canonical.com/#t

Thanks!

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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: Christian Brauner <brauner@kernel.org>

* refs/pull/52575/head: ceph_fs.h: add separate owner_{u,g}id fields Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> Reviewed-by: Xiubo Li <xiubli@redhat.com>

Inode operations that create a new filesystem object such as ->mknod, ->create, ->mkdir() and others don't take a {g,u}id argument explicitly. Instead the caller's fs{g,u}id is used for the {g,u}id of the new filesystem object. 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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: Christian Brauner <brauner@kernel.org>

vshankar · 2023-08-23T13:28:17Z

https://pulpito.ceph.com/vshankar-2023-08-23_03:59:53-fs-wip-vshankar-testing-20230822.060131-testing-default-smithi/

(will review when the run finishes)

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/ Link: ceph/ceph#52575 Link: 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> Acked-by: Christian Brauner <brauner@kernel.org> [ simplified version of patch for v6.4.10 ]

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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: Christian Brauner <brauner@kernel.org>

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#24-Aug-2023

mihalicyn · 2023-08-24T12:19:41Z

Thanks a lot for your help with this @vshankar!

mihalicyn · 2023-08-24T12:21:07Z

Speaking about backports. Do I need to prepare PRs for older releases by myself? Or this stuff is handled automatically somehow?

lxbsz · 2023-08-24T12:25:06Z

Speaking about backports. Do I need to prepare PRs for older releases by myself? Or this stuff is handled automatically somehow?

You need to backport it yourself one by one.

mihalicyn · 2023-08-24T12:26:38Z

Speaking about backports. Do I need to prepare PRs for older releases by myself? Or this stuff is handled automatically somehow?

You need to backport it yourself one by one.

Thanks, Xiubo! Will do.

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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: 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/ Link: ceph/ceph#52575 Link: 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> Acked-by: Christian Brauner <brauner@kernel.org> [ simplified version of patch for v6.4.10 ]

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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: 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/ Link: ceph/ceph#52575 Link: 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> Acked-by: Christian Brauner <brauner@kernel.org> [ simplified version of patch for v6.4.10 ]

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/ Link: ceph/ceph#52575 Link: 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.comReviewed-by: Xiubo Li <xiubli@redhat.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Acked-by: Christian Brauner <brauner@kernel.org>

github-actions bot added the cephfs Ceph File System label Jul 21, 2023

lxbsz requested a review from a team July 24, 2023 01:02

lxbsz requested changes Jul 24, 2023

View reviewed changes

vshankar requested a review from a team July 24, 2023 07:07

mihalicyn mentioned this pull request Jul 24, 2023

ceph-fuse: add option to map local user/group IDs to remote IDs #45667

Closed

14 tasks

mihalicyn force-pushed the prep_for_idmapped_mounts branch 2 times, most recently from 188bce8 to fb8717a Compare July 24, 2023 12:51

mihalicyn force-pushed the prep_for_idmapped_mounts branch 3 times, most recently from 47ef7c1 to e85f20b Compare July 24, 2023 13:16

mihalicyn requested review from lxbsz and vshankar July 24, 2023 14:22

lxbsz reviewed Jul 25, 2023

View reviewed changes

src/client/MetaRequest.h Outdated Show resolved Hide resolved

src/include/ceph_fs.h Show resolved Hide resolved

mihalicyn force-pushed the prep_for_idmapped_mounts branch 2 times, most recently from 159b0f2 to 5c6c236 Compare July 26, 2023 11:27

mihalicyn requested a review from lxbsz July 26, 2023 12:21

vshankar approved these changes Aug 24, 2023

View reviewed changes

vshankar merged commit 6639465 into ceph:main Aug 24, 2023

vshankar removed the wip-vshankar-testing2 label Aug 24, 2023

lxbsz mentioned this pull request Aug 24, 2023

mds/client: check the cephx mds auth access in client side #48027

Merged

14 tasks

This was referenced Aug 24, 2023

pacific: ceph_fs.h: add separate owner_{u,g}id fields #53137

Closed

quincy: ceph_fs.h: add separate owner_{u,g}id fields #53139

Merged

reef: ceph_fs.h: add separate owner_{u,g}id fields #53138

Merged

Conversation

mihalicyn commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mihalicyn commented Jul 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lxbsz commented Jul 24, 2023

Uh oh!

mihalicyn commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Jul 24, 2023

Uh oh!

mihalicyn commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihalicyn commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxbsz commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

mihalicyn commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxbsz commented Jul 25, 2023

Uh oh!

mihalicyn commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihalicyn commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxbsz commented Jul 26, 2023

Uh oh!

vshankar commented Jul 26, 2023

Uh oh!

mihalicyn commented Jul 26, 2023

Uh oh!

vshankar commented Aug 23, 2023

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

mihalicyn commented Aug 24, 2023

Uh oh!

mihalicyn commented Aug 24, 2023

Uh oh!

lxbsz commented Aug 24, 2023

Uh oh!

mihalicyn commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mihalicyn commented Jul 21, 2023 •

edited

Loading

mihalicyn commented Jul 24, 2023 •

edited

Loading

mihalicyn commented Jul 24, 2023 •

edited

Loading

mihalicyn commented Jul 24, 2023 •

edited

Loading

mihalicyn commented Jul 25, 2023 •

edited

Loading

mihalicyn commented Jul 25, 2023 •

edited

Loading

mihalicyn commented Jul 26, 2023 •

edited

Loading