[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 04/29] cifs: implement get acl method

[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].

In order to build a type safe posix api around get and set acl we need
all filesystem to implement get and set acl.

So far cifs wasn't able to implement get and set acl inode operations
because it needs access to the dentry. Now that we extended the set acl
inode operation to take a dentry argument and added a new get acl inode
operation that takes a dentry argument we can let cifs implement get and
set acl inode operations.

This is mostly a copy and paste of the codepaths currently used in cifs'
posix acl xattr handler. After we have fully implemented the posix acl
api and switched the vfs over to it, the cifs specific posix acl xattr
handler and associated code will be removed and the code duplication
will go away.

Note, until the vfs has been switched to the new posix acl api this
patch is a non-functional change.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/cifs/cifsacl.c   |  63 +++++++++++++++
 fs/cifs/cifsfs.c    |   2 +
 fs/cifs/cifsproto.h |   6 ++
 fs/cifs/cifssmb.c   | 190 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 261 insertions(+)

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index fa480d62f313..06ae721ec1e7 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -13,6 +13,7 @@
 #include <linux/string.h>
 #include <linux/keyctl.h>
 #include <linux/key-type.h>
+#include <uapi/linux/posix_acl.h>
 #include <keys/user-type.h>
 #include "cifspdu.h"
 #include "cifsglob.h"
@@ -20,6 +21,8 @@
 #include "cifsproto.h"
 #include "cifs_debug.h"
 #include "fs_context.h"
+#include "cifs_fs_sb.h"
+#include "cifs_unicode.h"
 
 /* security id for everyone/world system group */
 static const struct cifs_sid sid_everyone = {
@@ -1668,3 +1671,63 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 	kfree(pntsd);
 	return rc;
 }
+
+struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
+			       struct dentry *dentry, int type)
+{
+#if defined(CONFIG_CIFS_ALLOW_INSECURE_LEGACY) && defined(CONFIG_CIFS_POSIX)
+	struct posix_acl *acl = NULL;
+	ssize_t rc = -EOPNOTSUPP;
+	unsigned int xid;
+	struct super_block *sb = dentry->d_sb;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
+	struct tcon_link *tlink;
+	struct cifs_tcon *pTcon;
+	const char *full_path;
+	void *page;
+
+	tlink = cifs_sb_tlink(cifs_sb);
+	if (IS_ERR(tlink))
+		return ERR_CAST(tlink);
+	pTcon = tlink_tcon(tlink);
+
+	xid = get_xid();
+	page = alloc_dentry_path();
+
+	full_path = build_path_from_dentry(dentry, page);
+	if (IS_ERR(full_path)) {
+		acl = ERR_CAST(full_path);
+		goto out;
+	}
+
+	/* return alt name if available as pseudo attr */
+	switch (type) {
+	case ACL_TYPE_ACCESS:
+		if (sb->s_flags & SB_POSIXACL)
+			rc = cifs_do_get_acl(xid, pTcon, full_path, &acl,
+					     ACL_TYPE_ACCESS,
+					     cifs_sb->local_nls,
+					     cifs_remap(cifs_sb));
+		break;
+
+	case ACL_TYPE_DEFAULT:
+		if (sb->s_flags & SB_POSIXACL)
+			rc = cifs_do_get_acl(xid, pTcon, full_path, &acl,
+					     ACL_TYPE_DEFAULT,
+					     cifs_sb->local_nls,
+					     cifs_remap(cifs_sb));
+		break;
+	}
+
+	if (rc == -EINVAL)
+		acl = ERR_PTR(-EOPNOTSUPP);
+
+out:
+	free_dentry_path(page);
+	free_xid(xid);
+	cifs_put_tlink(tlink);
+	return acl;
+#else
+	return ERR_PTR(-EOPNOTSUPP);
+#endif
+}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index f54d8bf2732a..5c00d79fda99 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1128,6 +1128,7 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.symlink = cifs_symlink,
 	.mknod   = cifs_mknod,
 	.listxattr = cifs_listxattr,
+	.get_acl = cifs_get_acl,
 };
 
 const struct inode_operations cifs_file_inode_ops = {
@@ -1136,6 +1137,7 @@ const struct inode_operations cifs_file_inode_ops = {
 	.permission = cifs_permission,
 	.listxattr = cifs_listxattr,
 	.fiemap = cifs_fiemap,
+	.get_acl = cifs_get_acl,
 };
 
 const struct inode_operations cifs_symlink_inode_ops = {
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 3bc94bcc7177..953fd910da70 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -225,6 +225,8 @@ extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
 				      const char *, u32 *, u32);
 extern struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *,
 				const struct cifs_fid *, u32 *, u32);
+extern struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
+				      struct dentry *dentry, int type);
 extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
 				const char *, int);
 extern unsigned int setup_authusers_ACE(struct cifs_ace *pace);
@@ -542,6 +544,10 @@ extern int CIFSSMBGetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
 		const unsigned char *searchName,
 		char *acl_inf, const int buflen, const int acl_type,
 		const struct nls_table *nls_codepage, int remap_special_chars);
+extern int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
+			   const unsigned char *searchName,
+			   struct posix_acl **acl, const int acl_type,
+			   const struct nls_table *nls_codepage, int remap);
 extern int CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
 		const unsigned char *fileName,
 		const char *local_acl, const int buflen, const int acl_type,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 7aa91e272027..f53d2eb100ca 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -3212,6 +3212,196 @@ CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
 	return rc;
 }
 
+/**
+ * cifs_init_posix_acl - convert ACL from cifs to POSIX ACL format
+ * @ace: POSIX ACL entry to store converted ACL into
+ * @cifs: ACL in cifs format
+ *
+ * Convert an Access Control Entry from wire format to local POSIX xattr
+ * format.
+ *
+ * Note that the @cifs_uid member is used to store both {g,u}id_t.
+ */
+static void cifs_init_posix_acl(struct posix_acl_entry *ace,
+				struct cifs_posix_ace *cifs_ace)
+{
+	/* u8 cifs fields do not need le conversion */
+	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
+	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
+	switch (ace->e_tag) {
+	case ACL_USER:
+		ace->e_uid = make_kuid(&init_user_ns,
+				  cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid)));
+		break;
+	case ACL_GROUP:
+		ace->e_gid = make_kgid(&init_user_ns,
+				  cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid)));
+		break;
+	}
+/*
+	cifs_dbg(FYI, "perm %d tag %d id %d\n",
+		 ace->e_perm, ace->e_tag, ace->e_id);
+*/
+
+	return;
+}
+
+/**
+ * cifs_to_posix_acl - copy cifs ACL format to POSIX ACL format
+ * @acl: ACLs returned in POSIX ACL format
+ * @src: ACLs in cifs format
+ * @acl_type: type of POSIX ACL requested
+ * @size_of_data_area: size of SMB we got
+ *
+ * This function converts ACLs from cifs format to POSIX ACL format.
+ * If @acl is NULL then the size of the buffer required to store POSIX ACLs in
+ * their uapi format is returned.
+ */
+static int cifs_to_posix_acl(struct posix_acl **acl, char *src,
+			     const int acl_type, const int size_of_data_area)
+{
+	int size =  0;
+	__u16 count;
+	struct cifs_posix_ace *pACE;
+	struct cifs_posix_acl *cifs_acl = (struct cifs_posix_acl *)src;
+	struct posix_acl *kacl = NULL;
+	struct posix_acl_entry *pa, *pe;
+
+	if (le16_to_cpu(cifs_acl->version) != CIFS_ACL_VERSION)
+		return -EOPNOTSUPP;
+
+	if (acl_type == ACL_TYPE_ACCESS) {
+		count = le16_to_cpu(cifs_acl->access_entry_count);
+		pACE = &cifs_acl->ace_array[0];
+		size = sizeof(struct cifs_posix_acl);
+		size += sizeof(struct cifs_posix_ace) * count;
+		/* check if we would go beyond end of SMB */
+		if (size_of_data_area < size) {
+			cifs_dbg(FYI, "bad CIFS POSIX ACL size %d vs. %d\n",
+				 size_of_data_area, size);
+			return -EINVAL;
+		}
+	} else if (acl_type == ACL_TYPE_DEFAULT) {
+		count = le16_to_cpu(cifs_acl->access_entry_count);
+		size = sizeof(struct cifs_posix_acl);
+		size += sizeof(struct cifs_posix_ace) * count;
+/* skip past access ACEs to get to default ACEs */
+		pACE = &cifs_acl->ace_array[count];
+		count = le16_to_cpu(cifs_acl->default_entry_count);
+		size += sizeof(struct cifs_posix_ace) * count;
+		/* check if we would go beyond end of SMB */
+		if (size_of_data_area < size)
+			return -EINVAL;
+	} else {
+		/* illegal type */
+		return -EINVAL;
+	}
+
+	/* Allocate number of POSIX ACLs to store in VFS format. */
+	kacl = posix_acl_alloc(count, GFP_NOFS);
+	if (!kacl)
+		return -ENOMEM;
+
+	FOREACH_ACL_ENTRY(pa, kacl, pe) {
+		cifs_init_posix_acl(pa, pACE);
+		pACE++;
+	}
+
+	*acl = kacl;
+	return 0;
+}
+
+int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
+		    const unsigned char *searchName, struct posix_acl **acl,
+		    const int acl_type, const struct nls_table *nls_codepage,
+		    int remap)
+{
+/* SMB_QUERY_POSIX_ACL */
+	TRANSACTION2_QPI_REQ *pSMB = NULL;
+	TRANSACTION2_QPI_RSP *pSMBr = NULL;
+	int rc = 0;
+	int bytes_returned;
+	int name_len;
+	__u16 params, byte_count;
+
+	cifs_dbg(FYI, "In GetPosixACL (Unix) for path %s\n", searchName);
+
+queryAclRetry:
+	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
+		(void **) &pSMBr);
+	if (rc)
+		return rc;
+
+	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
+		name_len =
+			cifsConvertToUTF16((__le16 *) pSMB->FileName,
+					   searchName, PATH_MAX, nls_codepage,
+					   remap);
+		name_len++;     /* trailing null */
+		name_len *= 2;
+		pSMB->FileName[name_len] = 0;
+		pSMB->FileName[name_len+1] = 0;
+	} else {
+		name_len = copy_path_name(pSMB->FileName, searchName);
+	}
+
+	params = 2 /* level */  + 4 /* rsrvd */  + name_len /* incl null */ ;
+	pSMB->TotalDataCount = 0;
+	pSMB->MaxParameterCount = cpu_to_le16(2);
+	/* BB find exact max data count below from sess structure BB */
+	pSMB->MaxDataCount = cpu_to_le16(4000);
+	pSMB->MaxSetupCount = 0;
+	pSMB->Reserved = 0;
+	pSMB->Flags = 0;
+	pSMB->Timeout = 0;
+	pSMB->Reserved2 = 0;
+	pSMB->ParameterOffset = cpu_to_le16(
+		offsetof(struct smb_com_transaction2_qpi_req,
+			 InformationLevel) - 4);
+	pSMB->DataCount = 0;
+	pSMB->DataOffset = 0;
+	pSMB->SetupCount = 1;
+	pSMB->Reserved3 = 0;
+	pSMB->SubCommand = cpu_to_le16(TRANS2_QUERY_PATH_INFORMATION);
+	byte_count = params + 1 /* pad */ ;
+	pSMB->TotalParameterCount = cpu_to_le16(params);
+	pSMB->ParameterCount = pSMB->TotalParameterCount;
+	pSMB->InformationLevel = cpu_to_le16(SMB_QUERY_POSIX_ACL);
+	pSMB->Reserved4 = 0;
+	inc_rfc1001_len(pSMB, byte_count);
+	pSMB->ByteCount = cpu_to_le16(byte_count);
+
+	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
+		(struct smb_hdr *) pSMBr, &bytes_returned, 0);
+	cifs_stats_inc(&tcon->stats.cifs_stats.num_acl_get);
+	if (rc) {
+		cifs_dbg(FYI, "Send error in Query POSIX ACL = %d\n", rc);
+	} else {
+		/* decode response */
+
+		rc = validate_t2((struct smb_t2_rsp *)pSMBr);
+		/* BB also check enough total bytes returned */
+		if (rc || get_bcc(&pSMBr->hdr) < 2)
+			rc = -EIO;      /* bad smb */
+		else {
+			__u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
+			__u16 count = le16_to_cpu(pSMBr->t2.DataCount);
+			rc = cifs_to_posix_acl(acl,
+				(char *)&pSMBr->hdr.Protocol+data_offset,
+				acl_type, count);
+		}
+	}
+	cifs_buf_release(pSMB);
+	/*
+	 * The else branch after SendReceive() doesn't return EAGAIN so if we
+	 * allocated @acl in cifs_to_posix_acl() we are guaranteed to return
+	 * here and don't leak POSIX ACLs.
+	 */
+	if (rc == -EAGAIN)
+		goto queryAclRetry;
+	return rc;
+}
+
 int
 CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
 	       const int netfid, __u64 *pExtAttrBits, __u64 *pMask)
-- 
2.34.1

[PATCH 05/29] cifs: implement set acl method

[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].

In order to build a type safe posix api around get and set acl we need
all filesystem to implement get and set acl.

So far cifs wasn't able to implement get and set acl inode operations
because it needs access to the dentry. Now that we extended the set acl
inode operation to take a dentry argument and added a new get acl inode
operation that takes a dentry argument we can let cifs implement get and
set acl inode operations.

This is mostly a copy and paste of the codepaths currently used in cifs'
posix acl xattr handler. After we have fully implemented the posix acl
api and switched the vfs over to it, the cifs specific posix acl xattr
handler and associated code will be removed and the code duplication
will go away.

Note, until the vfs has been switched to the new posix acl api this
patch is a non-functional change.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/cifs/cifsacl.c   |  74 +++++++++++++++++++++
 fs/cifs/cifsfs.c    |   2 +
 fs/cifs/cifsproto.h |   6 ++
 fs/cifs/cifssmb.c   | 152 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 234 insertions(+)

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index 06ae721ec1e7..02bc096dad89 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -14,6 +14,8 @@
 #include <linux/keyctl.h>
 #include <linux/key-type.h>
 #include <uapi/linux/posix_acl.h>
+#include <linux/posix_acl.h>
+#include <linux/posix_acl_xattr.h>
 #include <keys/user-type.h>
 #include "cifspdu.h"
 #include "cifsglob.h"
@@ -1731,3 +1733,75 @@ struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
 	return ERR_PTR(-EOPNOTSUPP);
 #endif
 }
