quincy: ceph_fs.h: add separate owner_{u,g}id fields#53139
Merged
yuriw merged 1 commit intoceph:quincyfrom Oct 4, 2023
Merged
Conversation
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_RENAME,
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 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/
Fixes: https://tracker.ceph.com/issues/62217
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
(cherry picked from commit 46cb244)
Contributor
Author
|
jenkins retest this please |
1 similar comment
Contributor
Author
|
jenkins retest this please |
Contributor
Author
|
Failure looks unrelated: |
Contributor
Author
|
jenkins retest this please |
Contributor
Author
mchangir
approved these changes
Oct 3, 2023
Contributor
|
Reviewed-by: Milind Changire mchangir@redhat.com |
Contributor
|
@yuriw @mihalicyn @lxbsz @mchangir - fails smoke test - https://pulpito.ceph.com/yuriw-2023-10-19_16:09:31-smoke-quincy-release-distro-default-smithi/7432399/ |
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_RENAME, CEPH_MDS_OP_MKDIR, CEPH_MDS_OP_SYMLINK.
Before we have used caller_{u,g}id fields for two purposes:
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 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/
Fixes: https://tracker.ceph.com/issues/62217
Signed-off-by: Alexander Mikhalitsyn aleksandr.mikhalitsyn@canonical.com
(cherry picked from commit 46cb244)
backport tracker https://tracker.ceph.com/issues/62571
backport of #52575
parent tracker https://tracker.ceph.com/issues/62217
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows