[PATCH v2 0/4] tweak fs mapping helpers

[PATCH v2 0/4] tweak fs mapping helpers

[Christian Brauner]

Hey,

/* v2 */
Add some kernel docs to helpers as suggested by Christoph.
Switch to Al's naming proposal for these helpers.
(Added Acks.)

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.

The xfstests I sent out all pass for both xfs and ext4:

#### xfs
  1. Detached mount propagation
     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/631
     FSTYP         -- xfs (non-debug)
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
     MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch
     
     generic/631 9s ...  11s
     Ran: generic/631
     Passed all 1 tests
    
  2. Idmapped mounts test-suite
     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/632
     FSTYP         -- xfs (non-debug)
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
     MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch
     
     generic/632 13s ...  14s
     Ran: generic/632
     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/529
     FSTYP         -- xfs (non-debug)
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
     MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch
     
     xfs/529 42s ...  44s
     Ran: xfs/529
     Passed all 1 tests
    
  4. Testing xfs qutoas on idmapped mounts
     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check xfs/530
     hFSTYP         -- xfs (non-debug)
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop1
     MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

     xfs/530 20s ...  20s
     Ran: xfs/530
     Passed all 1 tests

#### ext4
  1. Detached mount propagation

     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/631
     FSTYP         -- ext4
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- /dev/loop1
     MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch
     
     generic/631 11s ...  8s
     Ran: generic/631
     Passed all 1 tests

  2. Idmapped mounts test-suite

     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check generic/632
     FSTYP         -- ext4
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- /dev/loop1
     MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch
     
     generic/632 14s ...  10s
     Ran: generic/632
     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/529
     FSTYP         -- ext4
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- /dev/loop1
     MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch
     
     xfs/529 44s ... [not run] not suitable for this filesystem type: ext4
     Ran: xfs/529
     Not run: xfs/529
     Passed all 1 tests

  4. Testing xfs qutoas on idmapped mounts
     ubuntu@f1-vm:~/src/git/xfstests$ sudo ./check xfs/530
     FSTYP         -- ext4
     PLATFORM      -- Linux/x86_64 f1-vm 5.12.0-rc3-idmapped-mounts-inode-helpers #351 SMP Sat Mar 20 10:32:48 UTC 2021
     MKFS_OPTIONS  -- /dev/loop1
     MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt/scratch
     
     xfs/530 20s ... [not run] not suitable for this filesystem type: ext4
     Ran: xfs/530
     Not run: xfs/530
     Passed all 1 tests

Thanks!
Christian

Christian Brauner (4):
  fs: document mapping helpers
  fs: document and rename fsid helpers
  fs: introduce fsuidgid_has_mapping() helper
  fs: introduce two inode i_{u,g}id 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   | 124 ++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 135 insertions(+), 20 deletions(-)


base-commit: 8b12a62a4e3ed4ae99c715034f557eb391d6b196
-- 
2.27.0

[PATCH v2 1/4] fs: document mapping helpers

[Christian Brauner]

Document new helpers we introduced this cycle.

Suggested-by: 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>
---
/* v2 */
patch introduced
- Christoph Hellwig <hch@lst.de>:
  - Add kernel docs to helpers.
---
 include/linux/fs.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index ec8f3ddf4a6a..4795a632ef0d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1574,36 +1574,84 @@ static inline void i_gid_write(struct inode *inode, gid_t gid)
 	inode->i_gid = make_kgid(inode->i_sb->s_user_ns, gid);
 }
 
+/**
+ * kuid_into_mnt - map a kuid down into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ * @kuid: kuid to be mapped
+ *
+ * Return @kuid mapped according to @mnt_userns.
+ * If @kuid has no mapping INVALID_UID is returned.
+ */
 static inline kuid_t kuid_into_mnt(struct user_namespace *mnt_userns,
 				   kuid_t kuid)
 {
 	return make_kuid(mnt_userns, __kuid_val(kuid));
 }
 
+/**
+ * kgid_into_mnt - map a kgid down into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ * @kgid: kgid to be mapped
+ *
+ * Return @kgid mapped according to @mnt_userns.
+ * If @kgid has no mapping INVALID_GID is returned.
+ */
 static inline kgid_t kgid_into_mnt(struct user_namespace *mnt_userns,
 				   kgid_t kgid)
 {
 	return make_kgid(mnt_userns, __kgid_val(kgid));
 }
 
