On Wed, Sep 14, 2022 at 10:14:26AM -0700, Darrick J. Wong wrote:
> On Fri, Sep 09, 2022 at 11:56:59AM +0200, Christian Brauner wrote:
> > A while ago we introduced a dedicated vfs{g,u}id_t type in commit
> > 1e5267cd0895 ("mnt_idmapping: add vfs{g,u}id_t"). We already switched
> > over a good part of the VFS. Ultimately we will remove all legacy
> > idmapped mount helpers that operate only on k{g,u}id_t
>
> Does this mean i_uid_into_mnt is going away?
Yes, all of the non-vfs{g,u}id_t helper will be removed once we finished
converting. That hopefully won't take long. I was just a bit busy with
posix acls (still am tbh) so didn't get around to removing it
completely yet. But the fs/attr.c and fs/posix_acl.c codepaths are
already switched.
>
> > in favor of the
> > new type safe helpers that operate on vfs{g,u}id_t.
> >
> > Cc: Dave Chinner <dchinner@xxxxxxxxxx>
> > Cc: Seth Forshee (Digital Ocean) <sforshee@xxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Cc: linux-xfs@xxxxxxxxxxxxxxx
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_inode.c | 5 ++---
> > fs/xfs/xfs_iops.c | 6 ++++--
> > fs/xfs/xfs_itable.c | 8 ++++++--
> > 3 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 28493c8e9bb2..bca204a5aecf 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -835,9 +835,8 @@ xfs_init_new_inode(
> > * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> > * (and only if the irix_sgid_inherit compatibility variable is set).
> > */
> > - if (irix_sgid_inherit &&
> > - (inode->i_mode & S_ISGID) &&
> > - !in_group_p(i_gid_into_mnt(mnt_userns, inode)))
> > + if (irix_sgid_inherit && (inode->i_mode & S_ISGID) &&
> > + !vfsgid_in_group_p(i_gid_into_vfsgid(mnt_userns, inode)))
> > inode->i_mode &= ~S_ISGID;
> >
> > ip->i_disk_size = 0;
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 45518b8c613c..5d670c85dcc2 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -558,6 +558,8 @@ xfs_vn_getattr(
> > struct inode *inode = d_inode(path->dentry);
> > struct xfs_inode *ip = XFS_I(inode);
> > struct xfs_mount *mp = ip->i_mount;
> > + vfsuid_t vfsuid = i_uid_into_vfsuid(mnt_userns, inode);
> > + vfsgid_t vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
> >
> > trace_xfs_getattr(ip);
> >
> > @@ -568,8 +570,8 @@ xfs_vn_getattr(
> > stat->dev = inode->i_sb->s_dev;
> > stat->mode = inode->i_mode;
> > stat->nlink = inode->i_nlink;
> > - stat->uid = i_uid_into_mnt(mnt_userns, inode);
> > - stat->gid = i_gid_into_mnt(mnt_userns, inode);
> > + stat->uid = vfsuid_into_kuid(vfsuid);
> > + stat->gid = vfsgid_into_kgid(vfsgid);
> > stat->ino = ip->i_ino;
> > stat->atime = inode->i_atime;
> > stat->mtime = inode->i_mtime;
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 36312b00b164..a1c2bcf65d37 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -66,6 +66,8 @@ xfs_bulkstat_one_int(
> > struct xfs_bulkstat *buf = bc->buf;
> > xfs_extnum_t nextents;
> > int error = -EINVAL;
> > + vfsuid_t vfsuid;
> > + vfsgid_t vfsgid;
> >
> > if (xfs_internal_inum(mp, ino))
> > goto out_advance;
> > @@ -81,14 +83,16 @@ xfs_bulkstat_one_int(
> > ASSERT(ip != NULL);
> > ASSERT(ip->i_imap.im_blkno != 0);
> > inode = VFS_I(ip);
> > + vfsuid = i_uid_into_vfsuid(mnt_userns, inode);
> > + vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
> >
> > /* xfs_iget returns the following without needing
> > * further change.
> > */
> > buf->bs_projectid = ip->i_projid;
> > buf->bs_ino = ino;
> > - buf->bs_uid = from_kuid(sb_userns, i_uid_into_mnt(mnt_userns, inode));
> > - buf->bs_gid = from_kgid(sb_userns, i_gid_into_mnt(mnt_userns, inode));
> > + buf->bs_uid = from_kuid(sb_userns, vfsuid_into_kuid(vfsuid));
> > + buf->bs_gid = from_kgid(sb_userns, vfsgid_into_kgid(vfsgid));
>
> Hmm. How does this work, exactly?
>
> First we map inode.i_uid into a uid in the (file) mount userns, which I
> guess is what a vfsuid_t is? And then we map that vfsuid_t into a
Yes.
> kernel uid (hence kuid_t)? After which we map that kuid_t into the...
> filesystem(?)'s userns, which is what we return to userspace?
Yes.
>
> /me confused, need picture. :/
Without going into all of [1] the gist is that we undo the filesystem
idmapping and apply the mount idmapping. For this we use a new type to
clearly indicate that this is a {g,u}id as seen from the mount.
Since this ioctl reports ownership information we now need to translate
that vfs{g,u}id_t into a userspace {g,u}id.
A filesystem would usually not see this unless it has something like xfs
does where it allows to report ownership outside of the vfs.
Note that the bulkstat code also doesn't currently support idmapped
mounts - and won't because it can operate on the whole filesystem not
just within a mount. So i_uid_into_vfsuid() is always a nop and just
changes the type.
>
> (Admittedly my unfamiliarity with idmapping isn't helping here -- this
> looks consistent with what the VFS does, but I don't quite grok the
> layers...)
[1]: So it is fully documented in
Documentations/filesystems/idmappings.rst.
I'll update that with the new helpers and types in a bit.