+
+int cifs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
+		 struct posix_acl *acl, int type)
+{
+	int rc = -EOPNOTSUPP;
+	unsigned int xid;
+	struct super_block *sb = dentry->d_sb;
+	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
+	struct tcon_link *tlink;
+	struct cifs_tcon *pTcon;
+	const char *full_path;
+	void *page;
+
+	tlink = cifs_sb_tlink(cifs_sb);
+	if (IS_ERR(tlink))
+		return PTR_ERR(tlink);
+	pTcon = tlink_tcon(tlink);
+
+	xid = get_xid();
+	page = alloc_dentry_path();
+
+	full_path = build_path_from_dentry(dentry, page);
+	if (IS_ERR(full_path)) {
+		rc = PTR_ERR(full_path);
+		goto out;
+	}
+	/* return dos attributes as pseudo xattr */
+	/* return alt name if available as pseudo attr */
+
+	/* if proc/fs/cifs/streamstoxattr is set then
+		search server for EAs or streams to
+		returns as xattrs */
+	if (posix_acl_xattr_size(acl->a_count) > CIFSMaxBufSize) {
+		cifs_dbg(FYI, "size of EA value too large\n");
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	switch (type) {
+#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
+	case ACL_TYPE_ACCESS:
+#ifdef CONFIG_CIFS_POSIX
+		if (!acl)
+			goto out;
+		if (sb->s_flags & SB_POSIXACL)
+			rc = cifs_do_set_acl(xid, pTcon, full_path, acl,
+					     ACL_TYPE_ACCESS,
+					     cifs_sb->local_nls,
+					     cifs_remap(cifs_sb));
+#endif  /* CONFIG_CIFS_POSIX */
+		break;
+
+	case ACL_TYPE_DEFAULT:
+#ifdef CONFIG_CIFS_POSIX
+		if (!acl)
+			goto out;
+		if (sb->s_flags & SB_POSIXACL)
+			rc = cifs_do_set_acl(xid, pTcon, full_path, acl,
+					     ACL_TYPE_DEFAULT,
+					     cifs_sb->local_nls,
+					     cifs_remap(cifs_sb));
+#endif  /* CONFIG_CIFS_POSIX */
+		break;
+#endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
+	}
+
+out:
+	free_dentry_path(page);
+	free_xid(xid);
+	cifs_put_tlink(tlink);
+	return rc;
+}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5c00d79fda99..c8d46c1b10e4 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1129,6 +1129,7 @@ const struct inode_operations cifs_dir_inode_ops = {
 	.mknod   = cifs_mknod,
 	.listxattr = cifs_listxattr,
 	.get_acl = cifs_get_acl,
+	.set_acl = cifs_set_acl,
 };
 
 const struct inode_operations cifs_file_inode_ops = {
@@ -1138,6 +1139,7 @@ const struct inode_operations cifs_file_inode_ops = {
 	.listxattr = cifs_listxattr,
 	.fiemap = cifs_fiemap,
 	.get_acl = cifs_get_acl,
+	.set_acl = cifs_set_acl,
 };
 
 const struct inode_operations cifs_symlink_inode_ops = {
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 953fd910da70..279e867dee2e 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -227,6 +227,8 @@ extern struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *,
 				const struct cifs_fid *, u32 *, u32);
 extern struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
 				      struct dentry *dentry, int type);
+extern int cifs_set_acl(struct user_namespace *mnt_userns,
+			struct dentry *dentry, struct posix_acl *acl, int type);
 extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
 				const char *, int);
 extern unsigned int setup_authusers_ACE(struct cifs_ace *pace);
@@ -552,6 +554,10 @@ extern int CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
 		const unsigned char *fileName,
 		const char *local_acl, const int buflen, const int acl_type,
 		const struct nls_table *nls_codepage, int remap_special_chars);
+extern int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
+			   const unsigned char *fileName,
+			   const struct posix_acl *acl, const int acl_type,
+			   const struct nls_table *nls_codepage, int remap);
 extern int CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
 			const int netfid, __u64 *pExtAttrBits, __u64 *pMask);
 #endif /* CIFS_ALLOW_INSECURE_LEGACY */
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index f53d2eb100ca..2d7ec32fdc7a 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -3402,6 +3402,158 @@ int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	return rc;
 }
 
+/**
+ * cifs_init_ace - convert ACL entry from POSIX ACL to cifs format
+ * @cifs_ace: the cifs ACL entry to store into
+ * @local_ace: the POSIX ACL entry to convert
+ */
+static void cifs_init_ace(struct cifs_posix_ace *cifs_ace,
+			  const struct posix_acl_entry *local_ace)
+{
+	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
+	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
+
+	switch (local_ace->e_tag) {
+	case ACL_USER:
+		cifs_ace->cifs_uid =
+			cpu_to_le64(from_kuid(&init_user_ns, local_ace->e_uid));
+		break;
+	case ACL_GROUP:
+		cifs_ace->cifs_uid =
+			cpu_to_le64(from_kgid(&init_user_ns, local_ace->e_gid));
+		break;
+	default:
+		cifs_ace->cifs_uid = cpu_to_le64(-1);
+	}
+}
+
+/**
+ * posix_acl_to_cifs - convert ACLs from POSIX ACL to cifs format
+ * @parm_data: ACLs in cifs format to conver to
+ * @acl: ACLs in POSIX ACL format to convert from
+ * @acl_type: the type of POSIX ACLs stored in @acl
+ *
+ * Return: the number cifs ACL entries after conversion
+ */
+static __u16 posix_acl_to_cifs(char *parm_data, const struct posix_acl *acl,
+			       const int acl_type)
+{
+	__u16 rc = 0;
+	struct cifs_posix_acl *cifs_acl = (struct cifs_posix_acl *)parm_data;
+	const struct posix_acl_entry *pa, *pe;
+	int count;
+	int i = 0;
+
+	if ((acl == NULL) || (cifs_acl == NULL))
+		return 0;
+
+	count = acl->a_count;
+	cifs_dbg(FYI, "setting acl with %d entries\n", count);
+
+	/*
+	 * Note that the uapi POSIX ACL version is verified by the VFS and is
+	 * independent of the cifs ACL version. Changing the POSIX ACL version
+	 * is a uapi change and if it's changed we will pass down the POSIX ACL
+	 * version in struct posix_acl from the VFS. For now there's really
+	 * only one that all filesystems know how to deal with.
+	 */
+	cifs_acl->version = cpu_to_le16(1);
+	if (acl_type == ACL_TYPE_ACCESS) {
+		cifs_acl->access_entry_count = cpu_to_le16(count);
+		cifs_acl->default_entry_count = cpu_to_le16(0xFFFF);
+	} else if (acl_type == ACL_TYPE_DEFAULT) {
+		cifs_acl->default_entry_count = cpu_to_le16(count);
+		cifs_acl->access_entry_count = cpu_to_le16(0xFFFF);
+	} else {
+		cifs_dbg(FYI, "unknown ACL type %d\n", acl_type);
+		return 0;
+	}
+	FOREACH_ACL_ENTRY(pa, acl, pe) {
+		cifs_init_ace(&cifs_acl->ace_array[i++], pa);
+	}
+	if (rc == 0) {
+		rc = (__u16)(count * sizeof(struct cifs_posix_ace));
+		rc += sizeof(struct cifs_posix_acl);
+		/* BB add check to make sure ACL does not overflow SMB */
+	}
+	return rc;
+}
+
+int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
+		    const unsigned char *fileName, const struct posix_acl *acl,
+		    const int acl_type, const struct nls_table *nls_codepage,
+		    int remap)
+{
+	struct smb_com_transaction2_spi_req *pSMB = NULL;
+	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
+	char *parm_data;
+	int name_len;
+	int rc = 0;
+	int bytes_returned = 0;
+	__u16 params, byte_count, data_count, param_offset, offset;
+
+	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
+setAclRetry:
+	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
+		      (void **) &pSMBr);
+	if (rc)
+		return rc;
+	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
+		name_len =
+			cifsConvertToUTF16((__le16 *) pSMB->FileName, fileName,
+					   PATH_MAX, nls_codepage, remap);
+		name_len++;     /* trailing null */
+		name_len *= 2;
+	} else {
+		name_len = copy_path_name(pSMB->FileName, fileName);
+	}
+	params = 6 + name_len;
+	pSMB->MaxParameterCount = cpu_to_le16(2);
+	/* BB find max SMB size from sess */
+	pSMB->MaxDataCount = cpu_to_le16(1000);
+	pSMB->MaxSetupCount = 0;
+	pSMB->Reserved = 0;
+	pSMB->Flags = 0;
+	pSMB->Timeout = 0;
+	pSMB->Reserved2 = 0;
+	param_offset = offsetof(struct smb_com_transaction2_spi_req,
+				InformationLevel) - 4;
+	offset = param_offset + params;
+	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
+	pSMB->ParameterOffset = cpu_to_le16(param_offset);
+
+	/* convert to on the wire format for POSIX ACL */
+	data_count = posix_acl_to_cifs(parm_data, acl, acl_type);
+
+	if (data_count == 0) {
+		rc = -EOPNOTSUPP;
+		goto setACLerrorExit;
+	}
+	pSMB->DataOffset = cpu_to_le16(offset);
+	pSMB->SetupCount = 1;
+	pSMB->Reserved3 = 0;
+	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
+	pSMB->InformationLevel = cpu_to_le16(SMB_SET_POSIX_ACL);
+	byte_count = 3 /* pad */  + params + data_count;
+	pSMB->DataCount = cpu_to_le16(data_count);
+	pSMB->TotalDataCount = pSMB->DataCount;
+	pSMB->ParameterCount = cpu_to_le16(params);
+	pSMB->TotalParameterCount = pSMB->ParameterCount;
+	pSMB->Reserved4 = 0;
+	inc_rfc1001_len(pSMB, byte_count);
+	pSMB->ByteCount = cpu_to_le16(byte_count);
+	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
+			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
+	if (rc)
+		cifs_dbg(FYI, "Set POSIX ACL returned %d\n", rc);
+
+setACLerrorExit:
+	cifs_buf_release(pSMB);
+	if (rc == -EAGAIN)
+		goto setAclRetry;
+	return rc;
+}
+
 int
 CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
 	       const int netfid, __u64 *pExtAttrBits, __u64 *pMask)
