[PATCH 0/6] acl: rework idmap handling when setting posix acls

[PATCH 0/6] acl: rework idmap handling when setting posix acls

[Christian Brauner]

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

Hey everyone,

As explained in detail in [1] POSIX ACLs are a bit wonky as they abuse
the uapi POSIX ACL structure to transport the values of k{g,u}id_t as
raw {g,u}id stored in ACL_{GROUP,USER} entries down to the filesystems.

The values stored in the POSIX ACL uapi struct have been mapped into the
caller's idmapping in setxattr_convert(). In addition, the VFS needs to
take idmapped mounts into during vfs_setxattr(). Currently, it uses the
uapi POSIX ACL structure for this as well.

While the handling of idmapped mounts needs to happen in vfs_setxattr()
or deeper in its callchain to guarantee that overlayfs handles POSIX
ACLs correctly on top of idmapped layers it isn't necessary to further
abuse the uapi POSIX ACL structure for this.

Instead of taking idmapped mounts into account and updaing the values in
the POSIX ACL uapi struct directly in vfs_setxattr() we can move it down
into posix_acl_xattr_set() helper. This allows us to make the value
argument of vfs_setxattr() const and gets rid of an additional loop.

Ultimately, we hope to still get rid of the POSIX ACL uapi struct abuse
completely but that requires a little more work.

This series also ports ntfs3 to rely on the standard POSXI ACL xattr
handler instead of rolling its own (currently broken) implementation.

This survives xfstests and LTP.

Thanks!
Christian

[1]: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org

Christian Brauner (6):
  ntfs3: rework xattr handlers and switch to POSIX ACL VFS helpers
  acl: return EOPNOTSUPP in posix_acl_fix_xattr_common()
  acl: add vfs_set_acl_prepare()
  acl: move idmapping handling into posix_acl_xattr_set()
  ovl: use vfs_set_acl_prepare()
  xattr: constify value argument in vfs_setxattr()

 fs/ntfs3/inode.c                  |   2 -
 fs/ntfs3/xattr.c                  | 102 +----------
 fs/overlayfs/overlayfs.h          |   2 +-
 fs/overlayfs/super.c              |  15 +-
 fs/posix_acl.c                    | 288 +++++++++++++++++++++++-------
 fs/xattr.c                        |   8 +-
 include/linux/posix_acl_xattr.h   |   3 +
 include/linux/xattr.h             |   2 +-
 security/integrity/evm/evm_main.c |  17 +-
 9 files changed, 262 insertions(+), 177 deletions(-)


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.34.1

[PATCH 1/6] ntfs3: rework xattr handlers and switch to POSIX ACL VFS helpers

[Christian Brauner]

The xattr code in ntfs3 is currently a bit confused. For example, it
defines a POSIX ACL i_op->set_acl() method but instead of relying on the
generic POSIX ACL VFS helpers it defines its own set of xattr helpers
with the consequence that i_op->set_acl() is currently dead code.

Switch ntfs3 to rely on the VFS POSIX ACL xattr handlers. Also remove
i_op->{g,s}et_acl() methods from symlink inode operations. Symlinks
don't support xattrs.

This is a preliminary change for the following patches which move
handling idmapped mounts directly in posix_acl_xattr_set().

This survives POSIX ACL xfstests.

Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/ntfs3/inode.c |   2 -
 fs/ntfs3/xattr.c | 102 +++--------------------------------------------
 2 files changed, 6 insertions(+), 98 deletions(-)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 51363d4e8636..26a76ebfe58f 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -1927,8 +1927,6 @@ const struct inode_operations ntfs_link_inode_operations = {
 	.setattr	= ntfs3_setattr,
 	.listxattr	= ntfs_listxattr,
 	.permission	= ntfs_permission,
-	.get_acl	= ntfs_get_acl,
-	.set_acl	= ntfs_set_acl,
 };
 
 const struct address_space_operations ntfs_aops = {
diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 6ae1f56b7358..7de8718c68a9 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -625,67 +625,6 @@ int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
 	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, false);
 }
 
-static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
-			      struct inode *inode, int type, void *buffer,
-			      size_t size)
-{
-	struct posix_acl *acl;
-	int err;
-
-	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
-		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
-		return -EOPNOTSUPP;
-	}
-
-	acl = ntfs_get_acl(inode, type, false);
-	if (IS_ERR(acl))
-		return PTR_ERR(acl);
-
-	if (!acl)
-		return -ENODATA;
-
-	err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
-	posix_acl_release(acl);
-
-	return err;
-}
-
-static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
-			      struct inode *inode, int type, const void *value,
-			      size_t size)
-{
-	struct posix_acl *acl;
-	int err;
-
-	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
-		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
-		return -EOPNOTSUPP;
-	}
-
-	if (!inode_owner_or_capable(mnt_userns, inode))
-		return -EPERM;
-
-	if (!value) {
-		acl = NULL;
-	} else {
-		acl = posix_acl_from_xattr(&init_user_ns, value, size);
-		if (IS_ERR(acl))
-			return PTR_ERR(acl);
-
-		if (acl) {
-			err = posix_acl_valid(&init_user_ns, acl);
-			if (err)
-				goto release_and_out;
-		}
-	}
-
-	err = ntfs_set_acl(mnt_userns, inode, acl, type);
-
-release_and_out:
-	posix_acl_release(acl);
-	return err;
-}
-
 /*
  * ntfs_init_acl - Initialize the ACLs of a new inode.
  *
@@ -852,23 +791,6 @@ static int ntfs_getxattr(const struct xattr_handler *handler, struct dentry *de,
 		goto out;
 	}
 
-#ifdef CONFIG_NTFS3_FS_POSIX_ACL
-	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
-	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
-		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
-	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
-	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
-		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
-		/* TODO: init_user_ns? */
-		err = ntfs_xattr_get_acl(
-			&init_user_ns, inode,
-			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
-				? ACL_TYPE_ACCESS
-				: ACL_TYPE_DEFAULT,
-			buffer, size);
-		goto out;
-	}
-#endif
 	/* Deal with NTFS extended attribute. */
 	err = ntfs_get_ea(inode, name, name_len, buffer, size, NULL);
 
@@ -981,22 +903,6 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
 		goto out;
 	}
 
-#ifdef CONFIG_NTFS3_FS_POSIX_ACL
-	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
-	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
-		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
-	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
-	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
-		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
-		err = ntfs_xattr_set_acl(
-			mnt_userns, inode,
-			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
-				? ACL_TYPE_ACCESS
-				: ACL_TYPE_DEFAULT,
-			value, size);
-		goto out;
-	}
-#endif
 	/* Deal with NTFS extended attribute. */
 	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
 
@@ -1086,7 +992,7 @@ static bool ntfs_xattr_user_list(struct dentry *dentry)
 }
 
 // clang-format off
