Re: [fuse-devel] [RFC] fuse: Support posix ACLs

[Seth Forshee <seth.forshee@canonical.com>]

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 <seth.forshee@canonical.com> wrote on 06/29/2016 02:07:31 PM:
> 
> > From: Seth Forshee <seth.forshee@canonical.com>
> > To: fuse-devel@lists.sourceforge.net, linux-fsdevel@vger.kernel.org
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>, Miklos Szeredi
> > <miklos@szeredi.hu>
> > 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 <miklos@szeredi.hu>");
> >  MODULE_DESCRIPTION("Filesystem in Userspace");
> > @@ -339,6 +340,7 @@ int fuse_reverse_inval_inode(struct super_block
> > *sb, u64 nodeid,
> >        return -ENOENT;
> >
> >     fuse_invalidate_attr(inode);
> > +   forget_all_cached_acls(inode);
> >     if (offset >= 0) {
> >        pg_start = offset >> PAGE_SHIFT;
> >        if (len <= 0)
> > @@ -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
> >