[RFC PATCH 00/29] acl: add vfs posix acl api

[RFC PATCH 00/29] acl: add vfs posix acl api

[Christian Brauner]

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

Hey everyone,

As we discussed and seen multiple times the current state of how posix
acls are handled isn't nice and comes with a lot of problems. For a long
and detailed explanation for just some of the issues [1] provides a good
summary.

The current way of handling posix acls via the generic xattr api is
error prone, hard to maintain, and type unsafe for the vfs until we call
into the filesystem's dedicated get and set inode operations.

It is already the case that posix acls are special-cased to death all
the way through the vfs. There are an uncounted number of hacks that
operate on the uapi posix acl struct instead of the dedicated vfs struct
posix_acl. And the vfs must be involved in order to interpret and fixup
posix acls before storing them to the backing store, caching them,
reporting them to userspace, or for permission checking.

Currently a range of hacks and duct tape exist to make this work. As
with most things this is really no ones fault it's just something that
happened over time. But the code is hard to understand and difficult
to maintain and one is constantly at risk of introducing bugs and
regressions when having to touch it.

Instead of continuing to hack posix acls through the xattr handlers this
series builds a dedicated posix acl api solely around the get and set
inode operations. Going forward, the vfs_get_acl(), vfs_remove_acl(),
and vfs_set_acl() helpers must be used in order to interact with posix
acls. They operate directly on the vfs internal struct posix_acl instead
of abusing the uapi posix acl struct as we currently do. In the end this
removes all of the hackiness, makes the codepaths easier to maintain,
and gets us type safety.

This series passes the LTP and xfstests suites without any regressions.
For xfstests the following combinations were tested:

* xfs
* ext4
* btrfs
* overlayfs
* overlayfs on top of idmapped mounts

For people wanting to run their own xfstests I'd recommend to shorten
their test runs via:

./check -g acl,attr,cap,idmapped,io_uring,perms,subvol,unlink

I would appreciate if the 9p and cifs folks could run any posix acl
related tests as I have no setup to really do this without causing me a
lot of pain.

Very likely there's a lot more simplifications for posix acls that we
can make in the future if the basic api has made it.

A few implementation details:

* The series makes sure to retain exactly the same security and
  integrity module permission checks. See [2] for annotated callchains.
  Especially for the integrity modules this api is a win because right
  now they convert the uapi posix acl struct passed to them via a void
  pointer into the vfs struct posix_acl format to perform permission
  checking on the mode.

  There's a new dedicated security hook for setting posix acls which
  passes the vfs struct posix_acl not a void pointer. Basing checking on
  the posix acl stored in the uapi format is really unreliable. The vfs
  currently hacks around directly in the uapi struct storing values that
  frankly the security and integrity modules can't correctly interpret
  as evidenced by bugs we reported and fixed in this area. It's not
  necessarily even their fault it's just that the format we provide to
  them is sub optimal.

* Some filesystems like 9p and cifs need access to the dentry in order
  to get and set posix acls which is why they either only partially or
  not even at all implement get and set inode operations. For example,
  cifs allows setxattr() and getxattr() operations but doesn't allow
  permission checking based on posix acls because it can't implement a
  get acl inode operation.

  Thus, this patch series updates the set acl inode operation to take a
  dentry instead of an inode argument. However, for the get acl inode
  operation we can't do this as the old get acl method is called in
  e.g., generic_permission() and inode_permission(). These helpers in
  turn are called in various filesystem's permission inode operation. So
  passing a dentry argument to the old get acl inode operation would
  amount to passing a dentry to the permission inode operation which we
  shouldn't and probably can't do.

  So instead of extending the existing inode operation Christoph
  suggested to add a new one. He also requested to ensure that the get
  and set acl inode operation taking a dentry are consistently named. So
  for this version the old get acl operation is renamed to
  ->get_inode_acl() and a new ->get_acl() inode operation taking a
  dentry is added. With this we can give both 9p and cifs get and set
  acl inode operations and in turn remove their complex custom posix
  xattr handlers.

* I've done a full audit of every codepaths using variant of the
  current generic xattr api to get and set posix acls and surprisingly
  it isn't that many places. There's of course always a chance that I
  might have missed some and I'm sure we'll find them soon enough.

  The crucial codepaths to be converted are obviously stacking
  filesystems such as ecryptfs and overlayfs.

  For a list of all callers currently using generic xattr api helpers
  see [2] including comments whether they support posix acls or not.

* The old vfs generic posix acl infrastructure doesn't obey
  the create and replace semantics promised on the setxattr(2) manpage.
  This patch series doesn't address this. It really is something we
  should revisit later though.

The patch series is roughly organized as follows:

// intended to be a non-functional change
1. Change existing set acl inode operation to take a dentry argument.

// intended to be a non-functional change
2. Rename existing get acl method.

// intended to be a non-functional change
3. Implement get and set acl inode operations for filesystems that
   couldn't implement one before because of the missing dentry. That's
   mostly 9p and cifs.

// intended to be a non-functional change
4. Build posix acl api, i.e., add vfs_get_acl(), vfs_remove_acl(), and
   vfs_set_acl() including security and integrity hooks.

// intended to be a non-functional change
5. Implement get and set acl inode operations for stacking filesystems.

// semantical change
6. Switch posix acl handling in stacking filesystems to new posix acl
   api now that all filesystems it can stack upon support it.

// semantical change
7. Switch vfs to new posix acl api

8. Remove all now unused helpers

The series can be pulled from:

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

The series contains a few preliminary patches which are scheduled for
the next merge window. It was just easier to base the series on top of
them. But if you pull this branch you'll get them included.

I've been working on this for a while and before going any further it'd
be nice to get some reviews. I think that it should be fine to have get
and set acl inode operations that operate on the dentry at least nothing
stuck out immediately that would prevent this. But obviously having
other people point out issues with that would be helpful.

Thanks to Seth for a lot of good discussion around this and
encouragement and input from Christoph.

[1]: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org
[2]: https://gist.github.com/brauner/12c795b93a05dc3b3056b1982549a633

Thanks!
Christian

