[PATCH 0/2] Support for posix ACLs in fuse

[PATCH 0/2] Support for posix ACLs in fuse

[Seth Forshee]

Hi Miklos,

Here's an updated set of patches for supporting posix ACLs in fuse. I
think I've incorporated all the feedback from the last RFC series, and
so I've dropped the RFC this time.

I also pushed to github the changes I made to libfuse for testing this.
They're a little rough and probably not 100% complete, but it is
sufficient for exercising the functionality of these patches with
fusexmp.

 https://github.com/sforshee/libfuse/tree/posix-acl

Changes since RFC v3:
 - Add terminating NULL element to fuse_xattr_handlers array.
 - Remove the FUSE_FS_POSIX_ACL config option and select FS_POSIX_ACL
   whenever FUSE_FS is enabled.
 - Use an INIT flag to negotiate ACL support with userspace, only when
   default_permissions is enabled.
 - Use a different set of xattr handlers when ACL support is negotiated,
   preserving the current behavior whenever ACLs are not being enforced
   by the kernel.
 - Use a PAGE_SIZE buffer initially in fuse_get_acl() and fall back to
   querying the xattr size only if that isn't large enough.
 - Remove code to keep ACLs and file mode in sync in the kernel. FUSE
   userspace will be responsible for this.

Thanks,
Seth

Seth Forshee (2):
  fuse: Use generic xattr ops
  fuse: Add posix ACL support

 fs/fuse/Kconfig           |   1 +
 fs/fuse/Makefile          |   2 +-
 fs/fuse/acl.c             | 100 ++++++++++++++++++++++++
 fs/fuse/dir.c             | 192 ++++++++--------------------------------------
 fs/fuse/fuse_i.h          |  23 ++++++
 fs/fuse/inode.c           |  10 +++
 fs/fuse/xattr.c           | 192 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fuse.h |   3 +
 8 files changed, 364 insertions(+), 159 deletions(-)
 create mode 100644 fs/fuse/acl.c
 create mode 100644 fs/fuse/xattr.c

[PATCH 1/2] fuse: Use generic xattr ops

[Seth Forshee]

In preparation for posix acl support, rework fuse to use xattr
handlers and the generic setxattr/getxattr/listxattr callbacks.
Split the xattr code out into it's own file, and promote symbols
to module-global scope as needed.

Functionally these changes have no impact, as fuse still uses a
single handler for all xattrs which uses the old callbacks.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/Makefile |   2 +-
 fs/fuse/dir.c    | 167 ++++----------------------------------------------
 fs/fuse/fuse_i.h |   5 ++
 fs/fuse/inode.c  |   1 +
 fs/fuse/xattr.c  | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 202 insertions(+), 157 deletions(-)
 create mode 100644 fs/fuse/xattr.c

diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
index e95eeb445e58..448aa27ada00 100644
--- a/fs/fuse/Makefile
+++ b/fs/fuse/Makefile
@@ -5,4 +5,4 @@
 obj-$(CONFIG_FUSE_FS) += fuse.o
 obj-$(CONFIG_CUSE) += cuse.o
 
