[RFC v3 0/2] Support for posix acls in fuse

[RFC v3 0/2] Support for posix acls in fuse

[Seth Forshee]

I have some updates from the pervious RFC patch for adding posix acl
support into fuse. Here's a high-level summary of the changes:

 - Split out changes to use xattr handlers into a separate patch.
 - Split out xattr and acl code into their own files, as dir.c was
   becoming rather long.
 - Add a CONFIG_FUSE_FS_POSIX_ACL config option.
 - Add some pieces that were missing for default acls.
 - Bug fixes.
 - Remove passthrough of acl xattrs when fuse acl support is disabled or
   default_permissions is not used.

This last change is user visible, but as fuse filesystems cannot
meaninfully support acls today it's not really a regression.

There's also a problem with default acls that I'm not sure there's
currently a solution for. As far as I can tell FUSE_CREATE doesn't give
back any indication of whether an existing file was opened or a new file
was created. Without knowing that I cannot know whether or not the inode
should inherit default acls from its parent. Is there any way I can tell
whether or not FUSE_CREATE created a new object?

Thanks,
Seth

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

 fs/fuse/Kconfig  |  13 ++++
 fs/fuse/Makefile |   2 +-
 fs/fuse/acl.c    | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/fuse/dir.c    | 195 ++++++++++-------------------------------------------
 fs/fuse/fuse_i.h |  34 ++++++++++
 fs/fuse/inode.c  |   3 +
 fs/fuse/xattr.c  | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 474 insertions(+), 159 deletions(-)
 create mode 100644 fs/fuse/acl.c
 create mode 100644 fs/fuse/xattr.c

[RFC v3 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  | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 201 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 8466e122ee62..9df55b8e776a 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)
 {
@@ -632,7 +633,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);
@@ -1719,152 +1720,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,
@@ -1879,10 +1734,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 = {
@@ -1900,10 +1755,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 = {
@@ -1911,10 +1766,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 9f4c3c82edd6..93a5e8191da1 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -903,6 +903,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);
 
@@ -964,4 +966,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
 
 void fuse_set_initialized(struct fuse_conn *fc);
 
+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 254f1944ee98..fee06e48157d 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1066,6 +1066,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..d30e99610217
--- /dev/null
+++ b/fs/fuse/xattr.c
@@ -0,0 +1,183 @@
+/*
+  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,
+};
-- 
2.7.4

Re: [RFC v3 1/2] fuse: Use generic xattr ops

[Miklos Szeredi]

On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> 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.

One comment at the very bottom of the patch.

Otherwise it looks okay.

Thanks,
Miklos

>
> 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  | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 201 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 8466e122ee62..9df55b8e776a 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)
>  {
> @@ -632,7 +633,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);
> @@ -1719,152 +1720,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,
> @@ -1879,10 +1734,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 = {
> @@ -1900,10 +1755,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 = {
> @@ -1911,10 +1766,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 9f4c3c82edd6..93a5e8191da1 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -903,6 +903,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);
>
> @@ -964,4 +966,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>
>  void fuse_set_initialized(struct fuse_conn *fc);
>
> +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 254f1944ee98..fee06e48157d 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1066,6 +1066,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..d30e99610217
> --- /dev/null
> +++ b/fs/fuse/xattr.c
> @@ -0,0 +1,183 @@
> +/*
> +  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,

Please terminate array with NULL, despite the catch-all prefix.  The
less subtle the better.

Thanks,
Miklos

Re: [RFC v3 1/2] fuse: Use generic xattr ops

[Seth Forshee]

On Thu, Aug 04, 2016 at 01:09:58PM +0200, Miklos Szeredi wrote:
> > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > +       &fuse_xattr_handler,
> 
> Please terminate array with NULL, despite the catch-all prefix.  The
> less subtle the better.

Will do, thanks for the review.

Seth

[RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
and default_permissions is used for the filesystem. When
default_permissions is not used fuse cannot meaninfully support
cals, as fuse_permission() only sends FUSE_PERMISSION from the
access, faccessat, chdir, fchdir, and chroot system calls.
Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
default_permissions is not used fuse will return -EOPNOTSUPP
whenever posix acl xattrs are read or written.

XXX: Default acls are currently broken for files created via
atomic_open.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/fuse/Kconfig  |  13 ++++
 fs/fuse/Makefile |   2 +-
 fs/fuse/acl.c    | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/fuse/dir.c    |  28 +++++++-
 fs/fuse/fuse_i.h |  29 ++++++++
 fs/fuse/inode.c  |   2 +
 fs/fuse/xattr.c  |  13 ++--
 7 files changed, 279 insertions(+), 8 deletions(-)
 create mode 100644 fs/fuse/acl.c

diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
index 1b2f6c2c3aaf..5d4ebb0cc0dc 100644
--- a/fs/fuse/Kconfig
+++ b/fs/fuse/Kconfig
@@ -16,6 +16,19 @@ config FUSE_FS
 	  If you want to develop a userspace FS, or if you want to use
 	  a filesystem based on FUSE, answer Y or M.
 
+config FUSE_FS_POSIX_ACL
+        bool "Fuse POSIX Access Control Lists"
+        depends on FUSE_FS
+        select FS_POSIX_ACL
+        help
+          POSIX Access Control Lists (ACLs) support permissions for users and
+          groups beyond the owner/group/world scheme.
+
+          To learn more about Access Control Lists, visit the POSIX ACLs for
+          Linux website <http://acl.bestbits.at/>.
+
+          If you don't know what Access Control Lists are, say N
+
 config CUSE
 	tristate "Character device in Userspace support"
 	depends on FUSE_FS
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..c0f412dfe9a7
--- /dev/null
+++ b/fs/fuse/acl.c
@@ -0,0 +1,200 @@
+/*
+  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>
+
+#ifdef CONFIG_FUSE_FS_POSIX_ACL
+
+static int fuse_xattr_acl_get(const struct xattr_handler *handler,
+			      struct dentry *dentry, struct inode *inode,
+			      const char *name, void *value, size_t size)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
+		if (handler->flags == ACL_TYPE_ACCESS)
+			return posix_acl_access_xattr_handler.get(
+					&posix_acl_access_xattr_handler,
+					dentry, inode, name, value, size);
+		if (handler->flags == ACL_TYPE_DEFAULT)
+			return posix_acl_default_xattr_handler.get(
+					&posix_acl_default_xattr_handler,
+					dentry, inode, name, value, size);
+	}
+	return -EOPNOTSUPP;
+}
+
+static int fuse_xattr_acl_set(const struct xattr_handler *handler,
+				struct dentry *dentry, struct inode *inode,
+				const char *name, const void *value, size_t size,
+				int flags)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+
+	if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
+		if (handler->flags == ACL_TYPE_ACCESS)
+			return posix_acl_access_xattr_handler.set(
+					&posix_acl_access_xattr_handler,
+					dentry, inode, name, value, size,
+					flags);
+		if (handler->flags == ACL_TYPE_DEFAULT)
+			return posix_acl_default_xattr_handler.set(
+					&posix_acl_default_xattr_handler,
+					dentry, inode, name, value, size,
+					flags);
+	}
+	return -EOPNOTSUPP;
+}
+
+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->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);
+
+	size = fuse_getxattr(inode, name, NULL, 0);
+	if (size > 0) {
+		value = kzalloc(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->no_setxattr)
+		return -EOPNOTSUPP;
+
+	if (type == ACL_TYPE_ACCESS) {
+		struct iattr attr;
+		name = XATTR_NAME_POSIX_ACL_ACCESS;
+		attr.ia_mode = inode->i_mode;
+		ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			acl = NULL;
+		if (inode->i_mode != attr.ia_mode) {
+			attr.ia_valid = ATTR_MODE | ATTR_CTIME;
+			attr.ia_ctime = current_fs_time(inode->i_sb);
+			ret = fuse_do_setattr(inode, &attr, NULL);
+			if (ret)
+				return ret;
+		}
+	} else if (type == ACL_TYPE_DEFAULT) {
+		name = XATTR_NAME_POSIX_ACL_DEFAULT;
+	} else {
+		return -EINVAL;
+	}
+
+	if (acl) {
+		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;
+}
+
+int fuse_init_acl(struct inode *inode, struct inode *dir)
+{
+	struct posix_acl *default_acl, *acl;
+	int err;
+
+	err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
+	if (err)
+		return err;
+
+	if (default_acl) {
+		err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
+		posix_acl_release(default_acl);
+	}
+	if (acl) {
+		if (!err)
+			err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS);
+		posix_acl_release(default_acl);
+	}
+	return err;
+}
+
+#else /* !CONFIG_FUSE_FS_POSIX_ACL */
+
+static int fuse_xattr_acl_get(const struct xattr_handler *handler,
+			      struct dentry *dentry, struct inode *inode,
+			      const char *name, void *value, size_t size)
+{
+	return -EOPNOTSUPP;
+}
+
+static int fuse_xattr_acl_set(const struct xattr_handler *handler,
+				struct dentry *dentry, struct inode *inode,
+				const char *name, const void *value, size_t size,
+				int flags)
+{
+	return -EOPNOTSUPP;
+}
+
+#endif /* CONFIG_FUSE_FS_POSIX_ACL */
+
+const struct xattr_handler fuse_xattr_acl_access_handler = {
+	.name	= XATTR_NAME_POSIX_ACL_ACCESS,
+	.flags	= ACL_TYPE_ACCESS,
+	.get	= fuse_xattr_acl_get,
+	.set	= fuse_xattr_acl_set,
+};
+
+const struct xattr_handler fuse_xattr_acl_default_handler = {
+	.name	= XATTR_NAME_POSIX_ACL_DEFAULT,
+	.flags	= ACL_TYPE_DEFAULT,
+	.get	= fuse_xattr_acl_get,
+	.set	= fuse_xattr_acl_set,
+};
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 9df55b8e776a..ca7d573f3121 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);
@@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args,
 	}
 	kfree(forget);
 
