[RFC] fuse: Support posix ACLs

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

[RFC] fuse: Support posix ACLs

Seth Forshee
Eric and I are working towards adding support for fuse mounts in
non-init user namespaces. Towards that end we'd like to add ACL support
to fuse as this will allow for a cleaner implementation overall. Below
is an initial patch to support this. I'd like to get some general
feedback on this patch and ask a couple of specific questions.

There are some indications that fuse supports ACLs on the userspace side
when default_permissions is not used (though I'm not seeing how that
works). Will these changes conflict with that support, and if how do we
avoid those conflicts?

Are there any places where we need to invalidate cached ACLs that aren't
covered in the patch?

Thanks,
Seth


diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 8466e122ee62..2f53b0491159 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -13,6 +13,8 @@
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
+#include <linux/posix_acl_xattr.h>
 
 static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
 {
@@ -1061,6 +1063,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);
 }
 
@@ -1719,9 +1722,8 @@ 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)
+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);
@@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry *unused, struct inode *inode,
  return err;
 }
 
-static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
-     const char *name, void *value, size_t size)
+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);
@@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
  return ret;
 }
 
-static int fuse_removexattr(struct dentry *entry, const char *name)
+static int fuse_removexattr(struct inode *inode, const char *name)
 {
- struct inode *inode = d_inode(entry);
  struct fuse_conn *fc = get_fuse_conn(inode);
  FUSE_ARGS(args);
  int err;
@@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
  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,
+};
+
+#ifndef CONFIG_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;
+}
+
+static const struct xattr_handler fuse_xattr_acl_access_handler = {
+ .name = XATTR_NAME_POSIX_ACL_ACCESS,
+ .get = fuse_xattr_acl_get,
+ .set = fuse_xattr_acl_set,
+};
+
+static const struct xattr_handler fuse_xattr_acl_default_handler = {
+ .name = XATTR_NAME_POSIX_ACL_DEFAULT,
+ .get = fuse_xattr_acl_get,
+ .set = fuse_xattr_acl_set,
+};
+#endif /* CONFIG_POSIX_ACL */
+
+const struct xattr_handler *fuse_xattr_handlers[] = {
+#ifdef CONFIG_FS_POSIX_ACL
+ &posix_acl_access_xattr_handler,
+ &posix_acl_default_xattr_handler,
+#else
+ &fuse_xattr_acl_access_handler,
+ &fuse_xattr_acl_default_handler,
+#endif
+ &fuse_xattr_handler,
+};
+
+static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
+{
+ int size;
+ const char *name;
+ void *value = NULL;
+ struct posix_acl *acl;
+
+ 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(inode->i_sb->s_user_ns, value, size);
+ } else if ((size == 0) || (size == -ENODATA)) {
+ acl = NULL;
+ } else {
+ acl = ERR_PTR(size);
+ }
+ kfree(value);
+
+ return acl;
+}
+
+static int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+ const char *name;
+ int ret;
+
+ if (type == ACL_TYPE_ACCESS)
+ name = XATTR_NAME_POSIX_ACL_ACCESS;
+ else if (type == ACL_TYPE_DEFAULT)
+ name = XATTR_NAME_POSIX_ACL_DEFAULT;
+ else
+ return -EINVAL;
+
+ if (acl) {
+ struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
+ 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(s_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;
+}
+
 static const struct inode_operations fuse_dir_inode_operations = {
  .lookup = fuse_lookup,
  .mkdir = fuse_mkdir,
@@ -1879,10 +2012,12 @@ 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,
+ .get_acl = fuse_get_acl,
+ .set_acl = fuse_set_acl,
 };
 
 static const struct file_operations fuse_dir_operations = {
@@ -1900,10 +2035,12 @@ 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,
+ .get_acl = fuse_get_acl,
+ .set_acl = fuse_set_acl,
 };
 
 static const struct inode_operations fuse_symlink_inode_operations = {
@@ -1911,10 +2048,12 @@ 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,
+ .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 9f4c3c82edd6..02c08796051e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -23,6 +23,7 @@
 #include <linux/poll.h>
 #include <linux/workqueue.h>
 #include <linux/kref.h>
+#include <linux/xattr.h>
 #include <linux/pid_namespace.h>
 #include <linux/user_namespace.h>
 
@@ -693,6 +694,8 @@ extern const struct file_operations fuse_dev_operations;
 
 extern const struct dentry_operations fuse_dentry_operations;
 
+extern const struct xattr_handler *fuse_xattr_handlers[];
+
 /**
  * Inode to nodeid comparison.
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 254f1944ee98..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)
@@ -1066,6 +1068,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;


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Michael j Theall

Going by the patch I posted a couple of years ago: https://sourceforge.net/p/fuse/mailman/message/33033653/

The only hole I see in your patch is that in setattr() you are not updating the cached acl if the ATTR_MODE is updated. The other major difference is that my version uses the get_acl/set_acl inode operations but you use that plus the xattr handlers. I'm not up-to-speed on the kernel so I'm not sure if you actually need to implement both.

Regards,
Michael Theall

Seth Forshee <[hidden email]> wrote on 06/29/2016 02:07:31 PM:

> From: Seth Forshee <[hidden email]>

> To: [hidden email], [hidden email]
> Cc: "Eric W. Biederman" <[hidden email]>, Miklos Szeredi
> <[hidden email]>

> Date: 06/29/2016 02:08 PM
> Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
>


> Eric and I are working towards adding support for fuse mounts in
> non-init user namespaces. Towards that end we'd like to add ACL support
> to fuse as this will allow for a cleaner implementation overall. Below
> is an initial patch to support this. I'd like to get some general
> feedback on this patch and ask a couple of specific questions.
>
> There are some indications that fuse supports ACLs on the userspace side
> when default_permissions is not used (though I'm not seeing how that
> works). Will these changes conflict with that support, and if how do we
> avoid those conflicts?
>
> Are there any places where we need to invalidate cached ACLs that aren't
> covered in the patch?
>
> Thanks,
> Seth
>
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 8466e122ee62..2f53b0491159 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -13,6 +13,8 @@
>  #include <linux/sched.h>
>  #include <linux/namei.h>
>  #include <linux/slab.h>
> +#include <linux/xattr.h>
> +#include <linux/posix_acl_xattr.h>
>  
>  static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
>  {
> @@ -1061,6 +1063,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);
>  }
>  
> @@ -1719,9 +1722,8 @@ 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)
> +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);
> @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> *unused, struct inode *inode,
>     return err;
>  }
>  
> -static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
> -              const char *name, void *value, size_t size)
> +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);
> @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> *entry, char *list, size_t size)
>     return ret;
>  }
>  
> -static int fuse_removexattr(struct dentry *entry, const char *name)
> +static int fuse_removexattr(struct inode *inode, const char *name)
>  {
> -   struct inode *inode = d_inode(entry);
>     struct fuse_conn *fc = get_fuse_conn(inode);
>     FUSE_ARGS(args);
>     int err;
> @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> *entry, const char *name)
>     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,
> +};
> +
> +#ifndef CONFIG_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;
> +}
> +
> +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> +   .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> +   .get   = fuse_xattr_acl_get,
> +   .set   = fuse_xattr_acl_set,
> +};
> +
> +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> +   .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> +   .get   = fuse_xattr_acl_get,
> +   .set   = fuse_xattr_acl_set,
> +};
> +#endif /* CONFIG_POSIX_ACL */
> +
> +const struct xattr_handler *fuse_xattr_handlers[] = {
> +#ifdef CONFIG_FS_POSIX_ACL
> +   &posix_acl_access_xattr_handler,
> +   &posix_acl_default_xattr_handler,
> +#else
> +   &fuse_xattr_acl_access_handler,
> +   &fuse_xattr_acl_default_handler,
> +#endif
> +   &fuse_xattr_handler,
> +};
> +
> +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> +{
> +   int size;
> +   const char *name;
> +   void *value = NULL;
> +   struct posix_acl *acl;
> +
> +   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(inode->i_sb->s_user_ns, value, size);
> +   } else if ((size == 0) || (size == -ENODATA)) {
> +      acl = NULL;
> +   } else {
> +      acl = ERR_PTR(size);
> +   }
> +   kfree(value);
> +
> +   return acl;
> +}
> +
> +static int fuse_set_acl(struct inode *inode, struct posix_acl *acl,int type)
> +{
> +   const char *name;
> +   int ret;
> +
> +   if (type == ACL_TYPE_ACCESS)
> +      name = XATTR_NAME_POSIX_ACL_ACCESS;
> +   else if (type == ACL_TYPE_DEFAULT)
> +      name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +   else
> +      return -EINVAL;
> +
> +   if (acl) {
> +      struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> +      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(s_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;
> +}
> +
>  static const struct inode_operations fuse_dir_inode_operations = {
>     .lookup      = fuse_lookup,
>     .mkdir      = fuse_mkdir,
> @@ -1879,10 +2012,12 @@ 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,
> +   .get_acl   = fuse_get_acl,
> +   .set_acl   = fuse_set_acl,
>  };
>  
>  static const struct file_operations fuse_dir_operations = {
> @@ -1900,10 +2035,12 @@ 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,
> +   .get_acl   = fuse_get_acl,
> +   .set_acl   = fuse_set_acl,
>  };
>  
>  static const struct inode_operations fuse_symlink_inode_operations = {
> @@ -1911,10 +2048,12 @@ 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,
> +   .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 9f4c3c82edd6..02c08796051e 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -23,6 +23,7 @@
>  #include <linux/poll.h>
>  #include <linux/workqueue.h>
>  #include <linux/kref.h>
> +#include <linux/xattr.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/user_namespace.h>
>  
> @@ -693,6 +694,8 @@ extern const struct file_operations fuse_dev_operations;
>  
>  extern const struct dentry_operations fuse_dentry_operations;
>  
> +extern const struct xattr_handler *fuse_xattr_handlers[];
> +
>  /**
>   * Inode to nodeid comparison.
>   */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 254f1944ee98..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)
> @@ -1066,6 +1068,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;
>
>
> ------------------------------------------------------------------------------
> Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> present their vision of the future. This family event has something for
> everyone, including kids. Get more information and register today.
> http://sdm.link/attshape
> --
> fuse-devel mailing list
> To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> lists/listinfo/fuse-devel
>


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Michael j Theall

More comments:

My patch does set_cached_acl() in fuse_get_acl(). I would investigate if this is necessary.


Since your patch invalidates the cached acls on getattr(), I would investigate on whether you also need to invalidate cached acls on the entries returned by readdirplus.

> There are some indications that fuse supports ACLs on the userspace side
> when default_permissions is not used (though I'm not seeing how that
> works). Will these changes conflict with that support, and if how do we
> avoid those conflicts?


I wouldn't exactly call it "support". Basically by turning off default_permissions, the fuser server can do what it wants permission-wise. My filesystem turns off default_permissions in order to support ACLs, but as far as fuse is concerned, it is totally oblivious and blindly does what my server says is permitted. That being said, there still lies the problem of checking permissions along the path. The low-level interface only gives you the final inode, which in the case of hard links, the path is ambiguous. This means you can't check each path component for permissions (and the kernel certainly won't! since you turned off default_permissions). Therefore the only "correct" way to implement ACL support on the fuse server is by setting the attr/entry timeout to 0 to force the lookup every time, which defeats many purposes of using fuse in the first place. You can read all the gory details in the thread I linked containing my original patch.

Fortunately, this patch (and mine) allows you to use default_permissions *and* have acl support, meaning permission checks will be performed in kernel for each path component. The extra benefit is that it allows any backing filesystem which supports xattrs to support acls, provided your fuse server has unencumbered privileges for that filesystem.

Regards,
Michael Theall

Michael j Theall/Houston/IBM@IBMUS wrote on 06/29/2016 02:24:14 PM:

> From: Michael j Theall/Houston/IBM@IBMUS

> To: Seth Forshee <[hidden email]>
> Cc: [hidden email], [hidden email],
> [hidden email], Szeredi <[hidden email]>

> Date: 06/29/2016 02:37 PM
> Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
>


> Going by the patch I posted a couple of years ago: https://
> sourceforge.net/p/fuse/mailman/message/33033653/
>
> The only hole I see in your patch is that in setattr() you are not
> updating the cached acl if the ATTR_MODE is updated. The other major
> difference is that my version uses the get_acl/set_acl inode
> operations but you use that plus the xattr handlers. I'm not up-to-
> speed on the kernel so I'm not sure if you actually need to implement both.
>
> Regards,
> Michael Theall
>
> Seth Forshee <[hidden email]> wrote on 06/29/2016 02:07:31 PM:
>
> > From: Seth Forshee <[hidden email]>
> > To: [hidden email], [hidden email]
> > Cc: "Eric W. Biederman" <[hidden email]>, Miklos Szeredi
> > <[hidden email]>
> > Date: 06/29/2016 02:08 PM
> > Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
> >
> > Eric and I are working towards adding support for fuse mounts in
> > non-init user namespaces. Towards that end we'd like to add ACL support
> > to fuse as this will allow for a cleaner implementation overall. Below
> > is an initial patch to support this. I'd like to get some general
> > feedback on this patch and ask a couple of specific questions.
> >
> > There are some indications that fuse supports ACLs on the userspace side
> > when default_permissions is not used (though I'm not seeing how that
> > works). Will these changes conflict with that support, and if how do we
> > avoid those conflicts?
> >
> > Are there any places where we need to invalidate cached ACLs that aren't
> > covered in the patch?
> >
> > Thanks,
> > Seth
> >
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 8466e122ee62..2f53b0491159 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/sched.h>
> >  #include <linux/namei.h>
> >  #include <linux/slab.h>
> > +#include <linux/xattr.h>
> > +#include <linux/posix_acl_xattr.h>
> >  
> >  static bool fuse_use_readdirplus(struct inode *dir, struct
> dir_context *ctx)
> >  {
> > @@ -1061,6 +1063,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);
> >  }
> >  
> > @@ -1719,9 +1722,8 @@ 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)
> > +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);
> > @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> > *unused, struct inode *inode,
> >     return err;
> >  }
> >  
> > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
> > -              const char *name, void *value, size_t size)
> > +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);
> > @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> > *entry, char *list, size_t size)
> >     return ret;
> >  }
> >  
> > -static int fuse_removexattr(struct dentry *entry, const char *name)
> > +static int fuse_removexattr(struct inode *inode, const char *name)
> >  {
> > -   struct inode *inode = d_inode(entry);
> >     struct fuse_conn *fc = get_fuse_conn(inode);
> >     FUSE_ARGS(args);
> >     int err;
> > @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> > *entry, const char *name)
> >     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,
> > +};
> > +
> > +#ifndef CONFIG_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;
> > +}
> > +
> > +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> > +   .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > +   .get   = fuse_xattr_acl_get,
> > +   .set   = fuse_xattr_acl_set,
> > +};
> > +
> > +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> > +   .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > +   .get   = fuse_xattr_acl_get,
> > +   .set   = fuse_xattr_acl_set,
> > +};
> > +#endif /* CONFIG_POSIX_ACL */
> > +
> > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > +#ifdef CONFIG_FS_POSIX_ACL
> > +   &posix_acl_access_xattr_handler,
> > +   &posix_acl_default_xattr_handler,
> > +#else
> > +   &fuse_xattr_acl_access_handler,
> > +   &fuse_xattr_acl_default_handler,
> > +#endif
> > +   &fuse_xattr_handler,
> > +};
> > +
> > +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > +{
> > +   int size;
> > +   const char *name;
> > +   void *value = NULL;
> > +   struct posix_acl *acl;
> > +
> > +   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(inode->i_sb->s_user_ns, value, size);
> > +   } else if ((size == 0) || (size == -ENODATA)) {
> > +      acl = NULL;
> > +   } else {
> > +      acl = ERR_PTR(size);
> > +   }
> > +   kfree(value);
> > +
> > +   return acl;
> > +}
> > +
> > +static int fuse_set_acl(struct inode *inode, struct posix_acl
> *acl,int type)
> > +{
> > +   const char *name;
> > +   int ret;
> > +
> > +   if (type == ACL_TYPE_ACCESS)
> > +      name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +   else if (type == ACL_TYPE_DEFAULT)
> > +      name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +   else
> > +      return -EINVAL;
> > +
> > +   if (acl) {
> > +      struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> > +      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(s_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;
> > +}
> > +
> >  static const struct inode_operations fuse_dir_inode_operations = {
> >     .lookup      = fuse_lookup,
> >     .mkdir      = fuse_mkdir,
> > @@ -1879,10 +2012,12 @@ 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,
> > +   .get_acl   = fuse_get_acl,
> > +   .set_acl   = fuse_set_acl,
> >  };
> >  
> >  static const struct file_operations fuse_dir_operations = {
> > @@ -1900,10 +2035,12 @@ 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,
> > +   .get_acl   = fuse_get_acl,
> > +   .set_acl   = fuse_set_acl,
> >  };
> >  
> >  static const struct inode_operations fuse_symlink_inode_operations = {
> > @@ -1911,10 +2048,12 @@ 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,
> > +   .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 9f4c3c82edd6..02c08796051e 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -23,6 +23,7 @@
> >  #include <linux/poll.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/kref.h>
> > +#include <linux/xattr.h>
> >  #include <linux/pid_namespace.h>
> >  #include <linux/user_namespace.h>
> >  
> > @@ -693,6 +694,8 @@ extern const struct file_operations fuse_dev_operations;
> >  
> >  extern const struct dentry_operations fuse_dentry_operations;
> >  
> > +extern const struct xattr_handler *fuse_xattr_handlers[];
> > +
> >  /**
> >   * Inode to nodeid comparison.
> >   */
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 254f1944ee98..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)
> > @@ -1066,6 +1068,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;
> >
> >
> >
> ------------------------------------------------------------------------------
> > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> > present their vision of the future. This family event has something for
> > everyone, including kids. Get more information and register today.
> > http://sdm.link/attshape
> > --
> > fuse-devel mailing list
> > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > lists/listinfo/fuse-devel
> >
> ------------------------------------------------------------------------------
> Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> present their vision of the future. This family event has something for
> everyone, including kids. Get more information and register today.
> http://sdm.link/attshape--
> fuse-devel mailing list
> To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> lists/listinfo/fuse-devel


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Eric W. Biederman
In reply to this post by Michael j Theall
"Michael j Theall" <[hidden email]> writes:

> Going by the patch I posted a couple of years ago:
> https://sourceforge.net/p/fuse/mailman/message/33033653/
>
> The only hole I see in your patch is that in setattr() you are not
> updating the cached acl if the ATTR_MODE is updated. The other major
> difference is that my version uses the get_acl/set_acl inode
> operations but you use that plus the xattr handlers. I'm not
> up-to-speed on the kernel so I'm not sure if you actually need to
> implement both.

That makes an interesting question.  Is it desirable to keep
inode->i_mode in sync with the posix acls in fuse or should a filesystem
that supports posix acls worry about that?

The fact that MS_POSIXACL is set indicates that fuse supports posix
acls.  Have posix acls never worked then?

Eric

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Michael j Theall

Hi,

MS_POSIXACL is only set so that fuse sends doesn't mask the mode sent with open/mkdir/mknod. This is necessary to properly implement POSIX ACLs, along with the umask value which is also sent with these requests.

Regards,
Michael Theall

[hidden email] (Eric W. Biederman) wrote on 06/29/2016 03:18:24 PM:

> From: [hidden email] (Eric W. Biederman)

> To: Michael j Theall/Houston/IBM@IBMUS
> Cc: Seth Forshee <[hidden email]>, fuse-
> [hidden email], [hidden email], Miklos
> Szeredi <[hidden email]>

> Date: 06/29/2016 03:30 PM
> Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
>


> "Michael j Theall" <[hidden email]> writes:
>
> > Going by the patch I posted a couple of years ago:
> > https://sourceforge.net/p/fuse/mailman/message/33033653/
> >
> > The only hole I see in your patch is that in setattr() you are not
> > updating the cached acl if the ATTR_MODE is updated. The other major
> > difference is that my version uses the get_acl/set_acl inode
> > operations but you use that plus the xattr handlers. I'm not
> > up-to-speed on the kernel so I'm not sure if you actually need to
> > implement both.
>
> That makes an interesting question.  Is it desirable to keep
> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> that supports posix acls worry about that?
>
> The fact that MS_POSIXACL is set indicates that fuse supports posix
> acls.  Have posix acls never worked then?
>
> Eric
>


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
In reply to this post by Michael j Theall
On Wed, Jun 29, 2016 at 02:24:14PM -0500, Michael j Theall wrote:
>
> Going by the patch I posted a couple of years ago:
> https://sourceforge.net/p/fuse/mailman/message/33033653/
>
> The only hole I see in your patch is that in setattr() you are not updating
> the cached acl if the ATTR_MODE is updated.

Okay, thanks.

> The other major difference is
> that my version uses the get_acl/set_acl inode operations but you use that
> plus the xattr handlers. I'm not up-to-speed on the kernel so I'm not sure
> if you actually need to implement both.

The xattr handlers use the get_acl/set_acl callbacks. Without using the
xattr handlers acl xattr reads won't be pulled from the acl cache, for
one thing.

Thanks,
Seth

>
> Regards,
> Michael Theall
>
> Seth Forshee <[hidden email]> wrote on 06/29/2016 02:07:31 PM:
>
> > From: Seth Forshee <[hidden email]>
> > To: [hidden email], [hidden email]
> > Cc: "Eric W. Biederman" <[hidden email]>, Miklos Szeredi
> > <[hidden email]>
> > Date: 06/29/2016 02:08 PM
> > Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
> >
> > Eric and I are working towards adding support for fuse mounts in
> > non-init user namespaces. Towards that end we'd like to add ACL support
> > to fuse as this will allow for a cleaner implementation overall. Below
> > is an initial patch to support this. I'd like to get some general
> > feedback on this patch and ask a couple of specific questions.
> >
> > There are some indications that fuse supports ACLs on the userspace side
> > when default_permissions is not used (though I'm not seeing how that
> > works). Will these changes conflict with that support, and if how do we
> > avoid those conflicts?
> >
> > Are there any places where we need to invalidate cached ACLs that aren't
> > covered in the patch?
> >
> > Thanks,
> > Seth
> >
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 8466e122ee62..2f53b0491159 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/sched.h>
> >  #include <linux/namei.h>
> >  #include <linux/slab.h>
> > +#include <linux/xattr.h>
> > +#include <linux/posix_acl_xattr.h>
> >
> >  static bool fuse_use_readdirplus(struct inode *dir, struct dir_context
> *ctx)
> >  {
> > @@ -1061,6 +1063,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);
> >  }
> >
> > @@ -1719,9 +1722,8 @@ 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)
> > +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);
> > @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> > *unused, struct inode *inode,
> >     return err;
> >  }
> >
> > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
> > -              const char *name, void *value, size_t size)
> > +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);
> > @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> > *entry, char *list, size_t size)
> >     return ret;
> >  }
> >
> > -static int fuse_removexattr(struct dentry *entry, const char *name)
> > +static int fuse_removexattr(struct inode *inode, const char *name)
> >  {
> > -   struct inode *inode = d_inode(entry);
> >     struct fuse_conn *fc = get_fuse_conn(inode);
> >     FUSE_ARGS(args);
> >     int err;
> > @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> > *entry, const char *name)
> >     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,
> > +};
> > +
> > +#ifndef CONFIG_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;
> > +}
> > +
> > +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> > +   .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > +   .get   = fuse_xattr_acl_get,
> > +   .set   = fuse_xattr_acl_set,
> > +};
> > +
> > +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> > +   .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > +   .get   = fuse_xattr_acl_get,
> > +   .set   = fuse_xattr_acl_set,
> > +};
> > +#endif /* CONFIG_POSIX_ACL */
> > +
> > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > +#ifdef CONFIG_FS_POSIX_ACL
> > +   &posix_acl_access_xattr_handler,
> > +   &posix_acl_default_xattr_handler,
> > +#else
> > +   &fuse_xattr_acl_access_handler,
> > +   &fuse_xattr_acl_default_handler,
> > +#endif
> > +   &fuse_xattr_handler,
> > +};
> > +
> > +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > +{
> > +   int size;
> > +   const char *name;
> > +   void *value = NULL;
> > +   struct posix_acl *acl;
> > +
> > +   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(inode->i_sb->s_user_ns, value, size);
> > +   } else if ((size == 0) || (size == -ENODATA)) {
> > +      acl = NULL;
> > +   } else {
> > +      acl = ERR_PTR(size);
> > +   }
> > +   kfree(value);
> > +
> > +   return acl;
> > +}
> > +
> > +static int fuse_set_acl(struct inode *inode, struct posix_acl *acl,int
> type)
> > +{
> > +   const char *name;
> > +   int ret;
> > +
> > +   if (type == ACL_TYPE_ACCESS)
> > +      name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +   else if (type == ACL_TYPE_DEFAULT)
> > +      name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +   else
> > +      return -EINVAL;
> > +
> > +   if (acl) {
> > +      struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> > +      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(s_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;
> > +}
> > +
> >  static const struct inode_operations fuse_dir_inode_operations = {
> >     .lookup      = fuse_lookup,
> >     .mkdir      = fuse_mkdir,
> > @@ -1879,10 +2012,12 @@ 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,
> > +   .get_acl   = fuse_get_acl,
> > +   .set_acl   = fuse_set_acl,
> >  };
> >
> >  static const struct file_operations fuse_dir_operations = {
> > @@ -1900,10 +2035,12 @@ 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,
> > +   .get_acl   = fuse_get_acl,
> > +   .set_acl   = fuse_set_acl,
> >  };
> >
> >  static const struct inode_operations fuse_symlink_inode_operations = {
> > @@ -1911,10 +2048,12 @@ 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,
> > +   .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 9f4c3c82edd6..02c08796051e 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -23,6 +23,7 @@
> >  #include <linux/poll.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/kref.h>
> > +#include <linux/xattr.h>
> >  #include <linux/pid_namespace.h>
> >  #include <linux/user_namespace.h>
> >
> > @@ -693,6 +694,8 @@ extern const struct file_operations
> fuse_dev_operations;
> >
> >  extern const struct dentry_operations fuse_dentry_operations;
> >
> > +extern const struct xattr_handler *fuse_xattr_handlers[];
> > +
> >  /**
> >   * Inode to nodeid comparison.
> >   */
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 254f1944ee98..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)
> > @@ -1066,6 +1068,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;
> >
> >
> >
> ------------------------------------------------------------------------------
>
> > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> > present their vision of the future. This family event has something for
> > everyone, including kids. Get more information and register today.
> > http://sdm.link/attshape
> > --
> > fuse-devel mailing list
> > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > lists/listinfo/fuse-devel
> >

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
In reply to this post by Michael j Theall
On Wed, Jun 29, 2016 at 02:52:37PM -0500, Michael j Theall wrote:
>
> More comments:
>
> My patch does set_cached_acl() in fuse_get_acl(). I would investigate if
> this is necessary.

Hmm, on the one hand it looks like a lot of filesystems do call it, on
the other hand it looks like get_acl is caching it. I'll have to take a
closer look.

> Since your patch invalidates the cached acls on getattr(), I would
> investigate on whether you also need to invalidate cached acls on the
> entries returned by readdirplus.

Possibly so. I missed the fact that fuse_direntplus_link might update
the inode attributes.

> > There are some indications that fuse supports ACLs on the userspace side
> > when default_permissions is not used (though I'm not seeing how that
> > works). Will these changes conflict with that support, and if how do we
> > avoid those conflicts?
>
> I wouldn't exactly call it "support". Basically by turning off
> default_permissions, the fuser server can do what it wants permission-wise.
> My filesystem turns off default_permissions in order to support ACLs, but
> as far as fuse is concerned, it is totally oblivious and blindly does what
> my server says is permitted. That being said, there still lies the problem
> of checking permissions along the path. The low-level interface only gives
> you the final inode, which in the case of hard links, the path is
> ambiguous. This means you can't check each path component for permissions
> (and the kernel certainly won't! since you turned off default_permissions).
> Therefore the only "correct" way to implement ACL support on the fuse
> server is by setting the attr/entry timeout to 0 to force the lookup every
> time, which defeats many purposes of using fuse in the first place. You can
> read all the gory details in the thread I linked containing my original
> patch.
>
> Fortunately, this patch (and mine) allows you to use default_permissions
> *and* have acl support, meaning permission checks will be performed in
> kernel for each path component. The extra benefit is that it allows any
> backing filesystem which supports xattrs to support acls, provided your
> fuse server has unencumbered privileges for that filesystem.

But if we're going through the posix acl xattr handlers then the acls
will be cached. What I worry about is that somehow we could end up in a
situation where the cached acls don't match those that the filesystem is
using, and userspace sees one thing in response to getfacl but something
different is being enforced.

Thanks,
Seth

>
> Regards,
> Michael Theall
>
> Michael j Theall/Houston/IBM@IBMUS wrote on 06/29/2016 02:24:14 PM:
>
> > From: Michael j Theall/Houston/IBM@IBMUS
> > To: Seth Forshee <[hidden email]>
> > Cc: [hidden email], [hidden email],
> > [hidden email], Szeredi <[hidden email]>
> > Date: 06/29/2016 02:37 PM
> > Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
> >
> > Going by the patch I posted a couple of years ago: https://
> > sourceforge.net/p/fuse/mailman/message/33033653/
> >
> > The only hole I see in your patch is that in setattr() you are not
> > updating the cached acl if the ATTR_MODE is updated. The other major
> > difference is that my version uses the get_acl/set_acl inode
> > operations but you use that plus the xattr handlers. I'm not up-to-
> > speed on the kernel so I'm not sure if you actually need to implement
> both.
> >
> > Regards,
> > Michael Theall
> >
> > Seth Forshee <[hidden email]> wrote on 06/29/2016 02:07:31
> PM:
> >
> > > From: Seth Forshee <[hidden email]>
> > > To: [hidden email], [hidden email]
> > > Cc: "Eric W. Biederman" <[hidden email]>, Miklos Szeredi
> > > <[hidden email]>
> > > Date: 06/29/2016 02:08 PM
> > > Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
> > >
> > > Eric and I are working towards adding support for fuse mounts in
> > > non-init user namespaces. Towards that end we'd like to add ACL support
> > > to fuse as this will allow for a cleaner implementation overall. Below
> > > is an initial patch to support this. I'd like to get some general
> > > feedback on this patch and ask a couple of specific questions.
> > >
> > > There are some indications that fuse supports ACLs on the userspace
> side
> > > when default_permissions is not used (though I'm not seeing how that
> > > works). Will these changes conflict with that support, and if how do we
> > > avoid those conflicts?
> > >
> > > Are there any places where we need to invalidate cached ACLs that
> aren't
> > > covered in the patch?
> > >
> > > Thanks,
> > > Seth
> > >
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 8466e122ee62..2f53b0491159 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -13,6 +13,8 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/namei.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/xattr.h>
> > > +#include <linux/posix_acl_xattr.h>
> > >
> > >  static bool fuse_use_readdirplus(struct inode *dir, struct
> > dir_context *ctx)
> > >  {
> > > @@ -1061,6 +1063,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);
> > >  }
> > >
> > > @@ -1719,9 +1722,8 @@ 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)
> > > +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);
> > > @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> > > *unused, struct inode *inode,
> > >     return err;
> > >  }
> > >
> > > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode
> *inode,
> > > -              const char *name, void *value, size_t size)
> > > +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);
> > > @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> > > *entry, char *list, size_t size)
> > >     return ret;
> > >  }
> > >
> > > -static int fuse_removexattr(struct dentry *entry, const char *name)
> > > +static int fuse_removexattr(struct inode *inode, const char *name)
> > >  {
> > > -   struct inode *inode = d_inode(entry);
> > >     struct fuse_conn *fc = get_fuse_conn(inode);
> > >     FUSE_ARGS(args);
> > >     int err;
> > > @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> > > *entry, const char *name)
> > >     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,
> > > +};
> > > +
> > > +#ifndef CONFIG_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;
> > > +}
> > > +
> > > +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> > > +   .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > > +   .get   = fuse_xattr_acl_get,
> > > +   .set   = fuse_xattr_acl_set,
> > > +};
> > > +
> > > +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> > > +   .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > > +   .get   = fuse_xattr_acl_get,
> > > +   .set   = fuse_xattr_acl_set,
> > > +};
> > > +#endif /* CONFIG_POSIX_ACL */
> > > +
> > > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > > +#ifdef CONFIG_FS_POSIX_ACL
> > > +   &posix_acl_access_xattr_handler,
> > > +   &posix_acl_default_xattr_handler,
> > > +#else
> > > +   &fuse_xattr_acl_access_handler,
> > > +   &fuse_xattr_acl_default_handler,
> > > +#endif
> > > +   &fuse_xattr_handler,
> > > +};
> > > +
> > > +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > > +{
> > > +   int size;
> > > +   const char *name;
> > > +   void *value = NULL;
> > > +   struct posix_acl *acl;
> > > +
> > > +   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(inode->i_sb->s_user_ns, value, size);
> > > +   } else if ((size == 0) || (size == -ENODATA)) {
> > > +      acl = NULL;
> > > +   } else {
> > > +      acl = ERR_PTR(size);
> > > +   }
> > > +   kfree(value);
> > > +
> > > +   return acl;
> > > +}
> > > +
> > > +static int fuse_set_acl(struct inode *inode, struct posix_acl
> > *acl,int type)
> > > +{
> > > +   const char *name;
> > > +   int ret;
> > > +
> > > +   if (type == ACL_TYPE_ACCESS)
> > > +      name = XATTR_NAME_POSIX_ACL_ACCESS;
> > > +   else if (type == ACL_TYPE_DEFAULT)
> > > +      name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > > +   else
> > > +      return -EINVAL;
> > > +
> > > +   if (acl) {
> > > +      struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> > > +      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(s_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;
> > > +}
> > > +
> > >  static const struct inode_operations fuse_dir_inode_operations = {
> > >     .lookup      = fuse_lookup,
> > >     .mkdir      = fuse_mkdir,
> > > @@ -1879,10 +2012,12 @@ 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,
> > > +   .get_acl   = fuse_get_acl,
> > > +   .set_acl   = fuse_set_acl,
> > >  };
> > >
> > >  static const struct file_operations fuse_dir_operations = {
> > > @@ -1900,10 +2035,12 @@ 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,
> > > +   .get_acl   = fuse_get_acl,
> > > +   .set_acl   = fuse_set_acl,
> > >  };
> > >
> > >  static const struct inode_operations fuse_symlink_inode_operations = {
> > > @@ -1911,10 +2048,12 @@ 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,
> > > +   .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 9f4c3c82edd6..02c08796051e 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/poll.h>
> > >  #include <linux/workqueue.h>
> > >  #include <linux/kref.h>
> > > +#include <linux/xattr.h>
> > >  #include <linux/pid_namespace.h>
> > >  #include <linux/user_namespace.h>
> > >
> > > @@ -693,6 +694,8 @@ extern const struct file_operations
> fuse_dev_operations;
> > >
> > >  extern const struct dentry_operations fuse_dentry_operations;
> > >
> > > +extern const struct xattr_handler *fuse_xattr_handlers[];
> > > +
> > >  /**
> > >   * Inode to nodeid comparison.
> > >   */
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index 254f1944ee98..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)
> > > @@ -1066,6 +1068,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;
> > >
> > >
> > >
> >
> ------------------------------------------------------------------------------
>
> > > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > > Francisco, CA to explore cutting-edge tech and listen to tech
> luminaries
> > > present their vision of the future. This family event has something for
> > > everyone, including kids. Get more information and register today.
> > > http://sdm.link/attshape
> > > --
> > > fuse-devel mailing list
> > > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > > lists/listinfo/fuse-devel
> > >
> >
> ------------------------------------------------------------------------------
>
> > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> > present their vision of the future. This family event has something for
> > everyone, including kids. Get more information and register today.
> > http://sdm.link/attshape--
> > fuse-devel mailing list
> > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > lists/listinfo/fuse-devel

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Michael j Theall

Seth Forshee <[hidden email]> wrote on 06/29/2016 04:03:18 PM:

> From: Seth Forshee <[hidden email]>

> To: Michael j Theall/Houston/IBM@IBMUS
> Cc: [hidden email], [hidden email],
> Szeredi <[hidden email]>, [hidden email]

> Date: 06/29/2016 04:03 PM
> Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
>


> On Wed, Jun 29, 2016 at 02:52:37PM -0500, Michael j Theall wrote:
> >
> > More comments:
> >
> > My patch does set_cached_acl() in fuse_get_acl(). I would investigate if
> > this is necessary.
>
> Hmm, on the one hand it looks like a lot of filesystems do call it, on
> the other hand it looks like get_acl is caching it. I'll have to take a
> closer look.
>
> > Since your patch invalidates the cached acls on getattr(), I would
> > investigate on whether you also need to invalidate cached acls on the
> > entries returned by readdirplus.
>
> Possibly so. I missed the fact that fuse_direntplus_link might update
> the inode attributes.
>
> > > There are some indications that fuse supports ACLs on the userspace side
> > > when default_permissions is not used (though I'm not seeing how that
> > > works). Will these changes conflict with that support, and if how do we
> > > avoid those conflicts?
> >
> > I wouldn't exactly call it "support". Basically by turning off
> > default_permissions, the fuser server can do what it wants permission-wise.
> > My filesystem turns off default_permissions in order to support ACLs, but
> > as far as fuse is concerned, it is totally oblivious and blindly does what
> > my server says is permitted. That being said, there still lies the problem
> > of checking permissions along the path. The low-level interface only gives
> > you the final inode, which in the case of hard links, the path is
> > ambiguous. This means you can't check each path component for permissions
> > (and the kernel certainly won't! since you turned off default_permissions).
> > Therefore the only "correct" way to implement ACL support on the fuse
> > server is by setting the attr/entry timeout to 0 to force the lookup every
> > time, which defeats many purposes of using fuse in the first place. You can
> > read all the gory details in the thread I linked containing my original
> > patch.
> >
> > Fortunately, this patch (and mine) allows you to use default_permissions
> > *and* have acl support, meaning permission checks will be performed in
> > kernel for each path component. The extra benefit is that it allows any
> > backing filesystem which supports xattrs to support acls, provided your
> > fuse server has unencumbered privileges for that filesystem.
>
> But if we're going through the posix acl xattr handlers then the acls
> will be cached. What I worry about is that somehow we could end up in a
> situation where the cached acls don't match those that the filesystem is
> using, and userspace sees one thing in response to getfacl but something
> different is being enforced.
>

This isn't any different from the unix permissions. This is the price you pay for caching any attributes. If it's a problem, the server can reduce the cache timeout, or eliminate caching by setting the timeout to 0.

Regards,
Michael Theall

> Thanks,
> Seth
>
> >
> > Regards,
> > Michael Theall
> >
> > Michael j Theall/Houston/IBM@IBMUS wrote on 06/29/2016 02:24:14 PM:
> >
> > > From: Michael j Theall/Houston/IBM@IBMUS
> > > To: Seth Forshee <[hidden email]>
> > > Cc: [hidden email], [hidden email],
> > > [hidden email], Szeredi <[hidden email]>
> > > Date: 06/29/2016 02:37 PM
> > > Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
> > >
> > > Going by the patch I posted a couple of years ago: https://
> > > sourceforge.net/p/fuse/mailman/message/33033653/
> > >
> > > The only hole I see in your patch is that in setattr() you are not
> > > updating the cached acl if the ATTR_MODE is updated. The other major
> > > difference is that my version uses the get_acl/set_acl inode
> > > operations but you use that plus the xattr handlers. I'm not up-to-
> > > speed on the kernel so I'm not sure if you actually need to implement
> > both.
> > >
> > > Regards,
> > > Michael Theall
> > >
> > > Seth Forshee <[hidden email]> wrote on 06/29/2016 02:07:31
> > PM:
> > >
> > > > From: Seth Forshee <[hidden email]>
> > > > To: [hidden email], [hidden email]
> > > > Cc: "Eric W. Biederman" <[hidden email]>, Miklos Szeredi
> > > > <[hidden email]>
> > > > Date: 06/29/2016 02:08 PM
> > > > Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
> > > >
> > > > Eric and I are working towards adding support for fuse mounts in
> > > > non-init user namespaces. Towards that end we'd like to add ACL support
> > > > to fuse as this will allow for a cleaner implementation overall. Below
> > > > is an initial patch to support this. I'd like to get some general
> > > > feedback on this patch and ask a couple of specific questions.
> > > >
> > > > There are some indications that fuse supports ACLs on the userspace
> > side
> > > > when default_permissions is not used (though I'm not seeing how that
> > > > works). Will these changes conflict with that support, and if how do we
> > > > avoid those conflicts?
> > > >
> > > > Are there any places where we need to invalidate cached ACLs that
> > aren't
> > > > covered in the patch?
> > > >
> > > > Thanks,
> > > > Seth
> > > >
> > > >
> > > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > > index 8466e122ee62..2f53b0491159 100644
> > > > --- a/fs/fuse/dir.c
> > > > +++ b/fs/fuse/dir.c
> > > > @@ -13,6 +13,8 @@
> > > >  #include <linux/sched.h>
> > > >  #include <linux/namei.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/xattr.h>
> > > > +#include <linux/posix_acl_xattr.h>
> > > >
> > > >  static bool fuse_use_readdirplus(struct inode *dir, struct
> > > dir_context *ctx)
> > > >  {
> > > > @@ -1061,6 +1063,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);
> > > >  }
> > > >
> > > > @@ -1719,9 +1722,8 @@ 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)
> > > > +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);
> > > > @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> > > > *unused, struct inode *inode,
> > > >     return err;
> > > >  }
> > > >
> > > > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode
> > *inode,
> > > > -              const char *name, void *value, size_t size)
> > > > +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);
> > > > @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> > > > *entry, char *list, size_t size)
> > > >     return ret;
> > > >  }
> > > >
> > > > -static int fuse_removexattr(struct dentry *entry, const char *name)
> > > > +static int fuse_removexattr(struct inode *inode, const char *name)
> > > >  {
> > > > -   struct inode *inode = d_inode(entry);
> > > >     struct fuse_conn *fc = get_fuse_conn(inode);
> > > >     FUSE_ARGS(args);
> > > >     int err;
> > > > @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> > > > *entry, const char *name)
> > > >     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,
> > > > +};
> > > > +
> > > > +#ifndef CONFIG_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;
> > > > +}
> > > > +
> > > > +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> > > > +   .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > > > +   .get   = fuse_xattr_acl_get,
> > > > +   .set   = fuse_xattr_acl_set,
> > > > +};
> > > > +
> > > > +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> > > > +   .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > > > +   .get   = fuse_xattr_acl_get,
> > > > +   .set   = fuse_xattr_acl_set,
> > > > +};
> > > > +#endif /* CONFIG_POSIX_ACL */
> > > > +
> > > > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > > > +#ifdef CONFIG_FS_POSIX_ACL
> > > > +   &posix_acl_access_xattr_handler,
> > > > +   &posix_acl_default_xattr_handler,
> > > > +#else
> > > > +   &fuse_xattr_acl_access_handler,
> > > > +   &fuse_xattr_acl_default_handler,
> > > > +#endif
> > > > +   &fuse_xattr_handler,
> > > > +};
> > > > +
> > > > +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > > > +{
> > > > +   int size;
> > > > +   const char *name;
> > > > +   void *value = NULL;
> > > > +   struct posix_acl *acl;
> > > > +
> > > > +   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(inode->i_sb->s_user_ns, value, size);
> > > > +   } else if ((size == 0) || (size == -ENODATA)) {
> > > > +      acl = NULL;
> > > > +   } else {
> > > > +      acl = ERR_PTR(size);
> > > > +   }
> > > > +   kfree(value);
> > > > +
> > > > +   return acl;
> > > > +}
> > > > +
> > > > +static int fuse_set_acl(struct inode *inode, struct posix_acl
> > > *acl,int type)
> > > > +{
> > > > +   const char *name;
> > > > +   int ret;
> > > > +
> > > > +   if (type == ACL_TYPE_ACCESS)
> > > > +      name = XATTR_NAME_POSIX_ACL_ACCESS;
> > > > +   else if (type == ACL_TYPE_DEFAULT)
> > > > +      name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > > > +   else
> > > > +      return -EINVAL;
> > > > +
> > > > +   if (acl) {
> > > > +      struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> > > > +      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(s_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;
> > > > +}
> > > > +
> > > >  static const struct inode_operations fuse_dir_inode_operations = {
> > > >     .lookup      = fuse_lookup,
> > > >     .mkdir      = fuse_mkdir,
> > > > @@ -1879,10 +2012,12 @@ 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,
> > > > +   .get_acl   = fuse_get_acl,
> > > > +   .set_acl   = fuse_set_acl,
> > > >  };
> > > >
> > > >  static const struct file_operations fuse_dir_operations = {
> > > > @@ -1900,10 +2035,12 @@ 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,
> > > > +   .get_acl   = fuse_get_acl,
> > > > +   .set_acl   = fuse_set_acl,
> > > >  };
> > > >
> > > >  static const struct inode_operations fuse_symlink_inode_operations = {
> > > > @@ -1911,10 +2048,12 @@ 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,
> > > > +   .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 9f4c3c82edd6..02c08796051e 100644
> > > > --- a/fs/fuse/fuse_i.h
> > > > +++ b/fs/fuse/fuse_i.h
> > > > @@ -23,6 +23,7 @@
> > > >  #include <linux/poll.h>
> > > >  #include <linux/workqueue.h>
> > > >  #include <linux/kref.h>
> > > > +#include <linux/xattr.h>
> > > >  #include <linux/pid_namespace.h>
> > > >  #include <linux/user_namespace.h>
> > > >
> > > > @@ -693,6 +694,8 @@ extern const struct file_operations
> > fuse_dev_operations;
> > > >
> > > >  extern const struct dentry_operations fuse_dentry_operations;
> > > >
> > > > +extern const struct xattr_handler *fuse_xattr_handlers[];
> > > > +
> > > >  /**
> > > >   * Inode to nodeid comparison.
> > > >   */
> > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > index 254f1944ee98..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)
> > > > @@ -1066,6 +1068,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;
> > > >
> > > >
> > > >
> > >
> >
> ------------------------------------------------------------------------------
> >
> > > > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > > > Francisco, CA to explore cutting-edge tech and listen to tech
> > luminaries
> > > > present their vision of the future. This family event has something for
> > > > everyone, including kids. Get more information and register today.
> > > > http://sdm.link/attshape
> > > > --
> > > > fuse-devel mailing list
> > > > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > > > lists/listinfo/fuse-devel
> > > >
> > >
> >
> ------------------------------------------------------------------------------
> >
> > > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > > Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> > > present their vision of the future. This family event has something for
> > > everyone, including kids. Get more information and register today.
> > > http://sdm.link/attshape--
> > > fuse-devel mailing list
> > > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > > lists/listinfo/fuse-devel
>


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Jean-Pierre André
In reply to this post by Seth Forshee
Seth Forshee wrote:
> Eric and I are working towards adding support for fuse mounts in
> non-init user namespaces. Towards that end we'd like to add ACL support
> to fuse as this will allow for a cleaner implementation overall. Below

My best wishes go with you.

> is an initial patch to support this. I'd like to get some general
> feedback on this patch and ask a couple of specific questions.
>
> There are some indications that fuse supports ACLs on the userspace side
> when default_permissions is not used (though I'm not seeing how that
> works). Will these changes conflict with that support, and if how do we
> avoid those conflicts?

ntfs-3g has both variants implemented. When supporting ACLs
within the userspace, it does not set default_permissions,
and it uses null cache timeouts.

When expecting ACLs supported at the kernel level, it sets
default_permissions and it uses non_null cache timeouts.

It sets FUSE_CAP_DONT_MASK in both cases.

I would expect default_permissions to make a clear divide
between those conditions, avoiding any conflicts.

Jean-Pierre

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Jean-Pierre André
In reply to this post by Eric W. Biederman
[hidden email] (Eric W. Biederman) wrote:

> "Michael j Theall" <[hidden email]> writes:
>
>> Going by the patch I posted a couple of years ago:
>> https://sourceforge.net/p/fuse/mailman/message/33033653/
>>
>> The only hole I see in your patch is that in setattr() you are not
>> updating the cached acl if the ATTR_MODE is updated. The other major
>> difference is that my version uses the get_acl/set_acl inode
>> operations but you use that plus the xattr handlers. I'm not
>> up-to-speed on the kernel so I'm not sure if you actually need to
>> implement both.
>
> That makes an interesting question.  Is it desirable to keep
> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> that supports posix acls worry about that?

Using a former implementation of ACLs within fuse at the
kernel level, I got the result below.
File systems expect consistency.

# Using the low level interface of fuse, with use of ACLs
# intended to be checked in the kernel, but not related to
# access control
rm -rf trydir
mkdir trydir
echo file > trydir/file
ls -l trydir/file
setfacl -m 'u::7,g::5,o::5' trydir/file
ls -l trydir/file
sleep 1
ls -l trydir/file

-rw-r--r-- 1 root root 5 2009-09-12 12:02 trydir/file
-rw-r--r-- 1 root root 5 2009-09-12 12:02 trydir/file
-rwxr-xr-x 1 root root 5 2009-09-12 12:02 trydir/file

Jean-Pierre


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
In reply to this post by Eric W. Biederman
On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:

> "Michael j Theall" <[hidden email]> writes:
>
> > Going by the patch I posted a couple of years ago:
> > https://sourceforge.net/p/fuse/mailman/message/33033653/
> >
> > The only hole I see in your patch is that in setattr() you are not
> > updating the cached acl if the ATTR_MODE is updated. The other major
> > difference is that my version uses the get_acl/set_acl inode
> > operations but you use that plus the xattr handlers. I'm not
> > up-to-speed on the kernel so I'm not sure if you actually need to
> > implement both.
>
> That makes an interesting question.  Is it desirable to keep
> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> that supports posix acls worry about that?

My first blush opinion is that the kernel should take care of this, not
the filesystems. Then a fuse filesystem which supports xattrs gets acl
support for free. Otherwise if a filesystem supports xattrs but not acls
internally, we have no way of knowing that in the kernel and they get
out of sync.

However if some filesystems are already doing this internally then we
have redundancy. Presumably this would be harmless aside from the wasted
effort.

Seth

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Eric W. Biederman
Seth Forshee <[hidden email]> writes:

> On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
>> "Michael j Theall" <[hidden email]> writes:
>>
>> > Going by the patch I posted a couple of years ago:
>> > https://sourceforge.net/p/fuse/mailman/message/33033653/
>> >
>> > The only hole I see in your patch is that in setattr() you are not
>> > updating the cached acl if the ATTR_MODE is updated. The other major
>> > difference is that my version uses the get_acl/set_acl inode
>> > operations but you use that plus the xattr handlers. I'm not
>> > up-to-speed on the kernel so I'm not sure if you actually need to
>> > implement both.
>>
>> That makes an interesting question.  Is it desirable to keep
>> inode->i_mode in sync with the posix acls in fuse or should a filesystem
>> that supports posix acls worry about that?
>
> My first blush opinion is that the kernel should take care of this, not
> the filesystems. Then a fuse filesystem which supports xattrs gets acl
> support for free. Otherwise if a filesystem supports xattrs but not acls
> internally, we have no way of knowing that in the kernel and they get
> out of sync.
>
> However if some filesystems are already doing this internally then we
> have redundancy. Presumably this would be harmless aside from the wasted
> effort.

Which means that in set_acl we need to something like:

        if (type == ACL_TYPE_ACCESS) {
        struct iattr attr;
                attr.ia_valid = ATTR_MODE;
                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 (attr.ia_mode != inode->i_mode) {
                        ret = fuse_do_setattr(inode, &attr, NULL);
                        if (ret < 0)
                return ret;
                }
        }

In fuse_setattr should wind up looking something like:

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)
                ret = fuse_do_setattr(inode, attr, attr->ia_file);
        else
                ret = fuse_do_setattr(inode, attr, NULL);

        if (ret == 0 && attr->ia_valid & ATTR_MODE)
        ret = posix_acl_chmod(inode, inode->i_mode);
        return ret;
}