Christian Brauner (29):
  fs: pass dentry to set acl method
  fs: rename current get acl method
  fs: add new get acl method
  cifs: implement get acl method
  cifs: implement set acl method
  9p: implement get acl method
  9p: implement set acl method
  acl: add vfs_set_acl()
  security: add set acl hook
  selinux: implement set acl hook
  smack: implement set acl hook
  evm: implement set acl hook
  acl: use set acl hook
  evm: add post set acl hook
  acl: add vfs_get_acl()
  acl: add vfs_remove_acl()
  evm: simplify evm_xattr_acl_change()
  ksmbd: use vfs_remove_acl()
  ecryptfs: implement get acl method
  ecryptfs: implement set acl method
  ovl: implement get acl method
  ovl: implement set acl method
  ovl: use posix acl api
  xattr: use posix acl api
  ecryptfs: use stub posix acl handlers
  ovl: use stub posix acl handlers
  cifs: use stub posix acl handlers
  9p: use stub posix acl handlers
  acl: remove a slew of now unused helpers

 Documentation/filesystems/locking.rst |   4 +-
 Documentation/filesystems/porting.rst |   4 +-
 Documentation/filesystems/vfs.rst     |   3 +-
 fs/9p/acl.c                           | 307 ++++++------
 fs/9p/acl.h                           |  19 +-
 fs/9p/vfs_inode_dotl.c                |   4 +
 fs/9p/xattr.c                         |   7 +-
 fs/9p/xattr.h                         |   2 -
 fs/bad_inode.c                        |   4 +-
 fs/btrfs/acl.c                        |   3 +-
 fs/btrfs/ctree.h                      |   2 +-
 fs/btrfs/inode.c                      |   8 +-
 fs/ceph/acl.c                         |   3 +-
 fs/ceph/dir.c                         |   2 +-
 fs/ceph/inode.c                       |   4 +-
 fs/ceph/super.h                       |   2 +-
 fs/cifs/cifsacl.c                     | 137 +++++
 fs/cifs/cifsfs.c                      |   4 +
 fs/cifs/cifsproto.h                   |  20 +-
 fs/cifs/cifssmb.c                     | 236 +++++----
 fs/cifs/xattr.c                       |  68 +--
 fs/ecryptfs/inode.c                   |  32 ++
 fs/erofs/inode.c                      |   6 +-
 fs/erofs/namei.c                      |   2 +-
 fs/ext2/acl.c                         |   3 +-
 fs/ext2/acl.h                         |   2 +-
 fs/ext2/file.c                        |   2 +-
 fs/ext2/inode.c                       |   2 +-
 fs/ext2/namei.c                       |   4 +-
 fs/ext4/acl.c                         |   3 +-
 fs/ext4/acl.h                         |   2 +-
 fs/ext4/file.c                        |   2 +-
 fs/ext4/inode.c                       |   2 +-
 fs/ext4/namei.c                       |   4 +-
 fs/f2fs/acl.c                         |   4 +-
 fs/f2fs/acl.h                         |   2 +-
 fs/f2fs/file.c                        |   4 +-
 fs/f2fs/namei.c                       |   4 +-
 fs/fuse/acl.c                         |   3 +-
 fs/fuse/dir.c                         |   4 +-
 fs/fuse/fuse_i.h                      |   2 +-
 fs/gfs2/acl.c                         |   3 +-
 fs/gfs2/acl.h                         |   2 +-
 fs/gfs2/inode.c                       |   6 +-
 fs/internal.h                         |   1 +
 fs/jffs2/acl.c                        |   3 +-
 fs/jffs2/acl.h                        |   2 +-
 fs/jffs2/dir.c                        |   2 +-
 fs/jffs2/file.c                       |   2 +-
 fs/jffs2/fs.c                         |   2 +-
 fs/jfs/acl.c                          |   3 +-
 fs/jfs/file.c                         |   4 +-
 fs/jfs/jfs_acl.h                      |   2 +-
 fs/jfs/namei.c                        |   2 +-
 fs/ksmbd/smb2pdu.c                    |   4 +-
 fs/ksmbd/smbacl.c                     |   4 +-
 fs/ksmbd/vfs.c                        |  17 +-
 fs/ksmbd/vfs.h                        |   4 +-
 fs/namei.c                            |   2 +-
 fs/nfs/nfs3_fs.h                      |   2 +-
 fs/nfs/nfs3acl.c                      |   3 +-
 fs/nfs/nfs3proc.c                     |   4 +-
 fs/nfsd/nfs2acl.c                     |   4 +-
 fs/nfsd/nfs3acl.c                     |   4 +-
 fs/nfsd/vfs.c                         |   4 +-
 fs/ntfs3/file.c                       |   4 +-
 fs/ntfs3/namei.c                      |   4 +-
 fs/ntfs3/ntfs_fs.h                    |   4 +-
 fs/ntfs3/xattr.c                      |   9 +-
 fs/ocfs2/acl.c                        |   3 +-
 fs/ocfs2/acl.h                        |   2 +-
 fs/ocfs2/file.c                       |   4 +-
 fs/ocfs2/namei.c                      |   2 +-
 fs/orangefs/acl.c                     |  47 +-
 fs/orangefs/inode.c                   |  47 +-
 fs/orangefs/namei.c                   |   2 +-
 fs/orangefs/orangefs-kernel.h         |   9 +-
 fs/orangefs/orangefs-utils.c          |  12 +-
 fs/overlayfs/copy_up.c                |   9 +
 fs/overlayfs/dir.c                    |  22 +-
 fs/overlayfs/inode.c                  | 140 +++++-
 fs/overlayfs/overlayfs.h              |  36 +-
 fs/overlayfs/super.c                  | 107 +---
 fs/overlayfs/util.c                   |  42 ++
 fs/posix_acl.c                        | 688 +++++++++++++-------------
 fs/reiserfs/acl.h                     |   6 +-
 fs/reiserfs/file.c                    |   2 +-
 fs/reiserfs/inode.c                   |   2 +-
 fs/reiserfs/namei.c                   |   4 +-
 fs/reiserfs/xattr_acl.c               |   9 +-
 fs/xattr.c                            |  78 ++-
 fs/xfs/xfs_acl.c                      |   3 +-
 fs/xfs/xfs_acl.h                      |   2 +-
 fs/xfs/xfs_iops.c                     |  16 +-
 include/linux/evm.h                   |  22 +
 include/linux/fs.h                    |  10 +-
 include/linux/lsm_hook_defs.h         |   2 +
 include/linux/lsm_hooks.h             |   4 +
 include/linux/posix_acl.h             |  35 +-
 include/linux/posix_acl_xattr.h       |  43 +-
 include/linux/security.h              |  11 +
 include/linux/xattr.h                 |   8 +
 io_uring/xattr.c                      |   2 +
 mm/shmem.c                            |   2 +-
 security/integrity/evm/evm_main.c     | 128 +++--
 security/security.c                   |  16 +
 security/selinux/hooks.c              |   8 +
 security/smack/smack_lsm.c            |  24 +
 108 files changed, 1586 insertions(+), 1062 deletions(-)


base-commit: 38e316398e4e6338b80223fb5f74415c0513718f
-- 
2.34.1

[PATCH 10/29] selinux: implement set acl hook

[Christian Brauner]

The current way of setting and getting posix acls through the generic
xattr interface is error prone and type unsafe. The vfs needs to
interpret and fixup posix acls before storing or reporting it to
userspace. Various hacks exist to make this work. The code is hard to
understand and difficult to maintain in it's current form. Instead of
making this work by hacking posix acls through xattr handlers we are
building a dedicated posix acl api around the get and set inode
operations. This removes a lot of hackiness and makes the codepaths
easier to maintain. A lot of background can be found in [1].

So far posix acls were passed as a void blob to the security and
integrity modules. Some of them like evm then proceed to interpret the
void pointer and convert it into the kernel internal struct posix acl
representation to perform their integrity checking magic. This is
obviously pretty problematic as that requires knowledge that only the
vfs is guaranteed to have and has lead to various bugs. Add a proper
security hook for setting posix acls and pass down the posix acls in
their appropriate vfs format instead of hacking it through a void
pointer stored in the uapi format.

I spent considerate time in the security module infrastructure and
audited all codepaths. SELinux has no restrictions based on the posix
acl values passed through it. The capability hook doesn't need to be
called either because it only has restrictions on security.* xattrs. So
this all becomes a very simple hook for SELinux.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 security/selinux/hooks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 79573504783b..bbc0ce3bde35 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3239,6 +3239,13 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns,
 			    &ad);
 }
 
+static int selinux_inode_set_acl(struct user_namespace *mnt_userns,
+				 struct dentry *dentry, const char *acl_name,
+				 struct posix_acl *kacl)
+{
+	return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+}
+
 static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 					const void *value, size_t size,
 					int flags)
@@ -7063,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
 	LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr),
 	LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
+	LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl),
 	LSM_HOOK_INIT(inode_getsecurity, selinux_inode_getsecurity),
 	LSM_HOOK_INIT(inode_setsecurity, selinux_inode_setsecurity),
 	LSM_HOOK_INIT(inode_listsecurity, selinux_inode_listsecurity),
-- 
2.34.1

[PATCH 12/29] evm: implement set acl hook

[Christian Brauner]

The current way of setting and getting posix acls through the generic
xattr interface is error prone and type unsafe. The vfs needs to
interpret and fixup posix acls before storing or reporting it to
userspace. Various hacks exist to make this work. The code is hard to
understand and difficult to maintain in it's current form. Instead of
making this work by hacking posix acls through xattr handlers we are
building a dedicated posix acl api around the get and set inode
operations. This removes a lot of hackiness and makes the codepaths
easier to maintain. A lot of background can be found in [1].

