[PATCH 0/3] tweak idmap helpers

[PATCH 0/3] tweak idmap helpers

[Christian Brauner]

Hey,

This little series tries to improve naming and developer friendliness of fs
idmapping helpers triggered by a request/comment from Vivek.
Let's remove the two open-coded checks for whether there's a mapping for
fsuid/fsgid in the s_user_ns of the underlying filesystem. Instead move them
into a tiny helper, getting rid of redundancy and making sure that if we ever
change something it's changed in all places. Also add two helpers to initialize
and inode's i_uid and i_gid fields taking into account idmapped mounts making
it easier for fs developers.

This patch series is on top of Darrick's changes in the xfs-5.12-fixes-2
tag or xfs-5.12-fixes branch since renaming the two helpers in the second patch
affects xfs which calls them when initializing quotas.

The xfstests I sent out all pass:

1. Idmapped mounts test-suite
ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/627
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-inode-helpers #343 SMP Mon Mar 15 12:57:02 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

generic/627      16s
Ran: generic/627
Passed all 1 tests

2. Detached mount propagation
ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/626
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-inode-helpers #343 SMP Mon Mar 15 12:57:02 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

generic/626 10s ...  9s
Ran: generic/626
Passed all 1 tests

3. Testing xfs quotas can't be exceeded/work correctly from idmapped mounts
ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check xfs/528
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-inode-helpers #343 SMP Mon Mar 15 12:57:02 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

xfs/528 41s ...  44s
Ran: xfs/528
Passed all 1 tests

4. Testing xfs qutoas on idmapped mounts
ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check xfs/529
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-inode-helpers #343 SMP Mon Mar 15 12:57:02 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

xfs/529 23s ...  25s
Ran: xfs/529
Passed all 1 tests

Thanks!
Christian

Christian Brauner (3):
  fs: introduce fsuidgid_has_mapping() helper
  fs: improve naming for fsid helpers
  fs: introduce two little fs{u,g}id inode initialization helpers

 fs/ext4/ialloc.c     |  2 +-
 fs/inode.c           |  4 ++--
 fs/namei.c           | 11 +++--------
 fs/xfs/xfs_inode.c   | 10 +++++-----
 fs/xfs/xfs_symlink.c |  4 ++--
 include/linux/fs.h   | 27 +++++++++++++++++++++++++--
 6 files changed, 38 insertions(+), 20 deletions(-)


base-commit: 1e28eed17697bcf343c6743f0028cc3b5dd88bf0
prerequisite-patch-id: 0bdc07ef3137ce6d6ef284ad308de9a9bc2ea1f3
prerequisite-patch-id: a1c05069d67f08ea75e88f54f5fdf86db4d82865
prerequisite-patch-id: 707b24a3c6f71599e038a66aa3474b270d2699fe
prerequisite-patch-id: c42d85f64933a4e48aa10192c374bebad07ca5c0
prerequisite-patch-id: 5fcf22eea4b225691a034308a23c309f1583f970
-- 
2.27.0

[PATCH 1/3] fs: introduce fsuidgid_has_mapping() helper

[Christian Brauner]

Don't open-code the checks and instead move them into a clean little
helper we can call. This also reduces the risk that if we ever change
something we forget to change all locations.

Inspired-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/namei.c         | 11 +++--------
 include/linux/fs.h | 11 +++++++++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 216f16e74351..bc03cbc37ba7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2823,16 +2823,14 @@ static int may_delete(struct user_namespace *mnt_userns, struct inode *dir,
 static inline int may_create(struct user_namespace *mnt_userns,
 			     struct inode *dir, struct dentry *child)
 {
-	struct user_namespace *s_user_ns;
 	audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
 	if (child->d_inode)
 		return -EEXIST;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
-	s_user_ns = dir->i_sb->s_user_ns;
-	if (!kuid_has_mapping(s_user_ns, fsuid_into_mnt(mnt_userns)) ||
-	    !kgid_has_mapping(s_user_ns, fsgid_into_mnt(mnt_userns)))
+	if (!fsuidgid_has_mapping(dir->i_sb, mnt_userns))
 		return -EOVERFLOW;
+
 	return inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC);
 }
 
