| Message ID | 1470086846-19844-3-git-send-email-seth.forshee@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote: > Seth Forshee <seth.forshee@canonical.com> writes: > > > On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote: > >> On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee > >> <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. > > Fuse is weird in dealing with the special xattrs. > > A typical filesystem will only support the special xattrs (aka > security.* system.*) when the underlying filesystem supports them. For > fuse that would be an INIT flag. > > The reason this comes up is all of this starts with allowing > unprivileged mounts of fuse filesystems. A fully unprivileged mount > implies your own private user namespace and your own private mount > namespace. > > As posix acls have embedded uids and gids their meaning depends on > which user namespace they are interpreted in. When a filesystem is > mounted outside of the initial user namespace this becomes interesting. > > The sanest approach I can see internal to the kernel is to require all > filesystems that care about posix acls to implement get_acl and set_acl > methods and not let the filesystems have access to the raw posix acl > data. That moves the communication from the vfs to the filesystem > into kuids and kgids (like all other uid and gid data). > > > For most filesystems that opt into receiving the posix acl xattrs only > when posix acls are implemented this is not a problem. For fuse this > is interesting. > > It is my intention to clean all of this up early November when I get > back from vacation. At which point the plan is to simply have > the vfs return a not supported error if passed or asked for a posix acl > xattr. > > My hope is that all of this is far enough along that fuse can do > something compatible in parallel such as return -EOPNOTSUPP or -ENOTSUP > when presented with posix acls xattrs if fuse posix acl support is not > enabled. > > Which leads me to this chunk of the code being reviewed: > > >> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c > >> > new file mode 100644 > >> > index 000000000000..c0f412dfe9a7 > >> > --- /dev/null > >> > +++ b/fs/fuse/acl.c > >> > @@ -0,0 +1,200 @@ > >> > +/* > >> > + FUSE: Filesystem in Userspace > >> > + Copyright (C) 2016 Canonical Ltd. <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. > > I don't quite understand this feedback, or the response. I can at least explain the response. What I mean is that we could use the generic posix acl xattr handlers and the get_acl/set_acl callbacks regardless of whether or not default_permissions is used. When it's not used we get the id translation even though the acls aren't being enforced. The cost to that though is pointless caching. I was also saying that you could theoretically have aliasing between the cache and the filesystem if changes were made in the filesystem outside of the kernel's knowledge, but I'm not convinced this is really a problem to be worried about. What still doesn't work well in that scenario is passing though the acl xattrs when CONFIG_FS_POSIX_ACL=n, as the ids would not get translated. But returning -EOPNOTSUPP (or whatever error) in this case seems reasonable to me. > I think what is desired here is: > > - In fuse_send_init to send a new flag FUSE_POSIX_ACL, > if the mount option "default_permissions" is set. ( Supporting posix > acls don't if fuse is not going to support them). > > - In process_init_reply test if FUSE_POSIX_ACL was returned > and set a boolean fc->posix_acl. > > - In the code above test fc->posix_acl instead of > FUSE_DEFAULT_PERMISSIONS. As having get_acl and set_acl succeeded > implies that that the filesystem implements posix acls. > > - Don't bother implementing a posix acl Kconfig option (after the review > comments there is so little code in the kernel it shouldn't matter). Without CONFIG_FS_POSIX_ACL=y we're missing definitions for some symbols, off the top of my head the posix acl xattr handlers but there may be others. That's one of those "hidden" options that's only enabled when some other option selects it. Using a Kconfig option for the filesystem like I did here is the usual way of handling it, and I don't see a good reason to make fuse different in this respect. > Which will result in the posix acl xattr not being transmitted to the > filesystem unless the filesystem has enabled posix acl support. > > That seems simple and an implementation that won't need to change when I > clean up the kernel posix acl support. > > Seth does that make sense to you? Yes, it makes sense. I was hoping we could do it without userspace modifications but I guess that isn't going to happen. What I'm not convinced of is that the userspace visible changes in behavior won't break someone's software, even if they aren't really getting acl enforcement. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote: > What I'm not convinced of is that the userspace visible changes in > behavior won't break someone's software, even if they aren't really > getting acl enforcement. That's a key point. Backward compatibility is important, and not even hard to do because fuse can negotiate supported features with the userspace filesystem. So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if "default_permissions" is on. If not set in INIT reply just pass all xattrs through to the filesystem. Caching should not be done. Don't think about whether it's logical or not, or if anyone could use it for anything sane. Just do what we are doing currently. Translating uids still makes sense, but that's another story. If the flag is set in INIT reply, then that means userspace filesystem wants handling of posix acl permission checking in kernel. It would also mean that caching of posix acl are allowed (lifetime linked to attribute lifetime). If filesystem wants to explicitly disable posix acl support, then it can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*". Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that filesystem can return in reply to FUSE_POSIX_ACL. I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it, just make any such code dependent on CONFIG_FS_POSIX_ACL. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote: > On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote: > > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote: > > What I'm not convinced of is that the userspace visible changes in > > behavior won't break someone's software, even if they aren't really > > getting acl enforcement. > > That's a key point. Backward compatibility is important, and not even > hard to do because fuse can negotiate supported features with the > userspace filesystem. > > So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if > "default_permissions" is on. > > If not set in INIT reply just pass all xattrs through to the > filesystem. Caching should not be done. Don't think about whether > it's logical or not, or if anyone could use it for anything sane. > Just do what we are doing currently. Translating uids still makes > sense, but that's another story. Translating uids is actually central to the differing positions that you and Eric have. What Eric wants is for the only path for supporting posix acls to be via the helpers, that way all concerns about translating uids can be addressed there. If fuse is to allow the xattrs to be passed directly through to the filesystem then there has to be a second mechanism which translates the uids for that case. > If the flag is set in INIT reply, then that means userspace filesystem > wants handling of posix acl permission checking in kernel. It would > also mean that caching of posix acl are allowed (lifetime linked to > attribute lifetime). > > If filesystem wants to explicitly disable posix acl support, then it > can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*". > Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that > filesystem can return in reply to FUSE_POSIX_ACL. > > I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it, > just make any such code dependent on CONFIG_FS_POSIX_ACL. But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't displayed in menuconfig, etc. If that's what you want, fine, but it seems like an unusual situation. Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Seth Forshee <seth.forshee@canonical.com> writes: > On Sat, Aug 06, 2016 at 11:09:54PM +0200, Miklos Szeredi wrote: >> On Sat, Aug 6, 2016 at 3:52 AM, Seth Forshee <seth.forshee@canonical.com> wrote: >> > On Fri, Aug 05, 2016 at 06:07:44PM -0500, Eric W. Biederman wrote: >> > What I'm not convinced of is that the userspace visible changes in >> > behavior won't break someone's software, even if they aren't really >> > getting acl enforcement. >> >> That's a key point. Backward compatibility is important, and not even >> hard to do because fuse can negotiate supported features with the >> userspace filesystem. >> >> So we can have a new FUSE_POSIX_ACL feature flag in INIT, sent if >> "default_permissions" is on. >> >> If not set in INIT reply just pass all xattrs through to the >> filesystem. Caching should not be done. Don't think about whether >> it's logical or not, or if anyone could use it for anything sane. >> Just do what we are doing currently. Translating uids still makes >> sense, but that's another story. > > Translating uids is actually central to the differing positions that you > and Eric have. What Eric wants is for the only path for supporting posix > acls to be via the helpers, that way all concerns about translating uids > can be addressed there. If fuse is to allow the xattrs to be passed > directly through to the filesystem then there has to be a second > mechanism which translates the uids for that case. I think I will agree with Miklos. Translating uids is another story and worst case we can work on that in September when I get back from vacation. Nothing of what you are talking about will make things worse for translating uids, so it is perfectly fine to merge posix acl support into fuse and then handle uid translation. Until we set FS_USER_NS fuse it is fine to not get the uid translation. Which yields a very simple implementation: In fuse_fill_super: if (!fc->posix_acl) { sb->s_xattr = fuse_xattr_handlers; } else { sb->s_xattr = fuse_acl_xattr_handlers; } Where fuse_acl_xattr_handlers are a different array that includes the posix acl interrcept (aka the array in patch 1 or the array in patch 2). Then fuse_get_acl and fuse_set_acl can just test fc->posix_acl and fail if that is not set. I think that is all you need to do, and we can worry about the other details after posix acl support has landed in fuse. >> If the flag is set in INIT reply, then that means userspace filesystem >> wants handling of posix acl permission checking in kernel. It would >> also mean that caching of posix acl are allowed (lifetime linked to >> attribute lifetime). >> >> If filesystem wants to explicitly disable posix acl support, then it >> can reply EOPNOTSUPP to getxattr and setxattr on "system.posix_acl_*". >> Alternatively we can add a FUSE_NO_POSIX_ACL feature flag, that >> filesystem can return in reply to FUSE_POSIX_ACL. >> >> I agree that adding CONFIG_FUSE_FS_POSIX_ACL is probably not worth it, >> just make any such code dependent on CONFIG_FS_POSIX_ACL. > > But CONFIG_FS_POSIX_ACL doesn't have an input prompt and thus isn't > displayed in menuconfig, etc. If that's what you want, fine, but it > seems like an unusual situation. Then in Kconfig I would have FUSE_FS "select FS_POSIX_ACL". That simplifies the problem. Unless Miklos wants something more granular. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Miklos, On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote: <snip> > 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. I've been playing with this over the past couple of days, and I wanted to get a little more feedback before I proceed. Things are pretty simple in the kernel if we just pass through the acl xattrs, but either the kernel or libfuse will need to work out the equivalent file mode when posix acls are written. I'm favoring libfuse for this, since it's very straightforward once you're already parsing the xattr and then we won't need to add a setattr+setxattr op. What we will need is to refresh the mode in the kernel from userspace. Right now after a successful setxattr we call fuse_invalidate_attr(), which should take care of that problem. I'm not sure the reasoning behind doing this still applies though. According to d331a415aef98717393dda0be69b7947da08eba3 it was added to force a refresh of ctime, but later in 31f3267b4ba16b12fb9dd3b1953ea0f221cc2ab4 fuse was changed to prefer ctime as maintained by the kernel, so it looks like that invalidate (and the one in removexattr, maybe others?) could be removed. If so, we could still keep it when setting posix acl xattrs, which would be the simplest option. Otherwise we need to get the mode back from userspace after the setxattr, either via a conditional outarg for setxattr or by adding a new operation. What's your preference for all of this? Thanks, Seth -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 16, 2016 at 10:59 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > Hi Miklos, > > On Thu, Aug 04, 2016 at 02:11:20PM +0200, Miklos Szeredi wrote: > > <snip> > >> 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. > > I've been playing with this over the past couple of days, and I wanted > to get a little more feedback before I proceed. > > Things are pretty simple in the kernel if we just pass through the acl > xattrs, but either the kernel or libfuse will need to work out the > equivalent file mode when posix acls are written. I'm favoring libfuse > for this, since it's very straightforward once you're already parsing > the xattr and then we won't need to add a setattr+setxattr op. What we > will need is to refresh the mode in the kernel from userspace. > > Right now after a successful setxattr we call fuse_invalidate_attr(), > which should take care of that problem. I'm not sure the reasoning > behind doing this still applies though. According to > d331a415aef98717393dda0be69b7947da08eba3 it was added to force a refresh > of ctime, but later in 31f3267b4ba16b12fb9dd3b1953ea0f221cc2ab4 fuse was > changed to prefer ctime as maintained by the kernel, so it looks like > that invalidate (and the one in removexattr, maybe others?) could be > removed. > > If so, we could still keep it when setting posix acl xattrs, which would > be the simplest option. Otherwise we need to get the mode back from > userspace after the setxattr, either via a conditional outarg for > setxattr or by adding a new operation. > > What's your preference for all of this? Lets just keep the invalidate. My philosophy is to not optimize until someone actually complains about performance. And these invalidates are unlikely to be a problem anyway. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +} + +struct posix_acl *fuse_get_acl(struct inode *inode, int type) +{ + struct fuse_conn *fc = get_fuse_conn(inode); + int size; + const char *name; + void *value = NULL; + struct posix_acl *acl; + + if (fc->no_getxattr) + return NULL; + + if (type == ACL_TYPE_ACCESS) + name = XATTR_NAME_POSIX_ACL_ACCESS; + else if (type == ACL_TYPE_DEFAULT) + name = XATTR_NAME_POSIX_ACL_DEFAULT; + else + return ERR_PTR(-EOPNOTSUPP); + + size = fuse_getxattr(inode, name, NULL, 0); + if (size > 0) { + value = kzalloc(size, GFP_KERNEL); + if (!value) + return ERR_PTR(-ENOMEM); + size = fuse_getxattr(inode, name, value, size); + } + if (size > 0) { + acl = posix_acl_from_xattr(&init_user_ns, value, size); + } else if ((size == 0) || (size == -ENODATA) || + (size == -EOPNOTSUPP && fc->no_getxattr)) { + acl = NULL; + } else { + acl = ERR_PTR(size); + } + kfree(value); + + return acl; +} + +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type) +{ + struct fuse_conn *fc = get_fuse_conn(inode); + const char *name; + int ret; + + if (fc->no_setxattr) + return -EOPNOTSUPP; + + if (type == ACL_TYPE_ACCESS) { + struct iattr attr; + name = XATTR_NAME_POSIX_ACL_ACCESS; + attr.ia_mode = inode->i_mode; + ret = posix_acl_equiv_mode(acl, &attr.ia_mode); + if (ret < 0) + return ret; + if (ret == 0) + acl = NULL; + if (inode->i_mode != attr.ia_mode) { + attr.ia_valid = ATTR_MODE | ATTR_CTIME; + attr.ia_ctime = current_fs_time(inode->i_sb); + ret = fuse_do_setattr(inode, &attr, NULL); + if (ret) + return ret; + } + } else if (type == ACL_TYPE_DEFAULT) { + name = XATTR_NAME_POSIX_ACL_DEFAULT; + } else { + return -EINVAL; + } + + if (acl) { + size_t size = posix_acl_xattr_size(acl->a_count); + void *value = kmalloc(size, GFP_KERNEL); + if (!value) + return -ENOMEM; + + ret = posix_acl_to_xattr(&init_user_ns, acl, value, size); + if (ret < 0) { + kfree(value); + return ret; + } + + ret = fuse_setxattr(inode, name, value, size, 0); + kfree(value); + } else { + ret = fuse_removexattr(inode, name); + } + if (ret == 0) + set_cached_acl(inode, type, acl); + return ret; +} + +int fuse_init_acl(struct inode *inode, struct inode *dir) +{ + struct posix_acl *default_acl, *acl; + int err; + + err = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl); + if (err) + return err; + + if (default_acl) { + err = fuse_set_acl(inode, default_acl, ACL_TYPE_DEFAULT); + posix_acl_release(default_acl); + } + if (acl) { + if (!err) + err = fuse_set_acl(inode, acl, ACL_TYPE_ACCESS); + posix_acl_release(default_acl); + } + return err; +} + +#else /* !CONFIG_FUSE_FS_POSIX_ACL */ + +static int fuse_xattr_acl_get(const struct xattr_handler *handler, + struct dentry *dentry, struct inode *inode, + const char *name, void *value, size_t size) +{ + return -EOPNOTSUPP; +} + +static int fuse_xattr_acl_set(const struct xattr_handler *handler, + struct dentry *dentry, struct inode *inode, + const char *name, const void *value, size_t size, + int flags) +{ + return -EOPNOTSUPP; +} + +#endif /* CONFIG_FUSE_FS_POSIX_ACL */ + +const struct xattr_handler fuse_xattr_acl_access_handler = { + .name = XATTR_NAME_POSIX_ACL_ACCESS, + .flags = ACL_TYPE_ACCESS, + .get = fuse_xattr_acl_get, + .set = fuse_xattr_acl_set, +}; + +const struct xattr_handler fuse_xattr_acl_default_handler = { + .name = XATTR_NAME_POSIX_ACL_DEFAULT, + .flags = ACL_TYPE_DEFAULT, + .get = fuse_xattr_acl_get, + .set = fuse_xattr_acl_set, +}; diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 9df55b8e776a..ca7d573f3121 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -14,6 +14,7 @@ #include <linux/namei.h> #include <linux/slab.h> #include <linux/xattr.h> +#include <linux/posix_acl.h> static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx) { @@ -244,6 +245,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags) if (ret || (outarg.attr.mode ^ inode->i_mode) & S_IFMT) goto invalid; + forget_all_cached_acls(inode); fuse_change_attributes(inode, &outarg.attr, entry_attr_timeout(&outarg), attr_version); @@ -554,6 +556,14 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_args *args, } kfree(forget); + if (args->in.h.opcode != FUSE_LINK) { + err = fuse_init_acl(inode, dir); + if (err) { + iput(inode); + return err; + } + } + err = d_instantiate_no_diralias(entry, inode); if (err) return err; @@ -916,6 +926,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat, if (time_before64(fi->i_time, get_jiffies_64())) { r = true; + forget_all_cached_acls(inode); err = fuse_do_getattr(inode, stat, file); } else { r = false; @@ -1062,6 +1073,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask) if (mask & MAY_NOT_BLOCK) return -ECHILD; + forget_all_cached_acls(inode); return fuse_do_getattr(inode, NULL, NULL); } @@ -1231,6 +1243,7 @@ retry: fi->nlookup++; spin_unlock(&fc->lock); + forget_all_cached_acls(inode); fuse_change_attributes(inode, &o->attr, entry_attr_timeout(o), attr_version); @@ -1698,14 +1711,19 @@ error: static int fuse_setattr(struct dentry *entry, struct iattr *attr) { struct inode *inode = d_inode(entry); + int ret; if (!fuse_allow_current_process(get_fuse_conn(inode))) return -EACCES; if (attr->ia_valid & ATTR_FILE) - return fuse_do_setattr(inode, attr, attr->ia_file); + ret = fuse_do_setattr(inode, attr, attr->ia_file); else - return fuse_do_setattr(inode, attr, NULL); + ret = fuse_do_setattr(inode, attr, NULL); + + if (!ret && (attr->ia_valid & ATTR_MODE)) + ret = posix_acl_chmod(inode, inode->i_mode); + return ret; } static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry, @@ -1738,6 +1756,8 @@ static const struct inode_operations fuse_dir_inode_operations = { .getxattr = generic_getxattr, .listxattr = fuse_listxattr, .removexattr = generic_removexattr, + .get_acl = fuse_get_acl, + .set_acl = fuse_set_acl, }; static const struct file_operations fuse_dir_operations = { @@ -1759,6 +1779,8 @@ static const struct inode_operations fuse_common_inode_operations = { .getxattr = generic_getxattr, .listxattr = fuse_listxattr, .removexattr = generic_removexattr, + .get_acl = fuse_get_acl, + .set_acl = fuse_set_acl, }; static const struct inode_operations fuse_symlink_inode_operations = { @@ -1770,6 +1792,8 @@ static const struct inode_operations fuse_symlink_inode_operations = { .getxattr = generic_getxattr, .listxattr = fuse_listxattr, .removexattr = generic_removexattr, + .get_acl = fuse_get_acl, + .set_acl = fuse_set_acl, }; void fuse_init_common(struct inode *inode) diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 93a5e8191da1..c1a630bb2b21 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -25,6 +25,7 @@ #include <linux/kref.h> #include <linux/pid_namespace.h> #include <linux/user_namespace.h> +#include <linux/xattr.h> /** Max number of pages that can be used in a single read request */ #define FUSE_MAX_PAGES_PER_REQ 32 @@ -966,7 +967,35 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr, void fuse_set_initialized(struct fuse_conn *fc); +int fuse_setxattr(struct inode *inode, const char *name, const void *value, + size_t size, int flags); +ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value, + size_t size); ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); +int fuse_removexattr(struct inode *inode, const char *name); extern const struct xattr_handler *fuse_xattr_handlers[]; +struct posix_acl; + +extern const struct xattr_handler fuse_xattr_acl_access_handler; +extern const struct xattr_handler fuse_xattr_acl_default_handler; + +#ifdef CONFIG_FUSE_FS_POSIX_ACL + +struct posix_acl *fuse_get_acl(struct inode *inode, int type); +int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type); +int fuse_init_acl(struct inode *inode, struct inode *dir); + +#else + +#define fuse_get_acl NULL +#define fuse_set_acl NULL + +static inline int fuse_init_acl(struct inode *inode, struct inode *dir) +{ + return 0; +} + +#endif + #endif /* _FS_FUSE_I_H */ diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index fee06e48157d..9c1519c269bb 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -21,6 +21,7 @@ #include <linux/sched.h> #include <linux/exportfs.h> #include <linux/pid_namespace.h> +#include <linux/posix_acl.h> MODULE_AUTHOR("Miklos Szeredi <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, };
Support posix acls in fuse only when CONFIG_FUSE_FS_POSIX_ACL=y and default_permissions is used for the filesystem. When default_permissions is not used fuse cannot meaninfully support cals, as fuse_permission() only sends FUSE_PERMISSION from the access, faccessat, chdir, fchdir, and chroot system calls. Therefore whenever CONFIG_FUSE_FS_POSIX_ACL=n or default_permissions is not used fuse will return -EOPNOTSUPP whenever posix acl xattrs are read or written. XXX: Default acls are currently broken for files created via atomic_open. Signed-off-by: Seth Forshee <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