-static const struct xattr_handler ntfs_xattr_handler = {
+static const struct xattr_handler ntfs_other_xattr_handler = {
 	.prefix	= "",
 	.get	= ntfs_getxattr,
 	.set	= ntfs_setxattr,
@@ -1094,7 +1000,11 @@ static const struct xattr_handler ntfs_xattr_handler = {
 };
 
 const struct xattr_handler *ntfs_xattr_handlers[] = {
-	&ntfs_xattr_handler,
+#ifdef CONFIG_NTFS3_FS_POSIX_ACL
+	&posix_acl_access_xattr_handler,
+	&posix_acl_default_xattr_handler,
+#endif
+	&ntfs_other_xattr_handler,
 	NULL,
 };
 // clang-format on
-- 
2.34.1

[PATCH 2/6] acl: return EOPNOTSUPP in posix_acl_fix_xattr_common()

[Christian Brauner]

Return EOPNOTSUPP when the POSIX ACL version doesn't match and zero if
there are no entries. This will allow us to reuse the helper in
posix_acl_from_xattr(). This change will have no user visible effects.

Fixes: 0c5fd887d2bb ("acl: move idmapped mount fixup into vfs_{g,s}etxattr()")
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/posix_acl.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 5af33800743e..abe387700ba9 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -710,9 +710,9 @@ EXPORT_SYMBOL(posix_acl_update_mode);
 /*
  * Fix up the uids and gids in posix acl extended attributes in place.
  */
-static int posix_acl_fix_xattr_common(void *value, size_t size)
+static int posix_acl_fix_xattr_common(const void *value, size_t size)
 {
-	struct posix_acl_xattr_header *header = value;
+	const struct posix_acl_xattr_header *header = value;
 	int count;
 
 	if (!header)
@@ -720,13 +720,13 @@ static int posix_acl_fix_xattr_common(void *value, size_t size)
 	if (size < sizeof(struct posix_acl_xattr_header))
 		return -EINVAL;
 	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
-		return -EINVAL;
+		return -EOPNOTSUPP;
 
 	count = posix_acl_xattr_count(size);
 	if (count < 0)
 		return -EINVAL;
 	if (count == 0)
-		return -EINVAL;
+		return 0;
 
 	return count;
 }
@@ -748,7 +748,7 @@ void posix_acl_getxattr_idmapped_mnt(struct user_namespace *mnt_userns,
 		return;
 
 	count = posix_acl_fix_xattr_common(value, size);
-	if (count < 0)
+	if (count <= 0)
 		return;
 
 	for (end = entry + count; entry != end; entry++) {
@@ -788,7 +788,7 @@ void posix_acl_setxattr_idmapped_mnt(struct user_namespace *mnt_userns,
 		return;
 
 	count = posix_acl_fix_xattr_common(value, size);
-	if (count < 0)
+	if (count <= 0)
 		return;
 
 	for (end = entry + count; entry != end; entry++) {
@@ -822,7 +822,7 @@ static void posix_acl_fix_xattr_userns(
 	kgid_t gid;
 
 	count = posix_acl_fix_xattr_common(value, size);
-	if (count < 0)
+	if (count <= 0)
 		return;
 
 	for (end = entry + count; entry != end; entry++) {
@@ -870,16 +870,9 @@ posix_acl_from_xattr(struct user_namespace *user_ns,
 	struct posix_acl *acl;
 	struct posix_acl_entry *acl_e;
 
-	if (!value)
-		return NULL;
-	if (size < sizeof(struct posix_acl_xattr_header))
-		 return ERR_PTR(-EINVAL);
-	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
-		return ERR_PTR(-EOPNOTSUPP);
-
-	count = posix_acl_xattr_count(size);
+	count = posix_acl_fix_xattr_common(value, size);
 	if (count < 0)
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(count);
 	if (count == 0)
 		return NULL;
 	
-- 
2.34.1

[PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner]

Various filesystems store POSIX ACLs on the backing store in their uapi
format. Such filesystems need to translate from the uapi POSIX ACL
format into the VFS format during i_op->get_acl(). The VFS provides the
posix_acl_from_xattr() helper for this task.

But the usage of posix_acl_from_xattr() is currently ambiguous. It is
intended to transform from a uapi POSIX ACL  to the VFS represenation.
For example, when retrieving POSIX ACLs for permission checking during
lookup or when calling getxattr() to retrieve system.posix_acl_{access,default}.

Calling posix_acl_from_xattr() during i_op->get_acl() will map the raw
{g,u}id values stored as ACL_{GROUP,USER} entries in the uapi POSIX ACL
format into k{g,u}id_t in the filesystem's idmapping and return a struct
posix_acl ready to be returned to the VFS for caching and to perform
permission checks on.

However, posix_acl_from_xattr() is also called during setxattr() for all
filesystems that rely on VFS provides posix_acl_{access,default}_xattr_handler.
The posix_acl_xattr_set() handler which is used for the ->set() method
of posix_acl_{access,default}_xattr_handler uses posix_acl_from_xattr()
to translate from the uapi POSIX ACL format to the VFS format so that it
can be passed to the i_op->set_acl() handler of the filesystem or for
direct caching in case no i_op->set_acl() handler is defined.

During setxattr() the {g,u}id values stored as ACL_{GROUP,USER} entries
in the uapi POSIX ACL format aren't raw {g,u}id values that need to be
mapped according to the filesystem's idmapping. Instead they are {g,u}id
values in the caller's idmapping which have been generated during
posix_acl_fix_xattr_from_user(). In other words, they are k{g,u}id_t
which are passed as raw {g,u}id values abusing the uapi POSIX ACL format
(Please note that this type safety violation has existed since the
introduction of k{g,u}id_t. Please see [1] for more details.).

So when posix_acl_from_xattr() is called in posix_acl_xattr_set() the
filesystem idmapping is completely irrelevant. Instead, we abuse the
initial idmapping to recover the k{g,u}id_t base on the value stored in
raw {g,u}id as ACL_{GROUP,USER} in the uapi POSIX ACL format.

We need to clearly distinguish betweeen these two operations as it is
really easy to confuse for filesystems as can be seen in ntfs3.

In order to do this we factor out make_posix_acl() which takes callbacks
allowing callers to pass dedicated methods to generate the correct
k{g,u}id_t. This is just an internal static helper which is not exposed
to any filesystems but it neatly encapsulates the basic logic of walking
through a uapi POSIX ACL and returning an allocated VFS POSIX ACL with
the correct k{g,u}id_t values.

The posix_acl_from_xattr() helper can then be implemented as a simple
call to make_posix_acl() with callbacks that generate the correct
k{g,u}id_t from the raw {g,u}id values in ACL_{GROUP,USER} entries in
the uapi POSIX ACL format as read from the backing store.

For setxattr() we add a new helper vfs_set_acl_prepare() which has
callbacks to map the POSIX ACLs from the uapi format with the k{g,u}id_t
values stored in raw {g,u}id format in ACL_{GROUP,USER} entries into the
correct k{g,u}id_t values in the filesystem idmapping. In contrast to
posix_acl_from_xattr() the vfs_set_acl_prepare() helper needs to take
the mount idmapping into account. The differences are explained in more
detail in the kernel doc for the new functions.

In follow up patches we will remove all abuses of posix_acl_from_xattr()
for setxattr() operations and replace it with calls to vfs_set_acl_prepare().

The new vfs_set_acl_prepare() helper allows us to deal with the
ambiguity in how the POSI ACL uapi struct stores {g,u}id values
depending on whether this is a getxattr() or setxattr() operation.

This also allows us to remove the posix_acl_setxattr_idmapped_mnt()
helper reducing the abuse of the POSIX ACL uapi format to pass values
that should be distinct types in {g,u}id values stored as
ACL_{GROUP,USER} entries.

The removal of posix_acl_setxattr_idmapped_mnt() in turn allows us to
re-constify the value parameter of vfs_setxattr() which in turn allows
us to avoid the nasty cast from a const void pointer to a non-const void
pointer on ovl_do_setxattr().

Ultimately, the plan is to get rid of the type violations completely and
never pass the values from k{g,u}id_t as raw {g,u}id in ACL_{GROUP,USER}
entries in uapi POSIX ACL format. But that's a longer way to go and this
is a preparatory step.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Co-Developed-by: Seth Forshee <sforshee@digitalocean.com>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/posix_acl.c                  | 213 ++++++++++++++++++++++++++++++--
 include/linux/posix_acl_xattr.h |   3 +
 2 files changed, 205 insertions(+), 11 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index abe387700ba9..31eac28e6582 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -857,12 +857,32 @@ void posix_acl_fix_xattr_to_user(void *value, size_t size)
 	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
 }
 
-/*
- * Convert from extended attribute to in-memory representation.
+/**
+ * make_posix_acl - convert POSIX ACLs from uapi to VFS format using the
+ *                  provided callbacks to map ACL_{GROUP,USER} entries into the
+ *                  appropriate format
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @value: the uapi representation of POSIX ACLs
+ * @size: the size of @void
+ * @uid_cb: callback to use for mapping the uid stored in ACL_USER entries
+ * @gid_cb: callback to use for mapping the gid stored in ACL_GROUP entries
+ *
+ * The make_posix_acl() helper is an abstraction to translate from uapi format
+ * into the VFS format allowing the caller to specific callbacks to map
+ * ACL_{GROUP,USER} entries into the expected format. This is used in
+ * posix_acl_from_xattr() and vfs_set_acl_prepare() and avoids pointless code
+ * duplication.
+ *
+ * Return: Allocated struct posix_acl on success, NULL for a valid header but
+ *         without actual POSIX ACL entries, or ERR_PTR() encoded error code.
  */
-struct posix_acl *
-posix_acl_from_xattr(struct user_namespace *user_ns,
-		     const void *value, size_t size)
+static struct posix_acl *make_posix_acl(struct user_namespace *mnt_userns,
+	struct user_namespace *fs_userns, const void *value, size_t size,
+	kuid_t (*uid_cb)(struct user_namespace *, struct user_namespace *,
+			 const struct posix_acl_xattr_entry *),
+	kgid_t (*gid_cb)(struct user_namespace *, struct user_namespace *,
+			 const struct posix_acl_xattr_entry *))
 {
 	const struct posix_acl_xattr_header *header = value;
 	const struct posix_acl_xattr_entry *entry = (const void *)(header + 1), *end;
@@ -893,16 +913,12 @@ posix_acl_from_xattr(struct user_namespace *user_ns,
 				break;
 
 			case ACL_USER:
-				acl_e->e_uid =
-					make_kuid(user_ns,
-						  le32_to_cpu(entry->e_id));
+				acl_e->e_uid = uid_cb(mnt_userns, fs_userns, entry);
 				if (!uid_valid(acl_e->e_uid))
 					goto fail;
 				break;
 			case ACL_GROUP:
-				acl_e->e_gid =
-					make_kgid(user_ns,
-						  le32_to_cpu(entry->e_id));
+				acl_e->e_gid = gid_cb(mnt_userns, fs_userns, entry);
 				if (!gid_valid(acl_e->e_gid))
 					goto fail;
 				break;
@@ -917,6 +933,181 @@ posix_acl_from_xattr(struct user_namespace *user_ns,
 	posix_acl_release(acl);
 	return ERR_PTR(-EINVAL);
 }
+
+/**
+ * vfs_set_acl_prepare_kuid - map ACL_USER uid according to mount- and
+ *                            filesystem idmapping
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @e: a ACL_USER entry in POSIX ACL uapi format
+ *
+ * The uid stored as ACL_USER entry in @e is a kuid_t stored as a raw {g,u}id
+ * value. The vfs_set_acl_prepare_kuid() will recover the kuid_t through
+ * KUIDT_INIT() and then map it according to the idmapped mount. The resulting
+ * kuid_t is the value which the filesystem can map up into a raw backing store
+ * id in the filesystem's idmapping.
+ *
+ * This is used in vfs_set_acl_prepare() to generate the proper VFS
+ * representation of POSIX ACLs with ACL_USER entries during setxattr().
+ *
+ * Return: A kuid in @fs_userns for the uid stored in @e.
+ */
+static inline kuid_t
+vfs_set_acl_prepare_kuid(struct user_namespace *mnt_userns,
+			 struct user_namespace *fs_userns,
+			 const struct posix_acl_xattr_entry *e)
+{
+	kuid_t kuid = KUIDT_INIT(le32_to_cpu(e->e_id));
+	return from_vfsuid(mnt_userns, fs_userns, VFSUIDT_INIT(kuid));
+}
+
+/**
+ * vfs_set_acl_prepare_kgid - map ACL_GROUP gid according to mount- and
+ *                            filesystem idmapping
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @e: a ACL_GROUP entry in POSIX ACL uapi format
+ *
+ * The gid stored as ACL_GROUP entry in @e is a kgid_t stored as a raw {g,u}id
+ * value. The vfs_set_acl_prepare_kgid() will recover the kgid_t through
+ * KGIDT_INIT() and then map it according to the idmapped mount. The resulting
+ * kgid_t is the value which the filesystem can map up into a raw backing store
+ * id in the filesystem's idmapping.
+ *
+ * This is used in vfs_set_acl_prepare() to generate the proper VFS
+ * representation of POSIX ACLs with ACL_GROUP entries during setxattr().
+ *
+ * Return: A kgid in @fs_userns for the gid stored in @e.
+ */
+static inline kgid_t
+vfs_set_acl_prepare_kgid(struct user_namespace *mnt_userns,
+			 struct user_namespace *fs_userns,
+			 const struct posix_acl_xattr_entry *e)
+{
+	kgid_t kgid = KGIDT_INIT(le32_to_cpu(e->e_id));
+	return from_vfsgid(mnt_userns, fs_userns, VFSGIDT_INIT(kgid));
+}
+
+/**
+ * vfs_set_acl_prepare - convert POSIX ACLs from uapi to VFS format taking
+ *                       mount and filesystem idmappings into account
+ * @mnt_userns: the mount's idmapping
+ * @fs_userns: the filesystem's idmapping
+ * @value: the uapi representation of POSIX ACLs
+ * @size: the size of @void
+ *
+ * When setting POSIX ACLs with ACL_{GROUP,USER} entries they need to be
+ * mapped according to the relevant mount- and filesystem idmapping. It is
+ * important that the ACL_{GROUP,USER} entries in struct posix_acl will be
+ * mapped into k{g,u}id_t that are supposed to be mapped up in the filesystem
+ * idmapping. This is crucial since the resulting struct posix_acl might be
+ * cached filesystem wide. The vfs_set_acl_prepare() function will take care to
+ * perform all necessary idmappings.
+ *
+ * Note, that since basically forever the {g,u}id values encoded as
+ * ACL_{GROUP,USER} entries in the uapi POSIX ACLs passed via @value contain
+ * values that have been mapped according to the caller's idmapping. In other
+ * words, POSIX ACLs passed in uapi format as @value during setxattr() contain
+ * {g,u}id values in their ACL_{GROUP,USER} entries that should actually have
+ * been stored as k{g,u}id_t.
+ *
+ * This means, vfs_set_acl_prepare() needs to first recover the k{g,u}id_t by
+ * calling K{G,U}IDT_INIT(). Afterwards they can be interpreted as vfs{g,u}id_t
+ * through from_vfs{g,u}id() to account for any idmapped mounts. The
+ * vfs_set_acl_prepare_k{g,u}id() helpers will take care to generate the
+ * correct k{g,u}id_t.
+ *
+ * The filesystem will then receive the POSIX ACLs ready to be cached
+ * filesystem wide and ready to be written to the backing store taking the
+ * filesystem's idmapping into account.
+ *
+ * Return: Allocated struct posix_acl on success, NULL for a valid header but
+ *         without actual POSIX ACL entries, or ERR_PTR() encoded error code.
+ */
+struct posix_acl *vfs_set_acl_prepare(struct user_namespace *mnt_userns,
+				      struct user_namespace *fs_userns,
+				      const void *value, size_t size)
+{
+	return make_posix_acl(mnt_userns, fs_userns, value, size,
+			      vfs_set_acl_prepare_kuid,
+			      vfs_set_acl_prepare_kgid);
+}
+EXPORT_SYMBOL(vfs_set_acl_prepare);
+
+/**
+ * posix_acl_from_xattr_kuid - map ACL_USER uid into filesystem idmapping
+ * @mnt_userns: unused
+ * @fs_userns: the filesystem's idmapping
+ * @e: a ACL_USER entry in POSIX ACL uapi format
+ *
+ * Map the uid stored as ACL_USER entry in @e into the filesystem's idmapping.
+ * This is used in posix_acl_from_xattr() to generate the proper VFS
+ * representation of POSIX ACLs with ACL_USER entries.
+ *
+ * Return: A kuid in @fs_userns for the uid stored in @e.
+ */
+static inline kuid_t
+posix_acl_from_xattr_kuid(struct user_namespace *mnt_userns,
+			  struct user_namespace *fs_userns,
+			  const struct posix_acl_xattr_entry *e)
+{
+	return make_kuid(fs_userns, le32_to_cpu(e->e_id));
+}
+
+/**
+ * posix_acl_from_xattr_kgid - map ACL_GROUP gid into filesystem idmapping
+ * @mnt_userns: unused
+ * @fs_userns: the filesystem's idmapping
+ * @e: a ACL_GROUP entry in POSIX ACL uapi format
+ *
+ * Map the gid stored as ACL_GROUP entry in @e into the filesystem's idmapping.
+ * This is used in posix_acl_from_xattr() to generate the proper VFS
+ * representation of POSIX ACLs with ACL_GROUP entries.
+ *
+ * Return: A kgid in @fs_userns for the gid stored in @e.
+ */
+static inline kgid_t
+posix_acl_from_xattr_kgid(struct user_namespace *mnt_userns,
+			  struct user_namespace *fs_userns,
+			  const struct posix_acl_xattr_entry *e)
+{
+	return make_kgid(fs_userns, le32_to_cpu(e->e_id));
+}
+
+/**
+ * posix_acl_from_xattr - convert POSIX ACLs from backing store to VFS format
+ * @fs_userns: the filesystem's idmapping
+ * @value: the uapi representation of POSIX ACLs
+ * @size: the size of @void
+ *
+ * Filesystems that store POSIX ACLs in the unaltered uapi format should use
+ * posix_acl_from_xattr() when reading them from the backing store and
+ * converting them into the struct posix_acl VFS format. The helper is
+ * specifically intended to be called from the ->get_acl() inode operation.
+ *
+ * The posix_acl_from_xattr() function will map the raw {g,u}id values stored
+ * in ACL_{GROUP,USER} entries into the filesystem idmapping in @fs_userns. The
+ * posix_acl_from_xattr_k{g,u}id() helpers will take care to generate the
+ * correct k{g,u}id_t. The returned struct posix_acl can be cached.
+ *
+ * Note that posix_acl_from_xattr() does not take idmapped mounts into account.
+ * If it did it calling is from the ->get_acl() inode operation would return
+ * POSIX ACLs mapped according to an idmapped mount which would mean that the
+ * value couldn't be cached for the filesystem. Idmapped mounts are taken into
+ * account on the fly during permission checking or right at the VFS -
+ * userspace boundary before reporting them to the user.
+ *
+ * Return: Allocated struct posix_acl on success, NULL for a valid header but
+ *         without actual POSIX ACL entries, or ERR_PTR() encoded error code.
+ */
+struct posix_acl *
+posix_acl_from_xattr(struct user_namespace *fs_userns,
+		     const void *value, size_t size)
+{
+	return make_posix_acl(&init_user_ns, fs_userns, value, size,
+			      posix_acl_from_xattr_kuid,
+			      posix_acl_from_xattr_kgid);
+}
 EXPORT_SYMBOL (posix_acl_from_xattr);
 
 /*
diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
index b6bd3eac2bcc..47eca15fd842 100644
--- a/include/linux/posix_acl_xattr.h
+++ b/include/linux/posix_acl_xattr.h
@@ -66,6 +66,9 @@ struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,
 				       const void *value, size_t size);
 int posix_acl_to_xattr(struct user_namespace *user_ns,
 		       const struct posix_acl *acl, void *buffer, size_t size);
+struct posix_acl *vfs_set_acl_prepare(struct user_namespace *mnt_userns,
+				      struct user_namespace *fs_userns,
+				      const void *value, size_t size);
 
 extern const struct xattr_handler posix_acl_access_xattr_handler;
 extern const struct xattr_handler posix_acl_default_xattr_handler;
-- 
2.34.1

[PATCH 4/6] acl: move idmapping handling into posix_acl_xattr_set()

[Christian Brauner]

The uapi POSIX ACL struct passed through the value argument during
setxattr() contains {g,u}id values encoded via ACL_{GROUP,USER} entries
that should actually be stored in the form of k{g,u}id_t (See [1] for a
long explanation of the issue.).

In 0c5fd887d2bb ("acl: move idmapped mount fixup into vfs_{g,s}etxattr()")
we took the mount's idmapping into account in order to let overlayfs
handle POSIX ACLs on idmapped layers correctly. The fixup is currently
performed directly in vfs_setxattr() which piles on top of the earlier
hackiness by handling the mount's idmapping and stuff the vfs{g,u}id_t
values into the uapi struct as well. While that is all correct and works
fine it's just ugly.

Now that we have introduced vfs_make_posix_acl() earlier move handling
idmapped mounts out of vfs_setxattr() and into the POSIX ACL handler
where it belongs.

Note that we also need to call vfs_make_posix_acl() for EVM which
interpretes POSIX ACLs during security_inode_setxattr(). Leave them a
longer comment for future reference.

All filesystems that support idmapped mounts via FS_ALLOW_IDMAP use the
standard POSIX ACL xattr handlers and are covered by this change. This
includes overlayfs which simply calls vfs_{g,s}etxattr().

The following filesystems use custom POSIX ACL xattr handlers: 9p, cifs,
ecryptfs, and ntfs3 (and overlayfs but we've covered that in the paragraph
above) and none of them support idmapped mounts yet.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org/ [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/posix_acl.c                    | 52 +++++++------------------------
 fs/xattr.c                        |  3 --
 security/integrity/evm/evm_main.c | 17 ++++++++--
 3 files changed, 25 insertions(+), 47 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 31eac28e6582..c759b8eef62e 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -771,46 +771,6 @@ void posix_acl_getxattr_idmapped_mnt(struct user_namespace *mnt_userns,
 	}
 }
 
-void posix_acl_setxattr_idmapped_mnt(struct user_namespace *mnt_userns,
-				     const struct inode *inode,
-				     void *value, size_t size)
-{
-	struct posix_acl_xattr_header *header = value;
-	struct posix_acl_xattr_entry *entry = (void *)(header + 1), *end;
-	struct user_namespace *fs_userns = i_user_ns(inode);
-	int count;
-	vfsuid_t vfsuid;
-	vfsgid_t vfsgid;
-	kuid_t uid;
-	kgid_t gid;
-
-	if (no_idmapping(mnt_userns, i_user_ns(inode)))
-		return;
-
-	count = posix_acl_fix_xattr_common(value, size);
-	if (count <= 0)
-		return;
-
-	for (end = entry + count; entry != end; entry++) {
-		switch (le16_to_cpu(entry->e_tag)) {
-		case ACL_USER:
-			uid = make_kuid(&init_user_ns, le32_to_cpu(entry->e_id));
-			vfsuid = VFSUIDT_INIT(uid);
-			uid = from_vfsuid(mnt_userns, fs_userns, vfsuid);
-			entry->e_id = cpu_to_le32(from_kuid(&init_user_ns, uid));
-			break;
-		case ACL_GROUP:
-			gid = make_kgid(&init_user_ns, le32_to_cpu(entry->e_id));
-			vfsgid = VFSGIDT_INIT(gid);
-			gid = from_vfsgid(mnt_userns, fs_userns, vfsgid);
-			entry->e_id = cpu_to_le32(from_kgid(&init_user_ns, gid));
-			break;
-		default:
-			break;
-		}
-	}
-}
-
 static void posix_acl_fix_xattr_userns(
 	struct user_namespace *to, struct user_namespace *from,
 	void *value, size_t size)
@@ -1211,7 +1171,17 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
 	int ret;
 
 	if (value) {
-		acl = posix_acl_from_xattr(&init_user_ns, value, size);
+		/*
+		 * By the time we end up here the {g,u}ids stored in
+		 * ACL_{GROUP,USER} have already been mapped according to the
+		 * caller's idmapping. The vfs_set_acl_prepare() helper will
+		 * recover them and take idmapped mounts into account. The
+		 * filesystem will receive the POSIX ACLs in in the correct
+		 * format ready to be cached or written to the backing store
+		 * taking the filesystem idmapping into account.
+		 */
+		acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
+					  value, size);
 		if (IS_ERR(acl))
 			return PTR_ERR(acl);
 	}
diff --git a/fs/xattr.c b/fs/xattr.c
index a1f4998bc6be..3ac68ec0c023 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -305,9 +305,6 @@ vfs_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		size = error;
 	}
 
-	if (size && is_posix_acl_xattr(name))
-		posix_acl_setxattr_idmapped_mnt(mnt_userns, inode, value, size);
-
 retry_deleg:
 	inode_lock(inode);
 	error = __vfs_setxattr_locked(mnt_userns, dentry, name, value, size,
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2e6fb6e2ffd2..23d484e05e6f 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -457,10 +457,21 @@ static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
 	int rc;
 
 	/*
-	 * user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact
-	 * on the inode mode (see posix_acl_equiv_mode()).
+	 * An earlier comment here mentioned that the idmappings for
+	 * ACL_{GROUP,USER} don't matter since EVM is only interested in the
+	 * mode stored as part of POSIX ACLs. Nonetheless, if it must translate
+	 * from the uapi POSIX ACL representation to the VFS internal POSIX ACL
+	 * representation it should do so correctly. There's no guarantee that
+	 * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters
+	 * for the mode at some point and it's difficult to keep track of all
+	 * the LSM and integrity modules and what they do to POSIX ACLs.
+	 *
+	 * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX
+	 * ACLs it received. It requires knowledge that only the VFS is
+	 * guaranteed to have.
 	 */
-	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
+	acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
+				  xattr_value, xattr_value_len);
 	if (IS_ERR_OR_NULL(acl))
 		return 1;
 
-- 
2.34.1

[PATCH 5/6] ovl: use vfs_set_acl_prepare()

[Christian Brauner]

The posix_acl_from_xattr() helper should mainly be used in
i_op->get_acl() handlers. It translates from the uapi struct into the
kernel internal POSIX ACL representation and doesn't care about mount
idmappings.

Use the vfs_set_acl_prepare() helper to generate a kernel internal POSIX
ACL representation in struct posix_acl format taking care to map from
the mount idmapping into the filesystem's idmapping.

The returned struct posix_acl is in the correct format to be cached by
the VFS or passed to the filesystem's i_op->set_acl() method to write to
the backing store.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/overlayfs/super.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ec746d447f1b..5da771b218d1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1022,7 +1022,20 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler,
 
 	/* Check that everything is OK before copy-up */
 	if (value) {
-		acl = posix_acl_from_xattr(&init_user_ns, value, size);
+		/* The above comment can be understood in two ways:
+		 *
+		 * 1. We just want to check whether the basic POSIX ACL format
+		 *    is ok. For example, if the header is correct and the size
+		 *    is sane.
+		 * 2. We want to know whether the ACL_{GROUP,USER} entries can
+		 *    be mapped according to the underlying filesystem.
+		 *
+		 * Currently, we only check 1. If we wanted to check 2. we
+		 * would need to pass the mnt_userns and the fs_userns of the
+		 * underlying filesystem. But frankly, I think checking 1. is
+		 * enough to start the copy-up.
+		 */
+		acl = vfs_set_acl_prepare(&init_user_ns, &init_user_ns, value, size);
 		if (IS_ERR(acl))
 			return PTR_ERR(acl);
 	}
-- 
2.34.1

[PATCH 6/6] xattr: constify value argument in vfs_setxattr()

[Christian Brauner]

Now that we don't perform translations directly in vfs_setxattr()
anymore we can constify the @value argument in vfs_setxattr(). This also
allows us to remove the hack to cast from a const in ovl_do_setxattr().

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/overlayfs/overlayfs.h | 2 +-
 fs/xattr.c               | 5 ++---
 include/linux/xattr.h    | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..ee93c825b06b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -250,7 +250,7 @@ static inline int ovl_do_setxattr(struct ovl_fs *ofs, struct dentry *dentry,
 				  size_t size, int flags)
 {
 	int err = vfs_setxattr(ovl_upper_mnt_userns(ofs), dentry, name,
-			       (void *)value, size, flags);
+			       value, size, flags);
 
 	pr_debug("setxattr(%pd2, \"%s\", \"%*pE\", %zu, %d) = %i\n",
 		 dentry, name, min((int)size, 48), value, size, flags, err);
diff --git a/fs/xattr.c b/fs/xattr.c
index 3ac68ec0c023..74fc8e021ebc 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -290,7 +290,7 @@ static inline bool is_posix_acl_xattr(const char *name)
 
 int
 vfs_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
-	     const char *name, void *value, size_t size, int flags)
+	     const char *name, const void *value, size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
 	struct inode *delegated_inode = NULL;
@@ -298,8 +298,7 @@ vfs_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	int error;
 
 	if (size && strcmp(name, XATTR_NAME_CAPS) == 0) {
-		error = cap_convert_nscap(mnt_userns, dentry,
-					  (const void **)&value, size);
+		error = cap_convert_nscap(mnt_userns, dentry, &value, size);
 		if (error < 0)
 			return error;
 		size = error;
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 979a9d3e5bfb..4c379d23ec6e 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -61,7 +61,7 @@ int __vfs_setxattr_locked(struct user_namespace *, struct dentry *,
 			  const char *, const void *, size_t, int,
 			  struct inode **);
 int vfs_setxattr(struct user_namespace *, struct dentry *, const char *,
-		 void *, size_t, int);
+		 const void *, size_t, int);
 int __vfs_removexattr(struct user_namespace *, struct dentry *, const char *);
 int __vfs_removexattr_locked(struct user_namespace *, struct dentry *,
 			     const char *, struct inode **);
-- 
2.34.1

Re: [PATCH 1/6] ntfs3: rework xattr handlers and switch to POSIX ACL VFS helpers

[Christian Brauner]

[Sorry, forgot to Cc ntfs3 developers.]

On Mon, Aug 29, 2022 at 02:38:40PM +0200, Christian Brauner wrote:
> The xattr code in ntfs3 is currently a bit confused. For example, it
> defines a POSIX ACL i_op->set_acl() method but instead of relying on the
> generic POSIX ACL VFS helpers it defines its own set of xattr helpers
> with the consequence that i_op->set_acl() is currently dead code.
> 
> Switch ntfs3 to rely on the VFS POSIX ACL xattr handlers. Also remove
> i_op->{g,s}et_acl() methods from symlink inode operations. Symlinks
> don't support xattrs.
> 
> This is a preliminary change for the following patches which move
> handling idmapped mounts directly in posix_acl_xattr_set().
> 
> This survives POSIX ACL xfstests.
> 
> Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>  fs/ntfs3/inode.c |   2 -
>  fs/ntfs3/xattr.c | 102 +++--------------------------------------------
>  2 files changed, 6 insertions(+), 98 deletions(-)
> 
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index 51363d4e8636..26a76ebfe58f 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -1927,8 +1927,6 @@ const struct inode_operations ntfs_link_inode_operations = {
>  	.setattr	= ntfs3_setattr,
>  	.listxattr	= ntfs_listxattr,
>  	.permission	= ntfs_permission,
> -	.get_acl	= ntfs_get_acl,
> -	.set_acl	= ntfs_set_acl,
>  };
>  
>  const struct address_space_operations ntfs_aops = {
> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
> index 6ae1f56b7358..7de8718c68a9 100644
> --- a/fs/ntfs3/xattr.c
> +++ b/fs/ntfs3/xattr.c
> @@ -625,67 +625,6 @@ int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
>  	return ntfs_set_acl_ex(mnt_userns, inode, acl, type, false);
>  }
>  
> -static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns,
> -			      struct inode *inode, int type, void *buffer,
> -			      size_t size)
> -{
> -	struct posix_acl *acl;
> -	int err;
> -
> -	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
> -		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
> -		return -EOPNOTSUPP;
> -	}
> -
> -	acl = ntfs_get_acl(inode, type, false);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -
> -	if (!acl)
> -		return -ENODATA;
> -
> -	err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> -	posix_acl_release(acl);
> -
> -	return err;
> -}
> -
> -static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns,
> -			      struct inode *inode, int type, const void *value,
> -			      size_t size)
> -{
> -	struct posix_acl *acl;
> -	int err;
> -
> -	if (!(inode->i_sb->s_flags & SB_POSIXACL)) {
> -		ntfs_inode_warn(inode, "add mount option \"acl\" to use acl");
> -		return -EOPNOTSUPP;
> -	}
> -
> -	if (!inode_owner_or_capable(mnt_userns, inode))
> -		return -EPERM;
> -
> -	if (!value) {
> -		acl = NULL;
> -	} else {
> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -		if (IS_ERR(acl))
> -			return PTR_ERR(acl);
> -
> -		if (acl) {
> -			err = posix_acl_valid(&init_user_ns, acl);
> -			if (err)
> -				goto release_and_out;
> -		}
> -	}
> -
> -	err = ntfs_set_acl(mnt_userns, inode, acl, type);
> -
> -release_and_out:
> -	posix_acl_release(acl);
> -	return err;
> -}
> -
>  /*
>   * ntfs_init_acl - Initialize the ACLs of a new inode.
>   *
> @@ -852,23 +791,6 @@ static int ntfs_getxattr(const struct xattr_handler *handler, struct dentry *de,
>  		goto out;
>  	}
>  
> -#ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
> -	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
> -		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
> -	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
> -	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
> -		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
> -		/* TODO: init_user_ns? */
> -		err = ntfs_xattr_get_acl(
> -			&init_user_ns, inode,
> -			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
> -				? ACL_TYPE_ACCESS
> -				: ACL_TYPE_DEFAULT,
> -			buffer, size);
> -		goto out;
> -	}
> -#endif
>  	/* Deal with NTFS extended attribute. */
>  	err = ntfs_get_ea(inode, name, name_len, buffer, size, NULL);
>  
> @@ -981,22 +903,6 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler,
>  		goto out;
>  	}
>  
> -#ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -	if ((name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1 &&
> -	     !memcmp(name, XATTR_NAME_POSIX_ACL_ACCESS,
> -		     sizeof(XATTR_NAME_POSIX_ACL_ACCESS))) ||
> -	    (name_len == sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1 &&
> -	     !memcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
> -		     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT)))) {
> -		err = ntfs_xattr_set_acl(
> -			mnt_userns, inode,
> -			name_len == sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1
> -				? ACL_TYPE_ACCESS
> -				: ACL_TYPE_DEFAULT,
> -			value, size);
> -		goto out;
> -	}
> -#endif
>  	/* Deal with NTFS extended attribute. */
>  	err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0);
>  
> @@ -1086,7 +992,7 @@ static bool ntfs_xattr_user_list(struct dentry *dentry)
>  }
>  
>  // clang-format off
> -static const struct xattr_handler ntfs_xattr_handler = {
> +static const struct xattr_handler ntfs_other_xattr_handler = {
>  	.prefix	= "",
>  	.get	= ntfs_getxattr,
>  	.set	= ntfs_setxattr,
> @@ -1094,7 +1000,11 @@ static const struct xattr_handler ntfs_xattr_handler = {
>  };
>  
>  const struct xattr_handler *ntfs_xattr_handlers[] = {
> -	&ntfs_xattr_handler,
> +#ifdef CONFIG_NTFS3_FS_POSIX_ACL
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
> +#endif
> +	&ntfs_other_xattr_handler,
>  	NULL,
>  };
>  // clang-format on
> -- 
> 2.34.1
> 

Re: [PATCH 5/6] ovl: use vfs_set_acl_prepare()

[Christian Brauner]

[Sorry, forgot to Cc ovl developers on accident.]

On Mon, Aug 29, 2022 at 02:38:44PM +0200, Christian Brauner wrote:
> The posix_acl_from_xattr() helper should mainly be used in
> i_op->get_acl() handlers. It translates from the uapi struct into the
> kernel internal POSIX ACL representation and doesn't care about mount
> idmappings.
> 
> Use the vfs_set_acl_prepare() helper to generate a kernel internal POSIX
> ACL representation in struct posix_acl format taking care to map from
> the mount idmapping into the filesystem's idmapping.
> 
> The returned struct posix_acl is in the correct format to be cached by
> the VFS or passed to the filesystem's i_op->set_acl() method to write to
> the backing store.
> 
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>  fs/overlayfs/super.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ec746d447f1b..5da771b218d1 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1022,7 +1022,20 @@ ovl_posix_acl_xattr_set(const struct xattr_handler *handler,
>  
>  	/* Check that everything is OK before copy-up */
>  	if (value) {
> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +		/* The above comment can be understood in two ways:
> +		 *
> +		 * 1. We just want to check whether the basic POSIX ACL format
> +		 *    is ok. For example, if the header is correct and the size
> +		 *    is sane.
> +		 * 2. We want to know whether the ACL_{GROUP,USER} entries can
> +		 *    be mapped according to the underlying filesystem.
> +		 *
> +		 * Currently, we only check 1. If we wanted to check 2. we
> +		 * would need to pass the mnt_userns and the fs_userns of the
> +		 * underlying filesystem. But frankly, I think checking 1. is
> +		 * enough to start the copy-up.
> +		 */
> +		acl = vfs_set_acl_prepare(&init_user_ns, &init_user_ns, value, size);
>  		if (IS_ERR(acl))
>  			return PTR_ERR(acl);
>  	}
> -- 
> 2.34.1
> 

Re: [PATCH 1/6] ntfs3: rework xattr handlers and switch to POSIX ACL VFS helpers

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:40PM +0200, Christian Brauner wrote:
> The xattr code in ntfs3 is currently a bit confused. For example, it
> defines a POSIX ACL i_op->set_acl() method but instead of relying on the
> generic POSIX ACL VFS helpers it defines its own set of xattr helpers
> with the consequence that i_op->set_acl() is currently dead code.
> 
> Switch ntfs3 to rely on the VFS POSIX ACL xattr handlers. Also remove
> i_op->{g,s}et_acl() methods from symlink inode operations. Symlinks
> don't support xattrs.
> 
> This is a preliminary change for the following patches which move
> handling idmapped mounts directly in posix_acl_xattr_set().
> 
> This survives POSIX ACL xfstests.
> 
> Fixes: be71b5cba2e6 ("fs/ntfs3: Add attrib operations")
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>>

Re: [PATCH 2/6] acl: return EOPNOTSUPP in posix_acl_fix_xattr_common()

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:41PM +0200, Christian Brauner wrote:
> Return EOPNOTSUPP when the POSIX ACL version doesn't match and zero if
> there are no entries. This will allow us to reuse the helper in
> posix_acl_from_xattr(). This change will have no user visible effects.
> 
> Fixes: 0c5fd887d2bb ("acl: move idmapped mount fixup into vfs_{g,s}etxattr()")
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>>

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:42PM +0200, Christian Brauner wrote:
> Various filesystems store POSIX ACLs on the backing store in their uapi
> format. Such filesystems need to translate from the uapi POSIX ACL
> format into the VFS format during i_op->get_acl(). The VFS provides the
> posix_acl_from_xattr() helper for this task.
> 
> But the usage of posix_acl_from_xattr() is currently ambiguous. It is
> intended to transform from a uapi POSIX ACL  to the VFS represenation.
> For example, when retrieving POSIX ACLs for permission checking during
> lookup or when calling getxattr() to retrieve system.posix_acl_{access,default}.
> 
> Calling posix_acl_from_xattr() during i_op->get_acl() will map the raw
> {g,u}id values stored as ACL_{GROUP,USER} entries in the uapi POSIX ACL
> format into k{g,u}id_t in the filesystem's idmapping and return a struct
> posix_acl ready to be returned to the VFS for caching and to perform
> permission checks on.
> 
> However, posix_acl_from_xattr() is also called during setxattr() for all
> filesystems that rely on VFS provides posix_acl_{access,default}_xattr_handler.
> The posix_acl_xattr_set() handler which is used for the ->set() method
> of posix_acl_{access,default}_xattr_handler uses posix_acl_from_xattr()
> to translate from the uapi POSIX ACL format to the VFS format so that it
> can be passed to the i_op->set_acl() handler of the filesystem or for
> direct caching in case no i_op->set_acl() handler is defined.
> 
> During setxattr() the {g,u}id values stored as ACL_{GROUP,USER} entries
> in the uapi POSIX ACL format aren't raw {g,u}id values that need to be
> mapped according to the filesystem's idmapping. Instead they are {g,u}id
> values in the caller's idmapping which have been generated during
> posix_acl_fix_xattr_from_user(). In other words, they are k{g,u}id_t
> which are passed as raw {g,u}id values abusing the uapi POSIX ACL format
> (Please note that this type safety violation has existed since the
> introduction of k{g,u}id_t. Please see [1] for more details.).
> 
> So when posix_acl_from_xattr() is called in posix_acl_xattr_set() the
> filesystem idmapping is completely irrelevant. Instead, we abuse the
> initial idmapping to recover the k{g,u}id_t base on the value stored in
> raw {g,u}id as ACL_{GROUP,USER} in the uapi POSIX ACL format.
> 
> We need to clearly distinguish betweeen these two operations as it is
> really easy to confuse for filesystems as can be seen in ntfs3.
> 
> In order to do this we factor out make_posix_acl() which takes callbacks
> allowing callers to pass dedicated methods to generate the correct
> k{g,u}id_t. This is just an internal static helper which is not exposed
> to any filesystems but it neatly encapsulates the basic logic of walking
> through a uapi POSIX ACL and returning an allocated VFS POSIX ACL with
> the correct k{g,u}id_t values.
> 
> The posix_acl_from_xattr() helper can then be implemented as a simple
> call to make_posix_acl() with callbacks that generate the correct
> k{g,u}id_t from the raw {g,u}id values in ACL_{GROUP,USER} entries in
> the uapi POSIX ACL format as read from the backing store.
> 
> For setxattr() we add a new helper vfs_set_acl_prepare() which has
> callbacks to map the POSIX ACLs from the uapi format with the k{g,u}id_t
> values stored in raw {g,u}id format in ACL_{GROUP,USER} entries into the
> correct k{g,u}id_t values in the filesystem idmapping. In contrast to
> posix_acl_from_xattr() the vfs_set_acl_prepare() helper needs to take
> the mount idmapping into account. The differences are explained in more
> detail in the kernel doc for the new functions.
> 
> In follow up patches we will remove all abuses of posix_acl_from_xattr()
> for setxattr() operations and replace it with calls to vfs_set_acl_prepare().
> 
> The new vfs_set_acl_prepare() helper allows us to deal with the
> ambiguity in how the POSI ACL uapi struct stores {g,u}id values
> depending on whether this is a getxattr() or setxattr() operation.
> 
> This also allows us to remove the posix_acl_setxattr_idmapped_mnt()
> helper reducing the abuse of the POSIX ACL uapi format to pass values
> that should be distinct types in {g,u}id values stored as
> ACL_{GROUP,USER} entries.
> 
> The removal of posix_acl_setxattr_idmapped_mnt() in turn allows us to
> re-constify the value parameter of vfs_setxattr() which in turn allows
> us to avoid the nasty cast from a const void pointer to a non-const void
> pointer on ovl_do_setxattr().
> 
> Ultimately, the plan is to get rid of the type violations completely and
> never pass the values from k{g,u}id_t as raw {g,u}id in ACL_{GROUP,USER}
> entries in uapi POSIX ACL format. But that's a longer way to go and this
> is a preparatory step.
> 
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Co-Developed-by: Seth Forshee <sforshee@digitalocean.com>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

I can't really give a Reviewed-by as co-author, but this does lgtm. One
nit below however.

> -/*
> - * Convert from extended attribute to in-memory representation.
> +/**
> + * make_posix_acl - convert POSIX ACLs from uapi to VFS format using the
> + *                  provided callbacks to map ACL_{GROUP,USER} entries into the
> + *                  appropriate format
> + * @mnt_userns: the mount's idmapping
> + * @fs_userns: the filesystem's idmapping
> + * @value: the uapi representation of POSIX ACLs
> + * @size: the size of @void

I think you mean "the size of @value"? This appears in a few other
comments too.

Re: [PATCH 4/6] acl: move idmapping handling into posix_acl_xattr_set()

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:43PM +0200, Christian Brauner wrote:
> The uapi POSIX ACL struct passed through the value argument during
> setxattr() contains {g,u}id values encoded via ACL_{GROUP,USER} entries
> that should actually be stored in the form of k{g,u}id_t (See [1] for a
> long explanation of the issue.).
> 
> In 0c5fd887d2bb ("acl: move idmapped mount fixup into vfs_{g,s}etxattr()")
> we took the mount's idmapping into account in order to let overlayfs
> handle POSIX ACLs on idmapped layers correctly. The fixup is currently
> performed directly in vfs_setxattr() which piles on top of the earlier
> hackiness by handling the mount's idmapping and stuff the vfs{g,u}id_t
> values into the uapi struct as well. While that is all correct and works
> fine it's just ugly.
> 
> Now that we have introduced vfs_make_posix_acl() earlier move handling
> idmapped mounts out of vfs_setxattr() and into the POSIX ACL handler
> where it belongs.
> 
> Note that we also need to call vfs_make_posix_acl() for EVM which
> interpretes POSIX ACLs during security_inode_setxattr(). Leave them a
> longer comment for future reference.
> 
> All filesystems that support idmapped mounts via FS_ALLOW_IDMAP use the
> standard POSIX ACL xattr handlers and are covered by this change. This
> includes overlayfs which simply calls vfs_{g,s}etxattr().
> 
> The following filesystems use custom POSIX ACL xattr handlers: 9p, cifs,
> ecryptfs, and ntfs3 (and overlayfs but we've covered that in the paragraph
> above) and none of them support idmapped mounts yet.
> 
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org/ [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

Re: [PATCH 5/6] ovl: use vfs_set_acl_prepare()

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:44PM +0200, Christian Brauner wrote:
> The posix_acl_from_xattr() helper should mainly be used in
> i_op->get_acl() handlers. It translates from the uapi struct into the
> kernel internal POSIX ACL representation and doesn't care about mount
> idmappings.
> 
> Use the vfs_set_acl_prepare() helper to generate a kernel internal POSIX
> ACL representation in struct posix_acl format taking care to map from
> the mount idmapping into the filesystem's idmapping.
> 
> The returned struct posix_acl is in the correct format to be cached by
> the VFS or passed to the filesystem's i_op->set_acl() method to write to
> the backing store.
> 
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

Re: [PATCH 6/6] xattr: constify value argument in vfs_setxattr()

[Seth Forshee]

On Mon, Aug 29, 2022 at 02:38:45PM +0200, Christian Brauner wrote:
> Now that we don't perform translations directly in vfs_setxattr()
> anymore we can constify the @value argument in vfs_setxattr(). This also
> allows us to remove the hack to cast from a const in ovl_do_setxattr().
> 
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

Re: [PATCH 2/6] acl: return EOPNOTSUPP in posix_acl_fix_xattr_common()

[Christoph Hellwig]

On Mon, Aug 29, 2022 at 02:38:41PM +0200, Christian Brauner wrote:
> -static int posix_acl_fix_xattr_common(void *value, size_t size)
> +static int posix_acl_fix_xattr_common(const void *value, size_t size)
>  {
> -	struct posix_acl_xattr_header *header = value;
> +	const struct posix_acl_xattr_header *header = value;

This constification looks unrelated to the rest and isn't documented
in the commit log.

Otherwise looks good:

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

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christoph Hellwig]

On Mon, Aug 29, 2022 at 02:38:42PM +0200, Christian Brauner wrote:
> Various filesystems store POSIX ACLs on the backing store in their uapi
> format. Such filesystems need to translate from the uapi POSIX ACL
> format into the VFS format during i_op->get_acl(). The VFS provides the
> posix_acl_from_xattr() helper for this task.

This has always been rather confusing.  Maybe we should add a separate
structure type for the on-disk vs uapi ACL formats?  They will be the
same in binary representation, but the extra type safety might make the
core a lot more readable.

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner]

On Tue, Sep 06, 2022 at 06:57:46AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 29, 2022 at 02:38:42PM +0200, Christian Brauner wrote:
> > Various filesystems store POSIX ACLs on the backing store in their uapi
> > format. Such filesystems need to translate from the uapi POSIX ACL
> > format into the VFS format during i_op->get_acl(). The VFS provides the
> > posix_acl_from_xattr() helper for this task.
> 
> This has always been rather confusing.  Maybe we should add a separate

Absolutely and it's pretty unsafe given that we're storing k{g,u}id_t in
the uapi struct in the form of {g,u}id_t which we then recover later on.
But I've documented this as best as I could in the helpers.

> structure type for the on-disk vs uapi ACL formats?  They will be the

We do already have separate format for uapi and VFS ACLs. I'm not sure
if you're suggesting another intermediate format.

I'm currently working on a larger series to get rid of the uapi struct
abuse for POSIX ACLs. Building on that work Seth will get rid of similar
abuses for VFS caps. I'm fairly close but the rough idea is:

struct xattr_args {
	const char *name;
	union {
		struct posix_acl *kacl;
		const void *kvalue;
		void *buffer;
	};
	size_t size;
};

struct xattr_handler {
	const char *name;
	const char *prefix;
	int flags;
	bool (*list)(struct dentry *dentry);
	int (*get)(const struct xattr_handler *,
		   struct dentry *dentry,
		   struct inode *inode,
		   struct xattr_args *args);
	int (*set)(const struct xattr_handler *,
		   struct user_namespace *mnt_userns,
		   struct dentry *dentry,
		   struct inode *inode,
		   const struct xattr_args *args,
		   int flags);
};

All __vfs_{g,s}etxattr() helpers stay the same and can't be used to
{g,s}et POSIX ACLs anymore instead we add:

int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
		const char *acl_name, struct posix_acl *acl, int flags)
{
	struct xattr_args xattr_args = {
		.name = acl_name,
	};

	if (!is_posix_acl_xattr(acl_name))
		return -EINVAL;

	return set_xattr_args(mnt_userns, dentry, &xattr_args, flags);
}

int vfs_get_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
		const char *acl_name, struct posix_acl **acl)
{
	int error;
	struct xattr_args xattr_args = {
		.name = acl_name,
	};

	if (!is_posix_acl_xattr(acl_name))
		return -EINVAL;

	error = get_xattr_args(mnt_userns, dentry, &xattr_args);
	if (error < 0)
		return error;

	*acl = xattr_args.kacl;
	return 0;
}

These two vfs helpers can then used by filesystems like overlayfs to set
POSIX ACLs. This gets rid of passing crucial data that the VFS needs to
interpret around in a void * blob as that's causing a lot of issues
currently bc often filesystems or security hooks don't have any idea how
to interpret them correctly.

So the internal vfs api for getxattr() itself would then be:

ssize_t do_get_acl(struct user_namespace *mnt_userns, struct dentry *d,
		   struct xattr_ctx *ctx)
{
	struct posix_acl *kacl = NULL;

	error = vfs_get_acl(mnt_userns, d, ctx->kname->name, &kacl);
	if (error)
		return error;

	/* convert to uapi format */
	if (ctx->size)
		error = vfs_posix_acl_to_xattr(mnt_userns, d_inode(d),
					       ctx->kacl, ctx->kvalue,
					       ctx->size);
	posix_acl_release(kacl);
	return error;
}

ssize_t
do_getxattr(struct user_namespace *mnt_userns, struct dentry *d,
	struct xattr_ctx *ctx)
{
	ssize_t error;
	char *kname = ctx->kname->name;

	if (ctx->size) {
		if (ctx->size > XATTR_SIZE_MAX)
			ctx->size = XATTR_SIZE_MAX;
		ctx->kvalue = kvzalloc(ctx->size, GFP_KERNEL);
		if (!ctx->kvalue)
			return -ENOMEM;
	}

	if (is_posix_acl_xattr(ctx->kname->name))
		error = do_get_acl(mnt_userns, d, ctx);
	else
		error = vfs_getxattr(mnt_userns, d, kname, ctx->kvalue, ctx->size);
	if (error > 0) {
		if (ctx->size && copy_to_user(ctx->value, ctx->kvalue, error))
			error = -EFAULT;
	} else if (error == -ERANGE && ctx->size >= XATTR_SIZE_MAX) {
		/* The file system tried to returned a value bigger
		   than XATTR_SIZE_MAX bytes. Not possible. */
		error = -E2BIG;
	}

	return error;
}

and all the helpers that hack stuff into uapi POSIX ACLs are then gone.
I'm fairly along but I'm happy to hear alternative ideas.

Christian

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christoph Hellwig]

On Tue, Sep 06, 2022 at 09:45:32AM +0200, Christian Brauner wrote:
> > structure type for the on-disk vs uapi ACL formats?  They will be the
> 
> We do already have separate format for uapi and VFS ACLs. I'm not sure
> if you're suggesting another intermediate format.

Right now struct posix_acl_xattr_header and
struct posix_acl_xattr_entry is used both for the UAPI, and the
on-disk format of various file systems, despite the different cases
using different kinds of uids/gids.

> I'm currently working on a larger series to get rid of the uapi struct
> abuse for POSIX ACLs. Building on that work Seth will get rid of similar
> abuses for VFS caps. I'm fairly close but the rough idea is:

Can we just stop accessing ACLs through the xattrs ops at all, and
just have dedicated methods instead?  This whole multiplexing of
ACLs through xattrs APIs has been an unmitigated disaster.

Similar for all other "xattrs" that are not just user data and
interpreted by the kernel, but ACLs are by far the worst.

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner]

