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

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

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

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

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

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

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

> 
> > +
> > +struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > +{
> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> > +       int size;
> > +       const char *name;
> > +       void *value = NULL;
> > +       struct posix_acl *acl;
> > +
> > +       if (fc->no_getxattr)
> > +               return NULL;
> > +
> > +       if (type == ACL_TYPE_ACCESS)
> > +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +       else if (type == ACL_TYPE_DEFAULT)
> > +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +       else
> > +               return ERR_PTR(-EOPNOTSUPP);
> > +
> > +       size = fuse_getxattr(inode, name, NULL, 0);
> > +       if (size > 0) {
> > +               value = kzalloc(size, GFP_KERNEL);
> > +               if (!value)
> > +                       return ERR_PTR(-ENOMEM);
> > +               size = fuse_getxattr(inode, name, value, size);
> 
> Can we optimize away the first getxattr?  Starting with a page size
> buffer and falling back to the two calls only if ERANGE is returned.

Yeah, that's a good idea.

> > +       }
> > +       if (size > 0) {
> > +               acl = posix_acl_from_xattr(&init_user_ns, value, size);
> > +       } else if ((size == 0) || (size == -ENODATA) ||
> > +                  (size == -EOPNOTSUPP && fc->no_getxattr)) {
> > +               acl = NULL;
> > +       } else {
> > +               acl = ERR_PTR(size);
> > +       }
> > +       kfree(value);
> > +
> > +       return acl;
> > +}
> > +
> > +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > +       struct fuse_conn *fc = get_fuse_conn(inode);
> > +       const char *name;
> > +       int ret;
> > +
> > +       if (fc->no_setxattr)
> > +               return -EOPNOTSUPP;
> > +
> > +       if (type == ACL_TYPE_ACCESS) {
> > +               struct iattr attr;
> > +               name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +               attr.ia_mode = inode->i_mode;
> > +               ret = posix_acl_equiv_mode(acl, &attr.ia_mode);
> > +               if (ret < 0)
> > +                       return ret;
> > +               if (ret == 0)
> > +                       acl = NULL;
> > +               if (inode->i_mode != attr.ia_mode) {
> > +                       attr.ia_valid = ATTR_MODE | ATTR_CTIME;
> > +                       attr.ia_ctime = current_fs_time(inode->i_sb);
> > +                       ret = fuse_do_setattr(inode, &attr, NULL);
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +       } else if (type == ACL_TYPE_DEFAULT) {
> > +               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +       } else {
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (acl) {
> > +               size_t size = posix_acl_xattr_size(acl->a_count);
> > +               void *value = kmalloc(size, GFP_KERNEL);
> > +               if (!value)
> > +                       return -ENOMEM;
> > +
> > +               ret = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> > +               if (ret < 0) {
> > +                       kfree(value);
> > +                       return ret;
> > +               }
> > +
> > +               ret = fuse_setxattr(inode, name, value, size, 0);
> > +               kfree(value);
> > +       } else {
> > +               ret = fuse_removexattr(inode, name);
> > +       }
> > +       if (ret == 0)
> > +               set_cached_acl(inode, type, acl);
> > +       return ret;
> > +}
> 
> I wonder if splitting setxattr into a setattr + setxattr isn't going
> to cause problems.  By doing this userspace filesystem can no longer
> guarantee atomicity of the update.
> 
> Maybe we just need to introduce a new operation that can do chmod+setxattr?

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

> > +
> > +int fuse_init_acl(struct inode *inode, struct inode *dir)
> > +{
> > +       struct posix_acl *default_acl, *acl;
> > +       int err;
> > +
> > +       err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> > +       if (err)
> > +               return err;
> > +
> > +       if (default_acl) {
> > +               err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> > +               posix_acl_release(default_acl);
> > +       }
> > +       if (acl) {
> > +               if (!err)
> > +                       err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS);
> > +               posix_acl_release(default_acl);
> > +       }
> > +       return err;
> > +}
> > +
> > +#else /* !CONFIG_FUSE_FS_POSIX_ACL */
> > +
> > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> > +                             struct dentry *dentry, struct inode *inode,
> > +                             const char *name, void *value, size_t size)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> > +                               struct dentry *dentry, struct inode *inode,
> > +                               const char *name, const void *value, size_t size,
> > +                               int flags)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +#endif /* CONFIG_FUSE_FS_POSIX_ACL */
> > +
> > +const struct xattr_handler fuse_xattr_acl_access_handler = {
> > +       .name   = XATTR_NAME_POSIX_ACL_ACCESS,
> > +       .flags  = ACL_TYPE_ACCESS,
> > +       .get    = fuse_xattr_acl_get,
> > +       .set    = fuse_xattr_acl_set,
> > +};
> > +
> > +const struct xattr_handler fuse_xattr_acl_default_handler = {
> > +       .name   = XATTR_NAME_POSIX_ACL_DEFAULT,
> > +       .flags  = ACL_TYPE_DEFAULT,
> > +       .get    = fuse_xattr_acl_get,
> > +       .set    = fuse_xattr_acl_set,
> > +};
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 9df55b8e776a..ca7d573f3121 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/namei.h>
> >  #include <linux/slab.h>
> >  #include <linux/xattr.h>
> > +#include <linux/posix_acl.h>
> >
> >  static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx)
> >  {
> > @@ -244,6 +245,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
> >                 if (ret || (outarg.attr.mode ^ inode->i_mode) & S_IFMT)
> >                         goto invalid;
> >
> > +               forget_all_cached_acls(inode);
> >                 fuse_change_attributes(inode, &outarg.attr,
> >                                        entry_attr_timeout(&outarg),
> >                                        attr_version);
> > @@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args,
> >         }
> >         kfree(forget);
> >
> > +       if (args->in.h.opcode != FUSE_LINK) {
> > +               err = fuse_init_acl(inode, dir);
> 
> Again, atomicity problems.
> 
> > +               if (err) {
> > +                       iput(inode);
> > +                       return err;
> > +               }
> > +       }
> > +
> >         err = d_instantiate_no_diralias(entry, inode);
> >         if (err)
> >                 return err;
> > @@ -916,6 +926,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
> >
> >         if (time_before64(fi->i_time, get_jiffies_64())) {
> >                 r = true;
> > +               forget_all_cached_acls(inode);
> >                 err = fuse_do_getattr(inode, stat, file);
> >         } else {
> >                 r = false;
> > @@ -1062,6 +1073,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
> >         if (mask & MAY_NOT_BLOCK)
> >                 return -ECHILD;
> >
> > +       forget_all_cached_acls(inode);
> >         return fuse_do_getattr(inode, NULL, NULL);
> >  }
> >
> > @@ -1231,6 +1243,7 @@ retry:
> >                 fi->nlookup++;
> >                 spin_unlock(&fc->lock);
> >
> > +               forget_all_cached_acls(inode);
> >                 fuse_change_attributes(inode, &o->attr,
> >                                        entry_attr_timeout(o),
> >                                        attr_version);
> > @@ -1698,14 +1711,19 @@ error:
> >  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
> >  {
> >         struct inode *inode = d_inode(entry);
> > +       int ret;
> >
> >         if (!fuse_allow_current_process(get_fuse_conn(inode)))
> >                 return -EACCES;
> >
> >         if (attr->ia_valid & ATTR_FILE)
> > -               return fuse_do_setattr(inode, attr, attr->ia_file);
> > +               ret = fuse_do_setattr(inode, attr, attr->ia_file);
> >         else
> > -               return fuse_do_setattr(inode, attr, NULL);
> > +               ret = fuse_do_setattr(inode, attr, NULL);
> > +
> > +       if (!ret && (attr->ia_valid & ATTR_MODE))
> > +               ret = posix_acl_chmod(inode, inode->i_mode);
> 
> And again.
> 
> I'm really wondering if it's simpler to just add an xattr parser to
> libfuse and do these at the filesystem level.  That would simplify
> this patchset a lot:
> 
> Reduce the scope to just permission checking, which is what we can do
> best and fastest in the kernel.  And leave the rest to userspace.
> They don't have performance impact, but trying to push this into the
> kernel is just asking for trouble.

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

Thanks,
Seth