That should be enough to keep everything in sync with the existing
fuse protocol.  And then fuse filesystems won't have to care in general
about the contents of acls (unless they choose to care).

Eric

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
On Thu, Jun 30, 2016 at 11:25:32AM -0500, Eric W. Biederman wrote:

> Seth Forshee <[hidden email]> writes:
>
> > On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
> >> "Michael j Theall" <[hidden email]> writes:
> >>
> >> > Going by the patch I posted a couple of years ago:
> >> > https://sourceforge.net/p/fuse/mailman/message/33033653/
> >> >
> >> > The only hole I see in your patch is that in setattr() you are not
> >> > updating the cached acl if the ATTR_MODE is updated. The other major
> >> > difference is that my version uses the get_acl/set_acl inode
> >> > operations but you use that plus the xattr handlers. I'm not
> >> > up-to-speed on the kernel so I'm not sure if you actually need to
> >> > implement both.
> >>
> >> That makes an interesting question.  Is it desirable to keep
> >> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> >> that supports posix acls worry about that?
> >
> > My first blush opinion is that the kernel should take care of this, not
> > the filesystems. Then a fuse filesystem which supports xattrs gets acl
> > support for free. Otherwise if a filesystem supports xattrs but not acls
> > internally, we have no way of knowing that in the kernel and they get
> > out of sync.
> >
> > However if some filesystems are already doing this internally then we
> > have redundancy. Presumably this would be harmless aside from the wasted
> > effort.
>
> Which means that in set_acl we need to something like:
>
> if (type == ACL_TYPE_ACCESS) {
>         struct iattr attr;
>                 attr.ia_valid = ATTR_MODE;
>                 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 (attr.ia_mode != inode->i_mode) {
> ret = fuse_do_setattr(inode, &attr, NULL);
>                if (ret < 0)
>                 return ret;
>                 }
>         }
>
> In fuse_setattr should wind up looking something like:
>
> 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)
> ret = fuse_do_setattr(inode, attr, attr->ia_file);
> else
> ret = fuse_do_setattr(inode, attr, NULL);
>
> if (ret == 0 && attr->ia_valid & ATTR_MODE)
>         ret = posix_acl_chmod(inode, inode->i_mode);
> return ret;
> }
>
> That should be enough to keep everything in sync with the existing
> fuse protocol.  And then fuse filesystems won't have to care in general
> about the contents of acls (unless they choose to care).