+	if (args->in.h.opcode != FUSE_LINK) {
+		err = fuse_init_acl(inode, dir);
+		if (err) {
+			iput(inode);
+			return err;
+		}
+	}
+
 	err = d_instantiate_no_diralias(entry, inode);
 	if (err)
 		return err;
@@ -916,6 +926,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;
@@ -1062,6 +1073,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);
 }
 
@@ -1231,6 +1243,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);
@@ -1698,14 +1711,19 @@ error:
 static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 {
 	struct inode *inode = d_inode(entry);
+	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 (!ret && (attr->ia_valid & ATTR_MODE))
+		ret = posix_acl_chmod(inode, inode->i_mode);
+	return ret;
 }
 
 static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
@@ -1738,6 +1756,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 = {
@@ -1759,6 +1779,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 = {
@@ -1770,6 +1792,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 93a5e8191da1..c1a630bb2b21 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -25,6 +25,7 @@
 #include <linux/kref.h>
 #include <linux/pid_namespace.h>
 #include <linux/user_namespace.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
@@ -966,7 +967,35 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
 
 void fuse_set_initialized(struct fuse_conn *fc);
 
+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[];
 
+struct posix_acl;
+
+extern const struct xattr_handler fuse_xattr_acl_access_handler;
+extern const struct xattr_handler fuse_xattr_acl_default_handler;
+
+#ifdef CONFIG_FUSE_FS_POSIX_ACL
+
+struct posix_acl *fuse_get_acl(struct inode *inode, int type);
+int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type);
+int fuse_init_acl(struct inode *inode, struct inode *dir);
+
+#else
+
+#define fuse_get_acl NULL
+#define fuse_set_acl NULL
+
+static inline int fuse_init_acl(struct inode *inode, struct inode *dir)
+{
+	return 0;
+}
+
+#endif
+
 #endif /* _FS_FUSE_I_H */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fee06e48157d..9c1519c269bb 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -21,6 +21,7 @@
 #include <linux/sched.h>
 #include <linux/exportfs.h>
 #include <linux/pid_namespace.h>
+#include <linux/posix_acl.h>
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
 MODULE_DESCRIPTION("Filesystem in Userspace");
@@ -339,6 +340,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)
diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
index d30e99610217..d87d58112eb1 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);
@@ -179,5 +180,7 @@ static const struct xattr_handler fuse_xattr_handler = {
 };
 
 const struct xattr_handler *fuse_xattr_handlers[] = {
+	&fuse_xattr_acl_access_handler,
+	&fuse_xattr_acl_default_handler,
 	&fuse_xattr_handler,
 };
-- 
2.7.4

Re: [RFC v3 2/2] fuse: Add posix acl support

[Miklos Szeredi]

On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
> and default_permissions is used for the filesystem. When
> default_permissions is not used fuse cannot meaninfully support
> cals, as fuse_permission() only sends FUSE_PERMISSION from the
> access, faccessat, chdir, fchdir, and chroot system calls.
> Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
> default_permissions is not used fuse will return -EOPNOTSUPP
> whenever posix acl xattrs are read or written.

Which is breaking backward compatibilty.  Please don't do this without
good reason.

Even having "default_permissions" change behavior might cause
problems.  I'd suggest adding an INIT flag to indicate whether the
filesystem wants acl checking or not.  Feature negotiation with the
INIT flag is good for another reason:  the filesystem can detect
whether this feature is available and act differently based on that.

More comments inline.

Thanks,
Miklos