-fuse-objs := dev.o dir.o file.o inode.o control.o
+fuse-objs := dev.o dir.o file.o inode.o control.o xattr.o
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index c47b7780ce37..7490db141dd9 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
 
 static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
 {
@@ -634,7 +635,7 @@ static int fuse_symlink(struct inode *dir, struct dentry *entry,
 	return create_new_entry(fc, &args, dir, entry, S_IFLNK);
 }
 
-static inline void fuse_update_ctime(struct inode *inode)
+void fuse_update_ctime(struct inode *inode)
 {
 	if (!IS_NOCMTIME(inode)) {
 		inode->i_ctime = current_fs_time(inode->i_sb);
@@ -1724,152 +1725,6 @@ static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
 	return fuse_update_attributes(inode, stat, NULL, NULL);
 }
 
-static int fuse_setxattr(struct dentry *unused, struct inode *inode,
-			 const char *name, const void *value,
-			 size_t size, int flags)
-{
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	FUSE_ARGS(args);
-	struct fuse_setxattr_in inarg;
-	int err;
-
-	if (fc->no_setxattr)
-		return -EOPNOTSUPP;
-
-	memset(&inarg, 0, sizeof(inarg));
-	inarg.size = size;
-	inarg.flags = flags;
-	args.in.h.opcode = FUSE_SETXATTR;
-	args.in.h.nodeid = get_node_id(inode);
-	args.in.numargs = 3;
-	args.in.args[0].size = sizeof(inarg);
-	args.in.args[0].value = &inarg;
-	args.in.args[1].size = strlen(name) + 1;
-	args.in.args[1].value = name;
-	args.in.args[2].size = size;
-	args.in.args[2].value = value;
-	err = fuse_simple_request(fc, &args);
-	if (err == -ENOSYS) {
-		fc->no_setxattr = 1;
-		err = -EOPNOTSUPP;
-	}
-	if (!err) {
-		fuse_invalidate_attr(inode);
-		fuse_update_ctime(inode);
-	}
-	return err;
-}
-
-static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
-			     const char *name, void *value, size_t size)
-{
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	FUSE_ARGS(args);
-	struct fuse_getxattr_in inarg;
-	struct fuse_getxattr_out outarg;
-	ssize_t ret;
-
-	if (fc->no_getxattr)
-		return -EOPNOTSUPP;
-
-	memset(&inarg, 0, sizeof(inarg));
-	inarg.size = size;
-	args.in.h.opcode = FUSE_GETXATTR;
-	args.in.h.nodeid = get_node_id(inode);
-	args.in.numargs = 2;
-	args.in.args[0].size = sizeof(inarg);
-	args.in.args[0].value = &inarg;
-	args.in.args[1].size = strlen(name) + 1;
-	args.in.args[1].value = name;
-	/* This is really two different operations rolled into one */
-	args.out.numargs = 1;
-	if (size) {
-		args.out.argvar = 1;
-		args.out.args[0].size = size;
-		args.out.args[0].value = value;
-	} else {
-		args.out.args[0].size = sizeof(outarg);
-		args.out.args[0].value = &outarg;
-	}
-	ret = fuse_simple_request(fc, &args);
-	if (!ret && !size)
-		ret = outarg.size;
-	if (ret == -ENOSYS) {
-		fc->no_getxattr = 1;
-		ret = -EOPNOTSUPP;
-	}
-	return ret;
-}
-
-static ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
-{
-	struct inode *inode = d_inode(entry);
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	FUSE_ARGS(args);
-	struct fuse_getxattr_in inarg;
-	struct fuse_getxattr_out outarg;
-	ssize_t ret;
-
-	if (!fuse_allow_current_process(fc))
-		return -EACCES;
-
-	if (fc->no_listxattr)
-		return -EOPNOTSUPP;
-
-	memset(&inarg, 0, sizeof(inarg));
-	inarg.size = size;
-	args.in.h.opcode = FUSE_LISTXATTR;
-	args.in.h.nodeid = get_node_id(inode);
-	args.in.numargs = 1;
-	args.in.args[0].size = sizeof(inarg);
-	args.in.args[0].value = &inarg;
-	/* This is really two different operations rolled into one */
-	args.out.numargs = 1;
-	if (size) {
-		args.out.argvar = 1;
-		args.out.args[0].size = size;
-		args.out.args[0].value = list;
-	} else {
-		args.out.args[0].size = sizeof(outarg);
-		args.out.args[0].value = &outarg;
-	}
-	ret = fuse_simple_request(fc, &args);
-	if (!ret && !size)
-		ret = outarg.size;
-	if (ret == -ENOSYS) {
-		fc->no_listxattr = 1;
-		ret = -EOPNOTSUPP;
-	}
-	return ret;
-}
-
-static int fuse_removexattr(struct dentry *entry, const char *name)
-{
-	struct inode *inode = d_inode(entry);
-	struct fuse_conn *fc = get_fuse_conn(inode);
-	FUSE_ARGS(args);
-	int err;
-
-	if (fc->no_removexattr)
-		return -EOPNOTSUPP;
-
-	args.in.h.opcode = FUSE_REMOVEXATTR;
-	args.in.h.nodeid = get_node_id(inode);
-	args.in.numargs = 1;
-	args.in.args[0].size = strlen(name) + 1;
-	args.in.args[0].value = name;
-	err = fuse_simple_request(fc, &args);
-	if (err == -ENOSYS) {
-		fc->no_removexattr = 1;
-		err = -EOPNOTSUPP;
-	}
-	if (!err) {
-		fuse_invalidate_attr(inode);
-		fuse_update_ctime(inode);
-	}
-	return err;
-}
-
 static const struct inode_operations fuse_dir_inode_operations = {
 	.lookup		= fuse_lookup,
 	.mkdir		= fuse_mkdir,
@@ -1884,10 +1739,10 @@ static const struct inode_operations fuse_dir_inode_operations = {
 	.mknod		= fuse_mknod,
 	.permission	= fuse_permission,
 	.getattr	= fuse_getattr,
-	.setxattr	= fuse_setxattr,
-	.getxattr	= fuse_getxattr,
+	.setxattr	= generic_setxattr,
+	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
-	.removexattr	= fuse_removexattr,
+	.removexattr	= generic_removexattr,
 };
 
 static const struct file_operations fuse_dir_operations = {
@@ -1905,10 +1760,10 @@ static const struct inode_operations fuse_common_inode_operations = {
 	.setattr	= fuse_setattr,
 	.permission	= fuse_permission,
 	.getattr	= fuse_getattr,
-	.setxattr	= fuse_setxattr,
-	.getxattr	= fuse_getxattr,
+	.setxattr	= generic_setxattr,
+	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
-	.removexattr	= fuse_removexattr,
+	.removexattr	= generic_removexattr,
 };
 
 static const struct inode_operations fuse_symlink_inode_operations = {
@@ -1916,10 +1771,10 @@ static const struct inode_operations fuse_symlink_inode_operations = {
 	.get_link	= fuse_get_link,
 	.readlink	= generic_readlink,
 	.getattr	= fuse_getattr,
-	.setxattr	= fuse_setxattr,
-	.getxattr	= fuse_getxattr,
+	.setxattr	= generic_setxattr,
+	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
-	.removexattr	= fuse_removexattr,
+	.removexattr	= generic_removexattr,
 };
 
 void fuse_init_common(struct inode *inode)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index d98d8cc84def..6db54d0bd81b 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -902,6 +902,8 @@ int fuse_allow_current_process(struct fuse_conn *fc);
 
 u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id);
 
+void fuse_update_ctime(struct inode *inode);
+
 int fuse_update_attributes(struct inode *inode, struct kstat *stat,
 			   struct file *file, bool *refreshed);
 
@@ -966,4 +968,7 @@ void fuse_set_initialized(struct fuse_conn *fc);
 void fuse_unlock_inode(struct inode *inode);
 void fuse_lock_inode(struct inode *inode);
 
+ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
+extern const struct xattr_handler *fuse_xattr_handlers[];
+
 #endif /* _FS_FUSE_I_H */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 4e05b51120f4..1e535f31fed0 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1071,6 +1071,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	sb->s_magic = FUSE_SUPER_MAGIC;
 	sb->s_op = &fuse_super_operations;
+	sb->s_xattr = fuse_xattr_handlers;
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_time_gran = 1;
 	sb->s_export_op = &fuse_export_operations;
diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
new file mode 100644
index 000000000000..9929cbe8cf03
--- /dev/null
+++ b/fs/fuse/xattr.c
@@ -0,0 +1,184 @@
+/*
+  FUSE: Filesystem in Userspace
+  Copyright (C) 2001-2008  Miklos Szeredi <miklos@szeredi.hu>
+
+  This program can be distributed under the terms of the GNU GPL.
+  See the file COPYING.
+*/
+
+#include "fuse_i.h"
+
+#include <linux/xattr.h>
+
+static int fuse_setxattr(struct inode *inode, const char *name,
+			 const void *value, size_t size, int flags)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	FUSE_ARGS(args);
+	struct fuse_setxattr_in inarg;
+	int err;
+
+	if (fc->no_setxattr)
+		return -EOPNOTSUPP;
+
+	memset(&inarg, 0, sizeof(inarg));
+	inarg.size = size;
+	inarg.flags = flags;
+	args.in.h.opcode = FUSE_SETXATTR;
+	args.in.h.nodeid = get_node_id(inode);
+	args.in.numargs = 3;
+	args.in.args[0].size = sizeof(inarg);
+	args.in.args[0].value = &inarg;
+	args.in.args[1].size = strlen(name) + 1;
+	args.in.args[1].value = name;
+	args.in.args[2].size = size;
+	args.in.args[2].value = value;
+	err = fuse_simple_request(fc, &args);
+	if (err == -ENOSYS) {
+		fc->no_setxattr = 1;
+		err = -EOPNOTSUPP;
+	}
+	if (!err) {
+		fuse_invalidate_attr(inode);
+		fuse_update_ctime(inode);
+	}
+	return err;
+}
+
+static ssize_t fuse_getxattr(struct inode *inode, const char *name,
+			     void *value, size_t size)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	FUSE_ARGS(args);
+	struct fuse_getxattr_in inarg;
+	struct fuse_getxattr_out outarg;
+	ssize_t ret;
+
+	if (fc->no_getxattr)
+		return -EOPNOTSUPP;
+
+	memset(&inarg, 0, sizeof(inarg));
+	inarg.size = size;
+	args.in.h.opcode = FUSE_GETXATTR;
+	args.in.h.nodeid = get_node_id(inode);
+	args.in.numargs = 2;
+	args.in.args[0].size = sizeof(inarg);
+	args.in.args[0].value = &inarg;
+	args.in.args[1].size = strlen(name) + 1;
+	args.in.args[1].value = name;
+	/* This is really two different operations rolled into one */
+	args.out.numargs = 1;
+	if (size) {
+		args.out.argvar = 1;
+		args.out.args[0].size = size;
+		args.out.args[0].value = value;
+	} else {
+		args.out.args[0].size = sizeof(outarg);
+		args.out.args[0].value = &outarg;
+	}
+	ret = fuse_simple_request(fc, &args);
+	if (!ret && !size)
+		ret = outarg.size;
+	if (ret == -ENOSYS) {
+		fc->no_getxattr = 1;
+		ret = -EOPNOTSUPP;
+	}
+	return ret;
+}
+
+ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
+{
+	struct inode *inode = d_inode(entry);
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	FUSE_ARGS(args);
+	struct fuse_getxattr_in inarg;
+	struct fuse_getxattr_out outarg;
+	ssize_t ret;
+
+	if (!fuse_allow_current_process(fc))
+		return -EACCES;
+
+	if (fc->no_listxattr)
+		return -EOPNOTSUPP;
+
+	memset(&inarg, 0, sizeof(inarg));
+	inarg.size = size;
+	args.in.h.opcode = FUSE_LISTXATTR;
+	args.in.h.nodeid = get_node_id(inode);
+	args.in.numargs = 1;
+	args.in.args[0].size = sizeof(inarg);
+	args.in.args[0].value = &inarg;
+	/* This is really two different operations rolled into one */
+	args.out.numargs = 1;
+	if (size) {
+		args.out.argvar = 1;
+		args.out.args[0].size = size;
+		args.out.args[0].value = list;
+	} else {
+		args.out.args[0].size = sizeof(outarg);
+		args.out.args[0].value = &outarg;
+	}
+	ret = fuse_simple_request(fc, &args);
+	if (!ret && !size)
+		ret = outarg.size;
+	if (ret == -ENOSYS) {
+		fc->no_listxattr = 1;
+		ret = -EOPNOTSUPP;
+	}
+	return ret;
+}
+
+static int fuse_removexattr(struct inode *inode, const char *name)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	FUSE_ARGS(args);
+	int err;
+
+	if (fc->no_removexattr)
+		return -EOPNOTSUPP;
+
+	args.in.h.opcode = FUSE_REMOVEXATTR;
+	args.in.h.nodeid = get_node_id(inode);
+	args.in.numargs = 1;
+	args.in.args[0].size = strlen(name) + 1;
+	args.in.args[0].value = name;
+	err = fuse_simple_request(fc, &args);
+	if (err == -ENOSYS) {
+		fc->no_removexattr = 1;
+		err = -EOPNOTSUPP;
+	}
+	if (!err) {
+		fuse_invalidate_attr(inode);
+		fuse_update_ctime(inode);
+	}
+	return err;
+}
+
+static int fuse_xattr_get(const struct xattr_handler *handler,
+			 struct dentry *dentry, struct inode *inode,
+			 const char *name, void *value, size_t size)
+{
+	return fuse_getxattr(inode, name, value, size);
+}
+
+static int fuse_xattr_set(const struct xattr_handler *handler,
+			  struct dentry *dentry, struct inode *inode,
+			  const char *name, const void *value, size_t size,
+			  int flags)
+{
+	if (!value)
+		return fuse_removexattr(inode, name);
+
+	return fuse_setxattr(inode, name, value, size, flags);
+}
+
+static const struct xattr_handler fuse_xattr_handler = {
+	.prefix = "",
+	.get    = fuse_xattr_get,
+	.set    = fuse_xattr_set,
+};
+
+const struct xattr_handler *fuse_xattr_handlers[] = {
+	&fuse_xattr_handler,
+	NULL
+};
-- 
2.7.4