Yes, I've already written pretty much the same code and am attempting to
test it.  The problem I'm having is finding a good filesystem to test
with. fusexmp works but is probably unfair as the underlying filesystem
is handling the acls and updating the mode. I haven't found any
filesystem yet that fully supports xattrs but doesn't do something
special with the posix acl xattrs.

Can anyone suggest a good filesystem for me to test with?

Thanks,
Seth

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Michael j Theall

> From: Seth Forshee <[hidden email]>
> To: "Eric W. Biederman" <[hidden email]>
> Cc: Michael j Theall/Houston/IBM@IBMUS, fuse-
> [hidden email], [hidden email], Miklos
> Szeredi <[hidden email]>

> Date: 06/30/2016 11:54 AM
> Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
>


> On Thu, Jun 30, 2016 at 11:25:32AM -0500, Eric W. Biederman wrote:
> > Seth Forshee <[hidden email]> writes:
> >
> > > On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
> > >> "Michael j Theall" <[hidden email]> writes:
> > >>
> > >> > Going by the patch I posted a couple of years ago:
> > >> > https://sourceforge.net/p/fuse/mailman/message/33033653/
> > >> >
> > >> > The only hole I see in your patch is that in setattr() you are not
> > >> > updating the cached acl if the ATTR_MODE is updated. The other major
> > >> > difference is that my version uses the get_acl/set_acl inode
> > >> > operations but you use that plus the xattr handlers. I'm not
> > >> > up-to-speed on the kernel so I'm not sure if you actually need to
> > >> > implement both.
> > >>
> > >> That makes an interesting question.  Is it desirable to keep
> > >> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> > >> that supports posix acls worry about that?
> > >
> > > My first blush opinion is that the kernel should take care of this, not
> > > the filesystems. Then a fuse filesystem which supports xattrs gets acl
> > > support for free. Otherwise if a filesystem supports xattrs but not acls
> > > internally, we have no way of knowing that in the kernel and they get
> > > out of sync.
> > >
> > > However if some filesystems are already doing this internally then we
> > > have redundancy. Presumably this would be harmless aside from the wasted
> > > effort.
> >
> > Which means that in set_acl we need to something like:
> >
> >    if (type == ACL_TYPE_ACCESS) {
> >            struct iattr attr;
> >                 attr.ia_valid = ATTR_MODE;
> >                 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 (attr.ia_mode != inode->i_mode) {
> >          ret = fuse_do_setattr(inode, &attr, NULL);
> >                    if (ret < 0)
> >                       return ret;
> >                 }
> >         }
> >
> > In fuse_setattr should wind up looking something like:
> >
> > 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)
> >       ret = fuse_do_setattr(inode, attr, attr->ia_file);
> >    else
> >       ret = fuse_do_setattr(inode, attr, NULL);
> >
> >    if (ret == 0 && attr->ia_valid & ATTR_MODE)
> >            ret = posix_acl_chmod(inode, inode->i_mode);
> >    return ret;
> > }
> >
> > That should be enough to keep everything in sync with the existing
> > fuse protocol.  And then fuse filesystems won't have to care in general
> > about the contents of acls (unless they choose to care).
>
> Yes, I've already written pretty much the same code and am attempting to
> test it.  The problem I'm having is finding a good filesystem to test
> with. fusexmp works but is probably unfair as the underlying filesystem
> is handling the acls and updating the mode. I haven't found any
> filesystem yet that fully supports xattrs but doesn't do something
> special with the posix acl xattrs.
>
> Can anyone suggest a good filesystem for me to test with?
>
> Thanks,
> Seth
>

