[PATCH 0/8] introduce dedicated type for idmapped mounts

[PATCH 0/8] introduce dedicated type for idmapped mounts

[Christian Brauner]

From: "Christian Brauner (Microsoft)" <brauner@kernel.org>

Hey everyone,

This series starts to introduce a new kmnt{g,u}id_t type. It allows to
distinguish {g,u}ids on idmapped mounts from filesystem k{g,u}ids.

We leverage the type framework to increase the safety for filesystems
and the vfs when dealing with idmapped mounts.

The series introduces the type and converts the setattr codepaths to
use the new type and associated helpers.

Currently these codepaths place the value that will ultimately be
written to inode->i_{g,u}id into attr->ia_{g,u}id which allows to avoid
changing a few callsites. But there are drawbacks to this approach.

As Linus rightly points out it makes some of the permission checks in
the attribute code harder to understand than they need and should be and
increases the probability for further issues.

This series makes it so that the values will always be treated as being
mapped into the idmapped mount. Only when the filesystem object is
actually updated will the value be mapped into the filesystem idmapping.

I first looked into this about ~7 months ago but put it on hold to focus
on the testsuite. Linus expressed the desire for something like this
last week so I got back to working on this.

Ideally I'd like to get at least this first series in for v5.20. The
conversion can the continue until we can remove all the regular non-type
safe helpers and will only be left with the type safe helpers.

Thanks!
Christian

Christian Brauner (8):
  mnt_idmapping: add kmnt{g,u}id_t
  fs: add two type safe mapping helpers
  fs: use mount types in iattr
  fs: introduce tiny iattr ownership update helpers
  fs: port to iattr ownership update helpers
  quota: port quota helpers mount ids
  security: pass down mount idmapping to setattr hook
  attr: port attribute changes to new types

 fs/attr.c                         |  69 +++++------
 fs/ext2/inode.c                   |   8 +-
 fs/ext4/inode.c                   |  14 +--
 fs/f2fs/file.c                    |  22 ++--
 fs/f2fs/recovery.c                |   2 +-
 fs/fat/file.c                     |   7 +-
 fs/jfs/file.c                     |   4 +-
 fs/ocfs2/file.c                   |   2 +-
 fs/open.c                         |  65 +++++++---
 fs/overlayfs/copy_up.c            |   4 +-
 fs/overlayfs/overlayfs.h          |  12 +-
 fs/quota/dquot.c                  |  17 ++-
 fs/reiserfs/inode.c               |   4 +-
 fs/xfs/xfs_iops.c                 |  11 +-
 fs/zonefs/super.c                 |   2 +-
 include/linux/evm.h               |   6 +-
 include/linux/fs.h                | 135 ++++++++++++++++++++-
 include/linux/mnt_idmapping.h     | 195 ++++++++++++++++++++++++++++++
 include/linux/quotaops.h          |  15 ++-
 include/linux/security.h          |   8 +-
 security/integrity/evm/evm_main.c |  12 +-
 security/security.c               |   5 +-
 22 files changed, 488 insertions(+), 131 deletions(-)


base-commit: a111daf0c53ae91e71fd2bfe7497862d14132e3e
-- 
2.34.1

[PATCH 1/8] mnt_idmapping: add kmnt{g,u}id_t

[Christian Brauner]

Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types
are just simple wrapper structs around regular {g,u}id_t types.

They allows to establish a type safety boundary between {g,u}ids on
idmapped mounts and {g,u}ids as they are represented in filesystems
themselves.

A kmnt{g,u}id_t is always created from a k{g,u}id_t, never directly from
a {g,u}id_t as idmapped mounts remap a given {g,u}id according to the
mount's idmapping. This is expressed in the KMNT{G,U}IDT_INIT() macros.

A kmnt{g,u}id_t may be used as a k{g,u}id_t via AS_K{G,U}IDT(). This
often happens when we need to check whether a {g,u}id mapped according
to an idmapped mount is identical to a given k{g,u}id_t. For an example,
see kmntgid_in_group_p() which determines whether the value of kmntgid_t
matches the value of any of the caller's groups. Similar logic is
expressed in the k{g,u}id_eq_kmnt{g,u}id().

The kmnt{g,u}id_to_k{g,u}id() helpers map a given kmnt{g,u}id_t from the
mount's idmapping into the filesystem idmapping. They make it possible
to update a filesystem object such as inode->i_{g,u}id with the correct
value.

This makes it harder to accidently write a wrong {g,u}id anwywhere.
The kmnt{g,u}id_has_mapping() helpers check whether a given
kmnt{g,u}id_t can be represented in the filesystem idmapping.

All new helpers are nops on non-idmapped mounts.

I've done work on this roughly 7 months ago but dropped it to focus on
the testsuite. Linus brought this up independently just last week and
it's time to move this along (see [1]).

[1]: https://lore.kernel.org/lkml/CAHk-=win6+ahs1EwLkcq8apqLi_1wXFWbrPf340zYEhObpz4jA@mail.gmail.com
Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 include/linux/mnt_idmapping.h | 190 ++++++++++++++++++++++++++++++++++
 1 file changed, 190 insertions(+)

diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
index ee5a217de2a8..8dbaef494e02 100644
--- a/include/linux/mnt_idmapping.h
+++ b/include/linux/mnt_idmapping.h
@@ -13,6 +13,122 @@ struct user_namespace;
  */
 extern struct user_namespace init_user_ns;
 
+typedef struct {
+	uid_t val;
+} kmntuid_t;
+
+typedef struct {
+	gid_t val;
+} kmntgid_t;
+
+#ifdef CONFIG_MULTIUSER
+static inline uid_t __kmntuid_val(kmntuid_t uid)
+{
+	return uid.val;
+}
+
+static inline gid_t __kmntgid_val(kmntgid_t gid)
+{
+	return gid.val;
+}
+#else
+static inline uid_t __kmntuid_val(kmntuid_t uid)
+{
+	return 0;
+}
+
+static inline gid_t __kmntgid_val(kmntgid_t gid)
+{
+	return 0;
+}
+#endif
+
+static inline bool kmntuid_valid(kmntuid_t uid)
+{
+	return __kmntuid_val(uid) != (uid_t) -1;
+}
+
+static inline bool kmntgid_valid(kmntgid_t gid)
+{
+	return __kmntgid_val(gid) != (gid_t) -1;
+}
+
+/**
+ * kuid_eq_kmntuid - check whether kuid and kmntuid have the same value
+ * @kuid: the kuid to compare
+ * @kmntuid: the kmntuid to compare
+ *
+ * Check whether @kuid and @kmntuid have the same values.
+ *
+ * Return: true if @kuid and @kmntuid have the same value, false if not.
+ */
+static inline bool kuid_eq_kmntuid(kuid_t kuid, kmntuid_t kmntuid)
+{
+	return __kmntuid_val(kmntuid) == __kuid_val(kuid);
+}
+
+/**
+ * kgid_eq_kmntgid - check whether kgid and kmntgid have the same value
+ * @kgid: the kgid to compare
+ * @kmntgid: the kmntgid to compare
+ *
+ * Check whether @kgid and @kmntgid have the same values.
+ *
+ * Return: true if @kgid and @kmntgid have the same value, false if not.
+ */
+static inline bool kgid_eq_kmntgid(kgid_t kgid, kmntgid_t kmntgid)
+{
+	return __kmntgid_val(kmntgid) == __kgid_val(kgid);
+}
+
+static inline bool kmntuid_eq(kmntuid_t left, kmntuid_t right)
+{
+	return __kmntuid_val(left) == __kmntuid_val(right);
+}
+
+static inline bool kmntgid_eq(kmntgid_t left, kmntgid_t right)
+{
+	return __kmntgid_val(left) == __kmntgid_val(right);
+}
+
+/*
+ * kmnt{g,u}ids are created from k{g,u}ids.
+ * We don't allow them to be created from regular {u,g}id.
+ */
+#define KMNTUIDT_INIT(val) (kmntuid_t){ __kuid_val(val) }
+#define KMNTGIDT_INIT(val) (kmntgid_t){ __kgid_val(val) }
+
+#define INVALID_KMNTUID KMNTUIDT_INIT(INVALID_UID)
+#define INVALID_KMNTGID KMNTGIDT_INIT(INVALID_GID)
+
+/*
+ * Allow a kmnt{g,u}id to be used as a k{g,u}id where we want to compare
+ * whether the mapped value is identical to value of a k{g,u}id.
+ */
+#define AS_KUIDT(val) (kuid_t){ __kmntuid_val(val) }
+#define AS_KGIDT(val) (kgid_t){ __kmntgid_val(val) }
+
+#ifdef CONFIG_MULTIUSER
+/**
+ * kmntgid_in_group_p() - check whether a kmntuid matches the caller's groups
+ * @kmntgid: the mnt gid to match
+ *
+ * This function can be used to determine whether @kmntuid matches any of the
+ * caller's groups.
+ *
+ * Return: 1 if kmntuid matches caller's groups, 0 if not.
+ */
+static inline int kmntgid_in_group_p(kmntgid_t kmntgid)
+{
+	return in_group_p(AS_KGIDT(kmntgid));
+}
+#else
+static inline int kmntgid_in_group_p(kmntgid_t kmntgid)
+{
+	return 1;
+}
+#endif
+
 /**
  * initial_idmapping - check whether this is the initial mapping
  * @ns: idmapping to check
@@ -157,6 +273,43 @@ static inline kuid_t mapped_kuid_user(struct user_namespace *mnt_userns,
 	return make_kuid(fs_userns, uid);
 }
 
+/**
+ * kmntuid_to_kuid - map a kmntuid into the filesystem idmapping
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kmntuid : kmntuid to be mapped
+ *
+ * Map @kmntuid into the filesystem idmapping. This function has to be used in
+ * order to e.g. write @kmntuid to inode->i_uid.
+ *
+ * Return: @kmntuid mapped into the filesystem idmapping
+ */
+static inline kuid_t kmntuid_to_kuid(struct user_namespace *mnt_userns,
+				     struct user_namespace *fs_userns,
+				     kmntuid_t kmntuid)
+{
+	return mapped_kuid_user(mnt_userns, fs_userns, AS_KUIDT(kmntuid));
+}
+
+/**
+ * kmntuid_has_mapping - check whether a kmntuid maps into the filesystem
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kmntuid: kmntuid to be mapped
+ *
+ * Check whether @kmntuid has a mapping in the filesystem idmapping. Use this
+ * function to check whether the filesystem idmapping has a mapping for
+ * @kmntuid.
+ *
+ * Return: true if @kmntuid has a mapping in the filesystem, false if not.
+ */
+static inline bool kmntuid_has_mapping(struct user_namespace *mnt_userns,
+				       struct user_namespace *fs_userns,
+				       kmntuid_t kmntuid)
+{
+	return uid_valid(kmntuid_to_kuid(mnt_userns, fs_userns, kmntuid));
+}
+
 /**
  * mapped_kgid_user - map a user kgid into a mnt_userns
  * @mnt_userns: the mount's idmapping
@@ -193,6 +346,43 @@ static inline kgid_t mapped_kgid_user(struct user_namespace *mnt_userns,
 	return make_kgid(fs_userns, gid);
 }
 
+/**
+ * kmntgid_to_kgid - map a kmntgid into the filesystem idmapping
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kmntgid : kmntgid to be mapped
+ *
+ * Map @kmntgid into the filesystem idmapping. This function has to be used in
+ * order to e.g. write @kmntgid to inode->i_gid.
+ *
+ * Return: @kmntgid mapped into the filesystem idmapping
+ */
+static inline kgid_t kmntgid_to_kgid(struct user_namespace *mnt_userns,
+				     struct user_namespace *fs_userns,
+				     kmntgid_t kmntgid)
+{
+	return mapped_kgid_user(mnt_userns, fs_userns, AS_KGIDT(kmntgid));
+}
+
+/**
+ * kmntgid_has_mapping - check whether a kmntgid maps into the filesystem
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @kmntgid: kmntgid to be mapped
+ *
+ * Check whether @kmntgid has a mapping in the filesystem idmapping. Use this
+ * function to check whether the filesystem idmapping has a mapping for
+ * @kmntgid.
+ *
+ * Return: true if @kmntgid has a mapping in the filesystem, false if not.
+ */
+static inline bool kmntgid_has_mapping(struct user_namespace *mnt_userns,
+				       struct user_namespace *fs_userns,
+				       kmntgid_t kmntgid)
+{
+	return gid_valid(kmntgid_to_kgid(mnt_userns, fs_userns, kmntgid));
+}
+
 /**
  * mapped_fsuid - return caller's fsuid mapped up into a mnt_userns
  * @mnt_userns: the mount's idmapping
-- 
2.34.1

[PATCH 2/8] fs: add two type safe mapping helpers

[Christian Brauner]

Introduce i_{g,u}id_into_mnt{g,u}id(). They return kmnt{g,u}id_t. This
makes it way harder to confused idmapped mount {g,u}ids with filesystem
{g,u}ids.

The two helpers will eventually replace the old non type safe
i_{g,u}id_into_mnt() helpers once we finished converting all places. Add
a comment noting that they will be removed in the future.

All new helpers are nops on non-idmapped mounts.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 include/linux/fs.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9ad5e3520fae..8724a31b95e5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1600,6 +1600,9 @@ static inline void i_gid_write(struct inode *inode, gid_t gid)
  * @mnt_userns: user namespace of the mount the inode was found from
  * @inode: inode to map
  *
+ * Note, this will eventually be removed completely in favor of the type-safe
+ * i_uid_into_mntuid().
+ *
  * 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.
  */