[PATCH 2/2] fuse: Add posix ACL support

[Seth Forshee]

Add a new INIT flag, FUSE_POSIX_ACL, for negotiating ACL support
with userspace. When default_permissions is enabled the kernel
will set this flag in FUSE_INIT, and if it is also set in the
response ACL support will be enabled.

When ACL support is enabled, the kernel will cache and have
responsibility for enforcing ACLs. ACL xattrs will be passed to
userspace, which is responsible for updating the ACLs in the
filesystem, keeping the file mode in sync, and inheritance of
default ACLs when new filesystem nodes are created.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/Kconfig           |   1 +
 fs/fuse/Makefile          |   2 +-
 fs/fuse/acl.c             | 100 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/fuse/dir.c             |  25 +++++++++++-
 fs/fuse/fuse_i.h          |  18 +++++++++
 fs/fuse/inode.c           |   9 +++++
 fs/fuse/xattr.c           |  18 ++++++---
 include/uapi/linux/fuse.h |   3 ++
 8 files changed, 168 insertions(+), 8 deletions(-)
 create mode 100644 fs/fuse/acl.c

diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
index 1b2f6c2c3aaf..76f09ce7e5b2 100644
--- a/fs/fuse/Kconfig
+++ b/fs/fuse/Kconfig
@@ -1,5 +1,6 @@
 config FUSE_FS
 	tristate "FUSE (Filesystem in Userspace) support"
+	select FS_POSIX_ACL
 	help
 	  With FUSE it is possible to implement a fully functional filesystem
 	  in a userspace program.
diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile
index 448aa27ada00..60da84a86dab 100644
--- a/fs/fuse/Makefile
+++ b/fs/fuse/Makefile
@@ -5,4 +5,4 @@
 obj-$(CONFIG_FUSE_FS) += fuse.o
 obj-$(CONFIG_CUSE) += cuse.o
 
-fuse-objs := dev.o dir.o file.o inode.o control.o xattr.o
+fuse-objs := dev.o dir.o file.o inode.o control.o xattr.o acl.o
diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
new file mode 100644
index 000000000000..87220da95e91
--- /dev/null
+++ b/fs/fuse/acl.c
@@ -0,0 +1,100 @@
+/*
+  FUSE: Filesystem in Userspace
+  Copyright (C) 2016 Canonical Ltd. <seth.forshee@canonical.com>
+
+  This program can be distributed under the terms of the GNU GPL.
+  See the file COPYING.
+*/
+
+#include "fuse_i.h"
+
+#include <linux/posix_acl.h>
+#include <linux/posix_acl_xattr.h>
+
+struct posix_acl *fuse_get_acl(struct inode *inode, int type)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	int size;
+	const char *name;
+	void *value = NULL;
+	struct posix_acl *acl;
+
+	if (!fc->posix_acl || fc->no_getxattr)
+		return NULL;
+
+	if (type == ACL_TYPE_ACCESS)
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
+	else if (type == ACL_TYPE_DEFAULT)
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
+	else
+		return ERR_PTR(-EOPNOTSUPP);
+
+	value = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!value)
+		return ERR_PTR(-ENOMEM);
+	size = fuse_getxattr(inode, name, value, PAGE_SIZE);
+	if (size == -ERANGE) {
+		kfree(value);
+		size = fuse_getxattr(inode, name, NULL, 0);
+		value = kmalloc(size, GFP_KERNEL);
+		if (!value)
+			return ERR_PTR(-ENOMEM);
+		size = fuse_getxattr(inode, name, value, size);
+	}
+
+	if (size > 0)
+		acl = posix_acl_from_xattr(&init_user_ns, value, size);
+	else if ((size == 0) || (size == -ENODATA) ||
+		 (size == -EOPNOTSUPP && fc->no_getxattr))
+		acl = NULL;
+	else
+		acl = ERR_PTR(size);
+
+	kfree(value);
+	return acl;
+}
+
+int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	const char *name;
+	int ret;
+
+	if (!fc->posix_acl || fc->no_setxattr)
+		return -EOPNOTSUPP;
+
+	if (type == ACL_TYPE_ACCESS)
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
+	else if (type == ACL_TYPE_DEFAULT)
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
+	else
+		return -EINVAL;
+
+	if (acl) {
+		/*
+		 * Fuse userspace is responsible for updating access
+		 * permissions in the inode, if needed. fuse_setxattr
+		 * invalidates the inode attributes, which will force
+		 * them to be refreshed the next time they are used,
+		 * and it also updates i_ctime.
+		 */
+		size_t size = posix_acl_xattr_size(acl->a_count);
+		void *value = kmalloc(size, GFP_KERNEL);
+		if (!value)
+			return -ENOMEM;
+
+		ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
+		if (ret < 0) {
+			kfree(value);
+			return ret;
+		}
+
+		ret = fuse_setxattr(inode, name, value, size, 0);
+		kfree(value);
+	} else {
+		ret = fuse_removexattr(inode, name);
+	}
+	if (ret == 0)
+		set_cached_acl(inode, type, acl);
+	return ret;
+}
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 7490db141dd9..d25e00cfe72d 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -14,6 +14,7 @@
 #include <linux/namei.h>
 #include <linux/slab.h>
 #include <linux/xattr.h>