So far posix acls were passed as a void blob to the security and
integrity modules. Some of them like evm then proceed to interpret the
void pointer and convert it into the kernel internal struct posix acl
representation to perform their integrity checking magic. This is
obviously pretty problematic as that requires knowledge that only the
vfs is guaranteed to have and has lead to various bugs. Add a proper
security hook for setting posix acls and pass down the posix acls in
their appropriate vfs format instead of hacking it through a void
pointer stored in the uapi format.

I spent considerate time in the security module and integrity
infrastructure and audited all codepaths. EVM is the only part that
really has restrictions based on the actual posix acl values passed
through it. Before this dedicated hook EVM used to translate from the
uapi posix acl format sent to it in the form of a void pointer into the
vfs format. This is not a good thing. Instead of hacking around in the
uapi struct give EVM the posix acls in the appropriate vfs format and
perform sane permissions checks that mirror what it used to to in the
generic xattr hook.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 include/linux/evm.h               | 10 +++++
 security/integrity/evm/evm_main.c | 66 ++++++++++++++++++++++++++++++-
 security/security.c               |  9 ++++-
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index aa63e0b3c0a2..aebcfd47d496 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -35,6 +35,9 @@ extern int evm_inode_removexattr(struct user_namespace *mnt_userns,
 				 struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
 				       const char *xattr_name);
+extern int evm_inode_set_acl(struct user_namespace *mnt_userns,
+			     struct dentry *dentry, const char *acl_name,
+			     struct posix_acl *kacl);
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -108,6 +111,13 @@ static inline void evm_inode_post_removexattr(struct dentry *dentry,
 	return;
 }
 
+static inline int evm_inode_set_acl(struct user_namespace *mnt_userns,
+				    struct dentry *dentry, const char *acl_name,
+				    struct posix_acl *kacl)
+{
+	return 0;
+}
+
 static inline int evm_inode_init_security(struct inode *inode,
 					  const struct xattr *xattr_array,
 					  struct xattr *evm)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 23d484e05e6f..15aa5995fff4 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -8,7 +8,7 @@
  *
  * File: evm_main.c
  *	implements evm_inode_setxattr, evm_inode_post_setxattr,
- *	evm_inode_removexattr, and evm_verifyxattr
+ *	evm_inode_removexattr, evm_verifyxattr, and evm_inode_set_acl.
  */
 
 #define pr_fmt(fmt) "EVM: "fmt
@@ -670,6 +670,70 @@ int evm_inode_removexattr(struct user_namespace *mnt_userns,
 	return evm_protect_xattr(mnt_userns, dentry, xattr_name, NULL, 0);
 }
 