@@ -3034,14 +3032,11 @@ static int may_o_create(struct user_namespace *mnt_userns,
 			const struct path *dir, struct dentry *dentry,
 			umode_t mode)
 {
-	struct user_namespace *s_user_ns;
 	int error = security_path_mknod(dir, dentry, mode, 0);
 	if (error)
 		return error;
 
-	s_user_ns = dir->dentry->d_sb->s_user_ns;
-	if (!kuid_has_mapping(s_user_ns, fsuid_into_mnt(mnt_userns)) ||
-	    !kgid_has_mapping(s_user_ns, fsgid_into_mnt(mnt_userns)))
+	if (!fsuidgid_has_mapping(dir->dentry->d_sb, mnt_userns))
 		return -EOVERFLOW;
 
 	error = inode_permission(mnt_userns, dir->dentry->d_inode,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ec8f3ddf4a6a..edcb1aa99fd6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1620,6 +1620,17 @@ static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
 
+static inline bool fsuidgid_has_mapping(struct super_block *sb,
+					struct user_namespace *mnt_userns)
+{
+	struct user_namespace *s_user_ns = sb->s_user_ns;
+
+	return kuid_has_mapping(s_user_ns,
+				kuid_from_mnt(mnt_userns, current_fsuid())) &&
+	       kgid_has_mapping(s_user_ns,
+				kgid_from_mnt(mnt_userns, current_fsgid()));
+}
+
 extern struct timespec64 current_time(struct inode *inode);
 
 /*
-- 
2.27.0

[PATCH 2/3] fs: improve naming for fsid helpers

[Christian Brauner]

Vivek pointed out that the current naming scheme can be misleading as it
conflicts with some of the other helpers naming. So get rid of the
confusion by simply calling those helpers idmapped_fs{u,g}id() that
make it very clear that and idmapped fsuid/fsgid is used. xfs needs to
use them directly in the quota allocation codepaths.

Inspired-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/xfs/xfs_inode.c   | 8 ++++----
 fs/xfs/xfs_symlink.c | 4 ++--
 include/linux/fs.h   | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index f93370bd7b1e..8703408bd1aa 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1007,8 +1007,8 @@ xfs_create(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
-			fsgid_into_mnt(mnt_userns), prid,
+	error = xfs_qm_vop_dqalloc(dp, idmapped_fsuid(mnt_userns),
+			idmapped_fsgid(mnt_userns), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 			&udqp, &gdqp, &pdqp);
 	if (error)
@@ -1158,8 +1158,8 @@ xfs_create_tmpfile(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
-			fsgid_into_mnt(mnt_userns), prid,
+	error = xfs_qm_vop_dqalloc(dp, idmapped_fsuid(mnt_userns),
+			idmapped_fsgid(mnt_userns), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 			&udqp, &gdqp, &pdqp);
 	if (error)
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 7f368b10ded1..669e8517e2e1 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -182,8 +182,8 @@ xfs_symlink(
 	/*
 	 * Make sure that we have allocated dquot(s) on disk.
 	 */
-	error = xfs_qm_vop_dqalloc(dp, fsuid_into_mnt(mnt_userns),
-			fsgid_into_mnt(mnt_userns), prid,
+	error = xfs_qm_vop_dqalloc(dp, idmapped_fsuid(mnt_userns),
+			idmapped_fsgid(mnt_userns), prid,
 			XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
 			&udqp, &gdqp, &pdqp);
 	if (error)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index edcb1aa99fd6..189673721726 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1610,12 +1610,12 @@ static inline kgid_t kgid_from_mnt(struct user_namespace *mnt_userns,
 	return KGIDT_INIT(from_kgid(mnt_userns, kgid));
 }
 
-static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
+static inline kuid_t idmapped_fsuid(struct user_namespace *mnt_userns)
 {
 	return kuid_from_mnt(mnt_userns, current_fsuid());
 }
 
-static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
+static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
 {
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
-- 
2.27.0

[PATCH 3/3] fs: introduce two little fs{u,g}id inode initialization helpers

[Christian Brauner]

Give filesystem two little helpers so that do the right thing when
initializing the i_uid and i_gid fields on idmapped and non-idmapped
mounts. Filesystemd don't need to bother with too many details for this.

Inspired-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/ext4/ialloc.c   |  2 +-
 fs/inode.c         |  4 ++--
 fs/xfs/xfs_inode.c |  2 +-
 include/linux/fs.h | 12 ++++++++++++
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 633ae7becd61..755a68bb7e22 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -970,7 +970,7 @@ struct inode *__ext4_new_inode(struct user_namespace *mnt_userns,
 		i_gid_write(inode, owner[1]);
 	} else if (test_opt(sb, GRPID)) {
 		inode->i_mode = mode;
-		inode->i_uid = fsuid_into_mnt(mnt_userns);
+		inode_fsuid_set(inode, mnt_userns);
 		inode->i_gid = dir->i_gid;
 	} else
 		inode_init_owner(mnt_userns, inode, dir, mode);
diff --git a/fs/inode.c b/fs/inode.c
index a047ab306f9a..21c5a620ca89 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2148,7 +2148,7 @@ EXPORT_SYMBOL(init_special_inode);
 void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		      const struct inode *dir, umode_t mode)
 {
-	inode->i_uid = fsuid_into_mnt(mnt_userns);
+	inode_fsuid_set(inode, mnt_userns);
 	if (dir && dir->i_mode & S_ISGID) {
 		inode->i_gid = dir->i_gid;
 
@@ -2160,7 +2160,7 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
 			mode &= ~S_ISGID;
 	} else
-		inode->i_gid = fsgid_into_mnt(mnt_userns);
+		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
 }
 EXPORT_SYMBOL(inode_init_owner);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 8703408bd1aa..20846810f13f 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -812,7 +812,7 @@ xfs_init_new_inode(
 
 	if (dir && !(dir->i_mode & S_ISGID) &&
 	    (mp->m_flags & XFS_MOUNT_GRPID)) {
-		inode->i_uid = fsuid_into_mnt(mnt_userns);
+		inode_fsuid_set(inode, mnt_userns);
 		inode->i_gid = dir->i_gid;
 		inode->i_mode = mode;
 	} else {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 189673721726..0cde0cbc20fc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1620,6 +1620,18 @@ static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
 
+static inline void inode_fsuid_set(struct inode *inode,
+				   struct user_namespace *mnt_userns)
+{
+	inode->i_uid = idmapped_fsuid(mnt_userns);
+}
+
+static inline void inode_fsgid_set(struct inode *inode,
+				   struct user_namespace *mnt_userns)
+{
+	inode->i_gid = idmapped_fsgid(mnt_userns);
+}
+
 static inline bool fsuidgid_has_mapping(struct super_block *sb,
 					struct user_namespace *mnt_userns)
 {
-- 
2.27.0

Re: [PATCH 1/3] fs: introduce fsuidgid_has_mapping() helper

[Christoph Hellwig]

On Mon, Mar 15, 2021 at 03:54:17PM +0100, Christian Brauner wrote:
> Don't open-code the checks and instead move them into a clean little
> helper we can call. This also reduces the risk that if we ever change
> something we forget to change all locations.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] fs: improve naming for fsid helpers

[Christoph Hellwig]

> -static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
> +static inline kuid_t idmapped_fsuid(struct user_namespace *mnt_userns)
>  {
>  	return kuid_from_mnt(mnt_userns, current_fsuid());
>  }
>  
> -static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> +static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
>  {
>  	return kgid_from_mnt(mnt_userns, current_fsgid());
>  }

I'm not sure the naming is an improvement.  I always think of
identity mapped when reading it, which couldn't be further from what
it does..  But either way comments describing what these helpers do
would be very useful.

Re: [PATCH 3/3] fs: introduce two little fs{u,g}id inode initialization helpers

[Christoph Hellwig]

On Mon, Mar 15, 2021 at 03:54:19PM +0100, Christian Brauner wrote:
> Give filesystem two little helpers so that do the right thing when
> initializing the i_uid and i_gid fields on idmapped and non-idmapped
> mounts. Filesystemd don't need to bother with too many details for this.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] fs: improve naming for fsid helpers

[Al Viro]

On Thu, Mar 18, 2021 at 07:27:23AM +0100, Christoph Hellwig wrote:
> > -static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
> > +static inline kuid_t idmapped_fsuid(struct user_namespace *mnt_userns)
> >  {
> >  	return kuid_from_mnt(mnt_userns, current_fsuid());
> >  }
> >  
> > -static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> > +static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
> >  {
> >  	return kgid_from_mnt(mnt_userns, current_fsgid());
> >  }
> 
> I'm not sure the naming is an improvement.  I always think of
> identity mapped when reading it, which couldn't be further from what
> it does..  But either way comments describing what these helpers do
> would be very useful.

s/idmapped/mapped/?

Re: [PATCH 2/3] fs: improve naming for fsid helpers

[Christian Brauner]

On Thu, Mar 18, 2021 at 06:30:50AM +0000, Al Viro wrote:
> On Thu, Mar 18, 2021 at 07:27:23AM +0100, Christoph Hellwig wrote:
> > > -static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
> > > +static inline kuid_t idmapped_fsuid(struct user_namespace *mnt_userns)
> > >  {
> > >  	return kuid_from_mnt(mnt_userns, current_fsuid());
> > >  }
> > >  
> > > -static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> > > +static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
> > >  {
> > >  	return kgid_from_mnt(mnt_userns, current_fsgid());
> > >  }
> > 
> > I'm not sure the naming is an improvement.  I always think of
> > identity mapped when reading it, which couldn't be further from what
> > it does..  But either way comments describing what these helpers do
> > would be very useful.
> 
> s/idmapped/mapped/?

Yeah, I think that's a good idea.

I'll also add comments.

Re: [PATCH 2/3] fs: improve naming for fsid helpers

[Christian Brauner]

On Thu, Mar 18, 2021 at 07:27:23AM +0100, Christoph Hellwig wrote:
> > -static inline kuid_t fsuid_into_mnt(struct user_namespace *mnt_userns)
> > +static inline kuid_t idmapped_fsuid(struct user_namespace *mnt_userns)
> >  {
> >  	return kuid_from_mnt(mnt_userns, current_fsuid());
> >  }
> >  
> > -static inline kgid_t fsgid_into_mnt(struct user_namespace *mnt_userns)
> > +static inline kgid_t idmapped_fsgid(struct user_namespace *mnt_userns)
> >  {
> >  	return kgid_from_mnt(mnt_userns, current_fsgid());
> >  }
> 
> I'm not sure the naming is an improvement.  I always think of
> identity mapped when reading it, which couldn't be further from what
> it does..  But either way comments describing what these helpers do
> would be very useful.

Good point. I'll add comments!