+#include <linux/posix_acl.h>
 
 static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
 {
@@ -244,6 +245,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 		if (ret || (outarg.attr.mode ^ inode->i_mode) & S_IFMT)
 			goto invalid;
 
+		forget_all_cached_acls(inode);
 		fuse_change_attributes(inode, &outarg.attr,
 				       entry_attr_timeout(&outarg),
 				       attr_version);
@@ -918,6 +920,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
 
 	if (time_before64(fi->i_time, get_jiffies_64())) {
 		r = true;
+		forget_all_cached_acls(inode);
 		err = fuse_do_getattr(inode, stat, file);
 	} else {
 		r = false;
@@ -1065,6 +1068,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
 	if (mask & MAY_NOT_BLOCK)
 		return -ECHILD;
 
+	forget_all_cached_acls(inode);
 	return fuse_do_getattr(inode, NULL, NULL);
 }
 
@@ -1234,6 +1238,7 @@ retry:
 		fi->nlookup++;
 		spin_unlock(&fc->lock);
 
+		forget_all_cached_acls(inode);
 		fuse_change_attributes(inode, &o->attr,
 				       entry_attr_timeout(o),
 				       attr_version);
@@ -1703,14 +1708,24 @@ error:
 static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 {
 	struct inode *inode = d_inode(entry);
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	int ret;
 
 	if (!fuse_allow_current_process(get_fuse_conn(inode)))
 		return -EACCES;
 
 	if (attr->ia_valid & ATTR_FILE)
-		return fuse_do_setattr(inode, attr, attr->ia_file);
+		ret = fuse_do_setattr(inode, attr, attr->ia_file);
 	else
-		return fuse_do_setattr(inode, attr, NULL);
+		ret = fuse_do_setattr(inode, attr, NULL);
+
+	/*
+	 * If filesystem supports acls it may have updated acl xattrs in
+	 * the filesystem, so forget cached acls for the inode.
+	 */
+	if (!ret && fc->posix_acl)
+		forget_all_cached_acls(inode);
+	return ret;
 }
 
 static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
@@ -1743,6 +1758,8 @@ static const struct inode_operations fuse_dir_inode_operations = {
 	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
 	.removexattr	= generic_removexattr,
+	.get_acl	= fuse_get_acl,
+	.set_acl	= fuse_set_acl,
 };
 
 static const struct file_operations fuse_dir_operations = {
@@ -1764,6 +1781,8 @@ static const struct inode_operations fuse_common_inode_operations = {
 	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
 	.removexattr	= generic_removexattr,
+	.get_acl	= fuse_get_acl,
+	.set_acl	= fuse_set_acl,
 };
 
 static const struct inode_operations fuse_symlink_inode_operations = {
@@ -1775,6 +1794,8 @@ static const struct inode_operations fuse_symlink_inode_operations = {
 	.getxattr	= generic_getxattr,
 	.listxattr	= fuse_listxattr,
 	.removexattr	= generic_removexattr,
+	.get_acl	= fuse_get_acl,
+	.set_acl	= fuse_set_acl,
 };
 
 void fuse_init_common(struct inode *inode)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 6db54d0bd81b..5d8a6bf6594d 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -23,6 +23,7 @@
 #include <linux/poll.h>
 #include <linux/workqueue.h>
 #include <linux/kref.h>
+#include <linux/xattr.h>
 
 /** Max number of pages that can be used in a single read request */
 #define FUSE_MAX_PAGES_PER_REQ 32
@@ -624,6 +625,9 @@ struct fuse_conn {
 	/** Is lseek not implemented by fs? */
 	unsigned no_lseek:1;
 
+	/** Does the filesystem support posix acls? */
+	unsigned posix_acl:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
@@ -968,7 +972,21 @@ void fuse_set_initialized(struct fuse_conn *fc);
 void fuse_unlock_inode(struct inode *inode);
 void fuse_lock_inode(struct inode *inode);
 
+int fuse_setxattr(struct inode *inode, const char *name, const void *value,
+		  size_t size, int flags);
+ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
+		      size_t size);
 ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
+int fuse_removexattr(struct inode *inode, const char *name);
 extern const struct xattr_handler *fuse_xattr_handlers[];
+extern const struct xattr_handler *fuse_acl_xattr_handlers[];
+
+struct posix_acl;
+
+extern const struct xattr_handler fuse_xattr_acl_access_handler;
+extern const struct xattr_handler fuse_xattr_acl_default_handler;
+
+struct posix_acl *fuse_get_acl(struct inode *inode, int type);
+int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type);
 
 #endif /* _FS_FUSE_I_H */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 1e535f31fed0..a3cfcb541243 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -20,6 +20,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/exportfs.h>
+#include <linux/posix_acl.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -340,6 +341,7 @@ int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
 		return -ENOENT;
 
 	fuse_invalidate_attr(inode);
+	forget_all_cached_acls(inode);
 	if (offset >= 0) {
 		pg_start = offset >> PAGE_SHIFT;
 		if (len <= 0)
@@ -912,6 +914,11 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 				fc->parallel_dirops = 1;
 			if (arg->time_gran && arg->time_gran <= 1000000000)
 				fc->sb->s_time_gran = arg->time_gran;
+			if ((arg->flags & FUSE_POSIX_ACL) &&
+			    (fc->flags & FUSE_DEFAULT_PERMISSIONS)) {
+				fc->posix_acl = 1;
+				fc->sb->s_xattr = fuse_acl_xattr_handlers;
+			}
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -942,6 +949,8 @@ static void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
 		FUSE_DO_READDIRPLUS | FUSE_READDIRPLUS_AUTO | FUSE_ASYNC_DIO |
 		FUSE_WRITEBACK_CACHE | FUSE_NO_OPEN_SUPPORT |
 		FUSE_PARALLEL_DIROPS;
+	if (fc->flags & FUSE_DEFAULT_PERMISSIONS)
+		arg->flags |= FUSE_POSIX_ACL;
 	req->in.h.opcode = FUSE_INIT;
 	req->in.numargs = 1;
 	req->in.args[0].size = sizeof(*arg);
diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
index 9929cbe8cf03..855e701f8c11 100644
--- a/fs/fuse/xattr.c
+++ b/fs/fuse/xattr.c
@@ -9,9 +9,10 @@
 #include "fuse_i.h"
 
 #include <linux/xattr.h>
+#include <linux/posix_acl_xattr.h>
 
-static int fuse_setxattr(struct inode *inode, const char *name,
-			 const void *value, size_t size, int flags)
+int fuse_setxattr(struct inode *inode, const char *name, const void *value,
+		  size_t size, int flags)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	FUSE_ARGS(args);
@@ -45,8 +46,8 @@ static int fuse_setxattr(struct inode *inode, const char *name,
 	return err;
 }
 
-static ssize_t fuse_getxattr(struct inode *inode, const char *name,
-			     void *value, size_t size)
+ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
+		      size_t size)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	FUSE_ARGS(args);
@@ -128,7 +129,7 @@ ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
 	return ret;
 }
 
-static int fuse_removexattr(struct inode *inode, const char *name)
+int fuse_removexattr(struct inode *inode, const char *name)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	FUSE_ARGS(args);
@@ -182,3 +183,10 @@ const struct xattr_handler *fuse_xattr_handlers[] = {
 	&fuse_xattr_handler,
 	NULL
 };
+
+const struct xattr_handler *fuse_acl_xattr_handlers[] = {
+	&posix_acl_access_xattr_handler,
+	&posix_acl_default_xattr_handler,
+	&fuse_xattr_handler,
+	NULL
+};
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 27e17363263a..cb48ec4546ce 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -108,6 +108,7 @@
  *
  *  7.25
  *  - add FUSE_PARALLEL_DIROPS
+ *  - add FUSE_POSIX_ACL
  */
 
 #ifndef _LINUX_FUSE_H
@@ -238,6 +239,7 @@ struct fuse_file_lock {
  * FUSE_WRITEBACK_CACHE: use writeback cache for buffered writes
  * FUSE_NO_OPEN_SUPPORT: kernel supports zero-message opens
  * FUSE_PARALLEL_DIROPS: allow parallel lookups and readdir
+ * FUSE_POSIX_ACL: filesystem supports posix acls
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -258,6 +260,7 @@ struct fuse_file_lock {
 #define FUSE_WRITEBACK_CACHE	(1 << 16)
 #define FUSE_NO_OPEN_SUPPORT	(1 << 17)
 #define FUSE_PARALLEL_DIROPS    (1 << 18)
+#define FUSE_POSIX_ACL		(1 << 19)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.7.4

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Nikolaus Rath]

Hi Seth,

On Aug 29 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
> Hi Miklos,
>
> Here's an updated set of patches for supporting posix ACLs in fuse. I
> think I've incorporated all the feedback from the last RFC series, and
> so I've dropped the RFC this time.
>
> I also pushed to github the changes I made to libfuse for testing this.
> They're a little rough and probably not 100% complete, but it is
> sufficient for exercising the functionality of these patches with
> fusexmp.
>
>  https://github.com/sforshee/libfuse/tree/posix-acl

I think it would be great to finally have this in FUSE. Thanks for
working on this!

Would you mind submitting a pull request for the userspace changes to
libfuse as well (via GitHub)?

Best,
-Nikolaus

-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

             »Time flies like an arrow, fruit flies like a Banana.«

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Seth Forshee]

On Tue, Sep 06, 2016 at 08:32:21PM -0700, Nikolaus Rath wrote:
> Hi Seth,
> 
> On Aug 29 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
> > Hi Miklos,
> >
> > Here's an updated set of patches for supporting posix ACLs in fuse. I
> > think I've incorporated all the feedback from the last RFC series, and
> > so I've dropped the RFC this time.
> >
> > I also pushed to github the changes I made to libfuse for testing this.
> > They're a little rough and probably not 100% complete, but it is
> > sufficient for exercising the functionality of these patches with
> > fusexmp.
> >
> >  https://github.com/sforshee/libfuse/tree/posix-acl
> 
> I think it would be great to finally have this in FUSE. Thanks for
> working on this!
> 
> Would you mind submitting a pull request for the userspace changes to
> libfuse as well (via GitHub)?

I can, I'll just have to find some time first to make sure I've covered
everything and to clean it all up.

Seth

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Miklos Szeredi]

