| Message ID | 20220909095659.944062-1-brauner@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: port to vfs{g,u}id_t and associated helpers | expand |
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 in favor of the > new type safe helpers that operate on vfs{g,u}id_t. > > Cc: Dave Chinner <dchinner@redhat.com> > Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Darrick J. Wong <djwong@kernel.org> > Cc: linux-xfs@vger.kernel.org > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > 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)); So we convert i_uid into vfsguid into kuid into the user space visible one here. I wonder if there is any good short cut, as reading this chaing feels odd. But I guess that's what we get for a non-VFS ioctl interface exposint uids/gids.
On Fri, Sep 09, 2022 at 04:40:47PM +0200, Christoph Hellwig 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 in favor of the > > new type safe helpers that operate on vfs{g,u}id_t. > > > > Cc: Dave Chinner <dchinner@redhat.com> > > Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Darrick J. Wong <djwong@kernel.org> > > Cc: linux-xfs@vger.kernel.org > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > --- > > 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)); > > So we convert i_uid into vfsguid into kuid into the user space visible > one here. I wonder if there is any good short cut, as reading this > chaing feels odd. But I guess that's what we get for a non-VFS > ioctl interface exposint uids/gids. We could add a tiny helper for this for sure but I think there's so little need for it that it wouldn't make much sense. But if more cases pop up during conversion of all direct users we should definitely add one.
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? > in favor of the > new type safe helpers that operate on vfs{g,u}id_t. > > Cc: Dave Chinner <dchinner@redhat.com> > Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Darrick J. Wong <djwong@kernel.org> > Cc: linux-xfs@vger.kernel.org > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > 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 kernel uid (hence kuid_t)? After which we map that kuid_t into the... filesystem(?)'s userns, which is what we return to userspace? /me confused, need picture. :/ (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...) --D > buf->bs_size = ip->i_disk_size; > > buf->bs_nlink = inode->i_nlink; > > base-commit: 7e18e42e4b280c85b76967a9106a13ca61c16179 > -- > 2.34.1 >
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@redhat.com> > > Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Darrick J. Wong <djwong@kernel.org> > > Cc: linux-xfs@vger.kernel.org > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > --- > > 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.
On Wed, Sep 14, 2022 at 07:29:21PM +0200, Christian Brauner wrote: > 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@redhat.com> > > > Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> > > > Cc: Christoph Hellwig <hch@lst.de> > > > Cc: Darrick J. Wong <djwong@kernel.org> > > > Cc: linux-xfs@vger.kernel.org > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > --- > > > 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. Heh, I'm surprised I understood all that. :) > 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. Ok. I did take a look at https://www.kernel.org/doc/html/latest/filesystems/idmappings.html but it was the lack of any mention of the new helpers that I got stuck on. :( With documentation updates :D, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D --D
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)); buf->bs_size = ip->i_disk_size; buf->bs_nlink = inode->i_nlink;
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 in favor of the new type safe helpers that operate on vfs{g,u}id_t. Cc: Dave Chinner <dchinner@redhat.com> Cc: Seth Forshee (Digital Ocean) <sforshee@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Darrick J. Wong <djwong@kernel.org> Cc: linux-xfs@vger.kernel.org Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- 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(-) base-commit: 7e18e42e4b280c85b76967a9106a13ca61c16179