-- 
2.34.1

[PATCH 18/29] ksmbd: use vfs_remove_acl()

[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].

Now that we've switched all filesystems that can serve as the lower
filesystem for ksmbd we can switch ksmbd over to rely on
the posix acl api. Note that this is orthogonal to switching the vfs
itself over.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/ksmbd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c
index 430962dd2efa..482bd0911127 100644
--- a/fs/ksmbd/vfs.c
+++ b/fs/ksmbd/vfs.c
@@ -1311,7 +1311,7 @@ int ksmbd_vfs_remove_acl_xattrs(struct user_namespace *user_ns,
 			     sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1) ||
 		    !strncmp(name, XATTR_NAME_POSIX_ACL_DEFAULT,
 			     sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1)) {
-			err = ksmbd_vfs_remove_xattr(user_ns, dentry, name);
+			err = vfs_remove_acl(user_ns, dentry, name);
 			if (err)
 				ksmbd_debug(SMB,
 					    "remove acl xattr failed : %s\n", name);
-- 
2.34.1

[PATCH 27/29] cifs: use stub posix acl handlers

[Christian Brauner]

Now that cifs supports the get and set acl inode operations and the vfs
has been switched to the new posi api, cifs can simply rely on the stub
posix acl handlers. The custom xattr handlers and associated unused
helpers can be removed.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/cifs/cifsproto.h |   8 --
 fs/cifs/cifssmb.c   | 298 --------------------------------------------
 fs/cifs/xattr.c     |  68 +---------
 3 files changed, 4 insertions(+), 370 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 279e867dee2e..9259da1b885d 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -542,18 +542,10 @@ extern int CIFSSMBGetCIFSACL(const unsigned int xid, struct cifs_tcon *tcon,
 			__u16 fid, struct cifs_ntsd **acl_inf, __u32 *buflen);
 extern int CIFSSMBSetCIFSACL(const unsigned int, struct cifs_tcon *, __u16,
 			struct cifs_ntsd *, __u32, int);
-extern int CIFSSMBGetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
-		const unsigned char *searchName,
-		char *acl_inf, const int buflen, const int acl_type,
-		const struct nls_table *nls_codepage, int remap_special_chars);
 extern int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
 			   const unsigned char *searchName,
 			   struct posix_acl **acl, const int acl_type,
 			   const struct nls_table *nls_codepage, int remap);
