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

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

On Thu, Jun 30, 2016 at 11:25:32AM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Wed, Jun 29, 2016 at 03:18:24PM -0500, Eric W. Biederman wrote:
> >> "Michael j Theall" <mtheall@us.ibm.com> writes:
> >> 
> >> > 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.
> >> 
> >> That makes an interesting question.  Is it desirable to keep
> >> inode->i_mode in sync with the posix acls in fuse or should a filesystem
> >> that supports posix acls worry about that?
> >
> > My first blush opinion is that the kernel should take care of this, not
> > the filesystems. Then a fuse filesystem which supports xattrs gets acl
> > support for free. Otherwise if a filesystem supports xattrs but not acls
> > internally, we have no way of knowing that in the kernel and they get
> > out of sync.
> >
> > However if some filesystems are already doing this internally then we
> > have redundancy. Presumably this would be harmless aside from the wasted
> > effort.
> 
> Which means that in set_acl we need to something like:
> 
> 	if (type == ACL_TYPE_ACCESS) {
>         	struct iattr attr;
>                 attr.ia_valid = ATTR_MODE;
>                 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 (attr.ia_mode != inode->i_mode) {
> 			ret = fuse_do_setattr(inode, &attr, NULL);
> 	                if (ret < 0)
>         	        	return ret;
>                 }
>         }
> 
> In fuse_setattr should wind up looking something like:
> 
> 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)
> 		ret = fuse_do_setattr(inode, attr, attr->ia_file);
> 	else
> 		ret = fuse_do_setattr(inode, attr, NULL);
> 
> 	if (ret == 0 && attr->ia_valid & ATTR_MODE)
>         	ret = posix_acl_chmod(inode, inode->i_mode);
> 	return ret;
> }
> 
> That should be enough to keep everything in sync with the existing
> fuse protocol.  And then fuse filesystems won't have to care in general
> about the contents of acls (unless they choose to care).

Yes, I've already written pretty much the same code and am attempting to
test it.  The problem I'm having is finding a good filesystem to test
with. fusexmp works but is probably unfair as the underlying filesystem
is handling the acls and updating the mode. I haven't found any
filesystem yet that fully supports xattrs but doesn't do something
special with the posix acl xattrs.

Can anyone suggest a good filesystem for me to test with?

Thanks,
Seth