From: Christian Brauner <brauner@kernel.org>
To: Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
Cc: Xiubo Li <xiubli@redhat.com>, Gregory Farnum <gfarnum@redhat.com>,
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: Wed, 14 Jun 2023 11:45:45 +0200 [thread overview]
Message-ID: <20230614-westseite-urlaub-7a5afcf0577a@brauner> (raw)
In-Reply-To: <CAEivzxeZ6fDgYMnjk21qXYz13tHqZa8rP-cZ2jdxkY0eX+dOjw@mail.gmail.com>
On Tue, Jun 13, 2023 at 02:46:02PM +0200, Aleksandr Mikhalitsyn wrote:
> On Tue, Jun 13, 2023 at 3:43 AM Xiubo Li <xiubli@redhat.com> wrote:
> >
> >
> > On 6/9/23 18:12, Aleksandr Mikhalitsyn wrote:
> > > On Fri, Jun 9, 2023 at 12:00 PM Christian Brauner <brauner@kernel.org> wrote:
> > >> On Fri, Jun 09, 2023 at 10:59:19AM +0200, Aleksandr Mikhalitsyn wrote:
> > >>> On Fri, Jun 9, 2023 at 3:57 AM Xiubo Li <xiubli@redhat.com> wrote:
> > >>>>
> > >>>> On 6/8/23 23:42, Alexander Mikhalitsyn wrote:
> > >>>>> Dear friends,
> > >>>>>
> > >>>>> This patchset was originally developed by Christian Brauner but I'll continue
> > >>>>> to push it forward. Christian allowed me to do that :)
> > >>>>>
> > >>>>> This feature is already actively used/tested with LXD/LXC project.
> > >>>>>
> > >>>>> Git tree (based on https://github.com/ceph/ceph-client.git master):
> > >>> Hi Xiubo!
> > >>>
> > >>>> Could you rebase these patches to 'testing' branch ?
> > >>> Will do in -v6.
> > >>>
> > >>>> And you still have missed several places, for example the following cases:
> > >>>>
> > >>>>
> > >>>> 1 269 fs/ceph/addr.c <<ceph_netfs_issue_op_inline>>
> > >>>> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETATTR,
> > >>>> mode);
> > >>> +
> > >>>
> > >>>> 2 389 fs/ceph/dir.c <<ceph_readdir>>
> > >>>> req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS);
> > >>> +
> > >>>
> > >>>> 3 789 fs/ceph/dir.c <<ceph_lookup>>
> > >>>> req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
> > >>> We don't have an idmapping passed to lookup from the VFS layer. As I
> > >>> mentioned before, it's just impossible now.
> > >> ->lookup() doesn't deal with idmappings and really can't otherwise you
> > >> risk ending up with inode aliasing which is really not something you
> > >> want. IOW, you can't fill in inode->i_{g,u}id based on a mount's
> > >> idmapping as inode->i_{g,u}id absolutely needs to be a filesystem wide
> > >> value. So better not even risk exposing the idmapping in there at all.
> > > Thanks for adding, Christian!
> > >
> > > I agree, every time when we use an idmapping we need to be careful with
> > > what we map. AFAIU, inode->i_{g,u}id should be based on the filesystem
> > > idmapping (not mount),
> > > but in this case, Xiubo want's current_fs{u,g}id to be mapped
> > > according to an idmapping.
> > > Anyway, it's impossible at now and IMHO, until we don't have any
> > > practical use case where
> > > UID/GID-based path restriction is used in combination with idmapped
> > > mounts it's not worth to
> > > make such big changes in the VFS layer.
> > >
> > > May be I'm not right, but it seems like UID/GID-based path restriction
> > > is not a widespread
> > > feature and I can hardly imagine it to be used with the container
> > > workloads (for instance),
> > > because it will require to always keep in sync MDS permissions
> > > configuration with the
> > > possible UID/GID ranges on the client. It looks like a nightmare for sysadmin.
> > > It is useful when cephfs is used as an external storage on the host, but if you
> > > share cephfs with a few containers with different user namespaces idmapping...
> >
> > Hmm, while this will break the MDS permission check in cephfs then in
> > lookup case. If we really couldn't support it we should make it to
> > escape the check anyway or some OPs may fail and won't work as expected.
>
> Hi Xiubo!
>
> Disabling UID/GID checks on the MDS side looks reasonable. IMHO the
> most important checks are:
> - open
> - mknod/mkdir/symlink/rename
> and for these checks we already have an idmapping.
>
> Also, I want to add that it's a little bit unusual when permission
> checks are done against the caller UID/GID.
The server side permission checking based on the sender's fs{g,u}id is
rather esoteric imho. So I would just disable it for idmapped mounts.
> Usually, if we have opened a file descriptor and, for instance, passed
> this file descriptor through a unix socket then
> file descriptor holder will be able to use it in accordance with the
> flags (O_RDONLY, O_RDWR, ...).
> We also have ->f_cred on the struct file that contains credentials of
> the file opener and permission checks are usually done
> based on this. But in cephfs we are always using syscall caller's
> credentials. It makes cephfs file descriptor "not transferable"
> in terms of permission checks.
Yeah, that's another good point.
next prev parent reply other threads:[~2023-06-14 9:46 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 [this message]
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
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=20230614-westseite-urlaub-7a5afcf0577a@brauner \
--to=brauner@kernel.org \
--cc=aleksandr.mikhalitsyn@canonical.com \
--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 \
--cc=xiubli@redhat.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).