+static int evm_inode_set_acl_change(struct user_namespace *mnt_userns,
+				    struct dentry *dentry, const char *name,
+				    struct posix_acl *kacl)
+{
+#ifdef CONFIG_FS_POSIX_ACL
+	int rc;
+	umode_t mode;
+	struct inode *inode = d_backing_inode(dentry);
+
+	rc = posix_acl_update_mode(mnt_userns, inode, &mode, &kacl);
+	if (rc || (inode->i_mode != mode))
+		return 1;
+#endif
+	return 0;
+}
+
+/**
+ * evm_inode_set_acl - protect the EVM extended attribute for posix acls
+ * @mnt_userns: user namespace of the idmapped mount
+ * @dentry: pointer to the affected dentry
+ * @acl_name: name of the posix acl
+ * @kacl: pointer to the posix acls
+ */
+int evm_inode_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
+		      const char *acl_name, struct posix_acl *kacl)
+{
+	enum integrity_status evm_status;
+
+	/* Policy permits modification of the protected xattrs even though
+	 * there's no HMAC key loaded
+	 */
+	if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+		return 0;
+
+	evm_status = evm_verify_current_integrity(dentry);
+	if ((evm_status == INTEGRITY_PASS) ||
+	    (evm_status == INTEGRITY_NOXATTRS))
+		return 0;
+
+	/* Exception if the HMAC is not going to be calculated. */
+	if (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL ||
+	    evm_status == INTEGRITY_UNKNOWN))
+		return 0;
+
+	/*
+	 * Writing other xattrs is safe for portable signatures, as portable
+	 * signatures are immutable and can never be updated.
+	 */
+	if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
+		return 0;
+
+	if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
+	    !evm_inode_set_acl_change(mnt_userns, dentry, acl_name, kacl))
+		return 0;
+
+	if (evm_status != INTEGRITY_PASS &&
+	    evm_status != INTEGRITY_PASS_IMMUTABLE)
+		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
+				    dentry->d_name.name, "appraise_metadata",
+				    integrity_status_msg[evm_status],
+				    -EPERM, 0);
+	return evm_status == INTEGRITY_PASS ? 0 : -EPERM;
+}
+
 static void evm_reset_status(struct inode *inode)
 {
 	struct integrity_iint_cache *iint;
diff --git a/security/security.c b/security/security.c
index 56d48e7254d6..a12a26a4494e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1374,9 +1374,16 @@ int security_inode_set_acl(struct user_namespace *mnt_userns,
 			   struct dentry *dentry, const char *acl_name,
 			   struct posix_acl *kacl)
 {
+	int ret;
+
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	return call_int_hook(inode_set_acl, 0, mnt_userns, dentry, acl_name, kacl);
+
+	ret = call_int_hook(inode_set_acl, 0, mnt_userns, dentry, acl_name, kacl);
+	if (ret)
+		return ret;
+
+	return evm_inode_set_acl(mnt_userns, dentry, acl_name, kacl);
 }
 
 void security_inode_post_setxattr(struct dentry *dentry, const char *name,
-- 
2.34.1

[PATCH 14/29] evm: add post set acl hook

[Christian Brauner]

The security_inode_post_setxattr() hook is used by security modules to
update their own security.* xattrs. Consequently none of the security
modules operate on posix acls. So we don't need an additional security
hook when post setting posix acls.

However, the integrity subsystem wants to be informed about posix acl
changes and specifically evm to update their hashes when the xattrs
change. The callchain for evm_inode_post_setxattr() is:

-> evm_inode_post_setxattr()
   -> evm_update_evmxattr()
      -> evm_calc_hmac()
         -> evm_calc_hmac_or_hash()

and evm_cacl_hmac_or_hash() walks the global list of protected xattr
names evm_config_xattrnames. This global list can be modified via
/sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is
restricted to security.* xattrs and the default xattrs in
evm_config_xattrnames only contains security.* xattrs as well.

So the actual value for posix acls is currently completely irrelevant
for evm during evm_inode_post_setxattr() and frankly it should stay that
way in the future to not cause the vfs any more headaches. But if the
actual posix acl values matter then evm shouldn't operate on the binary
void blob and try to hack around in the uapi struct anyway. Instead it
should then in the future add a dedicated hook which takes a struct
posix_acl argument passing the posix acls in the proper vfs format.

For now it is sufficient to make evm_inode_post_set_acl() a wrapper
around evm_inode_post_setxattr() not passing any actual values down.
This will still cause the hashes to be updated as before.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/posix_acl.c      |  5 ++++-
 include/linux/evm.h | 12 ++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 5ff0d8b05194..752e9bda8840 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -25,6 +25,7 @@
 #include <linux/namei.h>
 #include <linux/mnt_idmapping.h>
 #include <linux/security.h>
+#include <linux/evm.h>
 #include <linux/fsnotify.h>
 
 static struct posix_acl **acl_by_type(struct inode *inode, int type)
@@ -1350,8 +1351,10 @@ int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
 		error = -EIO;
 	else
 		error = -EOPNOTSUPP;
-	if (!error)
+	if (!error) {
 		fsnotify_xattr(dentry);
+		evm_inode_post_set_acl(dentry, acl_name, kacl);
+	}
 
 out_inode_unlock:
 	inode_unlock(inode);
diff --git a/include/linux/evm.h b/include/linux/evm.h
index aebcfd47d496..d735a1757bdf 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -38,6 +38,12 @@ extern void evm_inode_post_removexattr(struct dentry *dentry,
 extern int evm_inode_set_acl(struct user_namespace *mnt_userns,
 			     struct dentry *dentry, const char *acl_name,
 			     struct posix_acl *kacl);
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name,
+					  struct posix_acl *kacl)
+{
+	return evm_inode_post_setxattr(dentry, acl_name, NULL, 0);
+}
 extern int evm_inode_init_security(struct inode *inode,
 				   const struct xattr *xattr_array,
 				   struct xattr *evm);
@@ -118,6 +124,12 @@ static inline int evm_inode_set_acl(struct user_namespace *mnt_userns,
 	return 0;
 }
 
+static inline void evm_inode_post_set_acl(struct dentry *dentry,
+					  const char *acl_name)
+{
+	return;
+}
+
 static inline int evm_inode_init_security(struct inode *inode,
 					  const struct xattr *xattr_array,
 					  struct xattr *evm)
-- 
2.34.1

[PATCH 17/29] evm: simplify evm_xattr_acl_change()

[Christian Brauner]

The posix acl api provides a dedicated security and integrity hook for
setting posix acls. This means that

evm_protect_xattr()
-> evm_xattr_change()
   -> evm_xattr_acl_change()

is now only hit during vfs_remove_acl() at which point we are guaranteed
that xattr_value and xattr_value_len are NULL and 0. In this case evm
always used to return 1. Simplify this function to do just that.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 security/integrity/evm/evm_main.c | 62 +++++++------------------------
 1 file changed, 14 insertions(+), 48 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 15aa5995fff4..1fbe1b8d0364 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -436,62 +436,29 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
 
 /*
  * evm_xattr_acl_change - check if passed ACL changes the inode mode
- * @mnt_userns: user namespace of the idmapped mount
- * @dentry: pointer to the affected dentry
  * @xattr_name: requested xattr
  * @xattr_value: requested xattr value
  * @xattr_value_len: requested xattr value length
  *
- * Check if passed ACL changes the inode mode, which is protected by EVM.
+ * This is only hit during xattr removal at which point we always return 1.
+ * Splat a warning in case someone managed to pass data to this function. That
+ * should never happen.
  *
  * Returns 1 if passed ACL causes inode mode change, 0 otherwise.
  */
-static int evm_xattr_acl_change(struct user_namespace *mnt_userns,
-				struct dentry *dentry, const char *xattr_name,
-				const void *xattr_value, size_t xattr_value_len)
+static int evm_xattr_acl_change(const void *xattr_value, size_t xattr_value_len)
 {
-#ifdef CONFIG_FS_POSIX_ACL
-	umode_t mode;
-	struct posix_acl *acl = NULL, *acl_res;
-	struct inode *inode = d_backing_inode(dentry);
-	int rc;
-
-	/*
-	 * 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 = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),
-				  xattr_value, xattr_value_len);
-	if (IS_ERR_OR_NULL(acl))
-		return 1;
-
-	acl_res = acl;
-	/*
-	 * Passing mnt_userns is necessary to correctly determine the GID in
-	 * an idmapped mount, as the GID is used to clear the setgid bit in
-	 * the inode mode.
-	 */
-	rc = posix_acl_update_mode(mnt_userns, inode, &mode, &acl_res);
-
-	posix_acl_release(acl);
-
-	if (rc)
-		return 1;
+	int rc = 0;
 
-	if (inode->i_mode != mode)
-		return 1;
+#ifdef CONFIG_FS_POSIX_ACL
+	WARN_ONCE(xattr_value != NULL,
+		  "Passing xattr value for POSIX ACLs not supported\n");
+	WARN_ONCE(xattr_value_len != 0,
+		  "Passing non-zero length for POSIX ACLs not supported\n");
+	rc = 1;
 #endif
-	return 0;
+
+	return rc;
 }
 
 /*
@@ -514,8 +481,7 @@ static int evm_xattr_change(struct user_namespace *mnt_userns,
 	int rc = 0;
 
 	if (posix_xattr_acl(xattr_name))
-		return evm_xattr_acl_change(mnt_userns, dentry, xattr_name,
-					    xattr_value, xattr_value_len);
+		return evm_xattr_acl_change(xattr_value, xattr_value_len);
 
 	rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data,
 				0, GFP_NOFS);
-- 
2.34.1

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Casey Schaufler]

On 9/22/2022 8:16 AM, Christian Brauner wrote:
> From: "Christian Brauner (Microsoft)" <brauner@kernel.org>

Could we please see the entire patch set on the LSM list?
( linux-security-module@vger.kernel.org )
It's really tough to judge the importance of adding a new
LSM hook without seeing both how it is called and how the
security modules are expected to fulfill it. In particular,
it is important to see how a posix acl is different from
any other xattr. 

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Paul Moore]

On Thu, Sep 22, 2022 at 12:27 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 9/22/2022 8:16 AM, Christian Brauner wrote:
> > From: "Christian Brauner (Microsoft)" <brauner@kernel.org>
>
> Could we please see the entire patch set on the LSM list?
> ( linux-security-module@vger.kernel.org )
> It's really tough to judge the importance of adding a new
> LSM hook without seeing both how it is called and how the
> security modules are expected to fulfill it. In particular,
> it is important to see how a posix acl is different from
> any other xattr.

Yes, exactly.  I understand the desire to avoid dumping a large~ish
patchset on a lot of lists, but it's really hard to adequately review
something when you only see a small fraction of the overall change.

-- 
paul-moore.com

Re: [PATCH 10/29] selinux: implement set acl hook

[Paul Moore]

On Thu, Sep 22, 2022 at 11:18 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The current way of setting and getting posix acls through the generic
> xattr interface is error prone and type unsafe. The vfs needs to
> interpret and fixup posix acls before storing or reporting it to
> userspace. Various hacks exist to make this work. The code is hard to
> understand and difficult to maintain in it's current form. Instead of
> making this work by hacking posix acls through xattr handlers we are
> building a dedicated posix acl api around the get and set inode
> operations. This removes a lot of hackiness and makes the codepaths
> easier to maintain. A lot of background can be found in [1].
>
> So far posix acls were passed as a void blob to the security and
> integrity modules. Some of them like evm then proceed to interpret the
> void pointer and convert it into the kernel internal struct posix acl
> representation to perform their integrity checking magic. This is
> obviously pretty problematic as that requires knowledge that only the
> vfs is guaranteed to have and has lead to various bugs. Add a proper
> security hook for setting posix acls and pass down the posix acls in
> their appropriate vfs format instead of hacking it through a void
> pointer stored in the uapi format.
>
> I spent considerate time in the security module infrastructure and
> audited all codepaths. SELinux has no restrictions based on the posix
> acl values passed through it. The capability hook doesn't need to be
> called either because it only has restrictions on security.* xattrs. So
> this all becomes a very simple hook for SELinux.
>
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>  security/selinux/hooks.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 79573504783b..bbc0ce3bde35 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3239,6 +3239,13 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns,
>                             &ad);
>  }
>
> +static int selinux_inode_set_acl(struct user_namespace *mnt_userns,
> +                                struct dentry *dentry, const char *acl_name,
> +                                struct posix_acl *kacl)
> +{
> +       return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> +}
> +
>  static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
>                                         const void *value, size_t size,
>                                         int flags)
> @@ -7063,6 +7070,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
>         LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr),
>         LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
> +       LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl),

See my other reply about needing to see the full patchset in order to
properly review the changes, but one thing immediately jumped out at
me when looking at this: why is the LSM hook
"security_inode_set_acl()" when we are passing a dentry instead of an
inode?  We don't have a lot of them, but there are
`security_dentry_*()` LSM hooks in the existing kernel code.

-- 
paul-moore.com

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Linus Torvalds]

On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Could we please see the entire patch set on the LSM list?

While I don't think that's necessarily wrong, I would like to point
out that the gitweb interface actually does make it fairly easy to
just see the whole patch-set.

IOW, that

  https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework

that Christian pointed to is not a horrible way to see it all. Go to
the top-most commit, and it's easy to follow the parent links.

It's a bit more work to see them in another order, but I find the
easiest way is actually to just follow the parent links to get the
overview of what is going on (reading just the commit messages), and
then after that you "reverse course" and use the browser back button
to just go the other way while looking at the details of the patches.

And I suspect a lot of people are happier *without* large patch-sets
being posted to the mailing lists when most patches aren't necessarily
at all relevant to that mailing list except as context.

                 Linus

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Casey Schaufler]

On 9/22/2022 10:57 AM, Linus Torvalds wrote:
> On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Could we please see the entire patch set on the LSM list?
> While I don't think that's necessarily wrong, I would like to point
> out that the gitweb interface actually does make it fairly easy to
> just see the whole patch-set.
>
> IOW, that
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
>
> that Christian pointed to is not a horrible way to see it all. Go to
> the top-most commit, and it's easy to follow the parent links.

I understand that the web interface is fine for browsing the changes.
It isn't helpful for making comments on the changes. The discussion
on specific patches (e.g. selinux) may have impact on other parts of
the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
can be a real problem if the higher level mailing list (the LSM list
in this case) isn't included. 

>
> It's a bit more work to see them in another order, but I find the
> easiest way is actually to just follow the parent links to get the
> overview of what is going on (reading just the commit messages), and
> then after that you "reverse course" and use the browser back button
> to just go the other way while looking at the details of the patches.
>
> And I suspect a lot of people are happier *without* large patch-sets
> being posted to the mailing lists when most patches aren't necessarily
> at all relevant to that mailing list except as context.

I can certainly understand that. I don't think that the filesystem
specific bits are going to be especially interesting to me, but if
they are I do want to be able to comment on them.

>
>                  Linus

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Paul Moore]

On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 9/22/2022 10:57 AM, Linus Torvalds wrote:
> > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Could we please see the entire patch set on the LSM list?
> > While I don't think that's necessarily wrong, I would like to point
> > out that the gitweb interface actually does make it fairly easy to
> > just see the whole patch-set.
> >
> > IOW, that
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> >
> > that Christian pointed to is not a horrible way to see it all. Go to
> > the top-most commit, and it's easy to follow the parent links.
>
> I understand that the web interface is fine for browsing the changes.
> It isn't helpful for making comments on the changes. The discussion
> on specific patches (e.g. selinux) may have impact on other parts of
> the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
> can be a real problem if the higher level mailing list (the LSM list
> in this case) isn't included.

This is probably one of those few cases where Casey and I are in
perfect agreement.  I'd much rather see the patches hit my inbox than
have to go hunting for them and then awkwardly replying to them (and
yes, I know there are ways to do that, I just personally find it
annoying).  I figure we are all deluged with email on a daily basis
and have developed mechanisms to deal with that in a sane way, what is
29 more patches on the pile?

-- 
paul-moore.com

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Serge E. Hallyn]

On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote:
> On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 9/22/2022 10:57 AM, Linus Torvalds wrote:
> > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> Could we please see the entire patch set on the LSM list?
> > > While I don't think that's necessarily wrong, I would like to point
> > > out that the gitweb interface actually does make it fairly easy to
> > > just see the whole patch-set.
> > >
> > > IOW, that
> > >
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> > >
> > > that Christian pointed to is not a horrible way to see it all. Go to
> > > the top-most commit, and it's easy to follow the parent links.
> >
> > I understand that the web interface is fine for browsing the changes.
> > It isn't helpful for making comments on the changes. The discussion
> > on specific patches (e.g. selinux) may have impact on other parts of
> > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
> > can be a real problem if the higher level mailing list (the LSM list
> > in this case) isn't included.
> 
> This is probably one of those few cases where Casey and I are in
> perfect agreement.  I'd much rather see the patches hit my inbox than
> have to go hunting for them and then awkwardly replying to them (and
> yes, I know there are ways to do that, I just personally find it
> annoying).  I figure we are all deluged with email on a daily basis
> and have developed mechanisms to deal with that in a sane way, what is
> 29 more patches on the pile?

Even better than the web interface, is find the message-id in any of the
emails you did get, and run

b4 mbox 20220922151728.1557914-1-brauner@kernel.org

In general I'd agree with sending the whole set to the lsm list, but
then one needs to start knowing which lists do and don't want the whole
set...  b4 mbox and lei are now how I read all kernel related lists.

-serge

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Paul Moore]

On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote:
> > On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > On 9/22/2022 10:57 AM, Linus Torvalds wrote:
> > > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >> Could we please see the entire patch set on the LSM list?
> > > > While I don't think that's necessarily wrong, I would like to point
> > > > out that the gitweb interface actually does make it fairly easy to
> > > > just see the whole patch-set.
> > > >
> > > > IOW, that
> > > >
> > > >   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> > > >
> > > > that Christian pointed to is not a horrible way to see it all. Go to
> > > > the top-most commit, and it's easy to follow the parent links.
> > >
> > > I understand that the web interface is fine for browsing the changes.
> > > It isn't helpful for making comments on the changes. The discussion
> > > on specific patches (e.g. selinux) may have impact on other parts of
> > > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
> > > can be a real problem if the higher level mailing list (the LSM list
> > > in this case) isn't included.
> >
> > This is probably one of those few cases where Casey and I are in
> > perfect agreement.  I'd much rather see the patches hit my inbox than
> > have to go hunting for them and then awkwardly replying to them (and
> > yes, I know there are ways to do that, I just personally find it
> > annoying).  I figure we are all deluged with email on a daily basis
> > and have developed mechanisms to deal with that in a sane way, what is
> > 29 more patches on the pile?
>
> Even better than the web interface, is find the message-id in any of the
> emails you did get, and run
>
> b4 mbox 20220922151728.1557914-1-brauner@kernel.org
>
> In general I'd agree with sending the whole set to the lsm list, but
> then one needs to start knowing which lists do and don't want the whole
> set...  b4 mbox and lei are now how I read all kernel related lists.

In my opinion, sending the entire patchset to the relevant lists
should be the default for all the reasons mentioned above.  All the
other methods are fine, and I don't want to stop anyone from using
their favorite tool, but *requiring* the use of a separate tool to
properly review and comment on patches gets us away from the
email-is-universal argument.  Yes, all the other tools mentioned are
still based in a world of email, but if you are not emailing the
relevant stakeholders directly (or indirectly via a list), you are
placing another hurdle in front of the reviewers by requiring them to
leave their email client based workflow and jump over to lore, b4,
etc. to review the patchset.

The lore.kernel.org instance is wonderful, full stop, and the b4 tool
is equally wonderful, full stop, but they are tools intended to assist
and optimize; they should not replace the practice of sending patches,
with the full context, to the relevant parties.

-- 
paul-moore.com

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Christoph Hellwig]

On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote:
> In my opinion, sending the entire patchset to the relevant lists
> should be the default for all the reasons mentioned above.

Agreed.  I'm perfectly fine when people minimize the CCs to actual
people (but then for the entire patch set), but having only the
partial series in an mbox just makes it useful.  Either the list
or person on everything or nothing.  I can't actually do anything
with a partial CC except for either ignoring it or shoting at
you that I need the entire series to do something useful with it.

(although in this case I did get all of it anyway).

Re: [PATCH 10/29] selinux: implement set acl hook

[Christoph Hellwig]

On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> properly review the changes, but one thing immediately jumped out at
> me when looking at this: why is the LSM hook
> "security_inode_set_acl()" when we are passing a dentry instead of an
> inode?  We don't have a lot of them, but there are
> `security_dentry_*()` LSM hooks in the existing kernel code.

I'm no LSM expert, but isn't the inode vs dentry for if it is
related to an inode operation or dentry operation, not about that
the first argument is?

Re: [PATCH 10/29] selinux: implement set acl hook

[Christian Brauner]

On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > properly review the changes, but one thing immediately jumped out at
> > me when looking at this: why is the LSM hook
> > "security_inode_set_acl()" when we are passing a dentry instead of an
> > inode?  We don't have a lot of them, but there are
> > `security_dentry_*()` LSM hooks in the existing kernel code.
> 
> I'm no LSM expert, but isn't the inode vs dentry for if it is
> related to an inode operation or dentry operation, not about that
> the first argument is?

Indeed. For example,

void security_inode_post_setxattr(struct dentry *dentry, const char *name,
				  const void *value, size_t size, int flags)
{
	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return;
	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
	evm_inode_post_setxattr(dentry, name, value, size);
}

int security_inode_getxattr(struct dentry *dentry, const char *name)
{
	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;
	return call_int_hook(inode_getxattr, 0, dentry, name);
}

int security_inode_listxattr(struct dentry *dentry)
{
	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;
	return call_int_hook(inode_listxattr, 0, dentry);
}

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Christian Brauner]

On Thu, Sep 22, 2022 at 10:57:38AM -0700, Linus Torvalds wrote:
> On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > Could we please see the entire patch set on the LSM list?
> 
> While I don't think that's necessarily wrong, I would like to point
> out that the gitweb interface actually does make it fairly easy to
> just see the whole patch-set.
> 
> IOW, that
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> 
> that Christian pointed to is not a horrible way to see it all. Go to
> the top-most commit, and it's easy to follow the parent links.
> 
> It's a bit more work to see them in another order, but I find the
> easiest way is actually to just follow the parent links to get the
> overview of what is going on (reading just the commit messages), and
> then after that you "reverse course" and use the browser back button
> to just go the other way while looking at the details of the patches.
> 
> And I suspect a lot of people are happier *without* large patch-sets
> being posted to the mailing lists when most patches aren't necessarily
> at all relevant to that mailing list except as context.

The problem is also that it's impossible to please both parties here.

A good portion of people doesn't like being flooded with patches they
don't really care about and the other portion gets worked up when they
only see a single patch.

So honestly I just always make a judgement call based on the series. But
b4 makes it so so easy to just retrieve the whole series. So even if I
only receive a single patch and am curious then I just use b4.

I've even got it integrated into mutt directly:

# Pipe message to b4 to download patches and threads
macro index,pager A "<pipe-message>b4 am --apply-cover-trailers --sloppy-trailers --add-my-sob --guess-base --check-newer-revisions --no-cache --quilt-ready <enter>"
macro index,pager M "<pipe-message>b4 mbox <enter>"

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Christian Brauner]

On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote:
> On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> > On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote:
> > > On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > On 9/22/2022 10:57 AM, Linus Torvalds wrote:
> > > > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >> Could we please see the entire patch set on the LSM list?
> > > > > While I don't think that's necessarily wrong, I would like to point
> > > > > out that the gitweb interface actually does make it fairly easy to
> > > > > just see the whole patch-set.
> > > > >
> > > > > IOW, that
> > > > >
> > > > >   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> > > > >
> > > > > that Christian pointed to is not a horrible way to see it all. Go to
> > > > > the top-most commit, and it's easy to follow the parent links.
> > > >
> > > > I understand that the web interface is fine for browsing the changes.
> > > > It isn't helpful for making comments on the changes. The discussion
> > > > on specific patches (e.g. selinux) may have impact on other parts of
> > > > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
> > > > can be a real problem if the higher level mailing list (the LSM list
> > > > in this case) isn't included.
> > >
> > > This is probably one of those few cases where Casey and I are in
> > > perfect agreement.  I'd much rather see the patches hit my inbox than
> > > have to go hunting for them and then awkwardly replying to them (and
> > > yes, I know there are ways to do that, I just personally find it
> > > annoying).  I figure we are all deluged with email on a daily basis
> > > and have developed mechanisms to deal with that in a sane way, what is
> > > 29 more patches on the pile?
> >
> > Even better than the web interface, is find the message-id in any of the
> > emails you did get, and run
> >
> > b4 mbox 20220922151728.1557914-1-brauner@kernel.org
> >
> > In general I'd agree with sending the whole set to the lsm list, but
> > then one needs to start knowing which lists do and don't want the whole
> > set...  b4 mbox and lei are now how I read all kernel related lists.
> 
> In my opinion, sending the entire patchset to the relevant lists
> should be the default for all the reasons mentioned above.  All the
> other methods are fine, and I don't want to stop anyone from using
> their favorite tool, but *requiring* the use of a separate tool to
> properly review and comment on patches gets us away from the
> email-is-universal argument.  Yes, all the other tools mentioned are
> still based in a world of email, but if you are not emailing the
> relevant stakeholders directly (or indirectly via a list), you are
> placing another hurdle in front of the reviewers by requiring them to
> leave their email client based workflow and jump over to lore, b4,
> etc. to review the patchset.
> 
> The lore.kernel.org instance is wonderful, full stop, and the b4 tool
> is equally wonderful, full stop, but they are tools intended to assist
> and optimize; they should not replace the practice of sending patches,
> with the full context, to the relevant parties.

I'm happy to send all of v2 to the security mailing list.

But for v1 could you compromise and just use b4?

b4 mbox 20220922151728.1557914-1-brauner@kernel.org

This would mean you could provide reviews for v1 and we don't need to
fragment the v1 discussion because of a resend to include a mailing list.

Re: [PATCH 10/29] selinux: implement set acl hook

[Paul Moore]

On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote:
> On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > > properly review the changes, but one thing immediately jumped out at
> > > me when looking at this: why is the LSM hook
> > > "security_inode_set_acl()" when we are passing a dentry instead of an
> > > inode?  We don't have a lot of them, but there are
> > > `security_dentry_*()` LSM hooks in the existing kernel code.
> >
> > I'm no LSM expert, but isn't the inode vs dentry for if it is
> > related to an inode operation or dentry operation, not about that
> > the first argument is?
>
> Indeed. For example ...

If the goal is for this LSM hook to operate on an inode and not a
dentry, let's pass it an inode instead.  This should help prevent
misuse and I suspect the individual implementations will be quicker
for it anyway (it should be the case for SELinux, and while I'm not a
Smack expert it looks to be true for Smack as well).  There is the
potential for some additional work in the case where the inode needs
to be revalidated (I believe this is only a SELinux issue at the
moment) as well as if an audit event needs to be generated, but these
should both happen infrequently enough that I don't believe they are a
real concern.

-- 
paul-moore.com

Re: [PATCH 10/29] selinux: implement set acl hook

[Christian Brauner]

On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote:
> On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote:
> > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > > > properly review the changes, but one thing immediately jumped out at
> > > > me when looking at this: why is the LSM hook
> > > > "security_inode_set_acl()" when we are passing a dentry instead of an
> > > > inode?  We don't have a lot of them, but there are
> > > > `security_dentry_*()` LSM hooks in the existing kernel code.
> > >
> > > I'm no LSM expert, but isn't the inode vs dentry for if it is
> > > related to an inode operation or dentry operation, not about that
> > > the first argument is?
> >
> > Indeed. For example ...
> 
> If the goal is for this LSM hook to operate on an inode and not a
> dentry, let's pass it an inode instead.  This should help prevent

I would be ok with that but EVM requires a dentry being passed and as
evm is called from security_inode_set_acl() exactly like it is from
security_inode_setxattr() and similar the hook has to take a dentry.

And I want to minimize - ideally get rid of at some point - separate
calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook
should please stay in there.

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Paul Moore]

On Fri, Sep 23, 2022 at 4:46 AM Christian Brauner <brauner@kernel.org> wrote:
> On Thu, Sep 22, 2022 at 10:57:38AM -0700, Linus Torvalds wrote:
> > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >
> > > Could we please see the entire patch set on the LSM list?
> >
> > While I don't think that's necessarily wrong, I would like to point
> > out that the gitweb interface actually does make it fairly easy to
> > just see the whole patch-set.
> >
> > IOW, that
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
> >
> > that Christian pointed to is not a horrible way to see it all. Go to
> > the top-most commit, and it's easy to follow the parent links.
> >
> > It's a bit more work to see them in another order, but I find the
> > easiest way is actually to just follow the parent links to get the
> > overview of what is going on (reading just the commit messages), and
> > then after that you "reverse course" and use the browser back button
> > to just go the other way while looking at the details of the patches.
> >
> > And I suspect a lot of people are happier *without* large patch-sets
> > being posted to the mailing lists when most patches aren't necessarily
> > at all relevant to that mailing list except as context.
>
> The problem is also that it's impossible to please both parties here.

Oh the trials and tribulations of Linux Kernel development! ;)

I'm joking, but I do understand the difficulty of pleasing a large
group of people with very different desires.

> A good portion of people doesn't like being flooded with patches they
> don't really care about and the other portion gets worked up when they
> only see a single patch.

You are obviously never going to be able to make everyone happy, and
everyone with a solution to share obviously has some bias (I'm
definitely including myself in this statement), but I tend to fall
back on the idea that upstream kernel development has always required
those involved to deal with a large amount of email, so sending a full
patchset is not new.

> So honestly I just always make a judgement call based on the series. But
> b4 makes it so so easy to just retrieve the whole series. So even if I
> only receive a single patch and am curious then I just use b4.

As I mentioned previously in this thread, the issue is more on the
reply side.  Reading from lore or b4 isn't terrible for me, but
replying is a pain for me and my mail setup.

> I've even got it integrated into mutt directly:

I'm glad it works for you.  Although I would like to take this
opportunity to remind anyone still following this tangent that not
everyone uses mutt, some of us* really dislike it, but due to the
magic of email we are still able to participate with other mail
clients, services, and tools.

* I'm using "us" somewhat liberally here, I have no data to back up my
claims.  However, I'm fully prepared to accept the idea that I'm the
only person out of the thousands of kernel devs who dislikes mutt.
Bring it on haters, just know that you're all wrong ;)

-- 
paul-moore.com

Re: [RFC PATCH 00/29] acl: add vfs posix acl api

[Casey Schaufler]

On 9/23/2022 1:52 AM, Christian Brauner wrote:
> On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote:
>> On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote:
>>> On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote:
>>>> On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 9/22/2022 10:57 AM, Linus Torvalds wrote:
>>>>>> On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> Could we please see the entire patch set on the LSM list?
>>>>>> While I don't think that's necessarily wrong, I would like to point
>>>>>> out that the gitweb interface actually does make it fairly easy to
>>>>>> just see the whole patch-set.
>>>>>>
>>>>>> IOW, that
>>>>>>
>>>>>>   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework
>>>>>>
>>>>>> that Christian pointed to is not a horrible way to see it all. Go to
>>>>>> the top-most commit, and it's easy to follow the parent links.
>>>>> I understand that the web interface is fine for browsing the changes.
>>>>> It isn't helpful for making comments on the changes. The discussion
>>>>> on specific patches (e.g. selinux) may have impact on other parts of
>>>>> the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It
>>>>> can be a real problem if the higher level mailing list (the LSM list
>>>>> in this case) isn't included.
>>>> This is probably one of those few cases where Casey and I are in
>>>> perfect agreement.  I'd much rather see the patches hit my inbox than
>>>> have to go hunting for them and then awkwardly replying to them (and
>>>> yes, I know there are ways to do that, I just personally find it
>>>> annoying).  I figure we are all deluged with email on a daily basis
>>>> and have developed mechanisms to deal with that in a sane way, what is
>>>> 29 more patches on the pile?
>>> Even better than the web interface, is find the message-id in any of the
>>> emails you did get, and run
>>>
>>> b4 mbox 20220922151728.1557914-1-brauner@kernel.org
>>>
>>> In general I'd agree with sending the whole set to the lsm list, but
>>> then one needs to start knowing which lists do and don't want the whole
>>> set...  b4 mbox and lei are now how I read all kernel related lists.

Because of commonalities and interactions among the various security modules,
along with the ongoing efforts to enhance the infrastructure and the close
ties with the vfs and audit system, it's rare that the LSM crowd isn't going
to want to see the whole of a change.

>> In my opinion, sending the entire patchset to the relevant lists
>> should be the default for all the reasons mentioned above.  All the
>> other methods are fine, and I don't want to stop anyone from using
>> their favorite tool, but *requiring* the use of a separate tool to
>> properly review and comment on patches gets us away from the
>> email-is-universal argument.  Yes, all the other tools mentioned are
>> still based in a world of email, but if you are not emailing the
>> relevant stakeholders directly (or indirectly via a list), you are
>> placing another hurdle in front of the reviewers by requiring them to
>> leave their email client based workflow and jump over to lore, b4,
>> etc. to review the patchset.
>>
>> The lore.kernel.org instance is wonderful, full stop, and the b4 tool
>> is equally wonderful, full stop, but they are tools intended to assist
>> and optimize; they should not replace the practice of sending patches,
>> with the full context, to the relevant parties.
> I'm happy to send all of v2 to the security mailing list.

Thank you.

> But for v1 could you compromise and just use b4?

I cringe whenever someone says "just".

I'm sure b4 is a fine tool. I'm told mutt is useful. Gitweb is kewl.
But adopting a new and exciting development methodology every few
years since about 1978 has given me a real appreciation for the
raw email approach. I'll wait for v2.

>
> b4 mbox 20220922151728.1557914-1-brauner@kernel.org
>
> This would mean you could provide reviews for v1 and we don't need to
> fragment the v1 discussion because of a resend to include a mailing list.

Right, but I would need to learn yet another development tool set.
I fully expect you'd have v2 ready before I could be sufficiently
proficient with b4+mutt to contribute.

Re: [PATCH 10/29] selinux: implement set acl hook

[Paul Moore]

On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote:
> On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote:
> > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote:
> > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > > > > properly review the changes, but one thing immediately jumped out at
> > > > > me when looking at this: why is the LSM hook
> > > > > "security_inode_set_acl()" when we are passing a dentry instead of an
> > > > > inode?  We don't have a lot of them, but there are
> > > > > `security_dentry_*()` LSM hooks in the existing kernel code.
> > > >
> > > > I'm no LSM expert, but isn't the inode vs dentry for if it is
> > > > related to an inode operation or dentry operation, not about that
> > > > the first argument is?
> > >
> > > Indeed. For example ...
> >
> > If the goal is for this LSM hook to operate on an inode and not a
> > dentry, let's pass it an inode instead.  This should help prevent
>
> I would be ok with that but EVM requires a dentry being passed and as
> evm is called from security_inode_set_acl() exactly like it is from
> security_inode_setxattr() and similar the hook has to take a dentry.

If a dentry is truly needed by EVM (a quick look indicates that it may
just be for the VFS getxattr API, but I haven't traced the full code
path), then I'm having a hard time reconciling that this isn't a
dentry operation.  Yes, I get that the ACLs belong to the inode and
not the dentry, but then why do we need the dentry?  It seems like the
interfaces are broken slightly, or at least a little odd ... <shrug>

> And I want to minimize - ideally get rid of at some point - separate
> calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook
> should please stay in there.

For the record, I want to get rid of the IMA and EVM specific hooks in
the kernel.  They were a necessity back when there could only be one
LSM active at a given time, but with that no longer the case I see
little reason why IMA/EVM/etc. remain separate; it makes the code
worse and complicates a lot of things both at the LSM layer as well as
the rest of the kernel.  I've mentioned this to a few people,
including Mimi, and it came up during at talk at LPC this year.

-- 
paul-moore.com

Re: [PATCH 10/29] selinux: implement set acl hook

[Christian Brauner]

On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote:
> On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote:
> > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote:
> > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > > > > > properly review the changes, but one thing immediately jumped out at
> > > > > > me when looking at this: why is the LSM hook
> > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an
> > > > > > inode?  We don't have a lot of them, but there are
> > > > > > `security_dentry_*()` LSM hooks in the existing kernel code.
> > > > >
> > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is
> > > > > related to an inode operation or dentry operation, not about that
> > > > > the first argument is?
> > > >
> > > > Indeed. For example ...
> > >
> > > If the goal is for this LSM hook to operate on an inode and not a
> > > dentry, let's pass it an inode instead.  This should help prevent
> >
> > I would be ok with that but EVM requires a dentry being passed and as
> > evm is called from security_inode_set_acl() exactly like it is from
> > security_inode_setxattr() and similar the hook has to take a dentry.
> 
> If a dentry is truly needed by EVM (a quick look indicates that it may
> just be for the VFS getxattr API, but I haven't traced the full code
> path), then I'm having a hard time reconciling that this isn't a
> dentry operation.  Yes, I get that the ACLs belong to the inode and
> not the dentry, but then why do we need the dentry?  It seems like the
> interfaces are broken slightly, or at least a little odd ... <shrug>

There's multiple reasons for the generic xattr api to take a dentry. For
example, there are quite a few filesystems that require dentry access
during (specific or all) xattr operations. So ideally, we'd just want to
pass the dentry I'd say. But we can't do that because of security
modules. 

Some security modules call security_d_instantiate() which in turn calls
__vfs_{g,s}et_xattr() in the hook implementation. That's at least true
of SELinux and Smack iirc. They want dentry and inode but
security_d_instantiate() is called in e.g., d_instantiate and d_add()
before the inode is attached to the dentry:

selinux_d_instantiate()
-> inode_doinit_with_dentry()
   -> inode_doinit_use_xattr()
      -> __vfs_getxattr()

smack_d_instantiate()
-> __vfs_getxattr()
-> __vfs_setxattr()

So that mandates both dentry and inode in vfs xattr helpers.

I don't think we can and want to solve this in this patchset. For now we
can stick with the naming as set by precedent and then in the future the
security modules can decide whether they want to do a rename patchset
for most of the xattr hooks at some point.

> 
> > And I want to minimize - ideally get rid of at some point - separate
> > calls to security_*() and evm_*() or ima_() in the vfs. So the evm hook
> > should please stay in there.
> 
> For the record, I want to get rid of the IMA and EVM specific hooks in
> the kernel.  They were a necessity back when there could only be one
> LSM active at a given time, but with that no longer the case I see
> little reason why IMA/EVM/etc. remain separate; it makes the code
> worse and complicates a lot of things both at the LSM layer as well as
> the rest of the kernel.  I've mentioned this to a few people,
> including Mimi, and it came up during at talk at LPC this year.

Sounds good.

Re: [PATCH 10/29] selinux: implement set acl hook

[Paul Moore]

On Mon, Sep 26, 2022 at 5:05 AM Christian Brauner <brauner@kernel.org> wrote:
> On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote:
> > On Fri, Sep 23, 2022 at 10:35 AM Christian Brauner <brauner@kernel.org> wrote:
> > > On Fri, Sep 23, 2022 at 10:26:35AM -0400, Paul Moore wrote:
> > > > On Fri, Sep 23, 2022 at 3:57 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > > On Fri, Sep 23, 2022 at 08:47:07AM +0200, Christoph Hellwig wrote:
> > > > > > On Thu, Sep 22, 2022 at 01:16:57PM -0400, Paul Moore wrote:
> > > > > > > properly review the changes, but one thing immediately jumped out at
> > > > > > > me when looking at this: why is the LSM hook
> > > > > > > "security_inode_set_acl()" when we are passing a dentry instead of an
> > > > > > > inode?  We don't have a lot of them, but there are
> > > > > > > `security_dentry_*()` LSM hooks in the existing kernel code.
> > > > > >
> > > > > > I'm no LSM expert, but isn't the inode vs dentry for if it is
> > > > > > related to an inode operation or dentry operation, not about that
> > > > > > the first argument is?
> > > > >
> > > > > Indeed. For example ...
> > > >
> > > > If the goal is for this LSM hook to operate on an inode and not a
> > > > dentry, let's pass it an inode instead.  This should help prevent
> > >
> > > I would be ok with that but EVM requires a dentry being passed and as
> > > evm is called from security_inode_set_acl() exactly like it is from
> > > security_inode_setxattr() and similar the hook has to take a dentry.
> >
> > If a dentry is truly needed by EVM (a quick look indicates that it may
> > just be for the VFS getxattr API, but I haven't traced the full code
> > path), then I'm having a hard time reconciling that this isn't a
> > dentry operation.  Yes, I get that the ACLs belong to the inode and
> > not the dentry, but then why do we need the dentry?  It seems like the
> > interfaces are broken slightly, or at least a little odd ... <shrug>
>
> There's multiple reasons for the generic xattr api to take a dentry. For
> example, there are quite a few filesystems that require dentry access
> during (specific or all) xattr operations. So ideally, we'd just want to
> pass the dentry I'd say ...

Independent of the naming issue discussed above, I want to make it
clear that in general I believe that the dentry is more useful to the
LSMs if for no other reason that it is pretty trivial to go from a
dentry to an inode, whereas the opposite is not true.  Of course there
are cases where the dentry is not always available, or the
connectivity between the dentry and the inode is not yet present.  In
those cases we would obviously need to pass the inode, or both the
inode and the dentry, which gets me to my next point and my motivation
behind bringing all this up once we started down the rathole ...

My main concern is making sure that the LSM hooks are declared in such
a way that they are fairly resistant to misuse; not that I expect any
malice, but I do believe accidental misuse of the hooks is legitimate
concern.  With respect to the various VFS related hooks, one of the
things that has always caught my attention in this respect has been
hooks which pass both a dentry and a inode for the same file.  Of
course there are cases where this will always be necessary, but I hate
to see bad interfaces continued forward simply because "that's the way
we've always done it".

In this particular patch the hook prototype is reasonably well defined
as far as I'm concerned, with no worries of both a dentry and an
inode, so that's good.  I still don't really like the name disconnect
between the function name and the parameter list (inode vs dentry),
but que sera sera; my appetite for argument here is rapidly waning.

-- 
paul-moore.com

Re: [PATCH 10/29] selinux: implement set acl hook

[Christoph Hellwig]

On Fri, Sep 23, 2022 at 01:35:08PM -0400, Paul Moore wrote:
> If a dentry is truly needed by EVM (a quick look indicates that it may
> just be for the VFS getxattr API, but I haven't traced the full code
> path), then I'm having a hard time reconciling that this isn't a
> dentry operation.  Yes, I get that the ACLs belong to the inode and
> not the dentry, but then why do we need the dentry?  It seems like the
> interfaces are broken slightly, or at least a little odd ... <shrug>

The dentry_operations are bit misnamed and should probably have been
called dcache_operations, that is they are all about managing the
dcache state and closely related operations.  ACLs aren't like that.