-extern int CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
-		const unsigned char *fileName,
-		const char *local_acl, const int buflen, const int acl_type,
-		const struct nls_table *nls_codepage, int remap_special_chars);
 extern int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 			   const unsigned char *fileName,
 			   const struct posix_acl *acl, const int acl_type,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 2d7ec32fdc7a..779b4322a2ce 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2914,304 +2914,6 @@ CIFSSMB_set_compression(const unsigned int xid, struct cifs_tcon *tcon,
 
 #ifdef CONFIG_CIFS_POSIX
 
-/*Convert an Access Control Entry from wire format to local POSIX xattr format*/
-static void cifs_convert_ace(struct posix_acl_xattr_entry *ace,
-			     struct cifs_posix_ace *cifs_ace)
-{
-	/* u8 cifs fields do not need le conversion */
-	ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
-	ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
-	ace->e_id   = cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid));
-/*
-	cifs_dbg(FYI, "perm %d tag %d id %d\n",
-		 ace->e_perm, ace->e_tag, ace->e_id);
-*/
-
-	return;
-}
-
-/* Convert ACL from CIFS POSIX wire format to local Linux POSIX ACL xattr */
-static int cifs_copy_posix_acl(char *trgt, char *src, const int buflen,
-			       const int acl_type, const int size_of_data_area)
-{
-	int size =  0;
-	int i;
-	__u16 count;
-	struct cifs_posix_ace *pACE;
-	struct cifs_posix_acl *cifs_acl = (struct cifs_posix_acl *)src;
-	struct posix_acl_xattr_header *local_acl = (void *)trgt;
-
-	if (le16_to_cpu(cifs_acl->version) != CIFS_ACL_VERSION)
-		return -EOPNOTSUPP;
-
-	if (acl_type == ACL_TYPE_ACCESS) {
-		count = le16_to_cpu(cifs_acl->access_entry_count);
-		pACE = &cifs_acl->ace_array[0];
-		size = sizeof(struct cifs_posix_acl);
-		size += sizeof(struct cifs_posix_ace) * count;
-		/* check if we would go beyond end of SMB */
-		if (size_of_data_area < size) {
-			cifs_dbg(FYI, "bad CIFS POSIX ACL size %d vs. %d\n",
-				 size_of_data_area, size);
-			return -EINVAL;
-		}
-	} else if (acl_type == ACL_TYPE_DEFAULT) {
-		count = le16_to_cpu(cifs_acl->access_entry_count);
-		size = sizeof(struct cifs_posix_acl);
-		size += sizeof(struct cifs_posix_ace) * count;
-/* skip past access ACEs to get to default ACEs */
-		pACE = &cifs_acl->ace_array[count];
-		count = le16_to_cpu(cifs_acl->default_entry_count);
-		size += sizeof(struct cifs_posix_ace) * count;
-		/* check if we would go beyond end of SMB */
-		if (size_of_data_area < size)
-			return -EINVAL;
-	} else {
-		/* illegal type */
-		return -EINVAL;
-	}
-
-	size = posix_acl_xattr_size(count);
-	if ((buflen == 0) || (local_acl == NULL)) {
-		/* used to query ACL EA size */
-	} else if (size > buflen) {
-		return -ERANGE;
-	} else /* buffer big enough */ {
-		struct posix_acl_xattr_entry *ace = (void *)(local_acl + 1);
-
-		local_acl->a_version = cpu_to_le32(POSIX_ACL_XATTR_VERSION);
-		for (i = 0; i < count ; i++) {
-			cifs_convert_ace(&ace[i], pACE);
-			pACE++;
-		}
-	}
-	return size;
-}
-
-static void convert_ace_to_cifs_ace(struct cifs_posix_ace *cifs_ace,
-				     const struct posix_acl_xattr_entry *local_ace)
-{
-	cifs_ace->cifs_e_perm = le16_to_cpu(local_ace->e_perm);
-	cifs_ace->cifs_e_tag =  le16_to_cpu(local_ace->e_tag);
-	/* BB is there a better way to handle the large uid? */
-	if (local_ace->e_id == cpu_to_le32(-1)) {
-	/* Probably no need to le convert -1 on any arch but can not hurt */
-		cifs_ace->cifs_uid = cpu_to_le64(-1);
-	} else
-		cifs_ace->cifs_uid = cpu_to_le64(le32_to_cpu(local_ace->e_id));
-/*
-	cifs_dbg(FYI, "perm %d tag %d id %d\n",
-		 ace->e_perm, ace->e_tag, ace->e_id);
-*/
-}
-
-/* Convert ACL from local Linux POSIX xattr to CIFS POSIX ACL wire format */
-static __u16 ACL_to_cifs_posix(char *parm_data, const char *pACL,
-			       const int buflen, const int acl_type)
-{
-	__u16 rc = 0;
-	struct cifs_posix_acl *cifs_acl = (struct cifs_posix_acl *)parm_data;
-	struct posix_acl_xattr_header *local_acl = (void *)pACL;
-	struct posix_acl_xattr_entry *ace = (void *)(local_acl + 1);
-	int count;
-	int i;
-
-	if ((buflen == 0) || (pACL == NULL) || (cifs_acl == NULL))
-		return 0;
-
-	count = posix_acl_xattr_count((size_t)buflen);
-	cifs_dbg(FYI, "setting acl with %d entries from buf of length %d and version of %d\n",
-		 count, buflen, le32_to_cpu(local_acl->a_version));
-	if (le32_to_cpu(local_acl->a_version) != 2) {
-		cifs_dbg(FYI, "unknown POSIX ACL version %d\n",
-			 le32_to_cpu(local_acl->a_version));
-		return 0;
-	}
-	cifs_acl->version = cpu_to_le16(1);
-	if (acl_type == ACL_TYPE_ACCESS) {
-		cifs_acl->access_entry_count = cpu_to_le16(count);
-		cifs_acl->default_entry_count = cpu_to_le16(0xFFFF);
-	} else if (acl_type == ACL_TYPE_DEFAULT) {
-		cifs_acl->default_entry_count = cpu_to_le16(count);
-		cifs_acl->access_entry_count = cpu_to_le16(0xFFFF);
-	} else {
-		cifs_dbg(FYI, "unknown ACL type %d\n", acl_type);
-		return 0;
-	}
-	for (i = 0; i < count; i++)
-		convert_ace_to_cifs_ace(&cifs_acl->ace_array[i], &ace[i]);
-	if (rc == 0) {
-		rc = (__u16)(count * sizeof(struct cifs_posix_ace));
-		rc += sizeof(struct cifs_posix_acl);
-		/* BB add check to make sure ACL does not overflow SMB */
-	}
-	return rc;
-}
-
-int
-CIFSSMBGetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
-		   const unsigned char *searchName,
-		   char *acl_inf, const int buflen, const int acl_type,
-		   const struct nls_table *nls_codepage, int remap)
-{
-/* SMB_QUERY_POSIX_ACL */
-	TRANSACTION2_QPI_REQ *pSMB = NULL;
-	TRANSACTION2_QPI_RSP *pSMBr = NULL;
-	int rc = 0;
-	int bytes_returned;
-	int name_len;
-	__u16 params, byte_count;
-
-	cifs_dbg(FYI, "In GetPosixACL (Unix) for path %s\n", searchName);
-
-queryAclRetry:
-	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
-		(void **) &pSMBr);
-	if (rc)
-		return rc;
-
-	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
-		name_len =
-			cifsConvertToUTF16((__le16 *) pSMB->FileName,
-					   searchName, PATH_MAX, nls_codepage,
-					   remap);
-		name_len++;     /* trailing null */
-		name_len *= 2;
-		pSMB->FileName[name_len] = 0;
-		pSMB->FileName[name_len+1] = 0;
-	} else {
-		name_len = copy_path_name(pSMB->FileName, searchName);
-	}
-
-	params = 2 /* level */  + 4 /* rsrvd */  + name_len /* incl null */ ;
-	pSMB->TotalDataCount = 0;
-	pSMB->MaxParameterCount = cpu_to_le16(2);
-	/* BB find exact max data count below from sess structure BB */
-	pSMB->MaxDataCount = cpu_to_le16(4000);
-	pSMB->MaxSetupCount = 0;
-	pSMB->Reserved = 0;
-	pSMB->Flags = 0;
-	pSMB->Timeout = 0;
-	pSMB->Reserved2 = 0;
-	pSMB->ParameterOffset = cpu_to_le16(
-		offsetof(struct smb_com_transaction2_qpi_req,
-			 InformationLevel) - 4);
-	pSMB->DataCount = 0;
-	pSMB->DataOffset = 0;
-	pSMB->SetupCount = 1;
-	pSMB->Reserved3 = 0;
-	pSMB->SubCommand = cpu_to_le16(TRANS2_QUERY_PATH_INFORMATION);
-	byte_count = params + 1 /* pad */ ;
-	pSMB->TotalParameterCount = cpu_to_le16(params);
-	pSMB->ParameterCount = pSMB->TotalParameterCount;
-	pSMB->InformationLevel = cpu_to_le16(SMB_QUERY_POSIX_ACL);
-	pSMB->Reserved4 = 0;
-	inc_rfc1001_len(pSMB, byte_count);
-	pSMB->ByteCount = cpu_to_le16(byte_count);
-
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-		(struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	cifs_stats_inc(&tcon->stats.cifs_stats.num_acl_get);
-	if (rc) {
-		cifs_dbg(FYI, "Send error in Query POSIX ACL = %d\n", rc);
-	} else {
-		/* decode response */
-
-		rc = validate_t2((struct smb_t2_rsp *)pSMBr);
-		/* BB also check enough total bytes returned */
-		if (rc || get_bcc(&pSMBr->hdr) < 2)
-			rc = -EIO;      /* bad smb */
-		else {
-			__u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
-			__u16 count = le16_to_cpu(pSMBr->t2.DataCount);
-			rc = cifs_copy_posix_acl(acl_inf,
-				(char *)&pSMBr->hdr.Protocol+data_offset,
-				buflen, acl_type, count);
-		}
-	}
-	cifs_buf_release(pSMB);
-	if (rc == -EAGAIN)
-		goto queryAclRetry;
-	return rc;
-}
-
-int
-CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
-		   const unsigned char *fileName,
-		   const char *local_acl, const int buflen,
-		   const int acl_type,
-		   const struct nls_table *nls_codepage, int remap)
-{
-	struct smb_com_transaction2_spi_req *pSMB = NULL;
-	struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
-	char *parm_data;
-	int name_len;
-	int rc = 0;
-	int bytes_returned = 0;
-	__u16 params, byte_count, data_count, param_offset, offset;
-
-	cifs_dbg(FYI, "In SetPosixACL (Unix) for path %s\n", fileName);
-setAclRetry:
-	rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
-		      (void **) &pSMBr);
-	if (rc)
-		return rc;
-	if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
-		name_len =
-			cifsConvertToUTF16((__le16 *) pSMB->FileName, fileName,
-					   PATH_MAX, nls_codepage, remap);
-		name_len++;     /* trailing null */
-		name_len *= 2;
-	} else {
-		name_len = copy_path_name(pSMB->FileName, fileName);
-	}
-	params = 6 + name_len;
-	pSMB->MaxParameterCount = cpu_to_le16(2);
-	/* BB find max SMB size from sess */
-	pSMB->MaxDataCount = cpu_to_le16(1000);
-	pSMB->MaxSetupCount = 0;
-	pSMB->Reserved = 0;
-	pSMB->Flags = 0;
-	pSMB->Timeout = 0;
-	pSMB->Reserved2 = 0;
-	param_offset = offsetof(struct smb_com_transaction2_spi_req,
-				InformationLevel) - 4;
-	offset = param_offset + params;
-	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
-	pSMB->ParameterOffset = cpu_to_le16(param_offset);
-
-	/* convert to on the wire format for POSIX ACL */
-	data_count = ACL_to_cifs_posix(parm_data, local_acl, buflen, acl_type);
-
-	if (data_count == 0) {
-		rc = -EOPNOTSUPP;
-		goto setACLerrorExit;
-	}
-	pSMB->DataOffset = cpu_to_le16(offset);
-	pSMB->SetupCount = 1;
-	pSMB->Reserved3 = 0;
-	pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
-	pSMB->InformationLevel = cpu_to_le16(SMB_SET_POSIX_ACL);
-	byte_count = 3 /* pad */  + params + data_count;
-	pSMB->DataCount = cpu_to_le16(data_count);
-	pSMB->TotalDataCount = pSMB->DataCount;
-	pSMB->ParameterCount = cpu_to_le16(params);
-	pSMB->TotalParameterCount = pSMB->ParameterCount;
-	pSMB->Reserved4 = 0;
-	inc_rfc1001_len(pSMB, byte_count);
-	pSMB->ByteCount = cpu_to_le16(byte_count);
-	rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
-			 (struct smb_hdr *) pSMBr, &bytes_returned, 0);
-	if (rc)
-		cifs_dbg(FYI, "Set POSIX ACL returned %d\n", rc);
-
-setACLerrorExit:
-	cifs_buf_release(pSMB);
-	if (rc == -EAGAIN)
-		goto setAclRetry;
-	return rc;
-}
-
 /**
  * cifs_init_posix_acl - convert ACL from cifs to POSIX ACL format
  * @ace: POSIX ACL entry to store converted ACL into
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index 998fa51f9b68..293ffe89d6b2 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -200,32 +200,6 @@ static int cifs_xattr_set(const struct xattr_handler *handler,
 		}
 		break;
 	}
-
-#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
-	case XATTR_ACL_ACCESS:
-#ifdef CONFIG_CIFS_POSIX
-		if (!value)
-			goto out;
-		if (sb->s_flags & SB_POSIXACL)
-			rc = CIFSSMBSetPosixACL(xid, pTcon, full_path,
-				value, (const int)size,
-				ACL_TYPE_ACCESS, cifs_sb->local_nls,
-				cifs_remap(cifs_sb));
-#endif  /* CONFIG_CIFS_POSIX */
-		break;
-
-	case XATTR_ACL_DEFAULT:
-#ifdef CONFIG_CIFS_POSIX
-		if (!value)
-			goto out;
-		if (sb->s_flags & SB_POSIXACL)
-			rc = CIFSSMBSetPosixACL(xid, pTcon, full_path,
-				value, (const int)size,
-				ACL_TYPE_DEFAULT, cifs_sb->local_nls,
-				cifs_remap(cifs_sb));
-#endif  /* CONFIG_CIFS_POSIX */
-		break;
-#endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
 	}
 
 out:
@@ -366,27 +340,6 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
 		}
 		break;
 	}
-#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
-	case XATTR_ACL_ACCESS:
-#ifdef CONFIG_CIFS_POSIX
-		if (sb->s_flags & SB_POSIXACL)
-			rc = CIFSSMBGetPosixACL(xid, pTcon, full_path,
-				value, size, ACL_TYPE_ACCESS,
-				cifs_sb->local_nls,
-				cifs_remap(cifs_sb));
-#endif  /* CONFIG_CIFS_POSIX */
-		break;
-
-	case XATTR_ACL_DEFAULT:
-#ifdef CONFIG_CIFS_POSIX
-		if (sb->s_flags & SB_POSIXACL)
-			rc = CIFSSMBGetPosixACL(xid, pTcon, full_path,
-				value, size, ACL_TYPE_DEFAULT,
-				cifs_sb->local_nls,
-				cifs_remap(cifs_sb));
-#endif  /* CONFIG_CIFS_POSIX */
-		break;
-#endif /* ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
 	}
 
 	/* We could add an additional check for streams ie
@@ -525,21 +478,6 @@ static const struct xattr_handler smb3_ntsd_full_xattr_handler = {
 	.set = cifs_xattr_set,
 };
 
-
-static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
-	.name = XATTR_NAME_POSIX_ACL_ACCESS,
-	.flags = XATTR_ACL_ACCESS,
-	.get = cifs_xattr_get,
-	.set = cifs_xattr_set,
-};
-
-static const struct xattr_handler cifs_posix_acl_default_xattr_handler = {
-	.name = XATTR_NAME_POSIX_ACL_DEFAULT,
-	.flags = XATTR_ACL_DEFAULT,
-	.get = cifs_xattr_get,
-	.set = cifs_xattr_set,
-};
-
 const struct xattr_handler *cifs_xattr_handlers[] = {
 	&cifs_user_xattr_handler,
 	&cifs_os2_xattr_handler,
@@ -549,7 +487,9 @@ const struct xattr_handler *cifs_xattr_handlers[] = {
 	&smb3_ntsd_xattr_handler, /* alias for above since avoiding "cifs" */
 	&cifs_cifs_ntsd_full_xattr_handler,
 	&smb3_ntsd_full_xattr_handler, /* alias for above since avoiding "cifs" */
-	&cifs_posix_acl_access_xattr_handler,
-	&cifs_posix_acl_default_xattr_handler,
+#ifdef CONFIG_XFS_POSIX_ACL
+	&posix_acl_access_xattr_handler,
+	&posix_acl_default_xattr_handler,
+#endif
 	NULL
 };