I would suggest modifying fusexmp to detect the acl xattr names and rename them, like:

system.posix_acl_access -> user.system.posix_acl_access
system.posix_acl_default -> user.system.posix_acl_default

That way you preserve the xattr values without the underlying filesystem interpreting
the data as acls. This should emulate what you're trying to test.

Regards,
Michael Theall

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
On Thu, Jun 30, 2016 at 12:02:28PM -0500, Michael j Theall wrote:

> > Yes, I've already written pretty much the same code and am attempting to
> > test it.  The problem I'm having is finding a good filesystem to test
> > with. fusexmp works but is probably unfair as the underlying filesystem
> > is handling the acls and updating the mode. I haven't found any
> > filesystem yet that fully supports xattrs but doesn't do something
> > special with the posix acl xattrs.
> >
> > Can anyone suggest a good filesystem for me to test with?
> >
> > Thanks,
> > Seth
> >
>
> I would suggest modifying fusexmp to detect the acl xattr names and rename
> them, like:
>
> system.posix_acl_access -> user.system.posix_acl_access
> system.posix_acl_default -> user.system.posix_acl_default
>
> That way you preserve the xattr values without the underlying filesystem
> interpreting
> the data as acls. This should emulate what you're trying to test.

That should work if there's nothing else suitable.

