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

[Miklos Szeredi <miklos@szeredi.hu>]

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

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

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

More comments inline.

Thanks,
Miklos

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

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

> +
> +struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> +{
> +       struct fuse_conn *fc = get_fuse_conn(inode);
> +       int size;
> +       const char *name;
> +       void *value = NULL;
> +       struct posix_acl *acl;
> +
> +       if (fc->no_getxattr)
> +               return NULL;
> +
> +       if (type == ACL_TYPE_ACCESS)
> +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> +       else if (type == ACL_TYPE_DEFAULT)
> +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +       else
> +               return ERR_PTR(-EOPNOTSUPP);
> +
> +       size = fuse_getxattr(inode, name, NULL, 0);
> +       if (size > 0) {
> +               value = kzalloc(size, GFP_KERNEL);
> +               if (!value)
> +                       return ERR_PTR(-ENOMEM);
> +               size = fuse_getxattr(inode, name, value, size);

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

> +       }
> +       if (size > 0) {
> +               acl = posix_acl_from_xattr(&init_user_ns, value, size);
> +       } else if ((size == 0) || (size == -ENODATA) ||
> +                  (size == -EOPNOTSUPP && fc->no_getxattr)) {
> +               acl = NULL;
> +       } else {
> +               acl = ERR_PTR(size);
> +       }
> +       kfree(value);
> +
> +       return acl;
> +}
> +
> +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> +       struct fuse_conn *fc = get_fuse_conn(inode);
> +       const char *name;
> +       int ret;
> +
> +       if (fc->no_setxattr)
> +               return -EOPNOTSUPP;
> +
> +       if (type == ACL_TYPE_ACCESS) {
> +               struct iattr attr;
> +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> +               attr.ia_mode = inode->i_mode;
> +               ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
> +               if (ret < 0)
> +                       return ret;
> +               if (ret == 0)
> +                       acl = NULL;
> +               if (inode->i_mode != attr.ia_mode) {
> +                       attr.ia_valid = ATTR_MODE | ATTR_CTIME;
> +                       attr.ia_ctime = current_fs_time(inode->i_sb);
> +                       ret = fuse_do_setattr(inode, &attr, NULL);
> +                       if (ret)
> +                               return ret;
> +               }
> +       } else if (type == ACL_TYPE_DEFAULT) {
> +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       if (acl) {
> +               size_t size = posix_acl_xattr_size(acl->a_count);
> +               void *value = kmalloc(size, GFP_KERNEL);
> +               if (!value)
> +                       return -ENOMEM;
> +
> +               ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> +               if (ret < 0) {
> +                       kfree(value);
> +                       return ret;
> +               }
> +
> +               ret = fuse_setxattr(inode, name, value, size, 0);
> +               kfree(value);
> +       } else {
> +               ret = fuse_removexattr(inode, name);
> +       }
> +       if (ret == 0)
> +               set_cached_acl(inode, type, acl);
> +       return ret;
> +}

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

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

