Messages in this thread | | | Date | Mon, 24 Jul 2023 09:02:15 +0800 | Subject | Re: [PATCH v5 00/14] ceph: support idmapped mounts | From | Xiubo Li <> |
| |
On 7/21/23 23:43, Aleksandr Mikhalitsyn wrote: > On Thu, Jul 20, 2023 at 8:36 AM Xiubo Li <xiubli@redhat.com> wrote: >> >> 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. > Dear Xiubo, > > I've posted a PR https://github.com/ceph/ceph/pull/52575
Sure. Will check.
Thanks
- Xiubo
> Kind regards, > Alex > >> 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 >>>>>>>>>>>
| |