In the meantime here's the updated patch I'm testing. I added a couple
more calls to forget_all_cached_acls, so review of that would be
appreciated.

I'm also a bit surprised no one has said anything about the behavior
when CONFIG_POSIX_ACL=n. I expected that to be controversial.

Thanks,
Seth

>From 93f69b5feb27382a8fab3d9f5e5cdca873e24387 Mon Sep 17 00:00:00 2001
From: Seth Forshee <[hidden email]>
Date: Wed, 29 Jun 2016 10:38:10 -0500
Subject: [PATCH] fuse: Add posix acl support

Change fuse over to using generic_getxattr() and
generic_setxattr() for handling xattrs so that it can use the
standard posix acl xattr handlers, falling back to the generic
handlers for everything else. Add get_acl() and set_acl()
callbacks which pass the xattrs through to the generic fuse
xattr handlers and update the cached acl values as needed.

Forget cached acls when inode attributes expired or refreshed,
and keep inode->i_mode and acls in sync.

When CONFIG_POSIX_ACL=n fuse will now return -EOPNOTSUPP for
posix acls, as the ids in the xattrs have not been mapped into
s_user_ns.

Signed-off-by: Seth Forshee <[hidden email]>
---
 fs/fuse/dir.c    | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 fs/fuse/fuse_i.h |   3 +
 fs/fuse/inode.c  |   3 +
 3 files changed, 186 insertions(+), 18 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 8466e122ee62..4fdea86322ad 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -13,6 +13,8 @@
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
+#include <linux/posix_acl_xattr.h>
 
 static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
 {
@@ -243,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);
@@ -915,6 +918,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;
@@ -1061,6 +1065,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);
 }
 
