On Wed, Jun 29, 2016 at 02:52:37PM -0500, Michael j Theall wrote:
>
> More comments:
>
> My patch does set_cached_acl() in fuse_get_acl(). I would investigate if
> this is necessary.
Hmm, on the one hand it looks like a lot of filesystems do call it, on
the other hand it looks like get_acl is caching it. I'll have to take a
closer look.
> Since your patch invalidates the cached acls on getattr(), I would
> investigate on whether you also need to invalidate cached acls on the
> entries returned by readdirplus.
Possibly so. I missed the fact that fuse_direntplus_link might update
the inode attributes.
> > There are some indications that fuse supports ACLs on the userspace side
> > when default_permissions is not used (though I'm not seeing how that
> > works). Will these changes conflict with that support, and if how do we
> > avoid those conflicts?
>
> I wouldn't exactly call it "support". Basically by turning off
> default_permissions, the fuser server can do what it wants permission-wise.
> My filesystem turns off default_permissions in order to support ACLs, but
> as far as fuse is concerned, it is totally oblivious and blindly does what
> my server says is permitted. That being said, there still lies the problem
> of checking permissions along the path. The low-level interface only gives
> you the final inode, which in the case of hard links, the path is
> ambiguous. This means you can't check each path component for permissions
> (and the kernel certainly won't! since you turned off default_permissions).
> Therefore the only "correct" way to implement ACL support on the fuse
> server is by setting the attr/entry timeout to 0 to force the lookup every
> time, which defeats many purposes of using fuse in the first place. You can
> read all the gory details in the thread I linked containing my original
> patch.
>
> Fortunately, this patch (and mine) allows you to use default_permissions
> *and* have acl support, meaning permission checks will be performed in
> kernel for each path component. The extra benefit is that it allows any
> backing filesystem which supports xattrs to support acls, provided your
> fuse server has unencumbered privileges for that filesystem.
But if we're going through the posix acl xattr handlers then the acls
will be cached. What I worry about is that somehow we could end up in a
situation where the cached acls don't match those that the filesystem is
using, and userspace sees one thing in response to getfacl but something
different is being enforced.
Thanks,
Seth
>
> Regards,
> Michael Theall
>
> Michael j Theall/Houston/IBM@IBMUS wrote on 06/29/2016 02:24:14 PM:
>
> > From: Michael j Theall/Houston/IBM@IBMUS
> > To: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> > Cc: fuse-devel@xxxxxxxxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx,
> > Miklos@xxxxxxxxxxxxxxxxxxxxxxxxxx, Szeredi <miklos@xxxxxxxxxx>
> > Date: 06/29/2016 02:37 PM
> > Subject: Re: [fuse-devel] [RFC] fuse: Support posix ACLs
> >
> > Going by the patch I posted a couple of years ago: https://
> > sourceforge.net/p/fuse/mailman/message/33033653/
> >
> > The only hole I see in your patch is that in setattr() you are not
> > updating the cached acl if the ATTR_MODE is updated. The other major
> > difference is that my version uses the get_acl/set_acl inode
> > operations but you use that plus the xattr handlers. I'm not up-to-
> > speed on the kernel so I'm not sure if you actually need to implement
> both.
> >
> > Regards,
> > Michael Theall
> >
> > Seth Forshee <seth.forshee@xxxxxxxxxxxxx> wrote on 06/29/2016 02:07:31
> PM:
> >
> > > From: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> > > To: fuse-devel@xxxxxxxxxxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
> > > Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>, Miklos Szeredi
> > > <miklos@xxxxxxxxxx>
> > > Date: 06/29/2016 02:08 PM
> > > Subject: [fuse-devel] [RFC] fuse: Support posix ACLs
> > >
> > > Eric and I are working towards adding support for fuse mounts in
> > > non-init user namespaces. Towards that end we'd like to add ACL support
> > > to fuse as this will allow for a cleaner implementation overall. Below
> > > is an initial patch to support this. I'd like to get some general
> > > feedback on this patch and ask a couple of specific questions.
> > >
> > > There are some indications that fuse supports ACLs on the userspace
> side
> > > when default_permissions is not used (though I'm not seeing how that
> > > works). Will these changes conflict with that support, and if how do we
> > > avoid those conflicts?
> > >
> > > Are there any places where we need to invalidate cached ACLs that
> aren't
> > > covered in the patch?
> > >
> > > Thanks,
> > > Seth
> > >
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 8466e122ee62..2f53b0491159 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -13,6 +13,8 @@
> > > #include <linux/sched.h>
> > > #include <linux/namei.h>
> > > #include <linux/slab.h>
> > > +#include <linux/xattr.h>
> > > +#include <linux/posix_acl_xattr.h>
> > >
> > > static bool fuse_use_readdirplus(struct inode *dir, struct
> > dir_context *ctx)
> > > {
> > > @@ -1061,6 +1063,7 @@ static int fuse_perm_getattr(struct inode
> > > *inode, int mask)
> > > if (mask & MAY_NOT_BLOCK)
> > > return -ECHILD;
> > >
> > > + forget_all_cached_acls(inode);
> > > return fuse_do_getattr(inode, NULL, NULL);
> > > }
> > >
> > > @@ -1719,9 +1722,8 @@ static int fuse_getattr(struct vfsmount *mnt,
> > > struct dentry *entry,
> > > return fuse_update_attributes(inode, stat, NULL, NULL);
> > > }
> > >
> > > -static int fuse_setxattr(struct dentry *unused, struct inode *inode,
> > > - const char *name, const void *value,
> > > - size_t size, int flags)
> > > +static int fuse_setxattr(struct inode *inode, const char *name,
> > > + const void *value, size_t size, int flags)
> > > {
> > > struct fuse_conn *fc = get_fuse_conn(inode);
> > > FUSE_ARGS(args);
> > > @@ -1755,8 +1757,8 @@ static int fuse_setxattr(struct dentry
> > > *unused, struct inode *inode,
> > > return err;
> > > }
> > >
> > > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode
> *inode,
> > > - const char *name, void *value, size_t size)
> > > +static ssize_t fuse_getxattr(struct inode *inode, const char *name,
> > > + void *value, size_t size)
> > > {
> > > struct fuse_conn *fc = get_fuse_conn(inode);
> > > FUSE_ARGS(args);
> > > @@ -1838,9 +1840,8 @@ static ssize_t fuse_listxattr(struct dentry
> > > *entry, char *list, size_t size)
> > > return ret;
> > > }
> > >
> > > -static int fuse_removexattr(struct dentry *entry, const char *name)
> > > +static int fuse_removexattr(struct inode *inode, const char *name)
> > > {
> > > - struct inode *inode = d_inode(entry);
> > > struct fuse_conn *fc = get_fuse_conn(inode);
> > > FUSE_ARGS(args);
> > > int err;
> > > @@ -1865,6 +1866,138 @@ static int fuse_removexattr(struct dentry
> > > *entry, const char *name)
> > > return err;
> > > }
> > >
> > > +static int fuse_xattr_get(const struct xattr_handler *handler,
> > > + struct dentry *dentry, struct inode *inode,
> > > + const char *name, void *value, size_t size)
> > > +{
> > > + return fuse_getxattr(inode, name, value, size);
> > > +}
> > > +
> > > +static int fuse_xattr_set(const struct xattr_handler *handler,
> > > + struct dentry *dentry, struct inode *inode,
> > > + const char *name, const void *value, size_t size,
> > > + int flags)
> > > +{
> > > + if (!value)
> > > + return fuse_removexattr(inode, name);
> > > +
> > > + return fuse_setxattr(inode, name, value, size, flags);
> > > +}
> > > +
> > > +static const struct xattr_handler fuse_xattr_handler = {
> > > + .prefix = "",
> > > + .get = fuse_xattr_get,
> > > + .set = fuse_xattr_set,
> > > +};
> > > +
> > > +#ifndef CONFIG_POSIX_ACL
> > > +static int fuse_xattr_acl_get(const struct xattr_handler *handler,
> > > + struct dentry *dentry, struct inode *inode,
> > > + const char *name, void *value, size_t size)
> > > +{
> > > + return -EOPNOTSUPP;
> > > +}
> > > +
> > > +static int fuse_xattr_acl_set(const struct xattr_handler *handler,
> > > + struct dentry *dentry, struct inode *inode,
> > > + const char *name, const void *value, size_t size,
> > > + int flags)
> > > +{
> > > + return -EOPNOTSUPP;
> > > +}
> > > +
> > > +static const struct xattr_handler fuse_xattr_acl_access_handler = {
> > > + .name = XATTR_NAME_POSIX_ACL_ACCESS,
> > > + .get = fuse_xattr_acl_get,
> > > + .set = fuse_xattr_acl_set,
> > > +};
> > > +
> > > +static const struct xattr_handler fuse_xattr_acl_default_handler = {
> > > + .name = XATTR_NAME_POSIX_ACL_DEFAULT,
> > > + .get = fuse_xattr_acl_get,
> > > + .set = fuse_xattr_acl_set,
> > > +};
> > > +#endif /* CONFIG_POSIX_ACL */
> > > +
> > > +const struct xattr_handler *fuse_xattr_handlers[] = {
> > > +#ifdef CONFIG_FS_POSIX_ACL
> > > + &posix_acl_access_xattr_handler,
> > > + &posix_acl_default_xattr_handler,
> > > +#else
> > > + &fuse_xattr_acl_access_handler,
> > > + &fuse_xattr_acl_default_handler,
> > > +#endif
> > > + &fuse_xattr_handler,
> > > +};
> > > +
> > > +static struct posix_acl *fuse_get_acl(struct inode *inode, int type)
> > > +{
> > > + int size;
> > > + const char *name;
> > > + void *value = NULL;
> > > + struct posix_acl *acl;
> > > +
> > > + if (type == ACL_TYPE_ACCESS)
> > > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > > + else if (type == ACL_TYPE_DEFAULT)
> > > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > > + else
> > > + return ERR_PTR(-EOPNOTSUPP);
> > > +
> > > + size = fuse_getxattr(inode, name, NULL, 0);
> > > + if (size > 0) {
> > > + value = kzalloc(size, GFP_KERNEL);
> > > + if (!value)
> > > + return ERR_PTR(-ENOMEM);
> > > + size = fuse_getxattr(inode, name, value, size);
> > > + }
> > > + if (size > 0) {
> > > + acl = posix_acl_from_xattr(inode->i_sb->s_user_ns, value, size);
> > > + } else if ((size == 0) || (size == -ENODATA)) {
> > > + acl = NULL;
> > > + } else {
> > > + acl = ERR_PTR(size);
> > > + }
> > > + kfree(value);
> > > +
> > > + return acl;
> > > +}
> > > +
> > > +static int fuse_set_acl(struct inode *inode, struct posix_acl
> > *acl,int type)
> > > +{
> > > + const char *name;
> > > + int ret;
> > > +
> > > + if (type == ACL_TYPE_ACCESS)
> > > + name = XATTR_NAME_POSIX_ACL_ACCESS;
> > > + else if (type == ACL_TYPE_DEFAULT)
> > > + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > > + else
> > > + return -EINVAL;
> > > +
> > > + if (acl) {
> > > + struct user_namespace *s_user_ns = inode->i_sb->s_user_ns;
> > > + size_t size = posix_acl_xattr_size(acl->a_count);
> > > + void *value = kmalloc(size, GFP_KERNEL);
> > > + if (!value)
> > > + return -ENOMEM;
> > > +
> > > + ret = posix_acl_to_xattr(s_user_ns, acl, value, size);
> > > + if (ret < 0) {
> > > + kfree(value);
> > > + return ret;
> > > + }
> > > +
> > > + ret = fuse_setxattr(inode, name, value, size, 0);
> > > + kfree(value);
> > > + } else {
> > > + ret = fuse_removexattr(inode, name);
> > > + }
> > > + if (ret == 0)
> > > + set_cached_acl(inode, type, acl);
> > > + return ret;
> > > +}
> > > +
> > > static const struct inode_operations fuse_dir_inode_operations = {
> > > .lookup = fuse_lookup,
> > > .mkdir = fuse_mkdir,
> > > @@ -1879,10 +2012,12 @@ static const struct inode_operations
> > > fuse_dir_inode_operations = {
> > > .mknod = fuse_mknod,
> > > .permission = fuse_permission,
> > > .getattr = fuse_getattr,
> > > - .setxattr = fuse_setxattr,
> > > - .getxattr = fuse_getxattr,
> > > + .setxattr = generic_setxattr,
> > > + .getxattr = generic_getxattr,
> > > .listxattr = fuse_listxattr,
> > > - .removexattr = fuse_removexattr,
> > > + .removexattr = generic_removexattr,
> > > + .get_acl = fuse_get_acl,
> > > + .set_acl = fuse_set_acl,
> > > };
> > >
> > > static const struct file_operations fuse_dir_operations = {
> > > @@ -1900,10 +2035,12 @@ static const struct inode_operations
> > > fuse_common_inode_operations = {
> > > .setattr = fuse_setattr,
> > > .permission = fuse_permission,
> > > .getattr = fuse_getattr,
> > > - .setxattr = fuse_setxattr,
> > > - .getxattr = fuse_getxattr,
> > > + .setxattr = generic_setxattr,
> > > + .getxattr = generic_getxattr,
> > > .listxattr = fuse_listxattr,
> > > - .removexattr = fuse_removexattr,
> > > + .removexattr = generic_removexattr,
> > > + .get_acl = fuse_get_acl,
> > > + .set_acl = fuse_set_acl,
> > > };
> > >
> > > static const struct inode_operations fuse_symlink_inode_operations = {
> > > @@ -1911,10 +2048,12 @@ static const struct inode_operations
> > > fuse_symlink_inode_operations = {
> > > .get_link = fuse_get_link,
> > > .readlink = generic_readlink,
> > > .getattr = fuse_getattr,
> > > - .setxattr = fuse_setxattr,
> > > - .getxattr = fuse_getxattr,
> > > + .setxattr = generic_setxattr,
> > > + .getxattr = generic_getxattr,
> > > .listxattr = fuse_listxattr,
> > > - .removexattr = fuse_removexattr,
> > > + .removexattr = generic_removexattr,
> > > + .get_acl = fuse_get_acl,
> > > + .set_acl = fuse_set_acl,
> > > };
> > >
> > > void fuse_init_common(struct inode *inode)
> > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > index 9f4c3c82edd6..02c08796051e 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -23,6 +23,7 @@
> > > #include <linux/poll.h>
> > > #include <linux/workqueue.h>
> > > #include <linux/kref.h>
> > > +#include <linux/xattr.h>
> > > #include <linux/pid_namespace.h>
> > > #include <linux/user_namespace.h>
> > >
> > > @@ -693,6 +694,8 @@ extern const struct file_operations
> fuse_dev_operations;
> > >
> > > extern const struct dentry_operations fuse_dentry_operations;
> > >
> > > +extern const struct xattr_handler *fuse_xattr_handlers[];
> > > +
> > > /**
> > > * Inode to nodeid comparison.
> > > */
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index 254f1944ee98..9c1519c269bb 100644
> > > --- a/fs/fuse/inode.c
> > > +++ b/fs/fuse/inode.c
> > > @@ -21,6 +21,7 @@
> > > #include <linux/sched.h>
> > > #include <linux/exportfs.h>
> > > #include <linux/pid_namespace.h>
> > > +#include <linux/posix_acl.h>
> > >
> > > MODULE_AUTHOR("Miklos Szeredi <miklos@xxxxxxxxxx>");
> > > MODULE_DESCRIPTION("Filesystem in Userspace");
> > > @@ -339,6 +340,7 @@ int fuse_reverse_inval_inode(struct super_block
> > > *sb, u64 nodeid,
> > > return -ENOENT;
> > >
> > > fuse_invalidate_attr(inode);
> > > + forget_all_cached_acls(inode);
> > > if (offset >= 0) {
> > > pg_start = offset >> PAGE_SHIFT;
> > > if (len <= 0)
> > > @@ -1066,6 +1068,7 @@ static int fuse_fill_super(struct super_block
> > > *sb, void *data, int silent)
> > > }
> > > sb->s_magic = FUSE_SUPER_MAGIC;
> > > sb->s_op = &fuse_super_operations;
> > > + sb->s_xattr = fuse_xattr_handlers;
> > > sb->s_maxbytes = MAX_LFS_FILESIZE;
> > > sb->s_time_gran = 1;
> > > sb->s_export_op = &fuse_export_operations;
> > >
> > >
> > >
> >
> ------------------------------------------------------------------------------
>
> > > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > > Francisco, CA to explore cutting-edge tech and listen to tech
> luminaries
> > > present their vision of the future. This family event has something for
> > > everyone, including kids. Get more information and register today.
> > > http://sdm.link/attshape
> > > --
> > > fuse-devel mailing list
> > > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > > lists/listinfo/fuse-devel
> > >
> >
> ------------------------------------------------------------------------------
>
> > Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> > Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> > present their vision of the future. This family event has something for
> > everyone, including kids. Get more information and register today.
> > http://sdm.link/attshape--
> > fuse-devel mailing list
> > To unsubscribe or subscribe, visit https://lists.sourceforge.net/
> > lists/listinfo/fuse-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