>
> XXX: Default acls are currently broken for files created via
> atomic_open.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  fs/fuse/Kconfig  |  13 ++++
>  fs/fuse/Makefile |   2 +-
>  fs/fuse/acl.c    | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/dir.c    |  28 +++++++-
>  fs/fuse/fuse_i.h |  29 ++++++++
>  fs/fuse/inode.c  |   2 +
>  fs/fuse/xattr.c  |  13 ++--
>  7 files changed, 279 insertions(+), 8 deletions(-)
>  create mode 100644 fs/fuse/acl.c
>
> diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
> index 1b2f6c2c3aaf..5d4ebb0cc0dc 100644
> --- a/fs/fuse/Kconfig
> +++ b/fs/fuse/Kconfig
> @@ -16,6 +16,19 @@ config FUSE_FS
>           If you want to develop a userspace FS, or if you want to use
>           a filesystem based on FUSE, answer Y or M.
>
> +config FUSE_FS_POSIX_ACL
> +        bool "Fuse POSIX Access Control Lists"
> +        depends on FUSE_FS
> +        select FS_POSIX_ACL
> +        help
> +          POSIX Access Control Lists (ACLs) support permissions for users and
> +          groups beyond the owner/group/world scheme.
> +
> +          To learn more about Access Control Lists, visit the POSIX ACLs for
> +          Linux website <http://acl.bestbits.at/>.
> +
> +          If you don't know what Access Control Lists are, say N
> +
>  config CUSE
>         tristate "Character device in Userspace support"
>         depends on FUSE_FS
> 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..c0f412dfe9a7
> --- /dev/null
> +++ b/fs/fuse/acl.c
> @@ -0,0 +1,200 @@
> +/*
> +  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>
> +
> +#ifdef CONFIG_FUSE_FS_POSIX_ACL
> +
> +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> +                             struct dentry *dentry, struct inode *inode,
> +                             const char *name, void *value, size_t size)
> +{
> +       struct fuse_conn *fc = get_fuse_conn(inode);
> +
> +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> +               if (handler->flags == ACL_TYPE_ACCESS)
> +                       return posix_acl_access_xattr_handler.get(
> +                                       &posix_acl_access_xattr_handler,
> +                                       dentry, inode, name, value, size);
> +               if (handler->flags == ACL_TYPE_DEFAULT)
> +                       return posix_acl_default_xattr_handler.get(
> +                                       &posix_acl_default_xattr_handler,
> +                                       dentry, inode, name, value, size);
> +       }
> +       return -EOPNOTSUPP;
> +}
> +
> +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> +                               struct dentry *dentry, struct inode *inode,
> +                               const char *name, const void *value, size_t size,
> +                               int flags)
> +{
> +       struct fuse_conn *fc = get_fuse_conn(inode);
> +
> +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> +               if (handler->flags == ACL_TYPE_ACCESS)
> +                       return posix_acl_access_xattr_handler.set(
> +                                       &posix_acl_access_xattr_handler,
> +                                       dentry, inode, name, value, size,
> +                                       flags);
> +               if (handler->flags == ACL_TYPE_DEFAULT)
> +                       return posix_acl_default_xattr_handler.set(
> +                                       &posix_acl_default_xattr_handler,
> +                                       dentry, inode, name, value, size,
> +                                       flags);
> +       }
> +       return -EOPNOTSUPP;
> +}

The above should go away.  As I said, getxattr() and setxattr()
shouldn't behave differently depending on whether
"default_permissions" is set or not.

> +
> +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->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);
> +
> +       size = fuse_getxattr(inode, name, NULL, 0);
> +       if (size > 0) {
> +               value = kzalloc(size, GFP_KERNEL);
> +               if (!value)
> +                       return ERR_PTR(-ENOMEM);
> +               size = fuse_getxattr(inode, name, value, size);

Can we optimize away the first getxattr?  Starting with a page size
buffer and falling back to the two calls only if ERANGE is returned.

> +       }
> +       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->no_setxattr)
> +               return -EOPNOTSUPP;
> +
> +       if (type == ACL_TYPE_ACCESS) {
> +               struct iattr attr;
> +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> +               attr.ia_mode = inode->i_mode;
> +               ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
> +               if (ret < 0)
> +                       return ret;
> +               if (ret == 0)
> +                       acl = NULL;
> +               if (inode->i_mode != attr.ia_mode) {
> +                       attr.ia_valid = ATTR_MODE | ATTR_CTIME;
> +                       attr.ia_ctime = current_fs_time(inode->i_sb);
> +                       ret = fuse_do_setattr(inode, &attr, NULL);
> +                       if (ret)
> +                               return ret;
> +               }
> +       } else if (type == ACL_TYPE_DEFAULT) {
> +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       if (acl) {
> +               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;
> +}

I wonder if splitting setxattr into a setattr + setxattr isn't going
to cause problems.  By doing this userspace filesystem can no longer
guarantee atomicity of the update.

Maybe we just need to introduce a new operation that can do chmod+setxattr?

> +
> +int fuse_init_acl(struct inode *inode, struct inode *dir)
> +{
> +       struct posix_acl *default_acl, *acl;
> +       int err;
> +
> +       err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +       if (err)
> +               return err;
> +
> +       if (default_acl) {
> +               err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +               posix_acl_release(default_acl);
> +       }
> +       if (acl) {
> +               if (!err)
> +                       err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +               posix_acl_release(default_acl);
> +       }
> +       return err;
> +}
> +
> +#else /* !CONFIG_FUSE_FS_POSIX_ACL */
> +
> +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> +                             struct dentry *dentry, struct inode *inode,
> +                             const char *name, void *value, size_t size)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> +                               struct dentry *dentry, struct inode *inode,
> +                               const char *name, const void *value, size_t size,
> +                               int flags)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +#endif /* CONFIG_FUSE_FS_POSIX_ACL */
> +
> +const struct xattr_handler fuse_xattr_acl_access_handler = {
> +       .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> +       .flags  = ACL_TYPE_ACCESS,
> +       .get    = fuse_xattr_acl_get,
> +       .set    = fuse_xattr_acl_set,
> +};
> +
> +const struct xattr_handler fuse_xattr_acl_default_handler = {
> +       .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> +       .flags  = ACL_TYPE_DEFAULT,
> +       .get    = fuse_xattr_acl_get,
> +       .set    = fuse_xattr_acl_set,
> +};
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 9df55b8e776a..ca7d573f3121 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);
> @@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args,
>         }
>         kfree(forget);
>
> +       if (args->in.h.opcode != FUSE_LINK) {
> +               err = fuse_init_acl(inode, dir);

Again, atomicity problems.

> +               if (err) {
> +                       iput(inode);
> +                       return err;
> +               }
> +       }
> +
>         err = d_instantiate_no_diralias(entry, inode);
>         if (err)
>                 return err;
> @@ -916,6 +926,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;
> @@ -1062,6 +1073,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);
>  }
>
> @@ -1231,6 +1243,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);
> @@ -1698,14 +1711,19 @@ error:
>  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
>  {
>         struct inode *inode = d_inode(entry);
> +       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 (!ret && (attr->ia_valid & ATTR_MODE))
> +               ret = posix_acl_chmod(inode, inode->i_mode);

And again.

I'm really wondering if it's simpler to just add an xattr parser to
libfuse and do these at the filesystem level.  That would simplify
this patchset a lot:

Reduce the scope to just permission checking, which is what we can do
best and fastest in the kernel.  And leave the rest to userspace.
They don't have performance impact, but trying to push this into the
kernel is just asking for trouble.

> +       return ret;
>  }
>
>  static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
> @@ -1738,6 +1756,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 = {
> @@ -1759,6 +1779,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 = {
> @@ -1770,6 +1792,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 93a5e8191da1..c1a630bb2b21 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -25,6 +25,7 @@
>  #include <linux/kref.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/user_namespace.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
> @@ -966,7 +967,35 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>
>  void fuse_set_initialized(struct fuse_conn *fc);
>
> +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[];
>
> +struct posix_acl;
> +
> +extern const struct xattr_handler fuse_xattr_acl_access_handler;
> +extern const struct xattr_handler fuse_xattr_acl_default_handler;
> +
> +#ifdef CONFIG_FUSE_FS_POSIX_ACL
> +
> +struct posix_acl *fuse_get_acl(struct inode *inode, int type);
> +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> +int fuse_init_acl(struct inode *inode, struct inode *dir);
> +
> +#else
> +
> +#define fuse_get_acl NULL
> +#define fuse_set_acl NULL
> +
> +static inline int fuse_init_acl(struct inode *inode, struct inode *dir)
> +{
> +       return 0;
> +}
> +
> +#endif
> +
>  #endif /* _FS_FUSE_I_H */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index fee06e48157d..9c1519c269bb 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -21,6 +21,7 @@
>  #include <linux/sched.h>
>  #include <linux/exportfs.h>
>  #include <linux/pid_namespace.h>
> +#include <linux/posix_acl.h>
>
>  MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
>  MODULE_DESCRIPTION("Filesystem in Userspace");
> @@ -339,6 +340,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)
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index d30e99610217..d87d58112eb1 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);
> @@ -179,5 +180,7 @@ static const struct xattr_handler fuse_xattr_handler = {
>  };
>
>  const struct xattr_handler *fuse_xattr_handlers[] = {
> +       &fuse_xattr_acl_access_handler,
> +       &fuse_xattr_acl_default_handler,
>         &fuse_xattr_handler,
>  };
> --
> 2.7.4
>

Re: [RFC v3 2/2] fuse: Add posix acl support

[Ravishankar N]