@@ -1609,11 +1612,28 @@ static inline kuid_t i_uid_into_mnt(struct user_namespace *mnt_userns,
 	return mapped_kuid_fs(mnt_userns, i_user_ns(inode), inode->i_uid);
 }
 
+/**
+ * i_uid_into_mntuid - 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: whe inode's i_uid mapped down according to @mnt_userns.
+ * If the inode's i_uid has no mapping INVALID_KMNTUID is returned.
+ */
+static inline kmntuid_t i_uid_into_mntuid(struct user_namespace *mnt_userns,
+					  const struct inode *inode)
+{
+	return KMNTUIDT_INIT(i_uid_into_mnt(mnt_userns, inode));
+}
+
 /**
  * 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
  *
+ * Note, this will eventually be removed completely in favor of the type-safe
+ * i_gid_into_mntgid().
+ *
  * 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.
  */
@@ -1623,6 +1643,23 @@ static inline kgid_t i_gid_into_mnt(struct user_namespace *mnt_userns,
 	return mapped_kgid_fs(mnt_userns, i_user_ns(inode), inode->i_gid);
 }
 
+/**
+ * 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
+ *
+ * Note, this will eventually be removed completely in favor of the type-safe
+ * i_gid_into_mntgid().
+ *
+ * Return: the inode's i_gid mapped down according to @mnt_userns.
+ * If the inode's i_gid has no mapping INVALID_KMNTGID is returned.
+ */
+static inline kmntgid_t i_gid_into_mntgid(struct user_namespace *mnt_userns,
+					  const struct inode *inode)
+{
+	return KMNTGIDT_INIT(i_gid_into_mnt(mnt_userns, inode));
+}
+
 /**
  * inode_fsuid_set - initialize inode's i_uid field with callers fsuid
  * @inode: inode to initialize
-- 
2.34.1

[PATCH 3/8] fs: use mount types in iattr

[Christian Brauner]

Add ia_mnt{g,u}id members of type kmnt{g,u}id_t to struct iattr. We use
an anonymous union (similar to what we do in struct file) around
ia_{g,u}id and ia_mnt{g,u}id.

At the end of this series ia_{g,u}id and ia_mnt{g,u}id will always
contain the same value independent of whether struct iattr is
initialized from an idmapped mount. This is a change from how this is
done today.

Wrapping this in a anonymous unions has a few advantages. It allows us
to avoid needlessly increasing struct iattr. Since the types for
ia_{g,u}id and ia_mnt{g,u}id are structures with overlapping/identical
members they are covered by 6.5.2.3/6 of the C standard and it is safe
to initialize and access them.

Filesystems that raise FS_ALLOW_IDMAP and thus support idmapped mounts
will have to use ia_mnt{g,u}id and the associated helpers. And will be
ported at the end of this series. They will immediately benefit from the
type safe new helpers.

Filesystems that do not support FS_ALLOW_IDMAP can continue to use
ia_{g,u}id for now. The aim is to convert every filesystem to always use
ia_mnt{g,u}id and thus ultimately remove the ia_{g,u}id members.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 include/linux/fs.h            | 18 ++++++++++++++++--
 include/linux/mnt_idmapping.h |  5 +++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8724a31b95e5..0da6c0481dbd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -221,8 +221,22 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 struct iattr {
 	unsigned int	ia_valid;
 	umode_t		ia_mode;
-	kuid_t		ia_uid;
-	kgid_t		ia_gid;
+	/*
+	 * The two anonymous unions wrap structures with the same member.
+	 *
+	 * Filesystems raising FS_ALLOW_IDMAP need to use ia_mnt{g,u}id which
+	 * are a dedicated type requiring the filesystem to use the dedicated
+	 * helpers. Other filesystem can continue to use ia_{g,u}id until they
+	 * have been ported.
+	 */
+	union {
+		kuid_t		ia_uid;
+		kmntuid_t	ia_mntuid;
+	};
+	union {
+		kgid_t		ia_gid;
+		kmntgid_t	ia_mntgid;
+	};
 	loff_t		ia_size;
 	struct timespec64 ia_atime;
 	struct timespec64 ia_mtime;
diff --git a/include/linux/mnt_idmapping.h b/include/linux/mnt_idmapping.h
index 8dbaef494e02..8f555c746cf4 100644
--- a/include/linux/mnt_idmapping.h
+++ b/include/linux/mnt_idmapping.h
@@ -21,6 +21,11 @@ typedef struct {
 	gid_t val;
 } kmntgid_t;
 