-- 
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: [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: [PATCH 04/29] cifs: implement get acl method

[Steve French]

Looks like the SMB1 Protocol operations for get/set posix ACL were
removed in the companion patch (in SMB3, POSIX ACLs have to be handled
by mapping from rich acls).  Was this intentional or did I miss
something? I didn't see the functions for sending these over the wire
for SMB1 (which does support POSIX ACLs, not just RichACLs (SMB/NTFS
ACLs))

        pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
        pSMB->InformationLevel = cpu_to_le16(SMB_SET_POSIX_ACL);

On Thu, Sep 22, 2022 at 10:20 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].
>
> In order to build a type safe posix api around get and set acl we need
> all filesystem to implement get and set acl.
>
> So far cifs wasn't able to implement get and set acl inode operations
> because it needs access to the dentry. Now that we extended the set acl
> inode operation to take a dentry argument and added a new get acl inode
> operation that takes a dentry argument we can let cifs implement get and
> set acl inode operations.
>
> This is mostly a copy and paste of the codepaths currently used in cifs'
> posix acl xattr handler. After we have fully implemented the posix acl
> api and switched the vfs over to it, the cifs specific posix acl xattr
> handler and associated code will be removed and the code duplication
> will go away.
>
> Note, until the vfs has been switched to the new posix acl api this
> patch is a non-functional change.
>
> Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
>  fs/cifs/cifsacl.c   |  63 +++++++++++++++
>  fs/cifs/cifsfs.c    |   2 +
>  fs/cifs/cifsproto.h |   6 ++
>  fs/cifs/cifssmb.c   | 190 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 261 insertions(+)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index fa480d62f313..06ae721ec1e7 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/keyctl.h>
>  #include <linux/key-type.h>
> +#include <uapi/linux/posix_acl.h>
>  #include <keys/user-type.h>
>  #include "cifspdu.h"
>  #include "cifsglob.h"
> @@ -20,6 +21,8 @@
>  #include "cifsproto.h"
>  #include "cifs_debug.h"
>  #include "fs_context.h"
> +#include "cifs_fs_sb.h"
> +#include "cifs_unicode.h"
>
>  /* security id for everyone/world system group */
>  static const struct cifs_sid sid_everyone = {
> @@ -1668,3 +1671,63 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
>         kfree(pntsd);
>         return rc;
>  }
> +
> +struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
> +                              struct dentry *dentry, int type)
> +{
> +#if defined(CONFIG_CIFS_ALLOW_INSECURE_LEGACY) && defined(CONFIG_CIFS_POSIX)
> +       struct posix_acl *acl = NULL;
> +       ssize_t rc = -EOPNOTSUPP;
> +       unsigned int xid;
> +       struct super_block *sb = dentry->d_sb;
> +       struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> +       struct tcon_link *tlink;
> +       struct cifs_tcon *pTcon;
> +       const char *full_path;
> +       void *page;
> +
> +       tlink = cifs_sb_tlink(cifs_sb);
> +       if (IS_ERR(tlink))
> +               return ERR_CAST(tlink);
> +       pTcon = tlink_tcon(tlink);
> +
> +       xid = get_xid();
> +       page = alloc_dentry_path();
> +
> +       full_path = build_path_from_dentry(dentry, page);
> +       if (IS_ERR(full_path)) {
> +               acl = ERR_CAST(full_path);
> +               goto out;
> +       }
> +
> +       /* return alt name if available as pseudo attr */
> +       switch (type) {
> +       case ACL_TYPE_ACCESS:
> +               if (sb->s_flags & SB_POSIXACL)
> +                       rc = cifs_do_get_acl(xid, pTcon, full_path, &acl,
> +                                            ACL_TYPE_ACCESS,
> +                                            cifs_sb->local_nls,
> +                                            cifs_remap(cifs_sb));
> +               break;
> +
> +       case ACL_TYPE_DEFAULT:
> +               if (sb->s_flags & SB_POSIXACL)
> +                       rc = cifs_do_get_acl(xid, pTcon, full_path, &acl,
> +                                            ACL_TYPE_DEFAULT,
> +                                            cifs_sb->local_nls,
> +                                            cifs_remap(cifs_sb));
> +               break;
> +       }
> +
> +       if (rc == -EINVAL)
> +               acl = ERR_PTR(-EOPNOTSUPP);
> +
> +out:
> +       free_dentry_path(page);
> +       free_xid(xid);
> +       cifs_put_tlink(tlink);
> +       return acl;
> +#else
> +       return ERR_PTR(-EOPNOTSUPP);
> +#endif
> +}
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index f54d8bf2732a..5c00d79fda99 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1128,6 +1128,7 @@ const struct inode_operations cifs_dir_inode_ops = {
>         .symlink = cifs_symlink,
>         .mknod   = cifs_mknod,
>         .listxattr = cifs_listxattr,
> +       .get_acl = cifs_get_acl,
>  };
>
>  const struct inode_operations cifs_file_inode_ops = {
> @@ -1136,6 +1137,7 @@ const struct inode_operations cifs_file_inode_ops = {
>         .permission = cifs_permission,
>         .listxattr = cifs_listxattr,
>         .fiemap = cifs_fiemap,
> +       .get_acl = cifs_get_acl,
>  };
>
>  const struct inode_operations cifs_symlink_inode_ops = {
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 3bc94bcc7177..953fd910da70 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -225,6 +225,8 @@ extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
>                                       const char *, u32 *, u32);
>  extern struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *,
>                                 const struct cifs_fid *, u32 *, u32);
> +extern struct posix_acl *cifs_get_acl(struct user_namespace *mnt_userns,
> +                                     struct dentry *dentry, int type);
>  extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
>                                 const char *, int);
>  extern unsigned int setup_authusers_ACE(struct cifs_ace *pace);
> @@ -542,6 +544,10 @@ extern int CIFSSMBGetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
>                 const unsigned char *searchName,
>                 char *acl_inf, const int buflen, const int acl_type,
>                 const struct nls_table *nls_codepage, int remap_special_chars);
> +extern int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
> +                          const unsigned char *searchName,
> +                          struct posix_acl **acl, const int acl_type,
> +                          const struct nls_table *nls_codepage, int remap);
>  extern int CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
>                 const unsigned char *fileName,
>                 const char *local_acl, const int buflen, const int acl_type,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 7aa91e272027..f53d2eb100ca 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -3212,6 +3212,196 @@ CIFSSMBSetPosixACL(const unsigned int xid, struct cifs_tcon *tcon,
>         return rc;
>  }
>
> +/**
> + * cifs_init_posix_acl - convert ACL from cifs to POSIX ACL format
> + * @ace: POSIX ACL entry to store converted ACL into
> + * @cifs: ACL in cifs format
> + *
> + * Convert an Access Control Entry from wire format to local POSIX xattr
> + * format.
> + *
> + * Note that the @cifs_uid member is used to store both {g,u}id_t.
> + */
> +static void cifs_init_posix_acl(struct posix_acl_entry *ace,
> +                               struct cifs_posix_ace *cifs_ace)
> +{
> +       /* u8 cifs fields do not need le conversion */
> +       ace->e_perm = cpu_to_le16(cifs_ace->cifs_e_perm);
> +       ace->e_tag  = cpu_to_le16(cifs_ace->cifs_e_tag);
> +       switch (ace->e_tag) {
> +       case ACL_USER:
> +               ace->e_uid = make_kuid(&init_user_ns,
> +                                 cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid)));
> +               break;
> +       case ACL_GROUP:
> +               ace->e_gid = make_kgid(&init_user_ns,
> +                                 cpu_to_le32(le64_to_cpu(cifs_ace->cifs_uid)));
> +               break;
> +       }
> +/*
> +       cifs_dbg(FYI, "perm %d tag %d id %d\n",
> +                ace->e_perm, ace->e_tag, ace->e_id);
> +*/
> +
> +       return;
> +}
> +
> +/**
> + * cifs_to_posix_acl - copy cifs ACL format to POSIX ACL format
> + * @acl: ACLs returned in POSIX ACL format
> + * @src: ACLs in cifs format
> + * @acl_type: type of POSIX ACL requested
> + * @size_of_data_area: size of SMB we got
> + *
> + * This function converts ACLs from cifs format to POSIX ACL format.
> + * If @acl is NULL then the size of the buffer required to store POSIX ACLs in
> + * their uapi format is returned.
> + */
> +static int cifs_to_posix_acl(struct posix_acl **acl, char *src,
> +                            const int acl_type, const int size_of_data_area)
> +{
> +       int size =  0;
> +       __u16 count;
> +       struct cifs_posix_ace *pACE;
> +       struct cifs_posix_acl *cifs_acl = (struct cifs_posix_acl *)src;
> +       struct posix_acl *kacl = NULL;
> +       struct posix_acl_entry *pa, *pe;
> +
> +       if (le16_to_cpu(cifs_acl->version) != CIFS_ACL_VERSION)
> +               return -EOPNOTSUPP;
> +
> +       if (acl_type == ACL_TYPE_ACCESS) {
> +               count = le16_to_cpu(cifs_acl->access_entry_count);
> +               pACE = &cifs_acl->ace_array[0];
> +               size = sizeof(struct cifs_posix_acl);
> +               size += sizeof(struct cifs_posix_ace) * count;
> +               /* check if we would go beyond end of SMB */
> +               if (size_of_data_area < size) {
> +                       cifs_dbg(FYI, "bad CIFS POSIX ACL size %d vs. %d\n",
> +                                size_of_data_area, size);
> +                       return -EINVAL;
> +               }
> +       } else if (acl_type == ACL_TYPE_DEFAULT) {
> +               count = le16_to_cpu(cifs_acl->access_entry_count);
> +               size = sizeof(struct cifs_posix_acl);
> +               size += sizeof(struct cifs_posix_ace) * count;
> +/* skip past access ACEs to get to default ACEs */
> +               pACE = &cifs_acl->ace_array[count];
> +               count = le16_to_cpu(cifs_acl->default_entry_count);
> +               size += sizeof(struct cifs_posix_ace) * count;
> +               /* check if we would go beyond end of SMB */
> +               if (size_of_data_area < size)
> +                       return -EINVAL;
> +       } else {
> +               /* illegal type */
> +               return -EINVAL;
> +       }
> +
> +       /* Allocate number of POSIX ACLs to store in VFS format. */
> +       kacl = posix_acl_alloc(count, GFP_NOFS);
> +       if (!kacl)
> +               return -ENOMEM;
> +
> +       FOREACH_ACL_ENTRY(pa, kacl, pe) {
> +               cifs_init_posix_acl(pa, pACE);
> +               pACE++;
> +       }
> +
> +       *acl = kacl;
> +       return 0;
> +}
> +
> +int cifs_do_get_acl(const unsigned int xid, struct cifs_tcon *tcon,
> +                   const unsigned char *searchName, struct posix_acl **acl,
> +                   const int acl_type, const struct nls_table *nls_codepage,
> +                   int remap)
> +{
> +/* SMB_QUERY_POSIX_ACL */
> +       TRANSACTION2_QPI_REQ *pSMB = NULL;
> +       TRANSACTION2_QPI_RSP *pSMBr = NULL;
> +       int rc = 0;
> +       int bytes_returned;
> +       int name_len;
> +       __u16 params, byte_count;
> +
> +       cifs_dbg(FYI, "In GetPosixACL (Unix) for path %s\n", searchName);
> +
> +queryAclRetry:
> +       rc = smb_init(SMB_COM_TRANSACTION2, 15, tcon, (void **) &pSMB,
> +               (void **) &pSMBr);
> +       if (rc)
> +               return rc;
> +
> +       if (pSMB->hdr.Flags2 & SMBFLG2_UNICODE) {
> +               name_len =
> +                       cifsConvertToUTF16((__le16 *) pSMB->FileName,
> +                                          searchName, PATH_MAX, nls_codepage,
> +                                          remap);
> +               name_len++;     /* trailing null */
> +               name_len *= 2;
> +               pSMB->FileName[name_len] = 0;
> +               pSMB->FileName[name_len+1] = 0;
> +       } else {
> +               name_len = copy_path_name(pSMB->FileName, searchName);
> +       }
> +
> +       params = 2 /* level */  + 4 /* rsrvd */  + name_len /* incl null */ ;
> +       pSMB->TotalDataCount = 0;
> +       pSMB->MaxParameterCount = cpu_to_le16(2);
> +       /* BB find exact max data count below from sess structure BB */
> +       pSMB->MaxDataCount = cpu_to_le16(4000);
> +       pSMB->MaxSetupCount = 0;
> +       pSMB->Reserved = 0;
> +       pSMB->Flags = 0;
> +       pSMB->Timeout = 0;
> +       pSMB->Reserved2 = 0;
> +       pSMB->ParameterOffset = cpu_to_le16(
> +               offsetof(struct smb_com_transaction2_qpi_req,
> +                        InformationLevel) - 4);
> +       pSMB->DataCount = 0;
> +       pSMB->DataOffset = 0;
> +       pSMB->SetupCount = 1;
> +       pSMB->Reserved3 = 0;
> +       pSMB->SubCommand = cpu_to_le16(TRANS2_QUERY_PATH_INFORMATION);
> +       byte_count = params + 1 /* pad */ ;
> +       pSMB->TotalParameterCount = cpu_to_le16(params);
> +       pSMB->ParameterCount = pSMB->TotalParameterCount;
> +       pSMB->InformationLevel = cpu_to_le16(SMB_QUERY_POSIX_ACL);
> +       pSMB->Reserved4 = 0;
> +       inc_rfc1001_len(pSMB, byte_count);
> +       pSMB->ByteCount = cpu_to_le16(byte_count);
> +
> +       rc = SendReceive(xid, tcon->ses, (struct smb_hdr *) pSMB,
> +               (struct smb_hdr *) pSMBr, &bytes_returned, 0);
> +       cifs_stats_inc(&tcon->stats.cifs_stats.num_acl_get);
> +       if (rc) {
> +               cifs_dbg(FYI, "Send error in Query POSIX ACL = %d\n", rc);
> +       } else {
> +               /* decode response */
> +
> +               rc = validate_t2((struct smb_t2_rsp *)pSMBr);
> +               /* BB also check enough total bytes returned */
> +               if (rc || get_bcc(&pSMBr->hdr) < 2)
> +                       rc = -EIO;      /* bad smb */
> +               else {
> +                       __u16 data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
> +                       __u16 count = le16_to_cpu(pSMBr->t2.DataCount);
> +                       rc = cifs_to_posix_acl(acl,
> +                               (char *)&pSMBr->hdr.Protocol+data_offset,
> +                               acl_type, count);
> +               }
> +       }
> +       cifs_buf_release(pSMB);
> +       /*
> +        * The else branch after SendReceive() doesn't return EAGAIN so if we
> +        * allocated @acl in cifs_to_posix_acl() we are guaranteed to return
> +        * here and don't leak POSIX ACLs.
> +        */
> +       if (rc == -EAGAIN)
> +               goto queryAclRetry;
> +       return rc;
> +}
> +
>  int
>  CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
>                const int netfid, __u64 *pExtAttrBits, __u64 *pMask)
> --
> 2.34.1
>