On Tue, Sep 06, 2022 at 09:53:13AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 09:45:32AM +0200, Christian Brauner wrote:
> > > structure type for the on-disk vs uapi ACL formats?  They will be the
> > 
> > We do already have separate format for uapi and VFS ACLs. I'm not sure
> > if you're suggesting another intermediate format.
> 
> Right now struct posix_acl_xattr_header and
> struct posix_acl_xattr_entry is used both for the UAPI, and the
> on-disk format of various file systems, despite the different cases
> using different kinds of uids/gids.
> 
> > I'm currently working on a larger series to get rid of the uapi struct
> > abuse for POSIX ACLs. Building on that work Seth will get rid of similar
> > abuses for VFS caps. I'm fairly close but the rough idea is:
> 
> Can we just stop accessing ACLs through the xattrs ops at all, and
> just have dedicated methods instead?  This whole multiplexing of
> ACLs through xattrs APIs has been an unmitigated disaster.

IIuc then this is exactly what I tried to do (I have a still very hacky
version of this approach in
https://gitlab.com/brauner/linux/-/commits/fs.posix_acl.vfsuid/).

I've tried switching all filesystem to simply rely on
i_op->{g,s}et_acl() but this doesn't work for at least 9p and cifs
because they need access to the dentry. cifs hasn't even implemented
i_op->get_acl() and I don't think they can because of the lack of a
dentry argument.

The problem is not just that i_op->{g,s}et_acl() don't take a dentry
argument it's in principle also super annoying to pass it to them
because i_op->get_acl() is used to retrieve POSIX ACLs during permission
checking and thus is called from generic_permission() and thus
inode_permission() and I don't think we want or even can pass down a
dentry everywhere for those. So I stopped short of finishing this
implementation because of that.

So in order to make this work for cifs and 9p we would probably need a
new i_op method that is separate from the i_op->get_acl() one used in
the acl_permission_check() and friends...

> 
> Similar for all other "xattrs" that are not just user data and
> interpreted by the kernel, but ACLs are by far the worst.

I absolutely agree.

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christoph Hellwig]