[-- Attachment #1.1: Type: text/plain, Size: 1091 bytes --]

On 08/04/2016 05:41 PM, Miklos Szeredi wrote:
> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
> <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>  wrote:
>> >Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
>> >and default_permissions is used for the filesystem. When
>> >default_permissions is not used fuse cannot meaninfully support
>> >cals, as fuse_permission() only sends FUSE_PERMISSION from the
>> >access, faccessat, chdir, fchdir, and chroot system calls.
>> >Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
>> >default_permissions is not used fuse will return -EOPNOTSUPP
>> >whenever posix acl xattrs are read or written.
> Which is breaking backward compatibilty.  Please don't do this without
> good reason.
>
> Even having "default_permissions" change behavior might cause
> problems.
+1. Not much of an acl expert but gluster turns off default_permissions 
[1] to support posix acls [2].

[1] 
https://github.com/gluster/glusterfs/blob/master/xlators/mount/fuse/src/fuse-bridge.c#L5696
[2] https://sourceforge.net/p/fuse/mailman/message/33015624/

-Ravi

[-- Attachment #1.2: Type: text/html, Size: 2372 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 119 bytes --]

-- 
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel

Re: [RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:
> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
> > and default_permissions is used for the filesystem. When
> > default_permissions is not used fuse cannot meaninfully support
> > cals, as fuse_permission() only sends FUSE_PERMISSION from the
> > access, faccessat, chdir, fchdir, and chroot system calls.
> > Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
> > default_permissions is not used fuse will return -EOPNOTSUPP
> > whenever posix acl xattrs are read or written.
> 
> Which is breaking backward compatibilty.  Please don't do this without
> good reason.
> 
> Even having "default_permissions" change behavior might cause
> problems.  I'd suggest adding an INIT flag to indicate whether the
> filesystem wants acl checking or not.  Feature negotiation with the
> INIT flag is good for another reason:  the filesystem can detect
> whether this feature is available and act differently based on that.

This backwards compatibility is a bit of a mystery to me. I've seen
claims of fuse filesystems supporting acls internally, but in practice I
can't see how this can possibly work since fuse_permission() only sends
a FUSE_ACCESS request if !default_permissions and one of MAY_ACCESS or
MAY_CHDIR is set.

Of course it is also a change to not pass through the xattr even if it
isn't being enforced. I'm going to defer to Eric on this point, as he
has some broader-reaching ideas about this.

> 
> More comments inline.
> 
> Thanks,
> Miklos
> 
> >
> > XXX: Default acls are currently broken for files created via
> > atomic_open.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  fs/fuse/Kconfig  |  13 ++++
> >  fs/fuse/Makefile |   2 +-
> >  fs/fuse/acl.c    | 200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/dir.c    |  28 +++++++-
> >  fs/fuse/fuse_i.h |  29 ++++++++
> >  fs/fuse/inode.c  |   2 +
> >  fs/fuse/xattr.c  |  13 ++--
> >  7 files changed, 279 insertions(+), 8 deletions(-)
> >  create mode 100644 fs/fuse/acl.c
> >
> > diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
> > index 1b2f6c2c3aaf..5d4ebb0cc0dc 100644
> > --- a/fs/fuse/Kconfig
> > +++ b/fs/fuse/Kconfig
> > @@ -16,6 +16,19 @@ config FUSE_FS
> >           If you want to develop a userspace FS, or if you want to use
> >           a filesystem based on FUSE, answer Y or M.
> >
> > +config FUSE_FS_POSIX_ACL
> > +        bool "Fuse POSIX Access Control Lists"
> > +        depends on FUSE_FS
> > +        select FS_POSIX_ACL
> > +        help
> > +          POSIX Access Control Lists (ACLs) support permissions for users and
> > +          groups beyond the owner/group/world scheme.
> > +
> > +          To learn more about Access Control Lists, visit the POSIX ACLs for
> > +          Linux website <http://acl.bestbits.at/>.
> > +
> > +          If you don't know what Access Control Lists are, say N
> > +
> >  config CUSE
> >         tristate "Character device in Userspace support"
> >         depends on FUSE_FS
> > 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..c0f412dfe9a7
> > --- /dev/null
> > +++ b/fs/fuse/acl.c
> > @@ -0,0 +1,200 @@
> > +/*
> > +  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>
> > +
> > +#ifdef CONFIG_FUSE_FS_POSIX_ACL
> > +
> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> > +                             struct dentry *dentry, struct inode *inode,
> > +                             const char *name, void *value, size_t size)
> > +{
> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> > +
> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> > +               if (handler->flags == ACL_TYPE_ACCESS)
> > +                       return posix_acl_access_xattr_handler.get(
> > +                                       &posix_acl_access_xattr_handler,
> > +                                       dentry, inode, name, value, size);
> > +               if (handler->flags == ACL_TYPE_DEFAULT)
> > +                       return posix_acl_default_xattr_handler.get(
> > +                                       &posix_acl_default_xattr_handler,
> > +                                       dentry, inode, name, value, size);
> > +       }
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> > +                               struct dentry *dentry, struct inode *inode,
> > +                               const char *name, const void *value, size_t size,
> > +                               int flags)
> > +{
> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> > +
> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> > +               if (handler->flags == ACL_TYPE_ACCESS)
> > +                       return posix_acl_access_xattr_handler.set(
> > +                                       &posix_acl_access_xattr_handler,
> > +                                       dentry, inode, name, value, size,
> > +                                       flags);
> > +               if (handler->flags == ACL_TYPE_DEFAULT)
> > +                       return posix_acl_default_xattr_handler.set(
> > +                                       &posix_acl_default_xattr_handler,
> > +                                       dentry, inode, name, value, size,
> > +                                       flags);
> > +       }
> > +       return -EOPNOTSUPP;
> > +}
> 
> The above should go away.  As I said, getxattr() and setxattr()
> shouldn't behave differently depending on whether
> "default_permissions" is set or not.

So use the posix xattr handlers in all cases then? I'm okay with that,
my only reason for not doing it was that when default_permissions is not
set the acl xattr reads can come from the cache rather than the
filesystem.

> 
> > +
> > +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->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);
> > +
> > +       size = fuse_getxattr(inode, name, NULL, 0);
> > +       if (size > 0) {
> > +               value = kzalloc(size, GFP_KERNEL);
> > +               if (!value)
> > +                       return ERR_PTR(-ENOMEM);
> > +               size = fuse_getxattr(inode, name, value, size);
> 
> Can we optimize away the first getxattr?  Starting with a page size
> buffer and falling back to the two calls only if ERANGE is returned.

Yeah, that's a good idea.

> > +       }
> > +       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->no_setxattr)
> > +               return -EOPNOTSUPP;
> > +
> > +       if (type == ACL_TYPE_ACCESS) {
> > +               struct iattr attr;
> > +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +               attr.ia_mode = inode->i_mode;
> > +               ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
> > +               if (ret < 0)
> > +                       return ret;
> > +               if (ret == 0)
> > +                       acl = NULL;
> > +               if (inode->i_mode != attr.ia_mode) {
> > +                       attr.ia_valid = ATTR_MODE | ATTR_CTIME;
> > +                       attr.ia_ctime = current_fs_time(inode->i_sb);
> > +                       ret = fuse_do_setattr(inode, &attr, NULL);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       } else if (type == ACL_TYPE_DEFAULT) {
> > +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +       } else {
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (acl) {
> > +               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;
> > +}
> 
> I wonder if splitting setxattr into a setattr + setxattr isn't going
> to cause problems.  By doing this userspace filesystem can no longer
> guarantee atomicity of the update.
> 
> Maybe we just need to introduce a new operation that can do chmod+setxattr?

I was hoping that it could be done with only xattr support from the
filesystems, but if the lack of atomicity is going to be a problem then
maybe that isn't possible. If parts of this are to be pushed to
userspace then I'd prefer that they're implemented at the libfuse level
(as you suggest below) so there will at least be consistent behavior.

> > +
> > +int fuse_init_acl(struct inode *inode, struct inode *dir)
> > +{
> > +       struct posix_acl *default_acl, *acl;
> > +       int err;
> > +
> > +       err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> > +       if (err)
> > +               return err;
> > +
> > +       if (default_acl) {
> > +               err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> > +               posix_acl_release(default_acl);
> > +       }
> > +       if (acl) {
> > +               if (!err)
> > +                       err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS);
> > +               posix_acl_release(default_acl);
> > +       }
> > +       return err;
> > +}
> > +
> > +#else /* !CONFIG_FUSE_FS_POSIX_ACL */
> > +
> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> > +                             struct dentry *dentry, struct inode *inode,
> > +                             const char *name, void *value, size_t size)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> > +                               struct dentry *dentry, struct inode *inode,
> > +                               const char *name, const void *value, size_t size,
> > +                               int flags)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +#endif /* CONFIG_FUSE_FS_POSIX_ACL */
> > +
> > +const struct xattr_handler fuse_xattr_acl_access_handler = {
> > +       .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > +       .flags  = ACL_TYPE_ACCESS,
> > +       .get    = fuse_xattr_acl_get,
> > +       .set    = fuse_xattr_acl_set,
> > +};
> > +
> > +const struct xattr_handler fuse_xattr_acl_default_handler = {
> > +       .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > +       .flags  = ACL_TYPE_DEFAULT,
> > +       .get    = fuse_xattr_acl_get,
> > +       .set    = fuse_xattr_acl_set,
> > +};
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 9df55b8e776a..ca7d573f3121 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);
> > @@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args,
> >         }
> >         kfree(forget);
> >
> > +       if (args->in.h.opcode != FUSE_LINK) {
> > +               err = fuse_init_acl(inode, dir);
> 
> Again, atomicity problems.
> 
> > +               if (err) {
> > +                       iput(inode);
> > +                       return err;
> > +               }
> > +       }
> > +
> >         err = d_instantiate_no_diralias(entry, inode);
> >         if (err)
> >                 return err;
> > @@ -916,6 +926,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;
> > @@ -1062,6 +1073,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);
> >  }
> >
> > @@ -1231,6 +1243,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);
> > @@ -1698,14 +1711,19 @@ error:
> >  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
> >  {
> >         struct inode *inode = d_inode(entry);
> > +       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 (!ret && (attr->ia_valid & ATTR_MODE))
> > +               ret = posix_acl_chmod(inode, inode->i_mode);
> 
> And again.
> 
> I'm really wondering if it's simpler to just add an xattr parser to
> libfuse and do these at the filesystem level.  That would simplify
> this patchset a lot:
> 
> Reduce the scope to just permission checking, which is what we can do
> best and fastest in the kernel.  And leave the rest to userspace.
> They don't have performance impact, but trying to push this into the
> kernel is just asking for trouble.

We still need to go through the kernel acl xattr handlers for sure, but
on the setxattr side I suppose most of it could be handled by userspace.
There might be problems if you pair a kernel with fuse acl support with
a userspace which doesn't have it, for default acls at least. Not sure
what the solution is for that.

Thanks,
Seth

Re: [RFC v3 2/2] fuse: Add posix acl support

[Eric W. Biederman]

Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:

> On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:
>> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
>> <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
>> > Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
>> > and default_permissions is used for the filesystem. When
>> > default_permissions is not used fuse cannot meaninfully support
>> > cals, as fuse_permission() only sends FUSE_PERMISSION from the
>> > access, faccessat, chdir, fchdir, and chroot system calls.
>> > Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
>> > default_permissions is not used fuse will return -EOPNOTSUPP
>> > whenever posix acl xattrs are read or written.
>> 
>> Which is breaking backward compatibilty.  Please don't do this without
>> good reason.
>> 
>> Even having "default_permissions" change behavior might cause
>> problems.  I'd suggest adding an INIT flag to indicate whether the
>> filesystem wants acl checking or not.  Feature negotiation with the
>> INIT flag is good for another reason:  the filesystem can detect
>> whether this feature is available and act differently based on that.
>
> This backwards compatibility is a bit of a mystery to me. I've seen
> claims of fuse filesystems supporting acls internally, but in practice I
> can't see how this can possibly work since fuse_permission() only sends
> a FUSE_ACCESS request if !default_permissions and one of MAY_ACCESS or
> MAY_CHDIR is set.
>
> Of course it is also a change to not pass through the xattr even if it
> isn't being enforced. I'm going to defer to Eric on this point, as he
> has some broader-reaching ideas about this.

Fuse is weird in dealing with the special xattrs.

A typical filesystem will only support the special xattrs (aka
security.* system.*) when the underlying filesystem supports them.  For
fuse that would be an INIT flag.

The reason this comes up is all of this starts with allowing
unprivileged mounts of fuse filesystems.  A fully unprivileged mount
implies your own private user namespace and your own private mount
namespace.

As posix acls have embedded uids and gids their meaning depends on
which user namespace they are interpreted in.  When a filesystem is
mounted outside of the initial user namespace this becomes interesting.

The sanest approach I can see internal to the kernel is to require all
filesystems that care about posix acls to implement get_acl and set_acl
methods and not let the filesystems have access to the raw posix acl
data.  That moves the communication from the vfs to the filesystem
into kuids and kgids (like all other uid and gid data).


For most filesystems that opt into receiving the posix acl xattrs only
when posix acls are implemented this is not a problem.  For fuse this
is interesting.

It is my intention to clean all of this up early November when I get
back from vacation.  At which point the plan is to simply have
the vfs return a not supported error if passed or asked for a posix acl
xattr.

My hope is that all of this is far enough along that fuse can do
something compatible in parallel such as return -EOPNOTSUPP or -ENOTSUP
when presented with posix acls xattrs if fuse posix acl support is not
enabled.

Which leads me to this chunk of the code being reviewed:

>> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
>> > new file mode 100644
>> > index 000000000000..c0f412dfe9a7
>> > --- /dev/null
>> > +++ b/fs/fuse/acl.c
>> > @@ -0,0 +1,200 @@
>> > +/*
>> > +  FUSE: Filesystem in Userspace
>> > +  Copyright (C) 2016 Canonical Ltd. <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> > +
>> > +  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>
>> > +
>> > +#ifdef CONFIG_FUSE_FS_POSIX_ACL
>> > +
>> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
>> > +                             struct dentry *dentry, struct inode *inode,
>> > +                             const char *name, void *value, size_t size)
>> > +{
>> > +       struct fuse_conn *fc = get_fuse_conn(inode);
>> > +
>> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
>> > +               if (handler->flags == ACL_TYPE_ACCESS)
>> > +                       return posix_acl_access_xattr_handler.get(
>> > +                                       &posix_acl_access_xattr_handler,
>> > +                                       dentry, inode, name, value, size);
>> > +               if (handler->flags == ACL_TYPE_DEFAULT)
>> > +                       return posix_acl_default_xattr_handler.get(
>> > +                                       &posix_acl_default_xattr_handler,
>> > +                                       dentry, inode, name, value, size);
>> > +       }
>> > +       return -EOPNOTSUPP;
>> > +}
>> > +
>> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
>> > +                               struct dentry *dentry, struct inode *inode,
>> > +                               const char *name, const void *value, size_t size,
>> > +                               int flags)
>> > +{
>> > +       struct fuse_conn *fc = get_fuse_conn(inode);
>> > +
>> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
>> > +               if (handler->flags == ACL_TYPE_ACCESS)
>> > +                       return posix_acl_access_xattr_handler.set(
>> > +                                       &posix_acl_access_xattr_handler,
>> > +                                       dentry, inode, name, value, size,
>> > +                                       flags);
>> > +               if (handler->flags == ACL_TYPE_DEFAULT)
>> > +                       return posix_acl_default_xattr_handler.set(
>> > +                                       &posix_acl_default_xattr_handler,
>> > +                                       dentry, inode, name, value, size,
>> > +                                       flags);
>> > +       }
>> > +       return -EOPNOTSUPP;
>> > +}
>> 
>> The above should go away.  As I said, getxattr() and setxattr()
>> shouldn't behave differently depending on whether
>> "default_permissions" is set or not.
>
> So use the posix xattr handlers in all cases then? I'm okay with that,
> my only reason for not doing it was that when default_permissions is not
> set the acl xattr reads can come from the cache rather than the
> filesystem.

I don't quite understand this feedback, or the response.

I think what is desired here is:

- In fuse_send_init to send a new flag FUSE_POSIX_ACL,
  if the mount option "default_permissions" is set. ( Supporting posix
  acls don't if fuse is not going to support them).

- In process_init_reply test if FUSE_POSIX_ACL was returned
  and set a boolean fc->posix_acl.

- In the code above test fc->posix_acl instead of
  FUSE_DEFAULT_PERMISSIONS.  As having get_acl and set_acl succeeded
  implies that   that the filesystem implements posix acls.
  
- Don't bother implementing a posix acl Kconfig option (after the review
  comments there is so little code in the kernel it shouldn't matter).

Which will result in the posix acl xattr not being transmitted to the
filesystem unless the filesystem has enabled posix acl support.

That seems simple and an implementation that won't need to change when I
clean up the kernel posix acl support.

Seth does that make sense to you?

Eric

------------------------------------------------------------------------------
-- 
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel

Re: [RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:
> >> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
> >> <seth.forshee@canonical.com> wrote:
> >> > Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y
> >> > and default_permissions is used for the filesystem. When
> >> > default_permissions is not used fuse cannot meaninfully support
> >> > cals, as fuse_permission() only sends FUSE_PERMISSION from the
> >> > access, faccessat, chdir, fchdir, and chroot system calls.
> >> > Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or
> >> > default_permissions is not used fuse will return -EOPNOTSUPP
> >> > whenever posix acl xattrs are read or written.
> >> 
> >> Which is breaking backward compatibilty.  Please don't do this without
> >> good reason.
> >> 
> >> Even having "default_permissions" change behavior might cause
> >> problems.  I'd suggest adding an INIT flag to indicate whether the
> >> filesystem wants acl checking or not.  Feature negotiation with the
> >> INIT flag is good for another reason:  the filesystem can detect
> >> whether this feature is available and act differently based on that.
> >
> > This backwards compatibility is a bit of a mystery to me. I've seen
> > claims of fuse filesystems supporting acls internally, but in practice I
> > can't see how this can possibly work since fuse_permission() only sends
> > a FUSE_ACCESS request if !default_permissions and one of MAY_ACCESS or
> > MAY_CHDIR is set.
> >
> > Of course it is also a change to not pass through the xattr even if it
> > isn't being enforced. I'm going to defer to Eric on this point, as he
> > has some broader-reaching ideas about this.
> 
> Fuse is weird in dealing with the special xattrs.
> 
> A typical filesystem will only support the special xattrs (aka
> security.* system.*) when the underlying filesystem supports them.  For
> fuse that would be an INIT flag.
> 
> The reason this comes up is all of this starts with allowing
> unprivileged mounts of fuse filesystems.  A fully unprivileged mount
> implies your own private user namespace and your own private mount
> namespace.
> 
> As posix acls have embedded uids and gids their meaning depends on
> which user namespace they are interpreted in.  When a filesystem is
> mounted outside of the initial user namespace this becomes interesting.
> 
> The sanest approach I can see internal to the kernel is to require all
> filesystems that care about posix acls to implement get_acl and set_acl
> methods and not let the filesystems have access to the raw posix acl
> data.  That moves the communication from the vfs to the filesystem
> into kuids and kgids (like all other uid and gid data).
> 
> 
> For most filesystems that opt into receiving the posix acl xattrs only
> when posix acls are implemented this is not a problem.  For fuse this
> is interesting.
> 
> It is my intention to clean all of this up early November when I get
> back from vacation.  At which point the plan is to simply have
> the vfs return a not supported error if passed or asked for a posix acl
> xattr.
> 
> My hope is that all of this is far enough along that fuse can do
> something compatible in parallel such as return -EOPNOTSUPP or -ENOTSUP
> when presented with posix acls xattrs if fuse posix acl support is not
> enabled.
> 
> Which leads me to this chunk of the code being reviewed:
> 
> >> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> >> > new file mode 100644
> >> > index 000000000000..c0f412dfe9a7
> >> > --- /dev/null
> >> > +++ b/fs/fuse/acl.c
> >> > @@ -0,0 +1,200 @@
> >> > +/*
> >> > +  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>
> >> > +
> >> > +#ifdef CONFIG_FUSE_FS_POSIX_ACL
> >> > +
> >> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> >> > +                             struct dentry *dentry, struct inode *inode,
> >> > +                             const char *name, void *value, size_t size)
> >> > +{
> >> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> >> > +
> >> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> >> > +               if (handler->flags == ACL_TYPE_ACCESS)
> >> > +                       return posix_acl_access_xattr_handler.get(
> >> > +                                       &posix_acl_access_xattr_handler,
> >> > +                                       dentry, inode, name, value, size);
> >> > +               if (handler->flags == ACL_TYPE_DEFAULT)
> >> > +                       return posix_acl_default_xattr_handler.get(
> >> > +                                       &posix_acl_default_xattr_handler,
> >> > +                                       dentry, inode, name, value, size);
> >> > +       }
> >> > +       return -EOPNOTSUPP;
> >> > +}
> >> > +
> >> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> >> > +                               struct dentry *dentry, struct inode *inode,
> >> > +                               const char *name, const void *value, size_t size,
> >> > +                               int flags)
> >> > +{
> >> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> >> > +
> >> > +       if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
> >> > +               if (handler->flags == ACL_TYPE_ACCESS)
> >> > +                       return posix_acl_access_xattr_handler.set(
> >> > +                                       &posix_acl_access_xattr_handler,
> >> > +                                       dentry, inode, name, value, size,
> >> > +                                       flags);
> >> > +               if (handler->flags == ACL_TYPE_DEFAULT)
> >> > +                       return posix_acl_default_xattr_handler.set(
> >> > +                                       &posix_acl_default_xattr_handler,
> >> > +                                       dentry, inode, name, value, size,
> >> > +                                       flags);
> >> > +       }
> >> > +       return -EOPNOTSUPP;
> >> > +}
> >> 
> >> The above should go away.  As I said, getxattr() and setxattr()
> >> shouldn't behave differently depending on whether
> >> "default_permissions" is set or not.
> >
> > So use the posix xattr handlers in all cases then? I'm okay with that,
> > my only reason for not doing it was that when default_permissions is not
> > set the acl xattr reads can come from the cache rather than the
> > filesystem.
> 
> I don't quite understand this feedback, or the response.

I can at least explain the response.

What I mean is that we could use the generic posix acl xattr handlers
and the get_acl/set_acl callbacks regardless of whether or not
default_permissions is used. When it's not used we get the id
translation even though the acls aren't being enforced.

The cost to that though is pointless caching. I was also saying that you
could theoretically have aliasing between the cache and the filesystem if
changes were made in the filesystem outside of the kernel's knowledge,
but I'm not convinced this is really a problem to be worried about.

What still doesn't work well in that scenario is passing though the acl
xattrs when CONFIG_FS_POSIX_ACL=n, as the ids would not get translated.
But returning -EOPNOTSUPP (or whatever error) in this case seems
reasonable to me.

> I think what is desired here is:
> 
> - In fuse_send_init to send a new flag FUSE_POSIX_ACL,
>   if the mount option "default_permissions" is set. ( Supporting posix
>   acls don't if fuse is not going to support them).
> 
> - In process_init_reply test if FUSE_POSIX_ACL was returned
>   and set a boolean fc->posix_acl.
> 
> - In the code above test fc->posix_acl instead of
>   FUSE_DEFAULT_PERMISSIONS.  As having get_acl and set_acl succeeded
>   implies that   that the filesystem implements posix acls.
>   
> - Don't bother implementing a posix acl Kconfig option (after the review
>   comments there is so little code in the kernel it shouldn't matter).

Without CONFIG_FS_POSIX_ACL=y we're missing definitions for some
symbols, off the top of my head the posix acl xattr handlers but there
may be others. That's one of those "hidden" options that's only enabled
when some other option selects it. Using a Kconfig option for the
filesystem like I did here is the usual way of handling it, and I don't
see a good reason to make fuse different in this respect.

> Which will result in the posix acl xattr not being transmitted to the
> filesystem unless the filesystem has enabled posix acl support.
> 
> That seems simple and an implementation that won't need to change when I
> clean up the kernel posix acl support.
> 
> Seth does that make sense to you?

Yes, it makes sense. I was hoping we could do it without userspace
modifications but I guess that isn't going to happen.

What I'm not convinced of is that the userspace visible changes in
behavior won't break someone's software, even if they aren't really
getting acl enforcement.

Thanks,
Seth

Re: [RFC v3 2/2] fuse: Add posix acl support

[Miklos Szeredi]

On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote:
> On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
> What I'm not convinced of is that the userspace visible changes in
> behavior won't break someone's software, even if they aren't really
> getting acl enforcement.

That's a key point.  Backward compatibility is important, and not even
hard to do because fuse can negotiate supported features with the
userspace filesystem.

So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if
"default_permissions" is on.

If not set in INIT reply just pass all xattrs through to the
filesystem.  Caching should not be done. Don't think about whether
it's logical or not, or if anyone could use it for anything sane.
Just do what we are doing currently.  Translating uids still makes
sense, but that's another story.

If the flag is set in INIT reply, then that means userspace filesystem
wants handling of posix acl permission checking in kernel.  It would
also mean that caching of posix acl are allowed (lifetime linked to
attribute lifetime).

If filesystem wants to explicitly disable posix acl support, then it
can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*".
  Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that
filesystem can return in reply to FUSE_POSIX_ACL.

I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it,
just make any such code dependent on CONFIG_FS_POSIX_ACL.

Thanks,
Miklos

Re: [RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote:
> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote:
> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
> > What I'm not convinced of is that the userspace visible changes in
> > behavior won't break someone's software, even if they aren't really
> > getting acl enforcement.
> 
> That's a key point.  Backward compatibility is important, and not even
> hard to do because fuse can negotiate supported features with the
> userspace filesystem.
> 
> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if
> "default_permissions" is on.
> 
> If not set in INIT reply just pass all xattrs through to the
> filesystem.  Caching should not be done. Don't think about whether
> it's logical or not, or if anyone could use it for anything sane.
> Just do what we are doing currently.  Translating uids still makes
> sense, but that's another story.

Translating uids is actually central to the differing positions that you
and Eric have. What Eric wants is for the only path for supporting posix
acls to be via the helpers, that way all concerns about translating uids
can be addressed there. If fuse is to allow the xattrs to be passed
directly through to the filesystem then there has to be a second
mechanism which translates the uids for that case.

> If the flag is set in INIT reply, then that means userspace filesystem
> wants handling of posix acl permission checking in kernel.  It would
> also mean that caching of posix acl are allowed (lifetime linked to
> attribute lifetime).
> 
> If filesystem wants to explicitly disable posix acl support, then it
> can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*".
>   Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that
> filesystem can return in reply to FUSE_POSIX_ACL.
> 
> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it,
> just make any such code dependent on CONFIG_FS_POSIX_ACL.

But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't
displayed in menuconfig, etc. If that's what you want, fine, but it
seems like an unusual situation.

Thanks,
Seth

Re: [RFC v3 2/2] fuse: Add posix acl support

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote:
>> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote:
>> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
>> > What I'm not convinced of is that the userspace visible changes in
>> > behavior won't break someone's software, even if they aren't really
>> > getting acl enforcement.
>> 
>> That's a key point.  Backward compatibility is important, and not even
>> hard to do because fuse can negotiate supported features with the
>> userspace filesystem.
>> 
>> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if
>> "default_permissions" is on.
>> 
>> If not set in INIT reply just pass all xattrs through to the
>> filesystem.  Caching should not be done. Don't think about whether
>> it's logical or not, or if anyone could use it for anything sane.
>> Just do what we are doing currently.  Translating uids still makes
>> sense, but that's another story.
>
> Translating uids is actually central to the differing positions that you
> and Eric have. What Eric wants is for the only path for supporting posix
> acls to be via the helpers, that way all concerns about translating uids
> can be addressed there. If fuse is to allow the xattrs to be passed
> directly through to the filesystem then there has to be a second
> mechanism which translates the uids for that case.

I think I will agree with Miklos.  Translating uids is another story and
worst case we can work on that in September when I get back from
vacation.  Nothing of what you are talking about will make things worse
for translating uids, so it is perfectly fine to merge posix acl support
into fuse and then handle uid translation.

Until we set FS_USER_NS fuse it is fine to not get the uid translation.

Which yields a very simple implementation:

In fuse_fill_super:
	if (!fc->posix_acl) {
        	sb->s_xattr = fuse_xattr_handlers;
        } else {
        	sb->s_xattr = fuse_acl_xattr_handlers;
        }

Where fuse_acl_xattr_handlers are a different array that
includes the posix acl interrcept (aka the array in patch 1 or the array
in patch 2).

Then fuse_get_acl and fuse_set_acl can just test fc->posix_acl
and fail if that is not set.

I think that is all you need to do, and we can worry about the other
details after posix acl support has landed in fuse.

>> If the flag is set in INIT reply, then that means userspace filesystem
>> wants handling of posix acl permission checking in kernel.  It would
>> also mean that caching of posix acl are allowed (lifetime linked to
>> attribute lifetime).
>> 
>> If filesystem wants to explicitly disable posix acl support, then it
>> can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*".
>>   Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that
>> filesystem can return in reply to FUSE_POSIX_ACL.
>> 
>> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it,
>> just make any such code dependent on CONFIG_FS_POSIX_ACL.
>
> But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't
> displayed in menuconfig, etc. If that's what you want, fine, but it
> seems like an unusual situation.

Then in Kconfig I would have FUSE_FS "select FS_POSIX_ACL".  That
simplifies the problem.  Unless Miklos wants something more granular.

Eric

Re: [RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

[-- Attachment #1.1: Type: text/plain, Size: 3786 bytes --]

On Aug 7, 2016 8:12 AM, "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>
> Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> writes:
>
> > On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote:
> >> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <
seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> >> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote:
> >> > What I'm not convinced of is that the userspace visible changes in
> >> > behavior won't break someone's software, even if they aren't really
> >> > getting acl enforcement.
> >>
> >> That's a key point.  Backward compatibility is important, and not even
> >> hard to do because fuse can negotiate supported features with the
> >> userspace filesystem.
> >>
> >> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if
> >> "default_permissions" is on.
> >>
> >> If not set in INIT reply just pass all xattrs through to the
> >> filesystem.  Caching should not be done. Don't think about whether
> >> it's logical or not, or if anyone could use it for anything sane.
> >> Just do what we are doing currently.  Translating uids still makes
> >> sense, but that's another story.
> >
> > Translating uids is actually central to the differing positions that you
> > and Eric have. What Eric wants is for the only path for supporting posix
> > acls to be via the helpers, that way all concerns about translating uids
> > can be addressed there. If fuse is to allow the xattrs to be passed
> > directly through to the filesystem then there has to be a second
> > mechanism which translates the uids for that case.
>
> I think I will agree with Miklos.  Translating uids is another story and
> worst case we can work on that in September when I get back from
> vacation.  Nothing of what you are talking about will make things worse
> for translating uids, so it is perfectly fine to merge posix acl support
> into fuse and then handle uid translation.
>
> Until we set FS_USER_NS fuse it is fine to not get the uid translation.
>
> Which yields a very simple implementation:
>
> In fuse_fill_super:
>         if (!fc->posix_acl) {
>                 sb->s_xattr = fuse_xattr_handlers;
>         } else {
>                 sb->s_xattr = fuse_acl_xattr_handlers;
>         }
>
> Where fuse_acl_xattr_handlers are a different array that
> includes the posix acl interrcept (aka the array in patch 1 or the array
> in patch 2).
>
> Then fuse_get_acl and fuse_set_acl can just test fc->posix_acl
> and fail if that is not set.
>
> I think that is all you need to do, and we can worry about the other
> details after posix acl support has landed in fuse.

Ok.

> >> If the flag is set in INIT reply, then that means userspace filesystem
> >> wants handling of posix acl permission checking in kernel.  It would
> >> also mean that caching of posix acl are allowed (lifetime linked to
> >> attribute lifetime).
> >>
> >> If filesystem wants to explicitly disable posix acl support, then it
> >> can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*".
> >>   Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that
> >> filesystem can return in reply to FUSE_POSIX_ACL.
> >>
> >> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it,
> >> just make any such code dependent on CONFIG_FS_POSIX_ACL.
> >
> > But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't
> > displayed in menuconfig, etc. If that's what you want, fine, but it
> > seems like an unusual situation.
>
> Then in Kconfig I would have FUSE_FS "select FS_POSIX_ACL".  That
> simplifies the problem.  Unless Miklos wants something more granular.

Which is the the alternative I offered a couple of emails ago ;-)

Thanks,
Seth

[-- Attachment #1.2: Type: text/html, Size: 5076 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 119 bytes --]

-- 
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel

Re: [RFC v3 2/2] fuse: Add posix acl support

[Seth Forshee]

Hi Miklos,

On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:

<snip>

> And again.
> 
> I'm really wondering if it's simpler to just add an xattr parser to
> libfuse and do these at the filesystem level.  That would simplify
> this patchset a lot:
> 
> Reduce the scope to just permission checking, which is what we can do
> best and fastest in the kernel.  And leave the rest to userspace.
> They don't have performance impact, but trying to push this into the
> kernel is just asking for trouble.

I've been playing with this over the past couple of days, and I wanted
to get a little more feedback before I proceed.

Things are pretty simple in the kernel if we just pass through the acl
xattrs, but either the kernel or libfuse will need to work out the
equivalent file mode when posix acls are written. I'm favoring libfuse
for this, since it's very straightforward once you're already parsing
the xattr and then we won't need to add a setattr+setxattr op. What we
will need is to refresh the mode in the kernel from userspace.

Right now after a successful setxattr we call fuse_invalidate_attr(),
which should take care of that problem. I'm not sure the reasoning
behind doing this still applies though. According to
d331a415aef98717393dda0be69b7947da08eba3 it was added to force a refresh
of ctime, but later in 31f3267b4ba16b12fb9dd3b1953ea0f221cc2ab4 fuse was
changed to prefer ctime as maintained by the kernel, so it looks like
that invalidate (and the one in removexattr, maybe others?) could be
removed.

If so, we could still keep it when setting posix acl xattrs, which would
be the simplest option. Otherwise we need to get the mode back from
userspace after the setxattr, either via a conditional outarg for
setxattr or by adding a new operation.

What's your preference for all of this?

Thanks,
Seth

Re: [RFC v3 2/2] fuse: Add posix acl support

[Miklos Szeredi]

On Tue, Aug 16, 2016 at 10:59 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> Hi Miklos,
>
> On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:
>
> <snip>
>
>> And again.
>>
>> I'm really wondering if it's simpler to just add an xattr parser to
>> libfuse and do these at the filesystem level.  That would simplify
>> this patchset a lot:
>>
>> Reduce the scope to just permission checking, which is what we can do
>> best and fastest in the kernel.  And leave the rest to userspace.
>> They don't have performance impact, but trying to push this into the
>> kernel is just asking for trouble.
>
> I've been playing with this over the past couple of days, and I wanted
> to get a little more feedback before I proceed.
>
> Things are pretty simple in the kernel if we just pass through the acl
> xattrs, but either the kernel or libfuse will need to work out the
> equivalent file mode when posix acls are written. I'm favoring libfuse
> for this, since it's very straightforward once you're already parsing
> the xattr and then we won't need to add a setattr+setxattr op. What we
> will need is to refresh the mode in the kernel from userspace.
>
> Right now after a successful setxattr we call fuse_invalidate_attr(),
> which should take care of that problem. I'm not sure the reasoning
> behind doing this still applies though. According to
> d331a415aef98717393dda0be69b7947da08eba3 it was added to force a refresh
> of ctime, but later in 31f3267b4ba16b12fb9dd3b1953ea0f221cc2ab4 fuse was
> changed to prefer ctime as maintained by the kernel, so it looks like
> that invalidate (and the one in removexattr, maybe others?) could be
> removed.
>
> If so, we could still keep it when setting posix acl xattrs, which would
> be the simplest option. Otherwise we need to get the mode back from
> userspace after the setxattr, either via a conditional outarg for
> setxattr or by adding a new operation.
>
> What's your preference for all of this?

Lets just keep the invalidate.  My philosophy is to not optimize until
someone actually complains about performance.  And these invalidates
are unlikely to be a problem anyway.

Thanks,
Miklos

Re: [RFC v3 0/2] Support for posix acls in fuse

[Nikolaus Rath]

On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
> There's also a problem with default acls that I'm not sure there's
> currently a solution for. As far as I can tell FUSE_CREATE doesn't give
> back any indication of whether an existing file was opened or a new file
> was created. Without knowing that I cannot know whether or not the inode
> should inherit default acls from its parent.

Would it be possible for the FUSE file system to implement this
inheritance by a dumb-copy of the ACL-related xattrs of the parent?

This would solve your problem. But in addition to that, it also seems to
me that even if a file system uses default_permissions for ACL handling,
it may want implement a different policy for permission inheritance...


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: [RFC v3 0/2] Support for posix acls in fuse

[Seth Forshee]

On Mon, Aug 01, 2016 at 04:03:57PM -0700, Nikolaus Rath wrote:
> On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
> > There's also a problem with default acls that I'm not sure there's
> > currently a solution for. As far as I can tell FUSE_CREATE doesn't give
> > back any indication of whether an existing file was opened or a new file
> > was created. Without knowing that I cannot know whether or not the inode
> > should inherit default acls from its parent.
> 
> Would it be possible for the FUSE file system to implement this
> inheritance by a dumb-copy of the ACL-related xattrs of the parent?
> 
> This would solve your problem. But in addition to that, it also seems to
> me that even if a file system uses default_permissions for ACL handling,
> it may want implement a different policy for permission inheritance...

In my opinion it's preferable for the kernel to handle all of this and
for the filesystems to need only xattr support to get support for acls.

The inheritance behavior is standard for default acls. It shouldn't be
left to individual filesystems to decide the inheritance policy.

Thanks,
Seth

Re: [fuse-devel] [RFC v3 0/2] Support for posix acls in fuse

[Michael Theall]

On Mon, 2016-08-01 at 22:39 -0500, Seth Forshee wrote:
> On Mon, Aug 01, 2016 at 04:03:57PM -0700, Nikolaus Rath wrote:
> > 
> > On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
> > > 
> > > There's also a problem with default acls that I'm not sure
> > > there's
> > > currently a solution for. As far as I can tell FUSE_CREATE
> > > doesn't give
> > > back any indication of whether an existing file was opened or a
> > > new file
> > > was created. Without knowing that I cannot know whether or not
> > > the inode
> > > should inherit default acls from its parent.
> > Would it be possible for the FUSE file system to implement this
> > inheritance by a dumb-copy of the ACL-related xattrs of the parent?
> > 
> > This would solve your problem. But in addition to that, it also
> > seems to
> > me that even if a file system uses default_permissions for ACL
> > handling,
> > it may want implement a different policy for permission
> > inheritance...
> In my opinion it's preferable for the kernel to handle all of this
> and
> for the filesystems to need only xattr support to get support for
> acls.
> 
> The inheritance behavior is standard for default acls. It shouldn't
> be
> left to individual filesystems to decide the inheritance policy.
> 
> Thanks,
> Seth

In case this has any bearing, my filesystem would in fact interpret the
ACLs from the xattrs in order to apply them to the backing filesystem
(which supports ACLs but through a non-xattr interface). In my
particular case, it would be okay for the kernel to assume the
inherited ACLs since it should be the same as if the kernel requested
the ACLs after creation.

Regards,
Michael Theall

Re: [RFC v3 0/2] Support for posix acls in fuse

[Nikolaus Rath]

On Aug 02 2016, Michael Theall <mtheall-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> In case this has any bearing, my filesystem would in fact interpret the
> ACLs from the xattrs in order to apply them to the backing filesystem
> (which supports ACLs but through a non-xattr interface). In my
> particular case, it would be okay for the kernel to assume the
> inherited ACLs since it should be the same as if the kernel requested
> the ACLs after creation.

If I'm not mistaken, this means your file system will however be broken
by this change:

 - Remove passthrough of acl xattrs when fuse acl support is disabled or
   default_permissions is not used.

I assume you don't use default_permissons (because otherwise you
couldn't support ACLs), so this means with the patch you would no longer
get the ACLs in xattr forms.


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: [RFC v3 0/2] Support for posix acls in fuse

[Nikolaus Rath]

On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
>  - Remove passthrough of acl xattrs when fuse acl support is disabled or
>    default_permissions is not used.
>
> This last change is user visible, but as fuse filesystems cannot
> meaninfully support acls today it's not really a regression.

Are you sure about that? I believe there are FUSE file systems out there
that are parsing/constructing the kernel's xattr representation and
(together with no_default_permissions) support ACLs. Or is there another
problem?
 

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: [RFC v3 0/2] Support for posix acls in fuse

[Eric W. Biederman]

Nikolaus Rath <Nikolaus@rath.org> writes:

> On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
>>  - Remove passthrough of acl xattrs when fuse acl support is disabled or
>>    default_permissions is not used.
>>
>> This last change is user visible, but as fuse filesystems cannot
>> meaninfully support acls today it's not really a regression.
>
> Are you sure about that? I believe there are FUSE file systems out there
> that are parsing/constructing the kernel's xattr representation and
> (together with no_default_permissions) support ACLs. Or is there another
> problem?

fuse_permission does not have a mode where it always call into the
filesystem.  Without FUSE_DEFAULT_PERMISSIONS set the underlying
filesystem is at most called when the syscalls chdir, access, and
execve are called. (Basically

Which means there is no way to enforce any kind of general acls in fuse
without changes.

That said I we seem to have figured out an implmenetation where
passthrough is maintained for the time being when posix acl support is
not enabled.  And Miklos figures libfuse needs to parse the the xattr
anyway so that the filesystems can have atomic mode changes instead of
having two separate calls, one to setattr and another to setxattr.

So I don't believe when the dust settles there is any danger of
regression, despite the code not yet working in a way that enforces
acls.


Eric

Re: [RFC v3 0/2] Support for posix acls in fuse

[Nikolaus Rath]

On Aug 08 2016, ebiederm@xmission.com (Eric W. Biederman) wrote:
> Nikolaus Rath <Nikolaus@rath.org> writes:
>
>> On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
>>>  - Remove passthrough of acl xattrs when fuse acl support is disabled or
>>>    default_permissions is not used.
>>>
>>> This last change is user visible, but as fuse filesystems cannot
>>> meaninfully support acls today it's not really a regression.
>>
>> Are you sure about that? I believe there are FUSE file systems out there
>> that are parsing/constructing the kernel's xattr representation and
>> (together with no_default_permissions) support ACLs. Or is there another
>> problem?
>
> fuse_permission does not have a mode where it always call into the
> filesystem.  Without FUSE_DEFAULT_PERMISSIONS set the underlying
> filesystem is at most called when the syscalls chdir, access, and
> execve are called. (Basically

...plus on open, create, write, read, setattr, etc. On each of these
calls, the userspace fs is free to do an ACL check first and return an
error if access should not be granted.

So I think the only permission that cannot be enforced is execute. That
is a bug, but I wouldn't go as far as saying that what's left isn't a
meaningful feature that can just be removed entirely.

> That said I we seem to have figured out an implmenetation where
> passthrough is maintained for the time being when posix acl support is
> not enabled.  And Miklos figures libfuse needs to parse the the xattr
> anyway so that the filesystems can have atomic mode changes instead of
> having two separate calls, one to setattr and another to setxattr.
>
> So I don't believe when the dust settles there is any danger of
> regression, despite the code not yet working in a way that enforces
> acls.

I agree with this though. I was a little behind on emails.

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: [RFC v3 0/2] Support for posix acls in fuse

[Jean-Pierre André]

Nikolaus Rath wrote:
> On Aug 01 2016, Seth Forshee <seth.forshee@canonical.com> wrote:
>>   - Remove passthrough of acl xattrs when fuse acl support is disabled or
>>     default_permissions is not used.
>>
>> This last change is user visible, but as fuse filesystems cannot
>> meaninfully support acls today it's not really a regression.
>
> Are you sure about that? I believe there are FUSE file systems out there
> that are parsing/constructing the kernel's xattr representation and
> (together with no_default_permissions) support ACLs. Or is there another
> problem?

Indeed, the current implementation of Posix ACLs in
ntfs-3g relies on ACLs being sent to user space as
xattrs, with default_permissions not set (and cacheing
disabled). I agree this is not a "meaningful acl
support", but it has been the only way for ten years.

Why should this be changed when FUSE_POSIX_ACL
feature flag is not set in INIT ?

Jean-Pierre