+/**
+ * i_uid_into_mnt - map an inode's i_uid down into a mnt_userns
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @inode: inode to map
+ *
+ * Return the inode's i_uid mapped down according to @mnt_userns.
+ * If the inode's i_uid has no mapping INVALID_UID is returned.
+ */
 static inline kuid_t i_uid_into_mnt(struct user_namespace *mnt_userns,
 				    const struct inode *inode)
 {
 	return kuid_into_mnt(mnt_userns, inode->i_uid);
 }
 
+/**
+ * i_gid_into_mnt - map an inode's i_gid down into a mnt_userns
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @inode: inode to map
+ *
+ * Return the inode's i_gid mapped down according to @mnt_userns.
+ * If the inode's i_gid has no mapping INVALID_GID is returned.
+ */
 static inline kgid_t i_gid_into_mnt(struct user_namespace *mnt_userns,
 				    const struct inode *inode)
 {
 	return kgid_into_mnt(mnt_userns, inode->i_gid);
 }
 
+/**
+ * kuid_from_mnt - map a kuid up into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ * @kuid: kuid to be mapped
+ *
+ * Return @kuid mapped up according to @mnt_userns.
+ * If @kuid has no mapping INVALID_UID is returned.
+ */
 static inline kuid_t kuid_from_mnt(struct user_namespace *mnt_userns,
 				   kuid_t kuid)
 {
 	return KUIDT_INIT(from_kuid(mnt_userns, kuid));
 }
 