> +
> +int fuse_init_acl(struct inode *inode, struct inode *dir)
> +{
> +       struct posix_acl *default_acl, *acl;
> +       int err;
> +
> +       err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +       if (err)
> +               return err;
> +
> +       if (default_acl) {
> +               err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +               posix_acl_release(default_acl);
> +       }
> +       if (acl) {
> +               if (!err)
> +                       err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +               posix_acl_release(default_acl);
> +       }
> +       return err;
> +}
> +
> +#else /* !CONFIG_FUSE_FS_POSIX_ACL */
> +
> +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> +                             struct dentry *dentry, struct inode *inode,
> +                             const char *name, void *value, size_t size)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> +                               struct dentry *dentry, struct inode *inode,
> +                               const char *name, const void *value, size_t size,
> +                               int flags)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +#endif /* CONFIG_FUSE_FS_POSIX_ACL */
> +
> +const struct xattr_handler fuse_xattr_acl_access_handler = {
> +       .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> +       .flags  = ACL_TYPE_ACCESS,
> +       .get    = fuse_xattr_acl_get,
> +       .set    = fuse_xattr_acl_set,
> +};
> +
> +const struct xattr_handler fuse_xattr_acl_default_handler = {
> +       .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> +       .flags  = ACL_TYPE_DEFAULT,
> +       .get    = fuse_xattr_acl_get,
> +       .set    = fuse_xattr_acl_set,
> +};
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 9df55b8e776a..ca7d573f3121 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -14,6 +14,7 @@
>  #include <linux/namei.h>
>  #include <linux/slab.h>
>  #include <linux/xattr.h>
> +#include <linux/posix_acl.h>
>
>  static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
>  {
> @@ -244,6 +245,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
>                 if (ret || (outarg.attr.mode ^ inode->i_mode) & S_IFMT)
>                         goto invalid;
>
> +               forget_all_cached_acls(inode);
>                 fuse_change_attributes(inode, &outarg.attr,
>                                        entry_attr_timeout(&outarg),
>                                        attr_version);
> @@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args,
>         }
>         kfree(forget);
>
> +       if (args->in.h.opcode != FUSE_LINK) {
> +               err = fuse_init_acl(inode, dir);

Again, atomicity problems.

> +               if (err) {
> +                       iput(inode);
> +                       return err;
> +               }
> +       }
> +
>         err = d_instantiate_no_diralias(entry, inode);
>         if (err)
>                 return err;
> @@ -916,6 +926,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
>
>         if (time_before64(fi->i_time, get_jiffies_64())) {
>                 r = true;
> +               forget_all_cached_acls(inode);
>                 err = fuse_do_getattr(inode, stat, file);
>         } else {
>                 r = false;
> @@ -1062,6 +1073,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
>         if (mask & MAY_NOT_BLOCK)
>                 return -ECHILD;
>
> +       forget_all_cached_acls(inode);
>         return fuse_do_getattr(inode, NULL, NULL);
>  }
>
> @@ -1231,6 +1243,7 @@ retry:
>                 fi->nlookup++;
>                 spin_unlock(&fc->lock);
>
> +               forget_all_cached_acls(inode);
>                 fuse_change_attributes(inode, &o->attr,
>                                        entry_attr_timeout(o),
>                                        attr_version);
> @@ -1698,14 +1711,19 @@ error:
>  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
>  {
>         struct inode *inode = d_inode(entry);
> +       int ret;
>
>         if (!fuse_allow_current_process(get_fuse_conn(inode)))
>                 return -EACCES;
>
>         if (attr->ia_valid & ATTR_FILE)
> -               return fuse_do_setattr(inode, attr, attr->ia_file);
> +               ret = fuse_do_setattr(inode, attr, attr->ia_file);
>         else
> -               return fuse_do_setattr(inode, attr, NULL);
> +               ret = fuse_do_setattr(inode, attr, NULL);
> +
> +       if (!ret && (attr->ia_valid & ATTR_MODE))
> +               ret = posix_acl_chmod(inode, inode->i_mode);

And again.

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

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

> +       return ret;
>  }
>
>  static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
> @@ -1738,6 +1756,8 @@ static const struct inode_operations fuse_dir_inode_operations = {
>         .getxattr       = generic_getxattr,
>         .listxattr      = fuse_listxattr,
>         .removexattr    = generic_removexattr,
> +       .get_acl        = fuse_get_acl,
> +       .set_acl        = fuse_set_acl,
>  };
>
>  static const struct file_operations fuse_dir_operations = {
> @@ -1759,6 +1779,8 @@ static const struct inode_operations fuse_common_inode_operations = {
>         .getxattr       = generic_getxattr,
>         .listxattr      = fuse_listxattr,
>         .removexattr    = generic_removexattr,
> +       .get_acl        = fuse_get_acl,
> +       .set_acl        = fuse_set_acl,
>  };
>
>  static const struct inode_operations fuse_symlink_inode_operations = {
> @@ -1770,6 +1792,8 @@ static const struct inode_operations fuse_symlink_inode_operations = {
>         .getxattr       = generic_getxattr,
>         .listxattr      = fuse_listxattr,
>         .removexattr    = generic_removexattr,
> +       .get_acl        = fuse_get_acl,
> +       .set_acl        = fuse_set_acl,
>  };
>
>  void fuse_init_common(struct inode *inode)
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 93a5e8191da1..c1a630bb2b21 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -25,6 +25,7 @@
>  #include <linux/kref.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/user_namespace.h>
> +#include <linux/xattr.h>
>
>  /** Max number of pages that can be used in a single read request */
>  #define FUSE_MAX_PAGES_PER_REQ 32
> @@ -966,7 +967,35 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>
>  void fuse_set_initialized(struct fuse_conn *fc);
>
> +int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> +                 size_t size, int flags);
> +ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
> +                     size_t size);
>  ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size);
> +int fuse_removexattr(struct inode *inode, const char *name);
>  extern const struct xattr_handler *fuse_xattr_handlers[];
>
> +struct posix_acl;
> +
> +extern const struct xattr_handler fuse_xattr_acl_access_handler;
> +extern const struct xattr_handler fuse_xattr_acl_default_handler;
> +
> +#ifdef CONFIG_FUSE_FS_POSIX_ACL
> +
> +struct posix_acl *fuse_get_acl(struct inode *inode, int type);
> +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> +int fuse_init_acl(struct inode *inode, struct inode *dir);
> +
> +#else
> +
> +#define fuse_get_acl NULL
> +#define fuse_set_acl NULL
> +
> +static inline int fuse_init_acl(struct inode *inode, struct inode *dir)
> +{
> +       return 0;
> +}
> +
> +#endif
> +
>  #endif /* _FS_FUSE_I_H */
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index fee06e48157d..9c1519c269bb 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -21,6 +21,7 @@
>  #include <linux/sched.h>
>  #include <linux/exportfs.h>
>  #include <linux/pid_namespace.h>
> +#include <linux/posix_acl.h>
>
>  MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
>  MODULE_DESCRIPTION("Filesystem in Userspace");
> @@ -339,6 +340,7 @@ int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
>                 return -ENOENT;
>
>         fuse_invalidate_attr(inode);
> +       forget_all_cached_acls(inode);
>         if (offset >= 0) {
>                 pg_start = offset >> PAGE_SHIFT;
>                 if (len <= 0)
> diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c
> index d30e99610217..d87d58112eb1 100644
> --- a/fs/fuse/xattr.c
> +++ b/fs/fuse/xattr.c
> @@ -9,9 +9,10 @@
>  #include "fuse_i.h"
>
>  #include <linux/xattr.h>
> +#include <linux/posix_acl_xattr.h>
>
> -static int fuse_setxattr(struct inode *inode, const char *name,
> -                        const void *value, size_t size, int flags)
> +int fuse_setxattr(struct inode *inode, const char *name, const void *value,
> +                 size_t size, int flags)
>  {
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         FUSE_ARGS(args);
> @@ -45,8 +46,8 @@ static int fuse_setxattr(struct inode *inode, const char *name,
>         return err;
>  }
>
> -static ssize_t fuse_getxattr(struct inode *inode, const char *name,
> -                            void *value, size_t size)
> +ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value,
> +                     size_t size)
>  {
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         FUSE_ARGS(args);
> @@ -128,7 +129,7 @@ ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size)
>         return ret;
>  }
>
> -static int fuse_removexattr(struct inode *inode, const char *name)
> +int fuse_removexattr(struct inode *inode, const char *name)
>  {
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         FUSE_ARGS(args);
> @@ -179,5 +180,7 @@ static const struct xattr_handler fuse_xattr_handler = {
>  };
>
>  const struct xattr_handler *fuse_xattr_handlers[] = {
> +       &fuse_xattr_acl_access_handler,
> +       &fuse_xattr_acl_default_handler,
>         &fuse_xattr_handler,
>  };
> --
> 2.7.4
>