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

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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


------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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 <[hidden email]>
+
+  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


------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Seth Forshee
In reply to this post by 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 <[hidden email]>
---
 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. <[hidden email]>
+
+  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 <[hidden email]>");
 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


------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Nikolaus Rath
In reply to this post by Seth Forshee
On Aug 01 2016, Seth Forshee <[hidden email]> 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.«

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]> 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


------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Nikolaus Rath
In reply to this post by Seth Forshee
On Aug 01 2016, Seth Forshee <[hidden email]> wrote:

> On Mon, Aug 01, 2016 at 04:03:57PM -0700, Nikolaus Rath wrote:
>> On Aug 01 2016, Seth Forshee <[hidden email]> 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.

True. But there is a precedence. If the user creates a new directory in
a directory with setgid set, userspace cannot just use the provided
mode_t but needs to manually add S_ISGID.

...but then, maybe this should actually happen in the kernel too?


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.«

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Miklos Szeredi
In reply to this post by Seth Forshee
On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
<[hidden email]> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> +
> +  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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Miklos Szeredi
In reply to this post by Seth Forshee
On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
<[hidden email]> 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 <[hidden email]>
> ---
>  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. <[hidden email]>
> +
> +  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 <[hidden email]>");
>  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
>

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Ravishankar N
On 08/04/2016 05:41 PM, Miklos Szeredi wrote:
On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
[hidden email] 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

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

--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Seth Forshee
In reply to this post by Miklos Szeredi
On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote:

> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee
> <[hidden email]> 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 <[hidden email]>
> > ---
> >  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. <[hidden email]>
> > +
> > +  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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Seth Forshee
In reply to this post by Miklos Szeredi
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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Eric W. Biederman
In reply to this post by Seth Forshee
Seth Forshee <[hidden email]> 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
>> <[hidden email]> 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. <[hidden email]>
>> > +
>> > +  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
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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
> >> <[hidden email]> 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. <[hidden email]>
> >> > +
> >> > +  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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Miklos Szeredi
On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <[hidden email]> 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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Eric W. Biederman
Seth Forshee <[hidden email]> 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 <[hidden email]> 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

------------------------------------------------------------------------------
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Seth Forshee

On Aug 7, 2016 8:12 AM, "Eric W. Biederman" <[hidden email]> wrote:
>
> Seth Forshee <[hidden email]> 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 <[hidden email]> 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


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

--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Nikolaus Rath
In reply to this post by Michael Theall
On Aug 02 2016, Michael Theall <[hidden email]> 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.«

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. http://sdm.link/zohodev2dev
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
Reply | Threaded
Open this post in threaded view
|

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

Nikolaus Rath
In reply to this post by Seth Forshee
On Aug 01 2016, Seth Forshee <[hidden email]> 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.«

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity
planning reports. http://sdm.link/zohodev2dev
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
12