On 7/21/23 23:43, Aleksandr Mikhalitsyn wrote:
On Thu, Jul 20, 2023 at 8:36 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
On 7/19/23 19:57, Aleksandr Mikhalitsyn wrote:
On Tue, Jul 18, 2023 at 4:49 PM Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@xxxxxxxxxxxxx> wrote:
On Tue, Jul 18, 2023 at 3:45 AM Xiubo Li <xiubli@xxxxxxxxxx> 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