+/**
+ * kgid_from_mnt - map a kgid up into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ * @kgid: kgid to be mapped
+ *
+ * Return @kgid mapped up according to @mnt_userns.
+ * If @kgid has no mapping INVALID_GID is returned.
+ */
 static inline kgid_t kgid_from_mnt(struct user_namespace *mnt_userns,
 				   kgid_t kgid)
 {
-- 
2.27.0

[PATCH v2 2/4] fs: document and rename fsid helpers

[Christian Brauner]

Vivek pointed out that the fs{g,u}id_into_mnt() naming scheme can be
misleading as it could be understood as implying they do the exact same
thing as i_{g,u}id_into_mnt(). The original motivation for this naming
scheme was to signal to callers that the helpers will always take care
to map the k{g,u}id such that the ownership is expressed in terms of the
mnt_users.
Get rid of the confusion by renaming those helpers to something more
sensible. Al suggested mapped_fs{g,u}id() which seems a really good fit.
Usually filesystems don't need to bother with these helpers directly
only in some cases where they allocate objects that carry {g,u}ids which
are either filesystem specific (e.g. xfs quota objects) or don't have a
clean set of helpers as inodes have.

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
Inspired-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Christoph Hellwig <hch@lst.de>:
  - Add kernel docs to helpers.

- Al Viro <viro@zeniv.linux.org.uk>:
  - s/idmapped_{fsuid,fsgid}/mapped_{fsuid,fsgid}/g
---
 fs/ext4/ialloc.c     |  2 +-
 fs/inode.c           |  4 ++--
 fs/namei.c           |  8 ++++----
 fs/xfs/xfs_inode.c   | 10 +++++-----
 fs/xfs/xfs_symlink.c |  4 ++--
 include/linux/fs.h   | 28 ++++++++++++++++++++++++++--
 6 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 633ae7becd61..d0dc12197346 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->i_uid = mapped_fsuid(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..81a6a59b7dd3 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->i_uid = mapped_fsuid(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->i_gid = mapped_fsgid(mnt_userns);
 	inode->i_mode = mode;
 }
 EXPORT_SYMBOL(inode_init_owner);
diff --git a/fs/namei.c b/fs/namei.c
index 216f16e74351..6b5424d34962 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2830,8 +2830,8 @@ static inline int may_create(struct user_namespace *mnt_userns,
 	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 (!kuid_has_mapping(s_user_ns, mapped_fsuid(mnt_userns)) ||
+	    !kgid_has_mapping(s_user_ns, mapped_fsgid(mnt_userns)))
 		return -EOVERFLOW;
 	return inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC);
 }
@@ -3040,8 +3040,8 @@ static int may_o_create(struct user_namespace *mnt_userns,
 		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 (!kuid_has_mapping(s_user_ns, mapped_fsuid(mnt_userns)) ||
+	    !kgid_has_mapping(s_user_ns, mapped_fsgid(mnt_userns)))
 		return -EOVERFLOW;
 
 	error = inode_permission(mnt_userns, dir->dentry->d_inode,
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index f93370bd7b1e..dc91f8c34d35 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->i_uid = mapped_fsuid(mnt_userns);
 		inode->i_gid = dir->i_gid;
 		inode->i_mode = mode;
 	} else {
@@ -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, mapped_fsuid(mnt_userns),
+			mapped_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, mapped_fsuid(mnt_userns),
+			mapped_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..63edb4dbed4a 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, mapped_fsuid(mnt_userns),
+			mapped_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 4795a632ef0d..c8603969d21f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1658,12 +1658,36 @@ 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)
+/**
+ * mapped_fsuid - return caller's fsuid mapped up into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ *
+ * Return the caller's current fsuid mapped up according to @mnt_userns.
+ *
+ * Use this helper to initialize a new vfs or filesystem object based on
+ * the caller's fsuid. A common example is initializing the i_uid field of
+ * a newly allocated inode triggered by a creation event such as mkdir or
+ * O_CREAT. Other examples include the allocation of quotas for a specific
+ * user.
+ */
+static inline kuid_t mapped_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)
+/**
+ * mapped_fsgid - return caller's fsgid mapped up into a mnt_userns
+ * @mnt_userns: user namespace of the relevant mount
+ *
+ * Return the caller's current fsgid mapped up according to @mnt_userns.
+ *
+ * Use this helper to initialize a new vfs or filesystem object based on
+ * the caller's fsgid. A common example is initializing the i_gid field of
+ * a newly allocated inode triggered by a creation event such as mkdir or
+ * O_CREAT. Other examples include the allocation of quotas for a specific
+ * user.
+ */
+static inline kgid_t mapped_fsgid(struct user_namespace *mnt_userns)
 {
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
-- 
2.27.0

[PATCH v2 3/4] 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.

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

diff --git a/fs/namei.c b/fs/namei.c
index 6b5424d34962..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, mapped_fsuid(mnt_userns)) ||
-	    !kgid_has_mapping(s_user_ns, mapped_fsgid(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, mapped_fsuid(mnt_userns)) ||
-	    !kgid_has_mapping(s_user_ns, mapped_fsgid(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 c8603969d21f..0e2ce21b2552 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1692,6 +1692,26 @@ static inline kgid_t mapped_fsgid(struct user_namespace *mnt_userns)
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
 
+/**
+ * fsuidgid_has_mapping() - check whether caller's fsuid/fsgid is mapped
+ * @sb: the superblock we want a mapping in
+ * @mnt_userns: user namespace of the relevant mount
+ *
+ * Check whether the caller's fsuid and fsgid have a valid mapping in the
+ * s_user_ns of the superblock @sb. If the caller is on an idmapped mount map
+ * the caller's fsuid and fsgid according to the @mnt_userns first.
+ *
+ * Returns true if fsuid and fsgid is mapped, false if not.
+ */
+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, mapped_fsuid(mnt_userns)) &&
+	       kgid_has_mapping(s_user_ns, mapped_fsgid(mnt_userns));
+}
+
 extern struct timespec64 current_time(struct inode *inode);
 
 /*
-- 
2.27.0

[PATCH v2 4/4] fs: introduce two inode i_{u,g}id initialization helpers

[Christian Brauner]

Give filesystem two little helpers that do the right thing when
initializing the i_uid and i_gid fields on idmapped and non-idmapped
mounts. Filesystems shouldn't have to be concerned with too many
details.

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

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index d0dc12197346..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 = mapped_fsuid(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 81a6a59b7dd3..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 = mapped_fsuid(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 = mapped_fsgid(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 dc91f8c34d35..2a8bdf33e6c4 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 = mapped_fsuid(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 0e2ce21b2552..4a4af6c26a01 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1692,6 +1692,34 @@ static inline kgid_t mapped_fsgid(struct user_namespace *mnt_userns)
 	return kgid_from_mnt(mnt_userns, current_fsgid());
 }
 
+/**
+ * inode_fsuid_set - initialize inode's i_uid field with callers fsuid
+ * @inode: inode to initialize
+ * @mnt_userns: user namespace of the mount the inode was found from
+ *
+ * Initialize the i_uid field of @inode. If the inode was found/created via
+ * an idmapped mount map the caller's fsuid according to @mnt_users.
+ */
+static inline void inode_fsuid_set(struct inode *inode,
+				   struct user_namespace *mnt_userns)
+{
+	inode->i_uid = mapped_fsuid(mnt_userns);
+}
+
+/**
+ * inode_fsgid_set - initialize inode's i_gid field with callers fsgid
+ * @inode: inode to initialize
+ * @mnt_userns: user namespace of the mount the inode was found from
+ *
+ * Initialize the i_gid field of @inode. If the inode was found/created via
+ * an idmapped mount map the caller's fsgid according to @mnt_users.
+ */
+static inline void inode_fsgid_set(struct inode *inode,
+				   struct user_namespace *mnt_userns)
+{
+	inode->i_gid = mapped_fsgid(mnt_userns);
+}
+
 /**
  * fsuidgid_has_mapping() - check whether caller's fsuid/fsgid is mapped
  * @sb: the superblock we want a mapping in
-- 
2.27.0

Re: [PATCH v2 1/4] fs: document mapping helpers

[Christoph Hellwig]

Looks good,

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

Re: [PATCH v2 2/4] fs: document and rename fsid helpers

[Christoph Hellwig]

Looks good,

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

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

[Christoph Hellwig]

On Sat, Mar 20, 2021 at 01:26:23PM +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 v2 4/4] fs: introduce two inode i_{u,g}id initialization helpers

[Christoph Hellwig]

Looks good,

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

Re: [PATCH v2 1/4] fs: document mapping helpers

[Matthew Wilcox]

On Sat, Mar 20, 2021 at 01:26:21PM +0100, Christian Brauner wrote:
> +/**
> + * kuid_into_mnt - map a kuid down into a mnt_userns
> + * @mnt_userns: user namespace of the relevant mount
> + * @kuid: kuid to be mapped
> + *
> + * Return @kuid mapped according to @mnt_userns.
> + * If @kuid has no mapping INVALID_UID is returned.
> + */

If you could just put the ':' after 'Return', htmldoc would put this into
a nice section for you.

I also like to include a Context: section which lists whether the
function takes locks / requires locks to be held / can be called in
hard or soft interrupt context / may sleep / requires refcounts be held /
...  Generally, what do you expect from your callers, and what your callers
can expect from you.

I don't understand the thing you're documenting, so it may not make sense
to talk about interrupt context, for example.

Re: [PATCH v2 1/4] fs: document mapping helpers

[Christian Brauner]

On Mon, Mar 22, 2021 at 07:35:46AM +0000, Matthew Wilcox wrote:
> On Sat, Mar 20, 2021 at 01:26:21PM +0100, Christian Brauner wrote:
> > +/**
> > + * kuid_into_mnt - map a kuid down into a mnt_userns
> > + * @mnt_userns: user namespace of the relevant mount
> > + * @kuid: kuid to be mapped
> > + *
> > + * Return @kuid mapped according to @mnt_userns.
> > + * If @kuid has no mapping INVALID_UID is returned.
> > + */
> 
> If you could just put the ':' after 'Return', htmldoc would put this into
> a nice section for you.

I'll fix that up in my tree. Thanks!

> 
> I also like to include a Context: section which lists whether the
> function takes locks / requires locks to be held / can be called in
> hard or soft interrupt context / may sleep / requires refcounts be held /
> ...  Generally, what do you expect from your callers, and what your callers
> can expect from you.

Thanks for the hint about "Context:". The functions don't take any
locks, they don't require locks to be held and they don't sleep and
don't manipulate refcounts (The lifetime of @mnt_userns is tied to the
respective mnt it's from. It can't be altered anymore once a non-initial
@mnt_userns has been attached to the mnt so it can't go away behind the
caller's back.). Internally only smp_rmb and smp_wmb are used and so
they should be fine to call from soft and hard irq.
Because of that it seemed not explicitly mentioning all that is more
correct then describing all of that. I always thought "Context:"
sections are "Here's things to keep in mind." less then "Here's how it
behaves in all those contexts.", i.e. point out pitfalls, not describe
regular behavior. I might be wrong though or it's a matter of
preference.

(Should we also aim for all other fs.h helpers to have similar comments?)

Christian