+static_assert(sizeof(kmntuid_t) == sizeof(kuid_t));
+static_assert(sizeof(kmntgid_t) == sizeof(kgid_t));
+static_assert(offsetof(kmntuid_t, val) == offsetof(kuid_t, val));
+static_assert(offsetof(kmntgid_t, val) == offsetof(kgid_t, val));
+
 #ifdef CONFIG_MULTIUSER
 static inline uid_t __kmntuid_val(kmntuid_t uid)
 {
-- 
2.34.1

[PATCH 4/8] fs: introduce tiny iattr ownership update helpers

[Christian Brauner]

Nearly all fileystems currently open-code the same checks for
determining whether the i{g,u}_id fields of an inode need to be updated
and then updating the fields.

Introduce tiny helpers i_{g,u}id_needs_update() and i_{g,u}id_update()
that wrap this logic. This allows filesystems to not care updating
inode->i_{g,u}id with the correct values themselves instead leaving this
to the helpers.

We also get rid of a lot of code duplication and make it easier to
change struct iattr in the future since changes can be localized to
these helpers.

And finally we make it hard to conflate k{g,u}id_t types with
kmnt{g,u}id_t types for filesystems that support idmapped mounts.

In the following patch we will port all filesystems that raise
FS_ALLOW_IDMAP to use the new helpers. However, the ultimate goal is to
convert all filesystems to make use of these helpers.

All new helpers are nops on non-idmapped mounts.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 include/linux/fs.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0da6c0481dbd..998ac36ea7b0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1640,6 +1640,44 @@ static inline kmntuid_t i_uid_into_mntuid(struct user_namespace *mnt_userns,
 	return KMNTUIDT_INIT(i_uid_into_mnt(mnt_userns, inode));
 }
 
+/**
+ * i_uid_needs_update - check whether inode's i_uid needs to be updated
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @attr: the new attributes of @inode
+ * @inode: the inode to update
+ *
+ * Check whether the $inode's i_uid field needs to be updated taking idmapped
+ * mounts into account if the filesystem supports it.
+ *
+ * Return: true if @inode's i_uid field needs to be updated, false if not.
+ */
+static inline bool i_uid_needs_update(struct user_namespace *mnt_userns,
+				      const struct iattr *attr,
+				      const struct inode *inode)
+{
+	return ((attr->ia_valid & ATTR_UID) &&
+		!kmntuid_eq(attr->ia_mntuid,
+			    i_uid_into_mntuid(mnt_userns, inode)));
+}
+
+/**
+ * i_uid_update - update @inode's i_uid field
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @attr: the new attributes of @inode
+ * @inode: the inode to update
+ *
+ * Safely update @inode's i_uid field translating the kmntuid of any idmapped
+ * mount into the filesystem kuid.
+ */
+static inline void i_uid_update(struct user_namespace *mnt_userns,
+				const struct iattr *attr,
+				struct inode *inode)
+{
+	if (attr->ia_valid & ATTR_UID)
+		inode->i_uid = kmntuid_to_kuid(mnt_userns, i_user_ns(inode),
+					       attr->ia_mntuid);
+}
+
 /**
  * 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
@@ -1674,6 +1712,44 @@ static inline kmntgid_t i_gid_into_mntgid(struct user_namespace *mnt_userns,
 	return KMNTGIDT_INIT(i_gid_into_mnt(mnt_userns, inode));
 }
 
+/**
+ * i_gid_needs_update - check whether inode's i_gid needs to be updated
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @attr: the new attributes of @inode
+ * @inode: the inode to update
+ *
+ * Check whether the $inode's i_gid field needs to be updated taking idmapped
+ * mounts into account if the filesystem supports it.
+ *
+ * Return: true if @inode's i_gid field needs to be updated, false if not.
+ */
+static inline bool i_gid_needs_update(struct user_namespace *mnt_userns,
+				      const struct iattr *attr,
+				      const struct inode *inode)
+{
+	return ((attr->ia_valid & ATTR_GID) &&
+		!kmntgid_eq(attr->ia_mntgid,
+			    i_gid_into_mntgid(mnt_userns, inode)));
+}
+
+/**
+ * i_gid_update - update @inode's i_gid field
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @attr: the new attributes of @inode
+ * @inode: the inode to update
+ *
+ * Safely update @inode's i_gid field translating the kmntgid of any idmapped
+ * mount into the filesystem kgid.
+ */
+static inline void i_gid_update(struct user_namespace *mnt_userns,
+				const struct iattr *attr,
+				struct inode *inode)
+{
+	if (attr->ia_valid & ATTR_GID)
+		inode->i_gid = kmntgid_to_kgid(mnt_userns, i_user_ns(inode),
+					       attr->ia_mntgid);
+}
+
 /**
  * inode_fsuid_set - initialize inode's i_uid field with callers fsuid
  * @inode: inode to initialize
-- 
2.34.1

[PATCH 5/8] fs: port to iattr ownership update helpers

[Christian Brauner]

Earlier we introduce tiny new helpers to abstract ownership update and
remove code duplications. This converts all filesystems supporting
idmapped mounts to make use of these new helpers.

For now we always pass the initial idmapping which makes the idmapping
functions these helpers call nops.

This is done because we currently always pass the actual value to be
written to i_{g,u}id via struct iattr. While this allowed us to treat
the {g,u}id values in struct iattr as values that can be directly
written to inode->i_{g,u}id it also increases the potential for
confusion for filesystems.

Now that we are about to introduce dedicated types to prevent this
confusion we will ultimately only map the value from the idmapped mount
into a filesystem value that can be written to inode->i_{g,u}id when the
filesystem actually updates the inode. So pass down the initial
idmapping until we finished that conversion.

No functional changes intended.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/attr.c                         |  6 ++----
 fs/ext2/inode.c                   |  4 ++--
 fs/ext4/inode.c                   | 10 ++++------
 fs/f2fs/file.c                    | 18 ++++++------------
 fs/quota/dquot.c                  |  4 ++--
 fs/xfs/xfs_iops.c                 |  8 ++++----
 include/linux/quotaops.h          |  6 +++---
 security/integrity/evm/evm_main.c |  4 ++--
 8 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index dbe996b0dedf..2e180dd9460f 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -242,10 +242,8 @@ void setattr_copy(struct user_namespace *mnt_userns, struct inode *inode,
 {
 	unsigned int ia_valid = attr->ia_valid;
 
-	if (ia_valid & ATTR_UID)
-		inode->i_uid = attr->ia_uid;
-	if (ia_valid & ATTR_GID)
-		inode->i_gid = attr->ia_gid;
+	i_uid_update(&init_user_ns, attr, inode);
+	i_gid_update(&init_user_ns, attr, inode);
 	if (ia_valid & ATTR_ATIME)
 		inode->i_atime = attr->ia_atime;
 	if (ia_valid & ATTR_MTIME)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index e6b932219803..6dc66ab97d20 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1684,8 +1684,8 @@ int ext2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		if (error)
 			return error;
 	}
-	if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
-	    (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, inode->i_gid))) {
+	if (i_uid_needs_update(&init_user_ns, iattr, inode) ||
+	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
 		error = dquot_transfer(inode, iattr);
 		if (error)
 			return error;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 84c0eb55071d..05d932f81c53 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5356,8 +5356,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 			return error;
 	}
 
-	if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
-	    (ia_valid & ATTR_GID && !gid_eq(attr->ia_gid, inode->i_gid))) {
+	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
+	    i_gid_needs_update(&init_user_ns, attr, inode)) {
 		handle_t *handle;
 
 		/* (user+group)*(old+new) structure, inode write (sb,
@@ -5383,10 +5383,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		}
 		/* Update corresponding info in inode so that everything is in
 		 * one transaction */
-		if (attr->ia_valid & ATTR_UID)
-			inode->i_uid = attr->ia_uid;
-		if (attr->ia_valid & ATTR_GID)
-			inode->i_gid = attr->ia_gid;
+		i_uid_update(&init_user_ns, attr, inode);
+		i_gid_update(&init_user_ns, attr, inode);
 		error = ext4_mark_inode_dirty(handle, inode);
 		ext4_journal_stop(handle);
 		if (unlikely(error)) {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index bd14cef1b08f..a35d6b12bd63 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -861,10 +861,8 @@ static void __setattr_copy(struct user_namespace *mnt_userns,
 {
 	unsigned int ia_valid = attr->ia_valid;
 
-	if (ia_valid & ATTR_UID)
-		inode->i_uid = attr->ia_uid;
-	if (ia_valid & ATTR_GID)
-		inode->i_gid = attr->ia_gid;
+	i_uid_update(&init_user_ns, attr, inode);
+	i_gid_update(&init_user_ns, attr, inode);
 	if (ia_valid & ATTR_ATIME)
 		inode->i_atime = attr->ia_atime;
 	if (ia_valid & ATTR_MTIME)
@@ -922,10 +920,8 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		if (err)
 			return err;
 	}
-	if ((attr->ia_valid & ATTR_UID &&
-		!uid_eq(attr->ia_uid, inode->i_uid)) ||
-		(attr->ia_valid & ATTR_GID &&
-		!gid_eq(attr->ia_gid, inode->i_gid))) {
+	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
+	    i_gid_needs_update(&init_user_ns, attr, inode)) {
 		f2fs_lock_op(F2FS_I_SB(inode));
 		err = dquot_transfer(inode, attr);
 		if (err) {
@@ -938,10 +934,8 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		 * update uid/gid under lock_op(), so that dquot and inode can
 		 * be updated atomically.
 		 */
-		if (attr->ia_valid & ATTR_UID)
-			inode->i_uid = attr->ia_uid;
-		if (attr->ia_valid & ATTR_GID)
-			inode->i_gid = attr->ia_gid;
+		i_uid_update(&init_user_ns, attr, inode);
+		i_gid_update(&init_user_ns, attr, inode);
 		f2fs_mark_inode_dirty_sync(inode, true);
 		f2fs_unlock_op(F2FS_I_SB(inode));
 	}
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 09d1307959d0..6cec2bfbf51b 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2095,7 +2095,7 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
 	if (!dquot_active(inode))
 		return 0;
 
-	if (iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)){
+	if (i_uid_needs_update(&init_user_ns, iattr, inode)) {
 		dquot = dqget(sb, make_kqid_uid(iattr->ia_uid));
 		if (IS_ERR(dquot)) {
 			if (PTR_ERR(dquot) != -ESRCH) {
@@ -2106,7 +2106,7 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
 		}
 		transfer_to[USRQUOTA] = dquot;
 	}
-	if (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, inode->i_gid)){
+	if (i_gid_needs_update(&init_user_ns, iattr, inode)) {
 		dquot = dqget(sb, make_kqid_gid(iattr->ia_gid));
 		if (IS_ERR(dquot)) {
 			if (PTR_ERR(dquot) != -ESRCH) {
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 29f5b8b8aca6..31ec29565fb4 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -704,13 +704,13 @@ xfs_setattr_nonsize(
 	 * didn't have the inode locked, inode's dquot(s) would have changed
 	 * also.
 	 */
-	if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp) &&
-	    !uid_eq(inode->i_uid, iattr->ia_uid)) {
+	if (XFS_IS_UQUOTA_ON(mp) &&
+	    i_uid_needs_update(&init_user_ns, iattr, inode)) {
 		ASSERT(udqp);
 		old_udqp = xfs_qm_vop_chown(tp, ip, &ip->i_udquot, udqp);
 	}
-	if ((mask & ATTR_GID) && XFS_IS_GQUOTA_ON(mp) &&
-	    !gid_eq(inode->i_gid, iattr->ia_gid)) {
+	if (XFS_IS_GQUOTA_ON(mp) &&
+	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
 		ASSERT(xfs_has_pquotino(mp) || !XFS_IS_PQUOTA_ON(mp));
 		ASSERT(gdqp);
 		old_gdqp = xfs_qm_vop_chown(tp, ip, &ip->i_gdquot, gdqp);
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index a0f6668924d3..61ee34861ca2 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -22,9 +22,9 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
 /* i_mutex must being held */
 static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
 {
-	return (ia->ia_valid & ATTR_SIZE) ||
-		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
-		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
+	return ((ia->ia_valid & ATTR_SIZE) ||
+		i_uid_needs_update(&init_user_ns, ia, inode) ||
+		i_gid_needs_update(&init_user_ns, ia, inode));
 }
 
 #if defined(CONFIG_QUOTA)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cc88f02c7562..bcde6bc2a2ce 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -760,8 +760,8 @@ static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
 	struct inode *inode = d_backing_inode(dentry);
 	unsigned int ia_valid = attr->ia_valid;
 
-	if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
-	    (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
+	if (!i_uid_needs_update(&init_user_ns, attr, inode) &&
+	    !i_gid_needs_update(&init_user_ns, attr, inode) &&
 	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
 		return 0;
 
-- 
2.34.1

[PATCH 6/8] quota: port quota helpers mount ids

[Christian Brauner]

Port the is_quota_modification() and dqout_transfer() helper to type
safe kmnt{g,u}id_t. Since these helpers are only called by a few
filesystems don't introduce a new helper but simply extend the existing
helpers to pass down the mount's idmapping.

Note, that this is a non-functional change, i.e. nothing will have
happened here or at the end of this series to how quota are done! This
a change necessary because we will at the end of this series make
ownership changes easier to reason about by keeping the original value
in struct iattr for both non-idmapped and idmapped mounts.

For now we always pass the initial idmapping which makes the idmapping
functions these helpers call nops.

This is done because we currently always pass the actual value to be
written to i_{g,u}id via struct iattr. While this allowed us to treat
the {g,u}id values in struct iattr as values that can be directly
written to inode->i_{g,u}id it also increases the potential for
confusion for filesystems.

Now that we are about to introduce dedicated types to prevent this
confusion we will ultimately only map the value from the idmapped mount
into a filesystem value that can be written to inode->i_{g,u}id when the
filesystem actually updates the inode. So pass down the initial
idmapping until we finished that conversion.

Since struct iattr uses an anonymous union with overlapping types as
supported by the C filesystems that haven't converted to ia_mnt{g,u}id
won't see any difference and things will continue to work as before.
In other words, no functional changes intended with this change.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Jan Kara <jack@suse.cz>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/ext2/inode.c          | 4 ++--
 fs/ext4/inode.c          | 4 ++--
 fs/f2fs/file.c           | 4 ++--
 fs/f2fs/recovery.c       | 2 +-
 fs/jfs/file.c            | 4 ++--
 fs/ocfs2/file.c          | 2 +-
 fs/quota/dquot.c         | 3 ++-
 fs/reiserfs/inode.c      | 4 ++--
 fs/zonefs/super.c        | 2 +-
 include/linux/quotaops.h | 9 ++++++---
 10 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 6dc66ab97d20..593b79416e0e 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1679,14 +1679,14 @@ int ext2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, iattr)) {
+	if (is_quota_modification(&init_user_ns, inode, iattr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
 	}
 	if (i_uid_needs_update(&init_user_ns, iattr, inode) ||
 	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
-		error = dquot_transfer(inode, iattr);
+		error = dquot_transfer(&init_user_ns, inode, iattr);
 		if (error)
 			return error;
 	}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 05d932f81c53..72f08c184768 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5350,7 +5350,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (error)
 		return error;
 
-	if (is_quota_modification(inode, attr)) {
+	if (is_quota_modification(&init_user_ns, inode, attr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
@@ -5374,7 +5374,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		 * counts xattr inode references.
 		 */
 		down_read(&EXT4_I(inode)->xattr_sem);
-		error = dquot_transfer(inode, attr);
+		error = dquot_transfer(&init_user_ns, inode, attr);
 		up_read(&EXT4_I(inode)->xattr_sem);
 
 		if (error) {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index a35d6b12bd63..02b2d56d4edc 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -915,7 +915,7 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (err)
 		return err;
 
-	if (is_quota_modification(inode, attr)) {
+	if (is_quota_modification(&init_user_ns, inode, attr)) {
 		err = f2fs_dquot_initialize(inode);
 		if (err)
 			return err;
@@ -923,7 +923,7 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
 	    i_gid_needs_update(&init_user_ns, attr, inode)) {
 		f2fs_lock_op(F2FS_I_SB(inode));
-		err = dquot_transfer(inode, attr);
+		err = dquot_transfer(&init_user_ns, inode, attr);
 		if (err) {
 			set_sbi_flag(F2FS_I_SB(inode),
 					SBI_QUOTA_NEED_REPAIR);
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 3cb7f8a43b4d..8e5a089f1ac8 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -266,7 +266,7 @@ static int recover_quota_data(struct inode *inode, struct page *page)
 	if (!attr.ia_valid)
 		return 0;
 
-	err = dquot_transfer(inode, &attr);
+	err = dquot_transfer(&init_user_ns, inode, &attr);
 	if (err)
 		set_sbi_flag(F2FS_I_SB(inode), SBI_QUOTA_NEED_REPAIR);
 	return err;
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index 1d732fd223d4..c18569b9895d 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -95,14 +95,14 @@ int jfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (rc)
 		return rc;
 
-	if (is_quota_modification(inode, iattr)) {
+	if (is_quota_modification(&init_user_ns, inode, iattr)) {
 		rc = dquot_initialize(inode);
 		if (rc)
 			return rc;
 	}
 	if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
 	    (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, inode->i_gid))) {
-		rc = dquot_transfer(inode, iattr);
+		rc = dquot_transfer(&init_user_ns, inode, iattr);
 		if (rc)
 			return rc;
 	}
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 7497cd592258..0e09cd8911da 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1146,7 +1146,7 @@ int ocfs2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (status)
 		return status;
 
-	if (is_quota_modification(inode, attr)) {
+	if (is_quota_modification(&init_user_ns, inode, attr)) {
 		status = dquot_initialize(inode);
 		if (status)
 			return status;
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 6cec2bfbf51b..df9af1ce2851 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2085,7 +2085,8 @@ EXPORT_SYMBOL(__dquot_transfer);
 /* Wrapper for transferring ownership of an inode for uid/gid only
  * Called from FSXXX_setattr()
  */
-int dquot_transfer(struct inode *inode, struct iattr *iattr)
+int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
+		   struct iattr *iattr)
 {
 	struct dquot *transfer_to[MAXQUOTAS] = {};
 	struct dquot *dquot;
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 0cffe054b78e..1e89e76972a0 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3284,7 +3284,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	/* must be turned off for recursive notify_change calls */
 	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
 
-	if (is_quota_modification(inode, attr)) {
+	if (is_quota_modification(&init_user_ns, inode, attr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
@@ -3367,7 +3367,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		reiserfs_write_unlock(inode->i_sb);
 		if (error)
 			goto out;
-		error = dquot_transfer(inode, attr);
+		error = dquot_transfer(&init_user_ns, inode, attr);
 		reiserfs_write_lock(inode->i_sb);
 		if (error) {
 			journal_end(&th);
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 053299758deb..dd422b1d7320 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -616,7 +616,7 @@ static int zonefs_inode_setattr(struct user_namespace *mnt_userns,
 	     !uid_eq(iattr->ia_uid, inode->i_uid)) ||
 	    ((iattr->ia_valid & ATTR_GID) &&
 	     !gid_eq(iattr->ia_gid, inode->i_gid))) {
-		ret = dquot_transfer(inode, iattr);
+		ret = dquot_transfer(&init_user_ns, inode, iattr);
 		if (ret)
 			return ret;
 	}
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index 61ee34861ca2..0342ff6584fd 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -20,7 +20,8 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
 }
 
 /* i_mutex must being held */
-static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
+static inline bool is_quota_modification(struct user_namespace *mnt_userns,
+					 struct inode *inode, struct iattr *ia)
 {
 	return ((ia->ia_valid & ATTR_SIZE) ||
 		i_uid_needs_update(&init_user_ns, ia, inode) ||
@@ -115,7 +116,8 @@ int dquot_set_dqblk(struct super_block *sb, struct kqid id,
 		struct qc_dqblk *di);
 
 int __dquot_transfer(struct inode *inode, struct dquot **transfer_to);
-int dquot_transfer(struct inode *inode, struct iattr *iattr);
+int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
+		   struct iattr *iattr);
 
 static inline struct mem_dqinfo *sb_dqinfo(struct super_block *sb, int type)
 {
@@ -234,7 +236,8 @@ static inline void dquot_free_inode(struct inode *inode)
 {
 }
 
-static inline int dquot_transfer(struct inode *inode, struct iattr *iattr)
+static inline int dquot_transfer(struct user_namespace *mnt_userns,
+				 struct inode *inode, struct iattr *iattr)
 {
 	return 0;
 }
-- 
2.34.1

[PATCH 7/8] security: pass down mount idmapping to setattr hook

[Christian Brauner]

Before this change we used to take a shortcut and place the actual
values that would be written to inode->i_{g,u}id into struct iattr. This
had the advantage that we moved idmappings mostly out of the picture
early on but it made reasoning about changes more difficult than it
should be.

The filesystem was never explicitly told that it dealt with an idmapped
mount. The transition to the value that needed to be stored in
inode->i_{g,u}id appeared way too early and increased the probability of
bugs in various codepaths.

We know place the same value in struct iattr no matter if this is an
idmapped mount or not. The vfs will only deal with type safe
kmnt{g,u}id_t. This makes it massively safer to perform permission
checks as the type will tell us what checks we need to perform and what
helpers we need to use.

Adapt the security_inode_setattr() helper to pass down the mount's
idmapping to account for that change.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/attr.c                         | 2 +-
 fs/fat/file.c                     | 3 ++-
 include/linux/evm.h               | 6 ++++--
 include/linux/security.h          | 8 +++++---
 security/integrity/evm/evm_main.c | 8 +++++---
 security/security.c               | 5 +++--
 6 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 2e180dd9460f..88e2ca30d42e 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -411,7 +411,7 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
 	    !gid_valid(i_gid_into_mnt(mnt_userns, inode)))
 		return -EOVERFLOW;
 
-	error = security_inode_setattr(dentry, attr);
+	error = security_inode_setattr(&init_user_ns, dentry, attr);
 	if (error)
 		return error;
 	error = try_break_deleg(inode, delegated_inode);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 3dae3ed60f3a..530f18173db2 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -90,7 +90,8 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
 	 * out the RO attribute for checking by the security
 	 * module, just because it maps to a file mode.
 	 */
-	err = security_inode_setattr(file->f_path.dentry, &ia);
+	err = security_inode_setattr(&init_user_ns,
+				     file->f_path.dentry, &ia);
 	if (err)
 		goto out_unlock_inode;
 
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 4c374be70247..aa63e0b3c0a2 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -21,7 +21,8 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
 					     void *xattr_value,
 					     size_t xattr_value_len,
 					     struct integrity_iint_cache *iint);
-extern int evm_inode_setattr(struct dentry *dentry, struct iattr *attr);
+extern int evm_inode_setattr(struct user_namespace *mnt_userns,
+			     struct dentry *dentry, struct iattr *attr);
 extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid);
 extern int evm_inode_setxattr(struct user_namespace *mnt_userns,
 			      struct dentry *dentry, const char *name,
@@ -68,7 +69,8 @@ static inline enum integrity_status evm_verifyxattr(struct dentry *dentry,
 }
 #endif
 
-static inline int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
+static inline int evm_inode_setattr(struct user_namespace *mnt_userns,
+				    struct dentry *dentry, struct iattr *attr)
 {
 	return 0;
 }
diff --git a/include/linux/security.h b/include/linux/security.h
index 7fc4e9f49f54..4d0baf30266e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -353,7 +353,8 @@ int security_inode_readlink(struct dentry *dentry);
 int security_inode_follow_link(struct dentry *dentry, struct inode *inode,
 			       bool rcu);
 int security_inode_permission(struct inode *inode, int mask);
-int security_inode_setattr(struct dentry *dentry, struct iattr *attr);
+int security_inode_setattr(struct user_namespace *mnt_userns,
+			   struct dentry *dentry, struct iattr *attr);
 int security_inode_getattr(const struct path *path);
 int security_inode_setxattr(struct user_namespace *mnt_userns,
 			    struct dentry *dentry, const char *name,
@@ -848,8 +849,9 @@ static inline int security_inode_permission(struct inode *inode, int mask)
 	return 0;
 }
 
-static inline int security_inode_setattr(struct dentry *dentry,
-					  struct iattr *attr)
+static inline int security_inode_setattr(struct user_namespace *mnt_userns,
+					 struct dentry *dentry,
+					 struct iattr *attr)
 {
 	return 0;
 }
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index bcde6bc2a2ce..7f4af5b58583 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -755,7 +755,8 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
 	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
 }
 
-static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
+static int evm_attr_change(struct user_namespace *mnt_userns,
+			   struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	unsigned int ia_valid = attr->ia_valid;
@@ -775,7 +776,8 @@ static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
  * Permit update of file attributes when files have a valid EVM signature,
  * except in the case of them having an immutable portable signature.
  */
-int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
+int evm_inode_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
+		      struct iattr *attr)
 {
 	unsigned int ia_valid = attr->ia_valid;
 	enum integrity_status evm_status;
@@ -801,7 +803,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 		return 0;
 
 	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
-	    !evm_attr_change(dentry, attr))
+	    !evm_attr_change(mnt_userns, dentry, attr))
 		return 0;
 
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
diff --git a/security/security.c b/security/security.c
index 188b8f782220..f85afb02ea1c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1324,7 +1324,8 @@ int security_inode_permission(struct inode *inode, int mask)
 	return call_int_hook(inode_permission, 0, inode, mask);
 }
 
-int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
+int security_inode_setattr(struct user_namespace *mnt_userns,
+			   struct dentry *dentry, struct iattr *attr)
 {
 	int ret;
 
@@ -1333,7 +1334,7 @@ int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
 	ret = call_int_hook(inode_setattr, 0, dentry, attr);
 	if (ret)
 		return ret;
-	return evm_inode_setattr(dentry, attr);
+	return evm_inode_setattr(mnt_userns, dentry, attr);
 }
 EXPORT_SYMBOL_GPL(security_inode_setattr);
 
-- 
2.34.1

[PATCH 8/8] attr: port attribute changes to new types

[Christian Brauner]

Now that we introduced new infrastructure to increase the type safety
for filesystems supporting idmapped mounts port the first part of the
vfs over to them.

This ports the attribute changes codepaths to rely on the new better
helpers using a dedicated type.

Before this change we used to take a shortcut and place the actual
values that would be written to inode->i_{g,u}id into struct iattr. This
had the advantage that we moved idmappings mostly out of the picture
early on but it made reasoning about changes more difficult than it
should be.

The filesystem was never explicitly told that it dealt with an idmapped
mount. The transition to the value that needed to be stored in
inode->i_{g,u}id appeared way too early and increased the probability of
bugs in various codepaths.

We know place the same value in struct iattr no matter if this is an
idmapped mount or not. The vfs will only deal with type safe
kmnt{g,u}id_t. This makes it massively safer to perform permission
checks as the type will tell us what checks we need to perform and what
helpers we need to use. Any fileystem raising FS_ALLOW_IDMAP can't
simply write ia_mnt{g,u}id to inode->i_{g,u}id since they are different
types. Instead they need to use the dedicated kmnt{g,u}id_to_k{g,u}id()
helpers that map the kmnt{g,u}id into the filesystem.

The other nice effect is that filesystems like overlayfs don't need to
care about idmappings explicitly anymore and can simply set up struct
iattr accordingly.

Link: https://lore.kernel.org/lkml/CAHk-=win6+ahs1EwLkcq8apqLi_1wXFWbrPf340zYEhObpz4jA@mail.gmail.com [1]
Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/attr.c                         | 67 +++++++++++++++----------------
 fs/ext2/inode.c                   |  8 ++--
 fs/ext4/inode.c                   | 12 +++---
 fs/f2fs/file.c                    | 16 ++++----
 fs/fat/file.c                     |  6 +--
 fs/jfs/file.c                     |  4 +-
 fs/ocfs2/file.c                   |  2 +-
 fs/open.c                         | 65 +++++++++++++++++++++++-------
 fs/overlayfs/copy_up.c            |  4 +-
 fs/overlayfs/overlayfs.h          | 12 +-----
 fs/quota/dquot.c                  | 14 +++++--
 fs/reiserfs/inode.c               |  4 +-
 fs/xfs/xfs_iops.c                 |  7 ++--
 fs/zonefs/super.c                 |  2 +-
 include/linux/fs.h                |  4 ++
 include/linux/quotaops.h          |  4 +-
 security/integrity/evm/evm_main.c |  4 +-
 17 files changed, 134 insertions(+), 101 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 88e2ca30d42e..20ff225a80d3 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -32,14 +32,15 @@
  */
 static bool chown_ok(struct user_namespace *mnt_userns,
 		     const struct inode *inode,
-		     kuid_t uid)
+		     kmntuid_t ia_mntuid)
 {
-	kuid_t kuid = i_uid_into_mnt(mnt_userns, inode);
-	if (uid_eq(current_fsuid(), kuid) && uid_eq(uid, inode->i_uid))
+	kmntuid_t kmntuid = i_uid_into_mntuid(mnt_userns, inode);
+	if (kuid_eq_kmntuid(current_fsuid(), kmntuid) &&
+	    kmntuid_eq(ia_mntuid, kmntuid))
 		return true;
 	if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_CHOWN))
 		return true;
-	if (uid_eq(kuid, INVALID_UID) &&
+	if (kmntuid_eq(kmntuid, INVALID_KMNTUID) &&
 	    ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN))
 		return true;
 	return false;
@@ -58,21 +59,19 @@ static bool chown_ok(struct user_namespace *mnt_userns,
  * performed on the raw inode simply passs init_user_ns.
  */
 static bool chgrp_ok(struct user_namespace *mnt_userns,
-		     const struct inode *inode, kgid_t gid)
+		     const struct inode *inode, kmntgid_t ia_mntgid)
 {
-	kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
-	if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode))) {
-		kgid_t mapped_gid;
-
-		if (gid_eq(gid, inode->i_gid))
+	kmntgid_t kmntgid = i_gid_into_mntgid(mnt_userns, inode);
+	kmntuid_t kmntuid = i_uid_into_mntuid(mnt_userns, inode);
+	if (kuid_eq_kmntuid(current_fsuid(), kmntuid)) {
+		if (kmntgid_eq(ia_mntgid, kmntgid))
 			return true;
-		mapped_gid = mapped_kgid_fs(mnt_userns, i_user_ns(inode), gid);
-		if (in_group_p(mapped_gid))
+		if (kmntgid_in_group_p(ia_mntgid))
 			return true;
 	}
 	if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_CHOWN))
 		return true;
-	if (gid_eq(kgid, INVALID_GID) &&
+	if (kmntgid_eq(ia_mntgid, INVALID_KMNTGID) &&
 	    ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN))
 		return true;
 	return false;
@@ -120,28 +119,29 @@ int setattr_prepare(struct user_namespace *mnt_userns, struct dentry *dentry,
 		goto kill_priv;
 
 	/* Make sure a caller can chown. */
-	if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, inode, attr->ia_uid))
+	if ((ia_valid & ATTR_UID) &&
+	    !chown_ok(mnt_userns, inode, attr->ia_mntuid))
 		return -EPERM;
 
 	/* Make sure caller can chgrp. */
-	if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, inode, attr->ia_gid))
+	if ((ia_valid & ATTR_GID) &&
+	    !chgrp_ok(mnt_userns, inode, attr->ia_mntgid))
 		return -EPERM;
 
 	/* Make sure a caller can chmod. */
 	if (ia_valid & ATTR_MODE) {
-		kgid_t mapped_gid;
+		kmntgid_t kmntgid;
 
 		if (!inode_owner_or_capable(mnt_userns, inode))
 			return -EPERM;
 
 		if (ia_valid & ATTR_GID)
-			mapped_gid = mapped_kgid_fs(mnt_userns,
-						i_user_ns(inode), attr->ia_gid);
+			kmntgid = attr->ia_mntgid;
 		else
-			mapped_gid = i_gid_into_mnt(mnt_userns, inode);
+			kmntgid = i_gid_into_mntgid(mnt_userns, inode);
 
 		/* Also check the setgid bit! */
-		if (!in_group_p(mapped_gid) &&
+		if (!kmntgid_in_group_p(kmntgid) &&
 		    !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
 			attr->ia_mode &= ~S_ISGID;
 	}
@@ -219,9 +219,7 @@ EXPORT_SYMBOL(inode_newsize_ok);
  * setattr_copy must be called with i_mutex held.
  *
  * setattr_copy updates the inode's metadata with that specified
- * in attr on idmapped mounts. If file ownership is changed setattr_copy
- * doesn't map ia_uid and ia_gid. It will asssume the caller has already
- * provided the intended values. Necessary permission checks to determine
+ * in attr on idmapped mounts. Necessary permission checks to determine
  * whether or not the S_ISGID property needs to be removed are performed with
  * the correct idmapped mount permission helpers.
  * Noticeably missing is inode size update, which is more complex
@@ -242,8 +240,8 @@ void setattr_copy(struct user_namespace *mnt_userns, struct inode *inode,
 {
 	unsigned int ia_valid = attr->ia_valid;
 
-	i_uid_update(&init_user_ns, attr, inode);
-	i_gid_update(&init_user_ns, attr, inode);
+	i_uid_update(mnt_userns, attr, inode);
+	i_gid_update(mnt_userns, attr, inode);
 	if (ia_valid & ATTR_ATIME)
 		inode->i_atime = attr->ia_atime;
 	if (ia_valid & ATTR_MTIME)
@@ -252,8 +250,8 @@ void setattr_copy(struct user_namespace *mnt_userns, struct inode *inode,
 		inode->i_ctime = attr->ia_ctime;
 	if (ia_valid & ATTR_MODE) {
 		umode_t mode = attr->ia_mode;
-		kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
-		if (!in_group_p(kgid) &&
+		kmntgid_t kmntgid = i_gid_into_mntgid(mnt_userns, inode);
+		if (!kmntgid_in_group_p(kmntgid) &&
 		    !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
 			mode &= ~S_ISGID;
 		inode->i_mode = mode;
@@ -304,9 +302,6 @@ EXPORT_SYMBOL(may_setattr);
  * retry.  Because breaking a delegation may take a long time, the
  * caller should drop the i_mutex before doing so.
  *
- * If file ownership is changed notify_change() doesn't map ia_uid and
- * ia_gid. It will asssume the caller has already provided the intended values.
- *
  * Alternatively, a caller may pass NULL for delegated_inode.  This may
  * be appropriate for callers that expect the underlying filesystem not
  * to be NFS exported.  Also, passing NULL is fine for callers holding
@@ -395,23 +390,25 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
 	 * namespace of the superblock.
 	 */
 	if (ia_valid & ATTR_UID &&
-	    !kuid_has_mapping(inode->i_sb->s_user_ns, attr->ia_uid))
+	    !kmntuid_has_mapping(mnt_userns, inode->i_sb->s_user_ns,
+				 attr->ia_mntuid))
 		return -EOVERFLOW;
 	if (ia_valid & ATTR_GID &&
-	    !kgid_has_mapping(inode->i_sb->s_user_ns, attr->ia_gid))
+	    !kmntgid_has_mapping(mnt_userns, inode->i_sb->s_user_ns,
+				 attr->ia_mntgid))
 		return -EOVERFLOW;
 
 	/* Don't allow modifications of files with invalid uids or
 	 * gids unless those uids & gids are being made valid.
 	 */
 	if (!(ia_valid & ATTR_UID) &&
-	    !uid_valid(i_uid_into_mnt(mnt_userns, inode)))
+	    !kmntuid_valid(i_uid_into_mntuid(mnt_userns, inode)))
 		return -EOVERFLOW;
 	if (!(ia_valid & ATTR_GID) &&
-	    !gid_valid(i_gid_into_mnt(mnt_userns, inode)))
+	    !kmntgid_valid(i_gid_into_mntgid(mnt_userns, inode)))
 		return -EOVERFLOW;
 
-	error = security_inode_setattr(&init_user_ns, dentry, attr);
+	error = security_inode_setattr(mnt_userns, dentry, attr);
 	if (error)
 		return error;
 	error = try_break_deleg(inode, delegated_inode);
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 593b79416e0e..7a192e4e7fa9 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1679,14 +1679,14 @@ int ext2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (error)
 		return error;
 
-	if (is_quota_modification(&init_user_ns, inode, iattr)) {
+	if (is_quota_modification(mnt_userns, inode, iattr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
 	}
-	if (i_uid_needs_update(&init_user_ns, iattr, inode) ||
-	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
-		error = dquot_transfer(&init_user_ns, inode, iattr);
+	if (i_uid_needs_update(mnt_userns, iattr, inode) ||
+	    i_gid_needs_update(mnt_userns, iattr, inode)) {
+		error = dquot_transfer(mnt_userns, inode, iattr);
 		if (error)
 			return error;
 	}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 72f08c184768..3dcc1dd1f179 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5350,14 +5350,14 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (error)
 		return error;
 
-	if (is_quota_modification(&init_user_ns, inode, attr)) {
+	if (is_quota_modification(mnt_userns, inode, attr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
 	}
 
-	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
-	    i_gid_needs_update(&init_user_ns, attr, inode)) {
+	if (i_uid_needs_update(mnt_userns, attr, inode) ||
+	    i_gid_needs_update(mnt_userns, attr, inode)) {
 		handle_t *handle;
 
 		/* (user+group)*(old+new) structure, inode write (sb,
@@ -5374,7 +5374,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		 * counts xattr inode references.
 		 */
 		down_read(&EXT4_I(inode)->xattr_sem);
-		error = dquot_transfer(&init_user_ns, inode, attr);
+		error = dquot_transfer(mnt_userns, inode, attr);
 		up_read(&EXT4_I(inode)->xattr_sem);
 
 		if (error) {
@@ -5383,8 +5383,8 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		}
 		/* Update corresponding info in inode so that everything is in
 		 * one transaction */
-		i_uid_update(&init_user_ns, attr, inode);
-		i_gid_update(&init_user_ns, attr, inode);
+		i_uid_update(mnt_userns, attr, inode);
+		i_gid_update(mnt_userns, attr, inode);
 		error = ext4_mark_inode_dirty(handle, inode);
 		ext4_journal_stop(handle);
 		if (unlikely(error)) {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 02b2d56d4edc..d66e37d80a2d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -861,8 +861,8 @@ static void __setattr_copy(struct user_namespace *mnt_userns,
 {
 	unsigned int ia_valid = attr->ia_valid;
 
-	i_uid_update(&init_user_ns, attr, inode);
-	i_gid_update(&init_user_ns, attr, inode);
+	i_uid_update(mnt_userns, attr, inode);
+	i_gid_update(mnt_userns, attr, inode);
 	if (ia_valid & ATTR_ATIME)
 		inode->i_atime = attr->ia_atime;
 	if (ia_valid & ATTR_MTIME)
@@ -915,15 +915,15 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (err)
 		return err;
 
-	if (is_quota_modification(&init_user_ns, inode, attr)) {
+	if (is_quota_modification(mnt_userns, inode, attr)) {
 		err = f2fs_dquot_initialize(inode);
 		if (err)
 			return err;
 	}
-	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
-	    i_gid_needs_update(&init_user_ns, attr, inode)) {
+	if (i_uid_needs_update(mnt_userns, attr, inode) ||
+	    i_gid_needs_update(mnt_userns, attr, inode)) {
 		f2fs_lock_op(F2FS_I_SB(inode));
-		err = dquot_transfer(&init_user_ns, inode, attr);
+		err = dquot_transfer(mnt_userns, inode, attr);
 		if (err) {
 			set_sbi_flag(F2FS_I_SB(inode),
 					SBI_QUOTA_NEED_REPAIR);
@@ -934,8 +934,8 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		 * update uid/gid under lock_op(), so that dquot and inode can
 		 * be updated atomically.
 		 */
-		i_uid_update(&init_user_ns, attr, inode);
-		i_gid_update(&init_user_ns, attr, inode);
+		i_uid_update(mnt_userns, attr, inode);
+		i_gid_update(mnt_userns, attr, inode);
 		f2fs_mark_inode_dirty_sync(inode, true);
 		f2fs_unlock_op(F2FS_I_SB(inode));
 	}
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 530f18173db2..473b02833d4f 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -90,7 +90,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
 	 * out the RO attribute for checking by the security
 	 * module, just because it maps to a file mode.
 	 */
-	err = security_inode_setattr(&init_user_ns,
+	err = security_inode_setattr(file_mnt_user_ns(file),
 				     file->f_path.dentry, &ia);
 	if (err)
 		goto out_unlock_inode;
@@ -517,9 +517,9 @@ int fat_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	}
 
 	if (((attr->ia_valid & ATTR_UID) &&
-	     (!uid_eq(attr->ia_uid, sbi->options.fs_uid))) ||
+	     (!kuid_eq_kmntuid(sbi->options.fs_uid, attr->ia_mntuid))) ||
 	    ((attr->ia_valid & ATTR_GID) &&
-	     (!gid_eq(attr->ia_gid, sbi->options.fs_gid))) ||
+	     (!kgid_eq_kmntgid(sbi->options.fs_gid, attr->ia_mntgid))) ||
 	    ((attr->ia_valid & ATTR_MODE) &&
 	     (attr->ia_mode & ~FAT_VALID_MODE)))
 		error = -EPERM;
diff --git a/fs/jfs/file.c b/fs/jfs/file.c
index c18569b9895d..332dc9ac47a9 100644
--- a/fs/jfs/file.c
+++ b/fs/jfs/file.c
@@ -95,14 +95,14 @@ int jfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (rc)
 		return rc;
 
-	if (is_quota_modification(&init_user_ns, inode, iattr)) {
+	if (is_quota_modification(mnt_userns, inode, iattr)) {
 		rc = dquot_initialize(inode);
 		if (rc)
 			return rc;
 	}
 	if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
 	    (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, inode->i_gid))) {
-		rc = dquot_transfer(&init_user_ns, inode, iattr);
+		rc = dquot_transfer(mnt_userns, inode, iattr);
 		if (rc)
 			return rc;
 	}
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 0e09cd8911da..9c67edd215d5 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1146,7 +1146,7 @@ int ocfs2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	if (status)
 		return status;
 
-	if (is_quota_modification(&init_user_ns, inode, attr)) {
+	if (is_quota_modification(mnt_userns, inode, attr)) {
 		status = dquot_initialize(inode);
 		if (status)
 			return status;
diff --git a/fs/open.c b/fs/open.c
index 1d57fbde2feb..d3fea687c846 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -663,6 +663,47 @@ SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
 	return do_fchmodat(AT_FDCWD, filename, mode);
 }
 
+
+/**
+ * setattr_uid - check and set uid attribute
+ * @mnt_userns:	user namespace of the mount
+ * @kuid: new inode owner
+ *
+ * Check whether @kuid can be mapped into the mount and if so, place it in
+ * @attr's ia_mntuid member.
+ *
+ * Return: true if kuid has a mapping in the mount, false if not.
+ */
+static inline bool setattr_uid(struct user_namespace *mnt_userns,
+			       struct iattr *attr, kuid_t kuid)
+{
+	if (!kuid_has_mapping(mnt_userns, kuid))
+		return false;
+	attr->ia_valid |= ATTR_UID;
+	attr->ia_mntuid = KMNTUIDT_INIT(kuid);
+	return true;
+}
+
+/**
+ * setattr_gid - check and set gid attribute
+ * @mnt_userns:	user namespace of the mount
+ * @knid: new inode owner
+ *
+ * Check whether @kgid can be mapped into the mount and if so, place it in
+ * @attr's ia_mntgid member.
+ *
+ * Return: true if kgid has a mapping in the mount, false if not.
+ */
+static inline bool setattr_gid(struct user_namespace *mnt_userns,
+			       struct iattr *attr, kgid_t kgid)
+{
+	if (!kgid_has_mapping(mnt_userns, kgid))
+		return false;
+	attr->ia_valid |= ATTR_GID;
+	attr->ia_mntgid = KMNTGIDT_INIT(kgid);
+	return true;
+}
+
 int chown_common(const struct path *path, uid_t user, gid_t group)
 {
 	struct user_namespace *mnt_userns, *fs_userns;
@@ -678,28 +719,22 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
 
 	mnt_userns = mnt_user_ns(path->mnt);
 	fs_userns = i_user_ns(inode);
-	uid = mapped_kuid_user(mnt_userns, fs_userns, uid);
-	gid = mapped_kgid_user(mnt_userns, fs_userns, gid);
 
 retry_deleg:
 	newattrs.ia_valid =  ATTR_CTIME;
-	if (user != (uid_t) -1) {
-		if (!uid_valid(uid))
-			return -EINVAL;
-		newattrs.ia_valid |= ATTR_UID;
-		newattrs.ia_uid = uid;
-	}
-	if (group != (gid_t) -1) {
-		if (!gid_valid(gid))
-			return -EINVAL;
-		newattrs.ia_valid |= ATTR_GID;
-		newattrs.ia_gid = gid;
-	}
+	if ((user != (uid_t)-1) && !setattr_uid(mnt_userns, &newattrs, uid))
+		return -EINVAL;
+	if ((group != (gid_t)-1) && !setattr_gid(mnt_userns, &newattrs, gid))
+		return -EINVAL;
 	if (!S_ISDIR(inode->i_mode))
 		newattrs.ia_valid |=
 			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
 	inode_lock(inode);
-	error = security_path_chown(path, uid, gid);
+	/* Continue to send actual fs values, not the mount values. */
+	error = security_path_chown(
+		path,
+		kmntuid_to_kuid(mnt_userns, fs_userns, newattrs.ia_mntuid),
+		kmntgid_to_kgid(mnt_userns, fs_userns, newattrs.ia_mntgid));
 	if (!error)
 		error = notify_change(mnt_userns, path->dentry, &newattrs,
 				      &delegated_inode);
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 714ec569d25b..1087e44bb4e1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -331,8 +331,8 @@ int ovl_set_attr(struct ovl_fs *ofs, struct dentry *upperdentry,
 	if (!err) {
 		struct iattr attr = {
 			.ia_valid = ATTR_UID | ATTR_GID,
-			.ia_uid = stat->uid,
-			.ia_gid = stat->gid,
+			.ia_mntuid = KMNTUIDT_INIT(stat->uid),
+			.ia_mntgid = KMNTGIDT_INIT(stat->gid),
 		};
 		err = ovl_do_notify_change(ofs, upperdentry, &attr);
 	}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4f34b7e02eee..e22e20f4811a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -139,17 +139,7 @@ static inline int ovl_do_notify_change(struct ovl_fs *ofs,
 				       struct dentry *upperdentry,
 				       struct iattr *attr)
 {
-	struct user_namespace *upper_mnt_userns = ovl_upper_mnt_userns(ofs);
-	struct user_namespace *fs_userns = i_user_ns(d_inode(upperdentry));
-
-	if (attr->ia_valid & ATTR_UID)
-		attr->ia_uid = mapped_kuid_user(upper_mnt_userns,
-						fs_userns, attr->ia_uid);
-	if (attr->ia_valid & ATTR_GID)
-		attr->ia_gid = mapped_kgid_user(upper_mnt_userns,
-						fs_userns, attr->ia_gid);
-
-	return notify_change(upper_mnt_userns, upperdentry, attr, NULL);
+	return notify_change(ovl_upper_mnt_userns(ofs), upperdentry, attr, NULL);
 }
 
 static inline int ovl_do_rmdir(struct ovl_fs *ofs,
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index df9af1ce2851..8cb96f34fb63 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2096,8 +2096,11 @@ int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
 	if (!dquot_active(inode))
 		return 0;
 
-	if (i_uid_needs_update(&init_user_ns, iattr, inode)) {
-		dquot = dqget(sb, make_kqid_uid(iattr->ia_uid));
+	if (i_uid_needs_update(mnt_userns, iattr, inode)) {
+		kuid_t kuid = kmntuid_to_kuid(mnt_userns, i_user_ns(inode),
+					      iattr->ia_mntuid);
+
+		dquot = dqget(sb, make_kqid_uid(kuid));
 		if (IS_ERR(dquot)) {
 			if (PTR_ERR(dquot) != -ESRCH) {
 				ret = PTR_ERR(dquot);
@@ -2107,8 +2110,11 @@ int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
 		}
 		transfer_to[USRQUOTA] = dquot;
 	}
-	if (i_gid_needs_update(&init_user_ns, iattr, inode)) {
-		dquot = dqget(sb, make_kqid_gid(iattr->ia_gid));
+	if (i_gid_needs_update(mnt_userns, iattr, inode)) {
+		kgid_t kgid = kmntgid_to_kgid(mnt_userns, i_user_ns(inode),
+					      iattr->ia_mntgid);
+
+		dquot = dqget(sb, make_kqid_gid(kgid));
 		if (IS_ERR(dquot)) {
 			if (PTR_ERR(dquot) != -ESRCH) {
 				ret = PTR_ERR(dquot);
diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 1e89e76972a0..1141053b96ed 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -3284,7 +3284,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	/* must be turned off for recursive notify_change calls */
 	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
 
-	if (is_quota_modification(&init_user_ns, inode, attr)) {
+	if (is_quota_modification(mnt_userns, inode, attr)) {
 		error = dquot_initialize(inode);
 		if (error)
 			return error;
@@ -3367,7 +3367,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		reiserfs_write_unlock(inode->i_sb);
 		if (error)
 			goto out;
-		error = dquot_transfer(&init_user_ns, inode, attr);
+		error = dquot_transfer(mnt_userns, inode, attr);
 		reiserfs_write_lock(inode->i_sb);
 		if (error) {
 			journal_end(&th);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 31ec29565fb4..fb944721549b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -667,7 +667,8 @@ xfs_setattr_nonsize(
 		uint	qflags = 0;
 
 		if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp)) {
-			uid = iattr->ia_uid;
+			uid = kmntuid_to_kuid(mnt_userns, i_user_ns(inode),
+					      iattr->ia_mntuid);
 			qflags |= XFS_QMOPT_UQUOTA;
 		} else {
 			uid = inode->i_uid;
@@ -705,12 +706,12 @@ xfs_setattr_nonsize(
 	 * also.
 	 */
 	if (XFS_IS_UQUOTA_ON(mp) &&
-	    i_uid_needs_update(&init_user_ns, iattr, inode)) {
+	    i_uid_needs_update(mnt_userns, iattr, inode)) {
 		ASSERT(udqp);
 		old_udqp = xfs_qm_vop_chown(tp, ip, &ip->i_udquot, udqp);
 	}
 	if (XFS_IS_GQUOTA_ON(mp) &&
-	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
+	    i_gid_needs_update(mnt_userns, iattr, inode)) {
 		ASSERT(xfs_has_pquotino(mp) || !XFS_IS_PQUOTA_ON(mp));
 		ASSERT(gdqp);
 		old_gdqp = xfs_qm_vop_chown(tp, ip, &ip->i_gdquot, gdqp);
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index dd422b1d7320..f5d8338967cb 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -616,7 +616,7 @@ static int zonefs_inode_setattr(struct user_namespace *mnt_userns,
 	     !uid_eq(iattr->ia_uid, inode->i_uid)) ||
 	    ((iattr->ia_valid & ATTR_GID) &&
 	     !gid_eq(iattr->ia_gid, inode->i_gid))) {
-		ret = dquot_transfer(&init_user_ns, inode, iattr);
+		ret = dquot_transfer(mnt_userns, inode, iattr);
 		if (ret)
 			return ret;
 	}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 998ac36ea7b0..8de658aee1f9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -228,6 +228,10 @@ struct iattr {
 	 * are a dedicated type requiring the filesystem to use the dedicated
 	 * helpers. Other filesystem can continue to use ia_{g,u}id until they
 	 * have been ported.
+	 *
+	 * They always contain the same value. In other words FS_ALLOW_IDMAP
+	 * pass down the same value on idmapped mounts as they would on regular
+	 * mounts.
 	 */
 	union {
 		kuid_t		ia_uid;
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index 0342ff6584fd..0d8625d71733 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -24,8 +24,8 @@ static inline bool is_quota_modification(struct user_namespace *mnt_userns,
 					 struct inode *inode, struct iattr *ia)
 {
 	return ((ia->ia_valid & ATTR_SIZE) ||
-		i_uid_needs_update(&init_user_ns, ia, inode) ||
-		i_gid_needs_update(&init_user_ns, ia, inode));
+		i_uid_needs_update(mnt_userns, ia, inode) ||
+		i_gid_needs_update(mnt_userns, ia, inode));
 }
 
 #if defined(CONFIG_QUOTA)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 7f4af5b58583..93e8bc047a73 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -761,8 +761,8 @@ static int evm_attr_change(struct user_namespace *mnt_userns,
 	struct inode *inode = d_backing_inode(dentry);
 	unsigned int ia_valid = attr->ia_valid;
 
-	if (!i_uid_needs_update(&init_user_ns, attr, inode) &&
-	    !i_gid_needs_update(&init_user_ns, attr, inode) &&
+	if (!i_uid_needs_update(mnt_userns, attr, inode) &&
+	    !i_gid_needs_update(mnt_userns, attr, inode) &&
 	    (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
 		return 0;
 
-- 
2.34.1

Re: [PATCH 1/8] mnt_idmapping: add kmnt{g,u}id_t

[Linus Torvalds]

On Mon, Jun 20, 2022 at 8:50 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types
> are just simple wrapper structs around regular {g,u}id_t types.

Thanks for working on this.,

I haven't actually perused the series yet, but wanted to just
immediately react to "please don't make random-letter type names".

"gid" is something people understand. It's a thing.

"kgid" kind of made sense, in that it's the "kernel view of the gid",
and it was still short and fairly legible.

"kmntgid" is neither short, legible, or makes sense.

For one thing, the "k" part no longer makes any sense. It's not about
the "kernel view of the gid" any more. Sure, it's "kernel" in the
sense that any kernel code is "kernel", but it's no longer some kind
of "unified kernel view of a user-namespace gid".

So the "k" in "kgid" doesn't make sense because it's a kernel thing,
but more as a negative: "it is *not* the user visible gid".

So instead of changing all our old "git_t" definitions to be "ugid_t"
(for "user visible gid") the "kgid_t" thing was done.

As a result: when you translate it to the mount namespace, I do not
believe that the "k" makes sense any more, because now the point to
distinguish it from "user gids" no longer exists. So it's just one
random letter. In a long jumble of letters that isn't really very
legible or pronounceable.

If it didn't have that 'i' in it, I would think it's a IBM mnemonic
(and I use the word "mnemonic" ironically) for some random assembler
instruction. They used up all the vowels they were willing to use for
the "eieio" instructions, and all other instruction names are a jumble
of random consonants.

So please try to make the type names less of a random jumble of
letters picked from a bag.

That "kmnt[gu]id" pattern exists elsewhere too, in the conversion
functions etc, so it's not just the type name, but more of a generic
"please don't use letter-jumble names".

Maybe just "mnt_[gu]id"" instead of "kmnt[gu]id" would be better.

But even that smells wrong to me. Isn't it really "the guid/uid seen
by the filesystem after the mount mapping"? Wouldn't it be nice to
name by the same "seen by users" and "seen by kernel" to be "seen by
filesystrem"? So wouln't a name like "fs_[gu]id_t" make even more
sense?

I dunno. Maybe I'm thinking about this the wrong way, but I wish the
names would be more explanatory. My personal mental image is that the
user namespaces map a traditional uid into the "kernel id space", and
then the mount id mappings map into the "filesystem id space". Which
is why to me that "k" doesn't make sense, and the "mnt" doesn't really
make tons of sense either (the mount is the thing that _maps_ the id
spaces, but it's not the end result of said mapping).

IOW, I get the feeling that you've named the result with the mapping,
not with the end use. That would be like naming "kuid" by the mapping
(usernamespace), not the end result (the kernel namespace).

But maybe it's just me that is confused here. Particularly since I
didn't really more than *very* superficially and quickly scan the
patches.

                Linus

Re: [PATCH 1/8] mnt_idmapping: add kmnt{g,u}id_t

[Christian Brauner]

On Mon, Jun 20, 2022 at 09:28:00AM -0500, Linus Torvalds wrote:
> On Mon, Jun 20, 2022 at 8:50 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > Introduces new kmnt{g,u}id_t types. Similar to k{g,u}id_t the new types
> > are just simple wrapper structs around regular {g,u}id_t types.
> 
> Thanks for working on this.,
> 
> I haven't actually perused the series yet, but wanted to just
> immediately react to "please don't make random-letter type names".
> 
> "gid" is something people understand. It's a thing.
> 
> "kgid" kind of made sense, in that it's the "kernel view of the gid",
> and it was still short and fairly legible.
> 
> "kmntgid" is neither short, legible, or makes sense.
> 
> For one thing, the "k" part no longer makes any sense. It's not about
> the "kernel view of the gid" any more. Sure, it's "kernel" in the
> sense that any kernel code is "kernel", but it's no longer some kind
> of "unified kernel view of a user-namespace gid".
> 
> So the "k" in "kgid" doesn't make sense because it's a kernel thing,
> but more as a negative: "it is *not* the user visible gid".
> 
> So instead of changing all our old "git_t" definitions to be "ugid_t"
> (for "user visible gid") the "kgid_t" thing was done.
> 
> As a result: when you translate it to the mount namespace, I do not
> believe that the "k" makes sense any more, because now the point to
> distinguish it from "user gids" no longer exists. So it's just one
> random letter. In a long jumble of letters that isn't really very
> legible or pronounceable.
> 
> If it didn't have that 'i' in it, I would think it's a IBM mnemonic
> (and I use the word "mnemonic" ironically) for some random assembler
> instruction. They used up all the vowels they were willing to use for
> the "eieio" instructions, and all other instruction names are a jumble
> of random consonants.
> 
> So please try to make the type names less of a random jumble of
> letters picked from a bag.
> 
> That "kmnt[gu]id" pattern exists elsewhere too, in the conversion
> functions etc, so it's not just the type name, but more of a generic
> "please don't use letter-jumble names".
> 
> Maybe just "mnt_[gu]id"" instead of "kmnt[gu]id" would be better.
> 
> But even that smells wrong to me. Isn't it really "the guid/uid seen
> by the filesystem after the mount mapping"? Wouldn't it be nice to
> name by the same "seen by users" and "seen by kernel" to be "seen by
> filesystrem"? So wouln't a name like "fs_[gu]id_t" make even more
> sense?
> 
> I dunno. Maybe I'm thinking about this the wrong way, but I wish the
> names would be more explanatory. My personal mental image is that the
> user namespaces map a traditional uid into the "kernel id space", and
> then the mount id mappings map into the "filesystem id space". Which

Yes. Basically without idmapped mounts if the caller's idmapping and the
filesystem's idmapping contain the same kuid then the uid/gid passed in
from userspace can be represented in the filesystem idmapping and thus
ultimately on-disk. That's the very basic model.

If the caller uses an idmapped mount then the caller's idmapping and the
filesystem's idmapping are connected via the mount's idmapping. Iow, the
mount remaps the caller's kuid to a different kuid in the filesystem's
idmapping.

> is why to me that "k" doesn't make sense, and the "mnt" doesn't really
> make tons of sense either (the mount is the thing that _maps_ the id
> spaces, but it's not the end result of said mapping).

Yeah, fair point.

> 
> IOW, I get the feeling that you've named the result with the mapping,
> not with the end use. That would be like naming "kuid" by the mapping
> (usernamespace), not the end result (the kernel namespace).
> 
> But maybe it's just me that is confused here. Particularly since I
> didn't really more than *very* superficially and quickly scan the
> patches.

Originally I called that kfs{g,u}id_t but I was never happy with that
either... I think either vfs{g,u}id_t or fs_{g,u}id_t makes sense.

Re: [PATCH 1/8] mnt_idmapping: add kmnt{g,u}id_t

[Linus Torvalds]

On Mon, Jun 20, 2022 at 10:25 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Originally I called that kfs{g,u}id_t but I was never happy with that
> either... I think either vfs{g,u}id_t or fs_{g,u}id_t makes sense.

vfs[gu]id sounds good to me. That way we avoid the confusion with our
current "cred->fsuid" thing due to the access() system call.

               Linus

Re: [PATCH 6/8] quota: port quota helpers mount ids

[Jan Kara]

On Mon 20-06-22 15:49:45, Christian Brauner wrote:
> Port the is_quota_modification() and dqout_transfer() helper to type
> safe kmnt{g,u}id_t. Since these helpers are only called by a few
> filesystems don't introduce a new helper but simply extend the existing
> helpers to pass down the mount's idmapping.
> 
> Note, that this is a non-functional change, i.e. nothing will have
> happened here or at the end of this series to how quota are done! This
> a change necessary because we will at the end of this series make
> ownership changes easier to reason about by keeping the original value
> in struct iattr for both non-idmapped and idmapped mounts.
> 
> For now we always pass the initial idmapping which makes the idmapping
> functions these helpers call nops.
> 
> This is done because we currently always pass the actual value to be
> written to i_{g,u}id via struct iattr. While this allowed us to treat
> the {g,u}id values in struct iattr as values that can be directly
> written to inode->i_{g,u}id it also increases the potential for
> confusion for filesystems.
> 
> Now that we are about to introduce dedicated types to prevent this
> confusion we will ultimately only map the value from the idmapped mount
> into a filesystem value that can be written to inode->i_{g,u}id when the
> filesystem actually updates the inode. So pass down the initial
> idmapping until we finished that conversion.
> 
> Since struct iattr uses an anonymous union with overlapping types as
> supported by the C filesystems that haven't converted to ia_mnt{g,u}id
> won't see any difference and things will continue to work as before.
> In other words, no functional changes intended with this change.
> 
> Cc: Seth Forshee <sforshee@digitalocean.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> CC: linux-fsdevel@vger.kernel.org
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Yeah, this looks fairly innocent. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

Just when do you plan to make changes actually using the passed namespace?

								Honza

> ---
>  fs/ext2/inode.c          | 4 ++--
>  fs/ext4/inode.c          | 4 ++--
>  fs/f2fs/file.c           | 4 ++--
>  fs/f2fs/recovery.c       | 2 +-
>  fs/jfs/file.c            | 4 ++--
>  fs/ocfs2/file.c          | 2 +-
>  fs/quota/dquot.c         | 3 ++-
>  fs/reiserfs/inode.c      | 4 ++--
>  fs/zonefs/super.c        | 2 +-
>  include/linux/quotaops.h | 9 ++++++---
>  10 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 6dc66ab97d20..593b79416e0e 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1679,14 +1679,14 @@ int ext2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (error)
>  		return error;
>  
> -	if (is_quota_modification(inode, iattr)) {
> +	if (is_quota_modification(&init_user_ns, inode, iattr)) {
>  		error = dquot_initialize(inode);
>  		if (error)
>  			return error;
>  	}
>  	if (i_uid_needs_update(&init_user_ns, iattr, inode) ||
>  	    i_gid_needs_update(&init_user_ns, iattr, inode)) {
> -		error = dquot_transfer(inode, iattr);
> +		error = dquot_transfer(&init_user_ns, inode, iattr);
>  		if (error)
>  			return error;
>  	}
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 05d932f81c53..72f08c184768 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5350,7 +5350,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (error)
>  		return error;
>  
> -	if (is_quota_modification(inode, attr)) {
> +	if (is_quota_modification(&init_user_ns, inode, attr)) {
>  		error = dquot_initialize(inode);
>  		if (error)
>  			return error;
> @@ -5374,7 +5374,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  		 * counts xattr inode references.
>  		 */
>  		down_read(&EXT4_I(inode)->xattr_sem);
> -		error = dquot_transfer(inode, attr);
> +		error = dquot_transfer(&init_user_ns, inode, attr);
>  		up_read(&EXT4_I(inode)->xattr_sem);
>  
>  		if (error) {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index a35d6b12bd63..02b2d56d4edc 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -915,7 +915,7 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (err)
>  		return err;
>  
> -	if (is_quota_modification(inode, attr)) {
> +	if (is_quota_modification(&init_user_ns, inode, attr)) {
>  		err = f2fs_dquot_initialize(inode);
>  		if (err)
>  			return err;
> @@ -923,7 +923,7 @@ int f2fs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (i_uid_needs_update(&init_user_ns, attr, inode) ||
>  	    i_gid_needs_update(&init_user_ns, attr, inode)) {
>  		f2fs_lock_op(F2FS_I_SB(inode));
> -		err = dquot_transfer(inode, attr);
> +		err = dquot_transfer(&init_user_ns, inode, attr);
>  		if (err) {
>  			set_sbi_flag(F2FS_I_SB(inode),
>  					SBI_QUOTA_NEED_REPAIR);
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 3cb7f8a43b4d..8e5a089f1ac8 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -266,7 +266,7 @@ static int recover_quota_data(struct inode *inode, struct page *page)
>  	if (!attr.ia_valid)
>  		return 0;
>  
> -	err = dquot_transfer(inode, &attr);
> +	err = dquot_transfer(&init_user_ns, inode, &attr);
>  	if (err)
>  		set_sbi_flag(F2FS_I_SB(inode), SBI_QUOTA_NEED_REPAIR);
>  	return err;
> diff --git a/fs/jfs/file.c b/fs/jfs/file.c
> index 1d732fd223d4..c18569b9895d 100644
> --- a/fs/jfs/file.c
> +++ b/fs/jfs/file.c
> @@ -95,14 +95,14 @@ int jfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (rc)
>  		return rc;
>  
> -	if (is_quota_modification(inode, iattr)) {
> +	if (is_quota_modification(&init_user_ns, inode, iattr)) {
>  		rc = dquot_initialize(inode);
>  		if (rc)
>  			return rc;
>  	}
>  	if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, inode->i_uid)) ||
>  	    (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, inode->i_gid))) {
> -		rc = dquot_transfer(inode, iattr);
> +		rc = dquot_transfer(&init_user_ns, inode, iattr);
>  		if (rc)
>  			return rc;
>  	}
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index 7497cd592258..0e09cd8911da 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1146,7 +1146,7 @@ int ocfs2_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (status)
>  		return status;
>  
> -	if (is_quota_modification(inode, attr)) {
> +	if (is_quota_modification(&init_user_ns, inode, attr)) {
>  		status = dquot_initialize(inode);
>  		if (status)
>  			return status;
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 6cec2bfbf51b..df9af1ce2851 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2085,7 +2085,8 @@ EXPORT_SYMBOL(__dquot_transfer);
>  /* Wrapper for transferring ownership of an inode for uid/gid only
>   * Called from FSXXX_setattr()
>   */
> -int dquot_transfer(struct inode *inode, struct iattr *iattr)
> +int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
> +		   struct iattr *iattr)
>  {
>  	struct dquot *transfer_to[MAXQUOTAS] = {};
>  	struct dquot *dquot;
> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index 0cffe054b78e..1e89e76972a0 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -3284,7 +3284,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	/* must be turned off for recursive notify_change calls */
>  	ia_valid = attr->ia_valid &= ~(ATTR_KILL_SUID|ATTR_KILL_SGID);
>  
> -	if (is_quota_modification(inode, attr)) {
> +	if (is_quota_modification(&init_user_ns, inode, attr)) {
>  		error = dquot_initialize(inode);
>  		if (error)
>  			return error;
> @@ -3367,7 +3367,7 @@ int reiserfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>  		reiserfs_write_unlock(inode->i_sb);
>  		if (error)
>  			goto out;
> -		error = dquot_transfer(inode, attr);
> +		error = dquot_transfer(&init_user_ns, inode, attr);
>  		reiserfs_write_lock(inode->i_sb);
>  		if (error) {
>  			journal_end(&th);
> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
> index 053299758deb..dd422b1d7320 100644
> --- a/fs/zonefs/super.c
> +++ b/fs/zonefs/super.c
> @@ -616,7 +616,7 @@ static int zonefs_inode_setattr(struct user_namespace *mnt_userns,
>  	     !uid_eq(iattr->ia_uid, inode->i_uid)) ||
>  	    ((iattr->ia_valid & ATTR_GID) &&
>  	     !gid_eq(iattr->ia_gid, inode->i_gid))) {
> -		ret = dquot_transfer(inode, iattr);
> +		ret = dquot_transfer(&init_user_ns, inode, iattr);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index 61ee34861ca2..0342ff6584fd 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -20,7 +20,8 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>  }
>  
>  /* i_mutex must being held */
> -static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> +static inline bool is_quota_modification(struct user_namespace *mnt_userns,
> +					 struct inode *inode, struct iattr *ia)
>  {
>  	return ((ia->ia_valid & ATTR_SIZE) ||
>  		i_uid_needs_update(&init_user_ns, ia, inode) ||
> @@ -115,7 +116,8 @@ int dquot_set_dqblk(struct super_block *sb, struct kqid id,
>  		struct qc_dqblk *di);
>  
>  int __dquot_transfer(struct inode *inode, struct dquot **transfer_to);
> -int dquot_transfer(struct inode *inode, struct iattr *iattr);
> +int dquot_transfer(struct user_namespace *mnt_userns, struct inode *inode,
> +		   struct iattr *iattr);
>  
>  static inline struct mem_dqinfo *sb_dqinfo(struct super_block *sb, int type)
>  {
> @@ -234,7 +236,8 @@ static inline void dquot_free_inode(struct inode *inode)
>  {
>  }
>  
> -static inline int dquot_transfer(struct inode *inode, struct iattr *iattr)
> +static inline int dquot_transfer(struct user_namespace *mnt_userns,
> +				 struct inode *inode, struct iattr *iattr)
>  {
>  	return 0;
>  }
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 6/8] quota: port quota helpers mount ids

[Christian Brauner]

On Tue, Jun 21, 2022 at 12:20:27PM +0200, Jan Kara wrote:
> On Mon 20-06-22 15:49:45, Christian Brauner wrote:
> > Port the is_quota_modification() and dqout_transfer() helper to type
> > safe kmnt{g,u}id_t. Since these helpers are only called by a few
> > filesystems don't introduce a new helper but simply extend the existing
> > helpers to pass down the mount's idmapping.
> > 
> > Note, that this is a non-functional change, i.e. nothing will have
> > happened here or at the end of this series to how quota are done! This
> > a change necessary because we will at the end of this series make
> > ownership changes easier to reason about by keeping the original value
> > in struct iattr for both non-idmapped and idmapped mounts.
> > 
> > For now we always pass the initial idmapping which makes the idmapping
> > functions these helpers call nops.
> > 
> > This is done because we currently always pass the actual value to be
> > written to i_{g,u}id via struct iattr. While this allowed us to treat
> > the {g,u}id values in struct iattr as values that can be directly
> > written to inode->i_{g,u}id it also increases the potential for
> > confusion for filesystems.
> > 
> > Now that we are about to introduce dedicated types to prevent this
> > confusion we will ultimately only map the value from the idmapped mount
> > into a filesystem value that can be written to inode->i_{g,u}id when the
> > filesystem actually updates the inode. So pass down the initial
> > idmapping until we finished that conversion.
> > 
> > Since struct iattr uses an anonymous union with overlapping types as
> > supported by the C filesystems that haven't converted to ia_mnt{g,u}id
> > won't see any difference and things will continue to work as before.
> > In other words, no functional changes intended with this change.
> > 
> > Cc: Seth Forshee <sforshee@digitalocean.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > CC: linux-fsdevel@vger.kernel.org
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> 
> Yeah, this looks fairly innocent. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just when do you plan to make changes actually using the passed namespace?

Hey Jan,

Thank you for the review!
The changes are in the last patch of the series. But it is all unrelated
to quotas and doesn't alter them in any way. We really just change how
ia_{vfs}uid is set up to get type safety which requires the generic
codepaths like here to calculate the correct value when they actually
perform a write instead of having performed that calculation when we
set up iattr. That's really all.

Thanks!
Christian