[Adding Andreas Gruenbacher to Cc]

On Mon, Aug 29, 2016 at 3:46 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> Hi Miklos,
>
> Here's an updated set of patches for supporting posix ACLs in fuse. I
> think I've incorporated all the feedback from the last RFC series, and
> so I've dropped the RFC this time.

Pushed, with minor changes, to

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next

Please verify that I didn't break it.

> I also pushed to github the changes I made to libfuse for testing this.
> They're a little rough and probably not 100% complete, but it is
> sufficient for exercising the functionality of these patches with
> fusexmp.
>
>  https://github.com/sforshee/libfuse/tree/posix-acl

As for the libfuse part:

1) Please don't mess with fusexmp.c.  The added code is really an
anti-example.  Posix acls will will work fine in such pass-through
filesystems without doing anything.  The added complexity just makes
it brittle and racy without actually doing anything positive.

2) You define some constants and structures (POSIX_ACL_*) in
fuse_common.h that don't seem to belong there.  There's <sys/acl.h>
that contains some parts of that, but I'm not sure how much we want to
tie libfuse to libacl...  It's a difficult thing.  Generally I'd try
to keep the interface as narrow as possible.  Perhaps it's enough to
have a a function to return the equivalent mode from the xattr?

3) How will richacl's fit into this?

4) We really need a better example to check the efficiency of the new
interface, but that's hard because we need a "real" filesystem for
that and those are rare.  Ntfs-3g is one such, and it would be
interesting to "port" it to using the new API.

Jean-Pierre, how difficult would that be?

Thanks,
Miklos

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Jean-Pierre André]

Hi,

Miklos Szeredi wrote:
> [Adding Andreas Gruenbacher to Cc]
>
> On Mon, Aug 29, 2016 at 3:46 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
>> Hi Miklos,
>>
>> Here's an updated set of patches for supporting posix ACLs in fuse. I
>> think I've incorporated all the feedback from the last RFC series, and
>> so I've dropped the RFC this time.
>
> Pushed, with minor changes, to
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
>
> Please verify that I didn't break it.
>
>> I also pushed to github the changes I made to libfuse for testing this.
>> They're a little rough and probably not 100% complete, but it is
>> sufficient for exercising the functionality of these patches with
>> fusexmp.
>>
>>   https://github.com/sforshee/libfuse/tree/posix-acl
>
> As for the libfuse part:
>
> 1) Please don't mess with fusexmp.c.  The added code is really an
> anti-example.  Posix acls will will work fine in such pass-through
> filesystems without doing anything.  The added complexity just makes
> it brittle and racy without actually doing anything positive.
>
> 2) You define some constants and structures (POSIX_ACL_*) in
> fuse_common.h that don't seem to belong there.  There's <sys/acl.h>
> that contains some parts of that, but I'm not sure how much we want to
> tie libfuse to libacl...  It's a difficult thing.  Generally I'd try
> to keep the interface as narrow as possible.  Perhaps it's enough to
> have a a function to return the equivalent mode from the xattr?
>
> 3) How will richacl's fit into this?
>
> 4) We really need a better example to check the efficiency of the new
> interface, but that's hard because we need a "real" filesystem for
> that and those are rare.  Ntfs-3g is one such, and it would be
> interesting to "port" it to using the new API.
>
> Jean-Pierre, how difficult would that be?

The code for Posix ACLs support within the kernel is
already present in ntfs-3g (just have to define the
macro KERNELACLS in order to disable the checks
within ntfs-3g and rely on the kernel ones).

This however relies on assumptions I made a few years
back, and might be invalid now. I will at least have
to set some flag in the init callback.

The ACLs are supposed to be set and retrieved through
the extended attributes system.posix_acl_access and
system.posix_acl_default. Recent posts about it in
this list mention "xattr handlers", do they mean
getxattr() and setxattr() on these extended attributes ?

Also the sync with the file mode is done natively,
as the ACLs and modes are translated to a single
object (an NTFS ACL). There is no need for fuse to
duplicate the mode to a ACL setting or conversely.

Now the main problem for ntfs-3g is to migrate to
libfuse3, still keeping the compatibility with non-Linux
implementations (MacOSX, OpenIndiana, and others).
I have not done anything about it yet.