On Tue, Sep 06, 2022 at 10:07:44AM +0200, Christian Brauner wrote:
> I've tried switching all filesystem to simply rely on
> i_op->{g,s}et_acl() but this doesn't work for at least 9p and cifs
> because they need access to the dentry. cifs hasn't even implemented
> i_op->get_acl() and I don't think they can because of the lack of a
> dentry argument.
> 
> The problem is not just that i_op->{g,s}et_acl() don't take a dentry
> argument it's in principle also super annoying to pass it to them
> because i_op->get_acl() is used to retrieve POSIX ACLs during permission
> checking and thus is called from generic_permission() and thus
> inode_permission() and I don't think we want or even can pass down a
> dentry everywhere for those. So I stopped short of finishing this
> implementation because of that.
> 
> So in order to make this work for cifs and 9p we would probably need a
> new i_op method that is separate from the i_op->get_acl() one used in
> the acl_permission_check() and friends...

Even if we can't use the existing methods, I think adding new
set_denstry_acl/get_dentry_acl (or whatever we name them) methods is
still better than doing this overload of the xattr methods
(just like the uapi overload instead of separate syscalls, but we
can't fix that).

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner]

On Tue, Sep 06, 2022 at 10:15:10AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 06, 2022 at 10:07:44AM +0200, Christian Brauner wrote:
> > I've tried switching all filesystem to simply rely on
> > i_op->{g,s}et_acl() but this doesn't work for at least 9p and cifs
> > because they need access to the dentry. cifs hasn't even implemented
> > i_op->get_acl() and I don't think they can because of the lack of a
> > dentry argument.
> > 
> > The problem is not just that i_op->{g,s}et_acl() don't take a dentry
> > argument it's in principle also super annoying to pass it to them
> > because i_op->get_acl() is used to retrieve POSIX ACLs during permission
> > checking and thus is called from generic_permission() and thus
> > inode_permission() and I don't think we want or even can pass down a
> > dentry everywhere for those. So I stopped short of finishing this
> > implementation because of that.
> > 
> > So in order to make this work for cifs and 9p we would probably need a
> > new i_op method that is separate from the i_op->get_acl() one used in
> > the acl_permission_check() and friends...
> 
> Even if we can't use the existing methods, I think adding new
> set_denstry_acl/get_dentry_acl (or whatever we name them) methods is
> still better than doing this overload of the xattr methods
> (just like the uapi overload instead of separate syscalls, but we
> can't fix that).

Let me explore and see if I can finish the branch using dedicated i_op
methods instead of updating i_op->get_acl().

I think any data that requires to be interpreteted by the VFS needs to
have dedicated methods. Seth's branch for example, tries to add
i_op->{g,s}et_vfs_caps() for vfs caps which also store ownership
information instead of hacking it through the xattr api like we do now.

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christoph Hellwig]

