From: Xiubo Li <xiubli@redhat.com>
To: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Cc: Gregory Farnum <gfarnum@redhat.com>,
Christian Brauner <brauner@kernel.org>,
stgraber@ubuntu.com, linux-fsdevel@vger.kernel.org,
Ilya Dryomov <idryomov@gmail.com>,
Jeff Layton <jlayton@kernel.org>,
ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 00/14] ceph: support idmapped mounts
Date: Thu, 20 Jul 2023 14:36:17 +0800 [thread overview]
Message-ID: <3af4f092-8de7-d217-cd2d-d39dfc625edd@redhat.com> (raw)
In-Reply-To: <CAEivzxfw1fHO2TFA4dx3u23ZKK6Q+EThfzuibrhA3RKM=ZOYLg@mail.gmail.com>
On 7/19/23 19:57, Aleksandr Mikhalitsyn wrote:
> On Tue, Jul 18, 2023 at 4:49 PM Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
>> On Tue, Jul 18, 2023 at 3:45 AM Xiubo Li <xiubli@redhat.com> wrote:
[...]
>> No, the idea is to stop mapping a caller_{uid, gid}. And to add a new
>> fields like
>> inode_owner_{uid, gid} which will be idmapped (this field will be specific only
>> for those operations that create a new inode).
> I've decided to write some summary of different approaches and
> elaborate tricky places.
>
> Current implementation.
>
> We have head->caller_{uid,gid} fields mapped in according
> to the mount's idmapping. But as we don't have information about
> mount's idmapping in all call stacks (like ->lookup case), we
> are not able to map it always and they are left untouched in these cases.
>
> This tends to an inconsistency between different inode_operations,
> for example ->lookup (don't have an access to an idmapping) and
> ->mkdir (have an idmapping as an argument).
>
> This inconsistency is absolutely harmless if the user does not
> use UID/GID-based restrictions. Because in this use case head->caller_{uid,gid}
> fields used *only* to set inode owner UID/GID during the inode_operations
> which create inodes.
>
> Conclusion 1. head->caller_{uid,gid} fields have two meanings
> - UID/GID-based permission checks
> - inode owner information
>
> Solution 0. Ignore the issue with UID/GID-based restrictions and idmapped mounts
> until we are not blamed by users ;-)
>
> As far as I can see you are not happy about this way. :-)
>
> Solution 1. Let's add mount's idmapping argument to all inode_operations
> and always map head->caller_{uid,gid} fields.
>
> Not a best idea, because:
> - big modification of VFS layer
> - ideologically incorrect, for instance ->lookup should not care
> and know *anything* about mount's idmapping, because ->lookup works
> not on the mount level (it's not important who and through which mount
> triggered the ->lookup). Imagine that you've dentry cache filled and call
> open(...) in this case ->lookup can be uncalled. But if the user was not lucky
> enough to have cache filled then open(..) will trigger the lookup and
> then ->lookup results will be dependent on the mount's idmapping. It
> seems incorrect
> and unobvious consequence of introducing such a parameter to ->lookup operation.
> To summarize, ->lookup is about filling dentry cache and dentry cache
> is superblock-level
> thing, not mount-level.
>
> Solution 2. Add some kind of extra checks to ceph-client and ceph
> server to detect that
> mount idmappings used with UID/GID-based restrictions and restrict such mounts.
>
> Seems not ideal to me too. Because it's not a fix, it's a limitation
> and this limitation is
> not cheap from the implementation perspective (we need heavy changes
> in ceph server side and
> client side too). Btw, currently VFS API is also not ready for that,
> because we can't
> decide if idmapped mounts are allowed or not in runtime. It's a static
> thing that should be declared
> with FS_ALLOW_IDMAP flag in (struct file_system_type)->fs_flags. Not a
> big deal, but...
>
> Solution 3. Add a new UID/GID fields to ceph request structure in
> addition to head->caller_{uid,gid}
> to store information about inode owners (only for inode_operations
> which create inodes).
>
> How does it solves the problem?
> With these new fields we can leave head->caller_{uid,gid} untouched
> with an idmapped mounts code.
> It means that UID/GID-based restrictions will continue to work as intended.
>
> At the same time, new fields (let say "inode_owner_{uid,gid}") will be
> mapped in accordance with
> a mount's idmapping.
>
> This solution seems ideal, because it is philosophically correct, it
> makes cephfs idmapped mounts to work
> in the same manner and way as idmapped mounts work for any other
> filesystem like ext4.
Okay, this approach sounds more reasonable to me. But you need to do
some extra work to make it to be compatible between {old,new} kernels
and {old,new} cephs.
So then the caller uid/gid will always be the user uid/gid issuing the
requests as now.
Thanks
- Xiubo
> But yes, this requires cephfs protocol changes...
>
> I personally still believe that the "Solution 0" approach is optimal
> and we can go with "Solution 3" way
> as the next iteration.
>
> Kind regards,
> Alex
>
>> And also the same for other non-create requests. If
>>> so this will be incorrect for the cephx perm checks IMO.
>> Thanks,
>> Alex
>>
>>> Thanks
>>>
>>> - Xiubo
>>>
>>>
>>>> This makes a problem with path-based UID/GID restriction mechanism,
>>>> because it uses head->caller_{uid,gid} fields
>>>> to check if UID/GID is permitted or not.
>>>>
>>>> So, the problem is that we have one field in ceph request for two
>>>> different needs - to control permissions and to set inode owner.
>>>> Christian pointed that the most saner way is to modify ceph protocol
>>>> and add a separate field to store inode owner UID/GID,
>>>> and only this fields should be idmapped, but head->caller_{uid,gid}
>>>> will be untouched.
>>>>
>>>> With this approach, we will not affect UID/GID-based permission rules
>>>> with an idmapped mounts at all.
>>>>
>>>> Kind regards,
>>>> Alex
>>>>
>>>>> Thanks
>>>>>
>>>>> - Xiubo
>>>>>
>>>>>
>>>>>> Kind regards,
>>>>>> Alex
>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> - Xiubo
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alex
>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> - Xiubo
>>>>>>>>>
next prev parent reply other threads:[~2023-07-20 6:39 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-08 15:42 [PATCH v5 00/14] ceph: support idmapped mounts Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 01/14] fs: export mnt_idmap_get/mnt_idmap_put Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 02/14] ceph: stash idmapping in mdsc request Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 03/14] ceph: handle idmapped mounts in create_request_message() Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 04/14] ceph: pass an idmapping to mknod/symlink/mkdir/rename Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 05/14] ceph: allow idmapped getattr inode op Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 06/14] ceph: allow idmapped permission " Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 07/14] ceph: pass idmap to __ceph_setattr Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 08/14] ceph: allow idmapped setattr inode op Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 09/14] ceph/acl: allow idmapped set_acl " Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 10/14] ceph/file: allow idmapped atomic_open " Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 11/14] ceph: pass idmap to ceph_do_getattr Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 12/14] ceph: pass idmap to __ceph_setxattr Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 13/14] ceph: pass idmap to ceph_open/ioctl_set_layout Alexander Mikhalitsyn
2023-06-08 15:42 ` [PATCH v5 14/14] ceph: allow idmapped mounts Alexander Mikhalitsyn
2023-06-09 1:57 ` [PATCH v5 00/14] ceph: support " Xiubo Li
2023-06-09 8:59 ` Aleksandr Mikhalitsyn
2023-06-09 9:59 ` Christian Brauner
2023-06-09 10:12 ` Aleksandr Mikhalitsyn
2023-06-13 1:43 ` Xiubo Li
2023-06-13 12:46 ` Aleksandr Mikhalitsyn
2023-06-14 9:45 ` Christian Brauner
2023-06-13 14:53 ` Gregory Farnum
2023-06-13 16:27 ` Aleksandr Mikhalitsyn
2023-06-14 1:52 ` Xiubo Li
2023-06-14 12:39 ` Aleksandr Mikhalitsyn
[not found] ` <CAEivzxcr99sERxZX17rZ5jW9YSzAWYvAjOOhBH+FqRoso2=yng@mail.gmail.com>
2023-06-15 5:08 ` Xiubo Li
2023-06-15 11:05 ` Aleksandr Mikhalitsyn
2023-06-15 12:29 ` Xiubo Li
2023-06-15 12:54 ` Aleksandr Mikhalitsyn
2023-06-21 16:55 ` Aleksandr Mikhalitsyn
2023-06-26 1:04 ` Xiubo Li
2023-06-24 1:36 ` Xiubo Li
2023-06-24 7:11 ` Aleksandr Mikhalitsyn
2023-06-26 2:12 ` Xiubo Li
2023-06-26 11:23 ` Aleksandr Mikhalitsyn
2023-06-26 11:49 ` Aleksandr Mikhalitsyn
2023-07-04 1:10 ` Xiubo Li
2023-07-04 1:08 ` Xiubo Li
2023-07-14 12:57 ` Aleksandr Mikhalitsyn
2023-07-18 1:44 ` Xiubo Li
2023-07-18 14:49 ` Aleksandr Mikhalitsyn
2023-07-19 11:57 ` Aleksandr Mikhalitsyn
2023-07-20 6:36 ` Xiubo Li [this message]
2023-07-20 6:41 ` Aleksandr Mikhalitsyn
2023-07-21 15:43 ` Aleksandr Mikhalitsyn
2023-07-24 1:02 ` Xiubo Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3af4f092-8de7-d217-cd2d-d39dfc625edd@redhat.com \
--to=xiubli@redhat.com \
--cc=aleksandr.mikhalitsyn@canonical.com \
--cc=brauner@kernel.org \
--cc=ceph-devel@vger.kernel.org \
--cc=gfarnum@redhat.com \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=stgraber@ubuntu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).