Does the Posix ACL in-kernel support require libfuse3 ?

Finally, I am still using an old kernel, so I will
have to setup a test environment...

Jean-Pierre

>
> Thanks,
> Miklos
>

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Miklos Szeredi]

On Wed, Sep 21, 2016 at 2:25 PM, Jean-Pierre André
<jean-pierre.andre@wanadoo.fr> wrote:

> The code for Posix ACLs support within the kernel is
> already present in ntfs-3g (just have to define the
> macro KERNELACLS in order to disable the checks
> within ntfs-3g and rely on the kernel ones).
>
> This however relies on assumptions I made a few years
> back, and might be invalid now. I will at least have
> to set some flag in the init callback.
>
> The ACLs are supposed to be set and retrieved through
> the extended attributes system.posix_acl_access and
> system.posix_acl_default. Recent posts about it in
> this list mention "xattr handlers", do they mean
> getxattr() and setxattr() on these extended attributes ?

Right.

> Also the sync with the file mode is done natively,
> as the ACLs and modes are translated to a single
> object (an NTFS ACL). There is no need for fuse to
> duplicate the mode to a ACL setting or conversely.
>
> Now the main problem for ntfs-3g is to migrate to
> libfuse3, still keeping the compatibility with non-Linux
> implementations (MacOSX, OpenIndiana, and others).
> I have not done anything about it yet.
>
> Does the Posix ACL in-kernel support require libfuse3 ?

Not really, the most important change (and apparently the only one
that ntfs-3g cares about) will be the added capability flag, but it's
trivial to add it to libfuse2.

Thanks,
Miklos

Re: [fuse-devel] [PATCH 0/2] Support for posix ACLs in fuse

[Michael Theall]

On Wed, 2016-09-21 at 16:14 +0200, Miklos Szeredi wrote:
> On Wed, Sep 21, 2016 at 2:25 PM, Jean-Pierre André
> <jean-pierre.andre@wanadoo.fr> wrote:
> 
> > 
> > The code for Posix ACLs support within the kernel is
> > already present in ntfs-3g (just have to define the
> > macro KERNELACLS in order to disable the checks
> > within ntfs-3g and rely on the kernel ones).
> > 
> > This however relies on assumptions I made a few years
> > back, and might be invalid now. I will at least have
> > to set some flag in the init callback.
> > 
> > The ACLs are supposed to be set and retrieved through
> > the extended attributes system.posix_acl_access and
> > system.posix_acl_default. Recent posts about it in
> > this list mention "xattr handlers", do they mean
> > getxattr() and setxattr() on these extended attributes ?
> Right.
> 
> > 
> > Also the sync with the file mode is done natively,
> > as the ACLs and modes are translated to a single
> > object (an NTFS ACL). There is no need for fuse to
> > duplicate the mode to a ACL setting or conversely.
> > 
> > Now the main problem for ntfs-3g is to migrate to
> > libfuse3, still keeping the compatibility with non-Linux
> > implementations (MacOSX, OpenIndiana, and others).
> > I have not done anything about it yet.
> > 
> > Does the Posix ACL in-kernel support require libfuse3 ?
> Not really, the most important change (and apparently the only one
> that ntfs-3g cares about) will be the added capability flag, but it's
> trivial to add it to libfuse2.
> 
> Thanks,
> Miklos

Hi,

I'm currently trying to port my code to take advantage of this. I've
found myself in sort of a catch-22 situation: I don't know whether to
pass default_permissions or not.

Right now, I support ACLs by not using default_permissions. However,
the FUSE_POSIX_ACL capability is only given when default_permissions is
turned on. If I unconditionally set FUSE_POSIX_ACL in the "want" flags,
the capability remains disabled if default_permissions is off.

I need to support ACLs on backlevel platforms with default_permissions
off, and the coming platforms with default_permissions on. How do you
suggest I determine whether to provide it or not?

Regards,
Michael Theall

Re: [fuse-devel] [PATCH 0/2] Support for posix ACLs in fuse

[Miklos Szeredi]

On Wed, Sep 21, 2016 at 10:50 PM, Michael Theall
<mtheall@linux.vnet.ibm.com> wrote:

> I'm currently trying to port my code to take advantage of this. I've
> found myself in sort of a catch-22 situation: I don't know whether to
> pass default_permissions or not.
>
> Right now, I support ACLs by not using default_permissions. However,
> the FUSE_POSIX_ACL capability is only given when default_permissions is
> turned on. If I unconditionally set FUSE_POSIX_ACL in the "want" flags,
> the capability remains disabled if default_permissions is off.
>
> I need to support ACLs on backlevel platforms with default_permissions
> off, and the coming platforms with default_permissions on. How do you
> suggest I determine whether to provide it or not?

Good point.   FUSE_POSIX_ACL needs to be made independent of (but
implying) default_permissions.

Force pushed to fuse.git#for-next with other minor changes.

Thanks,
Miklos

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Seth Forshee]

On Wed, Sep 21, 2016 at 10:30:14AM +0200, Miklos Szeredi wrote:
> [Adding Andreas Gruenbacher to Cc]
> 
> On Mon, Aug 29, 2016 at 3:46 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > Hi Miklos,
> >
> > Here's an updated set of patches for supporting posix ACLs in fuse. I
> > think I've incorporated all the feedback from the last RFC series, and
> > so I've dropped the RFC this time.
> 
> Pushed, with minor changes, to
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
> 
> Please verify that I didn't break it.

I've reviewed the changes and they seem okay, still need to test.

> > I also pushed to github the changes I made to libfuse for testing this.
> > They're a little rough and probably not 100% complete, but it is
> > sufficient for exercising the functionality of these patches with
> > fusexmp.
> >
> >  https://github.com/sforshee/libfuse/tree/posix-acl
> 
> As for the libfuse part:
> 
> 1) Please don't mess with fusexmp.c.  The added code is really an
> anti-example.  Posix acls will will work fine in such pass-through
> filesystems without doing anything.  The added complexity just makes
> it brittle and racy without actually doing anything positive.

As you note below, it's hard to find a "real" filesystem to test it
with so fusexmp proved convenient for that. But I'll omit it when I
update the pull req.

> 2) You define some constants and structures (POSIX_ACL_*) in
> fuse_common.h that don't seem to belong there.  There's <sys/acl.h>
> that contains some parts of that, but I'm not sure how much we want to
> tie libfuse to libacl...  It's a difficult thing.  Generally I'd try
> to keep the interface as narrow as possible.  Perhaps it's enough to
> have a a function to return the equivalent mode from the xattr?

To be honest I only really meant that to serve as an example of all the
stuff that would need to happen in userspace based on the kernel
implementation. Looking now at libacl I guess it could just be expected
that filesystems will use that. It seems to provide the essentials to do
what I did with fusexmp at least, even an interface for getting the
equivalent mode (acl_equiv_mode). Not sure how well it works if e.g. a
filesystem needs to convert between the posix ACL format and some
different format native to that filesystem.

> 3) How will richacl's fit into this?

I don't know, I haven't looked at those patches closely, but in git I'm
not seeing any support for richacls in fuse yet anyhow.

Thanks,
Seth

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Miklos Szeredi]

On Wed, Sep 21, 2016 at 3:41 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> On Wed, Sep 21, 2016 at 10:30:14AM +0200, Miklos Szeredi wrote:

>> 2) You define some constants and structures (POSIX_ACL_*) in
>> fuse_common.h that don't seem to belong there.  There's <sys/acl.h>
>> that contains some parts of that, but I'm not sure how much we want to
>> tie libfuse to libacl...  It's a difficult thing.  Generally I'd try
>> to keep the interface as narrow as possible.  Perhaps it's enough to
>> have a a function to return the equivalent mode from the xattr?
>
> To be honest I only really meant that to serve as an example of all the
> stuff that would need to happen in userspace based on the kernel
> implementation. Looking now at libacl I guess it could just be expected
> that filesystems will use that. It seems to provide the essentials to do
> what I did with fusexmp at least, even an interface for getting the
> equivalent mode (acl_equiv_mode). Not sure how well it works if e.g. a
> filesystem needs to convert between the posix ACL format and some
> different format native to that filesystem.

The kernel structures and functions are quite good for converting to
native formats (it's simple and easy to encode/decode).

So we can use that instead of having to link to libacl, which does
seem a not a bit too abstracted.

But we should at least try to use the standard names for the constants.

Thanks,
Miklos

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Seth Forshee]

On Wed, Sep 21, 2016 at 08:41:56AM -0500, Seth Forshee wrote:
> On Wed, Sep 21, 2016 at 10:30:14AM +0200, Miklos Szeredi wrote:
> > [Adding Andreas Gruenbacher to Cc]
> > 
> > On Mon, Aug 29, 2016 at 3:46 PM, Seth Forshee
> > <seth.forshee@canonical.com> wrote:
> > > Hi Miklos,
> > >
> > > Here's an updated set of patches for supporting posix ACLs in fuse. I
> > > think I've incorporated all the feedback from the last RFC series, and
> > > so I've dropped the RFC this time.
> > 
> > Pushed, with minor changes, to
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
> > 
> > Please verify that I didn't break it.
> 
> I've reviewed the changes and they seem okay, still need to test.

Sorry, I got busy and it slipped my mind that I still needed to test.
Everything seems to be working fine with your for-next branch.

Thanks,
Seth

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:
> 3) How will richacl's fit into this?

As best as I can read the situation richacl support has not yet been
merged into Linux yet.

Last I was following the richacl discussion there were some fundamental
features of richacls that were incompatible with the expectations of
ordinary linux applications.  The negative acls if my memory serves.
That raised some concern if richacls could ever be safely be merged.

As I recall Christoph Hellwig was a primary on raising those concerns,
and when I read those arguments they seemed persuasive to me.

That said richacls (if they happen) will communicate to the filesystem
primarily with extended attributes just like the acls, and flag will
need to be added to the protocol to enable richacl support in the kernel
if and when that exists.

Or in short richacls are the same tune but different dance partners.

Eric

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Andreas Grünbacher]

2016-09-21 17:40 GMT+02:00 Eric W. Biederman <ebiederm@xmission.com>:
> Miklos Szeredi <miklos@szeredi.hu> writes:
>> 3) How will richacl's fit into this?
>
> As best as I can read the situation richacl support has not yet been
> merged into Linux yet.

True. The patches are stuck in Al Viro's inbox since a very long time.

> Last I was following the richacl discussion there were some fundamental
> features of richacls that were incompatible with the expectations of
> ordinary linux applications.

Not true.

> The negative acls if my memory serves.
> That raised some concern if richacls could ever be safely be merged.
>
> As I recall Christoph Hellwig was a primary on raising those concerns,
> and when I read those arguments they seemed persuasive to me.

The whole point of Richacls is to be compatible with the POSIX file
permission model. Entries that deny permissions are a necessary part
of Richacls for various reasons. Christoph doesn't like them, but that
doesn't make them any less necessary.

Andreas

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Eric W. Biederman]

Andreas Grünbacher <andreas.gruenbacher@gmail.com> writes:

> 2016-09-21 17:40 GMT+02:00 Eric W. Biederman <ebiederm@xmission.com>:
>> Miklos Szeredi <miklos@szeredi.hu> writes:
>>> 3) How will richacl's fit into this?
>>
>> As best as I can read the situation richacl support has not yet been
>> merged into Linux yet.
>
> True. The patches are stuck in Al Viro's inbox since a very long time.
>
>> Last I was following the richacl discussion there were some fundamental
>> features of richacls that were incompatible with the expectations of
>> ordinary linux applications.
>
> Not true.
>
>> The negative acls if my memory serves.
>> That raised some concern if richacls could ever be safely be merged.
>>
>> As I recall Christoph Hellwig was a primary on raising those concerns,
>> and when I read those arguments they seemed persuasive to me.
>
> The whole point of Richacls is to be compatible with the POSIX file
> permission model. Entries that deny permissions are a necessary part
> of Richacls for various reasons. Christoph doesn't like them, but that
> doesn't make them any less necessary.

Negative access control permissions are extremly nasty, and cause a lot
of problems in the real world, especially where they have not existed
before.   And sometimes where they are rare enough to not have existed
before.

I am would prefer you taking a measured approach rather than ignoring
people's concerns.

I certainly don't need any kind of ACLs for any kind of files I have
ever dealt with in practice.  I deal with ACLs and make certain the
kernel supports them well to ensure there are not nasty bugs waiting
in the wings.

If my memory serves there are very funny corner cases between negative
uids ACLs and user namespaces.  The kind that can lead to security
vulnerabilities etc.

The practical problem I would envision is users using user namespaces to
enable them to change their uid or gid and that in turn would result in
negative acls failing to contain deny users access to files.

So I would be very careful with adding negative permission checks that
are going to be (at best) very error prone to use to the current unix
permission model.

Eric

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Jeremy Allison]

On Wed, Sep 21, 2016 at 12:42:41PM -0500, Eric W. Biederman wrote:
> 
> I certainly don't need any kind of ACLs for any kind of files I have
> ever dealt with in practice...

<SARCASM FILTER ON>
Oh well, if *you* don't need them then job done !

I don't need iSCSI device support on my home systems
either, can we remove that code as well please ?
</SARCASM FILTER OFF>

RichACLs (or their NFSv4 equivalent) are an essential
feature for Linux enterprise storage.

You might not need or use them, but there
is a great demand for this feature in enterprise
storage applications.

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Andreas Grünbacher]

2016-09-21 19:42 GMT+02:00 Eric W. Biederman <ebiederm@xmission.com>:
> Andreas Grünbacher <andreas.gruenbacher@gmail.com> writes:
>
>> 2016-09-21 17:40 GMT+02:00 Eric W. Biederman <ebiederm@xmission.com>:
>>> Miklos Szeredi <miklos@szeredi.hu> writes:
>>>> 3) How will richacl's fit into this?
>>>
>>> As best as I can read the situation richacl support has not yet been
>>> merged into Linux yet.
>>
>> True. The patches are stuck in Al Viro's inbox since a very long time.
>>
>>> Last I was following the richacl discussion there were some fundamental
>>> features of richacls that were incompatible with the expectations of
>>> ordinary linux applications.
>>
>> Not true.
>>
>>> The negative acls if my memory serves.
>>> That raised some concern if richacls could ever be safely be merged.
>>>
>>> As I recall Christoph Hellwig was a primary on raising those concerns,
>>> and when I read those arguments they seemed persuasive to me.
>>
>> The whole point of Richacls is to be compatible with the POSIX file
>> permission model. Entries that deny permissions are a necessary part
>> of Richacls for various reasons. Christoph doesn't like them, but that
>> doesn't make them any less necessary.
>
> Negative access control permissions are extremly nasty, and cause a lot
> of problems in the real world, especially where they have not existed
> before.   And sometimes where they are rare enough to not have existed
> before.