@@ -1230,6 +1235,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);
@@ -1697,14 +1703,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,
@@ -1719,9 +1730,8 @@ 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)
+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);
@@ -1755,8 +1765,8 @@ static int fuse_setxattr(struct dentry *unused, struct inode *inode,
  return err;
 }
 
-static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode,
-     const char *name, void *value, size_t size)
+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);
@@ -1838,9 +1848,8 @@ static ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
  return ret;
 }
 
-static int fuse_removexattr(struct dentry *entry, const char *name)
+static int fuse_removexattr(struct inode *inode, const char *name)
 {
- struct inode *inode = d_inode(entry);
  struct fuse_conn *fc = get_fuse_conn(inode);
  FUSE_ARGS(args);
  int err;
@@ -1865,6 +1874,153 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
  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,
+};
+
+#ifndef CONFIG_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;
+}
+
+static const struct xattr_handler fuse_xattr_acl_access_handler = {
+ .name = XATTR_NAME_POSIX_ACL_ACCESS,
+ .get = fuse_xattr_acl_get,
+ .set = fuse_xattr_acl_set,
+};
+
+static const struct xattr_handler fuse_xattr_acl_default_handler = {
+ .name = XATTR_NAME_POSIX_ACL_DEFAULT,
+ .get = fuse_xattr_acl_get,
+ .set = fuse_xattr_acl_set,
+};
+#endif /* CONFIG_POSIX_ACL */
+
+const struct xattr_handler *fuse_xattr_handlers[] = {
+#ifdef CONFIG_FS_POSIX_ACL
+ &posix_acl_access_xattr_handler,
+ &posix_acl_default_xattr_handler,
+#else
+ &fuse_xattr_acl_access_handler,
+ &fuse_xattr_acl_default_handler,
+#endif
+ &fuse_xattr_handler,
+};
+
+static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
+{
+ int size;
+ const char *name;
+ void *value = NULL;
+ struct posix_acl *acl;
+
+ 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(inode->i_sb->s_user_ns, value, size);
+ } else if ((size == 0) || (size == -ENODATA)) {
+ acl = NULL;
+ } else {
+ acl = ERR_PTR(size);
+ }
+ kfree(value);
+
+ return acl;
+}
+
+static int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+ const char *name;
+ int ret;
+
+ 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) {
+ struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
+ 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(s_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;
+}
+
 static const struct inode_operations fuse_dir_inode_operations = {
  .lookup = fuse_lookup,
  .mkdir = fuse_mkdir,
@@ -1879,10 +2035,12 @@ 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,
+ .get_acl = fuse_get_acl,
+ .set_acl = fuse_set_acl,
 };
 
 static const struct file_operations fuse_dir_operations = {
@@ -1900,10 +2058,12 @@ 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,
+ .get_acl = fuse_get_acl,
+ .set_acl = fuse_set_acl,
 };
 
 static const struct inode_operations fuse_symlink_inode_operations = {
@@ -1911,10 +2071,12 @@ 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,
+ .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 9f4c3c82edd6..02c08796051e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -23,6 +23,7 @@
 #include <linux/poll.h>
 #include <linux/workqueue.h>
 #include <linux/kref.h>
+#include <linux/xattr.h>
 #include <linux/pid_namespace.h>
 #include <linux/user_namespace.h>
 
@@ -693,6 +694,8 @@ extern const struct file_operations fuse_dev_operations;
 
 extern const struct dentry_operations fuse_dentry_operations;
 
+extern const struct xattr_handler *fuse_xattr_handlers[];
+
 /**
  * Inode to nodeid comparison.
  */
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 254f1944ee98..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)
@@ -1066,6 +1068,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;
--
2.7.4


------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Seth Forshee
On Thu, Jun 30, 2016 at 01:43:54PM -0500, Seth Forshee wrote:

> On Thu, Jun 30, 2016 at 12:02:28PM -0500, Michael j Theall wrote:
> > > Yes, I've already written pretty much the same code and am attempting to
> > > test it.  The problem I'm having is finding a good filesystem to test
> > > with. fusexmp works but is probably unfair as the underlying filesystem
> > > is handling the acls and updating the mode. I haven't found any
> > > filesystem yet that fully supports xattrs but doesn't do something
> > > special with the posix acl xattrs.
> > >
> > > Can anyone suggest a good filesystem for me to test with?
> > >
> > > Thanks,
> > > Seth
> > >
> >
> > I would suggest modifying fusexmp to detect the acl xattr names and rename
> > them, like:
> >
> > system.posix_acl_access -> user.system.posix_acl_access
> > system.posix_acl_default -> user.system.posix_acl_default
> >
> > That way you preserve the xattr values without the underlying filesystem
> > interpreting
> > the data as acls. This should emulate what you're trying to test.
>
> That should work if there's nothing else suitable.

I did something similar to this and acls seem to be working fine. Thanks
for the suggestion!

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

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

> Eric and I are working towards adding support for fuse mounts in
> non-init user namespaces. Towards that end we'd like to add ACL support
> to fuse as this will allow for a cleaner implementation overall. Below
> is an initial patch to support this. I'd like to get some general
> feedback on this patch and ask a couple of specific questions.
>
> There are some indications that fuse supports ACLs on the userspace side
> when default_permissions is not used (though I'm not seeing how that
> works). Will these changes conflict with that support, and if how do we
> avoid those conflicts?
>
I think as long as the kernel interprets ACLs only if default_permission
is used, you should be fine.

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

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Nikolaus Rath
In reply to this post by Eric W. Biederman
On Jun 29 2016, [hidden email] (Eric W. Biederman) wrote:

> "Michael j Theall" <[hidden email]> writes:
>
>> Going by the patch I posted a couple of years ago:
>> https://sourceforge.net/p/fuse/mailman/message/33033653/
>>
>> The only hole I see in your patch is that in setattr() you are not
>> updating the cached acl if the ATTR_MODE is updated. The other major
>> difference is that my version uses the get_acl/set_acl inode
>> operations but you use that plus the xattr handlers. I'm not
>> up-to-speed on the kernel so I'm not sure if you actually need to
>> implement both.
>
> That makes an interesting question.  Is it desirable to keep
> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> that supports posix acls worry about that?

A FUSE file system should be able to support ACLs without requiring the
file system process to do more than support extended attributes. I
believe this means that the kernel should keep i_mode and the ACLs in
sync -- it would be a rather bug prone and redundant for each FUSE file
system to implement its own parser for format in which the ACLs are
stored in xattrs.

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

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

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

> On Thu, Jun 30, 2016 at 11:25:32AM -0500, Eric W. Biederman wrote:
>> Seth Forshee <[hidden email]> writes:
>>
>> > On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
>> >> "Michael j Theall" <[hidden email]> writes:
>> >>
>> >> > Going by the patch I posted a couple of years ago:
>> >> > https://sourceforge.net/p/fuse/mailman/message/33033653/
>> >> >
>> >> > The only hole I see in your patch is that in setattr() you are not
>> >> > updating the cached acl if the ATTR_MODE is updated. The other major
>> >> > difference is that my version uses the get_acl/set_acl inode
>> >> > operations but you use that plus the xattr handlers. I'm not
>> >> > up-to-speed on the kernel so I'm not sure if you actually need to
>> >> > implement both.
>> >>
>> >> That makes an interesting question.  Is it desirable to keep
>> >> inode->i_mode in sync with the posix acls in fuse or should a filesystem
>> >> that supports posix acls worry about that?
>> >
>> > My first blush opinion is that the kernel should take care of this, not
>> > the filesystems. Then a fuse filesystem which supports xattrs gets acl
>> > support for free. Otherwise if a filesystem supports xattrs but not acls
>> > internally, we have no way of knowing that in the kernel and they get
>> > out of sync.
>> >
>> > However if some filesystems are already doing this internally then we
>> > have redundancy. Presumably this would be harmless aside from the wasted
>> > effort.
>>
>> Which means that in set_acl we need to something like:
>>
>> if (type == ACL_TYPE_ACCESS) {
>>         struct iattr attr;
>>                 attr.ia_valid = ATTR_MODE;
>>                 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 (attr.ia_mode != inode->i_mode) {
>> ret = fuse_do_setattr(inode, &attr, NULL);
>>                if (ret < 0)
>>                 return ret;
>>                 }
>>         }
>>
>> In fuse_setattr should wind up looking something like:
>>
>> 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)
>> ret = fuse_do_setattr(inode, attr, attr->ia_file);
>> else
>> ret = fuse_do_setattr(inode, attr, NULL);
>>
>> if (ret == 0 && attr->ia_valid & ATTR_MODE)
>>         ret = posix_acl_chmod(inode, inode->i_mode);
>> return ret;
>> }
>>
>> That should be enough to keep everything in sync with the existing
>> fuse protocol.  And then fuse filesystems won't have to care in general
>> about the contents of acls (unless they choose to care).
>
> Yes, I've already written pretty much the same code and am attempting to
> test it.  The problem I'm having is finding a good filesystem to test
> with. fusexmp works but is probably unfair as the underlying filesystem
> is handling the acls and updating the mode. I haven't found any
> filesystem yet that fully supports xattrs but doesn't do something
> special with the posix acl xattrs.
>
> Can anyone suggest a good filesystem for me to test with?
>
> Thanks,
> Seth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
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] fuse: Support posix ACLs

Nikolaus Rath
In reply to this post by Seth Forshee
On Jun 30 2016, Seth Forshee <[hidden email]> wrote:
> Yes, I've already written pretty much the same code and am attempting to
> test it.  The problem I'm having is finding a good filesystem to test
> with. fusexmp works but is probably unfair as the underlying filesystem
> is handling the acls and updating the mode. I haven't found any
> filesystem yet that fully supports xattrs but doesn't do something
> special with the posix acl xattrs.
>
> Can anyone suggest a good filesystem for me to test with?

S3QL should do it with a small patch - at the moment it deliberately
returns an error when the kernel tries to acces system.posix_acl*. This
is done specifically so that the kernel doesn't think that ACLs are
supported when they actually aren't (because of the missing
synchronization with the permission bits).

Download from https://bitbucket.org/nikratio/s3ql/ and comment out the
check in src/s3ql/fs.py:getxattr():

        # http://code.google.com/p/s3ql/issues/detail?id=385
        elif name in (b'system.posix_acl_access',
                      b'system.posix_acl_default'):
            raise FUSEError(ACL_ERRNO)



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

------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
12