On Tue, Sep 06, 2022 at 10:24:28AM +0200, Christian Brauner wrote:
> I think any data that requires to be interpreteted by the VFS needs to
> have dedicated methods. Seth's branch for example, tries to add
> i_op->{g,s}et_vfs_caps() for vfs caps which also store ownership
> information instead of hacking it through the xattr api like we do now.

Yes.  Although with LSMs this will become really messy, but then again
creating a complete unreviewable und auditable mess is what the LSM
infrastructure was created for to start with..

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner]

On Tue, Sep 06, 2022 at 10:24:32AM +0200, Christian Brauner wrote:
> On Tue, Sep 06, 2022 at 10:15:10AM +0200, Christoph Hellwig wrote:
> > On Tue, Sep 06, 2022 at 10:07:44AM +0200, Christian Brauner wrote:
> > > I've tried switching all filesystem to simply rely on
> > > i_op->{g,s}et_acl() but this doesn't work for at least 9p and cifs
> > > because they need access to the dentry. cifs hasn't even implemented
> > > i_op->get_acl() and I don't think they can because of the lack of a
> > > dentry argument.
> > > 
> > > The problem is not just that i_op->{g,s}et_acl() don't take a dentry
> > > argument it's in principle also super annoying to pass it to them
> > > because i_op->get_acl() is used to retrieve POSIX ACLs during permission
> > > checking and thus is called from generic_permission() and thus
> > > inode_permission() and I don't think we want or even can pass down a
> > > dentry everywhere for those. So I stopped short of finishing this
> > > implementation because of that.
> > > 
> > > So in order to make this work for cifs and 9p we would probably need a
> > > new i_op method that is separate from the i_op->get_acl() one used in
> > > the acl_permission_check() and friends...
> > 
> > Even if we can't use the existing methods, I think adding new
> > set_denstry_acl/get_dentry_acl (or whatever we name them) methods is
> > still better than doing this overload of the xattr methods
> > (just like the uapi overload instead of separate syscalls, but we
> > can't fix that).
> 
> Let me explore and see if I can finish the branch using dedicated i_op
> methods instead of updating i_op->get_acl().
> 
> I think any data that requires to be interpreteted by the VFS needs to
> have dedicated methods. Seth's branch for example, tries to add
> i_op->{g,s}et_vfs_caps() for vfs caps which also store ownership
> information instead of hacking it through the xattr api like we do now.

I finished a draft of the series. It severly lacks in meangingful commit
messages and I won't be able to finish it before Plumbers next week.
If people want to take a look the branch is available on gitlab and
kernel.org:

https://gitlab.com/brauner/linux/-/commits/fs.posix_acl.vfsuid/
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.posix_acl.vfsuid

This passes xfstests (ext4, xfs, btrfs, overlayfs with and without
idmapped layers, and LTP). I only needed to add i_op->get_dentry_acl()
as it was possible to adapt ->set_acl() to take a dentry argument and
not an inode argument.

So we have a dedicated POSIX ACL api:

struct posix_acl *vfs_get_acl(struct user_namespace *mnt_userns,
                              struct dentry *dentry, const char *acl_name)
int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                const char *acl_name, struct posix_acl *kacl)
int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                   const char *acl_name)

only relying on i_op->get_dentry_acl() and i_op->set_acl() removing the
void * and uapi POSIX ACL abuse completely.

Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christoph Hellwig]

On Fri, Sep 09, 2022 at 10:03:39AM +0200, Christian Brauner wrote:
> This passes xfstests (ext4, xfs, btrfs, overlayfs with and without
> idmapped layers, and LTP). I only needed to add i_op->get_dentry_acl()
> as it was possible to adapt ->set_acl() to take a dentry argument and
> not an inode argument.

This looks pretty nice.  Two high level comments:

 - instead of adding lots of stub ->get_dentry_acl іmplementations that
   wrap ->get_acl, just call ->get_acl if ->get_dentry_acl is not
   implementet in the VFS
 - I think the methods that take a dentry should be named consisently,
   so either ->get_dentry_acl and ->get_dentry_acl vs ->get_acl,
   or ->get_acl and ->set_acl vs ->get_inode_acl or something like that.