-- 
Thanks,

Steve

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 04/29] cifs: implement get acl method

[Christian Brauner]

On Thu, Sep 22, 2022 at 10:52:43PM -0500, Steve French wrote:
> Looks like the SMB1 Protocol operations for get/set posix ACL were
> removed in the companion patch (in SMB3, POSIX ACLs have to be handled

Sorry, what companion patch? Is a patch in this series or are you
referring to something else?

> by mapping from rich acls).  Was this intentional or did I miss
> something? I didn't see the functions for sending these over the wire
> for SMB1 (which does support POSIX ACLs, not just RichACLs (SMB/NTFS
> ACLs))

I'm sorry, I don't understand. This is basically a 1:1 port of what you
currently have in cifs_xattr_set() and cifs_xattr_get() under the
XATTR_ACL_DEFAULT and XATTR_ACL_ACCESS switches. So basically, the
patches in this series just add almost 1:1 copies of
CIFSSMBSetPosixACL() and CIFSSMBGetPosixACL() just that instead of
operating on void * they operate on a proper vfs struct posix acl. So
nothing would've changed behavior wise. Ofc, there's always the chance
that I missed sm especially bc I'm not a cifs developer. :)

> 
>         pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
>         pSMB->InformationLevel = cpu_to_le16(SMB_SET_POSIX_ACL);

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: [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 04/29] cifs: implement get acl method

[Steve French]

On Fri, Sep 23, 2022 at 3:38 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Sep 22, 2022 at 10:52:43PM -0500, Steve French wrote:
> > Looks like the SMB1 Protocol operations for get/set posix ACL were
> > removed in the companion patch (in SMB3, POSIX ACLs have to be handled
>
> Sorry, what companion patch? Is a patch in this series or are you
> referring to something else?

I found it - the patch order was confusing (I saw patches 4 and 27,
but patch 5 was
missed).  The functions I was asking about were deleted in patch 27 in
your series but readded in patch 5 which I had missed.

On the more general topic of POSIX ACLs:
- Note that they are supported for SMB1 (to some servers, including Samba)
- But ... almost all servers (including modern ones, not just ancient
SMB1 servers) support "RichACLs" (remember that RichACLs  were
originally based on SMB/NTFS ACLs and include deny ACEs so cover use
cases that primitive POSIX ACLs can't handle) but for cifs.ko we have
to map the local UID to a global unique ID for each ACE (ie id to SID
translation).  I am interested in the topic for how it is recommended
to map "POSIX ACLs" to "RichACLs."  I am also interested in making
sure that cifs.ko supports the recommended mechanism for exposing
"richacls" - since there are various filesystems that support RichACLs
(including NFS, cifs.ko, ntfs and presumably others) and there are
even xfstests that test richacls.


> > by mapping from rich acls).  Was this intentional or did I miss
> > something? I didn't see the functions for sending these over the wire
> > for SMB1 (which does support POSIX ACLs, not just RichACLs (SMB/NTFS
> > ACLs))
>
> I'm sorry, I don't understand. This is basically a 1:1 port of what you
> currently have in cifs_xattr_set() and cifs_xattr_get() under the
> XATTR_ACL_DEFAULT and XATTR_ACL_ACCESS switches. So basically, the
> patches in this series just add almost 1:1 copies of
> CIFSSMBSetPosixACL() and CIFSSMBGetPosixACL() just that instead of
> operating on void * they operate on a proper vfs struct posix acl. So
> nothing would've changed behavior wise. Ofc, there's always the chance
> that I missed sm especially bc I'm not a cifs developer. :)
>
> >
> >         pSMB->SubCommand = cpu_to_le16(TRANS2_SET_PATH_INFORMATION);
> >         pSMB->InformationLevel = cpu_to_le16(SMB_SET_POSIX_ACL);



-- 
Thanks,

Steve

Re: [PATCH 04/29] cifs: implement get acl method

[Christian Brauner]

On Sun, Sep 25, 2022 at 05:53:03PM -0500, Steve French wrote:
> On Fri, Sep 23, 2022 at 3:38 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Sep 22, 2022 at 10:52:43PM -0500, Steve French wrote:
> > > Looks like the SMB1 Protocol operations for get/set posix ACL were
> > > removed in the companion patch (in SMB3, POSIX ACLs have to be handled
> >
> > Sorry, what companion patch? Is a patch in this series or are you
> > referring to something else?
> 
> I found it - the patch order was confusing (I saw patches 4 and 27,
> but patch 5 was
> missed).  The functions I was asking about were deleted in patch 27 in
> your series but readded in patch 5 which I had missed.

Ok, so we should be good.

> 
> On the more general topic of POSIX ACLs:
> - Note that they are supported for SMB1 (to some servers, including Samba)
> - But ... almost all servers (including modern ones, not just ancient
> SMB1 servers) support "RichACLs" (remember that RichACLs  were
> originally based on SMB/NTFS ACLs and include deny ACEs so cover use
> cases that primitive POSIX ACLs can't handle) but for cifs.ko we have
> to map the local UID to a global unique ID for each ACE (ie id to SID
> translation).  I am interested in the topic for how it is recommended
> to map "POSIX ACLs" to "RichACLs."  I am also interested in making

I think this calls for a session during next years LSFMM but it's a bit
out of scope for this refactoring. :) But we should keep this discussion
in mind!