Not sure what you're trying to say with that last sentence. But
anyway, let me cure you from the myth that "negative permmissions"
don't exist in the traditional UNIX permission model: when the owner
is assigned fewer permissions than the owning group (for example, mode
044), or the owning group is assigned fewer permissions than others
(for example, mode 604), particular users have fewer permissions than
others by virtue of their identity. That property is just expressed
differently in the traditional UNIX permission model and in Richacls.

By the way, here is Christoph's complaint that you're probably
referring to, and my reply:

  http://oss.sgi.com/archives/xfs/2016-03/msg00207.html
  http://oss.sgi.com/archives/xfs/2016-03/msg00219.html

There was no further follow-up by Christoph.

> I am would prefer you taking a measured approach rather than ignoring
> people's concerns.

I've long thought about this, and documented and argued why removing
deny ACEs wouldn't make sense. If you think otherwise, come up with
convincing technical arguments.

> I certainly don't need any kind of ACLs for any kind of files I have
> ever dealt with in practice.  I deal with ACLs and make certain the
> kernel supports them well to ensure there are not nasty bugs waiting
> in the wings.
>
> If my memory serves there are very funny corner cases between negative
> uids ACLs and user namespaces.  The kind that can lead to security
> vulnerabilities etc.
>
> The practical problem I would envision is users using user namespaces to
> enable them to change their uid or gid and that in turn would result in
> negative acls failing to contain deny users access to files.
>
> So I would be very careful with adding negative permission checks that
> are going to be (at best) very error prone to use to the current unix
> permission model.

See above for how it's possible to shoot yourself in the foot in that
way with namespaces already.

Andreas

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Andreas Grünbacher]

2016-09-21 10:30 GMT+02:00 Miklos Szeredi <miklos@szeredi.hu>:
> [Adding Andreas Gruenbacher to Cc]
>
> On Mon, Aug 29, 2016 at 3:46 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
>> Hi Miklos,
>>
>> Here's an updated set of patches for supporting posix ACLs in fuse. I
>> think I've incorporated all the feedback from the last RFC series, and
>> so I've dropped the RFC this time.
>
> Pushed, with minor changes, to
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
>
> Please verify that I didn't break it.
>
>> I also pushed to github the changes I made to libfuse for testing this.
>> They're a little rough and probably not 100% complete, but it is
>> sufficient for exercising the functionality of these patches with
>> fusexmp.
>>
>>  https://github.com/sforshee/libfuse/tree/posix-acl
>
> As for the libfuse part:
>
> 1) Please don't mess with fusexmp.c.  The added code is really an
> anti-example.  Posix acls will will work fine in such pass-through
> filesystems without doing anything.  The added complexity just makes
> it brittle and racy without actually doing anything positive.
>
> 2) You define some constants and structures (POSIX_ACL_*) in
> fuse_common.h that don't seem to belong there.  There's <sys/acl.h>
> that contains some parts of that, but I'm not sure how much we want to
> tie libfuse to libacl...  It's a difficult thing.  Generally I'd try
> to keep the interface as narrow as possible.  Perhaps it's enough to
> have a a function to return the equivalent mode from the xattr?

Using definitions from <sys/acl.h> doesn't necessarily create a
runtime dependency on libacl.

What does fuse actually need to do with POSIX ACLs? I understand that
setxattr("system.posix_acl_access") updates both the access ACL and
the file mode, and so does chmod. Both these operations can be
implemented quite easily based in the xattr value though; I wouldn't
choose to use libacl for that.

> 3) How will richacl's fit into this?

Richacls work similar to POSIX ACLs from fuse's point of view. The
algorithms for updating a richacl upon a chmod and for computing the
new file mode from setxattr("system.richacl") are significantly more
complicated though; also, librichacl isn't as over engineered as
libacl, so I would use librichacl for these operations.

> 4) We really need a better example to check the efficiency of the new
> interface, but that's hard because we need a "real" filesystem for
> that and those are rare.  Ntfs-3g is one such, and it would be
> interesting to "port" it to using the new API.

NTFS would be a good fit for supporting Richacls as well, obviously.

Thanks,
Andreas

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Miklos Szeredi]

On Wed, Sep 21, 2016 at 11:28 PM, Andreas Grünbacher
<andreas.gruenbacher@gmail.com> wrote:

> Using definitions from <sys/acl.h> doesn't necessarily create a
> runtime dependency on libacl.
>
> What does fuse actually need to do with POSIX ACLs? I understand that
> setxattr("system.posix_acl_access") updates both the access ACL and
> the file mode, and so does chmod. Both these operations can be
> implemented quite easily based in the xattr value though; I wouldn't
> choose to use libacl for that.

The xattr representation of posix acls is linux specific as far as I
understand.  By passing this representation to fuse filesystems it
becomes part of the fuse protocol.  This doesn't necessarily mean that
we have to actually decode the acl in the fuse library, but it needs
to be documented at least.

BTW, I find it strange that the xattr representation of posix acls are
not in any uapi header files.

>
>> 3) How will richacl's fit into this?
>
> Richacls work similar to POSIX ACLs from fuse's point of view. The
> algorithms for updating a richacl upon a chmod and for computing the
> new file mode from setxattr("system.richacl") are significantly more
> complicated though; also, librichacl isn't as over engineered as
> libacl, so I would use librichacl for these operations.

Does librichacl have xattr->acl and acl->xattr conversion functions?
Because libacl seems to lack these...

Thanks,
Miklos

Re: [PATCH 0/2] Support for posix ACLs in fuse

[Andreas Grünbacher]

2016-09-23 11:01 GMT+02:00 Miklos Szeredi <miklos@szeredi.hu>:
> On Wed, Sep 21, 2016 at 11:28 PM, Andreas Grünbacher
> <andreas.gruenbacher@gmail.com> wrote:
>
>> Using definitions from <sys/acl.h> doesn't necessarily create a
>> runtime dependency on libacl.
>>
>> What does fuse actually need to do with POSIX ACLs? I understand that
>> setxattr("system.posix_acl_access") updates both the access ACL and
>> the file mode, and so does chmod. Both these operations can be
>> implemented quite easily based in the xattr value though; I wouldn't
>> choose to use libacl for that.
>
> The xattr representation of posix acls is linux specific as far as I
> understand.  By passing this representation to fuse filesystems it
> becomes part of the fuse protocol.  This doesn't necessarily mean that
> we have to actually decode the acl in the fuse library, but it needs
> to be documented at least.
>
> BTW, I find it strange that the xattr representation of posix acls are
> not in any uapi header files.

Yes, that should be moved. Those headers are quite a bit older than
the uapi split.

>>> 3) How will richacl's fit into this?
>>
>> Richacls work similar to POSIX ACLs from fuse's point of view. The
>> algorithms for updating a richacl upon a chmod and for computing the
>> new file mode from setxattr("system.richacl") are significantly more
>> complicated though; also, librichacl isn't as over engineered as
>> libacl, so I would use librichacl for these operations.
>
> Does librichacl have xattr->acl and acl->xattr conversion functions?

Yes, for things like ceph and glusterfs.

> Because libacl seems to lack these...

In libacl, those functions are internal to the library. We could start
exporting them, but the in-memory ACL representation isn't exactly
great for many use cases, like fuse probably.

Andreas